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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Stale A_FULL interrupt status bit is not cleared in buffer_postenable, leading to immediate spurious interrupts and data corruption.
- [High] The threaded interrupt handler can execute while the IIO buffer is disabled, leading to a fatal NULL pointer dereference on active_scan_mask.
- [Medium] The driver bypasses the IIO core's clock selection mechanism by using ktime_get_ns() instead of iio_get_time_ns() for soft timestamps.
- [Low] Stale sensor data is leaked to user-space via untouched padding elements in the local data->buf array.
--

--- Patch [2]: [PATCH v9 2/2] iio: health: add MAX86150 ECG and PPG biosensor driver ---
commit 9f8edea36d720ecd87ad044648a96413f3156ea3
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..e8394031f3acb
> --- /dev/null
> +++ b/drivers/iio/health/max86150.c

[ ... ]

> +static int max86150_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct max86150_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_clear_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> +				MAX86150_SYS_CTRL_SHDN);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_WR_PTR, 0);
> +	if (ret)
> +		goto err_shutdown;
> +	ret = regmap_write(data->regmap, MAX86150_REG_OVF_COUNTER, 0);
> +	if (ret)
> +		goto err_shutdown;
> +	ret = regmap_write(data->regmap, MAX86150_REG_FIFO_RD_PTR, 0);
> +	if (ret)
> +		goto err_shutdown;
> +	ret = regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1,
> +			   MAX86150_INT_A_FULL);

[Severity: High]
Does this unmask the interrupt without first clearing the stale A_FULL
status in MAX86150_REG_INT_STATUS1?

If the FIFO was full when the buffer was previously stopped, the A_FULL
bit remains latched. When the interrupt is unmasked here with
zero-initialized FIFO pointers, couldn't the latched bit immediately
trigger the handler, causing it to read and push 32 samples of garbage?

> +	if (ret)
> +		goto err_shutdown;
> +	return 0;
> +
> +err_shutdown:
> +	regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> +			MAX86150_SYS_CTRL_SHDN);
> +	return ret;
> +}
> +
> +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);
> +	/*
> +	 * Mask the hardware interrupt first, then synchronize to ensure any
> +	 * threaded handler already in flight completes before the IIO core
> +	 * clears active_scan_mask; without this a delayed handler would
> +	 * dereference a NULL active_scan_mask.
> +	 */
> +	synchronize_irq(data->irq);

[Severity: High]
Is synchronize_irq() sufficient to prevent the handler from running after
the buffer is disabled?

If a spurious interrupt fires while the buffer is disabled, the OS-level
IRQ is still active since disable_irq() wasn't called. Wouldn't the
threaded handler run and unconditionally dereference
indio_dev->active_scan_mask (which the IIO core clears to NULL after
predisable), causing a fatal crash?

> +	return 0;
> +}

[ ... ]

> +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, n_avail;

[ ... ]

> +	/*
> +	 * Anchor timestamps to the interrupt time: sample (n_avail - 1) is
> +	 * the newest and corresponds to ts; earlier samples are back-calculated
> +	 * by one sample_period_ns per step.
> +	 */
> +	ts = ktime_get_ns();

[Severity: Medium]
Should this use iio_get_time_ns(indio_dev) instead of ktime_get_ns()?

Using ktime_get_ns() unconditionally generates timestamps from
CLOCK_MONOTONIC, which ignores the user's selected IIO clock source
(such as REALTIME or BOOTTIME) configured via sysfs.

> +
> +	for (i = 0; i < n_avail; i++) {
> +		s64 sample_ts = ts -
> +			(s64)(n_avail - 1 - i) * data->sample_period_ns;
> +
> +		ret = max86150_read_one_sample(data, &ppg_red, &ppg_ir, &ecg);
> +		if (ret)
> +			break;
> +
> +		j = 0;
> +		if (test_bit(MAX86150_IDX_PPG_RED, indio_dev->active_scan_mask))
> +			data->buf[j++] = ppg_red;
> +		if (test_bit(MAX86150_IDX_PPG_IR, indio_dev->active_scan_mask))
> +			data->buf[j++] = ppg_ir;
> +		if (test_bit(MAX86150_IDX_ECG, indio_dev->active_scan_mask))
> +			data->buf[j++] = ecg;
> +
> +		iio_push_to_buffers_with_ts(indio_dev, data->buf,
> +					    sizeof(data->buf), sample_ts);

[Severity: Low]
Does pushing sizeof(data->buf) leak stale sensor data in untouched padding
elements?

If a user enables 3 channels, reads data, then disables the buffer and
restarts it with only 1 channel enabled (e.g., ECG), data->buf is reused.
The IIO core pushes the ECG sample and uses the untouched data->buf[1]
element as padding to align the timestamp, leaking the stale PPG data
from the previous 3-channel scan.

> +	}
> +
> +	return IRQ_HANDLED;
> +}

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