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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
