Re: [PATCH]: cpmac bugfixes and enhancements

[Andrew Morton <akpm@linux-foundation.org>]

On Sun, 4 May 2008 19:04:22 +0200
Matteo Croce <matteo@openwrt.org> wrote:

> This patch fixes an IRQ storm, a locking issues, moves platform code in the right sections
> and other small fixes.
> 

Please feed this patch (and all future ones) through scripts/checkpatch.pl.
It picks up rather a lot of simple problems which there is no reason for
us to retain.

>
> ...
>
> +	spin_unlock(&priv->rx_lock);
> +	netif_rx_complete(priv->dev, napi);
> +	netif_stop_queue(priv->dev);
> +	napi_disable(&priv->napi);
> +	
> +	atomic_inc(&priv->reset_pending);
> +	cpmac_hw_stop(priv->dev);
> +	if (!schedule_work(&priv->reset_work))
> +		atomic_dec(&priv->reset_pending);
> +	return 0;
> + 
>  }
>  
>  static int cpmac_start_xmit(struct sk_buff *skb, struct net_device *dev)
> @@ -456,6 +549,9 @@ static int cpmac_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	struct cpmac_desc *desc;
>  	struct cpmac_priv *priv = netdev_priv(dev);
>  
> +	if (unlikely(atomic_read(&priv->reset_pending)))
> +		return NETDEV_TX_BUSY;
> +

This looks a bit strange.  schedule_work() will return zero if the work was
already scheduled, in which case we arrange for cpmac_start_xmit() to abort
early.

But if schedule_work() *doesn't* return zero, there is a time window in
which the reset is still pending.  Because it takes time for keventd to be
awoken and to run the work function.

I would have thought that we would want to prevent cpmac_start_xmit() from
running within that time window also?


But that's just a guess - the text which you used to describe your work is
missing much information, so I don't have a lot to work with here.