| Message ID | 20200317101813.30829-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/5] iio: pressure: bmp280: Tolerate IRQ before registering | expand |
On Tue, 17 Mar 2020 12:18:09 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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@linaro.org> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Hmm. Nasty corner case but fair enough to fix it up. I guess we should audit other drivers for similar paths... > --- > drivers/iio/pressure/bmp280-core.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > index 29c209cc1108..5e229b95308e 100644 > --- a/drivers/iio/pressure/bmp280-core.c > +++ b/drivers/iio/pressure/bmp280-core.c > @@ -712,8 +712,7 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas) > unsigned int delay_us; > unsigned int ctrl; > > - 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? Jonathan > > ret = regmap_write(data->regmap, BMP280_REG_CTRL_MEAS, ctrl_meas); > if (ret) > @@ -969,6 +968,9 @@ static int bmp085_fetch_eoc_irq(struct device *dev, > "trying to enforce it\n"); > irq_trig = IRQF_TRIGGER_RISING; > } > + > + init_completion(&data->done); > + > ret = devm_request_threaded_irq(dev, > irq, > bmp085_eoc_irq,
On Sun, Mar 22, 2020 at 7:14 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Tue, 17 Mar 2020 12:18:09 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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@linaro.org> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > 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.
On Sun, 22 Mar 2020 23:15:13 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sun, Mar 22, 2020 at 7:14 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Tue, 17 Mar 2020 12:18:09 +0200 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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@linaro.org> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > 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
On Mon, Mar 23, 2020 at 09:40:18AM +0000, Jonathan Cameron wrote: > On Sun, 22 Mar 2020 23:15:13 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sun, Mar 22, 2020 at 7:14 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > On Tue, 17 Mar 2020 12:18:09 +0200 > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: ... > > > > - 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. I will do this for v2. Thank you for review!
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index 29c209cc1108..5e229b95308e 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -712,8 +712,7 @@ static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas) unsigned int delay_us; unsigned int ctrl; - if (data->use_eoc) - init_completion(&data->done); + reinit_completion(&data->done); ret = regmap_write(data->regmap, BMP280_REG_CTRL_MEAS, ctrl_meas); if (ret) @@ -969,6 +968,9 @@ static int bmp085_fetch_eoc_irq(struct device *dev, "trying to enforce it\n"); irq_trig = IRQF_TRIGGER_RISING; } + + init_completion(&data->done); + ret = devm_request_threaded_irq(dev, irq, bmp085_eoc_irq,
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@linaro.org> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/iio/pressure/bmp280-core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)