Re: [PATCH v4 03/10] iio: adc: add the ti-ads1262 driver

[Jonathan Cameron <jic23@kernel.org>]

> commit 6093db1ff45e9e897720bc6b278550ffbef67e85
> Author: Kurt Borja <kuurtb@gmail.com>
> 
> iio: adc: add the ti-ads1262 driver
> 
> Add the ti-ads1262 driver with initial support for the primary ADC (ADC1).
> Various features such as accurate timeout delays, per-channel reference
> sources, scale, offset, and diagnostics are intentionally left out for
> future support.

One particularly interesting point from sashiko about regmap debugfs.

> 
> > diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-ads1262.c
> 

> > +static int ads1262_channel_read(struct iio_dev *indio_dev,
> > +				const struct iio_chan_spec *spec, __be32 *val)
> > +{
> > +	struct ads1262 *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> > +	if (IIO_DEV_ACQUIRE_FAILED(claim))
> > +		return -EBUSY;
> > +
> > +	ret = ads1262_set_runmode(st, ADS1262_RUNMODE_PULSE);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ads1262_channel_enable(st, spec);
> > +	if (ret)
> > +		return ret;
> > +
> > +	reinit_completion(&st->drdy);
> > +
> > +	ret = ads1262_dev_start_one(st);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ads1262_wait_for_conversion(st);
> > +	if (ret)
> > +		return ret;
> 
> [Severity: Medium]
> Does this error path leave the hardware running if the wait is interrupted?
> 
> If ads1262_wait_for_conversion() returns an error (like -ERESTARTSYS from
> a signal), the driver exits immediately without stopping the hardware. If a
> subsequent read calls reinit_completion(&st->drdy), but the abandoned
> hardware conversion finishes and asserts the DRDY interrupt right after,
> could the new read prematurely fetch stale data from the old conversion?
> 
> Should this path ensure the hardware is stopped and synchronized with the
> interrupt handler on timeout or interrupt?

For me this is obscure enough I'm not sure we care about stale data.

> 
> [ ... ]
> 
> > +static int ads1262_dev_configure(struct ads1262 *st)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	int id, ret;
> > +
> > +	ret = ads1262_dev_reset(st);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to reset device\n");
> > +
> > +	guard(mutex)(&st->xfer_lock);
> > +
> > +	ret = regmap_read(st->regmap, ADS1262_ID_REG, &id);
> 
> [Severity: Low]
> Is the type of id correct here?
> 
> The regmap_read() API expects an unsigned int * for its third argument, but
> id is declared as an int. Could this trigger a -Wpointer-sign compiler
> warning?

This one is minor but good to tidy up.

> 
> [ ... ]
> 
> > +static const struct regmap_config ads1262_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.writeable_reg = ads1262_writeable_reg,
> > +	.readable_reg = ads1262_readable_reg,
> > +	.volatile_reg = ads1262_volatile_reg,
> > +	.reg_defaults = ads1262_reg_defaults,
> > +	.num_reg_defaults = ARRAY_SIZE(ads1262_reg_defaults),
> > +	.max_register = ADS1262_ADC2FSC1_REG,
> > +	.can_sleep = true,
> > +	.cache_type = REGCACHE_MAPLE,
> > +};
> 
> [ ... ]
> 
> > +static int ads1262_regmap_read(void *context, const void *reg_buf,
> > +			       size_t reg_size, void *val_buf, size_t val_size)
> > +{
> > +	struct ads1262 *st = context;
> > +	u8 tx[2];
> > +
> > +	lockdep_assert_held(&st->xfer_lock);
> 
> [Severity: Medium]
> Will this lockdep assertion fail if the register is read via debugfs?
> 
> Because ads1262_regmap_config does not set disable_locking = true and does
> not provide custom lock/unlock callbacks, regmap uses its own internal
> mutex. When regmap's debugfs interface is read, the regmap core acquires
> its internal mutex and calls the driver's ads1262_regmap_read() callback
> without holding st->xfer_lock.
> 
> Could this bypass the xfer_lock, triggering the lockdep splat and allowing
> debugfs SPI operations to interleave with driver-initiated SPI transactions?

This is a corner I'd not thought about in adding lockdep markings to these
functions.  Up to you how you want to fix it.