* Re: [PATCH] Staging: iio: meter: Replace mlock with driver private lock
2017-10-16 12:28 ` Fwd: [PATCH] Staging: iio: meter: Replace mlock with driver private lock Georgiana Chelu
@ 2017-10-21 16:48 ` Jonathan Cameron
0 siblings, 0 replies; 2+ messages in thread
From: Jonathan Cameron @ 2017-10-21 16:48 UTC (permalink / raw)
To: Georgiana Chelu
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, linux-iio,
Daniel Baluta, Alison Schofield
On Mon, 16 Oct 2017 15:28:57 +0300
Georgiana Chelu <georgiana.chelu93@gmail.com> wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with a lock in the devices global data.
>
> Signed-off-by: Georgiana Chelu <georgiana.chelu93@gmail.com>
Hmm. The naming as rw_lock made me wonder what was actually
being protected in this driver as there is no need to explicitly
protect read and write operations. The spi bus is has all
the protections needed to ensure nothing clashes.
Upshot is the bit that needs protecting is that we have
a read modify write cycle going on in write_samp_frequency
Now, each element of this is protected by the buffer lock
but it is dropped between them. A nicer approach
than you have hear would be to add some unlocked
read and write helpers thus allowing you to take
buflock around this whole modify and avoid any necessity for
an additional lock.
This is similar to what has been done in other drivers
when unwinding the missuse of mlock.
I have no idea why we would ever need to take the lock
in the read cases. We might if we were aiming to have
some sort of caching that needed to be in sync with the
current state - but there is none of that as far as
I can see. So in those paths I think we would be
fine not to bother taking any additional lock at all.
Jonathan
> ---
> drivers/staging/iio/meter/ade7758.h | 2 ++
> drivers/staging/iio/meter/ade7758_core.c | 11 +++++++----
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/meter/ade7758.h
> b/drivers/staging/iio/meter/ade7758.h
> index 6ae78d8aa24f..728424a6648b 100644
> --- a/drivers/staging/iio/meter/ade7758.h
> +++ b/drivers/staging/iio/meter/ade7758.h
> @@ -112,6 +112,7 @@
> * @tx: transmit buffer
> * @rx: receive buffer
> * @buf_lock: mutex to protect tx and rx
> + * @lock: mutex to protect raw read and write
> **/
> struct ade7758_state {
> struct spi_device *us;
> @@ -119,6 +120,7 @@ struct ade7758_state {
> u8 *tx;
> u8 *rx;
> struct mutex buf_lock;
> + struct mutex rw_lock; /* protect raw read/write */
> struct spi_transfer ring_xfer[4];
> struct spi_message ring_msg;
> /*
> diff --git a/drivers/staging/iio/meter/ade7758_core.c
> b/drivers/staging/iio/meter/ade7758_core.c
> index 7b7ffe5ed186..6b153dd6d40d 100644
> --- a/drivers/staging/iio/meter/ade7758_core.c
> +++ b/drivers/staging/iio/meter/ade7758_core.c
> @@ -523,12 +523,13 @@ static int ade7758_read_raw(struct iio_dev *indio_dev,
> long mask)
> {
> int ret;
> + struct ade7758_state *st = iio_priv(indio_dev);
>
> switch (mask) {
> case IIO_CHAN_INFO_SAMP_FREQ:
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->rw_lock);
> ret = ade7758_read_samp_freq(&indio_dev->dev, val);
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->rw_lock);
> return ret;
> default:
> return -EINVAL;
> @@ -542,14 +543,15 @@ static int ade7758_write_raw(struct iio_dev *indio_dev,
> int val, int val2, long mask)
> {
> int ret;
> + struct ade7758_state *st = iio_priv(indio_dev);
>
> switch (mask) {
> case IIO_CHAN_INFO_SAMP_FREQ:
> if (val2)
> return -EINVAL;
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->rw_lock);
> ret = ade7758_write_samp_freq(&indio_dev->dev, val);
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->rw_lock);
> return ret;
> default:
> return -EINVAL;
> @@ -854,6 +856,7 @@ static int ade7758_probe(struct spi_device *spi)
> }
> st->us = spi;
> mutex_init(&st->buf_lock);
> + mutex_init(&st->rw_lock);
>
> indio_dev->name = spi->dev.driver->name;
> indio_dev->dev.parent = &spi->dev;
> --
> 2.11.0
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 2+ messages in thread