[PATCH v3 0/4] Make 'mlock' really private

[PATCH v3 0/4] Make 'mlock' really private

[Nuno Sá]

This patchset cleans all the drivers directly using the iio_device 'mlock'.
This lock is private and should not be used outside the core (or by using
proper helpers).

Most of the conversions where straight, but there are some that really need
extra looking. Mainly patches [13/15] and [14/15] were a bit hacky since
iio_device_claim_direct_mode() does not fit 100%. The reason is that we
want to check if the device is buffering and do something if it is (in
which case the API return -EBUSY and released the lock. I just used a
combinations of locks to get around this (hopefully I did not messed up).

Note that this series was only compiled tested using allyesconfig for
ARM. I ran 'git grep' to make sure there were no more users of 'mlock'.
Hopefully I covered them all...

v2:

[PATCH 1-8, 10-12/16]
 * Mention the inclusion of mutex.h in the commit message.

[PATCH 1-8, 10, 12/16]
 * Initialize mutex as late as possible.
Note that [PATCH 11/16] was not included since the code to do so was not
direct enough. Would need to get a pointer to the private struture
outside of scmi_alloc_iiodev() to do it. While not hard, the added changes
in the code is not really worth it (IMO of course).

[PATCH 1/16]
 * Refactored the commit message a bit. I guess this one will still needs
more discussion...

[PATCH 9/16]
 * New patch to add an helper function to read the samples.

[PATCH 13/16]
 * New patch to introduce iio_device_{claim|release}_buffer_mode() APIs.

[PATCH 14/16]
 * Make use of the new iio_device_{claim|release}_buffer_mode() helpers

[PATCH 15/16]
 * Make use of the new iio_device_{claim|release}_buffer_mode() helpers
in combination with claim_direct_mode(). This is needed so that we make sure
we always get one of the modes (and hence the iio_dev lock) to safely call
max30102_get_temp(). Note that I'm not particular "happy" with the code but
OTOH, it does not look as bad as I thought :). Anyways, if there are no
complains with it, I'm ok to leave it as-is. Otherwise, I think we can think
on the flag approach (briefly discussed in the first series).

v3:

[PATCH 1/4]
 * fix 'make W=1' warning about prototypes mismatch.

[PATCH 2/4]
 * improved commit message to explain why we are shadowing error codes.

[PATCH 4/4]
 * minor English fix on the commit message (as suggested by Andy).

Nuno Sá (4):
  iio: core: introduce iio_device_{claim|release}_buffer_mode() APIs
  iio: health: max30100: do not use internal iio_dev lock
  iio: health: max30102: do not use internal iio_dev lock
  iio: core: move 'mlock' to 'struct iio_dev_opaque'

 drivers/iio/TODO                   |  3 --
 drivers/iio/health/max30100.c      |  9 ++---
 drivers/iio/health/max30102.c      | 19 +++++++---
 drivers/iio/industrialio-buffer.c  | 29 ++++++++-------
 drivers/iio/industrialio-core.c    | 58 +++++++++++++++++++++++++-----
 drivers/iio/industrialio-event.c   |  4 +--
 drivers/iio/industrialio-trigger.c | 12 +++----
 include/linux/iio/iio-opaque.h     |  2 ++
 include/linux/iio/iio.h            |  5 ++-
 9 files changed, 97 insertions(+), 44 deletions(-)

-- 
2.38.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 1/4] iio: core: introduce iio_device_{claim|release}_buffer_mode() APIs

[Nuno Sá]

