Q: comedi timeouts

Greg KH greg at kroah.com
Wed Feb 5 18:42:14 UTC 2014


On Wed, Feb 05, 2014 at 06:02:04PM +0000, Hartley Sweeten wrote:
> 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?

Yes.

But if you are in interrupt context, you can't call cpu_relax().

> >> 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;
> }

Given this is such a common "pattern" in driver development, shouldn't
there be something like it in the driver core perhaps?

> 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.

Putting it in a common place would be great to have, if the driver core
can't handle it, then in the comedi core at the least sounds great.

But again, watch out for sleeping while in interrupt context, that's not
good.

greg k-h


More information about the devel mailing list