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@xxxxxxxxxx> 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@xxxxxxxxxx>
> 
> 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);