linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ARM: gic: use BUG_ON where possible
       [not found] <1352406191-14303-1-git-send-email-sasha.levin@oracle.com>
@ 2012-11-08 20:23 ` Sasha Levin
  2012-11-08 20:23 ` [PATCH] ARM: kprobes: " Sasha Levin
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: Sasha Levin @ 2012-11-08 20:23 UTC (permalink / raw)
  To: linux-arm-kernel

Just use BUG_ON() instead of constructions such as:

	if (...)
		BUG()

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) BUG();
+ BUG_ON(e);
// </smpl>

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 arch/arm/common/gic.c |   18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index aa52699..f0b8a10 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -336,10 +336,8 @@ 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();
-	if (irq_set_handler_data(irq, &gic_data[gic_nr]) != 0)
-		BUG();
+	BUG_ON(gic_nr >= MAX_GIC_NR);
+	BUG_ON(irq_set_handler_data(irq, &gic_data[gic_nr]) != 0);
 	irq_set_chained_handler(irq, gic_handle_cascade_irq);
 }
 
@@ -421,8 +419,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]);
@@ -456,8 +453,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]);
@@ -493,8 +489,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]);
@@ -519,8 +514,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]);
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH] ARM: kprobes: use BUG_ON where possible
       [not found] <1352406191-14303-1-git-send-email-sasha.levin@oracle.com>
  2012-11-08 20:23 ` [PATCH] ARM: gic: use BUG_ON where possible Sasha Levin
@ 2012-11-08 20:23 ` Sasha Levin
  2012-11-09  9:26   ` Jon Medhurst (Tixy)
  2012-11-08 20:23 ` [PATCH] ARM: EXYNOS: " Sasha Levin
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Sasha Levin @ 2012-11-08 20:23 UTC (permalink / raw)
  To: linux-arm-kernel

