r8169: non-NAPI netif_poll_disable removal

rtl8169_close contains a call to netif_poll_disable. This call sets and spins on the __LINK_STATE_RX_SCHED bit, which isn't really a problem if this function is only called in close (as that bit is off with NAPI off). However, if the driver hits a transmit timeout (or any other call that references rtl8169_wait_for_quiescence), the the module will hang indefinitely during any subsequent call to netif_poll_disable or dev_close function call because this bit is now set.

Since this function should not really be called with NAPI off, I chose to remove the calls if NAPI is not defined (See patch below). The patch is against the 2.6.10-rc1-mm2 version of the driver. I have verified that this fixes the problem on ppc64 and x86_64.

@@ -85,10 +85,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 rtl8169_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 rtl8169_poll_disable(dev)
 #endif
 
 /* media options */

@@ -1745,7 +1747,7 @@ static void rtl8169_wait_for_quiescence(
        synchronize_irq(dev->irq);
 
        /* Wait for any pending NAPI task to complete */
-       netif_poll_disable(dev);
+       rtl8169_poll_disable(dev);
 }
 
 static void rtl8169_reinit_task(void *_data)

@@ -2284,7 +2286,7 @@ rtl8169_close(struct net_device *dev)
 
        free_irq(dev->irq, dev);
 
-       netif_poll_disable(dev);
+       rtl8169_poll_disable(dev);
 
        rtl8169_tx_clear(tp);

@@ -85,10 +85,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 rtl8169_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 rtl8169_poll_disable(dev)
 #endif
 
 /* media options */

@@ -1745,7 +1747,7 @@ static void rtl8169_wait_for_quiescence(
 	synchronize_irq(dev->irq);
 
 	/* Wait for any pending NAPI task to complete */
-	netif_poll_disable(dev);
+	rtl8169_poll_disable(dev);
 }
 
 static void rtl8169_reinit_task(void *_data)

@@ -2284,7 +2286,7 @@ rtl8169_close(struct net_device *dev)
 
 	free_irq(dev->irq, dev);
 
-	netif_poll_disable(dev);
+	rtl8169_poll_disable(dev);
 
 	rtl8169_tx_clear(tp);
 

Jon Mason <[email protected]> :

Yep but it seems to me that there is still a window for a race with NAPI if the relevant error path is taken:

1 - Tx timeout
2 - rtl8169_reset_task: netif_poll_disable is issued. Assume that the
refilling of the Rx buffers fails -> rtl8169_reset_task schedules itself for later
3 - operator closes the device: netif_running(dev) == 0 and rtl8169_close()
is issued
4 - rtl8169_close() loops on netif_poll_disable()

Neither __netif_rx_schedule nor netif_poll_enable was issued. Deadlock.

It should be enough to change rtl8169_wait_for_quiescence() for:

static void rtl8169_wait_for_quiescence(struct net_device *dev)

{
        synchronize_irq(dev->irq);

        /* Wait for any pending NAPI task to complete */
        netif_poll_disable(dev);

	RTL_W16(IntrMask, 0x00);

        netif_poll_enable(dev);
}

There will be a few "interrupt XXX taken in poll" when it races with the irq handler but they are harmless anyway [*].

The lack of netif_poll_enable() after netif_wake_queue() in rtl8169_reset_task() which you suggested to me should also be fixed with this change.

[*] Ok, it should be possible to have a BUG_ON due to the simultaneous
invocation of rtl8169_reinit_task() and rtl8169_reset_task() under weird circumstances but this one will be easily fixed by an adequate netif_queue_stopped() in rtl8169_pcierr_interrupt().

--
Ueimor

On Saturday 06 November 2004 02:56 pm, Francois Romieu wrote:
[...]

There is an overall problem with the rtl8169_reset_task. While the driver continues to receive packets, can't allcate any new rx buffers, or the rx dma engine is hung and no rx buffers are processed, the rtl8169_reset_task will schedule itself and loop forever (as dirty_rx != cur_rx).

It seems to me a better idea to just remove the dirty_rx == cur_rx check, and let the function clean-up the mess the driver/adapter is in.

Jon Mason <[email protected]> :
[...]

rtl8169_reset_task() is issued after a rtl8169_hw_reset() which should disable the Rx/Tx DMA activity. dirty_rx == cur_rx means a complete Rx ring refill.

At first sight, I'd say that a partially refilled Rx ring is still usable if it verifies 0 < dirty_rx % NUM_RX_DESC < cur_rx % NUM_RX_DESC or cur_rx % NUM_RX_DESC == 0. I prefer to simply test for a complete refill.

Regarding the initial issue, what about the second patch below (against 2.6.10-rc1-bk15 + latest Jeff's netdev + first patch) ?

It is a bit paranoid but an user already reported a PCI error interruption on x86 so it should be possible to have a PCI error close to a Tx watchdog event. Despite what I claimed before, netif_queue_stopped() is not enough to synchronize the tasks.

diff -puN drivers/net/r8169.c~r8169-250 drivers/net/r8169.c
@@ -1742,10 +1742,17 @@ static void rtl8169_schedule_work(struct
 
 static void rtl8169_wait_for_quiescence(struct net_device *dev)
 {
+	struct rtl8169_private *tp = netdev_priv(dev);
+	void __iomem *ioaddr = tp->mmio_addr;
+
 	synchronize_irq(dev->irq);
 
 	/* Wait for any pending NAPI task to complete */
 	netif_poll_disable(dev);
+
+	RTL_W16(IntrMask, 0x0000);
+
+	netif_poll_enable(dev);
 }
 
 static void rtl8169_reinit_task(void *_data)

_