On Mon, 29 Oct 2018 17:47:24 +0100 Slawomir Stepien <sst@xxxxxxxxx> 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! >
