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

[Palmer Dabbelt]

On Mon, 22 May 2017 19:11:35 PDT (-0700), olof@xxxxxxxxx wrote:
> On Mon, May 22, 2017 at 5:41 PM, Palmer Dabbelt <palmer@xxxxxxxxxxx> 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.