[PATCH 2/3] r8169: code clean-up

This patch removes netif_poll_disable if NAPI is not enabled (otherwise 
adapter will hang while changing MTUs).  This patch also fixes a possible skb
alignment overrun, and fixes the rx skb allocation error logging. It also 
removes an unnecessary define, adds a link down notification, and cleans up 
some comments. 

It is all pretty trivial, but let me know if this is too much for one patch.

Tested on amd64 (and very lightly tested on x86).

Applies cleanly to linux-2.6.11-rc2-mm2 version of the driver.

Signed-off-by: Jon Mason <redacted>

--- drivers/net/r8169.c.orig 2005-02-16 17:16:19.000000000 -0600
+++ drivers/net/r8169.c 2005-02-16 17:17:40.000000000 -0600

@@ -84,10 +84,12 @@ VERSION 1.6LK <2004/04/14>
 #define rtl8169_rx_skb   netif_receive_skb
 #define rtl8169_rx_hwaccel_skb  vlan_hwaccel_rx
 #define rtl8169_rx_quota(count, quota) min(count, quota)
+#define netif_poll_disable(dev)  netif_poll_disable(dev)
 #else
 #define rtl8169_rx_skb   netif_rx
 #define rtl8169_rx_hwaccel_skb  vlan_hwaccel_receive_skb
 #define rtl8169_rx_quota(count, quota) count
+#define netif_poll_disable(dev)
 #endif
 
 /* media options */

@@ -102,11 +104,9 @@ static int max_interrupt_work = 20;
    The RTL chips use a 64 element hash table based on the Ethernet CRC.  */
 static int multicast_filter_limit = 32;
 
-/* MAC address length*/
+/* MAC address length */
 #define MAC_ADDR_LEN 6
 
-#define TX_FIFO_THRESH 256 /* In bytes */
-
 #define RX_FIFO_THRESH 7 /* 7 means NO threshold, Rx buffer level before first PCI xfer.  */
 #define RX_DMA_BURST 6 /* Maximum PCI burst, '6' is 1024 */
 #define TX_DMA_BURST 6 /* Maximum PCI burst, '6' is 1024 */

@@ -521,8 +521,10 @@ static void rtl8169_check_link_status(st
  if (tp->link_ok(ioaddr)) {
   netif_carrier_on(dev);
   printk(KERN_INFO PFX "%s: link up\n", dev->name);
- } else
+ } else {
   netif_carrier_off(dev);
+  printk(KERN_INFO PFX "%s: link down\n", dev->name);
+ } 
  spin_unlock_irqrestore(&tp->lock, flags);
 }
 

@@ -1549,10 +1551,10 @@ rtl8169_hw_start(struct net_device *dev)
  RTL_W8(ChipCmd, CmdTxEnb | CmdRxEnb);
  RTL_W8(EarlyTxThres, EarlyTxThld);
 
- // For gigabit rtl8169, MTU + header + CRC + VLAN
+ /* For gigabit rtl8169, MTU + header + CRC + VLAN */
  RTL_W16(RxMaxSize, tp->rx_buf_sz);
 
- // Set Rx Config register
+ /* Set Rx Config register */
  i = rtl8169_rx_config |
   (RTL_R32(RxConfig) & rtl_chip_info[tp->chipset].RxConfigMask);
  RTL_W32(RxConfig, i);

