On Wed, 2010-10-13 at 16:25 +0530, ext Varadarajan, Charulatha wrote: > > > -----Original Message----- > > From: Evgeny Kuznetsov [mailto:EXT-Eugeny.Kuznetsov@xxxxxxxxx] > > Sent: Wednesday, October 13, 2010 3:35 PM > > To: tony@xxxxxxxxxxx > > Cc: linux-omap@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm- > > kernel@xxxxxxxxxxxxxxxxxxx; Balbi, Felipe; linux@xxxxxxxxxxxxxxxx; > > khilman@xxxxxxxxxxxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx; Varadarajan, > > Charulatha; tero.kristo@xxxxxxxxx; ext-eugeny.kuznetsov@xxxxxxxxx > > Subject: [PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was > > dereferenced > > > > From: Evgeny Kuznetsov <ext-eugeny.kuznetsov@xxxxxxxxx> > > > > 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@xxxxxxxxx> > > --- > > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
