[PATCH] staging: iio: ad5933: rework probe to use devm_ function variants

[PATCH] staging: iio: ad5933: rework probe to use devm_ function variants

[Alexandru Ardelean]

This change cleans up the driver's probe function to use only devm_
function variants. This also gets rid of the remove function and moves the
clock & regulator de-initializations to the 'ad5933_cleanup()' callback.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 .../staging/iio/impedance-analyzer/ad5933.c   | 59 ++++++++-----------
 1 file changed, 23 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index af0bcf95ee8a..06a6dcd7883b 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -602,11 +602,12 @@ static const struct iio_buffer_setup_ops ad5933_ring_setup_ops = {
 	.postdisable = ad5933_ring_postdisable,
 };
 
-static int ad5933_register_ring_funcs_and_init(struct iio_dev *indio_dev)
+static int ad5933_register_ring_funcs_and_init(struct device *dev,
+					       struct iio_dev *indio_dev)
 {
 	struct iio_buffer *buffer;
 
-	buffer = iio_kfifo_allocate();
+	buffer = devm_iio_kfifo_allocate(dev);
 	if (!buffer)
 		return -ENOMEM;
 
@@ -676,6 +677,14 @@ static void ad5933_work(struct work_struct *work)
 	}
 }
 
+static void ad5933_cleanup(void *data)
+{
+	struct ad5933_state *st = data;
+
+	clk_disable_unprepare(st->mclk);
+	regulator_disable(st->reg);
+}
+
 static int ad5933_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -703,23 +712,28 @@ static int ad5933_probe(struct i2c_client *client,
 		dev_err(&client->dev, "Failed to enable specified VDD supply\n");
 		return ret;
 	}
+
+	ret = devm_add_action_or_reset(&client->dev, ad5933_cleanup, st);
+	if (ret)
+		return ret;
+
 	ret = regulator_get_voltage(st->reg);
 
 	if (ret < 0)
-		goto error_disable_reg;
+		return ret;
 
 	st->vref_mv = ret / 1000;
 
 	st->mclk = devm_clk_get(&client->dev, "mclk");
 	if (IS_ERR(st->mclk) && PTR_ERR(st->mclk) != -ENOENT) {
 		ret = PTR_ERR(st->mclk);
-		goto error_disable_reg;
+		return ret;
 	}
 
 	if (!IS_ERR(st->mclk)) {
 		ret = clk_prepare_enable(st->mclk);
 		if (ret < 0)
-			goto error_disable_reg;
+			return ret;
 		ext_clk_hz = clk_get_rate(st->mclk);
 	}
 
@@ -742,41 +756,15 @@ static int ad5933_probe(struct i2c_client *client,
 	indio_dev->channels = ad5933_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
 
-	ret = ad5933_register_ring_funcs_and_init(indio_dev);
+	ret = ad5933_register_ring_funcs_and_init(&client->dev, indio_dev);
 	if (ret)
-		goto error_disable_mclk;
+		return ret;
 
 	ret = ad5933_setup(st);
 	if (ret)
-		goto error_unreg_ring;
-
-	ret = iio_device_register(indio_dev);
-	if (ret)
-		goto error_unreg_ring;
-
-	return 0;
-
-error_unreg_ring:
-	iio_kfifo_free(indio_dev->buffer);
-error_disable_mclk:
-	clk_disable_unprepare(st->mclk);
-error_disable_reg:
-	regulator_disable(st->reg);
-
-	return ret;
-}
-
-static int ad5933_remove(struct i2c_client *client)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(client);
-	struct ad5933_state *st = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-	iio_kfifo_free(indio_dev->buffer);
-	regulator_disable(st->reg);
-	clk_disable_unprepare(st->mclk);
+		return ret;
 
-	return 0;
+	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
 static const struct i2c_device_id ad5933_id[] = {
@@ -801,7 +789,6 @@ static struct i2c_driver ad5933_driver = {
 		.of_match_table = ad5933_of_match,
 	},
 	.probe = ad5933_probe,
-	.remove = ad5933_remove,
 	.id_table = ad5933_id,
 };
 module_i2c_driver(ad5933_driver);
