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.
>
> > > dev_set_drvdata(dev, indio_dev);
> > >
> > > st->chip_info = &ad5064_chip_info_tbl[type];
> > > --
> > > 2.20.1
> > >
>
