Re: [PATCH v5 2/2] Ethernet driver for the WIZnet W5100 chip

[Francois Romieu]

Eric Dumazet <eric.dumazet@xxxxxxxxx> :
> Le vendredi 30 mars 2012 ?? 13:00 +0600, Mike Sinkovsky a ??crit :
> > Tested and used in production with Blackfin BF531 embedded processor.
[...]
> > +static int w5100_start_tx(struct sk_buff *skb, struct net_device *ndev)
> > +{
> > +	struct w5100_priv *priv = netdev_priv(ndev);
> > +	u16 offset;
> > +
> > +	if (w5100_read16(priv, W5100_S0_TX_FSR) < skb->len) {
> 
> There is a race here.
> 
> Interrupt came come right here before you set the stop_queue()
> 
> 
> > +		netif_stop_queue(ndev);
> 
> So Here I suggest adding a test again
> 	
> 	if (w5100_read16(priv, W5100_S0_TX_FSR) >= skb->len)
> 		netif_wake_queue(ndev);
> 	else
> 		return NETDEV_TX_BUSY;

The start_tx logic seems broken anyway: it claims it always can take
packets until it is given one it can't process. The driver should rather
do the opposite and stop queueing as soon as there is no guarantee for
the next packet.

-- 
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/