Re: [PATCH v5 2/3] iio: health: add MAX86150 ECG and PPG biosensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardware FIFO overrun and data corruption due to incorrect burst read length.
- [High] Infinite IRQ storm on level-triggered interrupts due to improper IRQF_ONESHOT handling, and potential DoS via missing .validate_device.
- [Medium] IIO trigger reference leak on probe error path.
--

commit 9590a122a97de73064d5ca132b0611828fac557d
Author: Md Shofiqul Islam <shofiqtest@gmail.com>

iio: health: add MAX86150 ECG and PPG biosensor driver

Add a new IIO driver for the Analog Devices MAX86150 integrated
biosensor, which combines two PPG optical channels (Red/IR LED) and
one ECG biopotential channel in a single I2C device.

> diff --git a/drivers/iio/health/max86150.c b/drivers/iio/health/max86150.c
> new file mode 100644
> index 0000000000000..92bf31f0a7615
> --- /dev/null
> +++ b/drivers/iio/health/max86150.c

[ ... ]

> +static int max86150_read_one_sample(struct max86150_data *data,
> +				    u32 *ppg_red, u32 *ppg_ir, s32 *ecg)
> +{
> +	int ret;
> +
> +	/*
> +	 * Use data->fifo_raw (heap memory) not a local array so the buffer is
> +	 * DMA-mappable for I2C host controllers that use DMA for burst reads.
> +	 */
> +	ret = regmap_noinc_read(data->regmap, MAX86150_REG_FIFO_DATA,
> +				data->fifo_raw, sizeof(data->fifo_raw));

[Severity: High]
Since data->fifo_raw is padded to ARCH_DMA_MINALIGN (e.g., 64 or 128 bytes)
for DMA safety, will sizeof(data->fifo_raw) cause the I2C burst read to pop
64 or 128 bytes from the hardware FIFO instead of the intended 9 bytes?

Could this destroy the 3-byte slot alignment and permanently drop data?

> +	if (ret)
> +		return ret;

[ ... ]

> +static const struct iio_trigger_ops max86150_trigger_ops = {
> +	.set_trigger_state = max86150_set_trigger_state,
> +};

[Severity: High]
Does the absence of .validate_device here allow external IIO devices to attach
to this trigger? 

If they do attach, could that prevent the consumer thread from clearing this 
device's interrupt, leading to a permanent IRQ storm?

[ ... ]

> +		irq_trig = irq_get_trigger_type(client->irq);
> +		if (!irq_trig)
> +			irq_trig = IRQF_TRIGGER_FALLING;
> +
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						NULL,
> +						iio_trigger_generic_data_rdy_poll,
> +						irq_trig | IRQF_ONESHOT,
> +						"max86150", data->trig);

[Severity: High]
For level-triggered interrupts, will IRQF_ONESHOT correctly mask the line
until the hardware is cleared?

Looking at iio_trigger_generic_data_rdy_poll(), it acts as the primary
threaded handler and returns IRQ_HANDLED immediately after scheduling the
consumer thread. 

Doesn't returning IRQ_HANDLED unmask the interrupt line before
max86150_trigger_handler() has a chance to read the hardware registers and
clear the source, causing an immediate IRQ storm?

> +		if (ret)
> +			return dev_err_probe(&client->dev, ret,
> +					     "Cannot request IRQ %d\n",
> +					     client->irq);
> +
> +		ret = devm_iio_trigger_register(&client->dev, data->trig);
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret,
> +					     "Cannot register trigger\n");
> +
> +		/*
> +		 * Set the default trigger so userspace can enable the buffer
> +		 * without a manual current_trigger write.  iio_device_unregister()
> +		 * calls iio_trigger_put() to release this reference.
> +		 */
> +		indio_dev->trig = iio_trigger_get(data->trig);
> +	}
> +
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      max86150_trigger_handler,
> +					      NULL);

[Severity: Medium]
If devm_iio_triggered_buffer_setup() fails here, is the trigger reference
acquired by iio_trigger_get() leaked?

Since INDIO_BUFFER_TRIGGERED is not set when buffer setup fails,
iio_dev_release() will skip calling iio_device_unregister_trigger_consumer().
Does this mean iio_trigger_put() is never called to clean up this reference?

> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Cannot setup triggered buffer\n");
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623201124.18271-1-shofiqtest@gmail.com?part=2