Re: [PATCH v2 11/15] iio: adc: ad4134: Support SPI 4-wire mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ad4134: ignored mux_state_try_select() errors mask failures and cause bus contention
- [High] ad4134: unconditional mux state deselection corrupts multiplexer semaphores
- [Medium] ad4134: concurrent regmap accesses corrupt IIO buffer hardware state
- [Medium] ad4134: direct assignment to spi->mode clobbers devicetree SPI flags
--

commit 9b8961eadc30a7239344b617959f3ffedd9a5a63
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

iio: adc: ad4134: Support SPI 4-wire mode
    
AD4134 devices can be wired in a few different ways. So far, only minimum
I/O mode was supported. While minimum I/O mode allows interfacing with
AD4134 with a reduced number of wires, that wiring configuration is not
optimal for high-throughput data acquisition.

Extend AD4134 support to enable interfacing in SPI 4-wire configuration.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 0b6843bf8a9e3..cc6bc325f6ee6 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c

[ ... ]

> @@ -230,6 +252,85 @@ static const struct regmap_access_table ad4134_regmap_wr_table = {
>  	.n_yes_ranges = ARRAY_SIZE(ad4134_regmap_wr_range),
>  };
>  
> +/*
> + * This function controls a multiplexer OUTSIDE OF AD4134 SILICON.
> + * When AD4134 SDO and DOUT0 pins are multiplexed, this function changes the
> + * multiplexer state to route SDO to the SPI controller. See AD4134 IIO
> + * documentation for details.
> + */
> +static int ad4134_set_register_access(struct ad4134_state *st)
> +{
> +	int ret;
> +
> +	guard(mutex)(&st->access_mode_lock);
> +
> +	st->spi->mode = SPI_MODE_0;

[Severity: Medium]
Do these direct assignments clear all other SPI flags populated by the SPI
core from devicetree properties, such as SPI_CS_HIGH, SPI_LSB_FIRST, or
SPI_3WIRE?

Should this instead use SPI_MODE_X_MASK to safely update only the CPOL/CPHA
bits?

> +	ret = spi_setup(st->spi);
> +	if (ret)
> +		return ret;
> +
> +	ret = mux_state_deselect(st->mux_st[AD4134_DOUT0_INPUT]);

[Severity: High]
Does this unconditionally deselect the DOUT0 multiplexer state without
verifying it is currently selected?

This could call up() on the semaphore and cause a semaphore count imbalance.

> +	if (ret)
> +		dev_err(&st->spi->dev, "error on DOUT0 deselect: %d\n", ret);
> +
> +	/*
> +	 * Try to update the multiplexer state to route SDO to the SPI
> +	 * controller. If the SDO mux state is BUSY (i.e. locked), that means
> +	 * the desired state is already selected and we should not block here.
> +	 */
> +	ret = mux_state_try_select(st->mux_st[AD4134_SDO_INPUT]);
> +	if (ret && ret != -EBUSY)
> +		return ret;

[Severity: High]
Is it safe to explicitly ignore the -EBUSY error here?

If the lock is actually held by another driver sharing the multiplexer,
proceeding could result in doing SPI transfers on a misrouted bus, leading
to contention and corrupted data transfers.

> +
> +	return 0;
> +}
> +
> +/*
> + * This function controls a multiplexer OUTSIDE OF AD4134 SILICON.
> + * When AD4134 SDO and DOUT0 pins are multiplexed, this function changes the
> + * multiplexer state to route DOUT0 to the SPI controller. On failure, fall
> + * back to routing SDO to the controller and return an errno. See AD4134 IIO
> + * documentation for details.
> + */
> +static int ad4134_set_sample_access(struct ad4134_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int mux_state_ret, ret;
> +
> +	guard(mutex)(&st->access_mode_lock);
> +
> +	ret = mux_state_deselect(st->mux_st[AD4134_SDO_INPUT]);

[Severity: High]
Is it safe to manually unlock the devres-managed mux state unconditionally
here?

The SDO mux is acquired using devm_mux_state_get_selected() during probe,
which registers a devm cleanup to deselect it on unbind. Manually unlocking
it could risk a semaphore imbalance when devm cleanup runs.

> +	if (ret)
> +		dev_err(&st->spi->dev, "error on SDO deselect: %d\n", ret);
> +
> +	ret = mux_state_try_select(st->mux_st[AD4134_DOUT0_INPUT]);
> +	if (ret) {
> +		dev_err(&st->spi->dev, "error on DOUT0 select: %d\n", ret);
> +		return mux_state_select(st->mux_st[AD4134_SDO_INPUT]);
> +	}

[Severity: High]
If DOUT0 selection fails, does returning the success code of the recovery
function (mux_state_select) swallow the original error code?

This might allow IIO buffer enablement to succeed while the hardware is
incorrectly routed to SDO, resulting in garbage data.

> +
> +	/*
> +	 * Data output on the DOUT lines is sampled on the falling edge
> +	 * (SPI mode 1).
> +	 */
> +	st->spi->mode = SPI_MODE_1;

[Severity: Medium]
Similar to register access mode, does this direct assignment clear all other
SPI flags, potentially breaking SPI communication on platforms requiring extra
SPI flags?

> +	ret = spi_setup(st->spi);

[ ... ]

> @@ -333,9 +434,24 @@ static int ad4134_register_read(struct ad4134_state *st, unsigned int reg,
>  static int ad4134_reg_read(void *context, unsigned int reg, unsigned int *val)
>  {
>  	struct ad4134_state *st = context;
> +	int ret, access_ret;
> +
> +	if (reg >= AD4134_CH_VREG(0)) {
> +		if (st->spi_mode == AD4134_SPI_MODE_NO_CS)
> +			return ad4134_data_read(st, reg, val);
> +
> +		ret = ad4134_set_sample_access(st);
> +		if (ret)
> +			return ret;
>  
> -	if (reg >= AD4134_CH_VREG(0))
> -		return ad4134_data_read(st, reg, val);
> +		ret = ad4134_data_read(st, reg, val);
> +
> +		access_ret = ad4134_set_register_access(st);

[Severity: Medium]
If a user reads a register via debugfs while the IIO buffer is active, does
this unconditionally revert the SPI and multiplexer state to register access
mode?

This could clobber the active IIO buffer's expected hardware state
(SPI_MODE_1 and DOUT0), causing subsequent trigger handler reads to fail
or receive garbage.

Also, for standard registers, does ad4134_reg_read() fall through to
ad4134_register_read(), where SPI transfers are initiated without holding
access_mode_lock and without verifying the hardware is in register access
mode?

> +		if (access_ret)
> +			dev_err(&st->spi->dev, "access mode error: %d\n", access_ret);
> +
> +		return ret;
> +	}
>  
>  	return ad4134_register_read(st, reg, val);
>  }

[ ... ]

> @@ -586,15 +717,68 @@ static int ad4134_probe(struct spi_device *spi)

[ ... ]

> +	} else {
> +		/*
> +		 * This controls a multiplexer OUTSIDE OF AD4134 SILICON.
> +		 * See AD4134 IIO documentation for details.
> +		 */
> +		st->mux_st[AD4134_SDO_INPUT] = devm_mux_state_get_selected(dev, "reg_access");

[Severity: High]
Does this lock the mux control and tie its unlock to driver unbind, making the
manual unlock seen in ad4134_set_sample_access() unsafe?

> +		if (IS_ERR(st->mux_st[AD4134_SDO_INPUT]))
> +			return dev_err_probe(dev, PTR_ERR(st->mux_st[AD4134_SDO_INPUT]),
> +					     "failed to get reg_access mux-state\n");
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789494473.git.marcelo.schmitt@analog.com?part=11