Re: [PATCH v5 03/12] iio: dac: ad5686: acquire lock when doing powerdown control

[Rodrigo Alencar]

On 26/05/05 12:05PM, Jonathan Cameron wrote:
> On Fri, 01 May 2026 10:14:56 +0100
> Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@xxxxxxxxxx> wrote:
> 
> > From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> > 
> > Protect write access of pwr_down_mode and pwr_down_mask fields with
> > existing mutex lock. Each channel exposes their own attributes for
> > controlling powerdown modes and powerdown state. This fixes potential
> > race conditions as those functions perform non-atomic read-modify-write
> > operations to those pwr_down_* fields. This issue exists since the
> > ad5686 driver was first introduced.
> > 
> > Fixes: c2f37c8dcadc ("iio: dac: New driver for AD5686R, AD5685R, AD5684R Digital to analog converters")
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> https://sashiko.dev/#/patchset/20260501-ad5686-fixes-v5-0-0b2f45488418%40analog.com
> 
> Reasonable concern about possible races in the read path leading to
> -EPERM rather than simply the wrong value.
> 
> The other bit about not updating hardware is true but I think it's fine not to change
> what powerdown mode we are in whilst powered down.

True! I've seen sashiko review for this, I have a v6 ready with its feeback.
Just waiting for some more reviews (like yours). I might be sending to many new versions
for this one in a short amount of time!

  
> > ---
> >  drivers/iio/dac/ad5686.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> > index 076fe8b8bd85..69358dd66cbc 100644
> > --- a/drivers/iio/dac/ad5686.c
> > +++ b/drivers/iio/dac/ad5686.c
> > @@ -39,6 +39,8 @@ static int ad5686_set_powerdown_mode(struct iio_dev *indio_dev,
> >  {
> >  	struct ad5686_state *st = iio_priv(indio_dev);
> >  
> > +	guard(mutex)(&st->lock);
> > +
> >  	st->pwr_down_mode &= ~(0x3 << (chan->channel * 2));
> >  	st->pwr_down_mode |= ((mode + 1) << (chan->channel * 2));
> >  
> > @@ -77,6 +79,8 @@ static ssize_t ad5686_write_dac_powerdown(struct iio_dev *indio_dev,
> >  	if (ret)
> >  		return ret;
> >  
> > +	guard(mutex)(&st->lock);
> > +
> >  	if (readin)
> >  		st->pwr_down_mask |= (0x3 << (chan->channel * 2));
> >  	else
> > 
> 

-- 
Kind regards,

Rodrigo Alencar