[PATCH 2/2] ucc_geth: Rework the TX logic.

[Joakim Tjernlund]

Anton Vorontsov <avorontsov at ru.mvista.com> 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:
> > 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 <Joakim.Tjernlund at transmode.se>
> > ---
> >  drivers/net/ucc_geth.c |   40 
++++++++++++++++++++--------------------
> >  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)
> >     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.

> 
> [...]
> >     /* 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. 
*/
> > -   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 :(

> 
> [...]
> > +      if (!in_be32((u32 __iomem *)(bd+4)))
> [...]
> > +      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.