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
signature.asc
Description: PGP signature
-- coreboot mailing list: coreboot@coreboot.org https://mail.coreboot.org/mailman/listinfo/coreboot
