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

[Marcelo Schmitt]

On 09/29, sashiko-bot@xxxxxxxxxx wrote:
...
> > +/*
> > + * 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 spi_setup() fails it will be tried again on the next register or sample read.
So, the device would be desynchronized only when not being used.

...
> >  	/*
> >  	 * 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?

The default on device reset is to output data from all four channels serialized
through DOUT0. Anyways, will make the configuration explicit in case the device
is not reset.

> > +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?

I'm not sure about this one. The intent is to prevent sysfs regmap access while
the buffer is enabled. IIO direct/buffer mode locking should provide that, no?
The rationale for the two mutexes is for access_lock to avoid concurrent updates
to the external multiplexer and for sync_lock to protect read/modify/write
sequences. Maybe IIO direct/buffer mode locking + sync_lock is enough?

> 
> > +
> > +	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?
> 
Not sure. Should it lock on a mutex before editing the regmap_config. Maybe
I get rid of access_lock and make regmap_config const again.


> [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?

Okay, that seems to be an issue. Will make the device request the GPIO if
offload/PWM is not available.

> 
> > +		/*
> > +		 * 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?
> 
NO_CS/minimum I/O mode doesn't need to constantly switch the external
multiplexer and can leave that in reg_access state. Also, since minimum I/O
always operate in SPI mode 0, nothing that access_lock protects is really used.