diff mbox series

[v2,16/16] iio: core: move 'mlock' to 'struct iio_dev_opaque'

Message ID 20221004134909.1692021-17-nuno.sa@analog.com (mailing list archive)
State Superseded
Headers show
Series Make 'mlock' really private | expand

Commit Message

Nuno Sa Oct. 4, 2022, 1:49 p.m. UTC 
Now that there are no more users accessing 'mlock' directly, we can move
it to the iio_dev private structure. Hence, it's now explicit that new
driver's should not directly this lock.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/TODO                   |  3 ---
 drivers/iio/industrialio-buffer.c  | 29 +++++++++++++++++------------
 drivers/iio/industrialio-core.c    | 26 +++++++++++++++-----------
 drivers/iio/industrialio-event.c   |  4 ++--
 drivers/iio/industrialio-trigger.c | 12 ++++++------
 include/linux/iio/iio-opaque.h     |  2 ++
 include/linux/iio/iio.h            |  3 ---
 7 files changed, 42 insertions(+), 37 deletions(-)

Comments

Andy Shevchenko Oct. 4, 2022, 2:21 p.m. UTC | #1 
On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> Now that there are no more users accessing 'mlock' directly, we can move
> it to the iio_dev private structure. Hence, it's now explicit that new
> driver's should not directly this lock.

use this


I like the end result!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

