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

[Palmer Dabbelt]

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