[PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe

[PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I noticed an issue around regulator error handling and managed to
hit another with my hacked together test setup.  Hence let's fix
those two issues first then we might as well follow up by converting
the last few bits of this driver to use device managed functions so
we can simplify the error handling and cleanup.

Testing conducted with QEMU hacking and insertion of errors at relevant
locations in the code.

Jonathan Cameron (3):
  iio: adc: ad7124: Fix missbalanced regulator enable / disable on
    error.
  iio: adc: ad7124: Fix potential overflow due to non sequential channel
    numbers
  iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop
    remove()

 drivers/iio/adc/ad7124.c | 89 ++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 45 deletions(-)

-- 
2.31.1

[PATCH 1/3] iio: adc: ad7124: Fix missbalanced regulator enable / disable on error.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

If the devm_regulator_get() call succeeded but not the regulator_enable()
then regulator_disable() would be called on a regulator that was not
enabled.

Fix this by moving regulator enabling / disabling over to
devm_ management via devm_add_action_or_reset.

Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
---
 drivers/iio/adc/ad7124.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 766c73333604..c0d0870a29ff 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -707,6 +707,11 @@ static int ad7124_setup(struct ad7124_state *st)
 	return ret;
 }
 
+static void ad7124_reg_disable(void *r)
+{
+	regulator_disable(r);
+}
+
 static int ad7124_probe(struct spi_device *spi)
 {
 	const struct ad7124_chip_info *info;
@@ -752,17 +757,20 @@ static int ad7124_probe(struct spi_device *spi)
 		ret = regulator_enable(st->vref[i]);
 		if (ret)
 			return ret;
+
+		ret = devm_add_action_or_reset(&spi->dev, ad7124_reg_disable,
+					       st->vref[i]);
+		if (ret)
+			return ret;
 	}
 
 	st->mclk = devm_clk_get(&spi->dev, "mclk");
-	if (IS_ERR(st->mclk)) {
-		ret = PTR_ERR(st->mclk);
-		goto error_regulator_disable;
-	}
+	if (IS_ERR(st->mclk))
+		return PTR_ERR(st->mclk);
 
 	ret = clk_prepare_enable(st->mclk);
 	if (ret < 0)
-		goto error_regulator_disable;
+		return ret;
 
 	ret = ad7124_soft_reset(st);
 	if (ret < 0)
@@ -792,11 +800,6 @@ static int ad7124_probe(struct spi_device *spi)
 	ad_sd_cleanup_buffer_and_trigger(indio_dev);
 error_clk_disable_unprepare:
 	clk_disable_unprepare(st->mclk);
-error_regulator_disable:
-	for (i = ARRAY_SIZE(st->vref) - 1; i >= 0; i--) {
-		if (!IS_ERR_OR_NULL(st->vref[i]))
-			regulator_disable(st->vref[i]);
-	}
 
 	return ret;
 }
@@ -805,17 +808,11 @@ static int ad7124_remove(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev = spi_get_drvdata(spi);
 	struct ad7124_state *st = iio_priv(indio_dev);
-	int i;
 
 	iio_device_unregister(indio_dev);
 	ad_sd_cleanup_buffer_and_trigger(indio_dev);
 	clk_disable_unprepare(st->mclk);
 
-	for (i = ARRAY_SIZE(st->vref) - 1; i >= 0; i--) {
-		if (!IS_ERR_OR_NULL(st->vref[i]))
-			regulator_disable(st->vref[i]);
-	}
-
 	return 0;
 }
 
-- 
2.31.1

[PATCH 2/3] iio: adc: ad7124: Fix potential overflow due to non sequential channel numbers

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Channel numbering must start at 0 and then not have any holes, or
it is possible to overflow the available storage.  Note this bug was
introduced as part of a fix to ensure we didn't rely on the ordering
of child nodes.  So we need to support arbitrary ordering but they all
need to be there somewhere.

Note I hit this when using qemu to test the rest of this series.
Arguably this isn't the best fix, but it is probably the most minimal
option for backporting etc.

Fixes: d7857e4ee1ba6 ("iio: adc: ad7124: Fix DT channel configuration")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ad7124.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index c0d0870a29ff..9c2401c5848e 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -616,6 +616,13 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
 		if (ret)
 			goto err;
 
+		if (channel >= indio_dev->num_channels) {
+			dev_err(indio_dev->dev.parent,
+				"Channel index >= number of channels\n");
+			ret = -EINVAL;
+			goto err;
+		}
+
 		ret = of_property_read_u32_array(child, "diff-channels",
 						 ain, 2);
 		if (ret)
-- 
2.31.1

[PATCH 3/3] iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop remove()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