Just use BUG_ON() instead of constructions such as:

	if (...)
		BUG()

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) BUG();
+ BUG_ON(e);
// </smpl>

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 arch/arm/kernel/kprobes-test.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
index 1862d8f..0fb370d 100644
--- a/arch/arm/kernel/kprobes-test.c
+++ b/arch/arm/kernel/kprobes-test.c
@@ -1212,8 +1212,7 @@ static int register_test_probe(struct test_probe *probe)
 {
 	int ret;
 
-	if (probe->registered)
-		BUG();
+	BUG_ON(probe->registered);
 
 	ret = register_kprobe(&probe->kprobe);
 	if (ret >= 0) {
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH] ARM: EXYNOS: use BUG_ON where possible
       [not found] <1352406191-14303-1-git-send-email-sasha.levin@oracle.com>
  2012-11-08 20:23 ` [PATCH] ARM: gic: use BUG_ON where possible Sasha Levin
  2012-11-08 20:23 ` [PATCH] ARM: kprobes: " Sasha Levin
@ 2012-11-08 20:23 ` Sasha Levin
  2012-11-12 15:12   ` Maarten Lankhorst
  2012-11-08 20:23 ` [PATCH] ARM: integrator: " Sasha Levin
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Sasha Levin @ 2012-11-08 20:23 UTC (permalink / raw)
  To: linux-arm-kernel

Just use BUG_ON() instead of constructions such as:

	if (...)
		BUG()

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) BUG();
+ BUG_ON(e);
// </smpl>

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 arch/arm/mach-exynos/common.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-exynos/common.c b/arch/arm/mach-exynos/common.c
index 4e577f6..6a55a5a 100644
--- a/arch/arm/mach-exynos/common.c
+++ b/arch/arm/mach-exynos/common.c
@@ -465,10 +465,8 @@ static void __init combiner_cascade_irq(unsigned int combiner_nr, unsigned int i
 	else
 		max_nr = EXYNOS4_MAX_COMBINER_NR;
 
-	if (combiner_nr >= max_nr)
-		BUG();
-	if (irq_set_handler_data(irq, &combiner_data[combiner_nr]) != 0)
-		BUG();
+	BUG_ON(combiner_nr >= max_nr);
+	BUG_ON(irq_set_handler_data(irq, &combiner_data[combiner_nr]) != 0);
 	irq_set_chained_handler(irq, combiner_handle_cascade_irq);
 }
 
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH] ARM: integrator: use BUG_ON where possible
       [not found] <1352406191-14303-1-git-send-email-sasha.levin@oracle.com>
                   ` (2 preceding siblings ...)
  2012-11-08 20:23 ` [PATCH] ARM: EXYNOS: " Sasha Levin
@ 2012-11-08 20:23 ` Sasha Levin
  2012-11-12 20:44   ` Arnd Bergmann
  2012-11-08 20:23 ` [PATCH] ARM: OMAP1: " Sasha Levin
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Sasha Levin @ 2012-11-08 20:23 UTC (permalink / raw)
  To: linux-arm-kernel

Just use BUG_ON() instead of constructions such as:

	if (...)
		BUG()

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) BUG();
+ BUG_ON(e);
// </smpl>

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 arch/arm/mach-integrator/pci_v3.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-integrator/pci_v3.c b/arch/arm/mach-integrator/pci_v3.c
index bbeca59..85938de 100644
--- a/arch/arm/mach-integrator/pci_v3.c
+++ b/arch/arm/mach-integrator/pci_v3.c
@@ -191,12 +191,9 @@ static void __iomem *v3_open_config_window(struct pci_bus *bus,
 	/*
 	 * Trap out illegal values
 	 */
-	if (offset > 255)
-		BUG();
-	if (busnr > 255)
-		BUG();
-	if (devfn > 255)
-		BUG();
+	BUG_ON(offset > 255);
+	BUG_ON(busnr > 255);
+	BUG_ON(devfn > 255);
 
 	if (busnr == 0) {
 		int slot = PCI_SLOT(devfn);
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH] ARM: OMAP1: use BUG_ON where possible
       [not found] <1352406191-14303-1-git-send-email-sasha.levin@oracle.com>
                   ` (3 preceding siblings ...)
  2012-11-08 20:23 ` [PATCH] ARM: integrator: " Sasha Levin
@ 2012-11-08 20:23 ` Sasha Levin
  2012-11-12 23:21   ` Tony Lindgren
  2012-11-08 20:23 ` [PATCH] ARM: dma: " Sasha Levin
  2012-11-08 20:23 ` [PATCH] ARM: versatile: " Sasha Levin
  6 siblings, 1 reply; 15+ messages in thread
From: Sasha Levin @ 2012-11-08 20:23 UTC (permalink / raw)
  To: linux-arm-kernel

Just use BUG_ON() instead of constructions such as:

	if (...)
		BUG()

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) BUG();
+ BUG_ON(e);
// </smpl>

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 arch/arm/mach-omap1/board-fsample.c  |    3 +--
 arch/arm/mach-omap1/board-h2.c       |    3 +--
 arch/arm/mach-omap1/board-h3.c       |    3 +--
 arch/arm/mach-omap1/board-perseus2.c |    3 +--
 4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap1/board-fsample.c b/arch/arm/mach-omap1/board-fsample.c
index 8b5800a..7ca6cc4 100644
--- a/arch/arm/mach-omap1/board-fsample.c
+++ b/arch/arm/mach-omap1/board-fsample.c
@@ -307,8 +307,7 @@ static void __init omap_fsample_init(void)
 
 	fsample_init_smc91x();
 
-	if (gpio_request(FSAMPLE_NAND_RB_GPIO_PIN, "NAND ready") < 0)
-		BUG();
+	BUG_ON(gpio_request(FSAMPLE_NAND_RB_GPIO_PIN, "NAND ready") < 0);
 	gpio_direction_input(FSAMPLE_NAND_RB_GPIO_PIN);
 
 	omap_cfg_reg(L3_1610_FLASH_CS2B_OE);
diff --git a/arch/arm/mach-omap1/board-h2.c b/arch/arm/mach-omap1/board-h2.c
index 9134b64..4953cf7 100644
--- a/arch/arm/mach-omap1/board-h2.c
+++ b/arch/arm/mach-omap1/board-h2.c
@@ -412,8 +412,7 @@ static void __init h2_init(void)
 
 	h2_nand_resource.end = h2_nand_resource.start = OMAP_CS2B_PHYS;
 	h2_nand_resource.end += SZ_4K - 1;
