RE: [PATCH 01/48] staging: comedi: introduce comedi_timeout()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux