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

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

[Chase Southwood]

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?
Also, do my error strings appear acceptable?

And finally, are timeouts here even necessary or helpful, or are there
any better ways to do it?

Thanks,
Chase

 drivers/staging/comedi/drivers/s626.c | 49 ++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 5ba4b4a..282636b 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -209,6 +209,8 @@ static const struct comedi_lrange s626_range_table = {
 static void s626_debi_transfer(struct comedi_device *dev)
 {
 	struct s626_private *devpriv = dev->private;
+	static const int timeout = 10000;
+	int i;
 
 	/* Initiate upload of shadow RAM to DEBI control register */
 	s626_mc_enable(dev, S626_MC2_UPLD_DEBI, S626_P_MC2);
@@ -221,8 +223,13 @@ static void s626_debi_transfer(struct comedi_device *dev)
 		;
 
 	/* Wait until DEBI transfer is done */
-	while (readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S)
-		;
+	for (i = 0; i < timeout; i++) {
+		if (!(readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S))
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "DEBI transfer timeout.");
 }
 
 /*
@@ -359,6 +366,8 @@ static const uint8_t s626_trimadrs[] = {
 static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 {
 	struct s626_private *devpriv = dev->private;
+	static const int timeout = 10000;
+	int i;
 
 	/* START THE SERIAL CLOCK RUNNING ------------- */
 
@@ -404,8 +413,13 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * Done by polling the DMAC enable flag; this flag is automatically
 	 * cleared when the transfer has finished.
 	 */
-	while (readl(devpriv->mmio + S626_P_MC1) & S626_MC1_A2OUT)
-		;
+	for  (i = 0; i < timeout; i++) {
+		if (!(readl(devpriv->mmio + S626_P_MC1) & S626_MC1_A2OUT))
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "DMA transfer timeout.");
 
 	/* START THE OUTPUT STREAM TO THE TARGET DAC -------------------- */
 
@@ -425,8 +439,13 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * finished transferring the DAC's data DWORD from the output FIFO
 	 * to the output buffer register.
 	 */
-	while (!(readl(devpriv->mmio + S626_P_SSR) & S626_SSR_AF2_OUT))
-		;
+	for (i = 0; i < timeout; i++) {
+		if (readl(devpriv->mmio + S626_P_SSR) & S626_SSR_AF2_OUT)
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "TSL timeout waiting for slot 1 to execute.");
 
 	/*
 	 * Set up to trap execution at slot 0 when the TSL sequencer cycles
@@ -466,8 +485,13 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 		 * from 0xFF to 0x00, which slot 0 causes to happen by shifting
 		 * out/in on SD2 the 0x00 that is always referenced by slot 5.
 		 */
-		while (readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000)
-			;
+		for (i = 0; i < timeout; i++) {
+			if (!(readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000))
+				break;
+			udelay(1);
+		}
+		if (i == timeout)
+			comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
 	}
 	/*
 	 * Either (1) we were too late setting the slot 0 trap; the TSL
@@ -486,8 +510,13 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * the next DAC write.  This is detected when FB_BUFFER2 MSB changes
 	 * from 0x00 to 0xFF.
 	 */
-	while (!(readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000))
-		;
+	for (i = 0; i < timeout; i++) {
+		if (readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000)
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "TLS timeout waiting for slot 0 to execute.");
 }
 
 /*
-- 
1.8.5.3

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

[Ian Abbott]

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

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

[Chase Southwood]

>On Friday, February 28, 2014 11:26 AM, Ian Abbott <abbotti@mev.co.uk> wrote:

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

Great, thanks!  I suppose I'll leave that number there then.

>
>> Also, do my error strings appear acceptable?
>
>Mostly.  There's a type in one of the strings that says "TLS" instead of 
>"TSL".
>

*Sigh* I promise I can type sometimes :P I'll get this corrected.

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

Awesome, I'll swap all of my timeouts out for comedi_timeout() in s626_send_dac().
As for propagating the timeout errors, could you please clarify that a bit further?  Both of the functions
which I add timeouts inside of in this patch return void, and so in their current state they cannot return any error
values.  Would you like them (or at least s626_send_dac()) to instead return an error upon timeout/or success on success,
or am I just totally misunderstanding your meaning of propagate here?

>
>There are other infinite loops involving calls to the s626_mc_test() 
>function, but those could be dealt with by other patches.
>

Yeah, I saw those...I'll whip up a patch for them, just wanted to verify that everything looks pretty good here
before I started on that.  I'll have that right out!

Thanks,
Chase
>
>-- 
>-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
>-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=- 

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

[Chase Southwood]

>On Friday, February 28, 2014 11:49 PM, Chase Southwood <chase.southwood@yahoo.com> wrote:

>>On Friday, February 28, 2014 11:26 AM, Ian Abbott <abbotti@mev.co.uk> wrote:
>>>On 2014-02-28 07:35, Chase Southwood wrote:

[snip]

>>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.
>
>
>Awesome, I'll swap all of my timeouts out for comedi_timeout() in s626_send_dac().

Actually, after taking another look at this, I don't think that using comedi_timeout()
here is going to work, actually.
The context from which s626_send_dac() is called allows sleep all right, but readl() isn't
a comedi function and therefore it doesn't behave (in parameters or return values) as
the callback function parameter to comedi_timeout() requires.  So unless I'm missing
something particularly large here, I believe we'll have to do the timeouts here manually
as well.  Am I correct here, and if so, would you like the iteration based timeouts here
as well, or a sleep-based timeout similar to that employed by comedi_timeout()?


Thanks,
Chase

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

[Ian Abbott]

On 2014-03-01 05:48, Chase Southwood wrote:
> On Friday, February 28, 2014 11:26 AM, Ian Abbott <abbotti@mev.co.uk> wrote:
>> On 2014-02-28 07:35, Chase Southwood wrote:
>>> 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.
>>
>
> Awesome, I'll swap all of my timeouts out for comedi_timeout() in s626_send_dac().
> As for propagating the timeout errors, could you please clarify that a bit further?  Both of the functions
> which I add timeouts inside of in this patch return void, and so in their current state they cannot return any error
> values.  Would you like them (or at least s626_send_dac()) to instead return an error upon timeout/or success on success,
> or am I just totally misunderstanding your meaning of propagate here?

s626_send_dac() could be changed to return an int value 0 on success or 
-ETIMEDOUT on timeout.  s626_set_dac() and s626_write_trim_dac() could 
be changed to return an int - just return the result of s626_send_dac(). 
  Similarly, the result of those functions could be either propagated 
upwards.  This could all be done in a separate patch (or patches).

>> There are other infinite loops involving calls to the s626_mc_test()
>> function, but those could be dealt with by other patches.
>
> Yeah, I saw those...I'll whip up a patch for them, just wanted to verify that everything looks pretty good here
> before I started on that.  I'll have that right out!

Yes, anything is an improvement on an infinite loop waiting for the 
hardware to do something!

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

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

[Ian Abbott]

On 2014-03-02 04:13, Chase Southwood wrote:
>> On Friday, February 28, 2014 11:49 PM, Chase Southwood <chase.southwood@yahoo.com> wrote:
>
>>> On Friday, February 28, 2014 11:26 AM, Ian Abbott <abbotti@mev.co.uk> wrote:
>>>> On 2014-02-28 07:35, Chase Southwood wrote:
>
> [snip]
>
>>> 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.
>>
>>
>> Awesome, I'll swap all of my timeouts out for comedi_timeout() in s626_send_dac().
>
> Actually, after taking another look at this, I don't think that using comedi_timeout()
> here is going to work, actually.
> The context from which s626_send_dac() is called allows sleep all right, but readl() isn't
> a comedi function and therefore it doesn't behave (in parameters or return values) as
> the callback function parameter to comedi_timeout() requires.  So unless I'm missing
> something particularly large here, I believe we'll have to do the timeouts here manually
> as well.  Am I correct here, and if so, would you like the iteration based timeouts here
> as well, or a sleep-based timeout similar to that employed by comedi_timeout()?

The readl() could be done in a small callback function.  As the 
different while loops are checking for different results from readl(), 
It would need a different callback functions for each case, or some 
creative use of the callback function's 'context' parameter.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

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

[Chase Southwood]

>On Monday, March 3, 2014 8:13 AM, Ian Abbott <abbotti@mev.co.uk> wrote:

>>On 2014-03-02 04:13, Chase Southwood wrote:
>>>On Friday, February 28, 2014 11:49 PM, Chase Southwood <chase.southwood@yahoo.com> wrote:
>>>> On Friday, February 28, 2014 11:26 AM, Ian Abbott <abbotti@mev.co.uk> wrote:
>>>>> On 2014-02-28 07:35, Chase Southwood wrote:
>>
>> [snip]
>>
>>>> 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.
>>>
>>>
>>> Awesome, I'll swap all of my timeouts out for comedi_timeout() in s626_send_dac().
>>
>> Actually, after taking another look at this, I don't think that using comedi_timeout()
>> here is going to work, actually.
>> The context from which s626_send_dac() is called allows sleep all right, but readl() isn't
>> a comedi function and therefore it doesn't behave (in parameters or return values) as
>> the callback function parameter to comedi_timeout() requires.  So unless I'm missing
>> something particularly large here, I believe we'll have to do the timeouts here manually
>> as well.  Am I correct here, and if so, would you like the iteration based timeouts here
>> as well, or a sleep-based timeout similar to that employed by comedi_timeout()?
>
>The readl() could be done in a small callback function.  As the 
>different while loops are checking for different results from readl(), 
>It would need a different callback functions for each case, or some 
>creative use of the callback function's 'context' parameter.
>

Oh, of course.  I'll see if there's a reasonably clear way that I can use the
context parameter to use a single callback, if not I'll just define a few of them.

I'll send a patch with the new callback(s), and the timeouts in s626_send_dac()
switched to comedi_timeout(), and I'll add the timeouts that were left out last
time, and then another patch or patchset propagating everything upwards.

Thanks,
Chase

>
>-- 
>-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
>-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-
>--

[PATCH v2 1/2] Staging: comedi: convert while loops to timeouts in s626.c

[Chase Southwood]

This patch changes a handful of while loops to timeouts to prevent
infinite looping on hardware failure.  A couple such loops are in a
function (s626_debi_transfer()) which is called from critical sections,
so comedi_timeout() is unusable for them, and an iterative timeout is
used instead.  For the while loops in a context where comedi_timeout() is
allowed, a new callback function, s626_send_dac_eoc(), has been defined
to evaluate the conditions that the while loops are testing.  The new
callback employs a switch statement based on the register offset values
that the status is to be checked from, as this information is enough to
recreate the entire readl() call, and is passed in the 'context'
parameter.  A couple of additional macros have been defined where this
method is not completely sufficient.  The proper comedi_timeout() calls
are then used.

Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---
Ian, my idea for the callback function here is a bit iffy, I'd love to
hear your feedback.  I've created the ret variable because patch 2/2
propogates all errors upwards.

2: Now using comedi_timeout() where allowable.

 drivers/staging/comedi/drivers/s626.c | 81 +++++++++++++++++++++++++++++------
 1 file changed, 69 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 5ba4b4a..5a7e1c0 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -209,6 +209,8 @@ static const struct comedi_lrange s626_range_table = {
 static void s626_debi_transfer(struct comedi_device *dev)
 {
 	struct s626_private *devpriv = dev->private;
+	static const int timeout = 10000;
+	int i;
 
 	/* Initiate upload of shadow RAM to DEBI control register */
 	s626_mc_enable(dev, S626_MC2_UPLD_DEBI, S626_P_MC2);
@@ -217,12 +219,22 @@ static void s626_debi_transfer(struct comedi_device *dev)
 	 * Wait for completion of upload from shadow RAM to
 	 * DEBI control register.
 	 */
-	while (!s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2))
-		;
+	for (i = 0; i < timeout; i++) {
+		if (s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2))
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "Timeout while uploading to DEBI control register.");
 
 	/* Wait until DEBI transfer is done */
-	while (readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S)
-		;
+	for (i = 0; i < timeout; i++) {
+		if (!(readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S))
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "DEBI transfer timeout.");
 }
 
 /*
@@ -351,6 +363,44 @@ static const uint8_t s626_trimadrs[] = {
 	0x40, 0x41, 0x42, 0x50, 0x51, 0x52, 0x53, 0x60, 0x61, 0x62, 0x63
 };
 
+#define S626_P_FB_BUFFER2_MSB_00	0
+#define S626_P_FB_BUFFER2_MSB_FF	1
+
+static int s626_send_dac_eoc(struct comedi_device *dev,
+							struct comedi_subdevice *s,
+							struct comedi_insn *insn,
+							unsigned long context)
+{
+	struct s626_private *devpriv = dev->private;
+	unsigned int status;
+
+	switch (context) {
+	case S626_P_MC1:
+		status = readl(devpriv->mmio + S626_P_MC1);
+		if (!(status & S626_MC1_A2OUT))
+			return 0;
+		break;
+	case S626_P_SSR:
+		status = readl(devpriv->mmio + S626_P_SSR);
+		if (status & S626_SSR_AF2_OUT)
+			return 0;
+		break;
+	case S626_P_FB_BUFFER2_MSB_00:
+		status = readl(devpriv->mmio + S626_P_FB_BUFFER2);
+		if (!(status & 0xff000000))
+			return 0;
+		break;
+	case S626_P_FB_BUFFER2_MSB_FF:
+		status = readl(devpriv->mmio + S626_P_FB_BUFFER2);
+		if (status & 0xff000000)
+			return 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return -EBUSY;
+}
+
 /*
  * Private helper function: Transmit serial data to DAC via Audio
  * channel 2.  Assumes: (1) TSL2 slot records initialized, and (2)
@@ -359,6 +409,7 @@ static const uint8_t s626_trimadrs[] = {
 static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 {
 	struct s626_private *devpriv = dev->private;
+	int ret;
 
 	/* START THE SERIAL CLOCK RUNNING ------------- */
 
@@ -404,8 +455,9 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * Done by polling the DMAC enable flag; this flag is automatically
 	 * cleared when the transfer has finished.
 	 */
-	while (readl(devpriv->mmio + S626_P_MC1) & S626_MC1_A2OUT)
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc, S626_P_MC1);
+	if (ret)
+		comedi_error(dev, "DMA transfer timeout.");
 
 	/* START THE OUTPUT STREAM TO THE TARGET DAC -------------------- */
 
@@ -425,8 +477,9 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * finished transferring the DAC's data DWORD from the output FIFO
 	 * to the output buffer register.
 	 */
-	while (!(readl(devpriv->mmio + S626_P_SSR) & S626_SSR_AF2_OUT))
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc, S626_P_SSR);
+	if (ret)
+		comedi_error(dev, "TSL timeout waiting for slot 1 to execute.");
 
 	/*
 	 * Set up to trap execution at slot 0 when the TSL sequencer cycles
@@ -466,8 +519,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 		 * from 0xFF to 0x00, which slot 0 causes to happen by shifting
 		 * out/in on SD2 the 0x00 that is always referenced by slot 5.
 		 */
-		while (readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000)
-			;
+		ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
+							S626_P_FB_BUFFER2_MSB_00);
+		if (ret)
+			comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
 	}
 	/*
 	 * Either (1) we were too late setting the slot 0 trap; the TSL
@@ -486,8 +541,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * the next DAC write.  This is detected when FB_BUFFER2 MSB changes
 	 * from 0x00 to 0xFF.
 	 */
