* [PATCH] iio:dac:ad5064 mlock cleanup - move to a local lock.
@ 2019-02-20 18:23 jic23
2019-02-22 11:58 ` Alexandru Ardelean
0 siblings, 1 reply; 5+ messages in thread
From: jic23 @ 2019-02-20 18:23 UTC (permalink / raw)
To: linux-iio; +Cc: Jonathan Cameron
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);
dev_set_drvdata(dev, indio_dev);
st->chip_info = &ad5064_chip_info_tbl[type];
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] iio:dac:ad5064 mlock cleanup - move to a local lock.
2019-02-20 18:23 [PATCH] iio:dac:ad5064 mlock cleanup - move to a local lock jic23
@ 2019-02-22 11:58 ` Alexandru Ardelean
2019-03-02 17:52 ` Jonathan Cameron
0 siblings, 1 reply; 5+ messages in thread
From: Alexandru Ardelean @ 2019-02-22 11:58 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, Jonathan Cameron, alexandru.ardelean
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()
> dev_set_drvdata(dev, indio_dev);
>
> st->chip_info = &ad5064_chip_info_tbl[type];
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] iio:dac:ad5064 mlock cleanup - move to a local lock.
2019-02-22 11:58 ` Alexandru Ardelean
@ 2019-03-02 17:52 ` Jonathan Cameron
2019-03-03 14:26 ` Alexandru Ardelean
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2019-03-02 17:52 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio, Jonathan Cameron, alexandru.ardelean
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).
> > dev_set_drvdata(dev, indio_dev);
> >
> > st->chip_info = &ad5064_chip_info_tbl[type];
> > --
> > 2.20.1
> >
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] iio:dac:ad5064 mlock cleanup - move to a local lock.
2019-03-02 17:52 ` Jonathan Cameron
@ 2019-03-03 14:26 ` Alexandru Ardelean
2019-03-16 14:53 ` Jonathan Cameron
0 siblings, 1 reply; 5+ messages in thread
From: Alexandru Ardelean @ 2019-03-03 14:26 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, Jonathan Cameron, alexandru.ardelean
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.
>
> > > dev_set_drvdata(dev, indio_dev);
> > >
> > > st->chip_info = &ad5064_chip_info_tbl[type];
> > > --
> > > 2.20.1
> > >
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] iio:dac:ad5064 mlock cleanup - move to a local lock.
2019-03-03 14:26 ` Alexandru Ardelean
@ 2019-03-16 14:53 ` Jonathan Cameron
0 siblings, 0 replies; 5+ messages in thread
From: Jonathan Cameron @ 2019-03-16 14:53 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio, Jonathan Cameron, alexandru.ardelean
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
> > > >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-16 14:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-20 18:23 [PATCH] iio:dac:ad5064 mlock cleanup - move to a local lock jic23
2019-02-22 11:58 ` Alexandru Ardelean
2019-03-02 17:52 ` Jonathan Cameron
2019-03-03 14:26 ` Alexandru Ardelean
2019-03-16 14:53 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox