Re: [PATCH 1/1] irq-gic: use BUG_ON instead of if()/BUG

From: Thomas Gleixner
Date: Fri Jun 19 2015 - 06:07:48 EST

On Fri, 19 Jun 2015, Maninder Singh wrote:
> Hi Thomas,
> >> {
> >> - if (gic_nr >= MAX_GIC_NR)
> >> - BUG();
> >> + BUG_ON(gic_nr >= MAX_GIC_NR);
> >> if (irq_set_handler_data(irq, &gic_data[gic_nr]) != 0)
> >> BUG();
> >
> >So this patch was clearly done just by running a script and not sanity
> >checked afterwards. Otherwise the next if() BUG(); construct would
> >have been fixed as well.
> Yes semantic patch did the changes to use preferred APIs,
> And it also changed this BUG_ON(irq_set_handler_data(irq, &gic_data[gic_nr]) != 0)
> But we have to take care that condition has no side effects i.e.
> if()/BUG conversion to BUG_ON must be avoided when there's side effect
> in condition. The reason being BUG_ON won't execute the condition when CONFIG_BUG
> is not defined As suggested by Julia Lawall
> Thats why did not take that change --> (BUG_ON(irq_set_handler_data(irq, &gic_data[gic_nr]) != 0))

Fair enough. But you should mention that in the changelog.

> >Further, while we are at that. It would be even more useful to analyze
> >whether the BUG_ON() is needed in the first place or at least could be
> >made conditional on some debug option.
> >
> >But that's not done by the script either, right?
> Yes coccinelle semantic patches did not do that changes.
> we have to choose whether to make BUG_ON conditional on some debug options.

Right, and that's what I'm asking for. IOW, instead of blindly running
scripts at least ask the question whether this stuff needs to be there

Such information can be put into the changelog and helps the reviewers
to distinguish thoughtful people from script bots.


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at