Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650112 > unrolled thread

Re: [PATCH 6/7] RISC-V: arch/riscv/kernel

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 6/7] RISC-V: arch/riscv/kernel Palmer Dabbelt <palmer@dabbelt.com> - 2017-05-25 04:00 +0200
    Re: [PATCH 6/7] RISC-V: arch/riscv/kernel Arnd Bergmann <arnd@arndb.de> - 2017-05-25 22:00 +0200
      Re: [PATCH 6/7] RISC-V: arch/riscv/kernel Palmer Dabbelt <palmer@dabbelt.com> - 2017-06-06 07:00 +0200
        Re: [PATCH 6/7] RISC-V: arch/riscv/kernel Arnd Bergmann <arnd@arndb.de> - 2017-06-06 11:10 +0200
          Re: [PATCH 6/7] RISC-V: arch/riscv/kernel Palmer Dabbelt <palmer@dabbelt.com> - 2017-06-06 22:40 +0200

#1650112 — Re: [PATCH 6/7] RISC-V: arch/riscv/kernel

On Mon, 22 May 2017 19:11:35 PDT (-0700), olof@lixom.net wrote:
> On Mon, May 22, 2017 at 5:41 PM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> What's missing from this patchset (ideally) is a good writeup under
> DOcumentation/ on expectations of system state (and/or configuration)
> upon entry of the kernel. For comparison, see the arm64 documentation
> where they were quite specific in this.

We don't have a spec written for this yet, but one is being written.  Is it OK
if I wait until there's a spec?

> This patch is also pushing size limits, and is getting unwieldy to
> comment on. I'll point out a few things below with plenty of snipped
> out lines.

I'm also going to start dropping diffs, as this is very big.

>> +++ b/arch/riscv/kernel/asm-offsets.c
>> @@ -0,0 +1,113 @@
>> +/*
>> + * Copyright (C) 2012 Regents of the University of California
>> + *
>> + *   This program is free software; you can redistribute it and/or
>> + *   modify it under the terms of the GNU General Public License
>> + *   as published by the Free Software Foundation, version 2.
>> + *
>> + *   This program is distributed in the hope that it will be useful, but
>> + *   WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
>> + *   NON INFRINGEMENT.  See the GNU General Public License for
>> + *   more details.
>
> Hmm, I haven't seen these terms used often, but they seem to exist
> around the tree in a few places. arch/tile is littered with them.
>
> I am not a lawyer, but I can't seem any reference to "good title" in
> the GPLv2 text.
>
> Rather than having to go through the process of figuring out if this
> license header is acceptable or not, you might find it easier to just
> go with something more established.

This almost certainly came from Tilera: I stole our ptrace from there becuase
that was the ISA I understood best, and that header must have proliferated
everywhere else.

I've changed it to a header I copied from ARM

  https://github.com/riscv/riscv-linux/commit/ccc4f51b40b28adf01b14ed6578bf26dc02f1425

>> +static int __populate_cache_leaves(unsigned int cpu)
>> +{
>> +       struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>> +       struct cacheinfo *this_leaf = this_cpu_ci->info_list;
>> +       struct device_node *np = of_cpu_device_node_get(cpu);
>> +       int levels = 1, level = 1;
>> +
>> +       if (of_property_read_bool(np, "cache-size"))   ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
>> +       if (of_property_read_bool(np, "i-cache-size")) ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
>> +       if (of_property_read_bool(np, "d-cache-size")) ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
>
> Please run checkpatch, kernel coding style doesn't use one-line ifs
> (here nor elsewhere).

I went through and fixed many of the checkpatch messages.  The ones that are
left fall into the following categories:

 * Lots of uses of BUG/BUG_ON instead of WARN_ON.  Lots of these are in boot
   code, but some of them can probably be fixed.

 * Parens around single-statement __asm__ macros.  For these I also get a
   message when they're wrapped in "do {} while (0)", so I'm not sure what else
   to do.

 * Parens around macros like "#define RISCV_PTR .dword".  These can't have
   parens because they go directly to the assembler, so I'm considering this a
   false-positive.

 * Warnings about volatile in function declarations in bitops.h.  These are
   copied from other architectures.  There were a handful of other volatiles
   that I fixed,, but I think these should stay.

 * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also present in
   other architectures.

 * We added new typedefs, I can remove these if that's a problem.  They're
   there to match our other code (bootloader and simulator).

 * A handful of lines over 80 characters that I think are onerous to break any
   more.

 * Some warnings about printk() not having a KERN_ prefix.  I fixed a handful
   of these, but the remaining ones I don't know how to fix (in show_regs, for
   example, where arm64 also has them).

 * Extern declarations in C files, all of which link to symbols in assembly or
   linker scripts.  These were copied from other architectures.

There's also a bunch of false positives:

 * The spelling of SEPC, which is correct (Supervisor Exception Program
   Counter).

 * Fall-through warnings, probably getting confused by the break looking like
   "break; \" (they're in macros).

I'll make another pass on these before a v2 patch set.

>> +/* Return -1 if not a valid hart */
>> +int riscv_of_processor_hart(struct device_node *node)
>> +{
>> +       const char *isa, *status;
>> +       u32 hart;
>> +
>> +       if (!of_device_is_compatible(node, "riscv")) return -1;
>> +       if (of_property_read_u32(node, "reg", &hart) || hart >= NR_CPUS) return -1;
>> +       if (of_property_read_string(node, "status", &status) || strcmp(status, "okay")) return -1;
>> +       if (of_property_read_string(node, "riscv,isa", &isa) || isa[0] != 'r' || isa[1] != 'v') return -1;
>> +
>> +       return hart;
>> +}
>
> We usually prefer to see real -E<foo> returns instead of -1 in the kernel.

