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.cThu Jul  5 16:10:16 2007 -0700
+++ b/drivers/net/7990.cFri 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


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

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
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.cThu Jul  5 16:10:16 2007 -0700
+++ b/drivers/net/7990.cFri 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.


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

[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 <[EMAIL PROTECTED]>

diff -r 6c0a10cc415a drivers/net/7990.c
--- a/drivers/net/7990.cThu Jul  5 16:10:16 2007 -0700
+++ b/drivers/net/7990.cFri 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 < (1btx_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 < (1brx_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);
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html