Re: [PATCH 1/1] iio: health: add MAX86150 ECG and PPG biosensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Buffer overflow during triggered buffer capture.
- [High] Missing IIO direct mode locking allows concurrent sysfs reads to silently corrupt the hardware FIFO state.
- [High] Calculation of available FIFO samples evaluates to 0 when the FIFO is fully saturated, leading to a permanent capture stall.
- [High] Buffer data packing ignores `active_scan_mask`, leading to data corruption when a subset of channels is enabled.
- [High] Requesting an IRQ with `IRQF_ONESHOT` without a threaded handler causes unconditional probe failure.
- [High] Hardware interrupt unconditionally enabled at boot can permanently stall the edge-triggered line.
- [Medium] UAPI Endianness Mismatch causing data corruption on Big Endian architectures.
- [Medium] Device is left running at full power and emitting interrupts when the driver is unloaded.
- [Medium] Documented DT regulators `vdd-supply` and `leds-supply` are ignored, causing initialization failure on unpowered platforms.
- [Medium] Unbalanced `iio_trigger_get()` causes a reference count memory leak.
--

commit 86527e62ebc7afa90aeb829c2bdf9c041cf99dd9
Author: Md Shofiqul Islam <shofiqtest@gmail.com>

iio: health: add MAX86150 ECG and PPG biosensor driver

The MAX86150 integrates PPG and ECG channels in a single I2C device. The
driver exposes three IIO channels and supports hardware-triggered continuous
low-overhead capture.

> diff --git a/drivers/iio/health/max86150.c b/drivers/iio/health/max86150.c
> new file mode 100755
> index 0000000000000..3cde4af6650b7
> --- /dev/null
> +++ b/drivers/iio/health/max86150.c

[ ... ]

> +struct max86150_data {
> +	struct regmap		*regmap;
> +	struct device		*dev;
> +	struct iio_trigger	*trig;
> +	s32  buf[4] __aligned(8);
> +};

[Severity: High]
Does this buffer have enough space for the 64-bit timestamp?

With three 32-bit channels (12 bytes) and a 64-bit timestamp needing 8-byte
alignment, the timestamp would be placed at byte offset 16. However, this
array is exactly 16 bytes.

Calling iio_push_to_buffers_with_timestamp() in max86150_trigger_handler()
could write the timestamp past the end of the array.

[ ... ]

> +static const struct iio_chan_spec max86150_channels[] = {
> +	{
> +		/* PPG Red LED - optical intensity, 19-bit unsigned */
> +		.type               = IIO_INTENSITY,
> +		.modified           = 1,
> +		.channel2           = IIO_MOD_LIGHT_RED,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index         = MAX86150_IDX_PPG_RED,
> +		.scan_type = {
> +			.sign        = 'u',
> +			.realbits    = 19,
> +			.storagebits = 32,
> +			.endianness  = IIO_LE,
> +		},

[Severity: Medium]
Since max86150_trigger_handler() places native integers directly into the
buffer without cpu_to_le32() conversions, will this advertise incorrect
endianness to userspace on big endian architectures?

[ ... ]

> +static int max86150_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct max86150_data *data = iio_priv(indio_dev);
> +	u32 ppg_red, ppg_ir;
> +	s32 ecg;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		/*
> +		 * Single-shot path: clear the FIFO so we know we are reading
> +		 * a sample that arrived after this call, not stale data.
> +		 */
> +		ret = regmap_write(data->regmap, MAX86150_REG_FIFO_WR_PTR, 0);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap, MAX86150_REG_OVF_COUNTER, 0);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap, MAX86150_REG_FIFO_RD_PTR, 0);

[Severity: High]
Does this path need iio_device_claim_direct_mode() to prevent concurrent
sysfs reads from silently corrupting the hardware FIFO pointers while a
continuous triggered buffer capture is active?

[ ... ]

> +static irqreturn_t max86150_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func	*pf   = p;
> +	struct iio_dev		*idev = pf->indio_dev;
> +	struct max86150_data	*data = iio_priv(idev);
> +	unsigned int status, wr_ptr, rd_ptr, available;
> +	u32 ppg_red, ppg_ir;
> +	s32 ecg;
> +	int ret;
> +
> +	/*
> +	 * Reading INT_STATUS1 clears the interrupt.  Do this before touching
> +	 * the FIFO so the pin is de-asserted while we drain samples.
> +	 */
> +	ret = regmap_read(data->regmap, MAX86150_REG_INT_STATUS1, &status);
> +	if (ret)
> +		goto done;
> +
> +	ret = regmap_read(data->regmap, MAX86150_REG_FIFO_WR_PTR, &wr_ptr);
> +	if (ret)
> +		goto done;
> +	ret = regmap_read(data->regmap, MAX86150_REG_FIFO_RD_PTR, &rd_ptr);
> +	if (ret)
> +		goto done;
> +
> +	/*
> +	 * Number of unread samples.  The FIFO is circular with depth 32, so
> +	 * wrap with (depth - 1) as mask.
> +	 */
> +	available = (wr_ptr - rd_ptr) & (MAX86150_FIFO_DEPTH - 1);