-	while (!(readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000))
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
+						S626_P_FB_BUFFER2_MSB_FF);
+	if (ret)
+		comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
 }
 
 /*
-- 
1.8.5.3

[PATCH v2 2/2] Staging: comedi: propagate timeout errors in s626.c

[Chase Southwood]

This patch for s626.c propagates the errors from the newly introduced
calls to comedi_timeout() as far as possible.

Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---
Compile tested only.

 drivers/staging/comedi/drivers/s626.c | 63 +++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 5a7e1c0..c6486b0 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -406,7 +406,7 @@ static int s626_send_dac_eoc(struct comedi_device *dev,
  * channel 2.  Assumes: (1) TSL2 slot records initialized, and (2)
  * dacpol contains valid target image.
  */
-static void s626_send_dac(struct comedi_device *dev, uint32_t val)
+static int s626_send_dac(struct comedi_device *dev, uint32_t val)
 {
 	struct s626_private *devpriv = dev->private;
 	int ret;
@@ -456,8 +456,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * cleared when the transfer has finished.
 	 */
 	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc, S626_P_MC1);
-	if (ret)
+	if (ret) {
 		comedi_error(dev, "DMA transfer timeout.");
+		return ret;
+	}
 
 	/* START THE OUTPUT STREAM TO THE TARGET DAC -------------------- */
 
@@ -478,8 +480,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * to the output buffer register.
 	 */
 	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc, S626_P_SSR);
-	if (ret)
+	if (ret) {
 		comedi_error(dev, "TSL timeout waiting for slot 1 to execute.");
+		return ret;
+	}
 
 	/*
 	 * Set up to trap execution at slot 0 when the TSL sequencer cycles
@@ -521,8 +525,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 		 */
 		ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
 							S626_P_FB_BUFFER2_MSB_00);
-		if (ret)
+		if (ret) {
 			comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
+			return ret;
+		}
 	}
 	/*
 	 * Either (1) we were too late setting the slot 0 trap; the TSL
@@ -543,14 +549,17 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 */
 	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
 						S626_P_FB_BUFFER2_MSB_FF);
-	if (ret)
+	if (ret) {
 		comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
+		return ret;
+	}
+	return 0;
 }
 
 /*
  * Private helper function: Write setpoint to an application DAC channel.
  */
