Re: [coreboot] Fw:Re: I want to submit some code, but there are someproblems.

[Jonathan Neuschäfer]

On Tue, Jul 03, 2018 at 05:11:57PM +0800, 王翔 wrote:
> > On Tue, Jul 03, 2018 at 04:23:07PM +0800, 王翔 wrote:

[...]
> Error print like this:
> WARNING: Macros with flow control statements should be avoided
> #43: FILE: src/arch/riscv/include/smp.h:43:
> +#define SMP_PAUSE(active) do \
> +{ \
> +    __label__ l_entry, l_exit; \
> +    static int _flag_; \
> +    if (active == read_csr(mhartid)) \
> +        goto l_entry; \
> +    do {barrier(); } while (!_flag_); \
> +    goto l_exit; \
> +l_entry:

checkpatch doesn't like goto (or return) in macros, because it can be
unexpected for the author of the calling code, that a macro causes a
jump or return.
> WARNING: please, no spaces at the start of a line
> #45: FILE: src/arch/riscv/include/smp.h:45:
> __label__ l_entry, l_exit; $

You should indent with tabs instead of spaces.
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #54: FILE: src/arch/riscv/include/smp.h:54:
> +#define SMP_RESUME() \
> _flag_ = 1; \
>
> +l_exit: \
> barrier(); \
>
> +} while (0)

Oh! SMP_PAUSE actually jumps here! I would *not* expect that, when I
read code like this:

SMP_PAUSE(active);
    foo();
    bar(42);
    SMP_RESUME();

Something like the following would be a lot clearer for me:

if (mhartid() == active) {
    	foo();
    	bar(42);
    }

Or maybe:

if (running_on_hart(active)) { ... }

But note that this lacks the barriers/locking that your code has.

Jonathan Neuschäfer