On paź 21, 2018 14:26, Jonathan Cameron wrote: > On Fri, 19 Oct 2018 20:20:13 +0200 > Slawomir Stepien <sst@xxxxxxxxx> wrote: > > > devm_* APIs are device managed and make code simpler. > > > > Signed-off-by: Slawomir Stepien <sst@xxxxxxxxx> > > Hi Slawomir, > > There are some complexities in using the managed allocators, almost > always around possible race conditions. See inline. Thank you so much for pointing the problems! > > @@ -692,7 +691,8 @@ static irqreturn_t ad7280_event_handler(int irq, void *private) > > unsigned int *channels; > > int i, ret; > > > > - channels = kcalloc(st->scan_cnt, sizeof(*channels), GFP_KERNEL); > > + channels = devm_kcalloc(&st->spi->dev, st->scan_cnt, sizeof(*channels), > > + GFP_KERNEL); > > if (!channels) > > return IRQ_HANDLED; > > > > @@ -744,7 +744,7 @@ static irqreturn_t ad7280_event_handler(int irq, void *private) > > } > > > > out: > > - kfree(channels); > > + devm_kfree(&st->spi->dev, channels); > > Now this I really don't want to see. > Using the managed framework is far from free. Please don't do it when the > normal path is to free the buffer like this... OK > > return IRQ_HANDLED; > > } > > static int ad7280_remove(struct spi_device *spi) > > @@ -958,16 +948,9 @@ static int ad7280_remove(struct spi_device *spi) > > struct iio_dev *indio_dev = spi_get_drvdata(spi); > > struct ad7280_state *st = iio_priv(indio_dev); > > > > - if (spi->irq > 0) > > - free_irq(spi->irq, indio_dev); > > - iio_device_unregister(indio_dev); > > - > > ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1, > > AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb); > So here, you need to think very carefully about what the various > steps are doing. By moving to devm_iio_device_unregister > what difference has it made to the sequence of calls in remove? > > The upshot is you just turned the device off before removing the > interfaces which would allow userspace / kernel consumers to > access the device. A classic race condition that 'might' open > up opportunities for problems. > > Often the reality is that these sorts of races have very minimal > impact, but they do break the cardinal rule that code should be > obviously right (if possible). Hence you can't do this sort > of conversion so simply. You can consider using the devm_add_action > approach to ensure the tear down is in the right order though... Yes I understand the problem here. I have some questions regarding devm_add_action that might solve the problem here: 1. My understanding is that the action has to be added on the devres list before the devm_iio_device_register call, so during unwinding the action will be called after the call to devm_iio_device_unreg. Other order will be still not correct. Am I thinking correctly here? Please note that doing the action from probe is changing the current behaviour of the driver - we will put the device into power-down software state also from probe() (if irq setup fails). 2. devm_iio_device_unregister from what I see could be used here in place of iio_device_unregister. Maybe that is the best way to go? -- Slawomir Stepien
