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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] iio: adc: ad4134: broken ACQUIRE() and DEFINE_GUARD_COND() for access claims
- [Medium] iio: adc: ad4134: duplicate devm_mux_state_get() call in ad4134_probe()
--

commit 2e7d5e9974efe0835378f504b67e632ce2d53ac2
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

iio: adc: ad4134: Support SPI 4-wire mode

This commit adds support for interfacing with the AD4134 ADC in SPI 4-wire
mode using an external multiplexer to switch between register access on SDO
and ADC sample data reads on DOUT0.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 4252244a23bc9..4efb63c45aa29 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c

[ ... ]

> @@ -221,6 +239,99 @@ 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 spi_mode = st->spi->mode & SPI_MODE_X_MASK;
> +	int ret;
> +
> +	if (st->spi_mode != AD4134_SPI_MODE_NO_CS && spi_mode != SPI_MODE_0) {
> +		st->spi->mode &= ~SPI_MODE_X_MASK;
> +		st->spi->mode |= SPI_MODE_0;
> +		ret = spi_setup(st->spi);
> +		if (ret) {
> +			st->spi->mode &= ~SPI_MODE_X_MASK;
> +			st->spi->mode |= spi_mode;
> +			return ret;
> +		}
> +	}
> +
> +	if (st->spi_mode != AD4134_SPI_MODE_NO_CS && st->mux_st[AD4134_SDO_INPUT])
> +		return mux_state_select(st->mux_st[AD4134_SDO_INPUT]);
> +
> +	return 0;
> +}
> +
> +/*
> + * Must be balanced with successful calls to ad4134_claim_reg_access().
> + */
> +static int ad4134_release_reg_access(struct ad4134_state *st)
> +{
> +	if (st->spi_mode != AD4134_SPI_MODE_NO_CS && st->mux_st[AD4134_SDO_INPUT])
> +		return mux_state_deselect(st->mux_st[AD4134_SDO_INPUT]);
> +
> +	return 0;
> +}
> +
> +DEFINE_GUARD(__ad4134_reg_access_mode, struct ad4134_state *,
> +	     ad4134_claim_reg_access(_T), ad4134_release_reg_access(_T));
> +DEFINE_GUARD_COND(__ad4134_reg_access_mode, _try, ad4134_claim_reg_access(_T));
> +
> +/*
> + * When operating in modes other than minimum I/O mode, AD4134 ADC sample data
> + * on the DOUT lines is sampled on SCLK falling edge (SPI mode 1). 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 DOUT0 to the SPI
> + * controller. With both SPI mode and multiplexer state properly set, ADC
> + * samples can be read normally. Successful call to this function must be paired
> + * with a call to ad4134_release_sample_access().
> + */
> +static int ad4134_claim_sample_access(struct ad4134_state *st)
> +{
> +	unsigned int spi_mode = st->spi->mode & SPI_MODE_X_MASK;
> +	int ret;
> +
> +	if (st->spi_mode != AD4134_SPI_MODE_NO_CS && spi_mode != SPI_MODE_1) {
> +		st->spi->mode &= ~SPI_MODE_X_MASK;
> +		st->spi->mode |= SPI_MODE_1;
> +		ret = spi_setup(st->spi);
> +		if (ret) {
> +			st->spi->mode &= ~SPI_MODE_X_MASK;
> +			st->spi->mode |= spi_mode;
> +			return ret;
> +		}
> +	}
> +
> +	if (st->spi_mode != AD4134_SPI_MODE_NO_CS && st->mux_st[AD4134_DOUT0_INPUT])
> +		return mux_state_select(st->mux_st[AD4134_DOUT0_INPUT]);
> +
> +	return 0;
> +}
> +
> +/*
> + * Must be balanced with successful calls to ad4134_claim_sample_access().
> + */
> +static int ad4134_release_sample_access(struct ad4134_state *st)
> +{
> +	if (st->spi_mode != AD4134_SPI_MODE_NO_CS && st->mux_st[AD4134_DOUT0_INPUT])
> +		return mux_state_deselect(st->mux_st[AD4134_DOUT0_INPUT]);
> +
> +	return 0;
> +}
> +
> +DEFINE_GUARD(__ad4134_sample_access_mode, struct ad4134_state *,
> +	     ad4134_claim_sample_access(_T), ad4134_release_sample_access(_T));
> +DEFINE_GUARD_COND(__ad4134_sample_access_mode, _try, ad4134_claim_sample_access(_T));
> +
>  static int ad4134_calc_spi_crc(u8 inst, u8 data)
>  {
>  	u8 buf[] = { inst, data };
> @@ -246,6 +357,10 @@ static int ad4134_reg_write(void *context, unsigned int reg, unsigned int val)
>  	};
>  	int ret;
>  
> +	ACQUIRE(__ad4134_reg_access_mode, claim)(st);
> +	if (ACQUIRE_ERR(__ad4134_reg_access_mode, &claim))
> +		return -EBUSY;
> +
>  	ad4134_prepare_spi_tx_buf(reg, val, st->tx_buf);
>  
>  	ret = spi_sync_transfer(st->spi, &xfer, 1);
> @@ -265,6 +380,10 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,
>  	u32 sample = 0;
>  	int ret;
>  
> +	ACQUIRE(__ad4134_sample_access_mode, claim)(st);
> +	if (ACQUIRE_ERR(__ad4134_sample_access_mode, &claim))
> +		return -EBUSY;
> +
>  	/*
>  	 * To be able to read data from all 4 channels through a single line, we
>  	 * set DOUTx output format to 0 in the digital interface config register
> @@ -305,6 +424,10 @@ static int ad4134_register_read(struct ad4134_state *st, unsigned int reg,
>  	unsigned int inst;
>  	int ret;
>  
> +	ACQUIRE(__ad4134_reg_access_mode, claim)(st);
> +	if (ACQUIRE_ERR(__ad4134_reg_access_mode, &claim))
> +		return -EBUSY;

[Severity: High]
Are these guard definitions and ACQUIRE() calls handling claim failures as
intended?

In ad4134_reg_write(), ad4134_data_read(), and ad4134_register_read(),
ACQUIRE() and ACQUIRE_ERR() are called with the unconditional guard classes
__ad4134_reg_access_mode and __ad4134_sample_access_mode rather than the
conditional _try variants (__ad4134_reg_access_mode_try and
__ad4134_sample_access_mode_try).

DEFINE_GUARD() defines a constructor of the form ({ _lock; _T; }), which
discards the return value of ad4134_claim_reg_access(st) or
ad4134_claim_sample_access(st) and unconditionally initializes claim to the
valid pointer st. As a result, ACQUIRE_ERR(..., &claim) always evaluates to 0.
If spi_setup() or mux_state_select() fails, the error is ignored, the SPI
transfer runs with the wrong SPI mode or mux routing, and scope exit runs
ad4134_release_*_access() -> mux_state_deselect(), calling up(&mux->lock) on
a semaphore that was never acquired.

Additionally, the conditional guards use the 3-argument DEFINE_GUARD_COND()
macro:

include/linux/cleanup.h:
#define DEFINE_GUARD_COND_3(_name, _ext, _condlock) \
	DEFINE_GUARD_COND_4(_name, _ext, _condlock, _RET)

Because ad4134_claim_reg_access() and ad4134_claim_sample_access() return 0
on success and a negative errno on failure, the default _RET condition treats
0 as failure and negative error codes as success. Should DEFINE_GUARD_COND()
pass _RET == 0 as the fourth argument, and should the callers use
__ad4134_reg_access_mode_try and __ad4134_sample_access_mode_try?

[ ... ]

> @@ -593,10 +727,64 @@ static int ad4134_probe(struct spi_device *spi)

[ ... ]

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

[Severity: Medium]
In ad4134_probe(), when st->spi_mode is not AD4134_SPI_MODE_NO_CS,
devm_mux_state_get(dev, "data_read") is called twice in succession, and the
second call's result in st->mux_st[AD4134_DOUT0_INPUT] is later overwritten
by mux_st without checking for errors.

Was the second devm_mux_state_get(dev, "data_read") call left in by mistake
when introducing the mux_st local variable, leaving an extra struct mux_state
allocation and mux_chip reference held for the lifetime of the device?

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