These APIs are analogous to iio_device_claim_direct_mode() and
iio_device_release_direct_mode() but, as the name suggests, with the
logic flipped. While this looks odd enough, it will have at least two
users (in following changes) and it will be important to move the iio
mlock to the private struct.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 38 +++++++++++++++++++++++++++++++++
 include/linux/iio/iio.h         |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 151ff3993354..cf80f81e4665 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -2083,6 +2083,44 @@ void iio_device_release_direct_mode(struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
 
+/**
+ * iio_device_claim_buffer_mode - Keep device in buffer mode
+ * @indio_dev:	the iio_dev associated with the device
+ *
+ * If the device is in buffer mode it is guaranteed to stay
+ * that way until iio_device_release_buffer_mode() is called.
+ *
+ * Use with iio_device_release_buffer_mode()
+ *
+ * Returns: 0 on success, -EBUSY on failure
+ */
+int iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
+{
+	mutex_lock(&indio_dev->mlock);
+
+	if (iio_buffer_enabled(indio_dev))
+		return 0;
+
+	mutex_unlock(&indio_dev->mlock);
+	return -EBUSY;
+}
+EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
+
+/**
+ * iio_device_release_buffer_mode - releases claim on buffer mode
+ * @indio_dev:	the iio_dev associated with the device
+ *
+ * Release the claim. Device is no longer guaranteed to stay
+ * in buffer mode.
+ *
+ * Use with iio_device_claim_buffer_mode()
+ */
+void iio_device_release_buffer_mode(struct iio_dev *indio_dev)
+{
+	mutex_unlock(&indio_dev->mlock);
+}
+EXPORT_SYMBOL_GPL(iio_device_release_buffer_mode);
+
 /**
  * iio_device_get_current_mode() - helper function providing read-only access to
  *				   the opaque @currentmode variable
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index f0ec8a5e5a7a..9d3bd6379eb8 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -629,6 +629,8 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
 int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
 int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
 void iio_device_release_direct_mode(struct iio_dev *indio_dev);
+int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
+void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
 
 extern struct bus_type iio_bus_type;
 
-- 
2.38.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 2/4] iio: health: max30100: do not use internal iio_dev lock

[Nuno Sá]

The pattern used in this device does not quite fit in the
iio_device_claim_direct_mode() typical usage. In this case,
iio_buffer_enabled() was being used not to prevent the raw access but to
allow it. Hence, let's make use of the new
iio_device_claim_buffer_mode() API to make sure we stay in buffered mode
during the complete read.

Note that we are shadowing the error code returned by
iio_device_claim_buffer_mode() so that we keep the original one
(-EAGAIN). The reason is that some userspace stack might already be
relying on this particular code so that we are not taking chances and
leave it alone.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/health/max30100.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
index 2cca5e0519f8..6ac49901c9da 100644
--- a/drivers/iio/health/max30100.c
+++ b/drivers/iio/health/max30100.c
@@ -387,18 +387,15 @@ static int max30100_read_raw(struct iio_dev *indio_dev,
 		 * Temperature reading can only be acquired while engine
 		 * is running
 		 */
-		mutex_lock(&indio_dev->mlock);
-
-		if (!iio_buffer_enabled(indio_dev))
+		if (iio_device_claim_buffer_mode(indio_dev)) {
 			ret = -EAGAIN;
-		else {
+		} else {
 			ret = max30100_get_temp(data, val);
 			if (!ret)
 				ret = IIO_VAL_INT;
 
+			iio_device_release_buffer_mode(indio_dev);
 		}
-
-		mutex_unlock(&indio_dev->mlock);
 		break;
 	case IIO_CHAN_INFO_SCALE:
 		*val = 1;  /* 0.0625 */
-- 
2.38.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 3/4] iio: health: max30102: do not use internal iio_dev lock

[Nuno Sá]

The pattern used in this device does not quite fit in the
iio_device_claim_direct_mode() typical usage. In this case, we want to
know if we are in buffered mode or not to know if the device is powered
(buffer mode) or not. And depending on that max30102_get_temp() will
power on the device if needed. Hence, in order to keep the same
functionality, we try to:

1. Claim Buffered mode;
2: If 1) succeeds call max30102_get_temp() without powering on the
   device;
3: Release Buffered mode;
4: If 1) fails, Claim Direct mode;
5: If 4) succeeds call max30102_get_temp() with powering on the device;
6: Release Direct mode;
7: If 4) fails, goto to 1) and try again.

