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

[Esben Haabendal <esben@geanix.com>]

<sashiko-bot@kernel.org> writes:

> 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?

Yes. Using the dev pointer directly is the right thing to do.  Added for
next version.

And also, the change that adds the client pointer will be rebased into
patch 4, where the client pointer is used. Sorry about that.

>>  		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