-	if (gpio_request(H2_NAND_RB_GPIO_PIN, "NAND ready") < 0)
-		BUG();
+	BUG_ON(gpio_request(H2_NAND_RB_GPIO_PIN, "NAND ready") < 0);
 	gpio_direction_input(H2_NAND_RB_GPIO_PIN);
 
 	omap_cfg_reg(L3_1610_FLASH_CS2B_OE);
diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c
index bf213d1..563ba16 100644
--- a/arch/arm/mach-omap1/board-h3.c
+++ b/arch/arm/mach-omap1/board-h3.c
@@ -406,8 +406,7 @@ static void __init h3_init(void)
 
 	nand_resource.end = nand_resource.start = OMAP_CS2B_PHYS;
 	nand_resource.end += SZ_4K - 1;
-	if (gpio_request(H3_NAND_RB_GPIO_PIN, "NAND ready") < 0)
-		BUG();
+	BUG_ON(gpio_request(H3_NAND_RB_GPIO_PIN, "NAND ready") < 0);
 	gpio_direction_input(H3_NAND_RB_GPIO_PIN);
 
 	/* GPIO10 Func_MUX_CTRL reg bit 29:27, Configure V2 to mode1 as GPIO */
diff --git a/arch/arm/mach-omap1/board-perseus2.c b/arch/arm/mach-omap1/board-perseus2.c
index 030bd48..67c2612 100644
--- a/arch/arm/mach-omap1/board-perseus2.c
+++ b/arch/arm/mach-omap1/board-perseus2.c
@@ -275,8 +275,7 @@ static void __init omap_perseus2_init(void)
 
 	perseus2_init_smc91x();
 
-	if (gpio_request(P2_NAND_RB_GPIO_PIN, "NAND ready") < 0)
-		BUG();
+	BUG_ON(gpio_request(P2_NAND_RB_GPIO_PIN, "NAND ready") < 0);
 	gpio_direction_input(P2_NAND_RB_GPIO_PIN);
 
 	omap_cfg_reg(L3_1610_FLASH_CS2B_OE);
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH] ARM: dma: use BUG_ON where possible
       [not found] <1352406191-14303-1-git-send-email-sasha.levin@oracle.com>
                   ` (4 preceding siblings ...)
  2012-11-08 20:23 ` [PATCH] ARM: OMAP1: " Sasha Levin
@ 2012-11-08 20:23 ` Sasha Levin
  2012-11-08 20:23 ` [PATCH] ARM: versatile: " Sasha Levin
  6 siblings, 0 replies; 15+ messages in thread
From: Sasha Levin @ 2012-11-08 20:23 UTC (permalink / raw)
  To: linux-arm-kernel

Just use BUG_ON() instead of constructions such as:

	if (...)
		BUG()

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) BUG();
+ BUG_ON(e);
// </smpl>

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 arch/arm/mach-rpc/dma.c     |    3 +--
 arch/arm/mach-s3c64xx/dma.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-rpc/dma.c b/arch/arm/mach-rpc/dma.c
index 85883b2..92e22ba 100644
--- a/arch/arm/mach-rpc/dma.c
+++ b/arch/arm/mach-rpc/dma.c
@@ -265,8 +265,7 @@ static void floppy_enable_dma(unsigned int chan, dma_t *dma)
 	unsigned int fiqhandler_length;
 	struct pt_regs regs;
 
-	if (fdma->dma.sg)
-		BUG();
+	BUG_ON(fdma->dma.sg);
 
 	if (fdma->dma.dma_mode == DMA_MODE_READ) {
 		extern unsigned char floppy_fiqin_start, floppy_fiqin_end;
diff --git a/arch/arm/mach-s3c64xx/dma.c b/arch/arm/mach-s3c64xx/dma.c
index f2a7a17..585c2ae 100644
--- a/arch/arm/mach-s3c64xx/dma.c
+++ b/arch/arm/mach-s3c64xx/dma.c
@@ -603,8 +603,7 @@ static irqreturn_t s3c64xx_dma_irq(int irq, void *pw)
 				&& buff->next != chan->next)
 			buff = buff->next;
 
