[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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://mail.coreboot.org/pipermail/coreboot/attachments/20180703/8b7f6c30/attachment.asc>