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

--

[ message continues ]

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

[ message continues ]

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

- k--

[ message continues ]

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

[ message continues ]

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

[ message continues ]

From: Timur Tabi <timur.tabi@gmail.com>

Indeed it does, Kumar this request seems reasonable.
--

[ message continues ]

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

[ message continues ]

From: Kumar Gala <galak@kernel.crashing.org>

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

[ message continues ]

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

- k--

[ message continues ]

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

[ message continues ]

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

[ message continues ]

From: Kumar Gala <galak@kernel.crashing.org>


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

[ message continues ]

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

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


--

[ message continues ]

From: Kumar Gala <galak@kernel.crashing.org>

I will do this tomorrow, thanks!
--

[ message continues ]