-		if (!buff)
-			BUG();
+		BUG_ON(!buff);
 
 		if (buff == chan->next)
 			buff = chan->end;
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH] ARM: versatile: use BUG_ON where possible
       [not found] <1352406191-14303-1-git-send-email-sasha.levin@oracle.com>
                   ` (5 preceding siblings ...)
  2012-11-08 20:23 ` [PATCH] ARM: dma: " Sasha Levin
@ 2012-11-08 20:23 ` Sasha Levin
  6 siblings, 0 replies; 15+ messages in thread
From: Sasha Levin @ 2012-11-08 20:23 UTC (permalink / raw)
  To: linux-arm-kernel

Just use BUG_ON() instead of constructions such as:

	if (...)
		BUG()

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) BUG();
+ BUG_ON(e);
// </smpl>

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 arch/arm/mach-versatile/pci.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-versatile/pci.c b/arch/arm/mach-versatile/pci.c
index 2f84f40..3936a11 100644
--- a/arch/arm/mach-versatile/pci.c
+++ b/arch/arm/mach-versatile/pci.c
@@ -82,12 +82,9 @@ static void __iomem *__pci_addr(struct pci_bus *bus,
 	/*
 	 * Trap out illegal values
 	 */
-	if (offset > 255)
-		BUG();
-	if (busnr > 255)
-		BUG();
-	if (devfn > 255)
-		BUG();
+	BUG_ON(offset > 255);
+	BUG_ON(busnr > 255);
+	BUG_ON(devfn > 255);
 
 	return VERSATILE_PCI_CFG_VIRT_BASE + ((busnr << 16) |
 		(PCI_SLOT(devfn) << 11) | (PCI_FUNC(devfn) << 8) | offset);
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH] ARM: kprobes: use BUG_ON where possible
  2012-11-08 20:23 ` [PATCH] ARM: kprobes: " Sasha Levin
@ 2012-11-09  9:26   ` Jon Medhurst (Tixy)
  0 siblings, 0 replies; 15+ messages in thread
From: Jon Medhurst (Tixy) @ 2012-11-09  9:26 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, 2012-11-08 at 15:23 -0500, Sasha Levin wrote:
> Just use BUG_ON() instead of constructions such as:
> 
> 	if (...)
> 		BUG()
> 
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression e;
> @@
> - if (e) BUG();
> + BUG_ON(e);
> // </smpl>
> 
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>

I'm not sure that trivial changes like this are worth it, but equally,
they're not worth having a discussion about, so...

Acked-by: Jon Medhurst <tixy@linaro.org>

> ---
>  arch/arm/kernel/kprobes-test.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
> index 1862d8f..0fb370d 100644
> --- a/arch/arm/kernel/kprobes-test.c
> +++ b/arch/arm/kernel/kprobes-test.c
> @@ -1212,8 +1212,7 @@ static int register_test_probe(struct test_probe *probe)
>  {
>  	int ret;
>  
> -	if (probe->registered)
> -		BUG();
> +	BUG_ON(probe->registered);
>  
>  	ret = register_kprobe(&probe->kprobe);
>  	if (ret >= 0) {

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] ARM: EXYNOS: use BUG_ON where possible
  2012-11-08 20:23 ` [PATCH] ARM: EXYNOS: " Sasha Levin