[Severity: High]
When the 32-entry FIFO becomes perfectly full and wr_ptr wraps around to
equal rd_ptr, will this evaluate to 0?

If so, the handler might skip the read loop entirely and leave the FIFO full,
causing the interrupt condition to persist and resulting in an endless
interrupt loop or a permanent capture stall.

> +
> +	while (available--) {
> +		ret = max86150_read_one_sample(data, &ppg_red, &ppg_ir, &ecg);
> +		if (ret)
> +			break;
> +
> +		data->buf[MAX86150_IDX_PPG_RED] = ppg_red;
> +		data->buf[MAX86150_IDX_PPG_IR]  = ppg_ir;
> +		data->buf[MAX86150_IDX_ECG]     = ecg;

[Severity: High]
If userspace enables only a subset of the channels (e.g., only ECG), will
this static indexing ignore the active_scan_mask?

IIO expects enabled channels to be tightly packed at the start of the buffer.
If only ECG is enabled, the IIO core calculates the timestamp offset based on
a single channel. The static writes here might cause the timestamp to later
overwrite the channel data, corrupting both the measurement and the
timestamp.

[ ... ]

> +static int max86150_chip_init(struct max86150_data *data)
> +{
> +	int ret;
> +
> +	/* Software reset; the bit self-clears within 1 ms */
> +	ret = regmap_write(data->regmap, MAX86150_REG_SYS_CTRL,
> +			   MAX86150_SYS_RESET);

[ ... ]

> +	/* Enable FIFO almost-full interrupt only */
> +	return regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1,
> +			    MAX86150_INT_A_FULL);
> +}

[Severity: Medium]
Are there teardown steps needed when the driver is unloaded?

This initialization starts continuous sampling, but no .remove callback or
devm action seems to be registered to write to MAX86150_SYS_SHDN or disable
the interrupts, leaving the device fully active on unload.

[Severity: High]
Could enabling the hardware interrupt here before the IIO buffer is active
cause the edge-triggered interrupt pin to get permanently stuck?

If the sensor asserts the active-low pin early, the hardirq handler will
fire but won't read INT_STATUS1 to clear the line (since the threaded handler
is only active when the buffer is enabled). The interrupt controller might
ignore the stuck low line forever, breaking capture once the buffer is
actually started.

[ ... ]

> +static int max86150_probe(struct i2c_client *client)
> +{
> +	struct iio_dev		*indio_dev;
> +	struct max86150_data	*data;
> +	unsigned int		 part_id;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data     = iio_priv(indio_dev);
> +	data->dev = &client->dev;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &max86150_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
> +				     "Failed to initialise regmap\n");

[Severity: Medium]
Will this fail to probe on unpowered platforms since the documented
devicetree regulators vdd-supply and leds-supply are not requested or
enabled before I2C communication starts?

[ ... ]

> +	if (client->irq > 0) {
> +		data->trig = devm_iio_trigger_alloc(&client->dev,
> +						    "%s-dev%d",
> +						    indio_dev->name,
> +						    iio_device_id(indio_dev));
> +		if (!data->trig)
> +			return -ENOMEM;
> +
> +		ret = devm_request_irq(&client->dev, client->irq,
> +				       iio_trigger_generic_data_rdy_poll,
> +				       IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +				       "max86150", data->trig);

[Severity: High]
Will the irq core reject this request during probe?

The interrupt core requires a threaded handler when IRQF_ONESHOT is used,
but iio_trigger_generic_data_rdy_poll is passed as the hardirq handler with
no threaded handler provided.

> +		if (ret)
> +			return dev_err_probe(&client->dev, ret,
> +					     "Cannot request IRQ %d\n",
> +					     client->irq);
> +
> +		iio_trigger_set_drvdata(data->trig, indio_dev);
> +
> +		ret = devm_iio_trigger_register(&client->dev, data->trig);
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret,
> +					     "Cannot register trigger\n");
> +
> +		indio_dev->trig = iio_trigger_get(data->trig);
> +	}

[Severity: Medium]
Is there a corresponding iio_trigger_put() needed here? Without it, the
trigger reference count could be leaked when the device is unbound or if a
subsequent probe step fails.

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