Anton Vorontsov [off-list ref] wrote on 26/03/2009 14:39:18:

Hi Joakim,

Hi Anton

On Thu, Mar 26, 2009 at 01:54:37PM +0100, Joakim Tjernlund wrote:

quoted

The line:
 if ((bd == ugeth->txBd[txQ]) && (netif_queue_stopped(dev) == 0))
       break;
in ucc_geth_tx() didn not make sense to me. Rework & cleanup
this logic to something understandable.

Signed-off-by: Joakim Tjernlund <redacted>
---
 drivers/net/ucc_geth.c |   40 

++++++++++++++++++++--------------------

quoted

 1 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index 7fc91aa..b6ebefd 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c

@@ -3048,6 +3048,7 @@ static int ucc_geth_start_xmit(struct sk_buff 

*skb, struct net_device *dev)

quoted

    u8 __iomem *bd;         /* BD pointer */
    u32 bd_status;
    u8 txQ = 0;
+   int txInd;

camelCase should not be used in Linux.

Surely, the driver is full of camelCases... though, it should be
fixed, not encouraged further.

OK, I will rename to tx_ind instead.

And btw, there is even Hungarian notation in the driver. :-(

Hopefully that will go away in time.

[...]

quoted

    /* If the next BD still needs to be cleaned up, then the bds
       are full.  We need to tell the kernel to stop sending us stuff. 

*/

quoted

-   if (bd == ugeth->confBd[txQ]) {
-      if (!netif_queue_stopped(dev))
-         netif_stop_queue(dev);
+   if (!in_be32((u32 __iomem *)(bd+4))) {

bd == ugeth->confBd[txQ]
and
!in_be32((u32 __iomem *)(bd+4))

Are not equivalent wrt. speed. MMIO accessors should be rather
slow comparing to normal memory.

Yes, I know. I did it this way because I something broke under stress
when ugeth->confBd[txQ] instead. The ucc_geth_tx() and 
ucc_geth_start_xmit()
gets out of sync somehow.

We should really do some performance tests (using gbit links).
I'll try to help you with the tests, but it might take some time.

Good, because I don't have GBE links :(

[...]

quoted

+      if (!in_be32((u32 __iomem *)(bd+4)))

[...]

quoted

+      out_be32((u32 __iomem *)(bd+4), (int)NULL); /* Mark it free */

How about some inline function that will self-document the bd + 4
stuff? Plus that way we'll get rid of the casts.

Good idea, will look at that.

Note that "bd+4" should be "bd + 4". And (int)NULL makes
little sense, just 0 will work.

Sure, done.