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

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

Pre-existing issues:
- [High] Missing synchronize_irq() in runtime suspend callback for a shared interrupt.
- [High] The IRQ handler incorrectly proceeds with I2C reads when the device's PM runtime is disabled.
- [High] Unprotected I2C hardware accesses in IIO callbacks without waking the device via PM runtime.
--

--- Patch [6]: [PATCH v4 6/6] iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe() ---
commit b1acc18af426345046769c66c8ed674e254ba4d0
Author: Esben Haabendal <[email protected]>

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

This commit updates mma8452_probe() to reuse the existing local dev pointer
instead of repeatedly dereferencing client->dev. This simplifies the code
and improves readability.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7992f9e388180..2fbf9f9ed9d75 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed an issue in
mma8452_runtime_suspend():

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

The device requests an interrupt with IRQF_SHARED, but regulators are disabled
without waiting for shared IRQ handlers to complete.

Could an interrupt fire for another device on the same line, causing the
mma8452_interrupt() handler to execute while this device is physically
unpowered? Should we add synchronize_irq() before disabling the regulators?


[Severity: High]
This isn't a bug introduced by this patch, but looking at mma8452_interrupt():

    pm_status = pm_runtime_get_if_active(&data->client->dev);
    if (pm_status == 0)
        return IRQ_NONE; /* device is powered down */

The strict check for 0 ignores the -EINVAL error returned when PM runtime is
disabled. If pm_runtime_disable() is called before free_irq() during driver
removal, pm_runtime_get_if_active() returns -EINVAL.

Does this cause the handler to incorrectly treat -EINVAL as active and proceed
to issue I2C transactions to an unpowered device?


[Severity: High]
This isn't a bug introduced by this patch, but I noticed unprotected I2C
hardware accesses in some IIO callbacks without waking the device via PM
runtime.

In mma8452_read_raw():

    case IIO_CHAN_INFO_CALIBBIAS:
        ret = i2c_smbus_read_byte_data(data->client,
                                       MMA8452_OFF_X +
                                       chan->scan_index);

And in mma8452_read_event_config():

    case IIO_EV_DIR_RISING:
        ret = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);

These branches perform direct I2C reads without first invoking
pm_runtime_resume_and_get() to ensure the device is powered.

If userspace reads corresponding sysfs attributes while the device is in
auto-suspend, will this result in I/O errors and bus timeouts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6