Messages in this thread | | | Date | Fri, 28 Feb 2014 17:18:03 +0000 | From | Ian Abbott <> | Subject | Re: [PATCH] Staging: comedi: add timeouts to while loops in s626.c |
| |
On 2014-02-28 07:35, Chase Southwood wrote: > Smatch located a handful of while loops testing readl calls in s626.c. > Since these while loops depend on readl succeeding, it's safer to make > sure they time out eventually. > > Signed-off-by: Chase Southwood <chase.southwood@yahoo.com> > --- > Ian and/or Hartley, I'd love your comments on this. It seems to me that > we want these kinds of while loops properly timed out, but I want to make > sure I'm doing everything properly. First off, s626_debi_transfer() says > directly that it is called from within critical sections, so I assume > that means that the new comedi_timeout() function is no good here, and > s626_send_dac() looked equally suspicious, so I opted for iterative > timeouts. Is this correct? Also, for these timeouts, I used a very > conservative 10000 iterations, would it be better to decrease that?
Well 10000 iterations is an improvement on infinity! If the hardware is working, you'd expect it to go round a lot fewer iterations than that, but if the hardware is broken all bets are off, especially if it is generating interrupts.
> Also, do my error strings appear acceptable?
Mostly. There's a type in one of the strings that says "TLS" instead of "TSL".
> And finally, are timeouts here even necessary or helpful, or are there > any better ways to do it?
In the case of s626_send_dac(), it doesn't seem to be used in any critical sections, so it could make use of Hartley's comedi_timeout().
Some of the timeout errors could be propagated, especially for s626_send_dac() which is only reachable from very few paths.
There are other infinite loops involving calls to the s626_mc_test() function, but those could be dealt with by other patches.
-- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
| |