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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] locking: mutex held across system calls in IIO buffer callbacks
- [High] iio: broken multi-channel readback in 4-wire fallback mode
- [High] iio: ODR GPIO uninitialized in 4-wire mode preventing software triggers
- [High] iio: data race on shared rx_buf in NO_CS mode
- [Medium] spi: local mode state desynchronized on spi_setup() failure
- [Medium] regmap: data race on global static ad4134_regmap_config
--

commit 26369b61324efb5e168b91e351dda0ac9d6716f3
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 f0196126515e6..50021001b3741 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c
[ ... ]
> @@ -226,6 +246,90 @@ 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. Still, to prevent concurrent hardware interface
> + * configurations, callers must hold the access_lock. Finally, 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) __must_hold(&st->access_lock)
> +{
> +	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, the local spi->mode keeps the SPI_MODE_0 value but the
hardware isn't updated.  On the next call, cur_mode will match SPI_MODE_0,
skipping spi_setup().  Could this leave the device permanently desynchronized
from the controller?

> +	}
> +
> +	if (st->mux_st[AD4134_SDO_INPUT])
> +		return mux_state_select(st->mux_st[AD4134_SDO_INPUT]);
> +
> +	return 0;
> +}
[ ... ]
> @@ -268,7 +381,11 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,
>  {
>  	unsigned int i;
>  	u32 sample;
> -	int ret;
> +	int access_ret, ret;
> +
> +	ret = ad4134_claim_sample_access(st);
> +	if (ret)
> +		return ret;
>  
>  	/*
>  	 * To be able to read data from all 4 channels through a single line, we
> @@ -284,7 +401,7 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,
>  		ret = spi_write_then_read(st->spi, NULL, 0, st->rx_buf,
>  					  BITS_TO_BYTES(AD4134_CHAN_PRECISION_BITS));

[Severity: High]
Since ad4134_min_io_mode_setup() is not called in 4-wire mode,
AD4134_DATA_FORMAT_SINGLE_CH_MODE is never set. Doesn't this mean
ad4134_data_read() blindly assumes the data is serialized on DOUT0 when it
isn't? Could this result in reading Channel 0 data four times instead of the
other channels?

>  		if (ret)
> -			return ret;
> +			goto out_data_read_release;
[ ... ]
> @@ -381,6 +526,41 @@ static irqreturn_t ad4134_trigger_handler(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> +static int ad4134_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4134_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&st->access_lock);
> +
> +	ret = ad4134_claim_sample_access(st);
> +	if (ret)
> +		goto out_unlock;
> +
> +	return 0;
> +
> +out_unlock:
> +	mutex_unlock(&st->access_lock);
> +	return ret;
> +}
> +
> +static int ad4134_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4134_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4134_release_sample_access(st);
> +
> +	mutex_unlock(&st->access_lock);

[Severity: High]
Is it intentional to hold st->access_lock across system calls here? Returning
to user space with the mutex held violates lock ownership semantics and can
trigger lockdep splats. Furthermore, could this deadlock any subsequent sysfs
regmap accesses while the buffer is enabled?

> +
> +	return ret;
> +}
[ ... ]
> @@ -588,15 +768,69 @@ static int ad4134_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_mutex_init(dev, &st->access_lock);
> +	if (ret)
> +		return ret;
> +
> +	ad4134_regmap_config.lock_arg = st;
>  	st->regmap = devm_regmap_init(dev, NULL, st, &ad4134_regmap_config);

[Severity: Medium]
Is it safe to modify the global ad4134_regmap_config structure during probe?
If multiple devices probe concurrently, could this cause a data race where one
device's lock_arg overwrites another's before devm_regmap_init() copies it?

>  	if (IS_ERR(st->regmap))
>  		return dev_err_probe(dev, PTR_ERR(st->regmap),
>  				     "failed to initialize regmap");
[ ... ]
> +	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");
> +		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 {

[Severity: High]
In 4-wire mode, ad4134_min_io_mode_setup() is bypassed. Since st->odr_gpio is
only retrieved inside ad4134_min_io_mode_setup(), won't it remain
uninitialized (NULL)? Will this cause all software triggers using
gpiod_set_value_cansleep(st->odr_gpio, ...) to silently fail in 4-wire mode?

> +		/*
> +		 * This controls a multiplexer OUTSIDE OF AD4134 SILICON.
> +		 * See AD4134 IIO documentation for details.
> +		 */
> +		st->mux_st[AD4134_SDO_INPUT] = devm_mux_state_get(dev, "reg_access");
> +		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");
> +
> +		st->mux_st[AD4134_DOUT0_INPUT] = devm_mux_state_get(dev, "data_read");
> +		if (IS_ERR(st->mux_st[AD4134_DOUT0_INPUT]))
> +			return dev_err_probe(dev, PTR_ERR(st->mux_st[AD4134_DOUT0_INPUT]),
> +					     "failed to get data_read mux-state\n");
> +
> +		indio_dev->setup_ops = &ad4134_buffer_setup_ops;
> +	}

[Severity: High]
Since setup_ops is only assigned for 4-wire mode, won't NO_CS mode bypass
ad4134_buffer_postenable() entirely?  If so, doesn't that mean
ad4134_trigger_handler() uses st->rx_buf without holding st->access_lock,
creating a data race with concurrent sysfs regmap accesses?

>  
>  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>  					      iio_pollfunc_store_time,

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