[PATCHv2 0/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[PATCHv2 0/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Evgeny Kuznetsov]

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

[PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Evgeny Kuznetsov]

From: Evgeny Kuznetsov <ext-eugeny.kuznetsov@nokia.com>

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 <EXT-Eugeny.Kuznetsov@nokia.com>
---
 arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index c05c653..d04913c 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -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);
 
-- 
1.6.3.3

RE: [PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Varadarajan, Charulatha]

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

> Warning and exit from function are added in this case.
> Also compilation check is added for correct architecture
> configuration.
> 
> Signed-off-by: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>
> ---
>  arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index c05c653..d04913c 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -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"

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

> +
> +#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?

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

RE: [PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Evgeny Kuznetsov]

On Wed, 2010-10-13 at 16:25 +0530, ext Varadarajan, Charulatha wrote:
> 
> > -----Original Message-----
> > From: Evgeny Kuznetsov [mailto:EXT-Eugeny.Kuznetsov@nokia.com]
> > Sent: Wednesday, October 13, 2010 3:35 PM
> > To: tony@atomide.com
> > Cc: linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; Balbi, Felipe; linux@arm.linux.org.uk;
> > khilman@deeprootsystems.com; akpm@linux-foundation.org; Varadarajan,
> > Charulatha; tero.kristo@nokia.com; ext-eugeny.kuznetsov@nokia.com
> > Subject: [PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was
> > dereferenced
> > 
> > From: Evgeny Kuznetsov <ext-eugeny.kuznetsov@nokia.com>
> > 
> > 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.
> 
> > Warning and exit from function are added in this case.
> > Also compilation check is added for correct architecture
> > configuration.
> > 
> > Signed-off-by: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>
> > ---
> >  arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
> >  1 files changed, 18 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> > index c05c653..d04913c 100644
> > --- a/arch/arm/plat-omap/gpio.c
> > +++ b/arch/arm/plat-omap/gpio.c
> > @@ -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"
> 
> 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.

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

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

RE: [PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Varadarajan, Charulatha]

<<snip>>

> > >
> > > From: Evgeny Kuznetsov <ext-eugeny.kuznetsov@nokia.com>
> > >
> > > 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.
> >
> > > Warning and exit from function are added in this case.
> > > Also compilation check is added for correct architecture
> > > configuration.
> > >
> > > Signed-off-by: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>
> > > ---
> > >  arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
> > >  1 files changed, 18 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> > > index c05c653..d04913c 100644
> > > --- a/arch/arm/plat-omap/gpio.c
> > > +++ b/arch/arm/plat-omap/gpio.c
> > > @@ -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"
> >
> > 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.
> 
> > 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.

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.

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

RE: [PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Evgeny Kuznetsov]

On Wed, 2010-10-13 at 17:20 +0530, ext Varadarajan, Charulatha wrote:
> 
> <<snip>>
> 
> > > >
> > > > From: Evgeny Kuznetsov <ext-eugeny.kuznetsov@nokia.com>
> > > >
> > > > 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.
> > >
> > > > Warning and exit from function are added in this case.
> > > > Also compilation check is added for correct architecture
> > > > configuration.
> > > >
> > > > Signed-off-by: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>
> > > > ---
> > > >  arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
> > > >  1 files changed, 18 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> > > > index c05c653..d04913c 100644
> > > > --- a/arch/arm/plat-omap/gpio.c
> > > > +++ b/arch/arm/plat-omap/gpio.c
> > > > @@ -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"
> > >
> > > 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.
> > 
> > > 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.
> 
> 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.
> 
> > 
> > >
> > > > +
> > > > +#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.
> > >
> > > 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.
> > >
> > > > +
> > > >  	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
> > >
> > 
> 

RE: [PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Evgeny Kuznetsov]

On Wed, 2010-10-13 at 16:09 +0400, Evgeny Kuznetsov wrote:
> On Wed, 2010-10-13 at 17:20 +0530, ext Varadarajan, Charulatha wrote:
> > 
> > <<snip>>
> > 
> > > > >
> > > > > From: Evgeny Kuznetsov <ext-eugeny.kuznetsov@nokia.com>
> > > > >
> > > > > 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.
> > > >
> > > > > Warning and exit from function are added in this case.
> > > > > Also compilation check is added for correct architecture
> > > > > configuration.
> > > > >
> > > > > Signed-off-by: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>
> > > > > ---
> > > > >  arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
> > > > >  1 files changed, 18 insertions(+), 0 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> > > > > index c05c653..d04913c 100644
> > > > > --- a/arch/arm/plat-omap/gpio.c
> > > > > +++ b/arch/arm/plat-omap/gpio.c
> > > > > @@ -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"
> > > >
> > > > 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.
> > > 
> > > > 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.
> > 
> > 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

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

Re: [PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Tony Lindgren]

* Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com> [101014 23:11]:
> On Wed, 2010-10-13 at 16:09 +0400, Evgeny Kuznetsov wrote:
> > 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.
> 
> 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