Re: [PATCH v6 2/2] iio: temperature: add support for Analog Devices MAX30210

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] iio: temperature: max30210: IRQ requested after
  devm_iio_device_register()
- [Medium] iio: max30210: status bits discarded in
  max30210_buffer_postenable()
--

commit 421e9f8d7f502a8b5bdb83438a4e54f4019fbac4
Author: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>

iio: temperature: add support for Analog Devices MAX30210

This commit adds an IIO driver for the Analog Devices MAX30210 I2C
temperature sensor with support for direct reads, threshold events, and a
hardware FIFO buffer.

> diff --git a/drivers/iio/temperature/max30210.c b/drivers/iio/temperature/max30210.c
> new file mode 100644
> index 0000000000000..8866a476eb37c
> --- /dev/null
> +++ b/drivers/iio/temperature/max30210.c

[ ... ]

> +/* The core calls this after hwfifo_set_watermark(), so the watermark is stable here. */
> +static int max30210_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct max30210_state *st = iio_priv(indio_dev);
> +	unsigned int status;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = max30210_autoconv_get(st);
> +	if (ret)
> +		return ret;
> +
> +	/* Read STATUS to clear a stale A_FULL so it cannot fire on the flushed FIFO. */
> +	ret = regmap_read(st->regmap, MAX30210_STATUS_REG, &status);
> +	if (!ret)
> +		ret = regmap_set_bits(st->regmap, MAX30210_INT_EN_REG,
> +				      MAX30210_STATUS_A_FULL_MASK);
> +	if (ret)
> +		max30210_autoconv_put(st);
> +
> +	return ret;
> +}

[Severity: Medium]
Can reading and discarding MAX30210_STATUS_REG here drop pending
temperature threshold events?

MAX30210_STATUS_REG is clear-on-read and shared with
MAX30210_STATUS_TEMP_HI_MASK and MAX30210_STATUS_TEMP_LO_MASK.  When
threshold events are already enabled (st->autoconv_users > 0), reading
MAX30210_STATUS_REG without reporting those bits clears any pending alarm
in hardware and causes a concurrent max30210_irq_handler() to read 0 and
return IRQ_NONE.

In addition, when st->autoconv_users == 0, calling max30210_autoconv_get()
before reading MAX30210_STATUS_REG starts autonomous conversions before
clearing the stale status byte.

[ ... ]

> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	if (!client->irq)
> +		return 0;
> +
> +	/*
> +	 * Request the IRQ after registering the IIO device so devm tears it
> +	 * down first on removal, before the event interface is freed.
> +	 */
> +	return devm_request_threaded_irq(dev, client->irq, NULL,
> +					 max30210_irq_handler, IRQF_ONESHOT,
> +					 indio_dev->name, indio_dev);
> +}

[Severity: Medium]
As reported by Jonathan Cameron on v4 [1], does registering the IIO device
before requesting client->irq expose the buffer and event interfaces
before the interrupt handler is installed, and invert the teardown order
on removal?

During probe, userspace can enable the buffer or threshold events (or
disable the buffer and hit synchronize_irq(st->irq) in
max30210_buffer_predisable()) before devm_request_threaded_irq() runs.  On
driver unbind, devres frees client->irq before devm_iio_device_unreg()
runs iio_device_unregister() -> iio_disable_all_buffers() ->
max30210_buffer_predisable(), which then calls synchronize_irq(st->irq) on
the already-freed IRQ, and before max30210_quiesce() disables hardware
interrupts.

The event interface is freed in iio_dev_release() (bound to
devm_iio_device_alloc()) rather than iio_device_unregister(), so
requesting the IRQ and registering max30210_quiesce() before
devm_iio_device_register() still tears down the IRQ before the event
interface is freed.

[1] https://lore.kernel.org/r/20261017230705.678a791c@jic23-hlaptop

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110021442.410-1-johnerasmusmari.geronimo@analog.com?part=2