Re: [PATCH] iio:dac:ad5064 mlock cleanup - move to a local lock.

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 3 Mar 2019 16:26:45 +0200
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sat, Mar 2, 2019 at 7:52 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 22 Feb 2019 13:58:13 +0200
> > Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
> >  
> > > On Wed, Feb 20, 2019 at 8:24 PM <jic23@kernel.org> wrote:  
> > > >  
> > >
> > > Hey,
> > >
> > > One comment inline.
> > >  
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > >
> > > > indio_dev->mlock is intended to protect state transitions in
> > > > the core. It's scope is tightly defined. For device specific
> > > > uses such as those made here, we should define a local lock
> > > > allowing the scope of the lock to be defined near to what it
> > > > is protecting.
> > > >
> > > > These mlock changes can be non obvious, but given we don't do
> > > > anything other than direct for DACs, these ones are easy to do.
> > > >
> > > > If anyone wants to help with this particular effort it would
> > > > be most welcome!
> > > >
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > ---
> > > >  drivers/iio/dac/ad5064.c | 15 +++++++++------
> > > >  1 file changed, 9 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/dac/ad5064.c b/drivers/iio/dac/ad5064.c
> > > > index 2f98cb2a3b96..6c3ba143839b 100644
> > > > --- a/drivers/iio/dac/ad5064.c
> > > > +++ b/drivers/iio/dac/ad5064.c
> > > > @@ -112,6 +112,8 @@ struct ad5064_state {
> > > >         bool                            use_internal_vref;
> > > >
> > > >         ad5064_write_func               write;
> > > > +       /* Lock used to maintain consistency between cached and dev state */
> > > > +       struct mutex lock;
> > > >
> > > >         /*
> > > >          * DMA (thus cache coherency maintenance) requires the
> > > > @@ -248,11 +250,11 @@ static int ad5064_set_powerdown_mode(struct iio_dev *indio_dev,
> > > >         struct ad5064_state *st = iio_priv(indio_dev);
> > > >         int ret;
> > > >
> > > > -       mutex_lock(&indio_dev->mlock);
> > > > +       mutex_lock(&st->lock);
> > > >         st->pwr_down_mode[chan->channel] = mode + 1;
> > > >
> > > >         ret = ad5064_sync_powerdown_mode(st, chan);
> > > > -       mutex_unlock(&indio_dev->mlock);
> > > > +       mutex_unlock(&st->lock);
> > > >
> > > >         return ret;
> > > >  }
> > > > @@ -291,11 +293,11 @@ static ssize_t ad5064_write_dac_powerdown(struct iio_dev *indio_dev,
> > > >         if (ret)
> > > >                 return ret;
> > > >
> > > > -       mutex_lock(&indio_dev->mlock);
> > > > +       mutex_lock(&st->lock);
> > > >         st->pwr_down[chan->channel] = pwr_down;
> > > >
> > > >         ret = ad5064_sync_powerdown_mode(st, chan);
> > > > -       mutex_unlock(&indio_dev->mlock);
> > > > +       mutex_unlock(&st->lock);
> > > >         return ret ? ret : len;
> > > >  }
> > > >
> > > > @@ -349,12 +351,12 @@ static int ad5064_write_raw(struct iio_dev *indio_dev,
> > > >                 if (val >= (1 << chan->scan_type.realbits) || val < 0)
> > > >                         return -EINVAL;
> > > >
> > > > -               mutex_lock(&indio_dev->mlock);
> > > > +               mutex_lock(&st->lock);
> > > >                 ret = ad5064_write(st, AD5064_CMD_WRITE_INPUT_N_UPDATE_N,
> > > >                                 chan->address, val, chan->scan_type.shift);
> > > >                 if (ret == 0)
> > > >                         st->dac_cache[chan->channel] = val;
> > > > -               mutex_unlock(&indio_dev->mlock);
> > > > +               mutex_unlock(&st->lock);
> > > >                 break;
> > > >         default:
> > > >                 ret = -EINVAL;
> > > > @@ -856,6 +858,7 @@ static int ad5064_probe(struct device *dev, enum ad5064_type type,
> > > >                 return  -ENOMEM;
> > > >
> > > >         st = iio_priv(indio_dev);
> > > > +       mutex_init(&st->lock);  
> > >
> > > An equivalent `mutex_destroy()` call would be a good idea in ad5064_remove()
> > >  
> > Ah, this one..
> >
> > mutex_destroy is only useful in finding use after free if mutex debugging is
> > turned on.  In my view it is of dubious benefit if it's getting called only
> > in remove, on the basis we should be pretty sure the driver won't take
> > the lock if we hit that path.
> >
> > The downside is that it complicates the remove path because there is no devm
> > version of mutex_init to automate the cleanup.
> >
> > I'll look at whether it makes sense here (i.e. if there is too much
> > cost to adding it).  
> 
> Hmm, ok.
> From my side it's fine whether to leave it or add it.
Applied before I forget about it.

Thanks,

Jonathan

> 
> >  
> > > >         dev_set_drvdata(dev, indio_dev);
> > > >
> > > >         st->chip_info = &ad5064_chip_info_tbl[type];
> > > > --
> > > > 2.20.1
> > > >  
> >