As not many steps were not already devm_ managed, use
devm_add_action_or_reset() to handle the rest.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
---
 drivers/iio/adc/ad7124.c | 53 ++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 9c2401c5848e..deb166d2b645 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -719,6 +719,16 @@ static void ad7124_reg_disable(void *r)
 	regulator_disable(r);
 }
 
+static void ad7124_clk_disable(void *c)
+{
+	clk_disable_unprepare(c);
+}
+
+static void ad7124_buffer_cleanup(void *idev)
+{
+	ad_sd_cleanup_buffer_and_trigger(idev);
+}
+
 static int ad7124_probe(struct spi_device *spi)
 {
 	const struct ad7124_chip_info *info;
@@ -740,8 +750,6 @@ static int ad7124_probe(struct spi_device *spi)
 
 	ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info);
 
-	spi_set_drvdata(spi, indio_dev);
-
 	indio_dev->name = st->chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &ad7124_info;
@@ -779,48 +787,36 @@ static int ad7124_probe(struct spi_device *spi)
 	if (ret < 0)
 		return ret;
 
+	ret = devm_add_action_or_reset(&spi->dev, ad7124_clk_disable, st->mclk);
+	if (ret)
+		return ret;
+
 	ret = ad7124_soft_reset(st);
 	if (ret < 0)
-		goto error_clk_disable_unprepare;
+		return ret;
 
 	ret = ad7124_check_chip_id(st);
 	if (ret)
-		goto error_clk_disable_unprepare;
+		return ret;
 
 	ret = ad7124_setup(st);
 	if (ret < 0)
-		goto error_clk_disable_unprepare;
+		return ret;
 
 	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
 	if (ret < 0)
-		goto error_clk_disable_unprepare;
-
-	ret = iio_device_register(indio_dev);
-	if (ret < 0) {
-		dev_err(&spi->dev, "Failed to register iio device\n");
-		goto error_remove_trigger;
-	}
+		return ret;
 
-	return 0;
+	ret = devm_add_action_or_reset(&spi->dev, ad7124_buffer_cleanup, indio_dev);
+	if (ret)
+		return ret;
 
-error_remove_trigger:
-	ad_sd_cleanup_buffer_and_trigger(indio_dev);
-error_clk_disable_unprepare:
-	clk_disable_unprepare(st->mclk);
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret)
+		dev_err(&spi->dev, "Failed to register iio device\n");
 
 	return ret;
-}
 
-static int ad7124_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct ad7124_state *st = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-	ad_sd_cleanup_buffer_and_trigger(indio_dev);
-	clk_disable_unprepare(st->mclk);
-
-	return 0;
 }
 
 static const struct of_device_id ad7124_of_match[] = {
@@ -838,7 +834,6 @@ static struct spi_driver ad71124_driver = {
 		.of_match_table = ad7124_of_match,
 	},
 	.probe = ad7124_probe,
-	.remove	= ad7124_remove,
 };
 module_spi_driver(ad71124_driver);
 
-- 
2.31.1

Re: [PATCH 1/3] iio: adc: ad7124: Fix missbalanced regulator enable / disable on error.

[Alexandru Ardelean]

On Sat, May 8, 2021 at 9:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> If the devm_regulator_get() call succeeded but not the regulator_enable()
> then regulator_disable() would be called on a regulator that was not
> enabled.
>
> Fix this by moving regulator enabling / disabling over to
> devm_ management via devm_add_action_or_reset.
>

Definitely looks unbalanced.
This was probably written in the days when I wasn't familiar with devm_ much.
I probably would have complained about this.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

> Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
> ---
>  drivers/iio/adc/ad7124.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 766c73333604..c0d0870a29ff 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -707,6 +707,11 @@ static int ad7124_setup(struct ad7124_state *st)
>         return ret;
>  }
>
> +static void ad7124_reg_disable(void *r)
> +{
> +       regulator_disable(r);
> +}
> +
>  static int ad7124_probe(struct spi_device *spi)
>  {
>         const struct ad7124_chip_info *info;
> @@ -752,17 +757,20 @@ static int ad7124_probe(struct spi_device *spi)
>                 ret = regulator_enable(st->vref[i]);
>                 if (ret)
>                         return ret;
> +
> +               ret = devm_add_action_or_reset(&spi->dev, ad7124_reg_disable,
> +                                              st->vref[i]);
> +               if (ret)
> +                       return ret;
>         }
>
>         st->mclk = devm_clk_get(&spi->dev, "mclk");
> -       if (IS_ERR(st->mclk)) {
> -               ret = PTR_ERR(st->mclk);
> -               goto error_regulator_disable;
> -       }
> +       if (IS_ERR(st->mclk))
> +               return PTR_ERR(st->mclk);
>
>         ret = clk_prepare_enable(st->mclk);
>         if (ret < 0)
> -               goto error_regulator_disable;
> +               return ret;
>
>         ret = ad7124_soft_reset(st);
>         if (ret < 0)
> @@ -792,11 +800,6 @@ static int ad7124_probe(struct spi_device *spi)
>         ad_sd_cleanup_buffer_and_trigger(indio_dev);
>  error_clk_disable_unprepare:
>         clk_disable_unprepare(st->mclk);
> -error_regulator_disable:
> -       for (i = ARRAY_SIZE(st->vref) - 1; i >= 0; i--) {
> -               if (!IS_ERR_OR_NULL(st->vref[i]))
> -                       regulator_disable(st->vref[i]);
> -       }
>
>         return ret;
>  }
> @@ -805,17 +808,11 @@ static int ad7124_remove(struct spi_device *spi)
>  {
>         struct iio_dev *indio_dev = spi_get_drvdata(spi);
>         struct ad7124_state *st = iio_priv(indio_dev);
> -       int i;
>
>         iio_device_unregister(indio_dev);
>         ad_sd_cleanup_buffer_and_trigger(indio_dev);
>         clk_disable_unprepare(st->mclk);
>
> -       for (i = ARRAY_SIZE(st->vref) - 1; i >= 0; i--) {
> -               if (!IS_ERR_OR_NULL(st->vref[i]))
> -                       regulator_disable(st->vref[i]);
> -       }
> -
>         return 0;
>  }
>
> --
> 2.31.1
>

Re: [PATCH 2/3] iio: adc: ad7124: Fix potential overflow due to non sequential channel numbers

[Alexandru Ardelean]

On Sat, May 8, 2021 at 9:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Channel numbering must start at 0 and then not have any holes, or
> it is possible to overflow the available storage.  Note this bug was
> introduced as part of a fix to ensure we didn't rely on the ordering
> of child nodes.  So we need to support arbitrary ordering but they all
> need to be there somewhere.
>
> Note I hit this when using qemu to test the rest of this series.
> Arguably this isn't the best fix, but it is probably the most minimal
> option for backporting etc.
>

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

> Fixes: d7857e4ee1ba6 ("iio: adc: ad7124: Fix DT channel configuration")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/adc/ad7124.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index c0d0870a29ff..9c2401c5848e 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -616,6 +616,13 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
>                 if (ret)
>                         goto err;
>
> +               if (channel >= indio_dev->num_channels) {
> +                       dev_err(indio_dev->dev.parent,
> +                               "Channel index >= number of channels\n");
> +                       ret = -EINVAL;
> +                       goto err;
> +               }
> +
>                 ret = of_property_read_u32_array(child, "diff-channels",
>                                                  ain, 2);
>                 if (ret)
> --
> 2.31.1
>

Re: [PATCH 3/3] iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop remove()

[Alexandru Ardelean]

On Sat, May 8, 2021 at 9:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> As not many steps were not already devm_ managed, use
> devm_add_action_or_reset() to handle the rest.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Cc: Alexandru Ardelean <ardeleanalex@gmail.com>

double Cc: tag here

One minor thought about maybe removing the log spam.
Other than that:

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>


