This patchset introduces two helper functions to simplify driver code
requiring the device to be locked in direct mode during execution of a
code path. The staging driver ad7192 is updated to demonstrate usage.
This could be applied to approximately 18 known cases where the driver
is holding the lock in direct mode. Unknown cases might be those that
should, but don't, hold the lock.
Alternate implementation: Generalize to support a claim on any mode.
Do iio_claim_mode(device,mode) where if the device is in *mode*, it
is guaranteed to stay that way until release is called. I considered
and rejected this option because a) not sure other modes would ever
need to be locked, and b) the semantic improvement is less when it
is generalized.
This patchset was inspired by a discussion on linux-iio:
http://www.spinics.net/lists/linux-iio/msg18540.html
Alison Schofield (2):
iio: core: implement iio_{claim|release}_direct_mode()
staging: iio: adc7192: use iio_{claim|release}_direct_mode()
drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++
drivers/staging/iio/adc/ad7192.c | 24 +++++++++---------------
include/linux/iio/iio.h | 2 ++
3 files changed, 50 insertions(+), 15 deletions(-)
--
2.1.4
It is often the case that the driver wants to be sure a device stays
in direct mode while it is executing a task or series of tasks. To
accomplish this today, the driver performs this sequence: 1) take the
device state lock, 2)verify it is not in a buffered mode, 3) execute
some tasks, and 4) release that lock.
This patch introduces a pair of helper functions that simplify these
steps and make it more semantically expressive.
iio_claim_direct_mode()
If the device is not in any buffered mode it is guaranteed
to stay that way until iio_release_direct_mode() is called.
iio_release_direct_mode()
Release the claim. Device is no longer guaranteed to stay
in direct mode.
Signed-off-by: Alison Schofield <[email protected]>
---
drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++
include/linux/iio/iio.h | 2 ++
2 files changed, 41 insertions(+)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 70cb7eb..f6f0c89 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -25,6 +25,7 @@
#include <linux/slab.h>
#include <linux/anon_inodes.h>
#include <linux/debugfs.h>
+#include <linux/mutex.h>
#include <linux/iio/iio.h>
#include "iio_core.h"
#include "iio_core_trigger.h"
@@ -1375,6 +1376,44 @@ void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev)
}
EXPORT_SYMBOL_GPL(devm_iio_device_unregister);
+/**
+ * iio_claim_direct_mode - Keep device in direct mode
+ * @indio_dev: the iio_dev associated with the device
+ *
+ * If the device is in direct mode it is guaranteed to
+ * stay that way until iio_release_direct_mode() is called.
+ *
+ * Use with iio_release_direct_mode()
+ *
+ * Returns: 0 on success, -EINVAL on failure
+ */
+int iio_claim_direct_mode(struct iio_dev *indio_dev)
+{
+ mutex_lock(&indio_dev->mlock);
+
+ if (iio_buffer_enabled(indio_dev)) {
+ mutex_unlock(&indio_dev->mlock);
+ return -EINVAL;
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(iio_claim_direct_mode);
+
+/**
+ * iio_release_direct_mode - releases claim on direct mode
+ * @indio_dev: the iio_dev associated with the device
+ *
+ * Release the claim. Device is no longer guaranteed to stay
+ * in direct mode.
+ *
+ * Use with iio_claim_direct_mode()
+ */
+void iio_release_direct_mode(struct iio_dev *indio_dev)
+{
+ mutex_unlock(&indio_dev->mlock);
+}
+EXPORT_SYMBOL_GPL(iio_release_direct_mode);
+
subsys_initcall(iio_init);
module_exit(iio_exit);
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index ce9e9c1..9efe2af 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -527,6 +527,8 @@ void iio_device_unregister(struct iio_dev *indio_dev);
int devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev);
void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev);
int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
+int iio_claim_direct_mode(struct iio_dev *indio_dev);
+void iio_release_direct_mode(struct iio_dev *indio_dev);
extern struct bus_type iio_bus_type;
--
2.1.4
Replace the code that guarantees the device stays in direct mode with
iio_{claim|release}_direct_mode() which does same.
Signed-off-by: Alison Schofield <[email protected]>
---
drivers/staging/iio/adc/ad7192.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index f843f19..401ec91 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -349,11 +349,9 @@ static ssize_t ad7192_write_frequency(struct device *dev,
if (lval == 0)
return -EINVAL;
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_claim_direct_mode(indio_dev);
+ if (ret)
return -EBUSY;
- }
div = st->mclk / (lval * st->f_order * 1024);
if (div < 1 || div > 1023) {
@@ -366,7 +364,7 @@ static ssize_t ad7192_write_frequency(struct device *dev,
ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
out:
- mutex_unlock(&indio_dev->mlock);
+ iio_release_direct_mode(indio_dev);
return ret ? ret : len;
}
@@ -434,11 +432,9 @@ static ssize_t ad7192_set(struct device *dev,
if (ret < 0)
return ret;
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_claim_direct_mode(indio_dev);
+ if (ret)
return -EBUSY;
- }
switch ((u32)this_attr->address) {
case AD7192_REG_GPOCON:
@@ -461,7 +457,7 @@ static ssize_t ad7192_set(struct device *dev,
ret = -EINVAL;
}
- mutex_unlock(&indio_dev->mlock);
+ iio_release_direct_mode(indio_dev);
return ret ? ret : len;
}
@@ -555,11 +551,9 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
int ret, i;
unsigned int tmp;
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_claim_direct_mode(indio_dev);
+ if (ret)
return -EBUSY;
- }
switch (mask) {
case IIO_CHAN_INFO_SCALE:
@@ -582,7 +576,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
ret = -EINVAL;
}
- mutex_unlock(&indio_dev->mlock);
+ iio_release_direct_mode(indio_dev);
return ret;
}
--
2.1.4
On 03/01/2016 08:02 PM, Alison Schofield wrote:
> It is often the case that the driver wants to be sure a device stays
> in direct mode while it is executing a task or series of tasks. To
> accomplish this today, the driver performs this sequence: 1) take the
> device state lock, 2)verify it is not in a buffered mode, 3) execute
> some tasks, and 4) release that lock.
>
> This patch introduces a pair of helper functions that simplify these
> steps and make it more semantically expressive.
>
> iio_claim_direct_mode()
> If the device is not in any buffered mode it is guaranteed
> to stay that way until iio_release_direct_mode() is called.
>
> iio_release_direct_mode()
> Release the claim. Device is no longer guaranteed to stay
> in direct mode.
>
> Signed-off-by: Alison Schofield <[email protected]>
Looks basically good.
> ---
> drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++
> include/linux/iio/iio.h | 2 ++
> 2 files changed, 41 insertions(+)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 70cb7eb..f6f0c89 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -25,6 +25,7 @@
> #include <linux/slab.h>
> #include <linux/anon_inodes.h>
> #include <linux/debugfs.h>
> +#include <linux/mutex.h>
> #include <linux/iio/iio.h>
> #include "iio_core.h"
> #include "iio_core_trigger.h"
> @@ -1375,6 +1376,44 @@ void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev)
> }
> EXPORT_SYMBOL_GPL(devm_iio_device_unregister);
>
> +/**
> + * iio_claim_direct_mode - Keep device in direct mode
> + * @indio_dev: the iio_dev associated with the device
> + *
> + * If the device is in direct mode it is guaranteed to
> + * stay that way until iio_release_direct_mode() is called.
> + *
> + * Use with iio_release_direct_mode()
> + *
> + * Returns: 0 on success, -EINVAL on failure
> + */
> +int iio_claim_direct_mode(struct iio_dev *indio_dev)
To be consistent with the reset of the API I'd use the iio_device_... prefix
here, same for the release function.
> +{
> + mutex_lock(&indio_dev->mlock);
> +
> + if (iio_buffer_enabled(indio_dev)) {
> + mutex_unlock(&indio_dev->mlock);
> + return -EINVAL;
-EINVAL doesn't make much sense here, -EBUSY is better.
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iio_claim_direct_mode);
>
On 02/03/16 13:28, Lars-Peter Clausen wrote:
> On 03/01/2016 08:02 PM, Alison Schofield wrote:
>> It is often the case that the driver wants to be sure a device stays
>> in direct mode while it is executing a task or series of tasks. To
>> accomplish this today, the driver performs this sequence: 1) take the
>> device state lock, 2)verify it is not in a buffered mode, 3) execute
>> some tasks, and 4) release that lock.
>>
>> This patch introduces a pair of helper functions that simplify these
>> steps and make it more semantically expressive.
>>
>> iio_claim_direct_mode()
>> If the device is not in any buffered mode it is guaranteed
>> to stay that way until iio_release_direct_mode() is called.
>>
>> iio_release_direct_mode()
>> Release the claim. Device is no longer guaranteed to stay
>> in direct mode.
>>
>> Signed-off-by: Alison Schofield <[email protected]>
>
> Looks basically good.
Agreed - nothing to add from me to what Lars has covered here.
Nice to 'hide' the accesses to mlock as well as will cut out the desire
to 'abuse it'. Amusingly we only just 'fixed' the docs to to say this
element of iio_dev was usable by drivers. Once we have these new functions
in use throughout the tree, we will want to flip that back again to internal
only.
Jonathan
>
>> ---
>> drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++
>> include/linux/iio/iio.h | 2 ++
>> 2 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>> index 70cb7eb..f6f0c89 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -25,6 +25,7 @@
>> #include <linux/slab.h>
>> #include <linux/anon_inodes.h>
>> #include <linux/debugfs.h>
>> +#include <linux/mutex.h>
>> #include <linux/iio/iio.h>
>> #include "iio_core.h"
>> #include "iio_core_trigger.h"
>> @@ -1375,6 +1376,44 @@ void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev)
>> }
>> EXPORT_SYMBOL_GPL(devm_iio_device_unregister);
>>
>> +/**
>> + * iio_claim_direct_mode - Keep device in direct mode
>> + * @indio_dev: the iio_dev associated with the device
>> + *
>> + * If the device is in direct mode it is guaranteed to
>> + * stay that way until iio_release_direct_mode() is called.
>> + *
>> + * Use with iio_release_direct_mode()
>> + *
>> + * Returns: 0 on success, -EINVAL on failure
>> + */
>> +int iio_claim_direct_mode(struct iio_dev *indio_dev)
>
> To be consistent with the reset of the API I'd use the iio_device_... prefix
> here, same for the release function.
>
>> +{
>> + mutex_lock(&indio_dev->mlock);
>> +
>> + if (iio_buffer_enabled(indio_dev)) {
>> + mutex_unlock(&indio_dev->mlock);
>> + return -EINVAL;
>
> -EINVAL doesn't make much sense here, -EBUSY is better.
>
>> + }
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(iio_claim_direct_mode);
>>
This patchset introduces two helper functions to simplify driver code
requiring the device to be locked in direct mode during execution of a
code path. The staging driver ad7192 is updated to demonstrate usage.
This could be applied to approximately 18 known cases where the driver
is holding the lock in direct mode. Unknown cases might be those that
should, but don't, hold the lock.
Alternate implementation: Generalize to support a claim on any mode.
Do iio_claim_mode(device,mode) where if the device is in *mode*, it
is guaranteed to stay that way until release is called. I considered
and rejected this option because a) not sure other modes would ever
need to be locked, and b) the semantic improvement is less when it
is generalized.
This patchset was inspired by a discussion on linux-iio:
http://www.spinics.net/lists/linux-iio/msg18540.html
Changes in v2:
o use iio_device prefix for new functions
o replace EINVAL with EBUSY on failure to claim direct mode
o update commit msg & changelog to reflect new prefix
Alison Schofield (2):
iio: core: implement iio_device_{claim|release}_direct_mode()
staging: iio: ad7192: use iio_device_{claim|release}_direct_mode()
drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++
drivers/staging/iio/adc/ad7192.c | 24 +++++++++---------------
include/linux/iio/iio.h | 2 ++
3 files changed, 50 insertions(+), 15 deletions(-)
--
2.1.4
It is often the case that the driver wants to be sure a device stays
in direct mode while it is executing a task or series of tasks. To
accomplish this today, the driver performs this sequence: 1) take the
device state lock, 2) verify it is not in a buffered mode, 3) execute
some tasks, and 4) release that lock.
This patch introduces a pair of helper functions that simplify these
steps and make it more semantically expressive.
iio_device_claim_direct_mode()
If the device is not in any buffered mode it is guaranteed
to stay that way until iio_release_direct_mode() is called.
iio_device_release_direct_mode()
Release the claim. Device is no longer guaranteed to stay
in direct mode.
Signed-off-by: Alison Schofield <[email protected]>
---
drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++
include/linux/iio/iio.h | 2 ++
2 files changed, 41 insertions(+)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 70cb7eb..2e768bc 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -25,6 +25,7 @@
#include <linux/slab.h>
#include <linux/anon_inodes.h>
#include <linux/debugfs.h>
+#include <linux/mutex.h>
#include <linux/iio/iio.h>
#include "iio_core.h"
#include "iio_core_trigger.h"
@@ -1375,6 +1376,44 @@ void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev)
}
EXPORT_SYMBOL_GPL(devm_iio_device_unregister);
+/**
+ * iio_device_claim_direct_mode - Keep device in direct mode
+ * @indio_dev: the iio_dev associated with the device
+ *
+ * If the device is in direct mode it is guaranteed to stay
+ * that way until iio_device_release_direct_mode() is called.
+ *
+ * Use with iio_device_release_direct_mode()
+ *
+ * Returns: 0 on success, -EBUSY on failure
+ */
+int iio_device_claim_direct_mode(struct iio_dev *indio_dev)
+{
+ mutex_lock(&indio_dev->mlock);
+
+ if (iio_buffer_enabled(indio_dev)) {
+ mutex_unlock(&indio_dev->mlock);
+ return -EBUSY;
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
+
+/**
+ * iio_device_release_direct_mode - releases claim on direct mode
+ * @indio_dev: the iio_dev associated with the device
+ *
+ * Release the claim. Device is no longer guaranteed to stay
+ * in direct mode.
+ *
+ * Use with iio_device_claim_direct_mode()
+ */
+void iio_device_release_direct_mode(struct iio_dev *indio_dev)
+{
+ mutex_unlock(&indio_dev->mlock);
+}
+EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
+
subsys_initcall(iio_init);
module_exit(iio_exit);
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index b2b1677..0b2773a 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -527,6 +527,8 @@ void iio_device_unregister(struct iio_dev *indio_dev);
int devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev);
void devm_iio_device_unregister(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);
extern struct bus_type iio_bus_type;
--
2.1.4
Replace the code that guarantees the device stays in direct mode with
iio_device_{claim|release}_direct_mode() which does same.
Signed-off-by: Alison Schofield <[email protected]>
---
drivers/staging/iio/adc/ad7192.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index f843f19..94a2e06 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -349,11 +349,9 @@ static ssize_t ad7192_write_frequency(struct device *dev,
if (lval == 0)
return -EINVAL;
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
return -EBUSY;
- }
div = st->mclk / (lval * st->f_order * 1024);
if (div < 1 || div > 1023) {
@@ -366,7 +364,7 @@ static ssize_t ad7192_write_frequency(struct device *dev,
ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
out:
- mutex_unlock(&indio_dev->mlock);
+ iio_device_release_direct_mode(indio_dev);
return ret ? ret : len;
}
@@ -434,11 +432,9 @@ static ssize_t ad7192_set(struct device *dev,
if (ret < 0)
return ret;
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
return -EBUSY;
- }
switch ((u32)this_attr->address) {
case AD7192_REG_GPOCON:
@@ -461,7 +457,7 @@ static ssize_t ad7192_set(struct device *dev,
ret = -EINVAL;
}
- mutex_unlock(&indio_dev->mlock);
+ iio_device_release_direct_mode(indio_dev);
return ret ? ret : len;
}
@@ -555,11 +551,9 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
int ret, i;
unsigned int tmp;
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
return -EBUSY;
- }
switch (mask) {
case IIO_CHAN_INFO_SCALE:
@@ -582,7 +576,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
ret = -EINVAL;
}
- mutex_unlock(&indio_dev->mlock);
+ iio_device_release_direct_mode(indio_dev);
return ret;
}
--
2.1.4
On Sat, Mar 05, 2016 at 06:02:36PM +0000, Jonathan Cameron wrote:
> On 02/03/16 13:28, Lars-Peter Clausen wrote:
> > On 03/01/2016 08:02 PM, Alison Schofield wrote:
> >> It is often the case that the driver wants to be sure a device stays
> >> in direct mode while it is executing a task or series of tasks. To
> >> accomplish this today, the driver performs this sequence: 1) take the
> >> device state lock, 2)verify it is not in a buffered mode, 3) execute
> >> some tasks, and 4) release that lock.
> >>
> >> This patch introduces a pair of helper functions that simplify these
> >> steps and make it more semantically expressive.
> >>
> >> iio_claim_direct_mode()
> >> If the device is not in any buffered mode it is guaranteed
> >> to stay that way until iio_release_direct_mode() is called.
> >>
> >> iio_release_direct_mode()
> >> Release the claim. Device is no longer guaranteed to stay
> >> in direct mode.
> >>
> >> Signed-off-by: Alison Schofield <[email protected]>
> >
> > Looks basically good.
> Agreed - nothing to add from me to what Lars has covered here.
> Nice to 'hide' the accesses to mlock as well as will cut out the desire
> to 'abuse it'. Amusingly we only just 'fixed' the docs to to say this
> element of iio_dev was usable by drivers. Once we have these new functions
> in use throughout the tree, we will want to flip that back again to internal
> only.
>
> Jonathan
>
Thanks for the review (& Lars too)
Thinking about your note about flipping the mlock field back to
INTERNAL (from DRIVER), this change, even when it's applied to
all relevant instances, doesn't get us all the way there.
While these claim/release functions will remove direct access to mlock
where a driver wants to hold direct mode, the drivers are grabbing
mlock for other reasons also. (too many reasons/instances for me to
quickly understand or summarize)
I'm willing to look at it further and comment if that's helpful.
alisons
On 09/03/16 20:06, Alison Schofield wrote:
> On Sat, Mar 05, 2016 at 06:02:36PM +0000, Jonathan Cameron wrote:
>> On 02/03/16 13:28, Lars-Peter Clausen wrote:
>>> On 03/01/2016 08:02 PM, Alison Schofield wrote:
>>>> It is often the case that the driver wants to be sure a device stays
>>>> in direct mode while it is executing a task or series of tasks. To
>>>> accomplish this today, the driver performs this sequence: 1) take the
>>>> device state lock, 2)verify it is not in a buffered mode, 3) execute
>>>> some tasks, and 4) release that lock.
>>>>
>>>> This patch introduces a pair of helper functions that simplify these
>>>> steps and make it more semantically expressive.
>>>>
>>>> iio_claim_direct_mode()
>>>> If the device is not in any buffered mode it is guaranteed
>>>> to stay that way until iio_release_direct_mode() is called.
>>>>
>>>> iio_release_direct_mode()
>>>> Release the claim. Device is no longer guaranteed to stay
>>>> in direct mode.
>>>>
>>>> Signed-off-by: Alison Schofield <[email protected]>
>>>
>>> Looks basically good.
>> Agreed - nothing to add from me to what Lars has covered here.
>> Nice to 'hide' the accesses to mlock as well as will cut out the desire
>> to 'abuse it'. Amusingly we only just 'fixed' the docs to to say this
>> element of iio_dev was usable by drivers. Once we have these new functions
>> in use throughout the tree, we will want to flip that back again to internal
>> only.
>>
>> Jonathan
>>
> Thanks for the review (& Lars too)
>
> Thinking about your note about flipping the mlock field back to
> INTERNAL (from DRIVER), this change, even when it's applied to
> all relevant instances, doesn't get us all the way there.
>
> While these claim/release functions will remove direct access to mlock
> where a driver wants to hold direct mode, the drivers are grabbing
> mlock for other reasons also. (too many reasons/instances for me to
> quickly understand or summarize)
>
> I'm willing to look at it further and comment if that's helpful.
It would certainly be interesting to evaluate this. I suspect that most
are either in some obscure way connected to the mode or are 'misusing'
the lock for more general purposes where a driver specific lock would make
more sense.
Jonathan
>
> alisons
>
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 09/03/16 19:25, Alison Schofield wrote:
> This patchset introduces two helper functions to simplify driver code
> requiring the device to be locked in direct mode during execution of a
> code path. The staging driver ad7192 is updated to demonstrate usage.
>
> This could be applied to approximately 18 known cases where the driver
> is holding the lock in direct mode. Unknown cases might be those that
> should, but don't, hold the lock.
>
> Alternate implementation: Generalize to support a claim on any mode.
> Do iio_claim_mode(device,mode) where if the device is in *mode*, it
> is guaranteed to stay that way until release is called. I considered
> and rejected this option because a) not sure other modes would ever
> need to be locked, and b) the semantic improvement is less when it
> is generalized.
I agree with your chosen approach.
One general process comment. After the positive emails you received for
v1, dropping the Request For Comment would have made sense marking the
series as in your opinion ready to be applied.
Jonathan
>
> This patchset was inspired by a discussion on linux-iio:
> http://www.spinics.net/lists/linux-iio/msg18540.html
>
> Changes in v2:
> o use iio_device prefix for new functions
> o replace EINVAL with EBUSY on failure to claim direct mode
> o update commit msg & changelog to reflect new prefix
>
> Alison Schofield (2):
> iio: core: implement iio_device_{claim|release}_direct_mode()
> staging: iio: ad7192: use iio_device_{claim|release}_direct_mode()
>
> drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++
> drivers/staging/iio/adc/ad7192.c | 24 +++++++++---------------
> include/linux/iio/iio.h | 2 ++
> 3 files changed, 50 insertions(+), 15 deletions(-)
>
On 09/03/16 19:30, Alison Schofield wrote:
> It is often the case that the driver wants to be sure a device stays
> in direct mode while it is executing a task or series of tasks. To
> accomplish this today, the driver performs this sequence: 1) take the
> device state lock, 2) verify it is not in a buffered mode, 3) execute
> some tasks, and 4) release that lock.
>
> This patch introduces a pair of helper functions that simplify these
> steps and make it more semantically expressive.
>
> iio_device_claim_direct_mode()
> If the device is not in any buffered mode it is guaranteed
> to stay that way until iio_release_direct_mode() is called.
>
> iio_device_release_direct_mode()
> Release the claim. Device is no longer guaranteed to stay
> in direct mode.
>
> Signed-off-by: Alison Schofield <[email protected]>
I like change. Nice and clean and should help reduce 'misuse' of this
lock by making it more focused in the long run.
Applied to the togreg branch of iio.git - initially pushed out as testing
for the autobuilders to play with it.
> ---
> drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++
> include/linux/iio/iio.h | 2 ++
> 2 files changed, 41 insertions(+)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 70cb7eb..2e768bc 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -25,6 +25,7 @@
> #include <linux/slab.h>
> #include <linux/anon_inodes.h>
> #include <linux/debugfs.h>
> +#include <linux/mutex.h>
> #include <linux/iio/iio.h>
> #include "iio_core.h"
> #include "iio_core_trigger.h"
> @@ -1375,6 +1376,44 @@ void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev)
> }
> EXPORT_SYMBOL_GPL(devm_iio_device_unregister);
>
> +/**
> + * iio_device_claim_direct_mode - Keep device in direct mode
> + * @indio_dev: the iio_dev associated with the device
> + *
> + * If the device is in direct mode it is guaranteed to stay
> + * that way until iio_device_release_direct_mode() is called.
> + *
> + * Use with iio_device_release_direct_mode()
> + *
> + * Returns: 0 on success, -EBUSY on failure
> + */
> +int iio_device_claim_direct_mode(struct iio_dev *indio_dev)
> +{
> + mutex_lock(&indio_dev->mlock);
> +
> + if (iio_buffer_enabled(indio_dev)) {
> + mutex_unlock(&indio_dev->mlock);
> + return -EBUSY;
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
> +
> +/**
> + * iio_device_release_direct_mode - releases claim on direct mode
> + * @indio_dev: the iio_dev associated with the device
> + *
> + * Release the claim. Device is no longer guaranteed to stay
> + * in direct mode.
> + *
> + * Use with iio_device_claim_direct_mode()
> + */
> +void iio_device_release_direct_mode(struct iio_dev *indio_dev)
> +{
> + mutex_unlock(&indio_dev->mlock);
> +}
> +EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> +
> subsys_initcall(iio_init);
> module_exit(iio_exit);
>
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index b2b1677..0b2773a 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -527,6 +527,8 @@ void iio_device_unregister(struct iio_dev *indio_dev);
> int devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev);
> void devm_iio_device_unregister(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);
>
> extern struct bus_type iio_bus_type;
>
>
On 09/03/16 19:30, Alison Schofield wrote:
> Replace the code that guarantees the device stays in direct mode with
> iio_device_{claim|release}_direct_mode() which does same.
>
> Signed-off-by: Alison Schofield <[email protected]>
A small improvement inline - don't eat the errors within the
driver.
Jonathan
> ---
> drivers/staging/iio/adc/ad7192.c | 24 +++++++++---------------
> 1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
> index f843f19..94a2e06 100644
> --- a/drivers/staging/iio/adc/ad7192.c
> +++ b/drivers/staging/iio/adc/ad7192.c
> @@ -349,11 +349,9 @@ static ssize_t ad7192_write_frequency(struct device *dev,
> if (lval == 0)
> return -EINVAL;
>
> - mutex_lock(&indio_dev->mlock);
> - if (iio_buffer_enabled(indio_dev)) {
> - mutex_unlock(&indio_dev->mlock);
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> return -EBUSY;
return ret; In theory we might be returning some other
error code so best not to 'eat' the result.
> - }
>
> div = st->mclk / (lval * st->f_order * 1024);
> if (div < 1 || div > 1023) {
> @@ -366,7 +364,7 @@ static ssize_t ad7192_write_frequency(struct device *dev,
> ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
>
> out:
> - mutex_unlock(&indio_dev->mlock);
> + iio_device_release_direct_mode(indio_dev);
>
> return ret ? ret : len;
> }
> @@ -434,11 +432,9 @@ static ssize_t ad7192_set(struct device *dev,
> if (ret < 0)
> return ret;
>
> - mutex_lock(&indio_dev->mlock);
> - if (iio_buffer_enabled(indio_dev)) {
> - mutex_unlock(&indio_dev->mlock);
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> return -EBUSY;
Same here.
> - }
>
> switch ((u32)this_attr->address) {
> case AD7192_REG_GPOCON:
> @@ -461,7 +457,7 @@ static ssize_t ad7192_set(struct device *dev,
> ret = -EINVAL;
> }
>
> - mutex_unlock(&indio_dev->mlock);
> + iio_device_release_direct_mode(indio_dev);
>
> return ret ? ret : len;
> }
> @@ -555,11 +551,9 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
> int ret, i;
> unsigned int tmp;
>
> - mutex_lock(&indio_dev->mlock);
> - if (iio_buffer_enabled(indio_dev)) {
> - mutex_unlock(&indio_dev->mlock);
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> return -EBUSY;
And here.
> - }
>
> switch (mask) {
> case IIO_CHAN_INFO_SCALE:
> @@ -582,7 +576,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
> ret = -EINVAL;
> }
>
> - mutex_unlock(&indio_dev->mlock);
> + iio_device_release_direct_mode(indio_dev);
>
> return ret;
> }
>
On 12/03/16 11:21, Jonathan Cameron wrote:
> On 09/03/16 19:30, Alison Schofield wrote:
>> Replace the code that guarantees the device stays in direct mode with
>> iio_device_{claim|release}_direct_mode() which does same.
>>
>> Signed-off-by: Alison Schofield <[email protected]>
> A small improvement inline - don't eat the errors within the
> driver.
As it is trivial - I've fixed it up as suggested and applied.
Seemed silly to go round again for such a small change.
I'm hoping you have the opportunity to take this tree wide.
Thanks,
Jonathan
>
> Jonathan
>> ---
>> drivers/staging/iio/adc/ad7192.c | 24 +++++++++---------------
>> 1 file changed, 9 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
>> index f843f19..94a2e06 100644
>> --- a/drivers/staging/iio/adc/ad7192.c
>> +++ b/drivers/staging/iio/adc/ad7192.c
>> @@ -349,11 +349,9 @@ static ssize_t ad7192_write_frequency(struct device *dev,
>> if (lval == 0)
>> return -EINVAL;
>>
>> - mutex_lock(&indio_dev->mlock);
>> - if (iio_buffer_enabled(indio_dev)) {
>> - mutex_unlock(&indio_dev->mlock);
>> + ret = iio_device_claim_direct_mode(indio_dev);
>> + if (ret)
>> return -EBUSY;
> return ret; In theory we might be returning some other
> error code so best not to 'eat' the result.
>> - }
>>
>> div = st->mclk / (lval * st->f_order * 1024);
>> if (div < 1 || div > 1023) {
>> @@ -366,7 +364,7 @@ static ssize_t ad7192_write_frequency(struct device *dev,
>> ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
>>
>> out:
>> - mutex_unlock(&indio_dev->mlock);
>> + iio_device_release_direct_mode(indio_dev);
>>
>> return ret ? ret : len;
>> }
>> @@ -434,11 +432,9 @@ static ssize_t ad7192_set(struct device *dev,
>> if (ret < 0)
>> return ret;
>>
>> - mutex_lock(&indio_dev->mlock);
>> - if (iio_buffer_enabled(indio_dev)) {
>> - mutex_unlock(&indio_dev->mlock);
>> + ret = iio_device_claim_direct_mode(indio_dev);
>> + if (ret)
>> return -EBUSY;
> Same here.
>> - }
>>
>> switch ((u32)this_attr->address) {
>> case AD7192_REG_GPOCON:
>> @@ -461,7 +457,7 @@ static ssize_t ad7192_set(struct device *dev,
>> ret = -EINVAL;
>> }
>>
>> - mutex_unlock(&indio_dev->mlock);
>> + iio_device_release_direct_mode(indio_dev);
>>
>> return ret ? ret : len;
>> }
>> @@ -555,11 +551,9 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
>> int ret, i;
>> unsigned int tmp;
>>
>> - mutex_lock(&indio_dev->mlock);
>> - if (iio_buffer_enabled(indio_dev)) {
>> - mutex_unlock(&indio_dev->mlock);
>> + ret = iio_device_claim_direct_mode(indio_dev);
>> + if (ret)
>> return -EBUSY;
> And here.
>> - }
>>
>> switch (mask) {
>> case IIO_CHAN_INFO_SCALE:
>> @@ -582,7 +576,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
>> ret = -EINVAL;
>> }
>>
>> - mutex_unlock(&indio_dev->mlock);
>> + iio_device_release_direct_mode(indio_dev);
>>
>> return ret;
>> }
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>