Re: [PATCH v6 6/9] iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] Undocumented scope modification and suboptimal pointer dereferencing.

Pre-existing issues:
- [High] Device teardown accesses powered-off hardware and causes unbalanced regulator disables.
- [Low] Unconditional assignment of PM callbacks causes dead code when PM is disabled.
--

--- Patch [6]: [PATCH v6 6/9] iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe() ---
commit 04da87f5e80c506bb658d3dd5161087ce55fa6b0
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe()

This commit refactors mma8452_probe() to use a local 'dev' variable instead
of repeatedly referencing &client->dev. It aims to improve code readability.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index d1e8eb2a4ad3a..7ef1a9a91c31f 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c

[ ... ]

> @@ -1801,7 +1799,8 @@ static void mma8452_remove(struct i2c_client *client)
>  #ifdef CONFIG_PM
>  static int mma8452_runtime_suspend(struct device *dev)
>  {
> -	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>  	struct mma8452_data *data = iio_priv(indio_dev);
>  	int ret;
>  
> @@ -1809,7 +1808,7 @@ static int mma8452_runtime_suspend(struct device *dev)
>  	ret = mma8452_standby(data);
>  	mutex_unlock(&data->lock);
>  	if (ret < 0) {
> -		dev_err(&data->client->dev, "powering off device failed\n");
> +		dev_err(&client->dev, "powering off device failed\n");

[Severity: Low]
The commit message states the changes are restricted to reusing the dev
pointer in mma8452_probe(), but this modifies mma8452_runtime_suspend().
Should the commit message be updated to reflect this scope change?

Also, since the function already receives a struct device *dev parameter,
and &client->dev evaluates to that exact same pointer, wouldn't it be 
simpler to use dev directly here rather than dereferencing client?

>  		return -EAGAIN;
>  	}


As a related note, looking at the device teardown path in mma8452_remove():

static void mma8452_remove(struct i2c_client *client)
{
[ ... ]
	pm_runtime_disable(&client->dev);
	pm_runtime_set_suspended(&client->dev);
[ ... ]
	mma8452_standby(iio_priv(indio_dev));

	regulator_disable(data->vddio_reg);
	regulator_disable(data->vdd_reg);
}

[Severity: High]
This is a pre-existing issue, but does this teardown sequence access
potentially powered-off hardware?

If the device is in the RPM_SUSPENDED state when remove() is called, its
regulators have already been disabled by mma8452_runtime_suspend(). The
call to mma8452_standby() attempts an I2C write to the powered-off device,
which might hang the I2C bus or cause transaction timeouts.

Furthermore, unconditionally calling regulator_disable() drops the usage
counts below zero, triggering a kernel warning for unbalanced disables.

Should pm_runtime_get_sync(&client->dev) be called at the beginning of
mma8452_remove() to ensure the hardware is active before teardown?


Finally, regarding the PM ops assignment in the driver struct:

[Severity: Low]
This isn't a bug introduced by this patch, but does the unconditional
assignment of &mma8452_pm_ops to the .pm field in mma8452_driver produce
dead code when CONFIG_PM is disabled?

According to the power management subsystem guidelines, this pointer should
be wrapped in pm_ptr(&mma8452_pm_ops) to allow the compiler to eliminate
the dead code.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=6