[PATCH] 7990 : Various fixes and cleanups

[PATCH] 7990 : Various fixes and cleanups

[Philippe De Muyter]

Hi all,

This patch
- avoids 7990 blocking when no tx buffer is available,
- implements tx_bytes statistic that was missing
- sets skb->dev before calling netix_rx()
- improves readability and code efficiency in buffer rings initialisation
- avoids useless memset part for tx packets smaller than ETH_ZLEN

Signed-off-by: Philippe De Muyter <phdm@macqel.be>

diff -r 6c0a10cc415a drivers/net/7990.c
--- a/drivers/net/7990.c	Thu Jul  5 16:10:16 2007 -0700
+++ b/drivers/net/7990.c	Fri Jul  6 11:27:20 2007 +0200
@@ -179,12 +179,14 @@ static void lance_init_ring (struct net_
 	lp->tx_full = 0;
         /* Setup the Tx ring entries */
         for (i = 0; i < (1<<lp->lance_log_tx_bufs); i++) {
+		volatile struct lance_tx_desc *td;
+                td = &ib->btx_ring [i];
                 leptr = LANCE_ADDR(&aib->tx_buf[i][0]);
-                ib->btx_ring [i].tmd0      = leptr;
-                ib->btx_ring [i].tmd1_hadr = leptr >> 16;
-                ib->btx_ring [i].tmd1_bits = 0;
-                ib->btx_ring [i].length    = 0xf000; /* The ones required by tmd2 */
-                ib->btx_ring [i].misc      = 0;
+                td->tmd0      = leptr;
+                td->tmd1_hadr = leptr >> 16;
+                td->tmd1_bits = 0;
+                td->length    = 0xf000; /* The ones required by tmd2 */
+                td->misc      = 0;
                 if (DEBUG_IRING)
                    printk ("%d: 0x%8.8x\n", i, leptr);
         }
@@ -193,14 +195,16 @@ static void lance_init_ring (struct net_
         if (DEBUG_IRING)
                 printk ("RX rings:\n");
         for (i = 0; i < (1<<lp->lance_log_rx_bufs); i++) {
+		volatile struct lance_rx_desc *rd;
+                rd = &ib->brx_ring [i];
                 leptr = LANCE_ADDR(&aib->rx_buf[i][0]);
 
-                ib->brx_ring [i].rmd0      = leptr;
-                ib->brx_ring [i].rmd1_hadr = leptr >> 16;
-                ib->brx_ring [i].rmd1_bits = LE_R1_OWN;
+                rd->rmd0      = leptr;
+                rd->rmd1_hadr = leptr >> 16;
+                rd->rmd1_bits = LE_R1_OWN;
                 /* 0xf000 == bits that must be one (reserved, presumably) */
-                ib->brx_ring [i].length    = -RX_BUFF_SIZE | 0xf000;
-                ib->brx_ring [i].mblength  = 0;
+                rd->length    = -RX_BUFF_SIZE | 0xf000;
+                rd->mblength  = 0;
                 if (DEBUG_IRING)
                         printk ("%d: 0x%8.8x\n", i, leptr);
         }
@@ -331,6 +335,7 @@ static int lance_rx (struct net_device *
                                 return 0;
                         }
 
+                        skb->dev = dev;
                         skb_reserve (skb, 2);           /* 16 byte align */
                         skb_put (skb, len);             /* make room */
                         eth_copy_and_sum(skb,
@@ -541,9 +546,6 @@ int lance_start_xmit (struct sk_buff *sk
         static int outs;
 	unsigned long flags;
 
-        if (!TX_BUFFS_AVAIL)
-                return -1;
-
 	netif_stop_queue (dev);
 
         skblen = skb->len;
@@ -565,9 +567,11 @@ int lance_start_xmit (struct sk_buff *sk
         ib->btx_ring [entry].length = (-len) | 0xf000;
         ib->btx_ring [entry].misc = 0;
 
-	if (skb->len < ETH_ZLEN)
-		memset((void *)&ib->tx_buf[entry][0], 0, ETH_ZLEN);
         skb_copy_from_linear_data(skb, (void *)&ib->tx_buf[entry][0], skblen);
+    	if (skblen < ETH_ZLEN)
+    		memset((char *)&ib->tx_buf[entry][0] + skblen, 0, ETH_ZLEN - skblen);
+
+	lp->stats.tx_bytes += skblen;
 
         /* Now, give the packet to the lance */
         ib->btx_ring [entry].tmd1_bits = (LE_T1_POK|LE_T1_OWN);

Re: [PATCH] 7990 : Various fixes and cleanups

[Jeff Garzik]

Philippe De Muyter wrote:
> This patch
> - avoids 7990 blocking when no tx buffer is available,
[...]
> diff -r 6c0a10cc415a drivers/net/7990.c
> --- a/drivers/net/7990.c	Thu Jul  5 16:10:16 2007 -0700
> +++ b/drivers/net/7990.c	Fri Jul  6 11:27:20 2007 +0200
[...]
> @@ -541,9 +546,6 @@ int lance_start_xmit (struct sk_buff *sk
>          static int outs;
>  	unsigned long flags;
>  
> -        if (!TX_BUFFS_AVAIL)
> -                return -1;
> -
>  	netif_stop_queue (dev);
>  
>          skblen = skb->len;
> @@ -565,9 +567,11 @@ int lance_start_xmit (struct sk_buff *sk
>          ib->btx_ring [entry].length = (-len) | 0xf000;
>          ib->btx_ring [entry].misc = 0;
>  
> -	if (skb->len < ETH_ZLEN)
> -		memset((void *)&ib->tx_buf[entry][0], 0, ETH_ZLEN);
>          skb_copy_from_linear_data(skb, (void *)&ib->tx_buf[entry][0], skblen);
> +    	if (skblen < ETH_ZLEN)
> +    		memset((char *)&ib->tx_buf[entry][0] + skblen, 0, ETH_ZLEN - skblen);
> +
> +	lp->stats.tx_bytes += skblen;


NAK

It "avoids" by removing an overrun check in hard_start_xmit that should 
not be removed.

Re: [PATCH] 7990 : Various fixes and cleanups

[Philippe De Muyter]

On Tue, Jul 10, 2007 at 12:38:45PM -0400, Jeff Garzik wrote:
> Philippe De Muyter wrote:
> >This patch
> >- avoids 7990 blocking when no tx buffer is available,
> [...]
> >diff -r 6c0a10cc415a drivers/net/7990.c
> >--- a/drivers/net/7990.c	Thu Jul  5 16:10:16 2007 -0700
> >+++ b/drivers/net/7990.c	Fri Jul  6 11:27:20 2007 +0200
> [...]
> >@@ -541,9 +546,6 @@ int lance_start_xmit (struct sk_buff *sk
> >         static int outs;
> > 	unsigned long flags;
> > 
> >-        if (!TX_BUFFS_AVAIL)
> >-                return -1;
> >-
> > 	netif_stop_queue (dev);
> > 
> >         skblen = skb->len;
> 
> 
> NAK
> 
> It "avoids" by removing an overrun check in hard_start_xmit that should 
> not be removed.

Yup, sorry.

The real fact is still that this prevents/fixes lance/driver blocking on my
board, while the tx_timeout mechanism does not succeed at that, and that
on my board the driver is blocked when we return -1 on !TX_BUFFS_AVAIL.

I'll investigate why.

Philippe

PS : did you apply the rest of the patch ?

-- 
Philippe De Muyter  phdm at macqel dot be  Tel +32 27029044
Macq Electronique SA  rue de l'Aeronef 2  B-1140 Bruxelles  Fax +32 27029077

Re: [PATCH] 7990 : Various fixes and cleanups

[Jeff Garzik]

Philippe De Muyter wrote:
> On Tue, Jul 10, 2007 at 12:38:45PM -0400, Jeff Garzik wrote:
>> Philippe De Muyter wrote:
>>> This patch
>>> - avoids 7990 blocking when no tx buffer is available,
>> [...]
>>> diff -r 6c0a10cc415a drivers/net/7990.c
>>> --- a/drivers/net/7990.c	Thu Jul  5 16:10:16 2007 -0700
>>> +++ b/drivers/net/7990.c	Fri Jul  6 11:27:20 2007 +0200
>> [...]
>>> @@ -541,9 +546,6 @@ int lance_start_xmit (struct sk_buff *sk
>>>         static int outs;
>>> 	unsigned long flags;
>>>
>>> -        if (!TX_BUFFS_AVAIL)
>>> -                return -1;
>>> -
>>> 	netif_stop_queue (dev);
>>>
>>>         skblen = skb->len;
>>
>> NAK
>>
>> It "avoids" by removing an overrun check in hard_start_xmit that should 
>> not be removed.
> 
> Yup, sorry.
> 
> The real fact is still that this prevents/fixes lance/driver blocking on my
> board, while the tx_timeout mechanism does not succeed at that, and that
> on my board the driver is blocked when we return -1 on !TX_BUFFS_AVAIL.

Note that it should be returning a NETDEV_TX_xxx return value, which may 
be confusing the net stack.  You have to let it know what happened to 
the skb passed to ->hard_start_xmit(), which is normally the 
responsibility of the ->hard_start_xmit() hook to free or queue as 
conditions warrant.

Yeah, you will need to investigate further what's going on here.


> PS : did you apply the rest of the patch ?

No, I don't apply partial patches.  You are welcome to resubmit a patch 
containing the non-controversial changes.  In fact, it's normal and 
encouraged in Linux to submit multiple patches for different logical 
changes.  Splitting cleanups and a TX code path change into two separate 
patches is certainly the best way to go.  If there is a problem, that 
allows users to use 'git bisect' to quickly locate which specific patch 
caused the problem.  If patches are split up properly, good-or-bad 
changes are identified more rapidly.

	Jeff