| Message ID | 20181019182013.GC20587@x220.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/1] staging: iio: adc: ad7280a: use devm_* APIs | expand |
On Fri, 19 Oct 2018 20:20:13 +0200 Slawomir Stepien <sst@poczta.fm> wrote: > devm_* APIs are device managed and make code simpler. > > Signed-off-by: Slawomir Stepien <sst@poczta.fm> Hi Slawomir, There are some complexities in using the managed allocators, almost always around possible race conditions. See inline. Jonathan > --- > Since v2: > * iio_device_register -> devm_iio_device_register > > Since v1: > * request_threaded_irq -> devm_request_threaded_irq > --- > drivers/staging/iio/adc/ad7280a.c | 61 +++++++++++-------------------- > 1 file changed, 22 insertions(+), 39 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c > index b736275c10f5..770e016c325b 100644 > --- a/drivers/staging/iio/adc/ad7280a.c > +++ b/drivers/staging/iio/adc/ad7280a.c > @@ -8,7 +8,6 @@ > > #include <linux/device.h> > #include <linux/kernel.h> > -#include <linux/slab.h> > #include <linux/sysfs.h> > #include <linux/spi/spi.h> > #include <linux/err.h> > @@ -492,8 +491,8 @@ static int ad7280_channel_init(struct ad7280_state *st) > { > int dev, ch, cnt; > > - st->channels = kcalloc((st->slave_num + 1) * 12 + 2, > - sizeof(*st->channels), GFP_KERNEL); > + st->channels = devm_kcalloc(&st->spi->dev, (st->slave_num + 1) * 12 + 2, > + sizeof(*st->channels), GFP_KERNEL); > if (!st->channels) > return -ENOMEM; > > @@ -553,9 +552,9 @@ static int ad7280_attr_init(struct ad7280_state *st) > { > int dev, ch, cnt; > > - st->iio_attr = kcalloc(2, sizeof(*st->iio_attr) * > - (st->slave_num + 1) * AD7280A_CELLS_PER_DEV, > - GFP_KERNEL); > + st->iio_attr = devm_kcalloc(&st->spi->dev, 2, sizeof(*st->iio_attr) * > + (st->slave_num + 1) * AD7280A_CELLS_PER_DEV, > + GFP_KERNEL); > if (!st->iio_attr) > return -ENOMEM; > > @@ -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... > > return IRQ_HANDLED; > } > @@ -909,48 +909,38 @@ static int ad7280_probe(struct spi_device *spi) > > ret = ad7280_attr_init(st); > if (ret < 0) > - goto error_free_channels; > + return ret; > > - ret = iio_device_register(indio_dev); > + ret = devm_iio_device_register(&spi->dev, indio_dev); > if (ret) > - goto error_free_attr; > + return ret; > > if (spi->irq > 0) { > ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, > AD7280A_ALERT, 1, > AD7280A_ALERT_RELAY_SIG_CHAIN_DOWN); > if (ret) > - goto error_unregister; > + return ret; > > ret = ad7280_write(st, ad7280a_devaddr(st->slave_num), > AD7280A_ALERT, 0, > AD7280A_ALERT_GEN_STATIC_HIGH | > (pdata->chain_last_alert_ignore & 0xF)); > if (ret) > - goto error_unregister; > - > - ret = request_threaded_irq(spi->irq, > - NULL, > - ad7280_event_handler, > - IRQF_TRIGGER_FALLING | > - IRQF_ONESHOT, > - indio_dev->name, > - indio_dev); > + return ret; > + > + ret = devm_request_threaded_irq(&spi->dev, spi->irq, > + NULL, > + ad7280_event_handler, > + IRQF_TRIGGER_FALLING | > + IRQF_ONESHOT, > + indio_dev->name, > + indio_dev); > if (ret) > - goto error_unregister; > + return ret; > } > > return 0; > -error_unregister: > - iio_device_unregister(indio_dev); > - > -error_free_attr: > - kfree(st->iio_attr); > - > -error_free_channels: > - kfree(st->channels); > - > - return ret; > } > > 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... > > - kfree(st->channels); > - kfree(st->iio_attr); These two are obviously fine as nothing happens after them anyway! > - > return 0; > } >
On paź 21, 2018 14:26, Jonathan Cameron wrote: > On Fri, 19 Oct 2018 20:20:13 +0200 > Slawomir Stepien <sst@poczta.fm> wrote: > > > devm_* APIs are device managed and make code simpler. > > > > Signed-off-by: Slawomir Stepien <sst@poczta.fm> > > 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?
On Tue, 23 Oct 2018 15:32:34 +0200 Slawomir Stepien <sst@poczta.fm> wrote: > On paź 21, 2018 14:26, Jonathan Cameron wrote: > > On Fri, 19 Oct 2018 20:20:13 +0200 > > Slawomir Stepien <sst@poczta.fm> wrote: > > > > > devm_* APIs are device managed and make code simpler. > > > > > > Signed-off-by: Slawomir Stepien <sst@poczta.fm> > > > > 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? Yes. That's correct. > > 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). True. In the case an irq being specified but not probing successfully we will fail the probe and put the device into a power down state. However, to my mind that's the right thing to do anyway. I can't see why we would want the device powered up having decided to abandon the attempt to load a driver for it? (am I missing something?) The more 'interesting' question is why we are registering the interrupts after iio_device_register in the first place. We have exposed our userspace interfaces, but not yet an interrupt that I assume has something to do with them? iio_device_register should almost always be the last thing run in probe. > > 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? > Definitely not this one. The only rare case for manually using the counter parts to the devm_ setup functions is to replace some data or configuration rather to manually unwind the steps for some error path. Thanks, Jonathan
Hi On paź 28, 2018 12:16, Jonathan Cameron wrote: > > > > 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? > Yes. That's correct. > > > > 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). > True. In the case an irq being specified but not probing successfully we will > fail the probe and put the device into a power down state. However, to my > mind that's the right thing to do anyway. I can't see why we would want > the device powered up having decided to abandon the attempt to load a driver > for it? (am I missing something?) I'll send a patch with this action. > The more 'interesting' question is why we are registering the interrupts > after iio_device_register in the first place. We have exposed our userspace > interfaces, but not yet an interrupt that I assume has something to do with them? > > iio_device_register should almost always be the last thing run in probe. I've looked at the data sheet and the code and concluded that the order is OK. Why? The irq handler can only fire after conversion is completed. The conversion can start only in two ways: (1) falling edge of CNVST input (default) which we don't control (2) rising edge of CS, which we control Since we only using the 2nd option, then it is wise to allow users to have CNVST connected and going down, before any readout of the values using this driver (this will change the CS). This way we will not loose any alert about UV or OV. In case we have irq handler ready before iio_device_register, then I assume that we might loose alert change event. Maybe it's hard (timing), but I think it's possible. What do you think? > > 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? > > > Definitely not this one. The only rare case for manually using the > counter parts to the devm_ setup functions is to replace some data or > configuration rather to manually unwind the steps for some error path. OK, got it!
On Mon, 29 Oct 2018 17:47:24 +0100 Slawomir Stepien <sst@poczta.fm> wrote: > Hi > > On paź 28, 2018 12:16, Jonathan Cameron wrote: > > > > > 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? > > Yes. That's correct. > > > > > > 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). > > True. In the case an irq being specified but not probing successfully we will > > fail the probe and put the device into a power down state. However, to my > > mind that's the right thing to do anyway. I can't see why we would want > > the device powered up having decided to abandon the attempt to load a driver > > for it? (am I missing something?) > > I'll send a patch with this action. > > > The more 'interesting' question is why we are registering the interrupts > > after iio_device_register in the first place. We have exposed our userspace > > interfaces, but not yet an interrupt that I assume has something to do with them? > > > > iio_device_register should almost always be the last thing run in probe. > > I've looked at the data sheet and the code and concluded that the order is OK. > Why? The irq handler can only fire after conversion is completed. The conversion > can start only in two ways: > > (1) falling edge of CNVST input (default) which we don't control That's a potential problem. We shouldn't start by default in a mode where interrupts can occur before we are potentially ready for them. They should only be enabled by a specific request from userspace. A quick at the datasheet suggests this is easily done by writing 0 to the alert register and only enabling it on demand. > (2) rising edge of CS, which we control > > Since we only using the 2nd option, then it is wise to allow users to have CNVST > connected and going down, before any readout of the values using this driver > (this will change the CS). This way we will not loose any alert about UV or OV. I agree we are looking at theoretical race, but as I mentioned it's about obviously correct (and general correct ordering) rather than anthing else. In theory we can have very long delay between exposing the interfaces and setting up the interrupt. So it's possible to hit case 2 before we get the interrupt set up. > > In case we have irq handler ready before iio_device_register, then I assume that > we might loose alert change event. Maybe it's hard (timing), but I think it's > possible. > > What do you think? > > > > 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? > > > > > Definitely not this one. The only rare case for manually using the > > counter parts to the devm_ setup functions is to replace some data or > > configuration rather to manually unwind the steps for some error path. > > OK, got it! >
On lis 03, 2018 10:18, Jonathan Cameron wrote: > On Mon, 29 Oct 2018 17:47:24 +0100 > Slawomir Stepien <sst@poczta.fm> wrote: > > On paź 28, 2018 12:16, Jonathan Cameron wrote: > > > > > > 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? > > > Yes. That's correct. > > > > > > > > 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). > > > True. In the case an irq being specified but not probing successfully we will > > > fail the probe and put the device into a power down state. However, to my > > > mind that's the right thing to do anyway. I can't see why we would want > > > the device powered up having decided to abandon the attempt to load a driver > > > for it? (am I missing something?) > > > > I'll send a patch with this action. > > > > > The more 'interesting' question is why we are registering the interrupts > > > after iio_device_register in the first place. We have exposed our userspace > > > interfaces, but not yet an interrupt that I assume has something to do with them? > > > > > > iio_device_register should almost always be the last thing run in probe. > > > > I've looked at the data sheet and the code and concluded that the order is OK. > > Why? The irq handler can only fire after conversion is completed. The conversion > > can start only in two ways: > > > > (1) falling edge of CNVST input (default) which we don't control > That's a potential problem. We shouldn't start by default in a mode where > interrupts can occur before we are potentially ready for them. They should > only be enabled by a specific request from userspace. > A quick at the datasheet suggests this is easily done by writing 0 to the > alert register and only enabling it on demand. Do I understand this correctly: the interrupts should be enabled on user request, for example by writing 1 to additional iio_dev_attr, and disabled when writing 0 to that file? Should that also be like this when using device tree node with interupt properties (e.g. interrupts) for that device (also working on that)? > > (2) rising edge of CS, which we control > > > > Since we only using the 2nd option, then it is wise to allow users to have CNVST > > connected and going down, before any readout of the values using this driver > > (this will change the CS). This way we will not loose any alert about UV or OV. > I agree we are looking at theoretical race, but as I mentioned it's about > obviously correct (and general correct ordering) rather than anthing else. > In theory we can have very long delay between exposing the interfaces and > setting up the interrupt. So it's possible to hit case 2 before we get > the interrupt set up.
On Fri, 9 Nov 2018 19:23:51 +0100 Slawomir Stepien <sst@poczta.fm> wrote: > On lis 03, 2018 10:18, Jonathan Cameron wrote: > > On Mon, 29 Oct 2018 17:47:24 +0100 > > Slawomir Stepien <sst@poczta.fm> wrote: > > > On paź 28, 2018 12:16, Jonathan Cameron wrote: > > > > > > > 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? > > > > Yes. That's correct. > > > > > > > > > > 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). > > > > True. In the case an irq being specified but not probing successfully we will > > > > fail the probe and put the device into a power down state. However, to my > > > > mind that's the right thing to do anyway. I can't see why we would want > > > > the device powered up having decided to abandon the attempt to load a driver > > > > for it? (am I missing something?) > > > > > > I'll send a patch with this action. > > > > > > > The more 'interesting' question is why we are registering the interrupts > > > > after iio_device_register in the first place. We have exposed our userspace > > > > interfaces, but not yet an interrupt that I assume has something to do with them? > > > > > > > > iio_device_register should almost always be the last thing run in probe. > > > > > > I've looked at the data sheet and the code and concluded that the order is OK. > > > Why? The irq handler can only fire after conversion is completed. The conversion > > > can start only in two ways: > > > > > > (1) falling edge of CNVST input (default) which we don't control > > That's a potential problem. We shouldn't start by default in a mode where > > interrupts can occur before we are potentially ready for them. They should > > only be enabled by a specific request from userspace. > > A quick at the datasheet suggests this is easily done by writing 0 to the > > alert register and only enabling it on demand. > > Do I understand this correctly: the interrupts should be enabled on user > request, for example by writing 1 to additional iio_dev_attr, and disabled when > writing 0 to that file? We should move explicitly from a safe mode (software triggered) to this remote mode. It's a form of trigger, be it one we can't 'see' in software. There are other instances of such hardware triggers (stm32 drivers have some). It isn't a problem, but you need to provide the validation functions to ensure they can't be used to trigger other devices etc. > Should that also be like this when using device tree node with interupt > properties (e.g. interrupts) for that device (also working on that)? The default should never be to trigger capture on a remote signal. That should only be enabled once the driver is ready for it, but a deliberate action from userspace. > > > > (2) rising edge of CS, which we control > > > > > > Since we only using the 2nd option, then it is wise to allow users to have CNVST > > > connected and going down, before any readout of the values using this driver > > > (this will change the CS). This way we will not loose any alert about UV or OV. > > I agree we are looking at theoretical race, but as I mentioned it's about > > obviously correct (and general correct ordering) rather than anthing else. > > In theory we can have very long delay between exposing the interfaces and > > setting up the interrupt. So it's possible to hit case 2 before we get > > the interrupt set up. >
diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c index b736275c10f5..770e016c325b 100644 --- a/drivers/staging/iio/adc/ad7280a.c +++ b/drivers/staging/iio/adc/ad7280a.c @@ -8,7 +8,6 @@ #include <linux/device.h> #include <linux/kernel.h> -#include <linux/slab.h> #include <linux/sysfs.h> #include <linux/spi/spi.h> #include <linux/err.h> @@ -492,8 +491,8 @@ static int ad7280_channel_init(struct ad7280_state *st) { int dev, ch, cnt; - st->channels = kcalloc((st->slave_num + 1) * 12 + 2, - sizeof(*st->channels), GFP_KERNEL); + st->channels = devm_kcalloc(&st->spi->dev, (st->slave_num + 1) * 12 + 2, + sizeof(*st->channels), GFP_KERNEL); if (!st->channels) return -ENOMEM; @@ -553,9 +552,9 @@ static int ad7280_attr_init(struct ad7280_state *st) { int dev, ch, cnt; - st->iio_attr = kcalloc(2, sizeof(*st->iio_attr) * - (st->slave_num + 1) * AD7280A_CELLS_PER_DEV, - GFP_KERNEL); + st->iio_attr = devm_kcalloc(&st->spi->dev, 2, sizeof(*st->iio_attr) * + (st->slave_num + 1) * AD7280A_CELLS_PER_DEV, + GFP_KERNEL); if (!st->iio_attr) return -ENOMEM; @@ -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); return IRQ_HANDLED; } @@ -909,48 +909,38 @@ static int ad7280_probe(struct spi_device *spi) ret = ad7280_attr_init(st); if (ret < 0) - goto error_free_channels; + return ret; - ret = iio_device_register(indio_dev); + ret = devm_iio_device_register(&spi->dev, indio_dev); if (ret) - goto error_free_attr; + return ret; if (spi->irq > 0) { ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_ALERT, 1, AD7280A_ALERT_RELAY_SIG_CHAIN_DOWN); if (ret) - goto error_unregister; + return ret; ret = ad7280_write(st, ad7280a_devaddr(st->slave_num), AD7280A_ALERT, 0, AD7280A_ALERT_GEN_STATIC_HIGH | (pdata->chain_last_alert_ignore & 0xF)); if (ret) - goto error_unregister; - - ret = request_threaded_irq(spi->irq, - NULL, - ad7280_event_handler, - IRQF_TRIGGER_FALLING | - IRQF_ONESHOT, - indio_dev->name, - indio_dev); + return ret; + + ret = devm_request_threaded_irq(&spi->dev, spi->irq, + NULL, + ad7280_event_handler, + IRQF_TRIGGER_FALLING | + IRQF_ONESHOT, + indio_dev->name, + indio_dev); if (ret) - goto error_unregister; + return ret; } return 0; -error_unregister: - iio_device_unregister(indio_dev); - -error_free_attr: - kfree(st->iio_attr); - -error_free_channels: - kfree(st->channels); - - return ret; } 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); - kfree(st->channels); - kfree(st->iio_attr); - return 0; }
devm_* APIs are device managed and make code simpler. Signed-off-by: Slawomir Stepien <sst@poczta.fm> --- Since v2: * iio_device_register -> devm_iio_device_register Since v1: * request_threaded_irq -> devm_request_threaded_irq --- drivers/staging/iio/adc/ad7280a.c | 61 +++++++++++-------------------- 1 file changed, 22 insertions(+), 39 deletions(-)