[PATCH v2 0/2] iio: adc: sun4i-gpadc-iio: Fix error handle in sun4i_gpadc_probe()

[PATCH v2 0/2] iio: adc: sun4i-gpadc-iio: Fix error handle in sun4i_gpadc_probe()

[Zhang Qilong]

This patch set fix three bugfixs include:

1) If thermal_zone_of_sensor_register() failed, PM disable
   depth will be imbalanced and iio_map_array may have been
   called. The first patch fixed them.

2) If devm_iio_device_register() failed, we don't revert
   thermal_zone registration. The second patch fix it.

Zhang Qilong (2):
  iio: adc: sun4i-gpadc-iio: Fix PM disable depth imbalance in
    sun4i_gpadc_probe()
  iio: adc: sun4i-gpadc-iio: Fix error handle when
    devm_iio_device_register() failed in sun4i_gpadc_probe()

 drivers/iio/adc/sun4i-gpadc-iio.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] iio: adc: sun4i-gpadc-iio: Fix PM disable depth imbalance in sun4i_gpadc_probe()

[Zhang Qilong]

The pm_runtime_enable will increase power disable depth.
Thus a pairing decrement is needed on the error handling
path to keep it balanced according to context. In addtion,
the iio_map_array path has potentially been called.

We fix it by gotoing err_map when thermal_zone register
failed.

Fixes: b0a242894f11 ("iio: adc: sun4i-gpadc-iio: register in the thermal after registering in pm")
Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
---
v2:
- revert iio_map_array if it's necessary when thermal_zone
  register failed.
---
 drivers/iio/adc/sun4i-gpadc-iio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index a6ade70dedf8..d2535dd28af8 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -648,7 +648,8 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev,
 				"could not register thermal sensor: %ld\n",
 				PTR_ERR(info->tzd));
-			return PTR_ERR(info->tzd);
+			ret = PTR_ERR(info->tzd);
+			goto err_map;
 		}
 	}
 
-- 
2.25.1

[PATCH v2 2/2] iio: adc: sun4i-gpadc-iio: Fix error handle when devm_iio_device_register() failed in sun4i_gpadc_probe()

[Zhang Qilong]

If devm_iio_device_register() failed, the thermal_zone may have been
registered. So we need call thermal_zone_of_sensor_unregister() when
CONFIG_THERMAL_OF is enabled. We fix it by adding a err_register and
gotoing it when devm_iio_device_register() failed.

Fixes: d1caa9905538 ("iio: adc: add support for Allwinner SoCs ADC")
Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
---
 drivers/iio/adc/sun4i-gpadc-iio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index d2535dd28af8..04717571cb2e 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -656,11 +656,17 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
 	ret = devm_iio_device_register(&pdev->dev, indio_dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "could not register the device\n");
-		goto err_map;
+		goto err_register;
 	}
 
 	return 0;
 
+err_register:
+	if (IS_ENABLED(CONFIG_THERMAL_OF)) {
+		devm_thermal_of_zone_unregister(info->sensor_device,
+						info->tzd);
+		info->tzd = NULL;
+	}
 err_map:
 	if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
 		iio_map_array_unregister(indio_dev);
-- 
2.25.1

Re: [PATCH v2 2/2] iio: adc: sun4i-gpadc-iio: Fix error handle when devm_iio_device_register() failed in sun4i_gpadc_probe()

[Jonathan Cameron]

On Thu, 20 Oct 2022 20:40:45 +0800
Zhang Qilong <zhangqilong3@huawei.com> wrote:

> If devm_iio_device_register() failed, the thermal_zone may have been
> registered. So we need call thermal_zone_of_sensor_unregister() when
> CONFIG_THERMAL_OF is enabled. We fix it by adding a err_register and
> gotoing it when devm_iio_device_register() failed.

This doesn't look right.  Any devm_ registered calls should be cleaned
up automatically if we fail the probe.

However, there is an issue in what you've hightlighed here in that
on the remove path we will have ripped out the iio_map_register() before
we remove the thermal zone that is dependent on it.

Easiest fix for that is probably to use devm_iio_array_map_register()
to allow the managed handling to clean that up at the correct point.

Follow on that path you may also be able to use devm_pm_runtime_enable()
here to deal with the tear down of runtime pm (which is currently wrong
anyway as no autosuspend disable.)

Thanks,

Jonathan



> 
> Fixes: d1caa9905538 ("iio: adc: add support for Allwinner SoCs ADC")
> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> ---
>  drivers/iio/adc/sun4i-gpadc-iio.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
> index d2535dd28af8..04717571cb2e 100644
> --- a/drivers/iio/adc/sun4i-gpadc-iio.c
> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
> @@ -656,11 +656,17 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>  	ret = devm_iio_device_register(&pdev->dev, indio_dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "could not register the device\n");
> -		goto err_map;
> +		goto err_register;
>  	}
>  
>  	return 0;
>  
> +err_register:
> +	if (IS_ENABLED(CONFIG_THERMAL_OF)) {
> +		devm_thermal_of_zone_unregister(info->sensor_device,
> +						info->tzd);
> +		info->tzd = NULL;
> +	}
>  err_map:
>  	if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>  		iio_map_array_unregister(indio_dev);

Re: [PATCH v2 1/2] iio: adc: sun4i-gpadc-iio: Fix PM disable depth imbalance in sun4i_gpadc_probe()

[Jonathan Cameron]

On Thu, 20 Oct 2022 20:40:44 +0800
Zhang Qilong <zhangqilong3@huawei.com> wrote:

> The pm_runtime_enable will increase power disable depth.
> Thus a pairing decrement is needed on the error handling
> path to keep it balanced according to context. In addtion,
> the iio_map_array path has potentially been called.
> 
> We fix it by gotoing err_map when thermal_zone register
> failed.
> 
> Fixes: b0a242894f11 ("iio: adc: sun4i-gpadc-iio: register in the thermal after registering in pm")
> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
As mentioned in review of patch 2 there are other problems in here that
will probably be better cleaned up by taking the whole lot over
to devm

Thanks,

Jonathan

> ---
> v2:
> - revert iio_map_array if it's necessary when thermal_zone
>   register failed.
> ---
>  drivers/iio/adc/sun4i-gpadc-iio.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
> index a6ade70dedf8..d2535dd28af8 100644
> --- a/drivers/iio/adc/sun4i-gpadc-iio.c
> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
> @@ -648,7 +648,8 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>  			dev_err(&pdev->dev,
>  				"could not register thermal sensor: %ld\n",
>  				PTR_ERR(info->tzd));
> -			return PTR_ERR(info->tzd);
> +			ret = PTR_ERR(info->tzd);
> +			goto err_map;
>  		}
>  	}
>