This dance between buffered and direct mode is not particularly pretty
(as well as the loop introduced by the goto statement) but it does allow
us to get rid of the mlock usage while keeping the same behavior.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/health/max30102.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
index 437298a29f2d..66df4aaa31a7 100644
--- a/drivers/iio/health/max30102.c
+++ b/drivers/iio/health/max30102.c
@@ -477,12 +477,23 @@ static int max30102_read_raw(struct iio_dev *indio_dev,
 		 * Temperature reading can only be acquired when not in
 		 * shutdown; leave shutdown briefly when buffer not running
 		 */
-		mutex_lock(&indio_dev->mlock);
-		if (!iio_buffer_enabled(indio_dev))
+any_mode_retry:
+		if (iio_device_claim_buffer_mode(indio_dev)) {
+			/*
+			 * This one is a *bit* hacky. If we cannot claim buffer
+			 * mode, then try direct mode so that we make sure
+			 * things cannot concurrently change. And we just keep
+			 * trying until we get one of the modes...
+			 */
+			if (iio_device_claim_direct_mode(indio_dev))
+				goto any_mode_retry;
+
 			ret = max30102_get_temp(data, val, true);
-		else
+			iio_device_release_direct_mode(indio_dev);
+		} else {
 			ret = max30102_get_temp(data, val, false);
-		mutex_unlock(&indio_dev->mlock);
+			iio_device_release_buffer_mode(indio_dev);
+		}
 		if (ret)
 			return ret;
 
-- 
2.38.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 4/4] iio: core: move 'mlock' to 'struct iio_dev_opaque'

[Nuno Sá]

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 use this lock.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 cf80f81e4665..5049404d1148 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.38.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 1/4] iio: core: introduce iio_device_{claim|release}_buffer_mode() APIs

[Andy Shevchenko]

On Wed, Oct 12, 2022 at 6:15 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> These APIs are analogous to iio_device_claim_direct_mode() and
> iio_device_release_direct_mode() but, as the name suggests, with the
> logic flipped. While this looks odd enough, it will have at least two
> users (in following changes) and it will be important to move the iio
> mlock to the private struct.

IIO

> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/iio/industrialio-core.c | 38 +++++++++++++++++++++++++++++++++
>  include/linux/iio/iio.h         |  2 ++
>  2 files changed, 40 insertions(+)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 151ff3993354..cf80f81e4665 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -2083,6 +2083,44 @@ void iio_device_release_direct_mode(struct iio_dev *indio_dev)
>  }
>  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
>
> +/**
> + * iio_device_claim_buffer_mode - Keep device in buffer mode
> + * @indio_dev: the iio_dev associated with the device
> + *
> + * If the device is in buffer mode it is guaranteed to stay
> + * that way until iio_device_release_buffer_mode() is called.
> + *
> + * Use with iio_device_release_buffer_mode()

Missed trailing period.

> + *
> + * Returns: 0 on success, -EBUSY on failure

Ditto.

> + */
> +int iio_device_claim_buffer_mode(struct iio_dev *indio_dev)
> +{
> +       mutex_lock(&indio_dev->mlock);
> +
> +       if (iio_buffer_enabled(indio_dev))
> +               return 0;
> +
> +       mutex_unlock(&indio_dev->mlock);
> +       return -EBUSY;
> +}
> +EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode);
> +
> +/**
> + * iio_device_release_buffer_mode - releases claim on buffer mode
> + * @indio_dev: the iio_dev associated with the device
> + *
> + * Release the claim. Device is no longer guaranteed to stay
> + * in buffer mode.
> + *
> + * Use with iio_device_claim_buffer_mode()

Ditto.

> + */
> +void iio_device_release_buffer_mode(struct iio_dev *indio_dev)
> +{
> +       mutex_unlock(&indio_dev->mlock);
> +}
> +EXPORT_SYMBOL_GPL(iio_device_release_buffer_mode);
> +
>  /**
>   * iio_device_get_current_mode() - helper function providing read-only access to
>   *                                the opaque @currentmode variable
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index f0ec8a5e5a7a..9d3bd6379eb8 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -629,6 +629,8 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
>  int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
>  void iio_device_release_direct_mode(struct iio_dev *indio_dev);
> +int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
> +void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
>
>  extern struct bus_type iio_bus_type;
>
> --
> 2.38.0
>


-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 2/4] iio: health: max30100: do not use internal iio_dev lock

[Andy Shevchenko]

On Wed, Oct 12, 2022 at 6:15 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> The pattern used in this device does not quite fit in the
> iio_device_claim_direct_mode() typical usage. In this case,
> iio_buffer_enabled() was being used not to prevent the raw access but to
> allow it. Hence, let's make use of the new
> iio_device_claim_buffer_mode() API to make sure we stay in buffered mode
> during the complete read.
>
> Note that we are shadowing the error code returned by
> iio_device_claim_buffer_mode() so that we keep the original one
> (-EAGAIN). The reason is that some userspace stack might already be
> relying on this particular code so that we are not taking chances and
> leave it alone.

The above line widths seem a bit arbitrary to me. But I think it's due
to function names in them.
Perhaps you can make them less deviated by shuffling a bit, like
moving "but to" to the next line.

> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/iio/health/max30100.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
> index 2cca5e0519f8..6ac49901c9da 100644
> --- a/drivers/iio/health/max30100.c
> +++ b/drivers/iio/health/max30100.c
> @@ -387,18 +387,15 @@ static int max30100_read_raw(struct iio_dev *indio_dev,
>                  * Temperature reading can only be acquired while engine
>                  * is running
>                  */
> -               mutex_lock(&indio_dev->mlock);
> -
> -               if (!iio_buffer_enabled(indio_dev))

> +               if (iio_device_claim_buffer_mode(indio_dev)) {

I think a summary of replacing error code is good to have here, like

/*
 * Replacing -EBUSY or other error code
 * returned by iio_device_claim_buffer_mode()
 * because user space may rely on the current
 * one.
 */

>                         ret = -EAGAIN;
> -               else {
> +               } else {
>                         ret = max30100_get_temp(data, val);
>                         if (!ret)
>                                 ret = IIO_VAL_INT;
>
> +                       iio_device_release_buffer_mode(indio_dev);
>                 }
> -
> -               mutex_unlock(&indio_dev->mlock);
>                 break;
>         case IIO_CHAN_INFO_SCALE:
>                 *val = 1;  /* 0.0625 */

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 0/4] Make 'mlock' really private

[Andy Shevchenko]

On Wed, Oct 12, 2022 at 6:15 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> This patchset cleans all the drivers directly using the iio_device 'mlock'.
> This lock is private and should not be used outside the core (or by using
> proper helpers).
>
> Most of the conversions where straight, but there are some that really need
> extra looking. Mainly patches [13/15] and [14/15] were a bit hacky since
> iio_device_claim_direct_mode() does not fit 100%. The reason is that we
> want to check if the device is buffering and do something if it is (in
> which case the API return -EBUSY and released the lock. I just used a
> combinations of locks to get around this (hopefully I did not messed up).
>
> Note that this series was only compiled tested using allyesconfig for
> ARM. I ran 'git grep' to make sure there were no more users of 'mlock'.
> Hopefully I covered them all...

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

I haven't seen any serious issues, some small ones regarding spelling,
indentation and comment are per individual patches.

> v2:
>
> [PATCH 1-8, 10-12/16]
>  * Mention the inclusion of mutex.h in the commit message.
>
> [PATCH 1-8, 10, 12/16]
>  * Initialize mutex as late as possible.
> Note that [PATCH 11/16] was not included since the code to do so was not
> direct enough. Would need to get a pointer to the private struture
> outside of scmi_alloc_iiodev() to do it. While not hard, the added changes
> in the code is not really worth it (IMO of course).
>
> [PATCH 1/16]
>  * Refactored the commit message a bit. I guess this one will still needs
> more discussion...
>
> [PATCH 9/16]
>  * New patch to add an helper function to read the samples.
>
> [PATCH 13/16]
>  * New patch to introduce iio_device_{claim|release}_buffer_mode() APIs.
>
> [PATCH 14/16]
>  * Make use of the new iio_device_{claim|release}_buffer_mode() helpers
>
> [PATCH 15/16]
>  * Make use of the new iio_device_{claim|release}_buffer_mode() helpers
> in combination with claim_direct_mode(). This is needed so that we make sure
> we always get one of the modes (and hence the iio_dev lock) to safely call
> max30102_get_temp(). Note that I'm not particular "happy" with the code but
> OTOH, it does not look as bad as I thought :). Anyways, if there are no
> complains with it, I'm ok to leave it as-is. Otherwise, I think we can think
> on the flag approach (briefly discussed in the first series).
>
> v3:
>
> [PATCH 1/4]
>  * fix 'make W=1' warning about prototypes mismatch.
>
> [PATCH 2/4]
>  * improved commit message to explain why we are shadowing error codes.
>
> [PATCH 4/4]
>  * minor English fix on the commit message (as suggested by Andy).
>
> Nuno Sá (4):
>   iio: core: introduce iio_device_{claim|release}_buffer_mode() APIs
>   iio: health: max30100: do not use internal iio_dev lock
>   iio: health: max30102: do not use internal iio_dev lock
>   iio: core: move 'mlock' to 'struct iio_dev_opaque'
>
>  drivers/iio/TODO                   |  3 --
>  drivers/iio/health/max30100.c      |  9 ++---
>  drivers/iio/health/max30102.c      | 19 +++++++---
>  drivers/iio/industrialio-buffer.c  | 29 ++++++++-------
>  drivers/iio/industrialio-core.c    | 58 +++++++++++++++++++++++++-----
>  drivers/iio/industrialio-event.c   |  4 +--
>  drivers/iio/industrialio-trigger.c | 12 +++----
>  include/linux/iio/iio-opaque.h     |  2 ++
>  include/linux/iio/iio.h            |  5 ++-
>  9 files changed, 97 insertions(+), 44 deletions(-)
>
> --
> 2.38.0
>


