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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Probe fails unconditionally when optional regulators are absent.
- [High] The `fifo_raw` buffer is not cacheline aligned, violating DMA API rules and risking memory corruption.
- [High] FIFO pointer math cannot distinguish an empty FIFO from an exactly full FIFO, permanently stalling capture.
- [High] Trigger reference leak on error path if triggered buffer setup fails.
- [High] The biosensor is fully powered on and pulsing its LEDs continuously while the driver is bound, causing excessive power drain.
- [Medium] The driver hardcodes `IRQF_TRIGGER_FALLING`, overriding the device tree interrupt configuration.
- [Medium] The driver lacks a `validate_trigger` callback, allowing incompatible external software triggers to corrupt FIFO reads.
- [Medium] Per-sample timestamps are incorrectly reconstructed backwards from the most recent sample, introducing load-dependent jitter.
--

commit b7b1aa92d36a3970970aa6f40becae60ad1ec8c5
Author: Md Shofiqul Islam <shofiqtest@gmail.com>
Subject: iio: health: add MAX86150 ECG and PPG biosensor driver

This commit adds an I2C driver for the MAX86150 ECG and PPG biosensor,
exposing three IIO channels. It includes a hardware FIFO triggered buffer
path and devm regulator support.

> diff --git a/drivers/iio/health/max86150.c b/drivers/iio/health/max86150.c
> --- /dev/null
> +++ b/drivers/iio/health/max86150.c
> @@ -113,8 +113,8 @@ struct max86150_data {
>  	struct regmap		*regmap;
>  	struct device		*dev;
>  	struct iio_trigger	*trig;
>  	u32			 sample_period_ns;
>  	u8			 fifo_raw[MAX86150_SAMPLE_BYTES];
>  	s32 buf[6] __aligned(8);
>  };

[Severity: High]
Does this struct layout risk memory corruption during DMA burst reads?

Since data->fifo_raw is passed to regmap_noinc_read(), which may perform DMA
burst transfers on certain I2C host controllers, it lacks cacheline alignment.

If the CPU flushes dirty cache lines encompassing the adjacent buf during the
DMA transfer, wouldn't the flush clobber the incoming DMA data? Could we add
__aligned(IIO_DMA_MINALIGN) to fifo_raw?

[ ... ]

