| Message ID | 20210701141130.940-1-wangborong@cdjrlc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] sched: Use BUG_ON | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (e289c2e239c638cab7e71143e0a65c7c4a057ad7) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 10 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Hi Jason, > The BUG_ON macro simplifies the if condition followed by BUG, so that > we can use BUG_ON instead of if condition followed by BUG. [...] > - if (spu_acquire(ctx)) > - BUG(); /* a kernel thread never has signals pending */ > + /* a kernel thread never has signals pending */ > + BUG_ON(spu_acquire(ctx)); I'm not convinced that this is an improvement; you've combined the acquire and the BUG into a single statement, and now it's no longer clear what the comment applies to. If you really wanted to use BUG_ON, something like this would be more clear: rc = spu_acquire(ctx); /* a kernel thread never has signals pending */ BUG_ON(rc); but we don't have a suitable rc variable handy, so we'd need one of those declared too. You could avoid that with: if (spu_acquire(ctx)) BUG_ON(1); /* a kernel thread never has signals pending */ but wait: no need for the constant there, so this would be better: if (spu_acquire(ctx)) BUG(); /* a kernel thread never has signals pending */ wait, what are we doing again? To me, this is a bit of shuffling code around, for no real benefit. Regards, Jeremy
diff --git a/arch/powerpc/platforms/cell/spufs/sched.c b/arch/powerpc/platforms/cell/spufs/sched.c index 369206489895..0f218d9e5733 100644 --- a/arch/powerpc/platforms/cell/spufs/sched.c +++ b/arch/powerpc/platforms/cell/spufs/sched.c @@ -904,8 +904,8 @@ static noinline void spusched_tick(struct spu_context *ctx) struct spu_context *new = NULL; struct spu *spu = NULL; - if (spu_acquire(ctx)) - BUG(); /* a kernel thread never has signals pending */ + /* a kernel thread never has signals pending */ + BUG_ON(spu_acquire(ctx)); if (ctx->state != SPU_STATE_RUNNABLE) goto out;
The BUG_ON macro simplifies the if condition followed by BUG, so that we can use BUG_ON instead of if condition followed by BUG. Signed-off-by: Jason Wang <wangborong@cdjrlc.com> --- arch/powerpc/platforms/cell/spufs/sched.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)