-static void s626_set_dac(struct comedi_device *dev, uint16_t chan,
+static int s626_set_dac(struct comedi_device *dev, uint16_t chan,
 			 int16_t dacdata)
 {
 	struct s626_private *devpriv = dev->private;
@@ -613,10 +622,10 @@ static void s626_set_dac(struct comedi_device *dev, uint16_t chan,
 	val |= ((uint32_t)(chan & 1) << 15);	/* Address the DAC channel
 						 * within the device. */
 	val |= (uint32_t)dacdata;	/* Include DAC setpoint data. */
-	s626_send_dac(dev, val);
+	return s626_send_dac(dev, val);
 }
 
-static void s626_write_trim_dac(struct comedi_device *dev, uint8_t logical_chan,
+static int s626_write_trim_dac(struct comedi_device *dev, uint8_t logical_chan,
 				uint8_t dac_data)
 {
 	struct s626_private *devpriv = dev->private;
@@ -663,17 +672,22 @@ static void s626_write_trim_dac(struct comedi_device *dev, uint8_t logical_chan,
 	 * Address the DAC channel within the trimdac device.
 	 * Include DAC setpoint data.
 	 */
-	s626_send_dac(dev, (chan << 8) | dac_data);
+	return s626_send_dac(dev, (chan << 8) | dac_data);
 }
 
-static void s626_load_trim_dacs(struct comedi_device *dev)
+static int s626_load_trim_dacs(struct comedi_device *dev)
 {
 	uint8_t i;
+	int ret;
 
 	/* Copy TrimDac setpoint values from EEPROM to TrimDacs. */
-	for (i = 0; i < ARRAY_SIZE(s626_trimchan); i++)
-		s626_write_trim_dac(dev, i,
+	for (i = 0; i < ARRAY_SIZE(s626_trimchan); i++) {
+		ret = s626_write_trim_dac(dev, i,
 				    s626_i2c_read(dev, s626_trimadrs[i]));
+		if (ret)
+			return ret;
+	}
+	return 0;
 }
 
 /* ******  COUNTER FUNCTIONS  ******* */
@@ -2372,6 +2386,7 @@ static int s626_ao_winsn(struct comedi_device *dev, struct comedi_subdevice *s,
 {
 	struct s626_private *devpriv = dev->private;
 	int i;
+	int ret;
 	uint16_t chan = CR_CHAN(insn->chanspec);
 	int16_t dacdata;
 
@@ -2380,7 +2395,9 @@ static int s626_ao_winsn(struct comedi_device *dev, struct comedi_subdevice *s,
 		devpriv->ao_readback[CR_CHAN(insn->chanspec)] = data[i];
 		dacdata -= (0x1fff);
 
-		s626_set_dac(dev, chan, dacdata);
+		ret = s626_set_dac(dev, chan, dacdata);
+		if (ret)
+			return ret;
 	}
 
 	return i;
@@ -2616,12 +2633,13 @@ static int s626_allocate_dma_buffers(struct comedi_device *dev)
 	return 0;
 }
 
-static void s626_initialize(struct comedi_device *dev)
+static int s626_initialize(struct comedi_device *dev)
 {
 	struct s626_private *devpriv = dev->private;
 	dma_addr_t phys_buf;
 	uint16_t chan;
 	int i;
+	int ret;
 
 	/* Enable DEBI and audio pins, enable I2C interface */
 	s626_mc_enable(dev, S626_MC1_DEBI | S626_MC1_AUDIO | S626_MC1_I2C,
@@ -2822,7 +2840,9 @@ static void s626_initialize(struct comedi_device *dev)
 	 * sometimes causes the first few TrimDAC writes to malfunction.
 	 */
 	s626_load_trim_dacs(dev);
-	s626_load_trim_dacs(dev);
+	ret = s626_load_trim_dacs(dev);
+	if (ret)
+		return ret;
 
 	/*
 	 * Manually init all gate array hardware in case this is a soft
@@ -2836,8 +2856,11 @@ static void s626_initialize(struct comedi_device *dev)
 	 * Init all DAC outputs to 0V and init all DAC setpoint and
 	 * polarity images.
 	 */
-	for (chan = 0; chan < S626_DAC_CHANNELS; chan++)
-		s626_set_dac(dev, chan, 0);
+	for (chan = 0; chan < S626_DAC_CHANNELS; chan++) {
+		ret = s626_set_dac(dev, chan, 0);
+		if (ret)
+			return ret;
+	}
 
 	/* Init counters */
 	s626_counters_init(dev);
@@ -2853,6 +2876,8 @@ static void s626_initialize(struct comedi_device *dev)
 
 	/* Initialize the digital I/O subsystem */
 	s626_dio_init(dev);
+
+	return 0;
 }
 
 static int s626_auto_attach(struct comedi_device *dev,
@@ -2973,7 +2998,9 @@ static int s626_auto_attach(struct comedi_device *dev,
 	s->insn_read	= s626_enc_insn_read;
 	s->insn_write	= s626_enc_insn_write;
 
-	s626_initialize(dev);
+	ret = s626_initialize(dev);
+	if (ret)
+		return ret;
 
 	return 0;
 }
-- 
1.8.5.3

Re: [PATCH v2 1/2] Staging: comedi: convert while loops to timeouts in s626.c

[Ian Abbott]

On 2014-03-04 08:43, Chase Southwood wrote:
> This patch changes a handful of while loops to timeouts to prevent
> infinite looping on hardware failure.  A couple such loops are in a
> function (s626_debi_transfer()) which is called from critical sections,
> so comedi_timeout() is unusable for them, and an iterative timeout is
> used instead.  For the while loops in a context where comedi_timeout() is
> allowed, a new callback function, s626_send_dac_eoc(), has been defined
> to evaluate the conditions that the while loops are testing.  The new
> callback employs a switch statement based on the register offset values
> that the status is to be checked from, as this information is enough to
> recreate the entire readl() call, and is passed in the 'context'
> parameter.  A couple of additional macros have been defined where this
> method is not completely sufficient.  The proper comedi_timeout() calls
> are then used.
>
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
> Ian, my idea for the callback function here is a bit iffy, I'd love to
> hear your feedback.  I've created the ret variable because patch 2/2
> propogates all errors upwards.
>
> 2: Now using comedi_timeout() where allowable.
>
>   drivers/staging/comedi/drivers/s626.c | 81 +++++++++++++++++++++++++++++------
>   1 file changed, 69 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
> index 5ba4b4a..5a7e1c0 100644
> --- a/drivers/staging/comedi/drivers/s626.c
> +++ b/drivers/staging/comedi/drivers/s626.c
> @@ -209,6 +209,8 @@ static const struct comedi_lrange s626_range_table = {
>   static void s626_debi_transfer(struct comedi_device *dev)
>   {
>   	struct s626_private *devpriv = dev->private;
> +	static const int timeout = 10000;
> +	int i;
>
>   	/* Initiate upload of shadow RAM to DEBI control register */
>   	s626_mc_enable(dev, S626_MC2_UPLD_DEBI, S626_P_MC2);
> @@ -217,12 +219,22 @@ static void s626_debi_transfer(struct comedi_device *dev)
>   	 * Wait for completion of upload from shadow RAM to
>   	 * DEBI control register.
>   	 */
> -	while (!s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2))
> -		;
> +	for (i = 0; i < timeout; i++) {
> +		if (s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2))
> +			break;
> +		udelay(1);
> +	}
> +	if (i == timeout)
> +		comedi_error(dev, "Timeout while uploading to DEBI control register.");
>
>   	/* Wait until DEBI transfer is done */
> -	while (readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S)
> -		;
> +	for (i = 0; i < timeout; i++) {
> +		if (!(readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S))
> +			break;
> +		udelay(1);
> +	}
> +	if (i == timeout)
> +		comedi_error(dev, "DEBI transfer timeout.");
>   }
>
>   /*
> @@ -351,6 +363,44 @@ static const uint8_t s626_trimadrs[] = {
>   	0x40, 0x41, 0x42, 0x50, 0x51, 0x52, 0x53, 0x60, 0x61, 0x62, 0x63
>   };
>
> +#define S626_P_FB_BUFFER2_MSB_00	0
> +#define S626_P_FB_BUFFER2_MSB_FF	1
> +
> +static int s626_send_dac_eoc(struct comedi_device *dev,
> +							struct comedi_subdevice *s,
> +							struct comedi_insn *insn,
> +							unsigned long context)
> +{
> +	struct s626_private *devpriv = dev->private;
> +	unsigned int status;
> +
> +	switch (context) {
> +	case S626_P_MC1:
> +		status = readl(devpriv->mmio + S626_P_MC1);
> +		if (!(status & S626_MC1_A2OUT))
> +			return 0;
> +		break;
> +	case S626_P_SSR:
> +		status = readl(devpriv->mmio + S626_P_SSR);
> +		if (status & S626_SSR_AF2_OUT)
> +			return 0;
> +		break;
> +	case S626_P_FB_BUFFER2_MSB_00:
> +		status = readl(devpriv->mmio + S626_P_FB_BUFFER2);
> +		if (!(status & 0xff000000))
> +			return 0;
> +		break;
> +	case S626_P_FB_BUFFER2_MSB_FF:
> +		status = readl(devpriv->mmio + S626_P_FB_BUFFER2);
> +		if (status & 0xff000000)
> +			return 0;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return -EBUSY;
> +}
> +

Rather than switching on a value that is either a register offset or 
some special value, it might be better to use an enum to define the 
context values you are switching on, e.g.:

enum {
	s626_send_dac_wait_not_mc1_a2out,
	s626_send_dac_wait_ssr_af2_out,
	s626_send_dac_wait_fb_buffer2_msb_00,
	s626_send_dac_wait_fb_buffer2_msb_ff
};

Alternatively, different callback functions could be used for each case.

One slight oddity about the original code that waits for the MSB of FB 
BUFFER2 to go from 0x00 to 0xFF is that it actually waits for it to 
become non-zero.  I'm presuming the intermediate values between 0x00 and 
0xFF are never seen.  (There is a non-Comedi, GPL'ed, Linux driver 
available from Sensoray that makes the same assumption, but I think the 
Comedi driver was derived from it.)

>   /*
>    * Private helper function: Transmit serial data to DAC via Audio
>    * channel 2.  Assumes: (1) TSL2 slot records initialized, and (2)
> @@ -359,6 +409,7 @@ static const uint8_t s626_trimadrs[] = {
>   static void s626_send_dac(struct comedi_device *dev, uint32_t val)
>   {
>   	struct s626_private *devpriv = dev->private;
> +	int ret;
>
>   	/* START THE SERIAL CLOCK RUNNING ------------- */
>
> @@ -404,8 +455,9 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
>   	 * Done by polling the DMAC enable flag; this flag is automatically
>   	 * cleared when the transfer has finished.
>   	 */
> -	while (readl(devpriv->mmio + S626_P_MC1) & S626_MC1_A2OUT)
> -		;
> +	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc, S626_P_MC1);
> +	if (ret)
> +		comedi_error(dev, "DMA transfer timeout.");
>
>   	/* START THE OUTPUT STREAM TO THE TARGET DAC -------------------- */
>
> @@ -425,8 +477,9 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
>   	 * finished transferring the DAC's data DWORD from the output FIFO
>   	 * to the output buffer register.
>   	 */
> -	while (!(readl(devpriv->mmio + S626_P_SSR) & S626_SSR_AF2_OUT))
> -		;
> +	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc, S626_P_SSR);
> +	if (ret)
> +		comedi_error(dev, "TSL timeout waiting for slot 1 to execute.");
>
>   	/*
>   	 * Set up to trap execution at slot 0 when the TSL sequencer cycles
> @@ -466,8 +519,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
>   		 * from 0xFF to 0x00, which slot 0 causes to happen by shifting
>   		 * out/in on SD2 the 0x00 that is always referenced by slot 5.
>   		 */
> -		while (readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000)
> -			;
> +		ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
> +							S626_P_FB_BUFFER2_MSB_00);
> +		if (ret)
> +			comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
>   	}
>   	/*
>   	 * Either (1) we were too late setting the slot 0 trap; the TSL
> @@ -486,8 +541,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
>   	 * the next DAC write.  This is detected when FB_BUFFER2 MSB changes
>   	 * from 0x00 to 0xFF.
>   	 */
> -	while (!(readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000))
> -		;
> +	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
> +						S626_P_FB_BUFFER2_MSB_FF);
> +	if (ret)
> +		comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
>   }
>
>   /*
>


-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH v2 2/2] Staging: comedi: propagate timeout errors in s626.c

[Ian Abbott]

On 2014-03-04 08:44, Chase Southwood wrote:
> This patch for s626.c propagates the errors from the newly introduced
> calls to comedi_timeout() as far as possible.
>
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
> Compile tested only.

Looks good, but if you change patch 1 this one will need rebasing to it.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH v2 1/2] Staging: comedi: convert while loops to timeouts in s626.c

[Chase Southwood]

>On Wednesday, March 5, 2014 6:10 AM, Ian Abbott <abbotti@mev.co.uk> wrote:

>>On 2014-03-04 08:43, Chase Southwood wrote:
>>This patch changes a handful of while loops to timeouts to prevent
>>infinite looping on hardware failure.  A couple such loops are in a
>>function (s626_debi_transfer()) which is called from critical sections,
>>so comedi_timeout() is unusable for them, and an iterative timeout is
>>used instead.  For the while loops in a context where comedi_timeout() is
>>allowed, a new callback function, s626_send_dac_eoc(), has been defined
>>to evaluate the conditions that the while loops are testing.  The new
>>callback employs a switch statement based on the register offset values
>>that the status is to be checked from, as this information is enough to
>>recreate the entire readl() call, and is passed in the 'context'
>>parameter.  A couple of additional macros have been defined where this
>>method is not completely sufficient.  The proper comedi_timeout() calls
>>are then used.
>>
>>Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
>>---
>>Ian, my idea for the callback function here is a bit iffy, I'd love to
>>hear your feedback.  I've created the ret variable because patch 2/2
>>propogates all errors upwards.
>>
>>2: Now using comedi_timeout() where allowable.

[snip]

>Rather than switching on a value that is either a register offset or 
>some special value, it might be better to use an enum to define the 
>context values you are switching on, e.g.:
>
>enum {
>    s626_send_dac_wait_not_mc1_a2out,
>    s626_send_dac_wait_ssr_af2_out,
>    s626_send_dac_wait_fb_buffer2_msb_00,
>    s626_send_dac_wait_fb_buffer2_msb_ff
>};
>
>Alternatively, different callback functions could be used for each case.

Ian,
Ah yes...this is much smarter than what I was trying before.  I've made
up a patch using this enum, and tomorrow I will get PATCH 2/2 rebased
and get a v2 of this patchset sent out.

Thanks,
Chase

>One slight oddity about the original code that waits for the MSB of FB 
>BUFFER2 to go from 0x00 to 0xFF is that it actually waits for it to 
>become non-zero.  I'm presuming the intermediate values between 0x00 and 
>0xFF are never seen.  (There is a non-Comedi, GPL'ed, Linux driver 
>available from Sensoray that makes the same assumption, but I think the 
>Comedi driver was derived from it.)
>

[snip]
>-- 
>-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
>-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-
>-- 

[PATCH v3 1/2] Staging: comedi: convert while loops to timeouts in

[Chase Southwood]

This patch changes a handful of while loops to timeouts to prevent
infinite looping on hardware failure. A couple such loops are in a
function (s626_debi_transfer()) which is called from critical sections,
so comedi_timeout() is unusable for them, and an iterative timeout is
used instead. For the while loops in a context where comedi_timeout() is
allowed, a new callback function, s626_send_dac_eoc(), has been defined
to evaluate the conditions that the while loops are testing.  The new
callback employs a switch statement based on a simple new enum so that
it is usable for all of the different conditions tested in while loops
in s626_send_dac().  The proper comedi_timeout() calls are then used.

Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---
Ian, here is a version of this patchset employing the enum you recommended.
The second patch has been rebased on top of this one.

2: Used comedi_timeout() where appropriate, introduce callback function

3: Updated callback to switch on new enum.

 drivers/staging/comedi/drivers/s626.c | 87 ++++++++++++++++++++++++++++++-----
 1 file changed, 75 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 5ba4b4a..ff9dde1 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -209,6 +209,8 @@ static const struct comedi_lrange s626_range_table = {
 static void s626_debi_transfer(struct comedi_device *dev)
 {
 	struct s626_private *devpriv = dev->private;
+	static const int timeout = 10000;
+	int i;
 
 	/* Initiate upload of shadow RAM to DEBI control register */
 	s626_mc_enable(dev, S626_MC2_UPLD_DEBI, S626_P_MC2);
@@ -217,12 +219,22 @@ static void s626_debi_transfer(struct comedi_device *dev)
 	 * Wait for completion of upload from shadow RAM to
 	 * DEBI control register.
 	 */
-	while (!s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2))
-		;
+	for (i = 0; i < timeout; i++) {
+		if (s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2))
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "Timeout while uploading to DEBI control register.");
 
 	/* Wait until DEBI transfer is done */
