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

[Giuseppe CAVALLARO <peppe.cavallaro@st.com>]

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

Hi Eric,
many thanks for your prompt feedback.

Eric Dumazet wrote:
> Giuseppe CAVALLARO a écrit :
> 
>> +static int stmmac_sw_tso(struct stmmac_priv *priv, struct sk_buff *skb)
[snip]
> 
> So stmmac_sw_tso() calls stmmac_xmit() for each seg skb.
> 
> But stmmac_sw_tso() was called from stmmac_xmit(),
> with priv->tx_lock locked, so I suspect something is wrong.

Yes, you are right on this.
I'm going to remove it. Indeed, I observed no gain during my performance
tests. I had added it just to become familiar with this.
Note also that our chips do not have the segmentation offloading in Hw.
Thanks for this observation.

> Also please change various
> 
> +	mac = kmalloc(sizeof(const struct mac_device_info), GFP_KERNEL);
> +	memset(mac, 0, sizeof(struct mac_device_info));
> +
> 
> +	mac = kmalloc(sizeof(const struct mac_device_info), GFP_KERNEL);
> +	memset(mac, 0, sizeof(struct mac_device_info));
> +
> 
> to use kzalloc(), and of course, you should check kmalloc()/kzalloc() dont return NULL !
> 

Yes I'll do that too.

> Also :
> 
> +static int stmmac_clean_tx(struct net_device *dev)
[snip]
> +		if (skb != NULL) {
> +			/*
> +			 * If there's room in the queue (limit it to size)
> +			 * we add this skb back into the pool,
> +			 * if it's the right size.
> +			 */
> +			if ((skb_queue_len(&priv->rx_recycle) <
> +				priv->dma_rx_size) &&
> +				skb_recycle_check(skb, priv->dma_buf_sz))
> +				__skb_queue_head(&priv->rx_recycle, skb);
> +			else
> +				dev_kfree_skb_any(skb);
> 
> 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?

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

Thanks again for the feedback.

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

iEYEARECAAYFAkrLOjkACgkQ2Xo3j31MSSIM5ACgrTLJgwO84ooKkcoEaNMZ5bcy
iBUAoK+q677OnyeCdeNndFq92AmwNPoa
=r7WK
-----END PGP SIGNATURE-----