On Friday, February 07, 2014 7:52 AM, Ian Abbott wrote:
> On 2014-02-06 23:48, H Hartley Sweeten wrote:
>> Introduce a comedi core helper function to handle the boilerplate
>> needed by the drivers to wait for a condition to occur. Typically
>> this condition is the analog input/output end-of-conversion used
>> with the comedi (*insn_read) and (*insn_write) operations.
>>
>> To use this function, the drivers just need to provide a callback
>> that checks for the desired condition. The callback should return
>> 0 if the condition is met or -EBUSY if it is still waiting. Any
>> other errno will be returned to the caller. If the timeout occurs
>> before the condition is met -ETIMEDOUT will be returned.
>>
>> The parameters to the callback function are the comedi_device,
>> comedi_subdevice, and comedi_insn pointers that were passed to the
>> (*insn_read) or (*insn_write) as well as an unsigned long, driver
>> specific, 'context' that can be used to pass any other information
>> that might be needed in the callback. This 'context' could be anything
>> such as the register offset to read the status or the bits needed
>> to check the status. The comedi_timeout() function itself does not
>> use any of these parameters.
>>
>> This will help remove all the crazy "wait this many loops" used by
>> some of the drivers. It also creates a common errno for comedi to
>> detect when a timeout occurs.
>>
>> ADC/DAC conversion times are typically pretty fast. A conservative
>> timeout of 1 second (HZ) is used in comedi_timeout().
>
> Perhaps it could be made tunable? Or at least the 1 second
> comedi_timeout(blah, blah, blah) could call comedi_timeout_ms(blah,
> blah, blah, 1000) or whatever.
I think that's a bit overkill. ADC/DAC conversion times are typically just
a couple usecs, at worst msecs. The 1 second timeout should be more
than enough for anything to finish. The timeout could probably be reduced
to something like 500ms and still be safe.
> More comments below....
<snip>
>> + unsigned long timeout = jiffies + msecs_to_jiffies(HZ);
>
> HZ is the number of jiffies in one second, so that should be:
>
> unsigned long timeout = jiffies + HZ;
>
> or:
>
> unsigned long timeout = jiffies + msecs_to_jiffies(1000);
>
> (If the time-out is to be made tunable in milliseconds, the second form
> would be preferred.)
Thanks for pointing that out.
To make the timeout _somewhat_ tunable how about defining this
in comedidev.h:
#define COMEDI_TIMEOUT 1000 /* msecs */
Then using the second form above:
unsigned long timeout = jiffies + msecs_to_jiffies(COMEDI_TIMEOUT);
>> + int ret;
>> +
>> + while (time_before(jiffies, timeout)) {
>> + ret = cb(dev, s, insn, context);
>> + if (ret == 0)
>> + return 0;
>
> That test is redundant as 0 != -EBUSY below.
I realize that. But it makes the "success" return more apparent.
>> + if (ret != -EBUSY)
>> + return ret;
And the propagation of non EBUSY errno's.
>> + udelay(1);
>
> It could just do a cpu_relax() instead of udelay(1).
Based on Greg's comments I thought this could also be used in
interrupt contexts but we could not use cpu_relax() then.
Based on your comments, this might not be true. In that case
I agree that cpu_relax() would probably be better.
>> + }
>> + return -ETIMEDOUT;
>> +}
>
> If the function is pre-empted for a long while after calling cb(), the
> time_before() test could fail without giving the cb() a final chance to
> return 0.
>
> How about:
>
> int ret;
>
> for (;;) {
> bool going = time_before(jiffies, timeout);
>
> ret = cb(dev, s, insn, context);
> if (ret != -EBUSY)
> return ret;
> if (!going)
> return -ETIMEDOUT;
> cpu_relax();
> }
>
> (N.B. #include <asm/processor.h> for cpu_relax().)
Personally I dislike for(;;) loops. Usually they can lead to deadlock
conditions.
The 1 second timeout should be safe even if we are pre-empted. But,
of course, this would need to be tested on a heavily loaded system with
a board that has slower conversion times.
I'll try to gather the data on what the conversion times actually are for
some of the boards. I think I have pulled most of the datasheets for the
drivers currently in comedi.
Regards,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
