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]

Message-ID: <20090326133918.GA27085@oksana.dev.rtsoft.ru>
Date:	Thu, 26 Mar 2009 16:39:18 +0300
From:	Anton Vorontsov <avorontsov@...mvista.com>
To:	Joakim Tjernlund <Joakim.Tjernlund@...nsmode.se>
Cc:	leoli@...escale.com, linuxppc-dev@...abs.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH 2/2] ucc_geth: Rework the TX logic.

Hi Joakim,

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.

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

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

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.

[...]
> +		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.

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

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@...il.com
irc://irc.freenode.net/bd2
--
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