Re: [PATCH v5 03/10] iio: adc: add the ti-ads1262 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] iio: adc: ti-ads1262: lockdep splat on regmap debugfs access
- [Medium] iio: adc: ti-ads1262: stale DRDY on interrupted channel read causes corrupted data
--

commit 5387f009404bbcc26fabb71c5f2ad0b5ba669607
Author: Kurt Borja <kuurtb@gmail.com>

    iio: adc: add the ti-ads1262 driver
    
    Add the ti-ads1262 driver with initial support for the primary ADC
    (ADC1). The ADS1263 auxiliary ADC (ADC2) is handled by a separate driver
    and interoperability considerations were taken into account.

> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> index 0000000000000..a54f78e88012c
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1262.c
[ ... ]
> +static int ads1262_channel_read(struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *spec, __be32 *val)
> +{
> +	struct ads1262 *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +	if (IIO_DEV_ACQUIRE_FAILED(claim))
> +		return -EBUSY;
> +
> +	ret = ads1262_set_runmode(st, ADS1262_RUNMODE_PULSE);
> +	if (ret)
> +		return ret;
> +
> +	ret = ads1262_channel_enable(st, spec);
> +	if (ret)
> +		return ret;
> +
> +	reinit_completion(&st->drdy);
> +
> +	ret = ads1262_dev_start_one(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ads1262_wait_for_conversion(st);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If ads1262_wait_for_conversion() is interrupted by a signal and returns
-ERESTARTSYS, does this leave the conversion running in the background?

If so, a stale DRDY interrupt from that background conversion might fire on
the next read exactly in the window between reinit_completion() and the
hardware processing the new START1 command. This would cause the wait to
complete prematurely and return corrupted or stale data to user space.

Is there a way to drain the hardware state or cancel the conversion when
the wait is interrupted?

[ ... ]
> +static int ads1262_regmap_read(void *context, const void *reg_buf,
> +			       size_t reg_size, void *val_buf, size_t val_size)
> +{
> +	struct ads1262 *st = context;
> +	u8 tx[2];
> +
> +	lockdep_assert_held(&st->xfer_lock);

[Severity: Medium]
Will this lockdep assertion fail when the regmap is accessed via debugfs?

Although the driver manually acquires st->xfer_lock before invoking regmap
updates, it does not wire this lock into the regmap core's configuration
(via custom lock/unlock callbacks, or by setting disable_locking = true).

As a result, the regmap core manages its own internal mutex and debugfs
accesses will bypass st->xfer_lock, calling into the bus callbacks directly
and triggering this assertion.

> +
> +	/*
> +	 * The register read operation uses a two byte command header followed
> +	 * by the register data:
> +	 *
> +	 *	byte 0:   RREG opcode | register address
> +	 *	byte 1:   number of registers to transfer, minus one
> +	 *	byte 2..: register data
> +	 */
> +	tx[0] = *(u8 *)reg_buf | ADS1262_OPCODE_RREG;
> +	tx[1] = val_size - 1;
> +
> +	return spi_write_then_read(st->spi, tx, sizeof(tx), val_buf, val_size);
> +}
> +
> +static int ads1262_regmap_gather_write(void *context, const void *reg_buf,
> +				       size_t reg_size, const void *val_buf,
> +				       size_t val_size)
> +{
> +	struct ads1262 *st = context;
> +	u8 tx[ADS1262_REGMAP_WRITE_SZ + 2];
> +
> +	lockdep_assert_held(&st->xfer_lock);

[Severity: Medium]
Similarly, will this assertion fail on regmap debugfs writes for the same
reasons?

> +
> +	/*
> +	 * The register write operation uses a two byte command header followed
> +	 * by the register data:
> +	 *
> +	 *	byte 0:   WREG opcode | register address
> +	 *	byte 1:   number of registers to transfer, minus one
> +	 *	byte 2..: register data
> +	 */
> +	tx[0] = *(u8 *)reg_buf | ADS1262_OPCODE_WREG;
> +	tx[1] = val_size - 1;
> +	memcpy(&tx[2], val_buf, val_size);
> +
> +	return spi_write_then_read(st->spi, tx, 2 + val_size, NULL, 0);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010-ads126x-v5-0-49e331cb2fed@gmail.com?part=3