LinuxLists.cc - [PATCH] Staging: comedi: move trailing statement to next line in ni_mio

[permalink] [raw]

Subject: Re: [PATCH] Staging: comedi: move trailing statement to next line in ni_mio_common.c

On 2014-01-14 07:23, Dan Carpenter wrote:
> On Mon, Jan 13, 2014 at 07:16:14PM -0800, Joe Perches wrote:
>> On Mon, 2014-01-13 at 21:13 -0600, Chase Southwood wrote:
>>> This patch for ni_mio_common.c silences a checkpatch error due to a
>>> trailing statement.
>> []
>>> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
>> []
>>> @@ -692,7 +692,8 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
>>> /* Flush the 6143 data FIFO */
>>> ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
>>> ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
>>> - while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
>>> + while (ni_readl(AIFIFO_Status_6143) & 0x10)
>>> + ; /* Wait for complete */
>>
>> It's generally better to use timeouts too.
>
> Just to clarify what Joe is saying do:
>
> /* Wait for complete */
> while (timemout < TIMEOUT) {
> if (ni_readl(AIFIFO_Status_6143) & 0x10)
> break;
> udelay(1);
> }
>
> I added in a delay... The problem is that you'd probably have to look
> at the hardware spec to know what timeout to use or if the delay is
> needed.

Some longish timeout of, say, 10000 iterations (~ 0.01 seconds) would
probably do as it's not that time critical.

We wouldn't expect code clean-up patches to have to deal with that sort
of thing though.

--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-

2014-01-14 11:48:23

[permalink] [raw]

Subject: Re: [PATCH] Staging: comedi: move trailing statement to next line in ni_mio_common.c

