Re: [PATCH v4 10/13] iio: adc: ad4134: Support SPI 4-wire mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] iio: ad4134: deadlock during probe due to recursive multiplexer locking in NO_CS mode
- [High] iio: adc: ad4134: debugfs reg access corrupts SPI mode during active capture
- [Medium] iio: ad4134: SPI mode state becomes permanently desynchronized on spi_setup() failure
--

commit f0535fb17ce60953154f2703e6049afff8114c19
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
suitable 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 b5207f16054e4..debc2ff027051 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c
[ ... ]
> @@ -226,6 +244,85 @@ static const struct regmap_access_table ad4134_regmap_wr_table = {
>  	.n_yes_ranges = ARRAY_SIZE(ad4134_regmap_wr_range),
>  };
>  
> +/*
> + * When operating in modes other than minimum I/O mode, AD4134 register data on
> + * the SDO line is sampled on SCLK rising edge (SPI mode 0). In addition to
> + * that, when AD4134 SDO and DOUT0 pins are multiplexed by hardware OUTSIDE
> + * AD4134 SILICON, the multiplexer state must be set to route SDO to the SPI
> + * controller. With both SPI mode and multiplexer state properly set, register
> + * access runs as usual. Successful calls to this function must be balanced with
> + * calls to ad4134_release_reg_access() so to release the mux state lock. See
> + * AD4134 IIO documentation for details.
> + */
> +static int ad4134_claim_reg_access(struct ad4134_state *st)
> +{
> +	unsigned int cur_mode = st->spi->mode & SPI_MODE_X_MASK;
> +	int ret;
> +
> +	if (st->spi_mode != AD4134_SPI_MODE_NO_CS && cur_mode != SPI_MODE_0) {
> +		st->spi->mode &= ~SPI_MODE_X_MASK;
> +		st->spi->mode |= SPI_MODE_0;
> +		ret = spi_setup(st->spi);
> +		if (ret)
> +			return ret;

[Severity: Medium]
If spi_setup() fails here, we return the error but st->spi->mode remains
modified. On subsequent calls, cur_mode != SPI_MODE_0 will evaluate to false
because the cached mode reflects the requested state.

Can this cause the driver to bypass spi_setup() on subsequent calls and
attempt to communicate with the device while the SPI hardware remains
incorrectly configured?

[Severity: High]
Can ad4134_debugfs_reg_access() execute concurrently with the active
triggered buffer capture?

ad4134_debugfs_reg_access() performs register operations without acquiring
the IIO direct mode lock. If executed concurrently with an active capture, it
will call ad4134_claim_reg_access() which modifies st->spi->mode and calls
spi_setup() without protection.

This abruptly alters the SPI hardware configuration while
ad4134_trigger_handler() is actively executing spi_sync_transfer(),
corrupting the ADC data stream and SPI transaction state.

Furthermore, if the "reg_access" and "data_read" mux states belong to
different hardware mux-controllers, the mux_state_select() call will not
block, causing concurrent, unprotected use of st->tx_buf and st->rx_buf
across threads.

> +	}
> +
> +	if (st->mux_st[AD4134_SDO_INPUT])
> +		return mux_state_select(st->mux_st[AD4134_SDO_INPUT]);
> +
> +	return 0;
> +}
[ ... ]
> @@ -594,10 +728,64 @@ static int ad4134_probe(struct spi_device *spi)
[ ... ]
> +	if (st->spi_mode == AD4134_SPI_MODE_NO_CS) {
> +		/*
> +		 * The support hardware for AD4134 may have a multiplexer for
> +		 * selecting between AD4134 SDO and AD4134 DOUT0. If that mux
> +		 * is set but the user still wants to run AD4134 in minimum I/O
> +		 * mode, then DOUT0 is not used and the multiplexer OUTSIDE OF
> +		 * AD4134 SILICON must be set to select AD4134 SDO. See AD4134
> +		 * IIO documentation for details.
> +		 */
> +		st->mux_st[AD4134_SDO_INPUT] =
> +			devm_mux_state_get_optional_selected(dev, "reg_access");

[Severity: High]
Does this cause a deadlock during device probe in NO_CS mode?

Here the driver acquires and permanently selects the SDO multiplexer via
devm_mux_state_get_optional_selected(), which acquires the multiplexer's
non-recursive semaphore.

Immediately after, probe calls ad4134_min_io_mode_setup(), which writes to
device registers via regmap_update_bits(). This triggers the following call
chain:

ad4134_min_io_mode_setup()
  regmap_update_bits()
    ad4134_reg_write()
      ad4134_claim_reg_access()

ad4134_claim_reg_access() unconditionally calls mux_state_select(), which
will attempt to acquire the exact same semaphore already held by the current
task, causing a deadlock.

Furthermore, ad4134_release_reg_access() then unconditionally calls
mux_state_deselect(), which would prematurely release the probe-acquired
lock.

> +		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");
> +
> +		ret = ad4134_min_io_mode_setup(st);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to setup minimum I/O mode\n");
> +	} else {
> +		/*

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