On Mon, 22 May 2017 19:11:35 PDT (-0700), o...@lixom.net wrote: > On Mon, May 22, 2017 at 5:41 PM, Palmer Dabbelt <pal...@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.