-	while (readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S)
-		;
+	for (i = 0; i < timeout; i++) {
+		if (!(readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S))
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "DEBI transfer timeout.");
 }
 
 /*
@@ -351,6 +363,48 @@ static const uint8_t s626_trimadrs[] = {
 	0x40, 0x41, 0x42, 0x50, 0x51, 0x52, 0x53, 0x60, 0x61, 0x62, 0x63
 };
 
+enum {
+	s626_send_dac_wait_not_mc1_a2out,
+	s626_send_dac_wait_ssr_af2_out,
+	s626_send_dac_wait_fb_buffer2_msb_00,
+	s626_send_dac_wait_fb_buffer2_msb_ff
+};
+
+static int s626_send_dac_eoc(struct comedi_device *dev,
+			     struct comedi_subdevice *s,
+			     struct comedi_insn *insn,
+			     unsigned long context)
+{
+	struct s626_private *devpriv = dev->private;
+	unsigned int status;
+
+	switch (context) {
+	case s626_send_dac_wait_not_mc1_a2out:
+		status = readl(devpriv->mmio + S626_P_MC1);
+		if (!(status & S626_MC1_A2OUT))
+			return 0;
+		break;
+	case s626_send_dac_wait_ssr_af2_out:
+		status = readl(devpriv->mmio + S626_P_SSR);
+		if (status & S626_SSR_AF2_OUT)
+			return 0;
+		break;
+	case s626_send_dac_wait_fb_buffer2_msb_00:
+		status = readl(devpriv->mmio + S626_P_FB_BUFFER2);
+		if (!(status & 0xff000000))
+			return 0;
+		break;
+	case s626_send_dac_wait_fb_buffer2_msb_ff:
+		status = readl(devpriv->mmio + S626_P_FB_BUFFER2);
+		if (status & 0xff000000)
+			return 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return -EBUSY;
+}
+
 /*
  * Private helper function: Transmit serial data to DAC via Audio
  * channel 2.  Assumes: (1) TSL2 slot records initialized, and (2)
@@ -359,6 +413,7 @@ static const uint8_t s626_trimadrs[] = {
 static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 {
 	struct s626_private *devpriv = dev->private;
+	int ret;
 
 	/* START THE SERIAL CLOCK RUNNING ------------- */
 
@@ -404,8 +459,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * Done by polling the DMAC enable flag; this flag is automatically
 	 * cleared when the transfer has finished.
 	 */
-	while (readl(devpriv->mmio + S626_P_MC1) & S626_MC1_A2OUT)
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
+			     s626_send_dac_wait_not_mc1_a2out);
+	if (ret)
+		comedi_error(dev, "DMA transfer timeout.");
 
 	/* START THE OUTPUT STREAM TO THE TARGET DAC -------------------- */
 
@@ -425,8 +482,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * finished transferring the DAC's data DWORD from the output FIFO
 	 * to the output buffer register.
 	 */
-	while (!(readl(devpriv->mmio + S626_P_SSR) & S626_SSR_AF2_OUT))
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
+			     s626_send_dac_wait_ssr_af2_out);
+	if (ret)
+		comedi_error(dev, "TSL timeout waiting for slot 1 to execute.");
 
 	/*
 	 * Set up to trap execution at slot 0 when the TSL sequencer cycles
@@ -466,8 +525,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 		 * from 0xFF to 0x00, which slot 0 causes to happen by shifting
 		 * out/in on SD2 the 0x00 that is always referenced by slot 5.
 		 */
-		while (readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000)
-			;
+		ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
+				     s626_send_dac_wait_fb_buffer2_msb_00);
+		if (ret)
+			comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
 	}
 	/*
 	 * Either (1) we were too late setting the slot 0 trap; the TSL
@@ -486,8 +547,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * the next DAC write.  This is detected when FB_BUFFER2 MSB changes
 	 * from 0x00 to 0xFF.
 	 */