-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 3/4] iio: health: max30102: do not use internal iio_dev lock

[Miquel Raynal]

Hi Nuno,

nuno.sa@analog.com wrote on Wed, 12 Oct 2022 17:16:19 +0200:

> The pattern used in this device does not quite fit in the
> iio_device_claim_direct_mode() typical usage. In this case, we want to
> know if we are in buffered mode or not to know if the device is powered
> (buffer mode) or not. And depending on that max30102_get_temp() will
> power on the device if needed. Hence, in order to keep the same
> functionality, we try to:
> 
> 1. Claim Buffered mode;
> 2: If 1) succeeds call max30102_get_temp() without powering on the
>    device;
> 3: Release Buffered mode;
> 4: If 1) fails, Claim Direct mode;
> 5: If 4) succeeds call max30102_get_temp() with powering on the device;
> 6: Release Direct mode;
> 7: If 4) fails, goto to 1) and try again.
> 
> This dance between buffered and direct mode is not particularly pretty
> (as well as the loop introduced by the goto statement) but it does allow
> us to get rid of the mlock usage while keeping the same behavior.

What about adding a TODO comment saying something like: "this comes
from static analysis and helped dropping mlock access, but someone with
the device needs to figure out if we can simplify this dance"? Because
the reason behind all this is that we don't want to risk breaking the
driver, but perhaps a simpler approach would work, right?

> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/iio/health/max30102.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/health/max30102.c
> b/drivers/iio/health/max30102.c index 437298a29f2d..66df4aaa31a7
> 100644 --- a/drivers/iio/health/max30102.c
> +++ b/drivers/iio/health/max30102.c
> @@ -477,12 +477,23 @@ static int max30102_read_raw(struct iio_dev
> *indio_dev,
>  		 * Temperature reading can only be acquired when not
> in
>  		 * shutdown; leave shutdown briefly when buffer not
> running */
> -		mutex_lock(&indio_dev->mlock);
> -		if (!iio_buffer_enabled(indio_dev))
> +any_mode_retry:
> +		if (iio_device_claim_buffer_mode(indio_dev)) {
> +			/*
> +			 * This one is a *bit* hacky. If we cannot
> claim buffer
> +			 * mode, then try direct mode so that we
> make sure
> +			 * things cannot concurrently change. And we
> just keep
> +			 * trying until we get one of the modes...
> +			 */
> +			if (iio_device_claim_direct_mode(indio_dev))
> +				goto any_mode_retry;
> +
>  			ret = max30102_get_temp(data, val, true);
> -		else
> +			iio_device_release_direct_mode(indio_dev);
> +		} else {
>  			ret = max30102_get_temp(data, val, false);
> -		mutex_unlock(&indio_dev->mlock);
> +			iio_device_release_buffer_mode(indio_dev);
> +		}
>  		if (ret)
>  			return ret;
>  


Thanks,
Miquèl

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 2/4] iio: health: max30100: do not use internal iio_dev lock