-- 
2.17.1

Re: [PATCH] staging: iio: ad5933: rework probe to use devm_ function variants

[Jonathan Cameron]

On Tue, 28 Apr 2020 12:31:28 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change cleans up the driver's probe function to use only devm_
> function variants. This also gets rid of the remove function and moves the
> clock & regulator de-initializations to the 'ad5933_cleanup()' callback.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Basic rule of thumb. Whatever you register with devm_add_action_or_reset
should only cleanup one one thing done in the probe path.
There is almost always a race if you do more than one bit of cleanup
per such callback + it's harder to review as it fails the 'obviously correct
test'.

Jonathan

> ---
>  .../staging/iio/impedance-analyzer/ad5933.c   | 59 ++++++++-----------
>  1 file changed, 23 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index af0bcf95ee8a..06a6dcd7883b 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -602,11 +602,12 @@ static const struct iio_buffer_setup_ops ad5933_ring_setup_ops = {
>  	.postdisable = ad5933_ring_postdisable,
>  };
>  
> -static int ad5933_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> +static int ad5933_register_ring_funcs_and_init(struct device *dev,
> +					       struct iio_dev *indio_dev)
>  {
>  	struct iio_buffer *buffer;
>  
> -	buffer = iio_kfifo_allocate();
> +	buffer = devm_iio_kfifo_allocate(dev);
>  	if (!buffer)
>  		return -ENOMEM;
>  
> @@ -676,6 +677,14 @@ static void ad5933_work(struct work_struct *work)
>  	}
>  }
>  
> +static void ad5933_cleanup(void *data)
> +{
> +	struct ad5933_state *st = data;
> +
> +	clk_disable_unprepare(st->mclk);
> +	regulator_disable(st->reg);

Please do two separate callbacks so that these can be handled
in the correct places.  I.e. you do something then immediately
register the handler to undo it.

Currently you can end up disabling a clock you haven't enabled
(which I am fairly sure will give you an error message).

> +}
> +
>  static int ad5933_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> @@ -703,23 +712,28 @@ static int ad5933_probe(struct i2c_client *client,
>  		dev_err(&client->dev, "Failed to enable specified VDD supply\n");
>  		return ret;
>  	}
> +
> +	ret = devm_add_action_or_reset(&client->dev, ad5933_cleanup, st);
> +	if (ret)
> +		return ret;
> +
>  	ret = regulator_get_voltage(st->reg);
>  
>  	if (ret < 0)
> -		goto error_disable_reg;
> +		return ret;
>  
>  	st->vref_mv = ret / 1000;
>  
>  	st->mclk = devm_clk_get(&client->dev, "mclk");
>  	if (IS_ERR(st->mclk) && PTR_ERR(st->mclk) != -ENOENT) {
>  		ret = PTR_ERR(st->mclk);
> -		goto error_disable_reg;
> +		return ret;
>  	}
>  
>  	if (!IS_ERR(st->mclk)) {
>  		ret = clk_prepare_enable(st->mclk);
>  		if (ret < 0)
> -			goto error_disable_reg;
> +			return ret;
>  		ext_clk_hz = clk_get_rate(st->mclk);
>  	}
>  
> @@ -742,41 +756,15 @@ static int ad5933_probe(struct i2c_client *client,
>  	indio_dev->channels = ad5933_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
>  
> -	ret = ad5933_register_ring_funcs_and_init(indio_dev);
> +	ret = ad5933_register_ring_funcs_and_init(&client->dev, indio_dev);
>  	if (ret)
> -		goto error_disable_mclk;
> +		return ret;
>  
>  	ret = ad5933_setup(st);
>  	if (ret)
> -		goto error_unreg_ring;
> -
> -	ret = iio_device_register(indio_dev);
> -	if (ret)
> -		goto error_unreg_ring;
> -
> -	return 0;
> -
> -error_unreg_ring:
> -	iio_kfifo_free(indio_dev->buffer);
> -error_disable_mclk:
> -	clk_disable_unprepare(st->mclk);
> -error_disable_reg:
> -	regulator_disable(st->reg);
> -
> -	return ret;
> -}
> -
> -static int ad5933_remove(struct i2c_client *client)
> -{
> -	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> -	struct ad5933_state *st = iio_priv(indio_dev);
> -
> -	iio_device_unregister(indio_dev);
> -	iio_kfifo_free(indio_dev->buffer);
> -	regulator_disable(st->reg);
> -	clk_disable_unprepare(st->mclk);
> +		return ret;
>  
> -	return 0;
> +	return devm_iio_device_register(&client->dev, indio_dev);
>  }
>  
>  static const struct i2c_device_id ad5933_id[] = {
> @@ -801,7 +789,6 @@ static struct i2c_driver ad5933_driver = {
>  		.of_match_table = ad5933_of_match,
>  	},
>  	.probe = ad5933_probe,
> -	.remove = ad5933_remove,
>  	.id_table = ad5933_id,
>  };
>  module_i2c_driver(ad5933_driver);