@@ -1659,11 +1661,11 @@ static int rtl8169_alloc_rx_skb(struct p
  dma_addr_t mapping;
  int ret = 0;
 
- skb = dev_alloc_skb(rx_buf_sz);
+ skb = dev_alloc_skb(rx_buf_sz + NET_IP_ALIGN);
  if (!skb)
   goto err_out;
 
- skb_reserve(skb, 2);
+ skb_reserve(skb, NET_IP_ALIGN);
  *sk_buff = skb;
 
  mapping = pci_map_single(pdev, skb->tail, rx_buf_sz,

@@ -2189,7 +2191,7 @@ rtl8169_rx_interrupt(struct net_device *
  tp->cur_rx = cur_rx;
 
  delta = rtl8169_rx_fill(tp, dev, tp->dirty_rx, tp->cur_rx);
- if (delta < 0) {
+ if (delta < 1 && count) {
   printk(KERN_INFO "%s: no Rx buffer allocated\n", dev->name);
   delta = 0;
  }

Jon Mason [off-list ref] :

This patch removes netif_poll_disable if NAPI is not enabled (otherwise 
adapter will hang while changing MTUs).

I am currently running a non-napi r8169 on x86/sparc64 based on 2.6.11-rc4
+ patches in Jeff's netdev and it apparently does not mind change of mtu.

How am I supposed to make it hang ?

This patch also fixes a possible skb alignment overrun,

Ok. Added some bits, see below.

and fixes the rx skb allocation error logging. It also 

Ok. I'd rather see it as shown below though (cuts some code and more
ppc-friendly unsigned ints).

removes an unnecessary define,

Ok.

adds a link down notification, and cleans up 

Ok. I'll netif_msg it before someone else complains that the driver
is too verbose.

----------------------8<-----------------------------------------

Fix rx skb allocation error logging

Signed arithmetic is not required as rtl8169_rx_fill() return belongs
to the [0; NUM_RX_DESC] interval.

Signed-off-by: Jon Mason <redacted>
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>

diff -puN drivers/net/r8169.c~r8169-400 drivers/net/r8169.c

--- a/drivers/net/r8169.c~r8169-400	2005-02-17 21:50:09.820173216 +0100
+++ b/drivers/net/r8169.c	2005-02-17 22:00:00.210450784 +0100

@@ -2156,8 +2156,8 @@ static int
 rtl8169_rx_interrupt(struct net_device *dev, struct rtl8169_private *tp,
 		     void __iomem *ioaddr)
 {
-	unsigned int cur_rx, rx_left, count;
-	int delta;
+	unsigned int cur_rx, rx_left;
+	unsigned int delta, count;
 
 	assert(dev != NULL);
 	assert(tp != NULL);

@@ -2225,10 +2225,8 @@ rtl8169_rx_interrupt(struct net_device *
 	tp->cur_rx = cur_rx;
 
 	delta = rtl8169_rx_fill(tp, dev, tp->dirty_rx, tp->cur_rx);
-	if (delta < 0) {
+	if (!delta && count)
 		printk(KERN_INFO "%s: no Rx buffer allocated\n", dev->name);
-		delta = 0;
-	}
 	tp->dirty_rx += delta;
 
 	/*

_

Nail an overrun in skb alignment and remove the relevant magic variable.

Signed-off-by: Jon Mason <redacted>
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>

diff -puN drivers/net/r8169.c~r8169-410 drivers/net/r8169.c

--- a/drivers/net/r8169.c~r8169-410	2005-02-17 22:13:04.209897364 +0100
+++ b/drivers/net/r8169.c	2005-02-17 22:17:25.747969121 +0100

@@ -1697,11 +1697,11 @@ static int rtl8169_alloc_rx_skb(struct p
 	dma_addr_t mapping;
 	int ret = 0;
 
-	skb = dev_alloc_skb(rx_buf_sz);
+	skb = dev_alloc_skb(rx_buf_sz + NET_IP_ALIGN);
 	if (!skb)
 		goto err_out;
 
-	skb_reserve(skb, 2);
+	skb_reserve(skb, NET_IP_ALIGN);
 	*sk_buff = skb;
 
 	mapping = pci_map_single(pdev, skb->tail, rx_buf_sz,

@@ -2140,9 +2140,9 @@ static inline int rtl8169_try_rx_copy(st
 	if (pkt_size < rx_copybreak) {
 		struct sk_buff *skb;
 
-		skb = dev_alloc_skb(pkt_size + 2);
+		skb = dev_alloc_skb(pkt_size + NET_IP_ALIGN);
 		if (skb) {
-			skb_reserve(skb, 2);
+			skb_reserve(skb, NET_IP_ALIGN);
 			eth_copy_and_sum(skb, sk_buff[0]->tail, pkt_size, 0);
 			*sk_buff = skb;
 			rtl8169_return_to_asic(desc, rx_buf_sz);

_


--
Ueimor

On Thursday 17 February 2005 05:28 pm, Francois Romieu wrote:

Jon Mason [off-list ref] :

quoted

This patch removes netif_poll_disable if NAPI is not enabled (otherwise
adapter will hang while changing MTUs).

I am currently running a non-napi r8169 on x86/sparc64 based on 2.6.11-rc4
+ patches in Jeff's netdev and it apparently does not mind change of mtu.

How am I supposed to make it hang ?

I can't seem to make it hang anymore.  I guess I was wrong.  Please remove 
this part of the patch.

quoted

This patch also fixes a possible skb alignment overrun,

Ok. Added some bits, see below.

Sorry for the oversight, but I had the 2nd part in the jumbo frames patch.

quoted

and fixes the rx skb allocation error logging. It also

Ok. I'd rather see it as shown below though (cuts some code and more
ppc-friendly unsigned ints).

Actually, I think it should be something like:
if (delta != count)

quoted

removes an unnecessary define,

Ok.

quoted

adds a link down notification, and cleans up

Ok. I'll netif_msg it before someone else complains that the driver
is too verbose.

ya, I was thinking about modifying most of the printks to dprintks (and 
possibly moving to the e1000 dprintk model),  but I like the link down 
notification.

Hello.

Francois Romieu wrote:

Jon Mason [off-list ref] :

[snip]

quoted

adds a link down notification, and cleans up 

Ok. I'll netif_msg it before someone else complains that the driver
is too verbose.

[snip]

I started working on adding netif_msg checks to the printks. How far 
have you got? I don't want to duplicate your work.

Incidentally, is there a to-do list somewhere for the r8169 driver? 
Maybe I can help out with some of the things.

Thanks, bye, Rich =]

-- 
Richard Dawe [ http://homepages.nildram.co.uk/~phekda/richdawe/ ]

"You can't evaluate a man by logic alone."
   -- McCoy, "I, Mudd", Star Trek

On Sunday 20 February 2005 11:52 am, Richard Dawe wrote:

Hello.

Francois Romieu wrote:

quoted

Jon Mason [off-list ref] :

[snip]

quoted

quoted

adds a link down notification, and cleans up

Ok. I'll netif_msg it before someone else complains that the driver
is too verbose.

[snip]

I started working on adding netif_msg checks to the printks. How far
have you got? I don't want to duplicate your work.

Incidentally, is there a to-do list somewhere for the r8169 driver?

Not to my knowledge.  It would be nice if there was a wiki out there which 
lists all of the drivers and areas to work on (features enablement, open 
bugs, etc.)

Maybe I can help out with some of the things.

I know Francois likes it when people test his patches.  :-)

Thanks, bye, Rich =]

Richard Dawe [off-list ref] :
[...]

I started working on adding netif_msg checks to the printks. How far 
have you got? I don't want to duplicate your work.

None so far. Go wild.

Incidentally, is there a to-do list somewhere for the r8169 driver? 

(without priority)

- validate suspend/resume/wol

- fix the dac issues on x64
  Weird. Pita. Status needs to be updated.

- push jumbo frames beyond 7k
  see with Jon Mason

- merge with the 8139cp driver
  always interrupted but I have not given up

- fix the gcc 2.95.x miscompilations
  people upgrade their compiler and return to real life before it can be
  diagnosed. It should not be hard but I have not had the time to install
  a broken debian to reproduce it so far :o(

- benchmark/improve the performances
  Executive summary by Lennert Buytenhek: it sucks. If someone could identify
  where the bottleneck is with hard numbers...

- harvest the hopeless r8169 users on the web
  time consuming like hell but it really helps to figure if things are
  stable or not.

- backport to 2.4
  RSN. Pending queue is available here and will be pushed once 2.6.x is
  up-to-date.

- ask Ben Greear if he still experiences keyboard issues with its laptop
  when he uses the r8169 driver
  Pending for 5 months... Ahem.

Maybe I can help out with some of the things.

Figure what's wrong with M. Gardiol setup ?

--
Ueimor

On Sunday 20 February 2005 06:28 pm, Francois Romieu wrote:

Richard Dawe [off-list ref] :
[...]

quoted

I started working on adding netif_msg checks to the printks. How far
have you got? I don't want to duplicate your work.

None so far. Go wild.

quoted

Incidentally, is there a to-do list somewhere for the r8169 driver?

(without priority)

- validate suspend/resume/wol

- fix the dac issues on x64
  Weird. Pita. Status needs to be updated.

- push jumbo frames beyond 7k
  see with Jon Mason

- merge with the 8139cp driver
  always interrupted but I have not given up

- fix the gcc 2.95.x miscompilations
  people upgrade their compiler and return to real life before it can be
  diagnosed. It should not be hard but I have not had the time to install
  a broken debian to reproduce it so far :o(

- benchmark/improve the performances
  Executive summary by Lennert Buytenhek: it sucks. If someone could
identify where the bottleneck is with hard numbers...

- harvest the hopeless r8169 users on the web
  time consuming like hell but it really helps to figure if things are
  stable or not.

- backport to 2.4
  RSN. Pending queue is available here and will be pushed once 2.6.x is
  up-to-date.

- ask Ben Greear if he still experiences keyboard issues with its laptop
  when he uses the r8169 driver
  Pending for 5 months... Ahem.

quoted

Maybe I can help out with some of the things.

Figure what's wrong with M. Gardiol setup ?

Fix ethtool statistics (amd64 only problem?)