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