> @@ -286,4 +286,4 @@ static int max86150_read_raw(struct iio_dev *indio_dev,
>  static const struct iio_info max86150_iio_info = {
>  	.read_raw = max86150_read_raw,
>  };

[Severity: Medium]
Should max86150_iio_info implement a validate_trigger callback?

This device relies on its hardware FIFO being correctly flushed and configured
only when its own hardware trigger is enabled via .set_trigger_state. Without
a validate_trigger check like iio_validate_own_trigger, userspace could bind
arbitrary software triggers (like sysfs or hrtimer).

If an external trigger is used, .set_trigger_state is bypassed, the hardware
interrupt is never enabled, and wouldn't the driver blindly read unaligned,
duplicated, or stale samples from the continuously running FIFO?

[ ... ]

> @@ -375,6 +375,6 @@ static irqreturn_t max86150_trigger_handler(int irq, void *p)
>  	if (ovf > 0)
>  		n_avail = MAX86150_FIFO_DEPTH;
>  	else
>  		n_avail = (wr_ptr - rd_ptr) & (MAX86150_FIFO_DEPTH - 1);

[Severity: High]
Does this logic correctly handle an exactly full FIFO?

If the FIFO reaches exactly 32 samples (completely full) but has not yet
overflowed (ovf == 0), wr_ptr will wrap and equal rd_ptr. This calculation
evaluates to n_avail = 0, causing the driver to skip reading any samples.

Because the interrupt is configured as edge-triggered, the line won't assert
again until the FIFO drops below the almost-full threshold. Will this
permanently stall the triggered buffer capture?

> @@ -387,4 +387,4 @@ static irqreturn_t max86150_trigger_handler(int irq, void *p)
>  	for (i = 0; i < n_avail; i++) {
>  		s64 ts = pf->timestamp -
>  			 (s64)(n_avail - 1 - i) * data->sample_period_ns;

[Severity: Medium]
Are the per-sample timestamps aligned correctly?

iio_pollfunc_store_time() records the interrupt capture time in pf->timestamp
when the FIFO hits the almost-full watermark (17 samples). If the threaded
IRQ handler is delayed by system load, additional samples may accumulate.

This loop assigns pf->timestamp to the newest sample (i = n_avail - 1)
instead of the watermark sample, backdating the rest. Doesn't this variable
scheduling latency shift all timestamps backwards and corrupt their regular
spacing?

[ ... ]

> @@ -441,7 +441,7 @@ static int max86150_chip_init(struct max86150_data *data)
>  
>  	/* PPG: 100 Hz sample rate, 16384 nA ADC full-scale range */
>  	ret = regmap_write(data->regmap, MAX86150_REG_PPG_CONFIG1,
>  			   FIELD_PREP(MAX86150_PPG_ADC_RGE,
>  				      MAX86150_PPG_ADC_RGE_16384) |
>  			   FIELD_PREP(MAX86150_PPG_SR,
>  				      MAX86150_PPG_SR_100HZ));

[Severity: High]
Is the biosensor intentionally left continuously powered on?

Because the driver doesn't implement IIO buffer setup_ops (e.g., preenable
and postdisable) or Runtime PM, the hardware remains active permanently after
initialization.

Won't the optical LEDs continuously pulse at 100 Hz (drawing ~50 mA) even
when no userspace process is capturing data?

[ ... ]

> @@ -528,10 +528,10 @@ static int max86150_probe(struct i2c_client *client)
>  	 * DT regulator node.
>  	 */
>  	ret = devm_regulator_get_enable_optional(&client->dev, "vdd");
>  	if (ret)
>  		return dev_err_probe(&client->dev, ret,
>  				     "Failed to get/enable vdd supply\n");
>  
>  	ret = devm_regulator_get_enable_optional(&client->dev, "leds");
>  	if (ret)
>  		return dev_err_probe(&client->dev, ret,
>  				     "Failed to get/enable leds supply\n");

[Severity: High]
Does the driver fail to probe on valid configurations without regulators?

If a board supplies power via fixed rails and omits the DT regulator nodes,
devm_regulator_get_enable_optional() correctly returns -ENODEV.

By directly forwarding ret to dev_err_probe() and aborting, doesn't this
prevent the driver from binding on hardware that intentionally omits these
optional regulators?

[ ... ]

> @@ -575,6 +575,6 @@ static int max86150_probe(struct i2c_client *client)
>  		ret = devm_request_irq(&client->dev, client->irq,
>  				       iio_trigger_generic_data_rdy_poll,
>  				       IRQF_TRIGGER_FALLING,
>  				       "max86150", data->trig);

[Severity: Medium]
Is it safe to hardcode IRQF_TRIGGER_FALLING here?

Statically setting the IRQF_TRIGGER_FALLING flag overrides any trigger type
specified by the firmware in the device tree's interrupts property.

Will this break compatibility with board layouts that use an inverter or
level-shifter on the interrupt line? Should it pass 0 or IRQF_ONESHOT
instead to allow the core to apply the DT-defined polarity?

[ ... ]

> @@ -602,8 +602,8 @@ static int max86150_probe(struct i2c_client *client)
>  		 * the iio_dev is freed.  No additional devm action needed.
>  		 */
>  		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: High]
Is there a trigger reference leak if the buffer setup fails?

iio_trigger_get() increments the default trigger's reference count. The IIO
core only balances this in iio_dev_release() if the INDIO_ALL_TRIGGERED_MODES
mask flag is set in indio_dev->modes.

Since this flag is set inside devm_iio_triggered_buffer_setup(), if the
buffer setup fails, the flag is never set. Consequently, the devm cleanup path
will skip iio_device_unregister_trigger_consumer(). Doesn't this permanently
leak the trigger device memory and its module reference?

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