[PATCH v3 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[PATCH v3 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Slawomir Stepien]

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(-)

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;
 }
 
-- 
2.19.1

Re: [PATCH v3 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Jonathan Cameron]

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;
>  }
>  

Re: [PATCH v3 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Slawomir Stepien]

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?

-- 
Slawomir Stepien

Re: [PATCH v3 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Jonathan Cameron]

On Tue, 23 Oct 2018 15:32:34 +0200
Slawomir Stepien <sst@poczta.fm> wrote:

> On pa=C5=BA 21, 2018 14:26, Jonathan Cameron wrote:
> > On Fri, 19 Oct 2018 20:20:13 +0200
> > Slawomir Stepien <sst@poczta.fm> wrote:
> >  =20
> > > devm_* APIs are device managed and make code simpler.
> > >=20
> > > Signed-off-by: Slawomir Stepien <sst@poczta.fm> =20
> >=20
> > Hi Slawomir,
> >=20
> > There are some complexities in using the managed allocators, almost
> > always around possible race conditions.  See inline. =20
>=20
> Thank you so much for pointing the problems!
>=20
> > > @@ -692,7 +691,8 @@ static irqreturn_t ad7280_event_handler(int irq, =
void *private)
> > >  	unsigned int *channels;
> > >  	int i, ret;
> > > =20
> > > -	channels =3D kcalloc(st->scan_cnt, sizeof(*channels), GFP_KERNEL);
> > > +	channels =3D devm_kcalloc(&st->spi->dev, st->scan_cnt, sizeof(*chan=
nels),
> > > +				GFP_KERNEL);
> > >  	if (!channels)
> > >  		return IRQ_HANDLED;
> > > =20
> > > @@ -744,7 +744,7 @@ static irqreturn_t ad7280_event_handler(int irq, =
void *private)
> > >  	}
> > > =20
> > >  out:
> > > -	kfree(channels);
> > > +	devm_kfree(&st->spi->dev, channels); =20
> >=20
> > Now this I really don't want to see.
> > Using the managed framework is far from free. Please don't do it when t=
he
> > normal path is to free the buffer like this... =20
>=20
> OK
>=20
> > >  	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 =3D spi_get_drvdata(spi);
> > >  	struct ad7280_state *st =3D iio_priv(indio_dev);
> > > =20
> > > -	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); =20
> > 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?
> >=20
> > 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.
> >=20
> > 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... =20
>=20
> Yes I understand the problem here. I have some questions regarding
> devm_add_action that might solve the problem here:
>=20
> 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 co=
rrect.
> Am I thinking correctly here?
Yes.  That's correct.
>=20
> Please note that doing the action from probe is changing the current beha=
viour
> of the driver - we will put the device into power-down software state als=
o from
> probe() (if irq setup fails).
True. In the case an irq being specified but not probing successfully we wi=
ll
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.

>=20
> 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?
>=20
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

Re: [PATCH v3 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Slawomir Stepien]

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!

-- 
Slawomir Stepien

Re: [PATCH v3 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Jonathan Cameron]

On Mon, 29 Oct 2018 17:47:24 +0100
Slawomir Stepien <sst@poczta.fm> wrote:

