On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@xxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxx> > --- > 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 > -- With Best Regards, Andy Shevchenko
