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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4ACB47B5.5050700@gmail.com>
Date:	Tue, 06 Oct 2009 15:35:49 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
CC:	netdev@...r.kernel.org, Andy Fleming <afleming@...escale.com>
Subject: Re: [PATCH] net: add support for STMicroelectronics Ethernet controllers.

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html