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

[Jonathan Cameron <jic23@kernel.org>]

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

Technically claiming mode would be enough and if the locking is only coupled
to "in buffered mode or not in buffered mode" I'm fine with that.
However, if you need to serialize for other reasons we probably need to
grow a more complex solution with a lock and a flag (similar to what the
IIO mode stuff is doing) so as to avoid holding a lock across syscalls
as this points out.

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

Good catch here.

You shouldn't be modifying the global. The mutex solution works because
devm_regmap_init() doesn't keep any references to the config anyway but
I'd just use a local copy instead.  One on the stack should be fine.


Jonathan