Re: [PATCH v3 4/4] iio: accel: mma8452: Allow open drain interrupt pin configuration

<[email protected]> writes:

> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
>
> New issues:
> - [High] Shared IRQ handler unconditionally accesses I2C bus while the device may be runtime-suspended.
> - [Medium] Incorrect shared interrupt handling falsely claims unrouted interrupts, masking spurious IRQs.
>
> Pre-existing issues:
> - [High] Volatile register context is not fully restored after PM runtime resume.
> - [High] Unbalanced regulator disable and unpowered I2C access during device removal.
> - [High] Sysfs callbacks modify cached state and access I2C without a PM runtime reference.
> - [High] Reference count leak of the IIO trigger device due to unbalanced iio_trigger_get().
> - [High] The driver ignores the requested IRQ polarity, leading to a hardware/software mismatch.
> --
>
> --- Patch [4]: [PATCH v3 4/4] iio: accel: mma8452: Allow open drain interrupt pin configuration ---
> commit 6358f754e42af1003c7e9459771157fe3985f6bb
> Author: Esben Haabendal <[email protected]>
>
> iio: accel: mma8452: Allow open drain interrupt pin configuration
>
> This patch allows configuring the MMA8452 interrupt pin in open-drain
> mode. It reads the "drive-open-drain" device property and sets the PP_OD
> bit in CTRL_REG3. It also adds the IRQF_SHARED flag to allow sharing the
> interrupt line with other devices.
>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index 723ca3d9cb96b..fa0915530f953 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>
> [ ... ]
>
>> @@ -1688,7 +1712,7 @@ static int mma8452_probe(struct i2c_client *client)
>>  		irq_flags = irq_get_trigger_type(client->irq);
>>  		if (irq_flags == IRQ_TYPE_NONE)
>>  			irq_flags = IRQF_TRIGGER_LOW;
>
> [Severity: High]
> This is a pre-existing issue, but the driver appears to ignore the requested
> IRQ polarity.
>
> While mma8452_probe() retrieves the requested IRQ trigger type to configure
> the OS handler, does the driver ever set the hardware IPOL bit (bit 1) in
> CTRL_REG3 to match it?
>
> The chip defaults to active-low, which might prevent interrupts from being
> delivered if the platform specifies an active-high interrupt.

This is per design. If IPOL bit is set based on the requested IRQ
polarity, support for system designs with additional hardware changing
the irq polarity (like a NOT gate) would not be possible.

If IPOL bit should be configurable, we probably need to add a dedicate
device-tree binding for it.

>> -		irq_flags |= IRQF_ONESHOT;
>> +		irq_flags |= IRQF_ONESHOT | IRQF_SHARED;
>
> [Severity: High]
> Does adding IRQF_SHARED here make it possible for the interrupt handler to
> run while the MMA8452 device is runtime-suspended?
>
> When another device on the shared line triggers an interrupt,
> mma8452_interrupt() is invoked and unconditionally reads MMA8452_INT_SRC
> over I2C:
>
> mma8452_interrupt()
>     src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
>
> If the device utilizes runtime PM and powers off its regulators when
> suspended, would this unpowered I2C access cause transfer failures or lock
> up the bus?

Oh. That does look like a problem.

I will add a fix for this for v4 of this series. And I will split the
shared interrupt support into a separate patch, as it is now supposed to
be usable without open-drain mode also.

> [Severity: Medium]
> Additionally, is the interrupt handler equipped to properly reject spurious
> interrupts on a shared line?
>
> With IRQF_SHARED, mma8452_interrupt() will run for other devices'
> interrupts. It unconditionally checks the DRDY bit:
>
> mma8452_interrupt()
>     if (src & MMA8452_INT_DRDY) {
>         iio_trigger_poll_nested(indio_dev->trig);
>         ret = IRQ_HANDLED;
>     }
>
> Since DRDY indicates data is ready even if the DRDY interrupt is not routed
> to the pin in CTRL_REG4, could this falsely claim unrouted interrupts and
> mask IRQ storms from a faulty shared line?

Maybe. I will send a patch with a fix for this when this series is merged.

>>  		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
>>  					   irq_flags, client->name, indio_dev);
>>  		if (ret)
>
> [ ... ]
>
>> @@ -1806,6 +1830,10 @@ static int mma8452_runtime_resume(struct device *dev)
>>  		return ret;
>>  	}
>>
>> +	ret = mma8452_set_interrupt_pin_mode(data);
>> +	if (ret)
>> +		goto runtime_resume_failed;
>> +
>>  	ret = mma8452_active(data);
>
> [Severity: High]
> This is a pre-existing issue, but does mma8452_runtime_resume() fully restore
> the volatile register context after a PM runtime suspend cycle?
>
> mma8452_runtime_suspend() disables regulators, cutting power to the chip.
> When resuming, CTRL_REG3 and CTRL_REG1 are restored, but are other volatile
> registers like DATA_CFG, TRANSIENT_THS, CTRL_REG4/5, and FF_MT_CFG left
> unconfigured?
>
> Could this cause the device to lose its sample rate, thresholds, and
> interrupt routing after the first auto-suspend cycle?
>
> While reviewing the PM runtime and cleanup paths, a few other pre-existing
> issues were noticed:
>
> [Severity: High]
> This is a pre-existing issue, but does mma8452_remove() safely handle
> removing a runtime-suspended device?
>
> mma8452_remove()
>     mma8452_standby(iio_priv(indio_dev));
>     regulator_disable(data->vddio_reg);
>     regulator_disable(data->vdd_reg);
>
> This unconditionally writes to I2C and disables regulators without
> guaranteeing the device is in an active PM state via pm_runtime_get_sync().
> If the device is already runtime-suspended, the regulators were already
> disabled. Could this double-disable regulators and cause unpowered I2C
> access?
>
> [Severity: High]
> This is a pre-existing issue, but do sysfs callbacks like
> __mma8452_write_raw() modify cached state and access I2C without a PM
> runtime reference?
>
> __mma8452_write_raw()
>     data->ctrl_reg1 &= ~MMA8452_CTRL_DR_MASK;
>     data->ctrl_reg1 |= i << MMA8452_CTRL_DR_SHIFT;
>     ...
>     return mma8452_change_config(data, MMA8452_CTRL_REG1, data->ctrl_reg1);
>
> If the device is in auto-suspend (regulators off) when user-space writes to
> sysfs attributes, would the I2C writes fail and corrupt the cached driver
> state?


As I am actually not using runtime PM with this device, I prefer to not
investigate these 3 issues further for now.

> [Severity: High]
> This is a pre-existing issue, but is there a reference count leak of the IIO
> trigger device?
>
> In mma8452_trigger_setup(), an extra reference is acquired:
>
>     indio_dev->trig = iio_trigger_get(trig);
>
> However, mma8452_trigger_cleanup() unregisters the trigger but does not
> appear to drop the reference with iio_trigger_put(). Could this permanently
> elevate the reference count and prevent the module from being unloaded?

Maybe. But in that case, this is a problem for basically all iio
drivers, as there are no explicit calls to iio_trigger_put() in any iio
drivers as of now.

/Esben