Re: [PATCH v6 5/9] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Esben Haabendal <esben@geanix.com>]

<sashiko-bot@kernel.org> writes:

> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Critical device register configurations are lost after a runtime suspend/resume cycle because `mma8452_runtime_resume` fails to restore them.
> - [High] Sysfs read/write callbacks and write_raw paths access the device hardware over I2C without waking it from runtime suspend.
> - [High] The `mma8452_remove()` function accesses unpowered I2C registers and causes unbalanced regulator disables if the device is runtime suspended.
> - [High] `mma8452_runtime_resume` modifies device active state and configuration registers without holding `data->lock`, causing a TOCTOU race with `mma8452_change_config`.
> - [High] Read-modify-write operations on I2C registers in sysfs operations are performed without holding `data->lock`.
> --
>
> --- Patch [5]: [PATCH v6 5/9] iio: accel: mma8452: Allow open drain interrupt pin configuration ---
> commit 0eb3a6b1fa62842d47690cc795da7896eb5b7153
> Author: Esben Haabendal <esben@geanix.com>
> Subject: iio: accel: mma8452: Allow open drain interrupt pin configuration
>
> This commit introduces the ability to configure the interrupt pin of mma8452
> chips in open-drain mode. It reads the "drive-open-drain" property during
> probe and updates the hardware configuration accordingly.
>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index 8eb97e6793d6f..d1e8eb2a4ad3a 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
> [ ... ]
>> @@ -1829,6 +1853,10 @@ static int mma8452_runtime_resume(struct device *dev)
>>
>>  	WRITE_ONCE(data->suspended, false);
>>
>> +	ret = mma8452_set_interrupt_pin_mode(data);
>> +	if (ret)
>> +		goto runtime_resume_failed;
>> +
>>  	ret = mma8452_active(data);
>>  	if (ret < 0)
>>  		goto runtime_resume_failed;
>
> [Severity: High]
> This is a pre-existing issue, but does mma8452_runtime_resume() fail to
> restore critical configurations?
>
> When the device autosuspends, mma8452_runtime_suspend() disables the VDD
> regulator, which cuts power to the chip and loses volatile register state.
> Upon resume, only CTRL_REG1 and CTRL_REG3 are restored.
>
> Are essential configurations set during probe (like DATA_CFG, TRANSIENT_THS,
> CTRL_REG4 for interrupt enables, and CTRL_REG5) lost after the first
> autosuspend?

Yes, I think they are lost. Looks like suspend/resume is not fully
implemented in this driver.

> [Severity: High]
> This is a pre-existing issue, but does mma8452_runtime_resume() introduce a
> race condition by not holding data->lock?
>
> The device state is modified to ACTIVE outside of the mutex lock here. If a
> concurrent sysfs write checks mma8452_is_active() and sees STANDBY, could
> this activation occur just before the I2C write executes?
>
> Would this cause the hardware to silently ignore the write since the device
> was unexpectedly transitioned to ACTIVE mode?

Yes. That sounds like something that could happen. But even more, all
the sysfs attributes does not even try to resume the device if it is
suspended. Which is also your next comment/issue.

So as mentioned above, suspend/resume implementation in this driver
seems very much incomplete.

> [Severity: High]
> This is a pre-existing issue, but do the sysfs read/write callbacks and
> write_raw paths access the device hardware over I2C without waking it from
> runtime suspend?
>
> drivers/iio/accel/mma8452.c:mma8452_write_raw() {
>     ...
> 	if (!iio_device_claim_direct(indio_dev))
> 		return -EBUSY;
>
> 	ret = __mma8452_write_raw(indio_dev, chan, val, val2, mask);
>     ...
> }
>
> Functions like mma8452_write_raw() and mma8452_show_hp_cutoff_avail() do
> direct I2C accesses without wrapping the operation in
> pm_runtime_resume_and_get() and pm_runtime_put().
>
> If the device is autosuspended, will the unpowered chip fail to ACK the
> transfers and return -EREMOTEIO?

Probably will.

> [Severity: High]
> This is a pre-existing issue, but does mma8452_remove() access unpowered I2C
> registers and cause unbalanced regulator disables?
>
> drivers/iio/accel/mma8452.c:mma8452_remove() {
>     ...
> 	mma8452_standby(iio_priv(indio_dev));
>
> 	regulator_disable(data->vddio_reg);
> 	regulator_disable(data->vdd_reg);
>     ...
> }
>
> If the device is runtime suspended, the vdd and vddio regulators are already
> disabled. Since pm_runtime_disable() does not wake the device,
> mma8452_remove() unconditionally calls mma8452_standby() which will time out
> on an unpowered bus.
>
> Will the subsequent unconditional regulator_disable() calls drive the usage
> counts below zero?

Yes and yes.

> [Severity: High]
> This is a pre-existing issue, but are read-modify-write operations on I2C
> registers in sysfs operations performed without holding data->lock?

Yes. But I don't think the data->lock is supposed to be used for this
purpose. It looks like the purpose is to protect the ACTIVE/STANDBY
state handling in mma8452_change_config().

>
> drivers/iio/accel/mma8452.c:mma8452_write_event_config() {
>     ...
> 	case IIO_EV_DIR_RISING:
> 		val = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);
> 		if (val < 0)
> 			return val;
>     ...
> }
>
> In functions like mma8452_write_event_config(), an I2C read of the
> configuration register happens completely outside data->lock.
>
> If two threads execute this concurrently, could they both read the old
> state, modify different bits, and serialize on mma8452_change_config(),
> causing the second thread to unconditionally overwrite the first thread's
> modifications?

Looks like a bug. I will try to write up a fix for this and all the
other similar race conditions that this driver seems to have quite a few
of.

/Esben

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