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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Lack of IRQ synchronization during buffer predisable leads to Use-After-Free/NULL-pointer dereference of `active_scan_mask`.
- [High] FIFO rollover and full condition handling causes an IRQ storm and deadlocks data capture.
- [High] Stale interrupt status flags are not cleared before polling in direct read mode, returning garbage data.
- [High] Interrupt handler returns IRQ_HANDLED on I2C read failure, causing a continuous IRQ storm.
- [High] Concurrent access to shared `fifo_raw` buffer causes data corruption and unsafe DMA memory sharing.
- [Medium] Missing Power-On Reset (POR) delay after enabling regulators causes I2C communication to fail.
--

--- Patch [2]: [PATCH v8 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver ---
commit d1ed22d53669726571ae3dc244a73238d9b89d76
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. The device has a 32-entry hardware FIFO with a configurable
almost-full interrupt.

> diff --git a/drivers/iio/health/max86150.c b/drivers/iio/health/max86150.c
> new file mode 100644
> index 0000000000000..1e3d28cb41b70
> --- /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;
> +
> +	ret = regmap_noinc_read(data->regmap, MAX86150_REG_FIFO_DATA,
> +				data->fifo_raw, MAX86150_SAMPLE_BYTES);

[Severity: High]
Are there concurrent access issues with the shared fifo_raw buffer?

The data->fifo_raw buffer appears to be shared between direct mode 
(max86150_read_raw) and buffer mode (max86150_interrupt_handler). Since 
there is no private mutex, both contexts might execute 
max86150_read_one_sample() simultaneously. 

Could this pass the same fifo_raw pointer to regmap_noinc_read() 
concurrently, potentially causing overlapping DMA maps or interleaved writes 
to the buffer?

[ ... ]

> +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);
> +	unsigned int ppg_rdy_status;
> +	u32 ppg_red, ppg_ir;
> +	s32 ecg;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +
> +		ret = regmap_clear_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> +					MAX86150_SYS_CTRL_SHDN);
> +		if (ret)
> +			goto out_shutdown;
> +
> +		ret = regmap_write(data->regmap, MAX86150_REG_FIFO_WR_PTR, 0);
> +		if (ret)
> +			goto out_shutdown;
> +		ret = regmap_write(data->regmap, MAX86150_REG_OVF_COUNTER, 0);
> +		if (ret)
> +			goto out_shutdown;
> +		ret = regmap_write(data->regmap, MAX86150_REG_FIFO_RD_PTR, 0);
> +		if (ret)
> +			goto out_shutdown;
> +
> +		/*
> +		 * Poll PPG_RDY rather than sleeping a fixed interval -- the
> +		 * internal oscillator may start slower than nominal, leaving
> +		 * the FIFO empty if we read too early.
> +		 */
> +		ret = regmap_read_poll_timeout(data->regmap,
> +					       MAX86150_REG_INT_STATUS1,
> +					       ppg_rdy_status,
> +					       ppg_rdy_status & MAX86150_INT_PPG_RDY,
> +					       1000, 25000);

[Severity: High]
Will this polling loop exit prematurely if there are stale interrupt status 
flags?

If MAX86150_INT_PPG_RDY was set during a previous active period, the 
INT_STATUS1 register might still hold this bit. Because the driver resets 
the FIFO pointers to 0 and immediately calls regmap_read_poll_timeout() 
without doing a dummy read to clear INT_STATUS1, the loop could exit 
immediately on the first iteration. 

The driver would then read an empty FIFO and return invalid data.

[ ... ]

> +static int max86150_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct max86150_data *data = iio_priv(indio_dev);
> +
> +	regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1, 0);
> +	regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> +			MAX86150_SYS_CTRL_SHDN);
> +	return 0;
> +}

[Severity: Critical]
Is it possible for the interrupt handler to execute after this function 
returns?

When the IIO buffer is disabled, this stops new hardware interrupts via I2C, 
but it doesn't call synchronize_irq(). The IIO core will then proceed to 
free indio_dev->active_scan_mask and set it to NULL.

If the threaded IRQ handler is currently executing or preempted on another 
CPU, won't it subsequently call test_bit(..., indio_dev->active_scan_mask) 
and dereference the NULL pointer?

[ ... ]

> +static irqreturn_t max86150_interrupt_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct max86150_data *data = iio_priv(indio_dev);
> +	unsigned int status, wr_ptr, rd_ptr, ovf;
> +	u32 ppg_red, ppg_ir;
> +	s32 ecg;
> +	s64 ts;
> +	int ret, n_avail, i, j;
> +
> +	ret = regmap_read(data->regmap, MAX86150_REG_INT_STATUS1, &status);
> +	if (ret)
> +		return IRQ_HANDLED;

[Severity: High]
Does returning IRQ_HANDLED on read failure risk causing an IRQ storm?

If the I2C read fails and the handler returns IRQ_HANDLED, the kernel will 
unmask the interrupt line. Because the interrupt is level-triggered and the 
device still holds the line asserted, it will immediately fire again. 
Returning IRQ_NONE would allow the kernel's spurious interrupt detector to 
safely disable the line.

[ ... ]

> +	ret = regmap_read(data->regmap, MAX86150_REG_FIFO_WR_PTR, &wr_ptr);
> +	if (ret)
> +		return IRQ_HANDLED;
> +	ret = regmap_read(data->regmap, MAX86150_REG_FIFO_RD_PTR, &rd_ptr);
> +	if (ret)
> +		return IRQ_HANDLED;
> +
> +	n_avail = (wr_ptr - rd_ptr) & (MAX86150_FIFO_DEPTH - 1);
> +	if (!n_avail)
> +		return IRQ_HANDLED;

[Severity: High]
Can n_avail evaluate to 0 when the FIFO is completely full?

If the CPU is delayed and the 32-entry FIFO fills completely, wr_ptr wraps 
and equals rd_ptr. The calculation (wr_ptr - rd_ptr) & 31 evaluates to 0.

Since the device initialization enables MAX86150_FIFO_CONFIG_ROLLOVER_EN, 
the hardware OVF_COUNTER is never incremented. The handler will see 
OVF_COUNTER == 0 and n_avail == 0, and exit with IRQ_HANDLED without 
reading any data. 

Could this cause an IRQ storm as the hardware re-asserts the A_FULL 
interrupt immediately because the FIFO remains full?

[ ... ]

> +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);
> +
> +	ret = devm_regulator_get_enable(&client->dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to get/enable vdd supply\n");
> +
> +	ret = devm_regulator_get_enable(&client->dev, "vled");
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to get/enable vled supply\n");
> +
> +	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");
> +
> +	ret = regmap_read(data->regmap, MAX86150_REG_PART_ID, &part_id);

[Severity: Medium]
Is a Power-On Reset delay needed after enabling the regulators?

The regulators might need time to stabilize, and the device typically 
requires a startup period before its I2C interface becomes responsive. 
If regmap_read() happens immediately after enabling the supplies, can this 
lead to an I2C communication failure during probe?

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