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);
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