@ 2012-11-12 15:12   ` Maarten Lankhorst
  2012-11-12 15:23     ` Russell King - ARM Linux
  2012-11-12 15:25     ` Sasha Levin
  0 siblings, 2 replies; 15+ messages in thread
From: Maarten Lankhorst @ 2012-11-12 15:12 UTC (permalink / raw)
  To: linux-arm-kernel

Op 08-11-12 21:23, Sasha Levin schreef:
> Just use BUG_ON() instead of constructions such as:
>
> 	if (...)
> 		BUG()
>
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression e;
> @@
> - if (e) BUG();
> + BUG_ON(e);
> // </smpl>
>
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
>  arch/arm/mach-exynos/common.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/common.c b/arch/arm/mach-exynos/common.c
> index 4e577f6..6a55a5a 100644
> --- a/arch/arm/mach-exynos/common.c
> +++ b/arch/arm/mach-exynos/common.c
> @@ -465,10 +465,8 @@ static void __init combiner_cascade_irq(unsigned int combiner_nr, unsigned int i
>  	else
>  		max_nr = EXYNOS4_MAX_COMBINER_NR;
>  
> -	if (combiner_nr >= max_nr)
> -		BUG();
> -	if (irq_set_handler_data(irq, &combiner_data[combiner_nr]) != 0)
> -		BUG();
> +	BUG_ON(combiner_nr >= max_nr);
> +	BUG_ON(irq_set_handler_data(irq, &combiner_data[combiner_nr]) != 0);
Is it really a good idea to put functions that perform work in a BUG_ON?
I don't know, but for some reason it just feels wrong. I'd expect code to
compile fine if BUG_ON was a noop, so doing verification calls only, not
actual work..

~Maarten

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] ARM: EXYNOS: use BUG_ON where possible
  2012-11-12 15:12   ` Maarten Lankhorst
@ 2012-11-12 15:23     ` Russell King - ARM Linux
  2012-11-12 15:52       ` Sasha Levin
  2012-11-12 15:25     ` Sasha Levin
  1 sibling, 1 reply; 15+ messages in thread
From: Russell King - ARM Linux @ 2012-11-12 15:23 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Nov 12, 2012 at 04:12:29PM +0100, Maarten Lankhorst wrote:
> Op 08-11-12 21:23, Sasha Levin schreef:
> > @@ -465,10 +465,8 @@ static void __init combiner_cascade_irq(unsigned int combiner_nr, unsigned int i
> >  	else
> >  		max_nr = EXYNOS4_MAX_COMBINER_NR;
> >  
> > -	if (combiner_nr >= max_nr)
> > -		BUG();
> > -	if (irq_set_handler_data(irq, &combiner_data[combiner_nr]) != 0)
> > -		BUG();
> > +	BUG_ON(combiner_nr >= max_nr);
> > +	BUG_ON(irq_set_handler_data(irq, &combiner_data[combiner_nr]) != 0);
>
> Is it really a good idea to put functions that perform work in a BUG_ON?
> I don't know, but for some reason it just feels wrong. I'd expect code to
> compile fine if BUG_ON was a noop, so doing verification calls only, not
> actual work..

Well, it is currently defined as:

include/asm-generic/bug.h:#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
include/asm-generic/bug.h:#define BUG_ON(condition) do { if (condition) ; } while(0)

but as these can be overridden, I don't think relying on those
implementations is a good idea; to do so would be fragile.  Eg, what if
the BUG_ON() implementation becomes just:

#define BUG_ON(x)

then the function call itself vanishes.  So, only put the actual bug test
inside a BUG_ON(), not the functional part which must always be executed.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] ARM: EXYNOS: use BUG_ON where possible
  2012-11-12 15:12   ` Maarten Lankhorst
  2012-11-12 15:23     ` Russell King - ARM Linux
@ 2012-11-12 15:25     ` Sasha Levin
  1 sibling, 0 replies; 15+ messages in thread
From: Sasha Levin @ 2012-11-12 15:25 UTC (permalink / raw)
  To: linux-arm-kernel