-	while (!(readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000))
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
+			     s626_send_dac_wait_fb_buffer2_msb_ff);
+	if (ret)
+		comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
 }
 
 /*
-- 
1.8.5.3

[PATCH v3 2/2] Staging: comedi: propagate timeout errors in s626.c

[Chase Southwood]

This patch for s626.c propagates the errors from the newly introduced
calls to comedi_timeout() as far as possible.

Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---
Compile tested.

2: This patch was introduced in v2 of this set.

3: Rebased on top of PATCH 1/2 edits.

 drivers/staging/comedi/drivers/s626.c | 63 +++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index fab8ccf..6518a4c 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -410,7 +410,7 @@ static int s626_send_dac_eoc(struct comedi_device *dev,
  * channel 2.  Assumes: (1) TSL2 slot records initialized, and (2)
  * dacpol contains valid target image.
  */
-static void s626_send_dac(struct comedi_device *dev, uint32_t val)
+static int s626_send_dac(struct comedi_device *dev, uint32_t val)
 {
 	struct s626_private *devpriv = dev->private;
 	int ret;
@@ -461,8 +461,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 */
 	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
 			     s626_send_dac_wait_not_mc1_a2out);
-	if (ret)
+	if (ret) {
 		comedi_error(dev, "DMA transfer timeout.");
+		return ret;
+	}
 
 	/* START THE OUTPUT STREAM TO THE TARGET DAC -------------------- */
 
@@ -484,8 +486,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 */
 	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
 			     s626_send_dac_wait_ssr_af2_out);
-	if (ret)
+	if (ret) {
 		comedi_error(dev, "TSL timeout waiting for slot 1 to execute.");
+		return ret;
+	}
 
 	/*
 	 * Set up to trap execution at slot 0 when the TSL sequencer cycles
@@ -527,8 +531,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 		 */
 		ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
 				     s626_send_dac_wait_fb_buffer2_msb_00);
-		if (ret)
+		if (ret) {
 			comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
+			return ret;
+		}
 	}
 	/*
 	 * Either (1) we were too late setting the slot 0 trap; the TSL
@@ -549,14 +555,17 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 */
 	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
 			     s626_send_dac_wait_fb_buffer2_msb_ff);
-	if (ret)
+	if (ret) {
 		comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
+		return ret;
+	}
+	return 0;
 }
 
 /*
  * Private helper function: Write setpoint to an application DAC channel.
  */
