On Sun, 3 Mar 2019 16:26:45 +0200
Alexandru Ardelean <ardeleanalex@xxxxxxxxx> wrote:
> On Sat, Mar 2, 2019 at 7:52 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Fri, 22 Feb 2019 13:58:13 +0200
> > Alexandru Ardelean <ardeleanalex@xxxxxxxxx> wrote:
> >
> > > On Wed, Feb 20, 2019 at 8:24 PM <jic23@xxxxxxxxxx> wrote:
> > > >
> > >
> > > Hey,
> > >
> > > One comment inline.
> > >
> > > > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > > 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
> > > >
> >
