Re: [PATCH] net: add support for STMicroelectronics Ethernet controllers.

[Eric Dumazet <eric.dumazet@gmail.com>]

Giuseppe CAVALLARO a écrit :

>> Why call dev_kfree_skb_any() here ? From NAPI context it is overkill.
> 
> The logic behind this piece of code should be the same one adopted in
> other drivers like gianfar, ucc_geth and mv643xx_eth. What am I missing?

Dont trust driver code too much, many of them are not upd2date.

gianfar disables irqs, so it calls dev_kfree_skb_any()

if (spin_trylock_irqsave(&priv->txlock, flags)) {
	tx_cleaned = gfar_clean_tx_ring(dev);
	spin_unlock_irqrestore(&priv->txlock, flags);
}

but in your case,

stmmac_clean_tx() runs in sofirq mode, you can call dev_kfree_skb()/consume_skb()

Please check drivers/net/tg3.c, function tg3_tx() for a _good_ example.

> 
>> static int stmmac_poll(struct napi_struct *napi, int budget)
>> +{
> [snip]
>> +
>> +	tx_cleaned = stmmac_clean_tx(dev);
>> +
>> +	work_done = stmmac_rx(dev, budget);
>> +
>>
>>
>> +	if (tx_cleaned)
>> +		return budget;
>>
>> Why tx_cleaned is used here to exit early ?
> 
> I've found interesting the approach used in gianfar (see commit
> 42199884594bc336c9185441cbed99a9324dab34).
> 

This looks buggy and not a clone of e1000 code, despite its Changelog claim.

e1000 code is OK, not gianfar.

static int e1000_clean(struct napi_struct *napi, int budget)
{
	tx_clean_complete = e1000_clean_tx_irq(adapter, &adapter->tx_ring[0]);

	adapter->clean_rx(adapter, &adapter->rx_ring[0], &work_done, budget);

	if (!tx_clean_complete)
		work_done = budget; // we say budget is fully consumed to force another poll round

	if (work_done < budget) {
		...
		napi_complete(napi);
		...
	}
}

You can see e1000_clean_tx_irq() doesnt return "number of completed skbs", but a 
boolean saying if more skb are still in tx queue "(count < tx_ring->count)"

This is because we want to check again tx queue before napi_complete(this_adapter)


Your code forces a poll() round if at least *one* skb was completed, this is very strange.