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

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

[Maninder Singh]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 1422 bytes --]

Hi Thomas,

Thanks For your suggestions.

> >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
>unconditionally....

And I checked for these if()/BUG, I think we don't even need these if()/BUG constructs in codes.
For below 4 functions.
1. gic_dist_save
2. gic_dist_restore
3. gic_cpu_save
4. gic_cpu_restore

As we are checking in loop for (i = 0; i < MAX_GIC_NR; i++)
and passing i as gic_nr.  So below condition never returns true for any case, i think
 if (gic_nr >= MAX_GIC_NR).

So we can remove if/BUG constructs from these functions ??

5. gic_migrate_target 
And also in gic_migrate_target , we initializing gic_nr = 0;
and then checking whether gic_nr >= MAX_GIC_NR.

6. gic_cascade_irq
Before calling this function we have checked the same condition in gic_init_bases

So we can also remove if()/BUG from these functions also ?

Thanks ,
Maninder
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠsåy«\x1e­æ¶\x17…\x01\x06­†ÛiÿÿðÃ\x0fí»\x1fè®\x0få’i\x7f

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

[Thomas Gleixner]

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
unconditionally....

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

Thanks,

	tglx

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

[Maninder Singh]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 1396 bytes --]

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))

>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.

Thanks,
Maninder


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Thomas Gleixner]

On Fri, 19 Jun 2015, Maninder Singh wrote:

> Use BUG_ON(condition) instead of if(condition)/BUG()
> As given in scripts/coccinelle/misc/bugon.cocci
> 
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> Reviewed-by: Vaneet Narang <v.narang@samsung.com>
> ---
>  drivers/irqchip/irq-gic.c |   18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 8d7e1c8..b222c7b 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -329,8 +329,7 @@ static struct irq_chip gic_chip = {
>  
>  void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
>  {
> -	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.

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?

Thanks,

	tglx

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

[Maninder Singh]

Use BUG_ON(condition) instead of if(condition)/BUG()
As given in scripts/coccinelle/misc/bugon.cocci

Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
Reviewed-by: Vaneet Narang <v.narang@samsung.com>
---
 drivers/irqchip/irq-gic.c |   18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 8d7e1c8..b222c7b 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -329,8 +329,7 @@ static struct irq_chip gic_chip = {
 
 void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 {
-	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();
 	irq_set_chained_handler(irq, gic_handle_cascade_irq);
@@ -444,8 +443,7 @@ static void gic_dist_save(unsigned int gic_nr)
 	void __iomem *dist_base;
 	int i;
 
-	if (gic_nr >= MAX_GIC_NR)
-		BUG();
+	BUG_ON(gic_nr >= MAX_GIC_NR);
 
 	gic_irqs = gic_data[gic_nr].gic_irqs;
 	dist_base = gic_data_dist_base(&gic_data[gic_nr]);
@@ -479,8 +477,7 @@ static void gic_dist_restore(unsigned int gic_nr)
 	unsigned int i;
 	void __iomem *dist_base;
 
-	if (gic_nr >= MAX_GIC_NR)
-		BUG();
+	BUG_ON(gic_nr >= MAX_GIC_NR);
 
 	gic_irqs = gic_data[gic_nr].gic_irqs;
 	dist_base = gic_data_dist_base(&gic_data[gic_nr]);
@@ -516,8 +513,7 @@ static void gic_cpu_save(unsigned int gic_nr)
 	void __iomem *dist_base;
 	void __iomem *cpu_base;
 
-	if (gic_nr >= MAX_GIC_NR)
-		BUG();
+	BUG_ON(gic_nr >= MAX_GIC_NR);
 
 	dist_base = gic_data_dist_base(&gic_data[gic_nr]);
 	cpu_base = gic_data_cpu_base(&gic_data[gic_nr]);
@@ -542,8 +538,7 @@ static void gic_cpu_restore(unsigned int gic_nr)
 	void __iomem *dist_base;
 	void __iomem *cpu_base;
 
-	if (gic_nr >= MAX_GIC_NR)
-		BUG();
+	BUG_ON(gic_nr >= MAX_GIC_NR);
 
 	dist_base = gic_data_dist_base(&gic_data[gic_nr]);
 	cpu_base = gic_data_cpu_base(&gic_data[gic_nr]);
@@ -699,8 +694,7 @@ void gic_migrate_target(unsigned int new_cpu_id)
 	int i, ror_val, cpu = smp_processor_id();
 	u32 val, cur_target_mask, active_mask;
 
-	if (gic_nr >= MAX_GIC_NR)
-		BUG();
+	BUG_ON(gic_nr >= MAX_GIC_NR);
 
 	dist_base = gic_data_dist_base(&gic_data[gic_nr]);
 	if (!dist_base)
-- 
1.7.9.5