Re: [PATCH v11 2/2] 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] Silent buffer failure and userspace hang when probed without a valid IRQ.
- [High] The driver disables the hardware interrupt but fails to synchronize with the threaded IRQ handler, leading to a NULL pointer dereference and data races with sysfs reads.
- [Medium] The driver performs I2C reads immediately after enabling regulators, violating power-up timing requirements.
- [Medium] The timestamp calculation for the non-overflow case uses an incorrect formula that generates future timestamps when the IRQ handler is delayed.
- [Low] The driver uses nonexistent macros `IIO_DEV_ACQUIRE_DIRECT_MODE` and `IIO_DEV_ACQUIRE_FAILED` to claim direct mode, which will cause a build failure. Additionally, the acquired lock is never released.
--

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

This commit adds a new IIO driver for the MAX86150 combined ECG and PPG 
biosensor. It registers a direct kfifo buffer and uses a threaded IRQ handler 
to drain the hardware FIFO for sensor monitoring.

> diff --git a/drivers/iio/health/max86150.c b/drivers/iio/health/max86150.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/iio/health/max86150.c
[ ... ]
> +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: {
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +
> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;

[Severity: Low]
Are these macros defined in the IIO headers? IIO_DEV_ACQUIRE_DIRECT_MODE 
and IIO_DEV_ACQUIRE_FAILED appear to be missing from the IIO core headers, 
which will result in a compile failure.

> +
> +		ret = max86150_do_read_raw(data, &ppg_red, &ppg_ir, &ecg);
> +		if (ret)
> +			return ret;
> +
> +		switch (chan->scan_index) {
> +		case MAX86150_IDX_PPG_RED:
> +			*val = ppg_red;
> +			return IIO_VAL_INT;
> +		case MAX86150_IDX_PPG_IR:
> +			*val = ppg_ir;
> +			return IIO_VAL_INT;
> +		case MAX86150_IDX_ECG:
> +			*val = ecg;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +	default:
> +		return -EINVAL;

[Severity: Low]
If the direct mode locking macros were valid, would this code leave the 
acquired lock unreleased on these return paths?

> +	}
> +}
[ ... ]
> +static int max86150_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct max86150_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1, 0);

[Severity: High]
Does this predisable callback properly synchronize with the threaded IRQ 
handler? It masks hardware interrupts but does not wait for the Linux IRQ 
thread to finish, such as by calling disable_irq().

If the user disables the IIO buffer via sysfs, the IIO core could free and 
clear active_scan_mask while the concurrent IRQ thread is still running and 
dereferencing it in max86150_irq_handler().

This lack of synchronization could also lead to data races on the shared 
data->fifo_raw buffer if a concurrent read_raw sysfs access overlaps with the 
residual IRQ handler.

> +	if (ret)
> +		return ret;
> +
> +	return regmap_set_bits(data->regmap, MAX86150_REG_SYS_CTRL,
> +			       MAX86150_SYS_SHDN);
> +}
[ ... ]
> +	for (unsigned int i = 0; i < n_avail; i++) {
> +		unsigned int j;
> +		s64 ts;
> +
> +		if (ovf > 0)
> +			ts = irq_time -
> +			     (s64)(n_avail - 1 - i) * data->sample_period_ns;
> +		else
> +			ts = irq_time +
> +			     ((s64)i - (MAX86150_FIFO_A_FULL_SAMPLES - 1)) *
> +			     data->sample_period_ns;

[Severity: Medium]
Will this timestamp calculation generate future timestamps? 

When there is no overflow, but the threaded IRQ handler execution is delayed 
by scheduling, more than 17 samples can accumulate in the FIFO. If n_avail 
is greater than MAX86150_FIFO_A_FULL_SAMPLES, this formula projects the 
timestamps of the newest samples into the future relative to the delayed 
irq_time.

> +
> +		ret = max86150_read_one_sample(data, &ppg_red, &ppg_ir, &ecg);
> +		if (ret)
> +			break;
[ ... ]
> +static int max86150_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct max86150_data *data;
> +	unsigned int part_id;
> +	int ret;
[ ... ]
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(max86150_supply_names),
> +					     max86150_supply_names);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable supplies\n");
> +
> +	ret = regmap_read(data->regmap, MAX86150_REG_PART_ID, &part_id);

[Severity: Medium]
Is a delay required before communicating with the device?

The driver performs an I2C read to MAX86150_REG_PART_ID immediately after 
enabling regulators via devm_regulator_bulk_get_enable(). Without an 
intervening power-up delay, the first I2C transaction might fail against an 
uninitialized chip.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot read part ID\n");
[ ... ]
> +	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> +					  &max86150_buffer_setup_ops);
> +	if (ret)
> +		return ret;
> +
> +	if (client->irq > 0) {
> +		ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +						max86150_irq_handler,

[Severity: High]
What happens to the IIO buffer if the device is probed without a valid IRQ?

The buffer setup operations will configure the hardware to generate 
interrupts, but if this IRQ registration is skipped, no handler will ever 
drain the FIFO. This could cause a userspace process blocking on a device 
read to hang indefinitely.

> +						IRQF_ONESHOT,
> +						"max86150", indio_dev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

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