Netpoll controller support for PPC EMAC driver

This patch adds netpoll controller support for PPC EMAC driver

Signed-off-by: Ruslan V. Sushko <redacted>

On Thu, Dec 08, 2005 at 03:34:15PM +0300, Ruslan V. Sushko wrote:

This patch adds netpoll controller support for PPC EMAC driver

Signed-off-by: Ruslan V. Sushko <redacted>

Patches in-line are easier to review... :-)

Why is this necessary?

Is the call to netif_poll_disable necessary?  Aren't NAPI and netpoll
aware of each other?

If it's never invoked, then why define it?

->poll is not related to ->poll_controller.  It is for NAPI, not
netpoll.

I'll defer to others to comment on the IBM EMAC manipulation code.
It looked reasonable to me at first glance.

John
-- 
John W. Linville
linville@tuxdriver.com

Please see my answers bellow.
John W. Linville wrote:

On Thu, Dec 08, 2005 at 03:34:15PM +0300, Ruslan V. Sushko wrote:
  

quoted

This patch adds netpoll controller support for PPC EMAC driver

Signed-off-by: Ruslan V. Sushko <redacted>
    

Patches in-line are easier to review... :-)

  

quoted

@@ -1071,8 +1071,16 @@ static int emac_start_xmit(struct sk_buf
 	struct ocp_enet_private *dev = ndev->priv;
 	unsigned int len = skb->len;
 	int slot;
+	u16 ctrl;
 
-	u16 ctrl = EMAC_TX_CTRL_GFCS | EMAC_TX_CTRL_GP | MAL_TX_CTRL_READY |
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	if (unlikely(dev->tx_cnt == NUM_TX_BUFF)) {
+		netif_stop_queue(ndev);
+		return -EBUSY;
+	}
+#endif
    

Why is this necessary?
  

When netpoll controller is enabled and packets are trapped bay netpoll controller the netif_stop_queue function does nothing. As result the packets which are queued for send may be overwritten/corrupted, because CPU insert packets significantly faster then netdev send them.

quoted

+
+	ctrl = EMAC_TX_CTRL_GFCS | EMAC_TX_CTRL_GP | MAL_TX_CTRL_READY |
 	    MAL_TX_CTRL_LAST | emac_tx_csum(dev, skb);
 
 	slot = dev->tx_slot++;

@@ -1938,6 +1946,33 @@ static int emac_ioctl(struct net_device  		return -EOPNOTSUPP;
 	}
 }
+#ifdef CONFIG_NET_POLL_CONTROLLER
+void
+poll_ctrl(struct net_device *dev)
+{
+	int budget = 16;
+	struct ibm_ocp_mal *mal = ((struct ocp_enet_private*)(dev->priv))->mal;
+	struct net_device *poll_dev = &(mal->poll_dev);
+
+	/* disable  MAL interrupts */
+	mal_disable_eob_irq(mal);
+	netif_poll_disable(poll_dev);
    

Is the call to netif_poll_disable necessary?  Aren't NAPI and netpoll
aware of each other?
  

In this case the device instance passed to netpoll controller functionality is not the same which used by NAPI. As result conflict will occurred if netpoll controller invokes polling in the time of NAPI poll working. Please see my comments below.

quoted

+
+	emac_poll_rx(dev->priv, budget);
+	emac_poll_tx(dev->priv);
+
+	netif_poll_enable(poll_dev);
+	/* Enable mal interrupts */
+	mal_enable_eob_irq(mal);
+}
+
+int
+poll_fake(struct net_device *dev, int *budget)
+{
+	/* It will be never invoked */
+	return 0;
+}
+#endif
    

If it's never invoked, then why define it?
  

Please see my comments bellow.

quoted

 
 static int __init emac_probe(struct ocp_device *ocpdev)
 {

@@ -2188,6 +2223,11 @@ static int __init emac_probe(struct ocp_
 	netif_carrier_off(ndev);
 	netif_stop_queue(ndev);
 
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	ndev->poll_controller = poll_ctrl;
+	ndev->poll = poll_fake;
+#endif
+
    

->poll is not related to ->poll_controller.  It is for NAPI, not
netpoll.
  

The main idea is devices passed to netpoll controller are not the same which is used for NAPI. This driver has three net_device instances. One is virtual device which is used only for NAPI (for this device real NAPI poll function is defined) and two others are usual devices without NAPI. For each of these two devices poll handlers functions are defined (emac_poll_rx and emac_poll_tx) which are invoked from NAPI device poll handler. Function emac_poll_rx uses netif_receive_skb function to pass skbs to high level. To pass skb to netpoll controller via netif_receive_skb function the poll field must be defined:
-------------------- cut from netif_receive_skb (net/dev.c) ----------
if (skb->dev->poll && netpoll_rx(skb))
return NET_RX_DROP;
--------------------------------------------------------------------------------
So the poll_fake function and poll field are defined and initialized only to cheat netif_receive_skb function. In same time the poll_fake will be never invoked because devices for whom this function is defined are not NAPI devices.

Thank you,
Ruslan

On Thu, Dec 08, 2005 at 03:34:15PM +0300, Ruslan V. Sushko wrote:

This patch adds netpoll controller support for PPC EMAC driver

Signed-off-by: Ruslan V. Sushko <redacted>

NAK. Please see long discussion on netdev couple of months ago about 
netpoll and how it is horribly broken.

In short, to implement netpoll support in EMAC driver requires full 
locking re-do (all locks must be IRQ disabling). I'm not gonna do 
this, because it's wrong approach.

If you want netpoll support, figure out a way to do this correctly, 
without penalizing users of EMAC driver by switching to IRQ locks.

-- 
Eugene