Re: [PATCH 2/5] iio: adc: Add ti-ads1262 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 15 potential issue(s) to consider:
- [High] Architecture Compatibility / Undefined Behavior due to direct bitfield mapping.
- [High] Out-of-Bounds Memory Access / Hardware Misconfiguration during active channel iteration.
- [High] Infinite Loop / Denial of Service in IDAC microamp parsing.
- [High] Logic Error: Inverted bounds check rejects valid pin numbers.
- [High] Race Condition / Lost Wakeup due to reinitializing completion after hardware trigger.
- [High] Missing Error Handling / Type Mismatch: Unsigned check of negative error code.
- [High] DMA Memory Corruption: Unaligned DMA buffer in union.
- [High] Uninitialized Memory Read (KMSAN) on stack variable.
- [High] Illegal DMA from Stack in regmap bus implementation.
- [High] Logic Error: GPIO get_multiple returns pin states incorrectly.
- [High] Hardware State / Timing violation during device reset.
- [High] Logic Error / Performance: Unconditional stall without IRQ.
- [Medium] Memory Leak of fwnode_handle reference on the error path.
- [Medium] Resource Leak of runtime PM usage counter on the error path.
- [Medium] API Contract Violation: Buffer mode lock released with direct mode API.
--

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

    iio: adc: Add ti-ads1262 driver
    
    Add ti-ads1262 driver for TI ADS1262 and ADS1263 ADCs with initial
    support for various features including IIO modes and channel configuration.

> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> new file mode 100644
> index 0000000000000..fd1911cf65ace
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1262.c
> @@ -0,0 +1,1816 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
[ ... ]
> +struct ads1262_channel {
> +	/* MODE0 */
> +	u8 conv_delay:4;
> +	u8 chop_mode:1;
> +	u8 idac_rot_mode:1;
> +	u8 runmode:1;
> +	u8 rev_vref_pol:1;
[ ... ]
> +static int ads1262_channel_enable_and_read(struct ads1262 *st,
> +					   struct ads1262_channel *chan,
> +					   __be32 *val)
> +{
[ ... ]
> +	mutex_lock(&st->chan_lock);
> +	memcpy(&st->tx[2], chan, sizeof(*chan));
> +	mutex_unlock(&st->chan_lock);

[Severity: High]
Does this code map a C bitfield directly to the SPI TX buffer? 

C bitfield layout is implementation-defined, so this can send garbage
configuration bits to the hardware on big-endian architectures.

> +struct ads1262 {
[ ... ]
> +	u8 tx[6] __aligned(IIO_DMA_MINALIGN);
> +	union {
> +		u8 rx[6];
> +		struct {
> +			__be32 data;
> +		} __packed shift_reg;
> +		struct {
> +			u8 dummy;
> +			__be32 data;
> +		} __packed holding_reg;
> +	};
> +};

[Severity: High]
Can this union cause DMA memory corruption?

Since the anonymous union lacks an __aligned(IIO_DMA_MINALIGN) attribute,
the RX buffers can share a cache line with surrounding struct members,
leading to memory corruption during DMA on non-cache-coherent architectures.

> +static int ads1262_dev_reset(struct ads1262 *st)
> +{
> +	int ret;
> +
> +	if (st->reset_gpiod)
> +		ret = gpiod_set_value_cansleep(st->reset_gpiod, 1);
> +	else
> +		ret = ads1262_dev_cmd(st, ADS1262_OPCODE_RESET);
> +	if (ret)
> +		return ret;
> +
> +	fsleep(1);

[Severity: High]
Is the 1 microsecond delay sufficient here? 

This short delay might violate hardware timing constraints for the device
reset, causing subsequent register writes to be ignored.

> +static int ads1262_read_calib(struct ads1262 *st, unsigned int reg, u32 *val)
> +{
> +	__le32 lval;
> +	int ret;
> +
> +	/*
> +	 * The calibration word is a signed 24 bit LSB-first value.
> +	 */
> +	ret = regmap_bulk_read(st->regmap, reg, &lval, 3);
> +	if (ret)
> +		return ret;
> +	*val = sign_extend32(le32_to_cpu(lval), 23);

[Severity: High]
Does this code evaluate an uninitialized byte?

The lval variable is 4 bytes long, but regmap_bulk_read only writes
3 bytes to it. The subsequent call to le32_to_cpu reads the
uninitialized 4th byte from the stack, which can trigger KMSAN warnings
and result in an incorrect sign extension.

> +static int ads1262_write_calib(struct ads1262 *st, unsigned int reg, u32 val)
> +{
> +	__le32 lval = cpu_to_le32(val);
> +
> +	/*
> +	 * The calibration word is a signed 24 bit LSB-first value.
> +	 */
> +	return regmap_bulk_write(st->regmap, reg, &lval, 3);
> +}

[Severity: High]
Can this result in an illegal DMA from the stack?

Calling regmap_bulk_write passes the lval stack variable to the
underlying SPI controller. This violates VMAP_STACK constraints and can
corrupt memory if the SPI controller uses DMA.

> +static void ads1262_wait_for_conversion(struct ads1262 *st)
> +{
> +	reinit_completion(&st->drdy);
> +
> +	/*
> +	 * The first conversion latency is affected by the channel's data rate,
[ ... ]
> +	wait_for_completion_timeout(&st->drdy, msecs_to_jiffies(6436));
> +}

[Severity: High]
Can wait_for_completion_timeout stall unconditionally if no IRQ is present?

Without a polling fallback, raw reads might block for the entire 6.4 second
timeout if the device isn't wired to an interrupt.

> +static int ads1262_channel_hot_reload(struct ads1262 *st,
> +				      const struct iio_chan_spec *chan)
> +{
[ ... ]
> +	weight = bitmap_weight(st->indio_dev->active_scan_mask,
> +			       iio_get_masklength(st->indio_dev));
> +	if (weight != 1) {
> +		iio_device_release_direct(st->indio_dev);
> +		return 0;
> +	}

[Severity: Medium]
Does this code release the wrong type of lock?

The iio_device_try_claim_buffer_mode API was used earlier to acquire
the buffer mode lock, but this path releases it using
iio_device_release_direct, violating the IIO locking contract.

> +static int ads1262_channel_read(struct ads1262 *st,
> +				struct ads1262_channel *chan_data,
> +				__be32 *val)
> +{
[ ... ]
> +	ret = ads1262_dev_start_one(st, runmode);
> +	if (ret)
> +		return ret;
> +
> +	ads1262_wait_for_conversion(st);

[Severity: High]
Is there a race condition with the completion here?

By calling ads1262_dev_start_one before ads1262_wait_for_conversion
(which re-initializes the completion), the hardware might fire the DRDY
interrupt before the completion is re-initialized, leading to a lost
wakeup.

> +static int ads1262_parse_idac_pins(struct ads1262 *st, u32 *pins,
> +				   unsigned int num_pins)
> +{
[ ... ]
> +		ret = device_property_read_u32(dev, propname, &val);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to read property %s\n",
> +					     propname);
> +		if (val < ADS1262_IDACMUX_COUNT)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "%s: Pin number out of range %d\n",
> +					     propname, val);

[Severity: High]
Is the bounds check logic inverted here?

Checking if val < ADS1262_IDACMUX_COUNT causes valid pin numbers to be
rejected with an error instead of catching out-of-range ones.

> +static int ads1262_parse_idac_mags(struct ads1262 *st, u32 *mags,
> +				   unsigned int num_mags)
> +{
[ ... ]
> +		for (mode = 0; mode < ARRAY_SIZE(idac_microamps); i++) {
> +			if (val == idac_microamps[mode])
> +				break;
> +		}

[Severity: High]
Can this create an infinite loop?

The loop iterates the mode variable but increments i (the outer loop's
induction variable), resulting in a lockup during probe.

> +static int ads1262_regulator_setup(struct ads1262 *st)
> +{
[ ... ]
> +	reg_id = "vref";
> +	st->vref_uV = devm_regulator_get_enable_read_voltage(dev, reg_id);
> +	if (st->vref_uV < 0)
> +		goto err_regulator_get;

[Severity: High]
Will this error check ever succeed?

The return value of devm_regulator_get_enable_read_voltage is stored
in st->vref_uV, which is an unsigned 32-bit integer. The negative
error code becomes a large positive number, making st->vref_uV < 0
statically false.

> +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;
> +	struct spi_transfer xfer[3] = {
[ ... ]
> +		{
> +			.rx_buf = val_buf,
> +			.len = val_size,
> +		},
> +	};
[ ... ]
> +	return spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer));
> +}

[Severity: High]
Does this custom regmap bus allow SPI DMA transfers directly to user-provided
buffers?

Passing val_buf directly into rx_buf might result in DMA to stack
memory if the caller passes a stack variable, violating VMAP_STACK constraints
and causing memory corruption.

> +static int ads1262_gpiochip_get_multiple(struct gpio_chip *gc,
> +					 unsigned long *mask,
> +					 unsigned long *bits)
> +{
> +	struct ads1262 *st = gpiochip_get_data(gc);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, ADS1262_GPIODAT_REG, &val);
> +	if (ret)
> +		return ret;
> +
> +	return val & *mask;
> +}

[Severity: High]
Does this code return the pin states incorrectly?

The GPIO get_multiple callback is expected to write the state to the
bits bitmap pointer, but instead it returns the state via the integer
return code.

> +static int ads1262_buffer_preenable(struct iio_dev *indio_dev)
> +{
[ ... ]
> +	iio_for_each_active_channel(indio_dev, i)
> +		ads1262_channel_set_runmode(st, &st->channels[i],
> +					    ADS1262_RUNMODE_CONTINUOUS);

[ ... ]
> +static int ads1262_fill_buffer_mult(struct ads1262 *st)
> +{
[ ... ]
> +	iio_for_each_active_channel(st->indio_dev, chan) {
> +		ret = ads1262_channel_enable_and_read(st, &st->channels[chan],
> +						      &val);

[Severity: High]
Can these loops access memory out of bounds?

iio_for_each_active_channel iterates over all active channels, including
the software timestamp channel if it is enabled. This can result in an
out-of-bounds array access on st->channels[chan] and misconfiguring the
hardware.

> +static int ads1262_aux_device_setup(struct ads1262 *st)
> +{
[ ... ]
> +out_node_put:
> +	fwnode_handle_put(node);
> +
> +	return ret;
> +}

[Severity: Medium]
Does this code leak the fwnode reference on the error path?

Because node is consumed and set to NULL by no_free_ptr(node) during
device_set_node, any subsequent failures jumping to out_node_put will
call fwnode_handle_put(NULL), leaving the reference leaked.

> +static int ads1262_gpiochip_request(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct ads1262 *st = gpiochip_get_data(gc);
> +	struct device *dev = &st->spi->dev;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(st->regmap, ADS1262_GPIOCON_REG,
> +				  BIT(offset), BIT(offset));
> +}

[Severity: Medium]
Does this leak the runtime PM usage counter on failure?

If regmap_update_bits fails, the function returns the error directly
without calling pm_runtime_put_autosuspend(dev) to balance the resume.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-ads126x-v1-0-894c788d03ed@gmail.com?part=2