Re: [PATCH 1/3] iio: light: gp2ap002: add led power supply

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] iio: light: gp2ap002: memory/resource leak of vled regulator in error paths
- [Medium] iio: light: gp2ap002: incorrect regulator disable order violates hardware teardown sequencing
--

commit 377213d2096a5527d0fa1ddbd0a74bf96e0940f2
Author: Christoph Rudorff <chris@rudorff.com>

iio: light: gp2ap002: add led power supply

This commit adds a missing VLED power supply to the gp2ap002 driver.
It initializes the vled regulator and handles its enable and disable paths
in the probe, remove, suspend, and resume functions.

> diff --git a/drivers/iio/light/gp2ap002.c b/drivers/iio/light/gp2ap002.c
> index de29081fd3b20..c03eb0f1988bc 100644
> --- a/drivers/iio/light/gp2ap002.c
> +++ b/drivers/iio/light/gp2ap002.c
[ ... ]
> @@ -560,6 +570,11 @@ static int gp2ap002_probe(struct i2c_client *client)
>  		dev_err(dev, "failed to enable VIO regulator\n");
>  		goto out_disable_vdd;
>  	}
> +	ret = regulator_enable(gp2ap002->vled);
> +	if (ret) {
> +		dev_err(dev, "failed to enable VLED regulator\n");
> +		goto out_disable_vio;
> +	}
>  
>  	msleep(20);

[Severity: Medium]
When gp2ap002_init() or devm_request_threaded_irq() fail further down in
gp2ap002_probe(), the code jumps to out_disable_vio or out_put_pm. However,
the error handling block only disables the vio and vdd regulators.

For example, when initialization fails:

	ret = gp2ap002_init(gp2ap002);
	if (ret) {
		dev_err(dev, "initialization failed\n");
		goto out_disable_vio;
	}

Does this regression leave the newly added vled regulator permanently enabled,
leaking its enable count? Should we add an out_disable_vled label that
disables the vled regulator?

[ ... ]
> @@ -635,6 +650,7 @@ static void gp2ap002_remove(struct i2c_client *client)
>  	iio_device_unregister(indio_dev);
>  	regulator_disable(gp2ap002->vio);
>  	regulator_disable(gp2ap002->vdd);
> +	regulator_disable(gp2ap002->vled);

[Severity: Medium]
In gp2ap002_probe(), the regulators are enabled in the order: vdd -> vio ->
vled. To prevent powering peripherals while the core logic is unpowered
(which could lead to backpowering or leakage currents), should the teardown
occur in the exact reverse order?

Disabling them here as vio -> vdd -> vled seems to violate that sequence.

>  }
>  
>  static int gp2ap002_runtime_suspend(struct device *dev)
> @@ -659,6 +675,7 @@ static int gp2ap002_runtime_suspend(struct device *dev)
>  	 */
>  	regulator_disable(gp2ap002->vio);
>  	regulator_disable(gp2ap002->vdd);
> +	regulator_disable(gp2ap002->vled);

[Severity: Medium]
As noted above in gp2ap002_remove(), does this disable sequence regression
also violate the hardware teardown ordering by disabling vled last instead
of first?

>  
>  	return 0;
>  }
> @@ -679,6 +696,11 @@ static int gp2ap002_runtime_resume(struct device *dev)
>  		dev_err(dev, "failed to enable VIO regulator in resume path\n");
>  		goto out_disable_vdd;
>  	}
> +	ret = regulator_enable(gp2ap002->vled);
> +	if (ret) {
> +		dev_err(dev, "failed to enable VLED regulator in resume path\n");
> +		goto out_disable_vio;
> +	}
>  
>  	msleep(20);

[Severity: Medium]
If gp2ap002_init() fails later in gp2ap002_runtime_resume(), the code jumps
to out_disable_vio:

	ret = gp2ap002_init(gp2ap002);
	if (ret) {
		dev_err(dev, "re-initialization failed\n");
		goto out_disable_vio;
	}

Similar to the probe function, does this regression leak the enable count
for the vled regulator, since it is not disabled in the error path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-iio-light-gp2ap002-v1-0-7c93312fa8d5@rudorff.com?part=1