[PATCH] gianfar: Wait for both RX and TX to stop

[PATCH] gianfar: Wait for both RX and TX to stop

[Andy Fleming]

When gracefully stopping the controller, the driver was continuing if
*either* RX or TX had stopped.  We need to wait for both, or the
controller could get into an invalid state.

Signed-off-by: Andy Fleming <afleming@freescale.com>
---
 drivers/net/gianfar.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 032073d..6038397 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1571,8 +1571,9 @@ static void gfar_halt_nodisable(struct net_device *dev)
 		tempval |= (DMACTRL_GRS | DMACTRL_GTS);
 		gfar_write(&regs->dmactrl, tempval);
 
-		while (!(gfar_read(&regs->ievent) &
-			 (IEVENT_GRSC | IEVENT_GTSC)))
+		while ((gfar_read(&regs->ievent) &
+			 (IEVENT_GRSC | IEVENT_GTSC)) !=
+			 (IEVENT_GRSC | IEVENT_GTSC))
 			cpu_relax();
 	}
 }
-- 
1.6.5.2.g6ff9a

Re: [PATCH] gianfar: Wait for both RX and TX to stop

[Timur Tabi]

On Sun, Apr 18, 2010 at 6:13 PM, Andy Fleming <afleming@freescale.com> wrote:

> -               while (!(gfar_read(&regs->ievent) &
> -                        (IEVENT_GRSC | IEVENT_GTSC)))
> +               while ((gfar_read(&regs->ievent) &
> +                        (IEVENT_GRSC | IEVENT_GTSC)) !=
> +                        (IEVENT_GRSC | IEVENT_GTSC))
>                        cpu_relax();

How about using spin_event_timeout()?  It streamlines this process and
includes a timeout.

The U-Boot version of this code doesn't have a timeout either, but
spin_event_timeout() is not available in U-Boot.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] gianfar: Wait for both RX and TX to stop

[Kumar Gala]

On Apr 19, 2010, at 4:08 PM, Timur Tabi wrote:

> On Sun, Apr 18, 2010 at 6:13 PM, Andy Fleming <afleming@freescale.com> wrote:
> 
>> -               while (!(gfar_read(&regs->ievent) &
>> -                        (IEVENT_GRSC | IEVENT_GTSC)))
>> +               while ((gfar_read(&regs->ievent) &
>> +                        (IEVENT_GRSC | IEVENT_GTSC)) !=
>> +                        (IEVENT_GRSC | IEVENT_GTSC))
>>                        cpu_relax();
> 
> How about using spin_event_timeout()?  It streamlines this process and
> includes a timeout.
> 
> The U-Boot version of this code doesn't have a timeout either, but
> spin_event_timeout() is not available in U-Boot.

spin_event_timeout doesn't make sense for this.  The patch is fine.

- k

Re: [PATCH] gianfar: Wait for both RX and TX to stop

[Timur Tabi]

On Mon, Apr 19, 2010 at 11:43 PM, Kumar Gala <galak@kernel.crashing.org> wrote:

> spin_event_timeout doesn't make sense for this.  The patch is fine.

Can you please elaborate on that?  I don't understand why you think
that.  spin_event_timeout() takes an expression and a timeout, and
loops over the expression calling cpu_relax(), just like this loop
does.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] gianfar: Wait for both RX and TX to stop

[Timur Tabi]

On Tue, Apr 20, 2010 at 10:01 AM, Timur Tabi <timur.tabi@gmail.com> wrote:
> On Mon, Apr 19, 2010 at 11:43 PM, Kumar Gala <galak@kernel.crashing.org> wrote:
>
>> spin_event_timeout doesn't make sense for this.  The patch is fine.
>
> Can you please elaborate on that?  I don't understand why you think
> that.  spin_event_timeout() takes an expression and a timeout, and
> loops over the expression calling cpu_relax(), just like this loop
> does.

I haven't tested it, but I think this should work:

	spin_event_timeout((gfar_read(&regs->ievent) &
		(IEVENT_GRSC | IEVENT_GTSC)) ==
		(IEVENT_GRSC | IEVENT_GTSC), -1, 0);

Ideally, Andy should use a timeout value other than -1, and then test
the result like this:

	u32 ret;

	ret = spin_event_timeout((gfar_read(&regs->ievent) &
		(IEVENT_GRSC | IEVENT_GTSC)) ==
		(IEVENT_GRSC | IEVENT_GTSC), 1000, 0);

	if (!ret)
		/* timeout has occurred */

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] gianfar: Wait for both RX and TX to stop

[David Miller]

From: Timur Tabi <timur.tabi@gmail.com>
Date: Tue, 20 Apr 2010 10:01:48 -0500

> On Mon, Apr 19, 2010 at 11:43 PM, Kumar Gala <galak@kernel.crashing.org> wrote:
> 
>> spin_event_timeout doesn't make sense for this.  The patch is fine.
> 
> Can you please elaborate on that?  I don't understand why you think
> that.  spin_event_timeout() takes an expression and a timeout, and
> loops over the expression calling cpu_relax(), just like this loop
> does.

Indeed it does, Kumar this request seems reasonable.

Re: [PATCH] gianfar: Wait for both RX and TX to stop

[Kumar Gala]

On Apr 20, 2010, at 8:06 PM, David Miller wrote:

> From: Timur Tabi <timur.tabi@gmail.com>
> Date: Tue, 20 Apr 2010 10:01:48 -0500
> 
>> On Mon, Apr 19, 2010 at 11:43 PM, Kumar Gala <galak@kernel.crashing.org> wrote:
>> 
>>> spin_event_timeout doesn't make sense for this.  The patch is fine.
>> 
>> Can you please elaborate on that?  I don't understand why you think
>> that.  spin_event_timeout() takes an expression and a timeout, and
>> loops over the expression calling cpu_relax(), just like this loop
>> does.
> 
> Indeed it does, Kumar this request seems reasonable.

Are we saying that cpu_relax() is useless and should be removed if we are spinning on a HW register?

Its fatally buggy HW if the bits never clear or get set in the few conditions that cpu_relax() are being used.

- k

Re: [PATCH] gianfar: Wait for both RX and TX to stop

[David Miller]

From: Kumar Gala <galak@kernel.crashing.org>
Date: Tue, 20 Apr 2010 23:22:19 -0500

> 
> On Apr 20, 2010, at 8:06 PM, David Miller wrote:
> 
>> From: Timur Tabi <timur.tabi@gmail.com>
>> Date: Tue, 20 Apr 2010 10:01:48 -0500
>> 
>>> On Mon, Apr 19, 2010 at 11:43 PM, Kumar Gala <galak@kernel.crashing.org> wrote:
>>> 
>>>> spin_event_timeout doesn't make sense for this.  The patch is fine.
>>> 
>>> Can you please elaborate on that?  I don't understand why you think
>>> that.  spin_event_timeout() takes an expression and a timeout, and
>>> loops over the expression calling cpu_relax(), just like this loop
>>> does.
>> 
>> Indeed it does, Kumar this request seems reasonable.
> 
> Are we saying that cpu_relax() is useless and should be removed if we are spinning on a HW register?

Kumar, take a deep breath and a step back.

spin_event_timeout() does the cpu_relax() too, that's what Timur is
trying to tell you.

The code will be basically identical as far as I can tell.

Re: [PATCH] gianfar: Wait for both RX and TX to stop

[Kumar Gala]

On Apr 21, 2010, at 12:36 AM, David Miller wrote:

> From: Kumar Gala <galak@kernel.crashing.org>
> Date: Tue, 20 Apr 2010 23:22:19 -0500
> 
>> 
>> On Apr 20, 2010, at 8:06 PM, David Miller wrote:
>> 
>>> From: Timur Tabi <timur.tabi@gmail.com>
>>> Date: Tue, 20 Apr 2010 10:01:48 -0500
>>> 
>>>> On Mon, Apr 19, 2010 at 11:43 PM, Kumar Gala <galak@kernel.crashing.org> wrote:
>>>> 
>>>>> spin_event_timeout doesn't make sense for this.  The patch is fine.
>>>> 
>>>> Can you please elaborate on that?  I don't understand why you think
>>>> that.  spin_event_timeout() takes an expression and a timeout, and
>>>> loops over the expression calling cpu_relax(), just like this loop
>>>> does.
>>> 
>>> Indeed it does, Kumar this request seems reasonable.
>> 
>> Are we saying that cpu_relax() is useless and should be removed if we are spinning on a HW register?
> 
> Kumar, take a deep breath and a step back.
> 
> spin_event_timeout() does the cpu_relax() too, that's what Timur is
> trying to tell you.
> 
> The code will be basically identical as far as I can tell.

I understand, its more a sense that we are saying we want to time out for what I consider a catastrophic HW failure.

- k

Re: [PATCH] gianfar: Wait for both RX and TX to stop

[Timur Tabi]

On Wed, Apr 21, 2010 at 7:17 AM, Kumar Gala <galak@kernel.crashing.org> wrote:

> I understand, its more a sense that we are saying we want to time out for what I consider a catastrophic HW failure.

And how else will you detect and recover from such a failure without a
timeout?  And are you absolutely certain that there will never be a
programming failure that will cause this loop to spin forever?

If you're really opposed to a timeout, you can still use
spin_event_timeout() by just setting the timeout to -1 and adding a
comment explaining why.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] gianfar: Wait for both RX and TX to stop

[Kumar Gala]

On Apr 21, 2010, at 9:33 AM, Timur Tabi wrote:

> On Wed, Apr 21, 2010 at 7:17 AM, Kumar Gala <galak@kernel.crashing.org> wrote:
> 
>> I understand, its more a sense that we are saying we want to time out for what I consider a catastrophic HW failure.
> 
> And how else will you detect and recover from such a failure without a
> timeout?  And are you absolutely certain that there will never be a
> programming failure that will cause this loop to spin forever?
> 
> If you're really opposed to a timeout, you can still use
> spin_event_timeout() by just setting the timeout to -1 and adding a
> comment explaining why.

I'm not opposed, I'm just asking if we are saying we shouldn't be using cpu_relax() for spinning on HW status registers ever.

If we are suggesting that cpu_relax() shouldn't be used in these scenarios going forward I'm ok w/the change you suggest and starting to convert other cpu_relax() calls to use spin_event_timeout()

- k

Re: [PATCH] gianfar: Wait for both RX and TX to stop

[David Miller]

From: Kumar Gala <galak@kernel.crashing.org>
Date: Wed, 21 Apr 2010 14:13:00 -0500

> I'm not opposed, I'm just asking if we are saying we shouldn't be using cpu_relax() for spinning on HW status registers ever.
> 
> If we are suggesting that cpu_relax() shouldn't be used in these scenarios going forward I'm ok w/the change you suggest and starting to convert other cpu_relax() calls to use spin_event_timeout()


Kumar this isn't an either-or thing.

In both cases we're using cpu_relax().

But by using spin_event_timeout() you're getting both the cpu_relax()
and a break-out in case the hardware gets wedged for some reason.

Re: [PATCH] gianfar: Wait for both RX and TX to stop

[Kumar Gala]

On Apr 18, 2010, at 6:13 PM, Andy Fleming wrote:

> When gracefully stopping the controller, the driver was continuing if
> *either* RX or TX had stopped.  We need to wait for both, or the
> controller could get into an invalid state.
> 
> Signed-off-by: Andy Fleming <afleming@freescale.com>
> ---
> drivers/net/gianfar.c |    5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)

Acked-by: Kumar Gala <galak@kernel.crashing.org>

(please pick this up for 2.6.34, fixes an annoying bug).

- k
 
> 
> diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
> index 032073d..6038397 100644
> --- a/drivers/net/gianfar.c
> +++ b/drivers/net/gianfar.c
> @@ -1571,8 +1571,9 @@ static void gfar_halt_nodisable(struct net_device *dev)
> 		tempval |= (DMACTRL_GRS | DMACTRL_GTS);
> 		gfar_write(&regs->dmactrl, tempval);
> 
> -		while (!(gfar_read(&regs->ievent) &
> -			 (IEVENT_GRSC | IEVENT_GTSC)))
> +		while ((gfar_read(&regs->ievent) &
> +			 (IEVENT_GRSC | IEVENT_GTSC)) !=
> +			 (IEVENT_GRSC | IEVENT_GTSC))
> 			cpu_relax();
> 	}
> }
> -- 
> 1.6.5.2.g6ff9a
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gianfar: Wait for both RX and TX to stop

[David Miller]

From: Kumar Gala <galak@kernel.crashing.org>
Date: Mon, 19 Apr 2010 23:44:49 -0500

> 
> On Apr 18, 2010, at 6:13 PM, Andy Fleming wrote:
> 
>> When gracefully stopping the controller, the driver was continuing if
>> *either* RX or TX had stopped.  We need to wait for both, or the
>> controller could get into an invalid state.
>> 
>> Signed-off-by: Andy Fleming <afleming@freescale.com>
>> ---
>> drivers/net/gianfar.c |    5 +++--
>> 1 files changed, 3 insertions(+), 2 deletions(-)
> 
> Acked-by: Kumar Gala <galak@kernel.crashing.org>
> 
> (please pick this up for 2.6.34, fixes an annoying bug).

I will do this tomorrow, thanks!