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

[Arnd Bergmann]

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.

>  * 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