[Nuno Sá]

On Wed, 2022-10-12 at 20:46 +0300, Andy Shevchenko wrote:
> On Wed, Oct 12, 2022 at 6:15 PM Nuno Sá <nuno.sa@analog.com> wrote:
> > 
> > The pattern used in this device does not quite fit in the
> > iio_device_claim_direct_mode() typical usage. In this case,
> > iio_buffer_enabled() was being used not to prevent the raw access
> > but to
> > allow it. Hence, let's make use of the new
> > iio_device_claim_buffer_mode() API to make sure we stay in buffered
> > mode
> > during the complete read.
> > 
> > Note that we are shadowing the error code returned by
> > iio_device_claim_buffer_mode() so that we keep the original one
> > (-EAGAIN). The reason is that some userspace stack might already be
> > relying on this particular code so that we are not taking chances
> > and
> > leave it alone.
> 
> The above line widths seem a bit arbitrary to me. But I think it's
> due
> to function names in them.
> Perhaps you can make them less deviated by shuffling a bit, like
> moving "but to" to the next line.
> 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > ---
> >  drivers/iio/health/max30100.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/health/max30100.c
> > b/drivers/iio/health/max30100.c
> > index 2cca5e0519f8..6ac49901c9da 100644
> > --- a/drivers/iio/health/max30100.c
> > +++ b/drivers/iio/health/max30100.c
> > @@ -387,18 +387,15 @@ static int max30100_read_raw(struct iio_dev
> > *indio_dev,
> >                  * Temperature reading can only be acquired while
> > engine
> >                  * is running
> >                  */
> > -               mutex_lock(&indio_dev->mlock);
> > -
> > -               if (!iio_buffer_enabled(indio_dev))
> 
> > +               if (iio_device_claim_buffer_mode(indio_dev)) {
> 
> I think a summary of replacing error code is good to have here, like
> 
> /*
>  * Replacing -EBUSY or other error code
>  * returned by iio_device_claim_buffer_mode()
>  * because user space may rely on the current
>  * one.
>  */
> 

This might make sense... I'll wait for Jonathan's review to see how
strong he feels about this. Maybe he can also add it when applying.

- Nuno Sá
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 3/4] iio: health: max30102: do not use internal iio_dev lock

[Nuno Sá]

On Wed, 2022-10-12 at 20:45 +0200, Miquel Raynal wrote:
> Hi Nuno,
> 
> nuno.sa@analog.com wrote on Wed, 12 Oct 2022 17:16:19 +0200:
> 
> > The pattern used in this device does not quite fit in the
> > iio_device_claim_direct_mode() typical usage. In this case, we want
> > to
> > know if we are in buffered mode or not to know if the device is
> > powered
> > (buffer mode) or not. And depending on that max30102_get_temp()
> > will
> > power on the device if needed. Hence, in order to keep the same
> > functionality, we try to:
> > 
> > 1. Claim Buffered mode;
> > 2: If 1) succeeds call max30102_get_temp() without powering on the
> >    device;
> > 3: Release Buffered mode;
> > 4: If 1) fails, Claim Direct mode;
> > 5: If 4) succeeds call max30102_get_temp() with powering on the
> > device;
> > 6: Release Direct mode;
> > 7: If 4) fails, goto to 1) and try again.
> > 
> > This dance between buffered and direct mode is not particularly
> > pretty
> > (as well as the loop introduced by the goto statement) but it does
> > allow
> > us to get rid of the mlock usage while keeping the same behavior.
> 
> What about adding a TODO comment saying something like: "this comes
> from static analysis and helped dropping mlock access, but someone
> with
> the device needs to figure out if we can simplify this dance"?
> Because
> the reason behind all this is that we don't want to risk breaking the
> driver, but perhaps a simpler approach would work, right?
> 