P.S. Shouldn't we annotate the respective APIs with might_sleep() and
Co (if it's not done yet)?

PPS Reading to the topic:
https://blog.ffwll.ch/2022/07/locking-engineering.html
https://blog.ffwll.ch/2022/08/locking-hierarchy.html
https://blog.ffwll.ch/2020/08/lockdep-false-positives.html


> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/iio/TODO                   |  3 ---
>  drivers/iio/industrialio-buffer.c  | 29 +++++++++++++++++------------
>  drivers/iio/industrialio-core.c    | 26 +++++++++++++++-----------
>  drivers/iio/industrialio-event.c   |  4 ++--
>  drivers/iio/industrialio-trigger.c | 12 ++++++------
>  include/linux/iio/iio-opaque.h     |  2 ++
>  include/linux/iio/iio.h            |  3 ---
>  7 files changed, 42 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/iio/TODO b/drivers/iio/TODO
> index 7d7326b7085a..2ace27d1ac62 100644
> --- a/drivers/iio/TODO
> +++ b/drivers/iio/TODO
> @@ -7,9 +7,6 @@ tree
>    - ABI Documentation
>    - Audit driviers/iio/staging/Documentation
>
> -- Replace iio_dev->mlock by either a local lock or use
> -iio_claim_direct.(Requires analysis of the purpose of the lock.)
> -
>  - Converting drivers from device tree centric to more generic
>  property handlers.
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 228598b82a2f..9cd7db549fcb 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -507,13 +507,14 @@ static ssize_t iio_scan_el_store(struct device *dev,
>         int ret;
>         bool state;
>         struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>         struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>         struct iio_buffer *buffer = this_attr->buffer;
>
>         ret = kstrtobool(buf, &state);
>         if (ret < 0)
>                 return ret;
> -       mutex_lock(&indio_dev->mlock);
> +       mutex_lock(&iio_dev_opaque->mlock);
>         if (iio_buffer_is_active(buffer)) {
>                 ret = -EBUSY;
>                 goto error_ret;
> @@ -532,7 +533,7 @@ static ssize_t iio_scan_el_store(struct device *dev,
>         }
>
>  error_ret:
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&iio_dev_opaque->mlock);
>
>         return ret < 0 ? ret : len;
>
> @@ -554,6 +555,7 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
>  {
>         int ret;
>         struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>         struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
>         bool state;
>
> @@ -561,14 +563,14 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
>         if (ret < 0)
>                 return ret;
>
> -       mutex_lock(&indio_dev->mlock);
> +       mutex_lock(&iio_dev_opaque->mlock);
>         if (iio_buffer_is_active(buffer)) {
>                 ret = -EBUSY;
>                 goto error_ret;
>         }
>         buffer->scan_timestamp = state;
>  error_ret:
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&iio_dev_opaque->mlock);
>
>         return ret ? ret : len;
>  }
> @@ -642,6 +644,7 @@ static ssize_t length_store(struct device *dev, struct device_attribute *attr,
>                             const char *buf, size_t len)
>  {
>         struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>         struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
>         unsigned int val;
>         int ret;
> @@ -653,7 +656,7 @@ static ssize_t length_store(struct device *dev, struct device_attribute *attr,
>         if (val == buffer->length)
>                 return len;
>
> -       mutex_lock(&indio_dev->mlock);
> +       mutex_lock(&iio_dev_opaque->mlock);
>         if (iio_buffer_is_active(buffer)) {
>                 ret = -EBUSY;
>         } else {
> @@ -665,7 +668,7 @@ static ssize_t length_store(struct device *dev, struct device_attribute *attr,
>         if (buffer->length && buffer->length < buffer->watermark)
>                 buffer->watermark = buffer->length;
>  out:
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&iio_dev_opaque->mlock);
>
>         return ret ? ret : len;
>  }
> @@ -1256,7 +1259,7 @@ int iio_update_buffers(struct iio_dev *indio_dev,
>                 return -EINVAL;
>
>         mutex_lock(&iio_dev_opaque->info_exist_lock);
> -       mutex_lock(&indio_dev->mlock);
> +       mutex_lock(&iio_dev_opaque->mlock);
>
>         if (insert_buffer && iio_buffer_is_active(insert_buffer))
>                 insert_buffer = NULL;
> @@ -1277,7 +1280,7 @@ int iio_update_buffers(struct iio_dev *indio_dev,
>         ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
>
>  out_unlock:
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&iio_dev_opaque->mlock);
>         mutex_unlock(&iio_dev_opaque->info_exist_lock);
>
>         return ret;
> @@ -1296,6 +1299,7 @@ static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
>         int ret;
>         bool requested_state;
>         struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>         struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
>         bool inlist;
>
> @@ -1303,7 +1307,7 @@ static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
>         if (ret < 0)
>                 return ret;
>
> -       mutex_lock(&indio_dev->mlock);
> +       mutex_lock(&iio_dev_opaque->mlock);
>
>         /* Find out if it is in the list */
>         inlist = iio_buffer_is_active(buffer);
> @@ -1317,7 +1321,7 @@ static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
>                 ret = __iio_update_buffers(indio_dev, NULL, buffer);
>
>  done:
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&iio_dev_opaque->mlock);
>         return (ret < 0) ? ret : len;
>  }
>
> @@ -1334,6 +1338,7 @@ static ssize_t watermark_store(struct device *dev,
>                                const char *buf, size_t len)
>  {
>         struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>         struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
>         unsigned int val;
>         int ret;
> @@ -1344,7 +1349,7 @@ static ssize_t watermark_store(struct device *dev,
>         if (!val)
>                 return -EINVAL;
>
> -       mutex_lock(&indio_dev->mlock);
> +       mutex_lock(&iio_dev_opaque->mlock);
>
>         if (val > buffer->length) {
>                 ret = -EINVAL;
> @@ -1358,7 +1363,7 @@ static ssize_t watermark_store(struct device *dev,
>
>         buffer->watermark = val;
>  out:
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&iio_dev_opaque->mlock);
>
>         return ret ? ret : len;
>  }
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index c23d3abb33c5..ebbc64e4f673 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -285,16 +285,16 @@ int iio_device_set_clock(struct iio_dev *indio_dev, clockid_t clock_id)
>         struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>         const struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
>
> -       ret = mutex_lock_interruptible(&indio_dev->mlock);
> +       ret = mutex_lock_interruptible(&iio_dev_opaque->mlock);
>         if (ret)
>                 return ret;
>         if ((ev_int && iio_event_enabled(ev_int)) ||
>             iio_buffer_enabled(indio_dev)) {
> -               mutex_unlock(&indio_dev->mlock);
> +               mutex_unlock(&iio_dev_opaque->mlock);
>                 return -EBUSY;
>         }
>         iio_dev_opaque->clock_id = clock_id;
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&iio_dev_opaque->mlock);
>
>         return 0;
>  }
> @@ -1674,7 +1674,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
>         indio_dev->dev.type = &iio_device_type;
>         indio_dev->dev.bus = &iio_bus_type;
>         device_initialize(&indio_dev->dev);
> -       mutex_init(&indio_dev->mlock);
> +       mutex_init(&iio_dev_opaque->mlock);
>         mutex_init(&iio_dev_opaque->info_exist_lock);
>         INIT_LIST_HEAD(&iio_dev_opaque->channel_attr_list);
>
> @@ -1696,7 +1696,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
>         INIT_LIST_HEAD(&iio_dev_opaque->ioctl_handlers);
>
>         lockdep_register_key(&iio_dev_opaque->mlock_key);
> -       lockdep_set_class(&indio_dev->mlock, &iio_dev_opaque->mlock_key);
> +       lockdep_set_class(&iio_dev_opaque->mlock, &iio_dev_opaque->mlock_key);
>
>         return indio_dev;
>  }
> @@ -2058,10 +2058,12 @@ EXPORT_SYMBOL_GPL(__devm_iio_device_register);
>   */
>  int iio_device_claim_direct_mode(struct iio_dev *indio_dev)
>  {
> -       mutex_lock(&indio_dev->mlock);
> +       struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +
> +       mutex_lock(&iio_dev_opaque->mlock);
>
>         if (iio_buffer_enabled(indio_dev)) {
> -               mutex_unlock(&indio_dev->mlock);
> +               mutex_unlock(&iio_dev_opaque->mlock);
>                 return -EBUSY;
>         }
>         return 0;
> @@ -2079,7 +2081,7 @@ EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
>   */
>  void iio_device_release_direct_mode(struct iio_dev *indio_dev)
>  {
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
>  }
>  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
>
> @@ -2096,12 +2098,14 @@ EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
>   */
>  int iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
>  {
> -       mutex_lock(&indio_dev->mlock);
> +       struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +
> +       mutex_lock(&iio_dev_opaque->mlock);
>
>         if (iio_buffer_enabled(indio_dev))
>                 return 0;
>
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&iio_dev_opaque->mlock);
>         return -EBUSY;
>  }
>  EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
> @@ -2117,7 +2121,7 @@ EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
>   */
>  void iio_device_release_buffer_mode(struct iio_dev *indio_dev)
>  {
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
>  }
>  EXPORT_SYMBOL_GPL(iio_device_release_buffer_mode);
>
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index 3d78da2531a9..1a26393a7c0c 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -198,7 +198,7 @@ static int iio_event_getfd(struct iio_dev *indio_dev)
>         if (ev_int == NULL)
>                 return -ENODEV;
>
> -       fd = mutex_lock_interruptible(&indio_dev->mlock);
> +       fd = mutex_lock_interruptible(&iio_dev_opaque->mlock);
>         if (fd)
>                 return fd;
>
> @@ -219,7 +219,7 @@ static int iio_event_getfd(struct iio_dev *indio_dev)
>         }
>
>  unlock:
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&iio_dev_opaque->mlock);
>         return fd;
>  }
>
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 6885a186fe27..a2f3cc2f65ef 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -120,12 +120,12 @@ int iio_trigger_set_immutable(struct iio_dev *indio_dev, struct iio_trigger *tri
>                 return -EINVAL;
>
>         iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> -       mutex_lock(&indio_dev->mlock);
> +       mutex_lock(&iio_dev_opaque->mlock);
>         WARN_ON(iio_dev_opaque->trig_readonly);
>
>         indio_dev->trig = iio_trigger_get(trig);
>         iio_dev_opaque->trig_readonly = true;
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&iio_dev_opaque->mlock);
>
>         return 0;
>  }
> @@ -438,16 +438,16 @@ static ssize_t current_trigger_store(struct device *dev,
>         struct iio_trigger *trig;
>         int ret;
>
> -       mutex_lock(&indio_dev->mlock);
> +       mutex_lock(&iio_dev_opaque->mlock);
>         if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) {
> -               mutex_unlock(&indio_dev->mlock);
> +               mutex_unlock(&iio_dev_opaque->mlock);
>                 return -EBUSY;
>         }
>         if (iio_dev_opaque->trig_readonly) {
> -               mutex_unlock(&indio_dev->mlock);
> +               mutex_unlock(&iio_dev_opaque->mlock);
>                 return -EPERM;
>         }
> -       mutex_unlock(&indio_dev->mlock);
> +       mutex_unlock(&iio_dev_opaque->mlock);
>
>         trig = iio_trigger_acquire_by_name(buf);
>         if (oldtrig == trig) {
> diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> index d1f8b30a7c8b..5aec3945555b 100644
> --- a/include/linux/iio/iio-opaque.h
> +++ b/include/linux/iio/iio-opaque.h
> @@ -11,6 +11,7 @@
>   *                             checked by device drivers but should be considered
>   *                             read-only as this is a core internal bit
>   * @driver_module:             used to make it harder to undercut users
> + * @mlock:                     lock used to prevent simultaneous device state changes
>   * @mlock_key:                 lockdep class for iio_dev lock
>   * @info_exist_lock:           lock to prevent use during removal
>   * @trig_readonly:             mark the current trigger immutable
> @@ -43,6 +44,7 @@ struct iio_dev_opaque {
>         int                             currentmode;
>         int                             id;
>         struct module                   *driver_module;
> +       struct mutex                    mlock;
>         struct lock_class_key           mlock_key;
>         struct mutex                    info_exist_lock;
>         bool                            trig_readonly;
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 9d3bd6379eb8..8e0afaaa3f75 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -548,8 +548,6 @@ struct iio_buffer_setup_ops {
>   *                     and owner
>   * @buffer:            [DRIVER] any buffer present
>   * @scan_bytes:                [INTERN] num bytes captured to be fed to buffer demux
> - * @mlock:             [INTERN] lock used to prevent simultaneous device state
> - *                     changes
>   * @available_scan_masks: [DRIVER] optional array of allowed bitmasks
>   * @masklength:                [INTERN] the length of the mask established from
>   *                     channels
> @@ -574,7 +572,6 @@ struct iio_dev {
>
>         struct iio_buffer               *buffer;
>         int                             scan_bytes;
> -       struct mutex                    mlock;
>
>         const unsigned long             *available_scan_masks;
>         unsigned                        masklength;
> --
> 2.37.3
>
Nuno Sá Oct. 5, 2022, 8:40 a.m. UTC | #2 
On Tue, 2022-10-04 at 17:21 +0300, Andy Shevchenko wrote:
> On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
> > 
> > Now that there are no more users accessing 'mlock' directly, we can
> > move
> > it to the iio_dev private structure. Hence, it's now explicit that
> > new
> > driver's should not directly this lock.
> 
> use this
> 
> 
> I like the end result!
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> P.S. Shouldn't we annotate the respective APIs with might_sleep() and
> Co (if it's not done yet)?
> 
> 

Hmm, I would say this is the same story as with sparse annotations... I
guess, at least, might_sleep() would make sense but I think we should
probably do it for the complete IIO subsystem where it makes sense
instead of having it in just this new API.

- Nuno Sá
Jonathan Cameron Oct. 9, 2022, 11:48 a.m. UTC | #3 
On Wed, 05 Oct 2022 10:40:03 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2022-10-04 at 17:21 +0300, Andy Shevchenko wrote:
> > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote:  
> > > 
> > > Now that there are no more users accessing 'mlock' directly, we can
> > > move
> > > it to the iio_dev private structure. Hence, it's now explicit that
> > > new
> > > driver's should not directly this lock.  
> > 
> > use this
> > 
> > 
> > I like the end result!
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > 
> > P.S. Shouldn't we annotate the respective APIs with might_sleep() and
> > Co (if it's not done yet)?
> > 
> >   
> 
> Hmm, I would say this is the same story as with sparse annotations... I
> guess, at least, might_sleep() would make sense but I think we should
> probably do it for the complete IIO subsystem where it makes sense
> instead of having it in just this new API.

We definitely could add such annotations.

From a documentation point of view that might be useful. 
From a protection / bug catching point of view the calls to mutex_lock()
should fire off much the same error reports, just one level down.

Jonathan

> 
> - Nuno Sá 
>
Jonathan Cameron Oct. 9, 2022, 11:52 a.m. UTC | #4 
On Tue, 4 Oct 2022 17:21:20 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
> >
> > Now that there are no more users accessing 'mlock' directly, we can move
> > it to the iio_dev private structure. Hence, it's now explicit that new
> > driver's should not directly this lock.  
> 
> use this
> 
> 
> I like the end result!
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Hi Andy,

Just to check, just this patch, or series (where you haven't commented?)

I'm going to queue this series up piecewise just because it's fairly
big and most of it is completely non controversial!  So for now I've
not applied your RB, but can do so before I push out as anything non
rebasing (which I won't do until after the merge window).

Thanks,

Jonathan
Jonathan Cameron Oct. 9, 2022, 12:19 p.m. UTC | #5 
On Tue, 4 Oct 2022 15:49:09 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> Now that there are no more users accessing 'mlock' directly, we can move
> it to the iio_dev private structure. Hence, it's now explicit that new
> driver's should not directly this lock.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
This looks good to me. 
So series wise, I think I've picked up 1-12. 
To resolve is remaining discussion plus the missmatch in function name vs
comment in 13.

Thanks for getting this tidied up!  It's the end of a very long process.
I almost feel like we need a party - there would be a quite a crowd given
how many people have been involved in this effort over the years. :)

Jonathan
Nuno Sá Oct. 10, 2022, 7:11 a.m. UTC | #6 
On Sun, 2022-10-09 at 13:19 +0100, Jonathan Cameron wrote:
> On Tue, 4 Oct 2022 15:49:09 +0200
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > Now that there are no more users accessing 'mlock' directly, we can
> > move
> > it to the iio_dev private structure. Hence, it's now explicit that
> > new
> > driver's should not directly this lock.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> This looks good to me. 
> So series wise, I think I've picked up 1-12. 
> To resolve is remaining discussion plus the missmatch in function
> name vs
> comment in 13.
> 

So, I think we are missing an improvement on the commit message for
patch 14 (about shadowing the error code) and fixing the function
name..
> Thanks for getting this tidied up!  It's the end of a very long
> process.
> I almost feel like we need a party - there would be a quite a crowd
> given
> how many people have been involved in this effort over the years. :)
> 

Sure, happy to help! This one felt like something that we really need
to take care :)

- Nuno Sá
Andy Shevchenko Oct. 10, 2022, 8:30 a.m. UTC | #7 
On Sun, Oct 9, 2022 at 2:52 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Tue, 4 Oct 2022 17:21:20 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > I like the end result!
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> Just to check, just this patch, or series (where you haven't commented?)

Just for this patch.

> I'm going to queue this series up piecewise just because it's fairly
> big and most of it is completely non controversial!  So for now I've
> not applied your RB, but can do so before I push out as anything non
> rebasing (which I won't do until after the merge window).
diff mbox series

Patch

diff --git a/drivers/iio/TODO b/drivers/iio/TODO
index 7d7326b7085a..2ace27d1ac62 100644
--- a/drivers/iio/TODO
+++ b/drivers/iio/TODO
@@ -7,9 +7,6 @@  tree
   - ABI Documentation
   - Audit driviers/iio/staging/Documentation
 
-- Replace iio_dev->mlock by either a local lock or use
-iio_claim_direct.(Requires analysis of the purpose of the lock.)
-
 - Converting drivers from device tree centric to more generic
 property handlers.
 
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 228598b82a2f..9cd7db549fcb 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -507,13 +507,14 @@  static ssize_t iio_scan_el_store(struct device *dev,
 	int ret;
 	bool state;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 	struct iio_buffer *buffer = this_attr->buffer;
 
 	ret = kstrtobool(buf, &state);
 	if (ret < 0)
 		return ret;
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 		goto error_ret;
@@ -532,7 +533,7 @@  static ssize_t iio_scan_el_store(struct device *dev,
 	}
 
 error_ret:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	return ret < 0 ? ret : len;
 
@@ -554,6 +555,7 @@  static ssize_t iio_scan_el_ts_store(struct device *dev,
 {
 	int ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
 	bool state;
 
@@ -561,14 +563,14 @@  static ssize_t iio_scan_el_ts_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 		goto error_ret;
 	}
 	buffer->scan_timestamp = state;
 error_ret:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	return ret ? ret : len;
 }
@@ -642,6 +644,7 @@  static ssize_t length_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
 	unsigned int val;
 	int ret;
@@ -653,7 +656,7 @@  static ssize_t length_store(struct device *dev, struct device_attribute *attr,
 	if (val == buffer->length)
 		return len;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 	if (iio_buffer_is_active(buffer)) {
 		ret = -EBUSY;
 	} else {
@@ -665,7 +668,7 @@  static ssize_t length_store(struct device *dev, struct device_attribute *attr,
 	if (buffer->length && buffer->length < buffer->watermark)
 		buffer->watermark = buffer->length;
 out:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	return ret ? ret : len;
 }
@@ -1256,7 +1259,7 @@  int iio_update_buffers(struct iio_dev *indio_dev,
 		return -EINVAL;
 
 	mutex_lock(&iio_dev_opaque->info_exist_lock);
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 
 	if (insert_buffer && iio_buffer_is_active(insert_buffer))
 		insert_buffer = NULL;
@@ -1277,7 +1280,7 @@  int iio_update_buffers(struct iio_dev *indio_dev,
 	ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
 
 out_unlock:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 	mutex_unlock(&iio_dev_opaque->info_exist_lock);
 
 	return ret;
@@ -1296,6 +1299,7 @@  static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
 	int ret;
 	bool requested_state;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
 	bool inlist;
 
@@ -1303,7 +1307,7 @@  static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
 	if (ret < 0)
 		return ret;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 
 	/* Find out if it is in the list */
 	inlist = iio_buffer_is_active(buffer);
@@ -1317,7 +1321,7 @@  static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
 		ret = __iio_update_buffers(indio_dev, NULL, buffer);
 
 done:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 	return (ret < 0) ? ret : len;
 }
 
@@ -1334,6 +1338,7 @@  static ssize_t watermark_store(struct device *dev,
 			       const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
 	unsigned int val;
 	int ret;
@@ -1344,7 +1349,7 @@  static ssize_t watermark_store(struct device *dev,
 	if (!val)
 		return -EINVAL;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 
 	if (val > buffer->length) {
 		ret = -EINVAL;
@@ -1358,7 +1363,7 @@  static ssize_t watermark_store(struct device *dev,
 
 	buffer->watermark = val;
 out:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	return ret ? ret : len;
 }
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index c23d3abb33c5..ebbc64e4f673 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -285,16 +285,16 @@  int iio_device_set_clock(struct iio_dev *indio_dev, clockid_t clock_id)
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	const struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
 
-	ret = mutex_lock_interruptible(&indio_dev->mlock);
+	ret = mutex_lock_interruptible(&iio_dev_opaque->mlock);
 	if (ret)
 		return ret;
 	if ((ev_int && iio_event_enabled(ev_int)) ||
 	    iio_buffer_enabled(indio_dev)) {
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&iio_dev_opaque->mlock);
 		return -EBUSY;
 	}
 	iio_dev_opaque->clock_id = clock_id;
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	return 0;
 }
@@ -1674,7 +1674,7 @@  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 	indio_dev->dev.type = &iio_device_type;
 	indio_dev->dev.bus = &iio_bus_type;
 	device_initialize(&indio_dev->dev);
-	mutex_init(&indio_dev->mlock);
+	mutex_init(&iio_dev_opaque->mlock);
 	mutex_init(&iio_dev_opaque->info_exist_lock);
 	INIT_LIST_HEAD(&iio_dev_opaque->channel_attr_list);
 
@@ -1696,7 +1696,7 @@  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 	INIT_LIST_HEAD(&iio_dev_opaque->ioctl_handlers);
 
 	lockdep_register_key(&iio_dev_opaque->mlock_key);
-	lockdep_set_class(&indio_dev->mlock, &iio_dev_opaque->mlock_key);
+	lockdep_set_class(&iio_dev_opaque->mlock, &iio_dev_opaque->mlock_key);
 
 	return indio_dev;
 }
@@ -2058,10 +2058,12 @@  EXPORT_SYMBOL_GPL(__devm_iio_device_register);
  */
 int iio_device_claim_direct_mode(struct iio_dev *indio_dev)
 {
-	mutex_lock(&indio_dev->mlock);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
+	mutex_lock(&iio_dev_opaque->mlock);
 
 	if (iio_buffer_enabled(indio_dev)) {
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&iio_dev_opaque->mlock);
 		return -EBUSY;
 	}
 	return 0;
@@ -2079,7 +2081,7 @@  EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
  */
 void iio_device_release_direct_mode(struct iio_dev *indio_dev)
 {
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
 }
 EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
 
@@ -2096,12 +2098,14 @@  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
  */
 int iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
 {
-	mutex_lock(&indio_dev->mlock);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
+	mutex_lock(&iio_dev_opaque->mlock);
 
 	if (iio_buffer_enabled(indio_dev))
 		return 0;
 
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 	return -EBUSY;
 }
 EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
@@ -2117,7 +2121,7 @@  EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
  */
 void iio_device_release_buffer_mode(struct iio_dev *indio_dev)
 {
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
 }
 EXPORT_SYMBOL_GPL(iio_device_release_buffer_mode);
 
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 3d78da2531a9..1a26393a7c0c 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -198,7 +198,7 @@  static int iio_event_getfd(struct iio_dev *indio_dev)
 	if (ev_int == NULL)
 		return -ENODEV;
 
-	fd = mutex_lock_interruptible(&indio_dev->mlock);
+	fd = mutex_lock_interruptible(&iio_dev_opaque->mlock);
 	if (fd)
 		return fd;
 
@@ -219,7 +219,7 @@  static int iio_event_getfd(struct iio_dev *indio_dev)
 	}
 
 unlock:
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 	return fd;
 }
 
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 6885a186fe27..a2f3cc2f65ef 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -120,12 +120,12 @@  int iio_trigger_set_immutable(struct iio_dev *indio_dev, struct iio_trigger *tri
 		return -EINVAL;
 
 	iio_dev_opaque = to_iio_dev_opaque(indio_dev);
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 	WARN_ON(iio_dev_opaque->trig_readonly);
 
 	indio_dev->trig = iio_trigger_get(trig);
 	iio_dev_opaque->trig_readonly = true;
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	return 0;
 }
@@ -438,16 +438,16 @@  static ssize_t current_trigger_store(struct device *dev,
 	struct iio_trigger *trig;
 	int ret;
 
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&iio_dev_opaque->mlock);
 	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) {
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&iio_dev_opaque->mlock);
 		return -EBUSY;
 	}
 	if (iio_dev_opaque->trig_readonly) {
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&iio_dev_opaque->mlock);
 		return -EPERM;
 	}
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&iio_dev_opaque->mlock);
 
 	trig = iio_trigger_acquire_by_name(buf);
 	if (oldtrig == trig) {
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index d1f8b30a7c8b..5aec3945555b 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -11,6 +11,7 @@ 
  *				checked by device drivers but should be considered
  *				read-only as this is a core internal bit
  * @driver_module:		used to make it harder to undercut users
+ * @mlock:			lock used to prevent simultaneous device state changes
  * @mlock_key:			lockdep class for iio_dev lock
  * @info_exist_lock:		lock to prevent use during removal
  * @trig_readonly:		mark the current trigger immutable
@@ -43,6 +44,7 @@  struct iio_dev_opaque {
 	int				currentmode;
 	int				id;
 	struct module			*driver_module;
+	struct mutex			mlock;
 	struct lock_class_key		mlock_key;
 	struct mutex			info_exist_lock;
 	bool				trig_readonly;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 9d3bd6379eb8..8e0afaaa3f75 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -548,8 +548,6 @@  struct iio_buffer_setup_ops {
  *			and owner
  * @buffer:		[DRIVER] any buffer present
  * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
- * @mlock:		[INTERN] lock used to prevent simultaneous device state
- *			changes
  * @available_scan_masks: [DRIVER] optional array of allowed bitmasks
  * @masklength:		[INTERN] the length of the mask established from
  *			channels
@@ -574,7 +572,6 @@  struct iio_dev {
 
 	struct iio_buffer		*buffer;
 	int				scan_bytes;
-	struct mutex			mlock;
 
 	const unsigned long		*available_scan_masks;
 	unsigned			masklength;