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

[Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>]

On Wed, 2010-10-06 at 10:33 +0400, Evgeny Kuznetsov wrote:
> On Tue, 2010-10-05 at 08:01 -0700, ext Kevin Hilman wrote:
> > Felipe Balbi <balbi@ti.com> writes:
> > 
> > > Hi,
> > >
> > > On Tue, Oct 05, 2010 at 03:42:10AM -0500, Evgeny Kuznetsov wrote:
> > >>+	if (!isr_reg) {
> > >>+		printk(KERN_ERR "FATAL: Incorrect GPIO method %i\n",
> > >>+				bank->method);
> > >>+		BUG();
> > >>+		}
> > >
> > > this could be simply:
> > >
> > > BUG_ON(!isr_reg);
> > 
> > WARN_ON is better.
> > 
> > A BUG() will panic the kernel and stop everything.  This is not an error
> > condition that should prevent the entire kernel from running.
> 



> 'isr_reg' is dereferenced later in code:
> 	...
> 	isr_saved = isr = __raw_readl(isr_reg) & enabled;
> 	...
> So this will stop kernel anyway.
> I just hoped to help in understanding of issue by log line. WARN_ON
> could be used for this.
> 
> As a variant compilation error could be added, to prevent situation when
> kernel is incorrectly configured.
> E.g.:
> #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
> 
> But there are still cases when 'isr_reg' could have NULL value (if
> 'bank->method' is not equal to configured one).
> 
> Regards,
> Evgeny

If 'isr_reg' is NULL then interrupt could not be handled. We may unmask
the GPIO bank interrupt to continue handle GPIO interrupts for other
lines. And exit handler to prevent kernel oops since 'isr_reg' is
dereferenced later in code(see my message above).

if (WARN_ON(!isr_reg)) {
	desc->chip->unmask(irq);
	return;
	}

One thing I warn about that we could not clear edge sensitive
interrupts:
	_enable_gpio_irqbank(bank, isr_saved & ~level_mask, 0);
	_clear_gpio_irqbank(bank, isr_saved & ~level_mask);
	_enable_gpio_irqbank(bank, isr_saved & ~level_mask, 1);


What do you think?

Evgeny.