Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1350955 > unrolled thread

Re: [RFC PATCH 1/2] iio: core: implement iio_{claim|release}_direct_mode()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH 1/2] iio: core: implement  iio_{claim|release}_direct_mode() Jonathan Cameron <jic23@kernel.org> - 2016-03-05 19:10 +0100
    Re: [RFC PATCH 1/2] iio: core: implement  iio_{claim|release}_direct_mode() Alison Schofield <amsfield22@gmail.com> - 2016-03-09 21:10 +0100
      Re: [RFC PATCH 1/2] iio: core: implement  iio_{claim|release}_direct_mode() Jonathan Cameron <jic23@kernel.org> - 2016-03-09 21:30 +0100

#1350955 — Re: [RFC PATCH 1/2] iio: core: implement iio_{claim|release}_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 <amsfield22@gmail.com>
> 
> 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);
>>

[toc] | [next] | [standalone]

#1354415

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 <amsfield22@gmail.com>
> > 
> > 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

[toc] | [prev] | [next] | [standalone]

#1354425

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 <amsfield22@gmail.com>
>>>
>>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web