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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The driver fails to assign `indio_dev->trig`, causing `iio_trigger_validate_own_device` to unconditionally reject the trigger.
- [High] Using a hard IRQ handler for the IIO trigger without interrupt masking causes a system-locking interrupt storm for level-triggered interrupts.
- [Medium] Timestamp reconstruction assigns inaccurate timestamps into the past when the hardware FIFO overflows because it ignores the `OVF_COUNTER` dropped sample count.
- [Medium] The I2C DMA buffer `fifo_raw` shares a cacheline with the pushed CPU buffer `buf`, violating DMA API constraints.
- [Medium] A blind sleep in `read_raw` risks reading an empty FIFO if the internal oscillator or device wake-up is slower than nominal.
--

commit 44deffd1bb30e8d4384d605ba5d156fb29678ad5
Author: Md Shofiqul Islam <shofiqtest@gmail.com>

iio: health: add MAX86150 ECG and PPG biosensor driver

The MAX86150 (Maxim/Analog Devices) combines two PPG optical channels
(Red/IR LED) and one ECG biopotential channel in a single I2C device with
a 32-entry hardware FIFO.

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

[ ... ]

> +struct max86150_data {
> +	struct regmap		*regmap;
> +	struct device		*dev;
> +	struct iio_trigger	*trig;
> +	u32			 sample_period_ns;
> +	u8			 fifo_raw[MAX86150_SAMPLE_BYTES] __aligned(ARCH_DMA_MINALIGN);
> +	s32 buf[6] __aligned(8);
> +};

[Severity: Medium]
Since fifo_raw is 9 bytes long, does the __aligned macro only align the
start of the array, leaving the buf array packed into the same DMA cacheline?

Mapping an unpadded buffer that shares a cacheline with CPU-accessed data
might violate kernel DMA mapping constraints and trigger
CONFIG_DMA_API_DEBUG warnings.

[ ... ]

> +static int max86150_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{

[ ... ]

> +		/* Wait for one complete sample period at 100 Hz (<= 10 ms) */
> +		usleep_range(11000, 13000);
> +
> +		ret = max86150_read_one_sample(data, &ppg_red, &ppg_ir, &ecg);

[Severity: Medium]
Is it possible for the hardware read to execute while the FIFO is still
empty if the internal oscillator or device wake-up is slower than nominal?

Would it be safer to use regmap_read_poll_timeout on the PPG_RDY status
bit to ensure a sample is actually ready?

[ ... ]

> +static irqreturn_t max86150_trigger_handler(int irq, void *p)
> +{

[ ... ]

> +	/*
> +	 * Anchor timestamps to the A_FULL IRQ capture time: sample index
> +	 * (MAX86150_A_FULL_SAMPLES - 1) corresponds to pf->timestamp.
> +	 * Samples that accumulated between the IRQ and handler execution
> +	 * receive timestamps in the future relative to the IRQ, eliminating
> +	 * load-dependent jitter in multi-sample drains.
> +	 */
> +	for (i = 0; i < n_avail; i++) {
> +		s64 ts = pf->timestamp +
> +			 (s64)(i - (MAX86150_A_FULL_SAMPLES - 1)) *
> +			 data->sample_period_ns;

[Severity: Medium]
When the FIFO overflows (ovf > 0), the hardware overwrites the oldest
samples. Does this calculation assign inaccurate timestamps into the past
because it ignores the OVF_COUNTER dropped sample count?

The remaining 32 samples are chronologically shifted forward by ovf relative
to the original A_FULL IRQ event.

[ ... ]

> +static int max86150_probe(struct i2c_client *client)
> +{

[ ... ]

> +	/*
> +	 * If the device tree provides an interrupt, set up a hardware
> +	 * trigger so userspace can use the FIFO almost-full signal to
> +	 * drive capture without polling.
> +	 */
> +	if (client->irq > 0) {
> +		unsigned long irq_trig;
> +
> +		data->trig = devm_iio_trigger_alloc(&client->dev,
> +						    "%s-dev%d",
> +						    indio_dev->name,
> +						    iio_device_id(indio_dev));
> +		if (!data->trig)
> +			return -ENOMEM;
> +
> +		data->trig->ops = &max86150_trigger_ops;
> +		iio_trigger_set_drvdata(data->trig, indio_dev);

[Severity: High]
Does the driver fail to assign indio_dev->trig = data->trig after
allocating the trigger?

Since validate_trigger is set to iio_trigger_validate_own_device, it looks
like missing this assignment will cause any attempt to write to the
current_trigger sysfs node to unconditionally fail with -EINVAL.

> +
> +		/*
> +		 * Honour the interrupt trigger type from the device tree.
> +		 * Fall back to falling-edge if the DT does not specify one.
> +		 */
> +		irq_trig = irq_get_trigger_type(client->irq);
> +		if (!irq_trig)
> +			irq_trig = IRQF_TRIGGER_FALLING;
> +
> +		ret = devm_request_irq(&client->dev, client->irq,
> +				       iio_trigger_generic_data_rdy_poll,
> +				       irq_trig,
> +				       "max86150", data->trig);

[Severity: High]
Will using a hard IRQ handler for the IIO trigger without interrupt masking
cause an interrupt storm for level-triggered interrupts?

For level-triggered interrupts, the hard handler wakes the consumer thread
and returns IRQ_HANDLED without clearing the physical interrupt via I2C.
The IRQ core then unmasks the still-asserted interrupt line, causing it to
immediately fire again and preventing the threaded consumer from ever
running to clear the source.

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