-static void s626_set_dac(struct comedi_device *dev, uint16_t chan,
+static int s626_set_dac(struct comedi_device *dev, uint16_t chan,
 			 int16_t dacdata)
 {
 	struct s626_private *devpriv = dev->private;
@@ -619,10 +628,10 @@ static void s626_set_dac(struct comedi_device *dev, uint16_t chan,
 	val |= ((uint32_t)(chan & 1) << 15);	/* Address the DAC channel
 						 * within the device. */
 	val |= (uint32_t)dacdata;	/* Include DAC setpoint data. */
-	s626_send_dac(dev, val);
+	return s626_send_dac(dev, val);
 }
 
-static void s626_write_trim_dac(struct comedi_device *dev, uint8_t logical_chan,
+static int s626_write_trim_dac(struct comedi_device *dev, uint8_t logical_chan,
 				uint8_t dac_data)
 {
 	struct s626_private *devpriv = dev->private;
@@ -669,17 +678,22 @@ static void s626_write_trim_dac(struct comedi_device *dev, uint8_t logical_chan,
 	 * Address the DAC channel within the trimdac device.
 	 * Include DAC setpoint data.
 	 */
-	s626_send_dac(dev, (chan << 8) | dac_data);
+	return s626_send_dac(dev, (chan << 8) | dac_data);
 }
 
-static void s626_load_trim_dacs(struct comedi_device *dev)
+static int s626_load_trim_dacs(struct comedi_device *dev)
 {
 	uint8_t i;
+	int ret;
 
 	/* Copy TrimDac setpoint values from EEPROM to TrimDacs. */
-	for (i = 0; i < ARRAY_SIZE(s626_trimchan); i++)
-		s626_write_trim_dac(dev, i,
+	for (i = 0; i < ARRAY_SIZE(s626_trimchan); i++) {
+		ret = s626_write_trim_dac(dev, i,
 				    s626_i2c_read(dev, s626_trimadrs[i]));
+		if (ret)
+			return ret;
+	}
+	return 0;
 }
 
 /* ******  COUNTER FUNCTIONS  ******* */
@@ -2378,6 +2392,7 @@ static int s626_ao_winsn(struct comedi_device *dev, struct comedi_subdevice *s,
 {
 	struct s626_private *devpriv = dev->private;
 	int i;
+	int ret;
 	uint16_t chan = CR_CHAN(insn->chanspec);
 	int16_t dacdata;
 
@@ -2386,7 +2401,9 @@ static int s626_ao_winsn(struct comedi_device *dev, struct comedi_subdevice *s,
 		devpriv->ao_readback[CR_CHAN(insn->chanspec)] = data[i];
 		dacdata -= (0x1fff);
 
-		s626_set_dac(dev, chan, dacdata);
+		ret = s626_set_dac(dev, chan, dacdata);
+		if (ret)
+			return ret;
 	}
 
 	return i;
@@ -2622,12 +2639,13 @@ static int s626_allocate_dma_buffers(struct comedi_device *dev)
 	return 0;
 }
 
-static void s626_initialize(struct comedi_device *dev)
+static int s626_initialize(struct comedi_device *dev)
 {
 	struct s626_private *devpriv = dev->private;
 	dma_addr_t phys_buf;
 	uint16_t chan;
 	int i;
+	int ret;
 
 	/* Enable DEBI and audio pins, enable I2C interface */
 	s626_mc_enable(dev, S626_MC1_DEBI | S626_MC1_AUDIO | S626_MC1_I2C,
@@ -2828,7 +2846,9 @@ static void s626_initialize(struct comedi_device *dev)
 	 * sometimes causes the first few TrimDAC writes to malfunction.
 	 */
 	s626_load_trim_dacs(dev);
-	s626_load_trim_dacs(dev);
+	ret = s626_load_trim_dacs(dev);
+	if (ret)
+		return ret;
 
 	/*
 	 * Manually init all gate array hardware in case this is a soft
@@ -2842,8 +2862,11 @@ static void s626_initialize(struct comedi_device *dev)
 	 * Init all DAC outputs to 0V and init all DAC setpoint and
 	 * polarity images.
 	 */
-	for (chan = 0; chan < S626_DAC_CHANNELS; chan++)
-		s626_set_dac(dev, chan, 0);
+	for (chan = 0; chan < S626_DAC_CHANNELS; chan++) {
+		ret = s626_set_dac(dev, chan, 0);
+		if (ret)
+			return ret;
+	}
 
 	/* Init counters */
 	s626_counters_init(dev);
@@ -2859,6 +2882,8 @@ static void s626_initialize(struct comedi_device *dev)
 
 	/* Initialize the digital I/O subsystem */
 	s626_dio_init(dev);
+
+	return 0;
 }
 
 static int s626_auto_attach(struct comedi_device *dev,
@@ -2979,7 +3004,9 @@ static int s626_auto_attach(struct comedi_device *dev,
 	s->insn_read	= s626_enc_insn_read;
 	s->insn_write	= s626_enc_insn_write;
 
-	s626_initialize(dev);
+	ret = s626_initialize(dev);
+	if (ret)
+		return ret;
 
 	return 0;
 }
-- 
1.8.5.3

Re: [PATCH v3 1/2] Staging: comedi: convert while loops to timeouts in

[Greg KH]

On Fri, Mar 07, 2014 at 07:43:04PM -0600, Chase Southwood wrote:
> This patch changes a handful of while loops to timeouts to prevent
> infinite looping on hardware failure. A couple such loops are in a
> function (s626_debi_transfer()) which is called from critical sections,
> so comedi_timeout() is unusable for them, and an iterative timeout is
> used instead. For the while loops in a context where comedi_timeout() is
> allowed, a new callback function, s626_send_dac_eoc(), has been defined
> to evaluate the conditions that the while loops are testing.  The new
> callback employs a switch statement based on a simple new enum so that
> it is usable for all of the different conditions tested in while loops
> in s626_send_dac().  The proper comedi_timeout() calls are then used.

Your subject seems to be missing a word at the end of it :(

Re: [PATCH v3 1/2] Staging: comedi: convert while loops to timeouts in

[Chase Southwood]

Hi Greg,

>On Saturday, March 8, 2014 9:00 PM, Greg KH <gregkh@linuxfoundation.org> wrote:

>>On Fri, Mar 07, 2014 at 07:43:04PM -0600, Chase Southwood wrote:
>>This patch changes a handful of while loops to timeouts to prevent
>>infinite looping on hardware failure. A couple such loops are in a
>>function (s626_debi_transfer()) which is called from critical sections,
>>so comedi_timeout() is unusable for them, and an iterative timeout is
>>used instead. For the while loops in a context where comedi_timeout() is
>>allowed, a new callback function, s626_send_dac_eoc(), has been defined
>>to evaluate the conditions that the while loops are testing.  The new
>>callback employs a switch statement based on a simple new enum so that
>>it is usable for all of the different conditions tested in while loops
>>in s626_send_dac().  The proper comedi_timeout() calls are then used.
>
>Your subject seems to be missing a word at the end of it :(

Well what a silly problem to have :( I don't know what happened there
but I'll fix the subject and send it out again.

Thanks,
Chase

[PATCH v3 1/2] Staging: comedi: convert while loops to timeouts in s626.c

[Chase Southwood]

This patch changes a handful of while loops to timeouts to prevent
infinite looping on hardware failure. A couple such loops are in a
function (s626_debi_transfer()) which is called from critical sections,
so comedi_timeout() is unusable for them, and an iterative timeout is
used instead. For the while loops in a context where comedi_timeout() is
allowed, a new callback function, s626_send_dac_eoc(), has been defined
to evaluate the conditions that the while loops are testing.  The new
callback employs a switch statement based on a simple new enum so that
it is usable for all of the different conditions tested in while loops
in s626_send_dac().  The proper comedi_timeout() calls are then used.

Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---
Ian, here is a version of this patchset employing the enum you recommended.
The second patch has been rebased on top of this one.

2: Used comedi_timeout() where appropriate, introduce callback function

3: Updated callback to switch on new enum.

 drivers/staging/comedi/drivers/s626.c | 87 ++++++++++++++++++++++++++++++-----
 1 file changed, 75 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 5ba4b4a..ff9dde1 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -209,6 +209,8 @@ static const struct comedi_lrange s626_range_table = {
 static void s626_debi_transfer(struct comedi_device *dev)
 {
 	struct s626_private *devpriv = dev->private;
+	static const int timeout = 10000;
+	int i;
 
 	/* Initiate upload of shadow RAM to DEBI control register */
 	s626_mc_enable(dev, S626_MC2_UPLD_DEBI, S626_P_MC2);
@@ -217,12 +219,22 @@ static void s626_debi_transfer(struct comedi_device *dev)
 	 * Wait for completion of upload from shadow RAM to
 	 * DEBI control register.
 	 */
-	while (!s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2))
-		;
+	for (i = 0; i < timeout; i++) {
+		if (s626_mc_test(dev, S626_MC2_UPLD_DEBI, S626_P_MC2))
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "Timeout while uploading to DEBI control register.");
 
 	/* Wait until DEBI transfer is done */
-	while (readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S)
-		;
+	for (i = 0; i < timeout; i++) {
+		if (!(readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S))
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "DEBI transfer timeout.");
 }
 
 /*
@@ -351,6 +363,48 @@ static const uint8_t s626_trimadrs[] = {
 	0x40, 0x41, 0x42, 0x50, 0x51, 0x52, 0x53, 0x60, 0x61, 0x62, 0x63
 };
 
+enum {
+	s626_send_dac_wait_not_mc1_a2out,
+	s626_send_dac_wait_ssr_af2_out,
+	s626_send_dac_wait_fb_buffer2_msb_00,
+	s626_send_dac_wait_fb_buffer2_msb_ff
+};
+
+static int s626_send_dac_eoc(struct comedi_device *dev,
+			     struct comedi_subdevice *s,
+			     struct comedi_insn *insn,
+			     unsigned long context)
+{
+	struct s626_private *devpriv = dev->private;
+	unsigned int status;
+
+	switch (context) {
+	case s626_send_dac_wait_not_mc1_a2out:
+		status = readl(devpriv->mmio + S626_P_MC1);
+		if (!(status & S626_MC1_A2OUT))
+			return 0;
+		break;
+	case s626_send_dac_wait_ssr_af2_out:
+		status = readl(devpriv->mmio + S626_P_SSR);
+		if (status & S626_SSR_AF2_OUT)
+			return 0;
+		break;
+	case s626_send_dac_wait_fb_buffer2_msb_00:
+		status = readl(devpriv->mmio + S626_P_FB_BUFFER2);
+		if (!(status & 0xff000000))
+			return 0;
+		break;
+	case s626_send_dac_wait_fb_buffer2_msb_ff:
+		status = readl(devpriv->mmio + S626_P_FB_BUFFER2);
+		if (status & 0xff000000)
+			return 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return -EBUSY;
+}
+
 /*
  * Private helper function: Transmit serial data to DAC via Audio
  * channel 2.  Assumes: (1) TSL2 slot records initialized, and (2)
@@ -359,6 +413,7 @@ static const uint8_t s626_trimadrs[] = {
 static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 {
 	struct s626_private *devpriv = dev->private;
+	int ret;
 
 	/* START THE SERIAL CLOCK RUNNING ------------- */
 
@@ -404,8 +459,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * Done by polling the DMAC enable flag; this flag is automatically
 	 * cleared when the transfer has finished.
 	 */
-	while (readl(devpriv->mmio + S626_P_MC1) & S626_MC1_A2OUT)
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
+			     s626_send_dac_wait_not_mc1_a2out);
+	if (ret)
+		comedi_error(dev, "DMA transfer timeout.");
 
 	/* START THE OUTPUT STREAM TO THE TARGET DAC -------------------- */
 
@@ -425,8 +482,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * finished transferring the DAC's data DWORD from the output FIFO
 	 * to the output buffer register.
 	 */
-	while (!(readl(devpriv->mmio + S626_P_SSR) & S626_SSR_AF2_OUT))
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
+			     s626_send_dac_wait_ssr_af2_out);
+	if (ret)
+		comedi_error(dev, "TSL timeout waiting for slot 1 to execute.");
 
 	/*
 	 * Set up to trap execution at slot 0 when the TSL sequencer cycles
@@ -466,8 +525,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 		 * from 0xFF to 0x00, which slot 0 causes to happen by shifting
 		 * out/in on SD2 the 0x00 that is always referenced by slot 5.
 		 */
-		while (readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000)
-			;
+		ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
+				     s626_send_dac_wait_fb_buffer2_msb_00);
+		if (ret)
+			comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
 	}
 	/*
 	 * Either (1) we were too late setting the slot 0 trap; the TSL
@@ -486,8 +547,10 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * the next DAC write.  This is detected when FB_BUFFER2 MSB changes
 	 * from 0x00 to 0xFF.
 	 */
-	while (!(readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000))
-		;
+	ret = comedi_timeout(dev, NULL, NULL, s626_send_dac_eoc,
+			     s626_send_dac_wait_fb_buffer2_msb_ff);
+	if (ret)
+		comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
 }
 
 /*
-- 
1.8.5.3

Re: [PATCH v3 1/2] Staging: comedi: convert while loops to timeouts in s626.c

[Ian Abbott]

On 2014-03-09 04:00, Chase Southwood wrote:
> This patch changes a handful of while loops to timeouts to prevent
> infinite looping on hardware failure. A couple such loops are in a
> function (s626_debi_transfer()) which is called from critical sections,
> so comedi_timeout() is unusable for them, and an iterative timeout is
> used instead. For the while loops in a context where comedi_timeout() is
> allowed, a new callback function, s626_send_dac_eoc(), has been defined
> to evaluate the conditions that the while loops are testing.  The new
> callback employs a switch statement based on a simple new enum so that
> it is usable for all of the different conditions tested in while loops
> in s626_send_dac().  The proper comedi_timeout() calls are then used.
>
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
> Ian, here is a version of this patchset employing the enum you recommended.
> The second patch has been rebased on top of this one.
>
> 2: Used comedi_timeout() where appropriate, introduce callback function
>
> 3: Updated callback to switch on new enum.

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

For future reference, for patches affecting a single comedi driver, we 
usually title the patches like this:

staging: comedi: name_of_driver: summary of patch

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH v3 2/2] Staging: comedi: propagate timeout errors in s626.c

[Ian Abbott]

On 2014-03-08 01:43, Chase Southwood wrote:
> This patch for s626.c propagates the errors from the newly introduced
> calls to comedi_timeout() as far as possible.
>
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
> Compile tested.
>
> 2: This patch was introduced in v2 of this set.
>
> 3: Rebased on top of PATCH 1/2 edits.

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH v3 1/2] Staging: comedi: convert while loops to timeouts in s626.c

[Chase Southwood]

>On Tuesday, March 11, 2014 9:26 AM, Ian Abbott <abbotti@mev.co.uk> wrote:

>>On 2014-03-09 04:00, Chase Southwood wrote:
>> This patch changes a handful of while loops to timeouts to prevent
>> infinite looping on hardware failure. A couple such loops are in a
>> function (s626_debi_transfer()) which is called from critical sections,
>> so comedi_timeout() is unusable for them, and an iterative timeout is
>> used instead. For the while loops in a context where comedi_timeout() is
>> allowed, a new callback function, s626_send_dac_eoc(), has been defined
>> to evaluate the conditions that the while loops are testing.  The new
>> callback employs a switch statement based on a simple new enum so that
>> it is usable for all of the different conditions tested in while loops
>> in s626_send_dac().  The proper comedi_timeout() calls are then used.
>>
>> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
>> ---
>> Ian, here is a version of this patchset employing the enum you recommended.
>> The second patch has been rebased on top of this one.
>>
>> 2: Used comedi_timeout() where appropriate, introduce callback function
>>
>> 3: Updated callback to switch on new enum.>
>
>Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
>
>For future reference, for patches affecting a single comedi driver, we 
>usually title the patches like this:
>
>staging: comedi: name_of_driver: summary of patch
>


Hi Greg!

I was just writing to inquire whether you were able to add this patch as well as
PATCH 2/2 Propagate timeout errors in s626.c, to your queue in their current state.
I had to resend this patch to you about a week ago because the subject line got
a little messed up, which might have lead to a bit of confusion regarding the 2
patch series, and I wanted to check in to see whether you need me to do anything
further.

Thanks,
Chase

Re: [PATCH v3 1/2] Staging: comedi: convert while loops to timeouts in s626.c

[gregkh]

On Fri, Mar 14, 2014 at 06:43:37PM -0700, Chase Southwood wrote:
> >On Tuesday, March 11, 2014 9:26 AM, Ian Abbott <abbotti@mev.co.uk> wrote:
> 
> >>On 2014-03-09 04:00, Chase Southwood wrote:
> >> This patch changes a handful of while loops to timeouts to prevent
> >> infinite looping on hardware failure. A couple such loops are in a
> >> function (s626_debi_transfer()) which is called from critical sections,
> >> so comedi_timeout() is unusable for them, and an iterative timeout is
> >> used instead. For the while loops in a context where comedi_timeout() is
> >> allowed, a new callback function, s626_send_dac_eoc(), has been defined
> >> to evaluate the conditions that the while loops are testing.  The new
> >> callback employs a switch statement based on a simple new enum so that
> >> it is usable for all of the different conditions tested in while loops
> >> in s626_send_dac().  The proper comedi_timeout() calls are then used.
> >>
> >> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> >> ---
> >> Ian, here is a version of this patchset employing the enum you recommended.
> >> The second patch has been rebased on top of this one.
> >>
> >> 2: Used comedi_timeout() where appropriate, introduce callback function
> >>
> >> 3: Updated callback to switch on new enum.>
> >
> >Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
> >
> >For future reference, for patches affecting a single comedi driver, we 
> >usually title the patches like this:
> >
> >staging: comedi: name_of_driver: summary of patch
> >
> 
> 
> Hi Greg!
> 
> I was just writing to inquire whether you were able to add this patch as well as
> PATCH 2/2 Propagate timeout errors in s626.c, to your queue in their current state.
> I had to resend this patch to you about a week ago because the subject line got
> a little messed up, which might have lead to a bit of confusion regarding the 2
> patch series, and I wanted to check in to see whether you need me to do anything
> further.

I've been on vacation this week and will dig through my huge patch queue
next week.  Then I will need another vacation...

Give me a chance to catch up, I'll let you know if I have problems with
them.

thanks,

greg k-h

Re: [PATCH v3 1/2] Staging: comedi: convert while loops to timeouts in s626.c

[Chase Southwood]

>On Saturday, March 15, 2014 12:26 AM, "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org> wrote:

>>On Fri, Mar 14, 2014 at 06:43:37PM -0700, Chase Southwood wrote:
>>>On Tuesday, March 11, 2014 9:26 AM, Ian Abbott <abbotti@mev.co.uk> wrote:
>>>>On 2014-03-09 04:00, Chase Southwood wrote:
>>>> This patch changes a handful of while loops to timeouts to prevent
>>>> infinite looping on hardware failure. A couple such loops are in a
>>>> function (s626_debi_transfer()) which is called from critical sections,
>>>> so comedi_timeout() is unusable for them, and an iterative timeout is
>>>> used instead. For the while loops in a context where comedi_timeout() is
>>>> allowed, a new callback function, s626_send_dac_eoc(), has been defined
>>>> to evaluate the conditions that the while loops are testing.  The new
>>>> callback employs a switch statement based on a simple new enum so that
>>>> it is usable for all of the different conditions tested in while loops
>>>> in s626_send_dac().  The proper comedi_timeout() calls are then used.
>>>>
>>>> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
>>>> ---
>>>> Ian, here is a version of this patchset employing the enum you recommended.
>>>> The second patch has been rebased on top of this one.
>>>>
>>>> 2: Used comedi_timeout() where appropriate, introduce callback function
>>>>
>>>> 3: Updated callback to switch on new enum.>
>>>
>>>Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
>>>
>>>For future reference, for patches affecting a single comedi driver, we 
>>>usually title the patches like this:
>>>
>>>staging: comedi: name_of_driver: summary of patch
>>>
>>
>>
>>Hi Greg!
>>
>>I was just writing to inquire whether you were able to add this patch as well as
>>PATCH 2/2 Propagate timeout errors in s626.c, to your queue in their current state.
>>I had to resend this patch to you about a week ago because the subject line got
>>a little messed up, which might have lead to a bit of confusion regarding the 2
>>patch series, and I wanted to check in to see whether you need me to do anything
>>further.
>
>I've been on vacation this week and will dig through my huge patch queue
>next week.  Then I will need another vacation...
>
>Give me a chance to catch up, I'll let you know if I have problems with
>them.>
>
>thanks,
>
>greg k-h

Greg,

Oh shoot, my apologies.  I don't mean to rush you at all!  I'll chill out a bit...

Thanks,
Chase