Re: [PATCH] staging: iio: ad5933: rework probe to use devm_ function variants

[Ardelean, Alexandru]

On Sat, 2020-05-02 at 19:25 +0100, Jonathan Cameron wrote:
> On Tue, 28 Apr 2020 12:31:28 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > This change cleans up the driver's probe function to use only devm_
> > function variants. This also gets rid of the remove function and moves the
> > clock & regulator de-initializations to the 'ad5933_cleanup()' callback.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> Basic rule of thumb. Whatever you register with devm_add_action_or_reset
> should only cleanup one one thing done in the probe path.
> There is almost always a race if you do more than one bit of cleanup
> per such callback + it's harder to review as it fails the 'obviously correct
> test'.

I did not know that.
That idea missed me up until now.

Will re-spin.
Thanks
Alex

> 
> Jonathan
> 
> > ---
> >  .../staging/iio/impedance-analyzer/ad5933.c   | 59 ++++++++-----------
> >  1 file changed, 23 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > index af0bcf95ee8a..06a6dcd7883b 100644
> > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > @@ -602,11 +602,12 @@ static const struct iio_buffer_setup_ops
> > ad5933_ring_setup_ops = {
> >  	.postdisable = ad5933_ring_postdisable,
> >  };
> >  
> > -static int ad5933_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> > +static int ad5933_register_ring_funcs_and_init(struct device *dev,
> > +					       struct iio_dev *indio_dev)
> >  {
> >  	struct iio_buffer *buffer;
> >  
> > -	buffer = iio_kfifo_allocate();
> > +	buffer = devm_iio_kfifo_allocate(dev);
> >  	if (!buffer)
> >  		return -ENOMEM;
> >  
> > @@ -676,6 +677,14 @@ static void ad5933_work(struct work_struct *work)
> >  	}
> >  }
> >  
> > +static void ad5933_cleanup(void *data)
> > +{
> > +	struct ad5933_state *st = data;
> > +
> > +	clk_disable_unprepare(st->mclk);
> > +	regulator_disable(st->reg);
> 
> Please do two separate callbacks so that these can be handled
> in the correct places.  I.e. you do something then immediately
> register the handler to undo it.
> 
> Currently you can end up disabling a clock you haven't enabled
> (which I am fairly sure will give you an error message).
> 
> > +}
> > +
> >  static int ad5933_probe(struct i2c_client *client,
> >  			const struct i2c_device_id *id)
> >  {
> > @@ -703,23 +712,28 @@ static int ad5933_probe(struct i2c_client *client,
> >  		dev_err(&client->dev, "Failed to enable specified VDD
> > supply\n");
> >  		return ret;
> >  	}
> > +
> > +	ret = devm_add_action_or_reset(&client->dev, ad5933_cleanup, st);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = regulator_get_voltage(st->reg);
> >  
> >  	if (ret < 0)
> > -		goto error_disable_reg;
> > +		return ret;
> >  
> >  	st->vref_mv = ret / 1000;
> >  
> >  	st->mclk = devm_clk_get(&client->dev, "mclk");
> >  	if (IS_ERR(st->mclk) && PTR_ERR(st->mclk) != -ENOENT) {
> >  		ret = PTR_ERR(st->mclk);
> > -		goto error_disable_reg;
> > +		return ret;
> >  	}
> >  
> >  	if (!IS_ERR(st->mclk)) {
> >  		ret = clk_prepare_enable(st->mclk);
> >  		if (ret < 0)
> > -			goto error_disable_reg;
> > +			return ret;
> >  		ext_clk_hz = clk_get_rate(st->mclk);
> >  	}
> >  
> > @@ -742,41 +756,15 @@ static int ad5933_probe(struct i2c_client *client,
> >  	indio_dev->channels = ad5933_channels;
> >  	indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
> >  
> > -	ret = ad5933_register_ring_funcs_and_init(indio_dev);
> > +	ret = ad5933_register_ring_funcs_and_init(&client->dev, indio_dev);
> >  	if (ret)
> > -		goto error_disable_mclk;
> > +		return ret;
> >  
> >  	ret = ad5933_setup(st);
> >  	if (ret)
> > -		goto error_unreg_ring;
> > -
> > -	ret = iio_device_register(indio_dev);
> > -	if (ret)
> > -		goto error_unreg_ring;
> > -
> > -	return 0;
> > -
> > -error_unreg_ring:
> > -	iio_kfifo_free(indio_dev->buffer);
> > -error_disable_mclk:
> > -	clk_disable_unprepare(st->mclk);
> > -error_disable_reg:
> > -	regulator_disable(st->reg);
> > -
> > -	return ret;
> > -}
> > -
> > -static int ad5933_remove(struct i2c_client *client)
> > -{
> > -	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > -	struct ad5933_state *st = iio_priv(indio_dev);
> > -
> > -	iio_device_unregister(indio_dev);
> > -	iio_kfifo_free(indio_dev->buffer);
> > -	regulator_disable(st->reg);
> > -	clk_disable_unprepare(st->mclk);
> > +		return ret;
> >  
> > -	return 0;
> > +	return devm_iio_device_register(&client->dev, indio_dev);
> >  }
> >  
> >  static const struct i2c_device_id ad5933_id[] = {
> > @@ -801,7 +789,6 @@ static struct i2c_driver ad5933_driver = {
> >  		.of_match_table = ad5933_of_match,
> >  	},
> >  	.probe = ad5933_probe,
> > -	.remove = ad5933_remove,
> >  	.id_table = ad5933_id,
> >  };
> >  module_i2c_driver(ad5933_driver);

Re: [PATCH] staging: iio: ad5933: rework probe to use devm_ function variants

[Dan Carpenter]

On Sat, May 02, 2020 at 07:25:42PM +0100, Jonathan Cameron wrote:
> On Tue, 28 Apr 2020 12:31:28 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> > +static void ad5933_cleanup(void *data)
> > +{
> > +	struct ad5933_state *st = data;
> > +
> > +	clk_disable_unprepare(st->mclk);
> > +	regulator_disable(st->reg);
> 
> Please do two separate callbacks so that these can be handled
> in the correct places.  I.e. you do something then immediately
> register the handler to undo it.
> 
> Currently you can end up disabling a clock you haven't enabled
> (which I am fairly sure will give you an error message).

Yeah.  It does.

It feels like we should just make a devm_ version of regulator_enable().
Or potentially this is more complicated than it seems, but in that case
probably adding devm_add_action_or_reset() is more complicated than it
seems as well.

regards,
dan carpenter

Re: [PATCH] staging: iio: ad5933: rework probe to use devm_ function variants

[Jonathan Cameron]

On Thu, 7 May 2020 12:50:16 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Sat, May 02, 2020 at 07:25:42PM +0100, Jonathan Cameron wrote:
> > On Tue, 28 Apr 2020 12:31:28 +0300
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:  
> > > +static void ad5933_cleanup(void *data)
> > > +{
> > > +	struct ad5933_state *st = data;
> > > +
> > > +	clk_disable_unprepare(st->mclk);
> > > +	regulator_disable(st->reg);  
> > 
> > Please do two separate callbacks so that these can be handled
> > in the correct places.  I.e. you do something then immediately
> > register the handler to undo it.
> > 
> > Currently you can end up disabling a clock you haven't enabled
> > (which I am fairly sure will give you an error message).  
> 
> Yeah.  It does.
> 
> It feels like we should just make a devm_ version of regulator_enable().
> Or potentially this is more complicated than it seems, but in that case
> probably adding devm_add_action_or_reset() is more complicated than it
> seems as well.
> 
> regards,
> dan carpenter

It has been a while since that was last proposed.   At the time the
counter argument was that you should almost always be doing some form
of PM and hence the regulator shouldn't have the same lifetime as the
driver.   Reality is that a lot of simple drivers either don't do
PM or have elected to not turn the regulator off so as to retain state
etc.

Mark what do you think?

Jonathan

Re: [PATCH] staging: iio: ad5933: rework probe to use devm_ function variants

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

On Fri, May 08, 2020 at 01:43:07PM +0100, Jonathan Cameron wrote:
> Dan Carpenter <dan.carpenter@oracle.com> wrote:

> > It feels like we should just make a devm_ version of regulator_enable().
> > Or potentially this is more complicated than it seems, but in that case
> > probably adding devm_add_action_or_reset() is more complicated than it
> > seems as well.

> It has been a while since that was last proposed.   At the time the
> counter argument was that you should almost always be doing some form
> of PM and hence the regulator shouldn't have the same lifetime as the
> driver.   Reality is that a lot of simple drivers either don't do
> PM or have elected to not turn the regulator off so as to retain state
> etc.

Same issue as before - I fear it's far too error prone in conjunction
with runtime PM, and if the driver really is just doing an enable and
disable at probe and remove then that seems fairly trivial anyway.  I
am constantly finding abuses of things like regulator_get_optional()
(which we do actually need) in drivers and it's not like I can review
all the users, I don't have much confidence in this stuff especially
when practically speaking few regulators ever change state at runtime so
issues don't manifest so often.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] staging: iio: ad5933: rework probe to use devm_ function variants

[Jonathan Cameron]

On Fri, 8 May 2020 13:57:46 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Fri, May 08, 2020 at 01:43:07PM +0100, Jonathan Cameron wrote:
> > Dan Carpenter <dan.carpenter@oracle.com> wrote:  
> 
> > > It feels like we should just make a devm_ version of regulator_enable().
> > > Or potentially this is more complicated than it seems, but in that case
> > > probably adding devm_add_action_or_reset() is more complicated than it
> > > seems as well.  
> 
> > It has been a while since that was last proposed.   At the time the
> > counter argument was that you should almost always be doing some form
> > of PM and hence the regulator shouldn't have the same lifetime as the
> > driver.   Reality is that a lot of simple drivers either don't do
> > PM or have elected to not turn the regulator off so as to retain state
> > etc.  
> 
> Same issue as before - I fear it's far too error prone in conjunction
> with runtime PM, and if the driver really is just doing an enable and
> disable at probe and remove then that seems fairly trivial anyway.  I
> am constantly finding abuses of things like regulator_get_optional()
> (which we do actually need) in drivers and it's not like I can review
> all the users, I don't have much confidence in this stuff especially
> when practically speaking few regulators ever change state at runtime so
> issues don't manifest so often.
> 

Fair enough.  We'll carry on doing it with devm_add_action_or_reset
which forces us to take a close look at why we always want the lifetime
to match that of the device.

Note the key thing here is we don't have a remove in these drivers.
Everything is managed.  Mixing and matching between managed and unmanaged
causes more subtle race conditions...

Jonathan