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: <4ACB4934.8090407@st.com>
Date:	Tue, 06 Oct 2009 15:42:12 +0200
From:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	netdev@...r.kernel.org, Andy Fleming <afleming@...escale.com>
Subject: Re: [PATCH] net: add support for STMicroelectronics Ethernet controllers.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Eric.

Eric Dumazet wrote:
> 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.

Thanks, I'll fix it taking as example the tg3 d.d.

>>> 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.
> 

I'll fix this too.

I'm going to post a new patch for the stmmac as soon as I fix all these
points.

Many thanks.
Regards,
Peppe
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkrLSTQACgkQ2Xo3j31MSSIKgQCeL733CI46GAUhDk/safdFH2Wj
q98An09JpuvsNGb0bihuy5dEaAIDUIIV
=R6wJ
-----END PGP SIGNATURE-----
--
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