> Hi
>=20
> On pa=C5=BA 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 =3D spi_get_drvdata(spi);
> > > > >  	struct ad7280_state *st =3D iio_priv(indio_dev);
> > > > > =20
> > > > > -	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);   =20
> > > > 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?
> > > >=20
> > > > 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.
> > > >=20
> > > > 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...   =
=20
> > >=20
> > > Yes I understand the problem here. I have some questions regarding
> > > devm_add_action that might solve the problem here:
> > >=20
> > > 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 wil=
l be called
> > > after the call to devm_iio_device_unreg. Other order will be still no=
t correct.
> > > Am I thinking correctly here? =20
> > Yes.  That's correct. =20
> > >=20
> > > 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). =20
> > True. In the case an irq being specified but not probing successfully w=
e 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 d=
river
> > for it?  (am I missing something?) =20
>=20
> I'll send a patch with this action.
>=20
> > The more 'interesting' question is why we are registering the interrupts
> > after iio_device_register in the first place.  We have exposed our user=
space
> > interfaces, but not yet an interrupt that I assume has something to do =
with them?
> >=20
> > iio_device_register should almost always be the last thing run in probe=
. =20
>=20
> I've looked at the data sheet and the code and concluded that the order i=
s OK.
> Why? The irq handler can only fire after conversion is completed. The con=
version
> can start only in two ways:
>=20
> (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
>=20
> Since we only using the 2nd option, then it is wise to allow users to hav=
e CNVST
> connected and going down, before any readout of the values using this dri=
ver
> (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.

>=20
> In case we have irq handler ready before iio_device_register, then I assu=
me that
> we might loose alert change event. Maybe it's hard (timing), but I think =
it's
> possible.
>=20
> What do you think?
>=20
> > > 2. devm_iio_device_unregister from what I see could be used here in p=
lace of
> > > iio_device_unregister. Maybe that is the best way to go?
> > >  =20
> > 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. =
=20
>=20
> OK, got it!
>=20

Re: [PATCH v3 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Slawomir Stepien]

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.

-- 
Slawomir Stepien

Re: [PATCH v3 1/1] staging: iio: adc: ad7280a: use devm_* APIs

[Jonathan Cameron]

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: =20
> > > On pa=C5=BA 28, 2018 12:16, Jonathan Cameron wrote: =20
> > > > > > >  static int ad7280_remove(struct spi_device *spi)
> > > > > > > @@ -958,16 +948,9 @@ static int ad7280_remove(struct spi_devi=
ce *spi)
> > > > > > >  	struct iio_dev *indio_dev =3D spi_get_drvdata(spi);
> > > > > > >  	struct ad7280_state *st =3D iio_priv(indio_dev);
> > > > > > > =20
> > > > > > > -	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);     =20
> > > > > > 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?
> > > > > >=20
> > > > > > 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.
> > > > > >=20
> > > > > > 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_a=
ction
> > > > > > approach to ensure the tear down is in the right order though..=
.     =20
> > > > >=20
> > > > > Yes I understand the problem here. I have some questions regarding
> > > > > devm_add_action that might solve the problem here:
> > > > >=20
> > > > > 1. My understanding is that the action has to be added on the dev=
res 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 stil=
l not correct.
> > > > > Am I thinking correctly here?   =20
> > > > Yes.  That's correct.   =20
> > > > >=20
> > > > > Please note that doing the action from probe is changing the curr=
ent behaviour
> > > > > of the driver - we will put the device into power-down software s=
tate also from
> > > > > probe() (if irq setup fails).   =20
> > > > True. In the case an irq being specified but not probing successful=
ly 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?)   =20
> > >=20
> > > I'll send a patch with this action.
> > >  =20
> > > > The more 'interesting' question is why we are registering the inter=
rupts
> > > > 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?
> > > >=20
> > > > iio_device_register should almost always be the last thing run in p=
robe.   =20
> > >=20
> > > I've looked at the data sheet and the code and concluded that the ord=
er is OK.
> > > Why? The irq handler can only fire after conversion is completed. The=
 conversion
> > > can start only in two ways:
> > >=20
> > > (1) falling edge of CNVST input (default) which we don't control =20
> > That's a potential problem.  We shouldn't start by default in a mode wh=
ere
> > interrupts can occur before we are potentially ready for them.  They sh=
ould
> > only be enabled by a specific request from userspace.
> > A quick at the datasheet suggests this is easily done by writing 0 to t=
he
> > alert register and only enabling it on demand. =20
>=20
> Do I understand this correctly: the interrupts should be enabled on user
> request, for example by writing 1 to additional iio_dev_attr, and disable=
d when
> writing 0 to that file?
We should move explicitly from a safe mode (software triggered) to this rem=
ote
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 sh=
ould
only be enabled once the driver is ready for it, but a deliberate action fr=
om
userspace.

>=20
> > > (2) rising edge of CS, which we control
> > >=20
> > > 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. =20
> > I agree we are looking at theoretical race, but as I mentioned it's abo=
ut
> > obviously correct (and general correct ordering) rather than anthing el=
se.
> > In theory we can have very long delay between exposing the interfaces a=
nd
> > setting up the interrupt.  So it's possible to hit case 2 before we get
> > the interrupt set up. =20
>=20