Netpoll controller support for PPC EMAC driver

Netpoll controller support for PPC EMAC driver

[Ruslan V. Sushko]

[-- Attachment #1: Type: text/plain, Size: 123 bytes --]

This patch adds netpoll controller support for PPC EMAC driver

Signed-off-by: Ruslan V. Sushko <rsushko@ru.mvista.com>




[-- Attachment #2: ppc_emac_napi_netpoll.patch --]
[-- Type: text/x-patch, Size: 4097 bytes --]

This patch adds netpoll controller support for PPC EMAC driver

---
commit a482261f165edb7e6af363d13ea94e8e7429f085
tree b5bee5eb8a971c3f40867946225051e1ff320763
parent e4f5c82a92c2a546a16af1614114eec19120e40a
author Ruslan V. Sushko <rsushko@ru.mvista.com> Thu, 08 Dec 2005 15:22:38 +0300
committer Ruslan V. Sushko <rsushko@ru.mvista.com> Thu, 08 Dec 2005 15:22:38 +0300

 drivers/net/ibm_emac/ibm_emac_core.c |   42 +++++++++++++++++++++++++++++++++-
 drivers/net/ibm_emac/ibm_emac_mal.c  |   14 -----------
 drivers/net/ibm_emac/ibm_emac_mal.h  |   12 ++++++++++
 3 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ibm_emac/ibm_emac_core.c b/drivers/net/ibm_emac/ibm_emac_core.c
--- a/drivers/net/ibm_emac/ibm_emac_core.c
+++ b/drivers/net/ibm_emac/ibm_emac_core.c
@@ -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
+
+	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);
+
+	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
 
 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
+
 	err = register_netdev(ndev);
 	if (err) {
 		printk(KERN_ERR "emac%d: failed to register net device (%d)!\n",
diff --git a/drivers/net/ibm_emac/ibm_emac_mal.c b/drivers/net/ibm_emac/ibm_emac_mal.c
--- a/drivers/net/ibm_emac/ibm_emac_mal.c
+++ b/drivers/net/ibm_emac/ibm_emac_mal.c
@@ -155,20 +155,6 @@ void mal_poll_del(struct ibm_ocp_mal *ma
 	local_bh_enable();
 }
 
-/* synchronized by mal_poll() */
-static inline void mal_enable_eob_irq(struct ibm_ocp_mal *mal)
-{
-	MAL_DBG2("%d: enable_irq" NL, mal->def->index);
-	set_mal_dcrn(mal, MAL_CFG, get_mal_dcrn(mal, MAL_CFG) | MAL_CFG_EOPIE);
-}
-
-/* synchronized by __LINK_STATE_RX_SCHED bit in ndev->state */
-static inline void mal_disable_eob_irq(struct ibm_ocp_mal *mal)
-{
-	set_mal_dcrn(mal, MAL_CFG, get_mal_dcrn(mal, MAL_CFG) & ~MAL_CFG_EOPIE);
-	MAL_DBG2("%d: disable_irq" NL, mal->def->index);
-}
-
 static irqreturn_t mal_serr(int irq, void *dev_instance, struct pt_regs *regs)
 {
 	struct ibm_ocp_mal *mal = dev_instance;
diff --git a/drivers/net/ibm_emac/ibm_emac_mal.h b/drivers/net/ibm_emac/ibm_emac_mal.h
--- a/drivers/net/ibm_emac/ibm_emac_mal.h
+++ b/drivers/net/ibm_emac/ibm_emac_mal.h
@@ -216,6 +216,18 @@ static inline void set_mal_dcrn(struct i
 	mtdcr(mal->dcrbase + reg, val);
 }
 
+/* synchronized by mal_poll() */
+static inline void mal_enable_eob_irq(struct ibm_ocp_mal *mal)
+{
+	set_mal_dcrn(mal, MAL_CFG, get_mal_dcrn(mal, MAL_CFG) | MAL_CFG_EOPIE);
+}
+
+/* synchronized by __LINK_STATE_RX_SCHED bit in ndev->state */
+static inline void mal_disable_eob_irq(struct ibm_ocp_mal *mal)
+{
+	set_mal_dcrn(mal, MAL_CFG, get_mal_dcrn(mal, MAL_CFG) & ~MAL_CFG_EOPIE);
+}
+
 /* Register MAL devices */
 int mal_init(void) __init;
 void mal_exit(void) __exit;


\f
!-------------------------------------------------------------flip-

Re: Netpoll controller support for PPC EMAC driver

[John W. Linville]

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 <rsushko@ru.mvista.com>

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

> @@ -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?

> +
> +	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?

> +
> +	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?

>  
>  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.

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

Re: Netpoll controller support for PPC EMAC driver

[Ruslan V. Sushko]

Please see my answers bellow.
John W. Linville wrote:
> 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 <rsushko@ru.mvista.com>
>>     
>
> Patches in-line are easier to review... :-)
>
>   
>> @@ -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.
>   
>> +
>> +	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.
>   
>> +
>> +	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.
>   
>>  
>>  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

Re: Netpoll controller support for PPC EMAC driver

[Eugene Surovegin]

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 <rsushko@ru.mvista.com>

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