On Sun, 22 Mar 2020 23:15:13 +0200
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Sun, Mar 22, 2020 at 7:14 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > On Tue, 17 Mar 2020 12:18:09 +0200
> > Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> >
> > > With DEBUG_SHIRQ enabled we have a kernel crash
> > >
> > > [ 116.482696] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > >
> > > ...
> > >
> > > [ 116.606571] Call Trace:
> > > [ 116.609023] <IRQ>
> > > [ 116.611047] complete+0x34/0x50
> > > [ 116.614206] bmp085_eoc_irq+0x9/0x10 [bmp280]
> > >
> > > because DEBUG_SHIRQ mechanism fires an IRQ before registration and drivers
> > > ought to be able to handle an interrupt happening before request_irq() returns.
> > >
> > > Fixes: aae953949651 ("iio: pressure: bmp280: add support for BMP085 EOC interrupt")
> > > Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> >
> > Hmm. Nasty corner case but fair enough to fix it up.
> > I guess we should audit other drivers for similar paths...
>
> One can easily test, no device even needed (however in this case I have one)
>
> ...
>
> > > - if (data->use_eoc)
> > > - init_completion(&data->done);
> > > + reinit_completion(&data->done);
> >
> > reinit on the completion when we don't even have an interrupt
> > (hence data->use_eoc == false) seems excessive. Why did
> > you drop the conditional?
>
> It's harmless and gives less code I believe.
> But if you insist I can put it back.
>
I agree it's harmless but breaks the logical flow by doing
something unnecessary so either we need a comment to say that
or easier option, just put the condition back in.
Jonathan
