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

[sashiko-bot@kernel.org]

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 <esben@geanix.com>

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.

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

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

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-mma8452-open-drain-v3-0-6149f406a409@geanix.com?part=4