linux-kernel - Re: [PATCH net-next 1/5] net: ethernet: ti: am65-cpsw: remove am65_cpsw_nuss_tx_compl_packets

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: <20250218180120.GC1615191@kernel.org>
Date: Tue, 18 Feb 2025 18:01:20 +0000
From: Simon Horman <horms@...nel.org>
To: Roger Quadros <rogerq@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Jesper Dangaard Brouer <hawk@...nel.org>,
	John Fastabend <john.fastabend@...il.com>,
	Siddharth Vadapalli <s-vadapalli@...com>,
	Md Danish Anwar <danishanwar@...com>, srk@...com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	bpf@...r.kernel.org
Subject: Re: [PATCH net-next 1/5] net: ethernet: ti: am65-cpsw: remove
 am65_cpsw_nuss_tx_compl_packets_2g()

On Mon, Feb 17, 2025 at 09:31:46AM +0200, Roger Quadros wrote:
> The only difference between am65_cpsw_nuss_tx_compl_packets_2g() and
> am65_cpsw_nuss_tx_compl_packets() is the usage of spin_lock() and
> netdev_tx_completed_queue() + am65_cpsw_nuss_tx_wake at every packet
> in the latter.
> 
> Insted of having 2 separate functions for TX completion, merge them

nit, in case there is a v2 for some other reason: Instead

> into one. This will reduce code duplication and make maintenance easier.
> 
> Signed-off-by: Roger Quadros <rogerq@...nel.org>

...

> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c

...

> @@ -1533,23 +1538,35 @@ static int am65_cpsw_nuss_tx_compl_packets(struct am65_cpsw_common *common,
>  		if (buf_type == AM65_CPSW_TX_BUF_TYPE_SKB) {
>  			skb = am65_cpsw_nuss_tx_compl_packet_skb(tx_chn, desc_dma);
>  			ndev = skb->dev;
> -			total_bytes = skb->len;
> +			pkt_len = skb->len;
>  			napi_consume_skb(skb, budget);
>  		} else {
>  			xdpf = am65_cpsw_nuss_tx_compl_packet_xdp(common, tx_chn,
>  								  desc_dma, &ndev);
> -			total_bytes = xdpf->len;
> +			pkt_len = xdpf->len;
>  			if (buf_type == AM65_CPSW_TX_BUF_TYPE_XDP_TX)
>  				xdp_return_frame_rx_napi(xdpf);
>  			else
>  				xdp_return_frame(xdpf);
>  		}
> +
> +		total_bytes += pkt_len;
>  		num_tx++;
>  
> -		netif_txq = netdev_get_tx_queue(ndev, chn);
> +		if (!single_port) {
> +			/* as packets from multi ports can be interleaved
> +			 * on the same channel, we have to figure out the
> +			 * port/queue at every packet and report it/wake queue.
> +			 */
> +			netif_txq = netdev_get_tx_queue(ndev, chn);
> +			netdev_tx_completed_queue(netif_txq, 1, pkt_len);
> +			am65_cpsw_nuss_tx_wake(tx_chn, ndev, netif_txq);
> +		}
> +	}
>  
> +	if (single_port) {
> +		netif_txq = netdev_get_tx_queue(ndev, chn);
>  		netdev_tx_completed_queue(netif_txq, num_tx, total_bytes);
> -
>  		am65_cpsw_nuss_tx_wake(tx_chn, ndev, netif_txq);
>  	}

Maybe it's not worth it, but it seems that a helper could
avoid duplication of the netif_txq handling immediately above (twice).

Regardless, this looks good to me.

Reviewed-by: Simon Horman <horms@...nel.org>

...