Re: [PATCH 4/8] iio: __iio_update_buffers: Update mode before preenable/after postdisable

[Jonathan Cameron <jic23@kernel.org>]

On 17/05/15 12:38, Lars-Peter Clausen wrote:
> On 05/17/2015 11:10 AM, Jonathan Cameron wrote:
>> On 13/05/15 15:04, Lars-Peter Clausen wrote:
>>> It is clear that we transition to INDIO_DIRECT_MODE when disabling the
>>> buffer(s) and it is also clear that we transition from INDIO_DIRECT_MODE
>>> when enabling the buffer(s). So leaving the currentmode field
>>> INDIO_DIRECT_MODE until after the preenable() callback and updating it to
>>> INDIO_DIRECT_MODE before the postdisable() callback doesn't add additional
>>> value. On the other hand some drivers will need to perform different
>>> actions depending on which mode the device is going to operate in/was
>>> operating in.
>>>
>>> Moving the update of currentmode before preenable() and after postdisable()
>>> enables us to have drivers which perform mode dependent actions in those
>>> callbacks.
>> I'm not sure that the argument that drivers might do something that requires
>> knowledge of this state in preenable or post disable is terribly relevant
>> (as inherently the driver always knows what is going on in these
>> callbacks!).
> 
> I have a driver which supports multiple different modes. And the
> selected mode depends on which type of buffer is attached. Depending
> on which mode got selected the driver has to do slightly different
> things in the callbacks.
> 
> But yea, this patch is a bit out of place in this series. I initially
> moved it up here since it made some of the other refactoring a bit
> easier. But I think that refactoring has been refactored so this is
> patch is no longer necessary. I'll drop it for now and resend it
> later in a context where it makes more sense.
>>
>> However, more interesting is the race conditions with drivers that cannot do
>> direct reads when in buffered mode. There are presumably still race conditions
>> in this region whatever we do here so it's up to the drivers to take appropriate
>> locks - I suspect many do not and hence might generate garbage readings if
>> a sysfs read is going on during the transition...
>>
>> I suppose adding additional states to say we are transitioning modes might be
>> useful.. Worth the bother?
> 
> We do hold the device lock for the whole duration of
> __iio_update_buffers(). So things should be ok as long as the driver
> holds the same lock when doing a direct conversion. If the driver
> does not then even adding additional states will not help.
> Unfortunately most drivers currently do something like
> 
>     if (iio_buffer_enabled(indio_dev))
>         return -EBUSY;
>     do_direct_conversion(...)
Exactly what I meant (my fault as I did it first, but that was a long time ago!)

> 
> So this is clearly broken at the moment. For one preenable() can
> already have been called and iio_buffer_enabled() still returns
> false. So that's something you could avoid having extra states. But
> there is nothing that prevents buffered mode from getting enabled
> right after the iio_buffer_enabled() check or even in the middle of
> do_direct_conversion(). So additional modes won't helper there only
> proper locking will do.> 
Agreed.
> A few other drivers do something like:
> 
>     mutex_lock(&indio_dev->mlock);
>     if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
>         ret = -EBUSY;
>     } else {
>         ret = do_direct_conversion(...);
>     }
>     mutex_unlock(&indio_dev->mlock);
> 
> This should be safe. Although we might want to add some helpers in
> the core that makes this a bit more straight forward and a bit more
> semantically expressive, maybe something like.
> 
>     if (!iio_device_claim_direct_mode(indio_dev))
>         return -EBUSY;
>     do_direct_conversion(...)
>     iio_device_release_direct_mode(indio_dev);
> 
> iio_device_claim_direct_mode() returns false if the device is in
> buffered mode. If it returns true it is guaranteed that the device
> stays in direct mode until iio_device_release_direct_mode() is
> called. This makes sure that the driver can do a direction conversion
> without having to bother that the device might suddenly switch to
> buffered mode.
Makes sense.
> 
> And while we should fix that, this is not necessarily part of the
> scope of this series.
Agreed.
> 
> - Lars
>