Makes sense.  https://github.com/riscv/riscv-linux/commit/10ef72b2aa16b2b69f9f349cffc06d12e183a56e

>> +asmlinkage void __irq_entry do_IRQ(unsigned int cause, struct pt_regs *regs)
>> +{
>> +       struct pt_regs *old_regs = set_irq_regs(regs);
>> +       irq_enter();
>> +
>> +       /* There are three classes of interrupt: timer, software, and
>> +          external devices.  We dispatch between them here.  External
>> +          device interrupts use the generic IRQ mechanisms. */
>> +       switch (cause) {
>> +               case INTERRUPT_CAUSE_TIMER:
>> +                       riscv_timer_interrupt();
>> +                       break;
>> +               case INTERRUPT_CAUSE_SOFTWARE:
>> +                       riscv_software_interrupt();
>> +                       break;
>> +               default: {
>> +                       struct irq_domain *domain = per_cpu(riscv_irq_data, smp_processor_id()).domain;
>
> Move this up to top of function and remove the { } wrap, please.

https://github.com/riscv/riscv-linux/commit/17879136caa05ed9f686736b3343f4b2063920ab

>> +static void riscv_irq_enable(struct irq_data *d)
>> +{
>> +       struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
>> +       atomic_long_or((1 << (long)d->hwirq), &per_cpu(riscv_early_sie, data->hart));
>
> This is a bit dense to get into without a few words of how it's
> expected to work.

OK, how does this look? https://github.com/riscv/riscv-linux/commit/112fd2d882c2363508a660061da558d772a4ff0b

>> +static int riscv_intc_init(struct device_node *node, struct device_node *parent)
>> +{
>> +       int hart;
>> +
>> +       if (parent) return 0; // should have no interrupt parent
>> +
>> +       if ((hart = riscv_of_processor_hart(node->parent)) >= 0) {
>
> Common pattern in kernel is to detect error instead:
>     hart = riscv_of_processor_hart(node->parent);
>     if (hart < 0) {
>         <from your else side here>
>         return 0;
>     }
>     <body if your if statement here>
>
>     return 0;

OK.  I've fixed this one here

  https://github.com/riscv/riscv-linux/commit/5f48d9ba0d3cd19dc0bf95f66370de4cfcd84cca

I'll try to remember to fix any others that I come across.

>> diff --git a/arch/riscv/kernel/pci.c b/arch/riscv/kernel/pci.c
>> new file mode 100644
>> index 000000000000..4191a5ffdd67
>> --- /dev/null
>> +++ b/arch/riscv/kernel/pci.c
>> @@ -0,0 +1,36 @@
>> +/*
>> + * Code borrowed from arch/arm64/kernel/pci.c
>
> So, you should add recursive reference from there (i.e. powerpc).
>
> But in the end, there's essentially no code in this file. :)

Well, now there's more copyright notices than lines of code... :)

  https://github.com/riscv/riscv-linux/commit/5ad312f755935319fdbb6739377b400ea81cd2ec

>> +#define MAX_DEVICES    1024 // 0 is reserved
>
> Seems like an odd comment to have here (and should probably not go at
> the end of the line)

Device 0 in the PLIC is reserved to mean "no device", which means "MAX_DEVICES"
is a bit of an odd name (there can only be MAX_DEVICES-1 devices).  I've added
a larger comment to describe this better.

  https://github.com/riscv/riscv-linux/commit/9d16413051dd86db0fb8a792f3d5f05ce788d145

>> +#define PLIC_HART_CONTEXT(data, i)     (struct plic_hart_context *)((char*)data->reg + HART_BASE + HART_SIZE*i)
>> +#define PLIC_ENABLE_CONTEXT(data, i)   (struct plic_enable_context *)((char*)data->reg + ENABLE_BASE + ENABLE_SIZE*i)
>> +#define PLIC_PRIORITY(data)            (struct plic_priority *)((char *)data->reg + PRIORITY_BASE)
>
> Since you have typecasting and stuff here, small static inlines with
> appropriate return types seems slightly tidier.

OK.  https://github.com/riscv/riscv-linux/commit/c416649b203276d944be595d87caf042c998727f

>> +static void plic_chained_handle_irq(struct irq_desc *desc)
>> +{
>> +        struct plic_handler *handler = irq_desc_get_handler_data(desc);
>> +       struct irq_chip *chip = irq_desc_get_chip(desc);
>
> Whitespace.

These were fixed along with the other checkpatch messages.


> [wrapping up review of this patch at this point to keep size down]
>
>
> -Olof

Thanks for the comments!  I'll batch these up into a v2 when I'm done with
everyone's comments from this round.

[toc] | [next] | [standalone]

#1650780

On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> On Mon, 22 May 2017 19:11:35 PDT (-0700), olof@lixom.net wrote:

>  * Parens around single-statement __asm__ macros.  For these I also get a
>    message when they're wrapped in "do {} while (0)", so I'm not sure what else
>    to do.

I would generally recommend using inline functions for those, and only do
macros when you need them.

>  * Parens around macros like "#define RISCV_PTR .dword".  These can't have
>    parens because they go directly to the assembler, so I'm considering this a
>    false-positive.

agreed

>  * Warnings about volatile in function declarations in bitops.h.  These are
>    copied from other architectures.  There were a handful of other volatiles
>    that I fixed,, but I think these should stay.

Agreed, bitops.h is one of the few headers that should use 'volatile'.

>  * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also present in
>    other architectures.

What is the warning here? I would assume that you should leave this
unchanged as well.

>  * We added new typedefs, I can remove these if that's a problem.  They're
>    there to match our other code (bootloader and simulator).

It depends. What typedefs are those? Removing the typedefs in both
the kernel and the other code that uses the same types is likely the
best option here.

>  * A handful of lines over 80 characters that I think are onerous to break any
>    more.

Right, don't worry about it too much, and use common sense for this
warning.

>  * Some warnings about printk() not having a KERN_ prefix.  I fixed a handful
>    of these, but the remaining ones I don't know how to fix (in show_regs, for
>    example, where arm64 also has them).

KERN_CONT

>  * Extern declarations in C files, all of which link to symbols in assembly or
>    linker scripts.  These were copied from other architectures.

I would try to fix those by using a header even if there is only one user.
I'd actually like to get a compile-time warning for those in the long run,
maybe with 'make W=1', so better don't introduce new ones.

     Arnd

[toc] | [prev] | [next] | [standalone]

#1658350

On Thu, 25 May 2017 12:51:54 PDT (-0700), Arnd Bergmann wrote:
> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> On Mon, 22 May 2017 19:11:35 PDT (-0700), olof@lixom.net wrote:
>
>>  * Parens around single-statement __asm__ macros.  For these I also get a
>>    message when they're wrapped in "do {} while (0)", so I'm not sure what else
>>    to do.
>
> I would generally recommend using inline functions for those, and only do
> macros when you need them.

We've tried to avoid new macros, so these are mostly from places where other
architectures use macros (like mb) or where we need to use CPP token pasting
(like __op_bit).  Should I change things like mb?

>>  * Parens around macros like "#define RISCV_PTR .dword".  These can't have
>>    parens because they go directly to the assembler, so I'm considering this a
>>    false-positive.
>
> agreed
>
>>  * Warnings about volatile in function declarations in bitops.h.  These are
>>    copied from other architectures.  There were a handful of other volatiles
>>    that I fixed,, but I think these should stay.
>
> Agreed, bitops.h is one of the few headers that should use 'volatile'.
>
>>  * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also present in
>>    other architectures.
>
> What is the warning here? I would assume that you should leave this
> unchanged as well.

ERROR: #define of 'ARCH_HAS_SETUP_ADDITIONAL_PAGES' is wrong - use Kconfig variables or standard guards instead
#2533: FILE: arch/riscv/include/asm/elf.h:79:
+#define ARCH_HAS_SETUP_ADDITIONAL_PAGES

>>  * We added new typedefs, I can remove these if that's a problem.  They're
>>    there to match our other code (bootloader and simulator).
>
> It depends. What typedefs are those? Removing the typedefs in both
> the kernel and the other code that uses the same types is likely the
> best option here.

OK, I'll add it to my TODO list.

>>  * A handful of lines over 80 characters that I think are onerous to break any
>>    more.
>
> Right, don't worry about it too much, and use common sense for this
> warning.
>
>>  * Some warnings about printk() not having a KERN_ prefix.  I fixed a handful
>>    of these, but the remaining ones I don't know how to fix (in show_regs, for
>>    example, where arm64 also has them).
>
> KERN_CONT

https://github.com/riscv/riscv-linux/commit/98e8fe9cb19d495180a9be03a0aa48c0183dd5be

>>  * Extern declarations in C files, all of which link to symbols in assembly or
>>    linker scripts.  These were copied from other architectures.
>
> I would try to fix those by using a header even if there is only one user.
> I'd actually like to get a compile-time warning for those in the long run,
> maybe with 'make W=1', so better don't introduce new ones.

OK, I'll fix them.

[toc] | [prev] | [next] | [standalone]

#1658500

On Tue, Jun 6, 2017 at 6:56 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> On Thu, 25 May 2017 12:51:54 PDT (-0700), Arnd Bergmann wrote:
>> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>> On Mon, 22 May 2017 19:11:35 PDT (-0700), olof@lixom.net wrote:
>>
>>>  * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also present in
>>>    other architectures.
>>
>> What is the warning here? I would assume that you should leave this
>> unchanged as well.
>
> ERROR: #define of 'ARCH_HAS_SETUP_ADDITIONAL_PAGES' is wrong - use Kconfig variables or standard guards instead
> #2533: FILE: arch/riscv/include/asm/elf.h:79:
> +#define ARCH_HAS_SETUP_ADDITIONAL_PAGES

Ok, you can definitely ignore this one. The warning is meant to prevent adding
new macros like that, but the macro already exists in the other architectures,
and I see no point in converting them all into a CONFIG_ symbol.

       Arnd

[toc] | [prev] | [next] | [standalone]

#1659186

On Tue, 06 Jun 2017 02:03:51 PDT (-0700), Arnd Bergmann wrote:
> On Tue, Jun 6, 2017 at 6:56 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> On Thu, 25 May 2017 12:51:54 PDT (-0700), Arnd Bergmann wrote:
>>> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>> On Mon, 22 May 2017 19:11:35 PDT (-0700), olof@lixom.net wrote:
>>>
>>>>  * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also present in
>>>>    other architectures.
>>>
>>> What is the warning here? I would assume that you should leave this
>>> unchanged as well.
>>
>> ERROR: #define of 'ARCH_HAS_SETUP_ADDITIONAL_PAGES' is wrong - use Kconfig variables or standard guards instead
>> #2533: FILE: arch/riscv/include/asm/elf.h:79:
>> +#define ARCH_HAS_SETUP_ADDITIONAL_PAGES
>
> Ok, you can definitely ignore this one. The warning is meant to prevent adding
> new macros like that, but the macro already exists in the other architectures,
> and I see no point in converting them all into a CONFIG_ symbol.

Sounds good.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web