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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 26 Mar 2009 17:43:25 +0100
From:	Joakim Tjernlund <Joakim.Tjernlund@...nsmode.se>
To:	avorontsov@...mvista.com
Cc:	leoli@...escale.com, linuxppc-dev@...abs.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH 2/2] ucc_geth: Rework the TX logic.

Anton Vorontsov <avorontsov@...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@...nsmode.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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html