omap: Ptr "isr_reg" tracked as NULL was dereferenced

PATCH Superseded Evgeny Kuznetsov 3 participants 8 messages Oct 13, 2010 – Oct 18, 2010

Subsystems (2)

Arm (32-bit) Arm sub-architectures

lore

Share & tools

lore

Evgeny Kuznetsov

author

2010-10-13 10:04 UTC #0 lore raw

v2 cover omap: Ptr "isr_reg" tracked as NULL was dereferenced

Hi,

Here is second version of patch which fixes bug in /arch/arm/plat-omap/gpio.c file.
Pointer which may have NULL value in some cases (depend on kernel configuration and GPIO method) is dereferenced later in code.

I removed BUG() macro to do not halt code execution accoding to comments. And added WARN_ON() macro and exit from function if pointer "isr_reg" is NULL. Also compilation check is added for correct architecture configuration.

Thanks,
Evgeny

Evgeny Kuznetsov (1):
omap: Ptr "isr_reg" tracked as NULL was dereferenced

 arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

Evgeny Kuznetsov

author

2010-10-13 10:04 UTC #1 lore raw

PATCH v2 1/1 omap: Ptr "isr_reg" tracked as NULL was dereferenced

From: Evgeny Kuznetsov <[email protected]>

Value of "isr_reg" pointer is depend on configuration and GPIO method. Potentially it may have NULL value and it is dereferenced later in code. If pointer is NULL there is some kernel issue. Warning and exit from function are added in this case.
Also compilation check is added for correct architecture configuration.

Signed-off-by Evgeny Kuznetsov <[email protected]>

---
 arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

arch/arm/plat-omap/gpio.c +18 -0

index c05c653..d04913c 100644
@@ -1318,6 +1318,23 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 	if (bank->method == METHOD_GPIO_44XX)
 		isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0;
 #endif