Hi Miquel,

AFAIU, either the device is powered (when buffer mode enabled) and we
can do the reading or it's not and we need to power it on/off
"manually" while making sure we don't race against enable/disabling
buffers. This "dance" is needed mainly to make sure that we grab
'mlock' one way or another... The other way would be to use some
specific device lock together with a flag (as discussed) but as
discussed with Jonathan we decided to go down this road... So,
honestly, I don't really see the necessity of "marking" this code with
a TODO but of course if someone comes in with something simpler, great
:).

Anyways, as I said, I'm not really keen in spinning a new version to
add this comment so I will defer the decision to Jonathan :)  

Thanks for the help!
- Nuno Sá


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 3/4] iio: health: max30102: do not use internal iio_dev lock

[Jonathan Cameron]

On Fri, 14 Oct 2022 09:25:59 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Wed, 2022-10-12 at 20:45 +0200, Miquel Raynal wrote:
> > Hi Nuno,
> > 
> > nuno.sa@analog.com wrote on Wed, 12 Oct 2022 17:16:19 +0200:
> >   
> > > The pattern used in this device does not quite fit in the
> > > iio_device_claim_direct_mode() typical usage. In this case, we want
> > > to
> > > know if we are in buffered mode or not to know if the device is
> > > powered
> > > (buffer mode) or not. And depending on that max30102_get_temp()
> > > will
> > > power on the device if needed. Hence, in order to keep the same
> > > functionality, we try to:
> > > 
> > > 1. Claim Buffered mode;
> > > 2: If 1) succeeds call max30102_get_temp() without powering on the
> > >    device;
> > > 3: Release Buffered mode;
> > > 4: If 1) fails, Claim Direct mode;
> > > 5: If 4) succeeds call max30102_get_temp() with powering on the
> > > device;
> > > 6: Release Direct mode;
> > > 7: If 4) fails, goto to 1) and try again.
> > > 
> > > This dance between buffered and direct mode is not particularly
> > > pretty
> > > (as well as the loop introduced by the goto statement) but it does
> > > allow
> > > us to get rid of the mlock usage while keeping the same behavior.  
> > 
> > What about adding a TODO comment saying something like: "this comes
> > from static analysis and helped dropping mlock access, but someone
> > with
> > the device needs to figure out if we can simplify this dance"?
> > Because
> > the reason behind all this is that we don't want to risk breaking the
> > driver, but perhaps a simpler approach would work, right?
> >   
> 
> Hi Miquel,
> 
> AFAIU, either the device is powered (when buffer mode enabled) and we
> can do the reading or it's not and we need to power it on/off
> "manually" while making sure we don't race against enable/disabling
> buffers. This "dance" is needed mainly to make sure that we grab
> 'mlock' one way or another... The other way would be to use some
> specific device lock together with a flag (as discussed) but as
> discussed with Jonathan we decided to go down this road... So,
> honestly, I don't really see the necessity of "marking" this code with
> a TODO but of course if someone comes in with something simpler, great
> :).

Agreed.  I don't expect to see any improvement in this in the future
so a TODO would just be noise and might encourage people to propose
the 'get the lock on it's own function' that we are going through this
dance to avoid adding.

Jonathan

> 
> Anyways, as I said, I'm not really keen in spinning a new version to
> add this comment so I will defer the decision to Jonathan :)  
> 
> Thanks for the help!
> - Nuno Sá
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 0/4] Make 'mlock' really private

[Jonathan Cameron]

