On Fri, 9 Nov 2018 19:23:51 +0100 Slawomir Stepien <sst@xxxxxxxxx> wrote: > On lis 03, 2018 10:18, Jonathan Cameron wrote: > > On Mon, 29 Oct 2018 17:47:24 +0100 > > Slawomir Stepien <sst@xxxxxxxxx> 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. >
