Re: [PATCH] Staging: comedi: add timeouts to while loops in s626.c

[Ian Abbott <abbotti@mev.co.uk>]

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         )=-