On 2014-01-14 03:13, Chase Southwood wrote:
> This patch for ni_mio_common.c silences a checkpatch error due to a
> trailing statement.
>
> Signed-off-by: Chase Southwood <[email protected]>
> ---
> drivers/staging/comedi/drivers/ni_mio_common.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index 457b884..dba19e9 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -692,7 +692,8 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
> /* Flush the 6143 data FIFO */
> ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
> ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
> - while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
> + while (ni_readl(AIFIFO_Status_6143) & 0x10)
> + ; /* Wait for complete */
> } else {
> devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
> if (board->reg_type == ni_reg_625x) {
>

As others mentioned, a timeout would be nice, but as this is just a
clean-up patch, I think it's fine as-is.

Reviewed-by: Ian Abbott <[email protected]>

--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-

2014-01-14 19:39:38

[permalink] [raw]

Subject: [PATCH v2] Staging: comedi: convert while loop to timeout in ni_mio_common.c.

This patch to ni_mio_common.c changes a while loop to a timeout for
loop, which is preferred.

Signed-off-by: Chase Southwood <[email protected]>
---

I know Mr. Abbott mentioned that he wouldn't expect clean-up patches to have to deal with this sort of thing, but I thought I'd at least give the timeout thing a try. Feel free to disregard this and take v1 of this patch instead if you would just like the simple clean-up. That being said, I used 10000 iterations at the suggestion of Mr. Abbott, and a short delay as well. Let me know if these values seem incorrect. Also, at some (but not all) locations in this file that currently use timeouts, contain a check for i == timeout, with a call to either printk or comedi_error if the operation actually times out. Would something like that be required here?

Sorry for all of the questions, but I'm sort of new around here and I'd like to help out with more than just clean-ups and this seemed like a good opportunity to at least try!

2: Changed from simple clean-up to swapping a timeout in for a while loop.

drivers/staging/comedi/drivers/ni_mio_common.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 457b884..05cd5ed 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -687,12 +687,19 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
{
const struct ni_board_struct *board = comedi_board(dev);
struct ni_private *devpriv = dev->private;
+ static const int timeout = 10000;
+ int i;

if (board->reg_type == ni_reg_6143) {
/* Flush the 6143 data FIFO */
ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
- while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
+ /* Wait for complete */
+ for (i = 0; i < timeout; i++) {
+ if(!(ni_readl(AIFIFO_Status_6143) & 0x10))
+ break;
+ udelay(1);
+ }
} else {
devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
if (board->reg_type == ni_reg_625x) {
--
1.8.4.2

2014-01-14 19:46:04

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH v2] Staging: comedi: convert while loop to timeout in ni_mio_common.c.

On Tue, 2014-01-14 at 13:38 -0600, Chase Southwood wrote:
> This patch to ni_mio_common.c changes a while loop to a timeout for
> loop, which is preferred.
>
> Signed-off-by: Chase Southwood <[email protected]>
> ---
>
> I know Mr. Abbott mentioned that he wouldn't expect clean-up patches to have to deal with this sort of thing, but I thought I'd at least give the timeout thing a try. Feel free to disregard this and take v1 of this patch instead if you would just like the simple clean-up. That being said, I used 10000 iterations at the suggestion of Mr. Abbott, and a short delay as well. Let me know if these values seem incorrect. Also, at some (but not all) locations in this file that currently use timeouts, contain a check for i == timeout, with a call to either printk or comedi_error if the operation actually times out. Would something like that be required here?
>
> Sorry for all of the questions, but I'm sort of new around here and I'd like to help out with more than just clean-ups and this seemed like a good opportunity to at least try!
>
> 2: Changed from simple clean-up to swapping a timeout in for a while loop.
>
> drivers/staging/comedi/drivers/ni_mio_common.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index 457b884..05cd5ed 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -687,12 +687,19 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
> {
> const struct ni_board_struct *board = comedi_board(dev);
> struct ni_private *devpriv = dev->private;
> + static const int timeout = 10000;
> + int i;
>
> if (board->reg_type == ni_reg_6143) {
> /* Flush the 6143 data FIFO */
> ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
> ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
> - while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
> + /* Wait for complete */
> + for (i = 0; i < timeout; i++) {
> + if(!(ni_readl(AIFIFO_Status_6143) & 0x10))
> + break;
> + udelay(1);
> + }

some sort of error message would be nice too

if (i >= timeout)
comedi_error(dev, "FIFO flush timeout");

2014-01-14 19:50:01

[permalink] [raw]

Subject: Re: [PATCH v2] Staging: comedi: convert while loop to timeout in ni_mio_common.c.

On Tue, Jan 14, 2014 at 01:38:46PM -0600, Chase Southwood wrote:
> This patch to ni_mio_common.c changes a while loop to a timeout for
> loop, which is preferred.
>
> Signed-off-by: Chase Southwood <[email protected]>
> ---
>
> I know Mr. Abbott mentioned that he wouldn't expect clean-up patches to have to deal with this sort of thing, but I thought I'd at least give the timeout thing a try. Feel free to disregard this and take v1 of this patch instead if you would just like the simple clean-up. That being said, I used 10000 iterations at the suggestion of Mr. Abbott, and a short delay as well. Let me know if these values seem incorrect. Also, at some (but not all) locations in this file that currently use timeouts, contain a check for i == timeout, with a call to either printk or comedi_error if the operation actually times out. Would something like that be required here?
>
> Sorry for all of the questions, but I'm sort of new around here and I'd like to help out with more than just clean-ups and this seemed like a good opportunity to at least try!
>
> 2: Changed from simple clean-up to swapping a timeout in for a while loop.
>
> drivers/staging/comedi/drivers/ni_mio_common.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index 457b884..05cd5ed 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -687,12 +687,19 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
> {
> const struct ni_board_struct *board = comedi_board(dev);
> struct ni_private *devpriv = dev->private;
> + static const int timeout = 10000;
> + int i;
>
> if (board->reg_type == ni_reg_6143) {
> /* Flush the 6143 data FIFO */
> ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
> ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
> - while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
> + /* Wait for complete */
> + for (i = 0; i < timeout; i++) {
> + if(!(ni_readl(AIFIFO_Status_6143) & 0x10))

Please always run your patches through scripts/checkpatch.pl so that we
don't just point out the issues that it would have told you about :)

> + break;
> + udelay(1);

This is good, but you don't need a counter, you could just look at the
time that has expired so far and if it exceeds a limit, then exit and
error out.

thanks,

greg k-h

2014-01-14 21:31:12

[permalink] [raw]

Subject: [PATCH v3] Staging: comedi: convert while loop to timeout in ni_mio_common.c

This patch to ni_mio_common.c changes a simple while loop to a timeout,
which is preferred.

Signed-off-by: Chase Southwood <[email protected]>
---

I removed the extra counter variable this time. Greg, you mentioned that I could just look at the time that has expired to far, and exit and error out if that exceeds a limit. Right now I'm just tracking how many times udelay is called, since that seems to conform more to other timeouts in this file, but did you instead want me to use some time function to keep track of actual real time that expires?

Thanks,
Chase Southwood

2: Changed from simple clean-up to swapping a timeout in for a while loop.

3: Removed extra counter variable, and added error checking.

drivers/staging/comedi/drivers/ni_mio_common.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 457b884..143b8b8 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -687,12 +687,21 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
{
const struct ni_board_struct *board = comedi_board(dev);
struct ni_private *devpriv = dev->private;
+ int timeout = 0;

if (board->reg_type == ni_reg_6143) {
/* Flush the 6143 data FIFO */
ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
- while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
+ /* Wait for complete */
+ while (timeout < 10000) {
+ if (!(ni_readl(AIFIFO_Status_6143) & 0x10))
+ break;
+ udelay(1);
+ timeout++;
+ }
+ if (timeout == 10000)
+ comedi_error(dev, "FIFO flush timeout.");
} else {
devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
if (board->reg_type == ni_reg_625x) {
--
1.8.4.2

2014-01-14 23:09:28

[permalink] [raw]

Subject: Re: [PATCH v3] Staging: comedi: convert while loop to timeout in ni_mio_common.c

On Tue, Jan 14, 2014 at 03:31:01PM -0600, Chase Southwood wrote:
> This patch to ni_mio_common.c changes a simple while loop to a timeout,
> which is preferred.
>
> Signed-off-by: Chase Southwood <[email protected]>
> ---
>
> I removed the extra counter variable this time. Greg, you mentioned that I could just look at the time that has expired to far, and exit and error out if that exceeds a limit. Right now I'm just tracking how many times udelay is called, since that seems to conform more to other timeouts in this file, but did you instead want me to use some time function to keep track of actual real time that expires?

Please wrap your lines of text, as that's one very long line :)

The best way to do this is to do:
unsigned long timeout;
...
timeout = jiffies + msec_to_jiffies(500); /* Or how ever long you want to wait */
do {
do something...
udelay(10);
} while (time_before(jiffies, timeout);

or switch the loop around and do:

while (something hasn't happened) {
if (time_after(jjiffies, timeout)) {
we timed out, handle it here;
break;
}
udelay(10);
}

That's much better than having to count up a "number" and have to look
to see how long you are sleeping for to determine just exactly how long
you are going to have until the loop times out, as your code did.

Make sense?

greg k-h

2014-01-15 00:23:15

[permalink] [raw]

Subject: [PATCH v4] Staging: comedi: convert while loop to timeout in ni_mio_common.c

This patch for ni_mio_common.c changes out a while loop for a timeout,
which is preferred.

Signed-off-by: Chase Southwood <[email protected]>
---

OK, here's another go at it. Hopefully everything looks more correct
this time. Greg, I've followed the pattern you gave me, and I really
appreciate all of the tips! As always, just let me know if there are
still things that need adjusting (especially length of timeout, udelay,
etc.).

Thanks,
Chase Southwood

2: Changed from simple clean-up to swapping a timeout in for a while loop.

3: Removed extra counter variable, and added error checking.

4: No longer using counter variable, using jiffies instead.

drivers/staging/comedi/drivers/ni_mio_common.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 457b884..882b249 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -687,12 +687,21 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
{
const struct ni_board_struct *board = comedi_board(dev);
struct ni_private *devpriv = dev->private;
+ unsigned long timeout;

if (board->reg_type == ni_reg_6143) {
/* Flush the 6143 data FIFO */
ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
- while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
+ /* Wait for complete */
+ timeout = jiffies + msec_to_jiffies(100);
+ while (ni_readl(AIFIFO_Status_6143) & 0x10) {
+ if (time_after(jiffies, timeout)) {
+ comedi_error(dev, "FIFO flush timeout.");
+ break;
+ }
+ udelay(1);
+ }
} else {
devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
if (board->reg_type == ni_reg_625x) {
--
1.8.4.2

2014-01-15 03:58:37

[permalink] [raw]

Subject: Re: [PATCH v4] Staging: comedi: convert while loop to timeout in ni_mio_common.c

On Tue, Jan 14, 2014 at 06:23:05PM -0600, Chase Southwood wrote:
> This patch for ni_mio_common.c changes out a while loop for a timeout,
> which is preferred.
>
> Signed-off-by: Chase Southwood <[email protected]>
> ---
>
> OK, here's another go at it. Hopefully everything looks more correct
> this time. Greg, I've followed the pattern you gave me, and I really
> appreciate all of the tips! As always, just let me know if there are
> still things that need adjusting (especially length of timeout, udelay,
> etc.).
>
> Thanks,
> Chase Southwood
>
> 2: Changed from simple clean-up to swapping a timeout in for a while loop.
>
> 3: Removed extra counter variable, and added error checking.
>
> 4: No longer using counter variable, using jiffies instead.
>
> drivers/staging/comedi/drivers/ni_mio_common.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index 457b884..882b249 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -687,12 +687,21 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
> {
> const struct ni_board_struct *board = comedi_board(dev);
> struct ni_private *devpriv = dev->private;
> + unsigned long timeout;
>
> if (board->reg_type == ni_reg_6143) {
> /* Flush the 6143 data FIFO */
> ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
> ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
> - while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
> + /* Wait for complete */
> + timeout = jiffies + msec_to_jiffies(100);
> + while (ni_readl(AIFIFO_Status_6143) & 0x10) {
> + if (time_after(jiffies, timeout)) {
> + comedi_error(dev, "FIFO flush timeout.");
> + break;
> + }
> + udelay(1);

Sleep for at least 10, as I think that's the smallest time delay you can
sleep for anyway (meaning it will be that long no matter what number you
put there less than 10, depending on the hardware used of course.)

Other than that, looks much better.

thanks,

greg k-h

2014-01-15 05:15:25

[permalink] [raw]

Subject: [PATCH v5] Staging: comedi: convert while loop to timeout in ni_mio_common.c

This patch for ni_mio_common.c changes out a while loop for a timeout,
which is preferred.

Signed-off-by: Chase Southwood <[email protected]>
---

All right, I think this guy's ready to go now! Thanks for all the help!
Chase

2: Changed from simple clean-up to swapping a timeout in for a while loop.

3: Removed extra counter variable, and added error checking.

4: No longer using counter variable, using jiffies instead.

5: udelay for 10u, instead of 1u.

drivers/staging/comedi/drivers/ni_mio_common.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 457b884..07e9777 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -687,12 +687,21 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
{
const struct ni_board_struct *board = comedi_board(dev);
struct ni_private *devpriv = dev->private;
+ unsigned long timeout;

if (board->reg_type == ni_reg_6143) {
/* Flush the 6143 data FIFO */
ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
- while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
+ /* Wait for complete */
+ timeout = jiffies + msec_to_jiffies(500);
+ while (ni_readl(AIFIFO_Status_6143) & 0x10) {
+ if (time_after(jiffies, timeout)) {
+ comedi_error(dev, "FIFO flush timeout.");
+ break;
+ }
+ udelay(10);
+ }
} else {
devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
if (board->reg_type == ni_reg_625x) {
--
1.8.4.2

2014-01-15 10:30:07

[permalink] [raw]

Subject: Re: [PATCH v4] Staging: comedi: convert while loop to timeout in ni_mio_common.c

On 2014-01-15 03:58, Greg KH wrote:
> On Tue, Jan 14, 2014 at 06:23:05PM -0600, Chase Southwood wrote:
>> This patch for ni_mio_common.c changes out a while loop for a timeout,
>> which is preferred.
>>
>> Signed-off-by: Chase Southwood <[email protected]>
>> ---
>>
>> OK, here's another go at it. Hopefully everything looks more correct
>> this time. Greg, I've followed the pattern you gave me, and I really
>> appreciate all of the tips! As always, just let me know if there are
>> still things that need adjusting (especially length of timeout, udelay,
>> etc.).
>>
>> Thanks,
>> Chase Southwood
>>
>> 2: Changed from simple clean-up to swapping a timeout in for a while loop.
>>
>> 3: Removed extra counter variable, and added error checking.
>>
>> 4: No longer using counter variable, using jiffies instead.
>>
>> drivers/staging/comedi/drivers/ni_mio_common.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
>> index 457b884..882b249 100644
>> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
>> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
>> @@ -687,12 +687,21 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
>> {
>> const struct ni_board_struct *board = comedi_board(dev);
>> struct ni_private *devpriv = dev->private;
>> + unsigned long timeout;
>>
>> if (board->reg_type == ni_reg_6143) {
>> /* Flush the 6143 data FIFO */
>> ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
>> ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
>> - while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
>> + /* Wait for complete */
>> + timeout = jiffies + msec_to_jiffies(100);
>> + while (ni_readl(AIFIFO_Status_6143) & 0x10) {
>> + if (time_after(jiffies, timeout)) {
>> + comedi_error(dev, "FIFO flush timeout.");
>> + break;
>> + }
>> + udelay(1);
>
> Sleep for at least 10, as I think that's the smallest time delay you can
> sleep for anyway (meaning it will be that long no matter what number you
> put there less than 10, depending on the hardware used of course.)

udelay() doesn't sleep (though it may get pre-empted) and I think you
can use any value you like within reason (up to a few 1000 microseconds).

>
> Other than that, looks much better.
>
> thanks,
>
> greg k-h
>

--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-

2014-01-15 10:38:11

[permalink] [raw]

Subject: Re: [PATCH v5] Staging: comedi: convert while loop to timeout in ni_mio_common.c

On 2014-01-15 05:15, Chase Southwood wrote:
> This patch for ni_mio_common.c changes out a while loop for a timeout,
> which is preferred.
>
> Signed-off-by: Chase Southwood <[email protected]>
> ---
>
> All right, I think this guy's ready to go now! Thanks for all the help!
> Chase
>
> 2: Changed from simple clean-up to swapping a timeout in for a while loop.
>
> 3: Removed extra counter variable, and added error checking.
>
> 4: No longer using counter variable, using jiffies instead.
>
> 5: udelay for 10u, instead of 1u.
>
> drivers/staging/comedi/drivers/ni_mio_common.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index 457b884..07e9777 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -687,12 +687,21 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
> {
> const struct ni_board_struct *board = comedi_board(dev);
> struct ni_private *devpriv = dev->private;
> + unsigned long timeout;
>
> if (board->reg_type == ni_reg_6143) {
> /* Flush the 6143 data FIFO */
> ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
> ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
> - while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
> + /* Wait for complete */
> + timeout = jiffies + msec_to_jiffies(500);
> + while (ni_readl(AIFIFO_Status_6143) & 0x10) {
> + if (time_after(jiffies, timeout)) {
> + comedi_error(dev, "FIFO flush timeout.");
> + break;
> + }
> + udelay(10);

I preferred the earlier udelay(1) from your PATCH v4, but I think it's
even better to replace it with:

cpu_relax();

You might also need:

#include <asm/processor.h>

to define cpu_relax().

> + }
> } else {
> devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
> if (board->reg_type == ni_reg_625x) {
>

--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-

2014-01-15 17:52:00

[permalink] [raw]

Subject: [PATCH v6] Staging: comedi: convert while loop to timeout in ni_mio_common.c

This patch for ni_mio_common.c changes out a while loop for a timeout,
which is preferred.

Signed-off-by: Chase Southwood <[email protected]>
---

2: Changed from simple clean-up to swapping a timeout in for a while loop.

3: Removed extra counter variable, and added error checking.

4: No longer using counter variable, using jiffies instead.

5: udelay for 10u, instead of 1u.

6: Scrap udelay entirely, in favor of cpu_relax. Include asm/processor.h
in order to use cpu_relax.

drivers/staging/comedi/drivers/ni_mio_common.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 457b884..fe2f434 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -55,6 +55,7 @@
#include <linux/interrupt.h>
#include <linux/sched.h>
#include <linux/delay.h>
+#include <asm/processor.h>
#include "8255.h"
#include "mite.h"
#include "comedi_fc.h"
@@ -687,12 +688,21 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
{
const struct ni_board_struct *board = comedi_board(dev);
struct ni_private *devpriv = dev->private;
+ unsigned long timeout;

if (board->reg_type == ni_reg_6143) {
/* Flush the 6143 data FIFO */
ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
- while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
+ /* Wait for complete */
+ timeout = jiffies + msec_to_jiffies(500);
+ while (ni_readl(AIFIFO_Status_6143) & 0x10) {
+ if (time_after(jiffies, timeout)) {
+ comedi_error(dev, "FIFO flush timeout.");
+ break;
+ }
+ cpu_relax();
+ }
} else {
devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
if (board->reg_type == ni_reg_625x) {
--
1.8.4.2

2014-01-15 18:30:08

by Hartley Sweeten

[permalink] [raw]

Subject: RE: [PATCH v4] Staging: comedi: convert while loop to timeout in ni_mio_common.c

On Tuesday, January 14, 2014 8:59 PM, Greg KH wrote:
> Sleep for at least 10, as I think that's the smallest time delay you can
> sleep for anyway (meaning it will be that long no matter what number you
> put there less than 10, depending on the hardware used of course.)

A bit off topic here but I have a somewhat related question about timeouts.

There are a number of comedi drivers that do a "wait for end-of-conversion"
as part of the (*insn_read) for an analog input subdevice or (*insn_write) for
an analog output subdevice. These functions return an errno if a timeout occurs.

Currently either -ETIME or -ETIMEDOUT is returned. This errno ends up getting
returned to the user as the result of the unlocked_ioctl file operation. What is
the more appropriate errno? Or is there is better one that should be used?

Regards,
Hartley

2014-01-15 18:48:37

by Hartley Sweeten

[permalink] [raw]

Subject: RE: [PATCH v6] Staging: comedi: convert while loop to timeout in ni_mio_common.c

On Wednesday, January 15, 2014 10:52 AM, Chase Southwood wrote:
> This patch for ni_mio_common.c changes out a while loop for a timeout,
> which is preferred.
>
> Signed-off-by: Chase Southwood <[email protected]>
> ---
>
> 2: Changed from simple clean-up to swapping a timeout in for a while loop.
>
> 3: Removed extra counter variable, and added error checking.
>
> 4: No longer using counter variable, using jiffies instead.
>
> 5: udelay for 10u, instead of 1u.
>
> 6: Scrap udelay entirely, in favor of cpu_relax. Include asm/processor.h
> in order to use cpu_relax.
>
> drivers/staging/comedi/drivers/ni_mio_common.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index 457b884..fe2f434 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -55,6 +55,7 @@
> #include <linux/interrupt.h>
> #include <linux/sched.h>
> #include <linux/delay.h>
> +#include <asm/processor.h>
> #include "8255.h"
> #include "mite.h"
> #include "comedi_fc.h"
> @@ -687,12 +688,21 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
> {
> const struct ni_board_struct *board = comedi_board(dev);
> struct ni_private *devpriv = dev->private;
> + unsigned long timeout;
>
> if (board->reg_type == ni_reg_6143) {
> /* Flush the 6143 data FIFO */
> ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
> ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
> - while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
> + /* Wait for complete */
> + timeout = jiffies + msec_to_jiffies(500);

Did you build this?

I think you meant to use msecs_to_jiffies().

Regards,
Hartley

2014-01-15 19:22:40

[permalink] [raw]

Subject: [PATCH v7] Staging: comedi: convert while loop to timeout in ni_mio_common.c

This patch for ni_mio_common.c changes out a while loop for a timeout,
which is preferred.

Signed-off-by: Chase Southwood <[email protected]>
---

Hartley,
I sincerely apologize for the obvious mistake, I thought I had built it
but clearly I made a mistake somewhere, as your observation is exactly
correct. It is now fixed.

Thanks,
Chase Southwood

2: Changed from simple clean-up to swapping a timeout in for a while loop.

3: Removed extra counter variable, and added error checking.

4: No longer using counter variable, using jiffies instead.

5: udelay for 10u, instead of 1u.

6: Scrap udelay entirely, in favor of cpu_relax. Include asm/processor.h
in order to use cpu_relax.

7: Fix typo (msec vs msecs).

drivers/staging/comedi/drivers/ni_mio_common.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 457b884..ab7a74c 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -55,6 +55,7 @@
#include <linux/interrupt.h>
#include <linux/sched.h>
#include <linux/delay.h>
+#include <asm/processor.h>
#include "8255.h"
#include "mite.h"
#include "comedi_fc.h"
@@ -687,12 +688,21 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
{
const struct ni_board_struct *board = comedi_board(dev);
struct ni_private *devpriv = dev->private;
+ unsigned long timeout;

if (board->reg_type == ni_reg_6143) {
/* Flush the 6143 data FIFO */
ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
- while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
+ /* Wait for complete */
+ timeout = jiffies + msecs_to_jiffies(500);
+ while (ni_readl(AIFIFO_Status_6143) & 0x10) {
+ if (time_after(jiffies, timeout)) {
+ comedi_error(dev, "FIFO flush timeout.");
+ break;
+ }
+ cpu_relax();
+ }
} else {
devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
if (board->reg_type == ni_reg_625x) {
--
1.8.4.2

2014-01-16 02:30:06

[permalink] [raw]

Subject: Re: [PATCH v4] Staging: comedi: convert while loop to timeout in ni_mio_common.c

On Wed, Jan 15, 2014 at 06:29:21PM +0000, Hartley Sweeten wrote:
> On Tuesday, January 14, 2014 8:59 PM, Greg KH wrote:
> > Sleep for at least 10, as I think that's the smallest time delay you can
> > sleep for anyway (meaning it will be that long no matter what number you
> > put there less than 10, depending on the hardware used of course.)
>
> A bit off topic here but I have a somewhat related question about timeouts.
>
> There are a number of comedi drivers that do a "wait for end-of-conversion"
> as part of the (*insn_read) for an analog input subdevice or (*insn_write) for
> an analog output subdevice. These functions return an errno if a timeout occurs.
>
> Currently either -ETIME or -ETIMEDOUT is returned. This errno ends up getting
> returned to the user as the result of the unlocked_ioctl file operation. What is
> the more appropriate errno? Or is there is better one that should be used?

I think they should all be -ETIMEDOUT, -ETIME is used for something
else, and shouldn't be sent to userspace as I don't think it knows what
to do with it.

thanks,

greg k-h

2014-01-16 11:08:29

[permalink] [raw]

Subject: Re: [PATCH v4] Staging: comedi: convert while loop to timeout in ni_mio_common.c

On 2014-01-16 02:30, Greg KH wrote:
> On Wed, Jan 15, 2014 at 06:29:21PM +0000, Hartley Sweeten wrote:
>> On Tuesday, January 14, 2014 8:59 PM, Greg KH wrote:
>>> Sleep for at least 10, as I think that's the smallest time delay you can
>>> sleep for anyway (meaning it will be that long no matter what number you
>>> put there less than 10, depending on the hardware used of course.)
>>
>> A bit off topic here but I have a somewhat related question about timeouts.
>>
>> There are a number of comedi drivers that do a "wait for end-of-conversion"
>> as part of the (*insn_read) for an analog input subdevice or (*insn_write) for
>> an analog output subdevice. These functions return an errno if a timeout occurs.
>>
>> Currently either -ETIME or -ETIMEDOUT is returned. This errno ends up getting
>> returned to the user as the result of the unlocked_ioctl file operation. What is
>> the more appropriate errno? Or is there is better one that should be used?
>
> I think they should all be -ETIMEDOUT, -ETIME is used for something
> else, and shouldn't be sent to userspace as I don't think it knows what
> to do with it.

Linux libc ought to deal with both. glibc 2.17 has the following error
strings in the "C" locale:

ETIMEDOUT --> "Connection timed out"
ETIME --> "Timer expired"

I think BSD has:

ETIMEDOUT --> "Operation timed out"
ETIME doesn't exist

so -ETIMEDOUT is better, even though the error string is a bit dodgy in
some cases under Linux. This betrays the original purpose of the
ETIMEDOUT error code, in the same way that the identifier and original
purpose of the ENOTTY error code is betrayed by the original error
string "Not a typewriter" (which is now "Inappropriate ioctl for device"
in current glibc).

--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-

2014-01-16 11:30:57

[permalink] [raw]

Subject: Re: [PATCH v7] Staging: comedi: convert while loop to timeout in ni_mio_common.c

On 2014-01-15 19:22, Chase Southwood wrote:
> This patch for ni_mio_common.c changes out a while loop for a timeout,
> which is preferred.
>
> Signed-off-by: Chase Southwood <[email protected]>
> ---
>
> Hartley,
> I sincerely apologize for the obvious mistake, I thought I had built it
> but clearly I made a mistake somewhere, as your observation is exactly
> correct. It is now fixed.
>
> Thanks,
> Chase Southwood
>
> 2: Changed from simple clean-up to swapping a timeout in for a while loop.
>
> 3: Removed extra counter variable, and added error checking.
>
> 4: No longer using counter variable, using jiffies instead.
>
> 5: udelay for 10u, instead of 1u.
>
> 6: Scrap udelay entirely, in favor of cpu_relax. Include asm/processor.h
> in order to use cpu_relax.
>
> 7: Fix typo (msec vs msecs).
>
> drivers/staging/comedi/drivers/ni_mio_common.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index 457b884..ab7a74c 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -55,6 +55,7 @@
> #include <linux/interrupt.h>
> #include <linux/sched.h>
> #include <linux/delay.h>
> +#include <asm/processor.h>
> #include "8255.h"
> #include "mite.h"
> #include "comedi_fc.h"
> @@ -687,12 +688,21 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
> {
> const struct ni_board_struct *board = comedi_board(dev);
> struct ni_private *devpriv = dev->private;
> + unsigned long timeout;
>
> if (board->reg_type == ni_reg_6143) {
> /* Flush the 6143 data FIFO */
> ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
> ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
> - while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
> + /* Wait for complete */
> + timeout = jiffies + msecs_to_jiffies(500);
> + while (ni_readl(AIFIFO_Status_6143) & 0x10) {
> + if (time_after(jiffies, timeout)) {
> + comedi_error(dev, "FIFO flush timeout.");
> + break;
> + }
> + cpu_relax();
> + }
> } else {
> devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
> if (board->reg_type == ni_reg_625x) {
>

Sorry this is your worst nightmare, Chase. Your patch is great and has
been modified in all the ways various people have suggested, but I don't
think it is suitable.

After examining the code, it turns out ni_clear_ai_fifo() is sometimes
called from an interrupt service routine and I don't think you can wait
for jiffies to change in that case. I think we need to go back to your
PATCH v2 and fix up the checkpatch.pl warning that Greg mentioned.

Sorry about that.

--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-

2014-01-16 17:49:32

[permalink] [raw]

Subject: Re: [PATCH v7] Staging: comedi: convert while loop to timeout in ni_mio_common.c

>On Thursday, January 16, 2014 5:31 AM, Ian Abbott <[email protected]> wrote:

>On 2014-01-15 19:22, Chase Southwood wrote:
>> This patch for ni_mio_common.c changes out a while loop for a timeout,
>> which is preferred.
>>
>> Signed-off-by: Chase Southwood <[email protected]>
>> ---
>>
>> Hartley,
>> I sincerely apologize for the obvious mistake, I thought I had built it
>> but clearly I made a mistake somewhere, as your observation is exactly
>> correct.? It is now fixed.
>>
>> Thanks,
>> Chase Southwood
>>
>> 2: Changed from simple clean-up to swapping a timeout in for a while loop.
>>
>> 3: Removed extra counter variable, and added error checking.
>>
>> 4: No longer using counter variable, using jiffies instead.
>>
>> 5: udelay for 10u, instead of 1u.
>>
>> 6: Scrap udelay entirely, in favor of cpu_relax. Include asm/processor.h
>> in order to use cpu_relax.
>>
>> 7: Fix typo (msec vs msecs).
>>
>>? drivers/staging/comedi/drivers/ni_mio_common.c | 12 +++++++++++-
>>? 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c >b/drivers/staging/comedi/drivers/ni_mio_common.c
>> index 457b884..ab7a74c 100644
>> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
>> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
>> @@ -55,6 +55,7 @@
>>? #include <linux/interrupt.h>
>>? #include <linux/sched.h>
>>? #include <linux/delay.h>
>> +#include <asm/processor.h>
>>? #include "8255.h"
>>? #include "mite.h"
>>? #include "comedi_fc.h"
>> @@ -687,12 +688,21 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
>>? {
>>? ??? const struct ni_board_struct *board = comedi_board(dev);
>>? ??? struct ni_private *devpriv = dev->private;
>> +??? unsigned long timeout;
>>
>>? ??? if (board->reg_type == ni_reg_6143) {
>>? ??? ??? /*? Flush the 6143 data FIFO */
>>? ??? ??? ni_writel(0x10, AIFIFO_Control_6143);??? /*? Flush fifo */
>>? ??? ??? ni_writel(0x00, AIFIFO_Control_6143);??? /*? Flush fifo */
>> -??? ??? while (ni_readl(AIFIFO_Status_6143) & 0x10) ;??? /*? Wait for complete */
>> +??? ??? /*? Wait for complete */
>> +??? ??? timeout = jiffies + msecs_to_jiffies(500);
>> +??? ??? while (ni_readl(AIFIFO_Status_6143) & 0x10) {
>> +??? ??? ??? if (time_after(jiffies, timeout)) {
>> +??? ??? ??? ??? comedi_error(dev, "FIFO flush timeout.");
>> +??? ??? ??? ??? break;
>> +??? ??? ??? }
>> +??? ??? ??? cpu_relax();
>> +??? ??? }
>>? ??? } else {
>>? ??? ??? devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
>>? ??? ??? if (board->reg_type == ni_reg_625x) {
>>

>Sorry this is your worst nightmare, Chase.? Your patch is great and has
>been modified in all the ways various people have suggested, but I don't
>think it is suitable.
>
>After examining the code, it turns out ni_clear_ai_fifo() is sometimes
>called from an interrupt service routine and I don't think you can wait
>for jiffies to change in that case.? I think we need to go back to your
>PATCH v2 and fix up the checkpatch.pl warning that Greg mentioned.
>
>Sorry about that.
>
>--
>-=( Ian Abbott @ MEV Ltd.? ? E-mail: <[email protected]>? ? ? ? )=-
>-=( Tel: +44 (0)161 477 1898? FAX: +44 (0)161 718 3587? ? ? ? )=-

It's absolutely no problem. ?I'll get v2 cleaned up and sent out right away!

Thanks,
Chase Southwood

2014-01-16 18:27:38

[permalink] [raw]

Subject: [PATCH v8] Staging: comedi: convert while loop to timeout in ni_mio_common.c

This patch for ni_mio_common.c changes out a while loop for a timeout,
which is preferred.

Signed-off-by: Chase Southwood <[email protected]>
---

Okay, back to v2, basically. I fixed the checkpatch warning from v2,
and added the error checking that was from v3, but otherwise it is the
same. Of note, I have used a udelay of 1 here (Greg had suggested to
use 10, but Ian prefers 1 so I have gone with that, and I assume that
cpu_relax is no longer an option.).

2: Changed from simple clean-up to swapping a timeout in for a while loop.

3: Removed extra counter variable, and added error checking.

4: No longer using counter variable, using jiffies instead.

5: udelay for 10u, instead of 1u.

6: Scrap udelay entirely, in favor of cpu_relax. Include asm/processor.h
in order to use cpu_relax.

7: Fix typo (msec vs msecs).

8: Revert back to v2, with some small changes (see above).

drivers/staging/comedi/drivers/ni_mio_common.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 457b884..10c27cb 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -687,12 +687,22 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
{
const struct ni_board_struct *board = comedi_board(dev);
struct ni_private *devpriv = dev->private;
+ static const int timeout = 10000;
+ int i;

if (board->reg_type == ni_reg_6143) {
/* Flush the 6143 data FIFO */
ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
- while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for complete */
+ /* Wait for complete */
+ for (i = 0; i < timeout; i++) {
+ if (!(ni_readl(AIFIFO_Status_6143) & 0x10))
+ break;
+ udelay(1);
+ }
+ if (i == timeout) {
+ comedi_error(dev, "FIFO flush timeout.");
+ }
} else {
devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
if (board->reg_type == ni_reg_625x) {
--
1.8.4.2

2014-01-17 13:12:56