+
+#if !defined(CONFIG_ARCH_OMAP1) &&		\
+		!defined(CONFIG_ARCH_OMAP15XX) &&	\
+		!defined(CONFIG_ARCH_OMAP16XX) &&	\
+		!defined(CONFIG_ARCH_OMAP730) &&	\
+		!defined(CONFIG_ARCH_OMAP850) &&	\
+		!defined(CONFIG_ARCH_OMAP2) &&	\
+		!defined(CONFIG_ARCH_OMAP3) &&	\
+		!defined(CONFIG_ARCH_OMAP4)
+	
+#error "Incorrect arch configuration"
+	
+#endif
+	
+	if (WARN_ON(!isr_reg))
+		goto exit;
+
 	while(1) {
 		u32 isr_saved, level_mask = 0;
 		u32 enabled;

@@ -1377,6 +1394,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 	configured, we must unmask the bank interrupt only after
 	handler(s) are executed in order to avoid spurious bank
 	interrupt */
+exit:
 	if (!unmasked)
 		desc->chip->unmask(irq);

Signature · 1 line

1.6.3.3

Varadarajan, Charulatha

2010-10-13 10:55 UTC #2 lore raw

Quoted 13 lines

-----Original Message-----
From: Evgeny Kuznetsov [mailto:EXT-Eugeny.Kuznetsov at nokia.com]
Sent: Wednesday, October 13, 2010 3:35 PM
To: tony at atomide.com
Cc: linux-omap at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm- kernel at lists.infradead.org; Balbi, Felipe; linux at arm.linux.org.uk; khilman at deeprootsystems.com; akpm at linux-foundation.org; Varadarajan, Charulatha; tero.kristo at nokia.com; ext-eugeny.kuznetsov at nokia.com
Subject: [PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

Quoted 5 lines

From: Evgeny Kuznetsov <[email protected]>
Value of "isr_reg" pointer is depend on configuration and GPIO method. Potentially it may have NULL value and it is dereferenced later in code. If pointer is NULL there is some kernel issue.

Can you elaborate?

Quoted 19 lines

Warning and exit from function are added in this case. Also compilation check is added for correct architecture configuration.
Signed-off-by Evgeny Kuznetsov <[email protected]>
---
 arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)
arch/arm/plat-omap/gpio.c +1 -0
index c05c653..d04913c 100644
@@ -1318,6 +1318,23 @@ static void gpio_irq_handler(unsigned int irq,
 struct irq_desc *desc)
 	if (bank->method == METHOD_GPIO_44XX)
 		isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0;
 #endif
+

arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, · line 1332

Quoted 10 lines

arch/arm/plat-omap/gpio.c +10 -0

+#if !defined(CONFIG_ARCH_OMAP1) &&		\
+		!defined(CONFIG_ARCH_OMAP15XX) &&	\
+		!defined(CONFIG_ARCH_OMAP16XX) &&	\
+		!defined(CONFIG_ARCH_OMAP730) &&	\
+		!defined(CONFIG_ARCH_OMAP850) &&	\
+		!defined(CONFIG_ARCH_OMAP2) &&	\
+		!defined(CONFIG_ARCH_OMAP3) &&	\
+		!defined(CONFIG_ARCH_OMAP4)
+
+#error "Incorrect arch configuration"

This is not required. If the architecture is not one of the above mentioned, gpio_irq_handler() will not be used/called at all.

Also all the possible gpio methods for a given OMAP architecture are already considered with "#ifdef"s and (bank->method) checks in gpio_irq_handler().

arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1336

Quoted 5 lines

arch/arm/plat-omap/gpio.c +5 -0
+
+#endif
+
+	if (WARN_ON(!isr_reg))
+		goto exit;

For the above mentioned reason, this isr_reg would be non-NULL. Have you observed this error anytime?

Also, the omap-gpio code has similar code spread all over and has to be anyway cleaned-up. Is there any reason why gpio_irq_handler() alone is addressed in this patch?

Quoted 15 lines

arch/arm/plat-omap/gpio.c +1 -0

+
 	while(1) {
 		u32 isr_saved, level_mask = 0;

arch/arm/plat-omap/gpio.c +1 -1

 		u32 enabled;

@@ -1377,6 +1394,7 @@ static void gpio_irq_handler(unsigned int irq,
 struct irq_desc *desc)
 	configured, we must unmask the bank interrupt only after
 	handler(s) are executed in order to avoid spurious bank
 	interrupt */
+exit:
 	if (!unmasked)
 		desc->chip->unmask(irq);
 
--

1.6.3.3

Evgeny Kuznetsov

author

2010-10-13 11:43 UTC #3 lore raw

Quoted 12 lines · Charulatha

Quoted 12 lines
-----Original Message-----
From: Evgeny Kuznetsov [mailto:EXT-Eugeny.Kuznetsov at nokia.com]
Sent: Wednesday, October 13, 2010 3:35 PM
To: tony at atomide.com
Cc: linux-omap at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm- kernel at lists.infradead.org; Balbi, Felipe; linux at arm.linux.org.uk; khilman at deeprootsystems.com; akpm at linux-foundation.org; Varadarajan, Charulatha; tero.kristo at nokia.com; ext-eugeny.kuznetsov at nokia.com
Subject: [PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

Quoted 7 lines

Quoted 5 lines
From: Evgeny Kuznetsov <[email protected]>
Value of "isr_reg" pointer is depend on configuration and GPIO method. Potentially it may have NULL value and it is dereferenced later in code. If pointer is NULL there is some kernel issue.
Can you elaborate?

"isr_reg" should not be NULL. But if it is NULL then there is kernel bug. And WARN_ON() used to show it.
I did not see this bug, this is potentially may happen.

Quoted 25 lines

Warning and exit from function are added in this case. Also compilation check is added for correct architecture configuration.

Signed-off-by Evgeny Kuznetsov <[email protected]>

---
 arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

arch/arm/plat-omap/gpio.c +6 -0

index c05c653..d04913c 100644
@@ -1318,6 +1318,23 @@ static void gpio_irq_handler(unsigned int irq,
 struct irq_desc *desc)
 	if (bank->method == METHOD_GPIO_44XX)
 		isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0;
 #endif
+
+#if !defined(CONFIG_ARCH_OMAP1) &&		\
+		!defined(CONFIG_ARCH_OMAP15XX) &&	\
+		!defined(CONFIG_ARCH_OMAP16XX) &&	\
+		!defined(CONFIG_ARCH_OMAP730) &&	\
+		!defined(CONFIG_ARCH_OMAP850) &&	\

Quoted 8 lines

arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1331
Quoted 5 lines
arch/arm/plat-omap/gpio.c +5 -0
+		!defined(CONFIG_ARCH_OMAP2) &&	\
+		!defined(CONFIG_ARCH_OMAP3) &&	\
+		!defined(CONFIG_ARCH_OMAP4)
+
+#error "Incorrect arch configuration"
This is not required. If the architecture is not one of the above mentioned, gpio_irq_handler() will not be used/called at all.

This could be removed.

Quoted 3 lines

Also all the possible gpio methods for a given OMAP architecture are already considered with "#ifdef"s and (bank->method) checks in gpio_irq_handler().

It is not cover all cased, e.g. for OMAP4 arch:
....

	#if defined(CONFIG_ARCH_OMAP4)
		if (bank->method == METHOD_GPIO_44XX)
			isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0;
	#endif

.....

If (bank->method != METHOD_GPIO_44XX) then isr_reg will be NULL. This should not happen, but potentially may have place.

Quoted 9 lines

arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1336
Quoted 6 lines
arch/arm/plat-omap/gpio.c +5 -0
+
+#endif
+
+	if (WARN_ON(!isr_reg))
+		goto exit;
For the above mentioned reason, this isr_reg would be non-NULL. Have you observed this error anytime?

I did not see this bug, this is potentially may happen.

Quoted 4 lines

Also, the omap-gpio code has similar code spread all over and has to be anyway cleaned-up. Is there any reason why gpio_irq_handler() alone is addressed in this patch?

Here "isr_reg" is used later in code and may cause oops if it is NULL.

Quoted 17 lines

Quoted 16 lines

arch/arm/plat-omap/gpio.c +1 -0

+
 	while(1) {
 		u32 isr_saved, level_mask = 0;

arch/arm/plat-omap/gpio.c +1 -1

 		u32 enabled;

@@ -1377,6 +1394,7 @@ static void gpio_irq_handler(unsigned int irq,
 struct irq_desc *desc)
 	configured, we must unmask the bank interrupt only after
 	handler(s) are executed in order to avoid spurious bank
 	interrupt */
+exit:
 	if (!unmasked)
 		desc->chip->unmask(irq);
 
--

1.6.3.3

Varadarajan, Charulatha

2010-10-13 11:50 UTC #4 lore raw

<<snip>>

Quoted 51 lines

Quoted 8 lines
Quoted 6 lines
From: Evgeny Kuznetsov <[email protected]>
Value of "isr_reg" pointer is depend on configuration and GPIO method. Potentially it may have NULL value and it is dereferenced later in code. If pointer is NULL there is some kernel issue.
Can you elaborate?
"isr_reg" should not be NULL. But if it is NULL then there is kernel bug. And WARN_ON() used to show it.
I did not see this bug, this is potentially may happen.
Quoted 25 lines
Quoted 25 lines
Warning and exit from function are added in this case. Also compilation check is added for correct architecture configuration.
Signed-off-by Evgeny Kuznetsov <[email protected]>
---
 arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)
arch/arm/plat-omap/gpio.c +6 -0
index c05c653..d04913c 100644
@@ -1318,6 +1318,23 @@ static void gpio_irq_handler(unsigned int irq,
 struct irq_desc *desc)
 	if (bank->method == METHOD_GPIO_44XX)
 		isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0;
 #endif
+
+#if !defined(CONFIG_ARCH_OMAP1) &&		\
+		!defined(CONFIG_ARCH_OMAP15XX) &&	\
+		!defined(CONFIG_ARCH_OMAP16XX) &&	\
+		!defined(CONFIG_ARCH_OMAP730) &&	\
+		!defined(CONFIG_ARCH_OMAP850) &&	\
Quoted 8 lines
arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1331
Quoted 5 lines
arch/arm/plat-omap/gpio.c +5 -0
+		!defined(CONFIG_ARCH_OMAP2) &&	\
+		!defined(CONFIG_ARCH_OMAP3) &&	\
+		!defined(CONFIG_ARCH_OMAP4)
+
+#error "Incorrect arch configuration"
This is not required. If the architecture is not one of the above mentioned, gpio_irq_handler() will not be used/called at all.
This could be removed.
Quoted 3 lines
Also all the possible gpio methods for a given OMAP architecture are already considered with "#ifdef"s and (bank->method) checks in gpio_irq_handler().
It is not cover all cased, e.g. for OMAP4 arch:
....

Quoted 8 lines

	#if defined(CONFIG_ARCH_OMAP4)
		if (bank->method == METHOD_GPIO_44XX)
			isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0;
	#endif
.....
If (bank->method != METHOD_GPIO_44XX) then isr_reg will be NULL. This should not happen, but potentially may have place.

When would it fail? If it is CONFIG_ARCH_OMAP4, the gpio method can only be METHOD_GPIO_44XX. Else if it is for some other OMAP architecture, the gpio_method is similarly taken care. So this cannot happen.

Quoted 34 lines

Quoted 10 lines
arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1336
Quoted 7 lines
arch/arm/plat-omap/gpio.c +5 -0
+
+#endif
+
+	if (WARN_ON(!isr_reg))
+		goto exit;
For the above mentioned reason, this isr_reg would be non-NULL. Have you observed this error anytime?
I did not see this bug, this is potentially may happen.
Quoted 4 lines
Also, the omap-gpio code has similar code spread all over and has to be anyway cleaned-up. Is there any reason why gpio_irq_handler() alone is addressed in this patch?
Here "isr_reg" is used later in code and may cause oops if it is NULL.
Quoted 17 lines
Quoted 16 lines
arch/arm/plat-omap/gpio.c +1 -0
+
 	while(1) {
 		u32 isr_saved, level_mask = 0;
arch/arm/plat-omap/gpio.c +1 -1
 		u32 enabled;
@@ -1377,6 +1394,7 @@ static void gpio_irq_handler(unsigned int irq,
 struct irq_desc *desc)
 	configured, we must unmask the bank interrupt only after
 	handler(s) are executed in order to avoid spurious bank
 	interrupt */
+exit:
 	if (!unmasked)
 		desc->chip->unmask(irq);
 
--
1.6.3.3

Evgeny Kuznetsov

author

2010-10-13 12:09 UTC #5 lore raw

Quoted 60 lines · Charulatha

<<snip>>
Quoted 57 lines
Quoted 8 lines
Quoted 6 lines
From: Evgeny Kuznetsov <[email protected]>
Value of "isr_reg" pointer is depend on configuration and GPIO method. Potentially it may have NULL value and it is dereferenced later in code. If pointer is NULL there is some kernel issue.
Can you elaborate?
"isr_reg" should not be NULL. But if it is NULL then there is kernel bug. And WARN_ON() used to show it.
I did not see this bug, this is potentially may happen.
Quoted 25 lines
Quoted 25 lines
Warning and exit from function are added in this case. Also compilation check is added for correct architecture configuration.
Signed-off-by Evgeny Kuznetsov <[email protected]>
---
 arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)
arch/arm/plat-omap/gpio.c +6 -0
index c05c653..d04913c 100644
@@ -1318,6 +1318,23 @@ static void gpio_irq_handler(unsigned int irq,
 struct irq_desc *desc)
 	if (bank->method == METHOD_GPIO_44XX)
 		isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0;
 #endif
+
+#if !defined(CONFIG_ARCH_OMAP1) &&		\
+		!defined(CONFIG_ARCH_OMAP15XX) &&	\
+		!defined(CONFIG_ARCH_OMAP16XX) &&	\
+		!defined(CONFIG_ARCH_OMAP730) &&	\
+		!defined(CONFIG_ARCH_OMAP850) &&	\
Quoted 8 lines
arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1331
Quoted 5 lines
arch/arm/plat-omap/gpio.c +5 -0
+		!defined(CONFIG_ARCH_OMAP2) &&	\
+		!defined(CONFIG_ARCH_OMAP3) &&	\
+		!defined(CONFIG_ARCH_OMAP4)
+
+#error "Incorrect arch configuration"
This is not required. If the architecture is not one of the above mentioned, gpio_irq_handler() will not be used/called at all.
This could be removed.
Quoted 3 lines
Also all the possible gpio methods for a given OMAP architecture are already considered with "#ifdef"s and (bank->method) checks in gpio_irq_handler().
It is not cover all cased, e.g. for OMAP4 arch:
....
	#if defined(CONFIG_ARCH_OMAP4)
		if (bank->method == METHOD_GPIO_44XX)
			isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0;
	#endif
.....

Quoted 6 lines

Quoted 2 lines
If (bank->method != METHOD_GPIO_44XX) then isr_reg will be NULL. This should not happen, but potentially may have place.
When would it fail? If it is CONFIG_ARCH_OMAP4, the gpio method can only be METHOD_GPIO_44XX. Else if it is for some other OMAP architecture, the gpio_method is similarly taken care. So this cannot happen.

This is similar check as e.g. in _enable_gpio_irqbank() there default case for bank->methos switch is WARN_ON(1).
Just to warn in case.

Quoted 36 lines

Quoted 35 lines
Quoted 11 lines
arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1336
Quoted 8 lines
arch/arm/plat-omap/gpio.c +5 -0
+
+#endif
+
+	if (WARN_ON(!isr_reg))
+		goto exit;
For the above mentioned reason, this isr_reg would be non-NULL. Have you observed this error anytime?
I did not see this bug, this is potentially may happen.
Quoted 4 lines
Also, the omap-gpio code has similar code spread all over and has to be anyway cleaned-up. Is there any reason why gpio_irq_handler() alone is addressed in this patch?
Here "isr_reg" is used later in code and may cause oops if it is NULL.
Quoted 17 lines
Quoted 16 lines
arch/arm/plat-omap/gpio.c +2 -1
+
 	while(1) {
 		u32 isr_saved, level_mask = 0;
 		u32 enabled;
@@ -1377,6 +1394,7 @@ static void gpio_irq_handler(unsigned int irq,
 struct irq_desc *desc)
 	configured, we must unmask the bank interrupt only after
 	handler(s) are executed in order to avoid spurious bank
 	interrupt */
+exit:
 	if (!unmasked)
 		desc->chip->unmask(irq);
 
--
1.6.3.3

Evgeny Kuznetsov

author

2010-10-15 06:16 UTC #6 lore raw

Quoted 64 lines · Evgeny Kuznetsov

Quoted 63 lines · Charulatha
<<snip>>
Quoted 59 lines
Quoted 8 lines
Quoted 6 lines
From: Evgeny Kuznetsov <[email protected]>
Value of "isr_reg" pointer is depend on configuration and GPIO method. Potentially it may have NULL value and it is dereferenced later in code. If pointer is NULL there is some kernel issue.
Can you elaborate?
"isr_reg" should not be NULL. But if it is NULL then there is kernel bug. And WARN_ON() used to show it.
I did not see this bug, this is potentially may happen.
Quoted 25 lines
Quoted 25 lines
Warning and exit from function are added in this case. Also compilation check is added for correct architecture configuration.
Signed-off-by Evgeny Kuznetsov <[email protected]>
---
 arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)
arch/arm/plat-omap/gpio.c +6 -0
index c05c653..d04913c 100644
@@ -1318,6 +1318,23 @@ static void gpio_irq_handler(unsigned int irq,
 struct irq_desc *desc)
 	if (bank->method == METHOD_GPIO_44XX)
 		isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0;
 #endif
+
+#if !defined(CONFIG_ARCH_OMAP1) &&		\
+		!defined(CONFIG_ARCH_OMAP15XX) &&	\
+		!defined(CONFIG_ARCH_OMAP16XX) &&	\
+		!defined(CONFIG_ARCH_OMAP730) &&	\
+		!defined(CONFIG_ARCH_OMAP850) &&	\
Quoted 8 lines
arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1331
Quoted 5 lines
arch/arm/plat-omap/gpio.c +5 -0
+		!defined(CONFIG_ARCH_OMAP2) &&	\
+		!defined(CONFIG_ARCH_OMAP3) &&	\
+		!defined(CONFIG_ARCH_OMAP4)
+
+#error "Incorrect arch configuration"
This is not required. If the architecture is not one of the above mentioned, gpio_irq_handler() will not be used/called at all.
This could be removed.
Quoted 3 lines
Also all the possible gpio methods for a given OMAP architecture are already considered with "#ifdef"s and (bank->method) checks in gpio_irq_handler().
It is not cover all cased, e.g. for OMAP4 arch:
....
	#if defined(CONFIG_ARCH_OMAP4)
		if (bank->method == METHOD_GPIO_44XX)
			isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0;
	#endif
.....
If (bank->method != METHOD_GPIO_44XX) then isr_reg will be NULL. This should not happen, but potentially may have place.

Quoted 6 lines

Quoted 3 lines
When would it fail? If it is CONFIG_ARCH_OMAP4, the gpio method can only be METHOD_GPIO_44XX. Else if it is for some other OMAP architecture, the gpio_method is similarly taken care. So this cannot happen.
This is similar check as e.g. in _enable_gpio_irqbank() there default case for bank->methos switch is WARN_ON(1).
Just to warn in case.

Hi Tony,

It is not a bug fix, it is just check to prevent potential issues. Used to warn in case of bug and prevent kernel oops. Check added only here (not all gpio code cleanup) since here it could cause kernel opps.

Would you apply patch if I will leave only if condition section in patch?

	if (WARN_ON(!isr_reg))
		goto exit;

If yes, I'll resend v3 patch.

Thanks,
Regards,
Evgeny

Quoted 37 lines

Quoted 36 lines
Quoted 35 lines
Quoted 11 lines
arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1336
Quoted 8 lines
arch/arm/plat-omap/gpio.c +5 -0
+
+#endif
+
+	if (WARN_ON(!isr_reg))
+		goto exit;
For the above mentioned reason, this isr_reg would be non-NULL. Have you observed this error anytime?
I did not see this bug, this is potentially may happen.
Quoted 4 lines
Also, the omap-gpio code has similar code spread all over and has to be anyway cleaned-up. Is there any reason why gpio_irq_handler() alone is addressed in this patch?
Here "isr_reg" is used later in code and may cause oops if it is NULL.
Quoted 17 lines
Quoted 16 lines
arch/arm/plat-omap/gpio.c +1 -0
+
 	while(1) {
 		u32 isr_saved, level_mask = 0;
arch/arm/plat-omap/gpio.c +1 -1
 		u32 enabled;
@@ -1377,6 +1394,7 @@ static void gpio_irq_handler(unsigned int irq,
 struct irq_desc *desc)
 	configured, we must unmask the bank interrupt only after
 	handler(s) are executed in order to avoid spurious bank
 	interrupt */
+exit:
 	if (!unmasked)
 		desc->chip->unmask(irq);
 
--
1.6.3.3

Tony Lindgren

2010-10-18 23:11 UTC #7 lore raw

Quoted 8 lines · Evgeny Kuznetsov

Quoted 1 line · Evgeny Kuznetsov
On Wed, 2010-10-13 at 17:20 +0530, ext Varadarajan, Charulatha wrote:
Hi Tony,
It is not a bug fix, it is just check to prevent potential issues. Used to warn in case of bug and prevent kernel oops. Check added only here (not all gpio code cleanup) since here it could cause kernel opps.

Quoted 7 lines

Would you apply patch if I will leave only if condition section in patch?
	if (WARN_ON(!isr_reg))
		goto exit;
If yes, I'll resend v3 patch.

Sounds like a valid check to me.

Tony