lkml.org 
[lkml]   [2010]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRE: [PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced
    From
    Date
    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
    > > >
    > >
    >




    \
     
     \ /
      Last update: 2010-10-13 14:15    [W:0.034 / U:0.140 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site