On Wed, 12 Oct 2022 20:49:13 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Oct 12, 2022 at 6:15 PM Nuno Sá <nuno.sa@analog.com> wrote:
> >
> > This patchset cleans all the drivers directly using the iio_device 'mlock'.
> > This lock is private and should not be used outside the core (or by using
> > proper helpers).
> >
> > Most of the conversions where straight, but there are some that really need
> > extra looking. Mainly patches [13/15] and [14/15] were a bit hacky since
> > iio_device_claim_direct_mode() does not fit 100%. The reason is that we
> > want to check if the device is buffering and do something if it is (in
> > which case the API return -EBUSY and released the lock. I just used a
> > combinations of locks to get around this (hopefully I did not messed up).
> >
> > Note that this series was only compiled tested using allyesconfig for
> > ARM. I ran 'git grep' to make sure there were no more users of 'mlock'.
> > Hopefully I covered them all...  
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> I haven't seen any serious issues, some small ones regarding spelling,
> indentation and comment are per individual patches.

Tweaked inline with Andy's suggestions whilst applying.

Applied to the togreg branch of iio.git and pushed out as testing for now
as I'll be rebasing on rc1 once available.

Thanks,

Jonathan

> 
> > v2:
> >
> > [PATCH 1-8, 10-12/16]
> >  * Mention the inclusion of mutex.h in the commit message.
> >
> > [PATCH 1-8, 10, 12/16]
> >  * Initialize mutex as late as possible.
> > Note that [PATCH 11/16] was not included since the code to do so was not
> > direct enough. Would need to get a pointer to the private struture
> > outside of scmi_alloc_iiodev() to do it. While not hard, the added changes
> > in the code is not really worth it (IMO of course).
> >
> > [PATCH 1/16]
> >  * Refactored the commit message a bit. I guess this one will still needs
> > more discussion...
> >
> > [PATCH 9/16]
> >  * New patch to add an helper function to read the samples.
> >
> > [PATCH 13/16]
> >  * New patch to introduce iio_device_{claim|release}_buffer_mode() APIs.
> >
> > [PATCH 14/16]
> >  * Make use of the new iio_device_{claim|release}_buffer_mode() helpers
> >
> > [PATCH 15/16]
> >  * Make use of the new iio_device_{claim|release}_buffer_mode() helpers
> > in combination with claim_direct_mode(). This is needed so that we make sure
> > we always get one of the modes (and hence the iio_dev lock) to safely call
> > max30102_get_temp(). Note that I'm not particular "happy" with the code but
> > OTOH, it does not look as bad as I thought :). Anyways, if there are no
> > complains with it, I'm ok to leave it as-is. Otherwise, I think we can think
> > on the flag approach (briefly discussed in the first series).
> >
> > v3:
> >
> > [PATCH 1/4]
> >  * fix 'make W=1' warning about prototypes mismatch.
> >
> > [PATCH 2/4]
> >  * improved commit message to explain why we are shadowing error codes.
> >
> > [PATCH 4/4]
> >  * minor English fix on the commit message (as suggested by Andy).
> >
> > Nuno Sá (4):
> >   iio: core: introduce iio_device_{claim|release}_buffer_mode() APIs
> >   iio: health: max30100: do not use internal iio_dev lock
> >   iio: health: max30102: do not use internal iio_dev lock
> >   iio: core: move 'mlock' to 'struct iio_dev_opaque'
> >
> >  drivers/iio/TODO                   |  3 --
> >  drivers/iio/health/max30100.c      |  9 ++---
> >  drivers/iio/health/max30102.c      | 19 +++++++---
> >  drivers/iio/industrialio-buffer.c  | 29 ++++++++-------
> >  drivers/iio/industrialio-core.c    | 58 +++++++++++++++++++++++++-----
> >  drivers/iio/industrialio-event.c   |  4 +--
> >  drivers/iio/industrialio-trigger.c | 12 +++----
> >  include/linux/iio/iio-opaque.h     |  2 ++
> >  include/linux/iio/iio.h            |  5 ++-
> >  9 files changed, 97 insertions(+), 44 deletions(-)
> >
> > --
> > 2.38.0
> >  
> 
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic