* [PATCH] gianfar: Wait for both RX and TX to stop
@ 2010-04-18 23:13 Andy Fleming
2010-04-19 21:08 ` Timur Tabi
2010-04-20 4:44 ` Kumar Gala
0 siblings, 2 replies; 14+ messages in thread
From: Andy Fleming @ 2010-04-18 23:13 UTC (permalink / raw)
To: davem; +Cc: netdev
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(®s->dmactrl, tempval);
- while (!(gfar_read(®s->ievent) &
- (IEVENT_GRSC | IEVENT_GTSC)))
+ while ((gfar_read(®s->ievent) &
+ (IEVENT_GRSC | IEVENT_GTSC)) !=
+ (IEVENT_GRSC | IEVENT_GTSC))
cpu_relax();
}
}
--
1.6.5.2.g6ff9a
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] gianfar: Wait for both RX and TX to stop
2010-04-18 23:13 [PATCH] gianfar: Wait for both RX and TX to stop Andy Fleming
@ 2010-04-19 21:08 ` Timur Tabi
2010-04-20 4:43 ` Kumar Gala
2010-04-20 4:44 ` Kumar Gala
1 sibling, 1 reply; 14+ messages in thread
From: Timur Tabi @ 2010-04-19 21:08 UTC (permalink / raw)
To: Andy Fleming; +Cc: davem, netdev
On Sun, Apr 18, 2010 at 6:13 PM, Andy Fleming <afleming@freescale.com> wrote:
> - while (!(gfar_read(®s->ievent) &
> - (IEVENT_GRSC | IEVENT_GTSC)))
> + while ((gfar_read(®s->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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gianfar: Wait for both RX and TX to stop
2010-04-19 21:08 ` Timur Tabi
@ 2010-04-20 4:43 ` Kumar Gala
2010-04-20 15:01 ` Timur Tabi
0 siblings, 1 reply; 14+ messages in thread
From: Kumar Gala @ 2010-04-20 4:43 UTC (permalink / raw)
To: Timur Tabi; +Cc: Andy Fleming, davem, netdev
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(®s->ievent) &
>> - (IEVENT_GRSC | IEVENT_GTSC)))
>> + while ((gfar_read(®s->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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gianfar: Wait for both RX and TX to stop
2010-04-18 23:13 [PATCH] gianfar: Wait for both RX and TX to stop Andy Fleming
2010-04-19 21:08 ` Timur Tabi
@ 2010-04-20 4:44 ` Kumar Gala
2010-04-20 8:18 ` David Miller
1 sibling, 1 reply; 14+ messages in thread
From: Kumar Gala @ 2010-04-20 4:44 UTC (permalink / raw)
To: David Miller; +Cc: Netdev, Andy Fleming
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(®s->dmactrl, tempval);
>
> - while (!(gfar_read(®s->ievent) &
> - (IEVENT_GRSC | IEVENT_GTSC)))
> + while ((gfar_read(®s->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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gianfar: Wait for both RX and TX to stop
2010-04-20 4:44 ` Kumar Gala
@ 2010-04-20 8:18 ` David Miller
0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2010-04-20 8:18 UTC (permalink / raw)
To: galak; +Cc: netdev, afleming
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!
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gianfar: Wait for both RX and TX to stop
2010-04-20 4:43 ` Kumar Gala
@ 2010-04-20 15:01 ` Timur Tabi
2010-04-20 15:59 ` Timur Tabi
2010-04-21 1:06 ` David Miller
0 siblings, 2 replies; 14+ messages in thread
From: Timur Tabi @ 2010-04-20 15:01 UTC (permalink / raw)
To: Kumar Gala; +Cc: Andy Fleming, davem, netdev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gianfar: Wait for both RX and TX to stop
2010-04-20 15:01 ` Timur Tabi
@ 2010-04-20 15:59 ` Timur Tabi
2010-04-21 1:06 ` David Miller
1 sibling, 0 replies; 14+ messages in thread
From: Timur Tabi @ 2010-04-20 15:59 UTC (permalink / raw)
To: Kumar Gala; +Cc: Andy Fleming, davem, netdev
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(®s->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(®s->ievent) &
(IEVENT_GRSC | IEVENT_GTSC)) ==
(IEVENT_GRSC | IEVENT_GTSC), 1000, 0);
if (!ret)
/* timeout has occurred */
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gianfar: Wait for both RX and TX to stop
2010-04-20 15:01 ` Timur Tabi
2010-04-20 15:59 ` Timur Tabi
@ 2010-04-21 1:06 ` David Miller
2010-04-21 4:22 ` Kumar Gala
1 sibling, 1 reply; 14+ messages in thread
From: David Miller @ 2010-04-21 1:06 UTC (permalink / raw)
To: timur.tabi; +Cc: galak, afleming, netdev
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gianfar: Wait for both RX and TX to stop
2010-04-21 1:06 ` David Miller
@ 2010-04-21 4:22 ` Kumar Gala
2010-04-21 5:36 ` David Miller
0 siblings, 1 reply; 14+ messages in thread
From: Kumar Gala @ 2010-04-21 4:22 UTC (permalink / raw)
To: David Miller; +Cc: timur.tabi, afleming, netdev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gianfar: Wait for both RX and TX to stop
2010-04-21 4:22 ` Kumar Gala
@ 2010-04-21 5:36 ` David Miller
2010-04-21 12:17 ` Kumar Gala
0 siblings, 1 reply; 14+ messages in thread
From: David Miller @ 2010-04-21 5:36 UTC (permalink / raw)
To: galak; +Cc: timur.tabi, afleming, netdev
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gianfar: Wait for both RX and TX to stop
2010-04-21 5:36 ` David Miller
@ 2010-04-21 12:17 ` Kumar Gala
2010-04-21 14:33 ` Timur Tabi
0 siblings, 1 reply; 14+ messages in thread
From: Kumar Gala @ 2010-04-21 12:17 UTC (permalink / raw)
To: David Miller; +Cc: timur.tabi, afleming, netdev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gianfar: Wait for both RX and TX to stop
2010-04-21 12:17 ` Kumar Gala
@ 2010-04-21 14:33 ` Timur Tabi
2010-04-21 19:13 ` Kumar Gala
0 siblings, 1 reply; 14+ messages in thread
From: Timur Tabi @ 2010-04-21 14:33 UTC (permalink / raw)
To: Kumar Gala; +Cc: David Miller, afleming, netdev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gianfar: Wait for both RX and TX to stop
2010-04-21 14:33 ` Timur Tabi
@ 2010-04-21 19:13 ` Kumar Gala
2010-04-21 21:33 ` David Miller
0 siblings, 1 reply; 14+ messages in thread
From: Kumar Gala @ 2010-04-21 19:13 UTC (permalink / raw)
To: Timur Tabi; +Cc: David Miller, afleming, netdev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gianfar: Wait for both RX and TX to stop
2010-04-21 19:13 ` Kumar Gala
@ 2010-04-21 21:33 ` David Miller
0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2010-04-21 21:33 UTC (permalink / raw)
To: galak; +Cc: timur.tabi, afleming, netdev
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2010-04-21 21:33 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-04-18 23:13 [PATCH] gianfar: Wait for both RX and TX to stop Andy Fleming
2010-04-19 21:08 ` Timur Tabi
2010-04-20 4:43 ` Kumar Gala
2010-04-20 15:01 ` Timur Tabi
2010-04-20 15:59 ` Timur Tabi
2010-04-21 1:06 ` David Miller
2010-04-21 4:22 ` Kumar Gala
2010-04-21 5:36 ` David Miller
2010-04-21 12:17 ` Kumar Gala
2010-04-21 14:33 ` Timur Tabi
2010-04-21 19:13 ` Kumar Gala
2010-04-21 21:33 ` David Miller
2010-04-20 4:44 ` Kumar Gala
2010-04-20 8:18 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.