Q: comedi timeouts

[Hartley Sweeten]

On Wednesday, February 05, 2014 10:40 AM, Greg KH wrote:
> On Tue, Feb 04, 2014 at 06:59:43PM +0000, Hartley Sweeten wrote:
>> I'm in the process of cleaning up all the "timeout" code in the
>> comedi drivers.  The general form of the timeout code in all
>> the drivers is:
>> 
>> static int foo_eoc(struct comedi_device *dev, unsigned long timeout)
>> {
>> 	unsigned int status;
>> 	while (timeout--) {
>> 		status = inb(dev->iobase + STATUS_REG);
>> 		if (status & EOC)
>> 			return 0;
>> 		udelay(1);
>> 	}
>> 	return -ETIMEDOUT;
>> }
>> 
>> My question is about the 'udelay()'. Is this the preferred way to
>> delay between each check of the status register of would
>> something like a 'cpu_relax()' be better?
>
> udelay is fine, if you want really "tiny" sleep calls.  What about
> msleep() to really delay?
>
> And the whole "this is the number of loops" is just crazy, how about
> moving everything to use a real time value to timeout with instead?
> Then use the proper jiffies comparison functions so it's obvious as to
> what is being done here.

I agree that the "number of loops" stuff is nuts.

Some of the timeout loops are used in interrupt contexts. Can jiffies
comparisons be done then?

>> Also, the 'timeout' values used in the comedi drivers vary a lot.
>> I have seen value of 3, 5, 10, 100, 1000, 10000, 40000, and 100000.
>> Usually the smaller values have the udelay(1) in the loop, the
>> larger values don't. Assuming cpu_relax() would be a better way
>> to handle the delays, would it be a good idea to standardize the
>> timeout wait to a set number of loops?
>
> Not number of loops, but real time values, that's easier to audit for
> and understand.

I agree. I'm just trying to figure out the "best" way to handle this.

I think a 1 second timeout should be more than enough time for any
analog input/output conversion to complete. If would also be nice
if the comedi drivers released the cpu for other processes while waiting
for the end-of-conversion. Based on that I was thinking about adding
a comedi core helper like this:

int comedi_eoc_timeout(struct comedi_device *dev,
		       struct comedi_subdevice *s,
		       struct comedi_insn *insn,
		       int (*cb)(struct comedi_device *dev,
				 struct comedi_subdevice *s,
				 struct comedi_insn *insn))
{
	unsigned long timeout = jiffies + msec_to_jiffies(1000);
	int ret;

	while (time_before(jiffies, timeout)) {
		ret = cb(dev, s, insn);
		if (ret == 0)
			return 0;
		if (ret != -EBUSY)
			return ret;
		cpu_relax();	/* or udelay(1)? */
	}
	return -ETIMEDOUT;
}

Most of the drivers just need the 'dev' parameter to do the
checking but some need the 's' parameter to determine the
proper address to read. Some also need the channel that is
packed in the 'insn' to determine the bit to check.

The drivers would then just provide the callback that actually checks
for the end-of-conversion:

static int foo_eoc(struct comedi_device *dev,
		   struct comedi_subdevice *s,
		   struct comedi_insn *insn)
{
	unsigned int status;

	status = inl(dev->iobase FOO_STATUS_REG);
	if (status & FOO_EOC_BIT)
		return 0;
	/*
	 * Any other driver specific tests.
	* All non 0 or -EBUSY errno's are propagated.
	 */
	return -EBUSY;
}

Then use core helper to wait for the end-of-conversion:

	ret = comedi_eoc_timeout(dev, s, insn, fooc_eoc);
	if (ret)
		return ret;

This seems like it would cover all the comedi drivers. I also
gets all the timeout stuff into a common place.

Thanks,
Hartley