Re: [PATCH v5 6/8] iio: imu: adis16607: Add FIFO-based buffered/triggered capture

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iio: imu: adis16607: level IRQ storm via has_fifo in adis16607_probe()
--

commit 4d8ecf6737569661c0f9a47d7a29ff4680559b17
Author: Radu Sabau <radu.sabau@analog.com>

iio: imu: adis16607: Add FIFO-based buffered/triggered capture

This commit adds buffered capture support to the adis16607 IMU driver using
the device's hardware FIFO, draining samples in a trigger handler signaled by
the level-high DEV_NOK interrupt and exposing watermark sysfs attributes.

> diff --git a/drivers/iio/imu/adis16607.c b/drivers/iio/imu/adis16607.c
> index 5e2c999e68ac5..bd7752f7a1ec8 100644
> --- a/drivers/iio/imu/adis16607.c
> +++ b/drivers/iio/imu/adis16607.c

[ ... ]

> @@ -229,6 +263,304 @@ static const struct adis_ops adis16607_ops = {

[ ... ]

> +/*
> + * Drain the device FIFO. Triggered by the DEV_NOK interrupt, which fires on
> + * any DIAG_STAT flag, not just FIFO_THRESH_MET - the DIAG_STAT read below
> + * both checks which flag(s) raised the interrupt and clears the latched
> + * status, and must happen before any FIFO pop, or we'd pop for nothing on
> + * an unrelated flag.
> + */
> +static void adis16607_fifo_drain(struct iio_dev *indio_dev, s64 irq_timestamp)
> +{
> +	struct adis16607 *st = iio_priv(indio_dev);
> +	u8 fifo_sample[ADIS16607_FIFO_DATA_LEN];
> +	u16 diag_stat, word_cnt, sample_cnt;
> +	s64 timestamp;
> +	int ret;
> +
> +	adis_dev_auto_lock(&st->adis);
> +
> +	/* Check if FIFO threshold is met */
> +	ret = __adis_read_reg_16(&st->adis, ADIS16607_REG_DIAG_STAT, &diag_stat);

[ ... ]

> +static irqreturn_t adis16607_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +
> +	adis16607_fifo_drain(indio_dev, pf->timestamp);
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}

[ ... ]

> @@ -737,6 +1098,7 @@ static const struct adis_data adis16607_data = {
>  	.prod_id_reg = ADIS16607_REG_DEV_ID,
>  	.prod_id = 0x6000,
>  	.unmasked_drdy = true,
> +	.has_fifo = true,
>  	.init = adis16607_init,
>  	.self_test = adis16607_self_test,
>  	.status_error_msgs = adis16607_status_error_msgs,

[ ... ]

> @@ -855,6 +1220,25 @@ static int adis16607_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * DEV_NOK stays asserted until DIAG_STAT is read, so the interrupt
> +	 * must be level-triggered, active high.
> +	 */
> +	irq_type = irq_get_trigger_type(spi->irq);
> +	if (irq_type != IRQ_TYPE_LEVEL_HIGH)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid interrupt type 0x%x specified\n",
> +				     irq_type);
> +
> +	st->adis.irq_flag = IRQF_TRIGGER_HIGH;
> +
> +	ret = devm_adis_setup_buffer_and_trigger_with_attrs(&st->adis, indio_dev,
> +							    adis16607_trigger_handler,
> +							    &adis16607_buffer_ops,
> +							    adis16607_fifo_attributes);

[Severity: High]
Does setting has_fifo = true in adis16607_data combined with a level-high
interrupt and devm_adis_setup_buffer_and_trigger_with_attrs() in
adis16607_probe() cause a kernel warning and a level-IRQ storm when buffered
capture is enabled?

When adis->data->has_fifo is set, devm_adis_probe_trigger() in
drivers/iio/imu/adis_trigger.c requests the physical SPI interrupt as a
threaded IRQ with iio_trigger_generic_data_rdy_poll() as the thread function:

	if (adis->data->has_fifo)
		ret = devm_request_threaded_irq(&adis->spi->dev, adis->spi->irq,
						NULL,
						&iio_trigger_generic_data_rdy_poll,
						adis->irq_flag | IRQF_ONESHOT,
						indio_dev->name,
						adis->trig);

Meanwhile, devm_adis_setup_buffer_and_trigger_with_attrs() registers
adis16607_trigger_handler() on a separate virtual subirq thread via
devm_iio_triggered_buffer_setup_ext() with iio_pollfunc_store_time() as its
top-half handler.

When the level-high DEV_NOK interrupt fires, irq_thread_fn() runs
iio_trigger_generic_data_rdy_poll() in process context with local CPU
interrupts enabled. That calls iio_trigger_poll() -> generic_handle_irq() ->
handle_simple_irq() -> handle_irq_event() -> __handle_irq_event_percpu(),
which runs iio_pollfunc_store_time() and then hits the hardirq-context check
in kernel/irq/handle.c:__handle_irq_event_percpu():

	if (WARN_ONCE(!irqs_disabled(),"irq %u handler %pS enabled interrupts\n",
		      irq, action->handler))
		local_irq_disable();

More importantly, __handle_irq_event_percpu() only wakes the separate pollfunc
consumer thread for adis16607_trigger_handler() asynchronously and returns
immediately. iio_trigger_generic_data_rdy_poll() then returns IRQ_HANDLED to
irq_thread_fn(), which calls irq_finalize_oneshot() and unmasks the physical
level-high spi->irq line before adis16607_trigger_handler() ->
adis16607_fifo_drain() has run and read ADIS16607_REG_DIAG_STAT over SPI.

Because DEV_NOK remains asserted high until ADIS16607_REG_DIAG_STAT is read in
adis16607_fifo_drain(), unmasking spi->irq immediately after waking the
consumer thread causes the level-high interrupt to re-fire in an interrupt
storm (and can starve the consumer thread on a single CPU). In addition,
because no validate_trigger or validate_device callback is set, binding
another device to this trigger or vice versa would leave nothing reading
ADIS16607_REG_DIAG_STAT to clear DEV_NOK.

Since hardware FIFO capture is the only buffered mode supported by this
device, should adis16607_probe() register the buffer with
devm_iio_kfifo_buffer_setup_ext() and request the threaded IRQ directly
instead of going through an IIO trigger?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-adis16607-v5-0-4f98b0d271ac@analog.com?part=6