> ---
>  drivers/iio/adc/ad7124.c | 53 ++++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 9c2401c5848e..deb166d2b645 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -719,6 +719,16 @@ static void ad7124_reg_disable(void *r)
>         regulator_disable(r);
>  }
>
> +static void ad7124_clk_disable(void *c)
> +{
> +       clk_disable_unprepare(c);
> +}
> +
> +static void ad7124_buffer_cleanup(void *idev)
> +{
> +       ad_sd_cleanup_buffer_and_trigger(idev);

We never seem to have done a devm_ version for ad_sd_setup_buffer_and_trigger().


> +}
> +
>  static int ad7124_probe(struct spi_device *spi)
>  {
>         const struct ad7124_chip_info *info;
> @@ -740,8 +750,6 @@ static int ad7124_probe(struct spi_device *spi)
>
>         ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info);
>
> -       spi_set_drvdata(spi, indio_dev);
> -
>         indio_dev->name = st->chip_info->name;
>         indio_dev->modes = INDIO_DIRECT_MODE;
>         indio_dev->info = &ad7124_info;
> @@ -779,48 +787,36 @@ static int ad7124_probe(struct spi_device *spi)
>         if (ret < 0)
>                 return ret;
>
> +       ret = devm_add_action_or_reset(&spi->dev, ad7124_clk_disable, st->mclk);
> +       if (ret)
> +               return ret;
> +
>         ret = ad7124_soft_reset(st);
>         if (ret < 0)
> -               goto error_clk_disable_unprepare;
> +               return ret;
>
>         ret = ad7124_check_chip_id(st);
>         if (ret)
> -               goto error_clk_disable_unprepare;
> +               return ret;
>
>         ret = ad7124_setup(st);
>         if (ret < 0)
> -               goto error_clk_disable_unprepare;
> +               return ret;
>
>         ret = ad_sd_setup_buffer_and_trigger(indio_dev);
>         if (ret < 0)
> -               goto error_clk_disable_unprepare;
> -
> -       ret = iio_device_register(indio_dev);
> -       if (ret < 0) {
> -               dev_err(&spi->dev, "Failed to register iio device\n");
> -               goto error_remove_trigger;
> -       }
> +               return ret;
>
> -       return 0;
> +       ret = devm_add_action_or_reset(&spi->dev, ad7124_buffer_cleanup, indio_dev);
> +       if (ret)
> +               return ret;
>
> -error_remove_trigger:
> -       ad_sd_cleanup_buffer_and_trigger(indio_dev);
> -error_clk_disable_unprepare:
> -       clk_disable_unprepare(st->mclk);
> +       ret = devm_iio_device_register(&spi->dev, indio_dev);
> +       if (ret)
> +               dev_err(&spi->dev, "Failed to register iio device\n");

I guess Andy may come along and suggest that we remove this log spam
and just do a direct return.
Which would sound like a reasonable idea.

>
>         return ret;
> -}
>
> -static int ad7124_remove(struct spi_device *spi)
> -{
> -       struct iio_dev *indio_dev = spi_get_drvdata(spi);
> -       struct ad7124_state *st = iio_priv(indio_dev);
> -
> -       iio_device_unregister(indio_dev);
> -       ad_sd_cleanup_buffer_and_trigger(indio_dev);
> -       clk_disable_unprepare(st->mclk);
> -
> -       return 0;
>  }
>
>  static const struct of_device_id ad7124_of_match[] = {
> @@ -838,7 +834,6 @@ static struct spi_driver ad71124_driver = {
>                 .of_match_table = ad7124_of_match,
>         },
>         .probe = ad7124_probe,
> -       .remove = ad7124_remove,
>  };
>  module_spi_driver(ad71124_driver);
>
> --
> 2.31.1
>

Re: [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe

[Alexandru Ardelean]

On Sat, May 8, 2021 at 9:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> I noticed an issue around regulator error handling and managed to
> hit another with my hacked together test setup.  Hence let's fix
> those two issues first then we might as well follow up by converting
> the last few bits of this driver to use device managed functions so
> we can simplify the error handling and cleanup.
>
> Testing conducted with QEMU hacking and insertion of errors at relevant
> locations in the code.

I guess the devm_ cleanup didn't catch-on much with others.
It means I'll probably resume it :)

>
> Jonathan Cameron (3):
>   iio: adc: ad7124: Fix missbalanced regulator enable / disable on
>     error.
>   iio: adc: ad7124: Fix potential overflow due to non sequential channel
>     numbers
>   iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop
>     remove()
>
>  drivers/iio/adc/ad7124.c | 89 ++++++++++++++++++++--------------------
>  1 file changed, 44 insertions(+), 45 deletions(-)
>
> --
> 2.31.1
>

Re: [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe

[Jonathan Cameron]

On Sun, 9 May 2021 10:31:59 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sat, May 8, 2021 at 9:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > I noticed an issue around regulator error handling and managed to
> > hit another with my hacked together test setup.  Hence let's fix
> > those two issues first then we might as well follow up by converting
> > the last few bits of this driver to use device managed functions so
> > we can simplify the error handling and cleanup.
> >
> > Testing conducted with QEMU hacking and insertion of errors at relevant
> > locations in the code.  
> 
> I guess the devm_ cleanup didn't catch-on much with others.
> It means I'll probably resume it :)

It's a fun one for when you have 20 mins left at the end of a day.

Though less fun when you then hit a random memory corruption bug and
loose an hour :(

Jonathan

> 
> >
> > Jonathan Cameron (3):
> >   iio: adc: ad7124: Fix missbalanced regulator enable / disable on
> >     error.
> >   iio: adc: ad7124: Fix potential overflow due to non sequential channel
> >     numbers
> >   iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop
> >     remove()
> >
> >  drivers/iio/adc/ad7124.c | 89 ++++++++++++++++++++--------------------
> >  1 file changed, 44 insertions(+), 45 deletions(-)
> >
> > --
> > 2.31.1
> >