On Sun, 2018-09-16 at 12:16 +0100, Jonathan Cameron wrote: > On Thu, 13 Sep 2018 12:51:38 +0300 > Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote: > > > No significant functional changes. This just replaces the code with > > devm_* > > that reduce the driver code, and simplifies some error code paths in > > the > > ad7606_probe() function. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> > > Sometimes there are reasons why devm functions aren't used. They are > almost > always about potential races in remove paths and that is definitely the > case > here. Suggestions inline on how to avoid that problem. > I'll admit that I was a bit naive about this one. But I'll take it as a quick crash-course on this subject and adapt my thinking for these drivers + devm_ APIs. Naturally, I'd like to drop this patch; I'll take a quick look at the initialization sequence, and if I see anything wrong, I'll come back with patch for that issue. Thanks Alex > Thanks, > > Jonathan > > > --- > > drivers/staging/iio/adc/ad7606.c | 25 +++++++++---------------- > > 1 file changed, 9 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/staging/iio/adc/ad7606.c > > b/drivers/staging/iio/adc/ad7606.c > > index 87d5fb073c95..793de92f27ed 100644 > > --- a/drivers/staging/iio/adc/ad7606.c > > +++ b/drivers/staging/iio/adc/ad7606.c > > @@ -458,28 +458,25 @@ int ad7606_probe(struct device *dev, int irq, > > void __iomem *base_address, > > if (ret) > > dev_warn(st->dev, "failed to RESET: no RESET GPIO > > specified\n"); > > > > - ret = request_irq(irq, ad7606_interrupt, IRQF_TRIGGER_FALLING, > > name, > > - indio_dev); > > + ret = devm_request_irq(dev, irq, ad7606_interrupt, > > + IRQF_TRIGGER_FALLING, > > + name, indio_dev); > > You cannot safely mix devm and non devm setup. In this case you end up > with an interesting race around when the regulator is turned off > resulting > in the device being powered down before the interrupt is released. > > Now, that might be fine, but it's really hard to be sure when reviewing > so > as a rule of thumb we never allow non obvious ordering. > > Now, one option for this is to use devm_add_action to automatically call > the cleanup in the right sequence when remove occurs. > > > if (ret) > > goto error_disable_reg; > > > > - ret = iio_triggered_buffer_setup(indio_dev, > > &ad7606_trigger_handler, > > - NULL, NULL); > > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > > + &ad7606_trigger_handler, > > + NULL, NULL); > > if (ret) > > - goto error_free_irq; > > + goto error_disable_reg; > > > > - ret = iio_device_register(indio_dev); > > + ret = devm_iio_device_register(dev, indio_dev); > > if (ret) > > - goto error_unregister_ring; > > + goto error_disable_reg; > > Definitely not on this one. The power will be cut before the interfaces > are removed. That one is definitely going to be a nasty race condition! > > > > > dev_set_drvdata(dev, indio_dev); > > > > return 0; > > -error_unregister_ring: > > - iio_triggered_buffer_cleanup(indio_dev); > > - > > -error_free_irq: > > - free_irq(irq, indio_dev); > > > > error_disable_reg: > > regulator_disable(st->reg); > > @@ -492,10 +489,6 @@ int ad7606_remove(struct device *dev, int irq) > > struct iio_dev *indio_dev = dev_get_drvdata(dev); > > struct ad7606_state *st = iio_priv(indio_dev); > > > > - iio_device_unregister(indio_dev); > > - iio_triggered_buffer_cleanup(indio_dev); > > - > > - free_irq(irq, indio_dev); > > regulator_disable(st->reg); > > > > return 0; > >