On 11/12/2012 10:12 AM, Maarten Lankhorst wrote:
> Op 08-11-12 21:23, Sasha Levin schreef:
>> Just use BUG_ON() instead of constructions such as:
>>
>> 	if (...)
>> 		BUG()
>>
>> A simplified version of the semantic patch that makes this transformation
>> is as follows: (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @@
>> expression e;
>> @@
>> - if (e) BUG();
>> + BUG_ON(e);
>> // </smpl>
>>
>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>> ---
>>  arch/arm/mach-exynos/common.c |    6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/common.c b/arch/arm/mach-exynos/common.c
>> index 4e577f6..6a55a5a 100644
>> --- a/arch/arm/mach-exynos/common.c
>> +++ b/arch/arm/mach-exynos/common.c
>> @@ -465,10 +465,8 @@ static void __init combiner_cascade_irq(unsigned int combiner_nr, unsigned int i
>>  	else
>>  		max_nr = EXYNOS4_MAX_COMBINER_NR;
>>  
>> -	if (combiner_nr >= max_nr)
>> -		BUG();
>> -	if (irq_set_handler_data(irq, &combiner_data[combiner_nr]) != 0)
>> -		BUG();
>> +	BUG_ON(combiner_nr >= max_nr);
>> +	BUG_ON(irq_set_handler_data(irq, &combiner_data[combiner_nr]) != 0);
> Is it really a good idea to put functions that perform work in a BUG_ON?
> I don't know, but for some reason it just feels wrong. I'd expect code to
> compile fine if BUG_ON was a noop, so doing verification calls only, not
> actual work..

You can't modify the side-effects of a macro based on kernel configuration. If
we're evaluating the expression when BUG_ON() is enabled, you must also evaluate
the expression when BUG_ON() is not enabled (HAVE_ARCH_BUG_ON is not set).

The only reason I'd not include function calls in a BUG_ON() call is due to
readability considerations. In this case it looked okay to me, but if someone
thinks that getting the function call into the BUG_ON() complicated things I'm
fine with skipping that.


Thanks,
Sasha

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] ARM: EXYNOS: use BUG_ON where possible
  2012-11-12 15:23     ` Russell King - ARM Linux
@ 2012-11-12 15:52       ` Sasha Levin
  0 siblings, 0 replies; 15+ messages in thread
From: Sasha Levin @ 2012-11-12 15:52 UTC (permalink / raw)
  To: linux-arm-kernel

On 11/12/2012 10:23 AM, Russell King - ARM Linux wrote:
> On Mon, Nov 12, 2012 at 04:12:29PM +0100, Maarten Lankhorst wrote:
>> Op 08-11-12 21:23, Sasha Levin schreef:
>>> @@ -465,10 +465,8 @@ static void __init combiner_cascade_irq(unsigned int combiner_nr, unsigned int i
>>>  	else
>>>  		max_nr = EXYNOS4_MAX_COMBINER_NR;
>>>  
>>> -	if (combiner_nr >= max_nr)
>>> -		BUG();
>>> -	if (irq_set_handler_data(irq, &combiner_data[combiner_nr]) != 0)
>>> -		BUG();
>>> +	BUG_ON(combiner_nr >= max_nr);
>>> +	BUG_ON(irq_set_handler_data(irq, &combiner_data[combiner_nr]) != 0);
>>
>> Is it really a good idea to put functions that perform work in a BUG_ON?
>> I don't know, but for some reason it just feels wrong. I'd expect code to
>> compile fine if BUG_ON was a noop, so doing verification calls only, not
>> actual work..
> 
> Well, it is currently defined as:
> 
> include/asm-generic/bug.h:#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
> include/asm-generic/bug.h:#define BUG_ON(condition) do { if (condition) ; } while(0)
> 
> but as these can be overridden, I don't think relying on those
> implementations is a good idea; to do so would be fragile.  Eg, what if
> the BUG_ON() implementation becomes just:
> 
> #define BUG_ON(x)
> 
> then the function call itself vanishes.  So, only put the actual bug test
> inside a BUG_ON(), not the functional part which must always be executed.

Even if we ignore that modifying the side-effects is wrong, there's already
more than enough code in the kernel (both in kernel/ / mm/, and in arch/) to
cause breakage if for some reason the expression is not evaluated.

If some arch decides to not evaluate the expression there it's going to be
inherently broken.


Thanks,
Sasha

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] ARM: integrator: use BUG_ON where possible
  2012-11-08 20:23 ` [PATCH] ARM: integrator: " Sasha Levin
@ 2012-11-12 20:44   ` Arnd Bergmann
  2012-11-17 18:41     ` Linus Walleij
  0 siblings, 1 reply; 15+ messages in thread
From: Arnd Bergmann @ 2012-11-12 20:44 UTC (permalink / raw)
  To: linux-arm-kernel

On Thursday 08 November 2012, Sasha Levin wrote:
> Just use BUG_ON() instead of constructions such as:
> 
> 	if (...)
> 		BUG()
> 
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression e;
> @@
> - if (e) BUG();
> + BUG_ON(e);
> // </smpl>
> 
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>

Linus Walleij is doing most of the integrator work these days, maybe he
wants to apply the patch.

Acked-by: Arnd Bergmann <arnd@arndb.de>

>  arch/arm/mach-integrator/pci_v3.c |    9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/mach-integrator/pci_v3.c b/arch/arm/mach-integrator/pci_v3.c
> index bbeca59..85938de 100644
> --- a/arch/arm/mach-integrator/pci_v3.c
> +++ b/arch/arm/mach-integrator/pci_v3.c
> @@ -191,12 +191,9 @@ static void __iomem *v3_open_config_window(struct pci_bus *bus,
>  	/*
>  	 * Trap out illegal values
>  	 */
> -	if (offset > 255)
> -		BUG();
> -	if (busnr > 255)
> -		BUG();
> -	if (devfn > 255)
> -		BUG();
> +	BUG_ON(offset > 255);
> +	BUG_ON(busnr > 255);
> +	BUG_ON(devfn > 255);
>  
>  	if (busnr == 0) {
>  		int slot = PCI_SLOT(devfn);
> -- 
> 1.7.10.4
> 
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] ARM: OMAP1: use BUG_ON where possible
  2012-11-08 20:23 ` [PATCH] ARM: OMAP1: " Sasha Levin
@ 2012-11-12 23:21   ` Tony Lindgren
  0 siblings, 0 replies; 15+ messages in thread
From: Tony Lindgren @ 2012-11-12 23:21 UTC (permalink / raw)
  To: linux-arm-kernel

* Sasha Levin <sasha.levin@oracle.com> [121108 12:26]:
> Just use BUG_ON() instead of constructions such as:
> 
> 	if (...)
> 		BUG()
> 
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression e;
> @@
> - if (e) BUG();
> + BUG_ON(e);
> // </smpl>
> 
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>

Thanks applying into omap-for-v3.8/board-v2.

Regards,

Tony

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] ARM: integrator: use BUG_ON where possible
  2012-11-12 20:44   ` Arnd Bergmann
@ 2012-11-17 18:41     ` Linus Walleij
  0 siblings, 0 replies; 15+ messages in thread
From: Linus Walleij @ 2012-11-17 18:41 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Nov 12, 2012 at 9:44 PM, Arnd Bergmann <arnd@arndb.de> wrote:

>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>
> Linus Walleij is doing most of the integrator work these days, maybe he
> wants to apply the patch.
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

OK I'll apply it to my stash and submit a pull request to
simplify things.

I could split this into its own topic but with noone else changing
anything orthogonally on the Integrator I don't know if it's really
worth it, probably it's just better to take the stuff I sent yesterday
and a patch I made this evening and request it all to be pulled
at once.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2012-11-17 18:41 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <1352406191-14303-1-git-send-email-sasha.levin@oracle.com>
2012-11-08 20:23 ` [PATCH] ARM: gic: use BUG_ON where possible Sasha Levin
2012-11-08 20:23 ` [PATCH] ARM: kprobes: " Sasha Levin
2012-11-09  9:26   ` Jon Medhurst (Tixy)
2012-11-08 20:23 ` [PATCH] ARM: EXYNOS: " Sasha Levin
2012-11-12 15:12   ` Maarten Lankhorst
2012-11-12 15:23     ` Russell King - ARM Linux
2012-11-12 15:52       ` Sasha Levin
2012-11-12 15:25     ` Sasha Levin
2012-11-08 20:23 ` [PATCH] ARM: integrator: " Sasha Levin
2012-11-12 20:44   ` Arnd Bergmann
2012-11-17 18:41     ` Linus Walleij
2012-11-08 20:23 ` [PATCH] ARM: OMAP1: " Sasha Levin
2012-11-12 23:21   ` Tony Lindgren
2012-11-08 20:23 ` [PATCH] ARM: dma: " Sasha Levin
2012-11-08 20:23 ` [PATCH] ARM: versatile: " Sasha Levin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).