Re: Netpoll controller support for PPC EMAC driver

["John W. Linville" <linville@tuxdriver.com>]

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