Re: [PATCH net 1/2] net: macb: credit unsent frames to BQL in macb_tx_error_task()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix leaves a second site with the same bug.

--

net: macb: credit unsent frames to BQL in macb_tx_error_task()

This commit moves the BQL packets/bytes accounting in macb_tx_error_task()
out of the "TX complete" branch. Every frame removed from the ring is now
credited through netdev_tx_completed_queue(), whether it was sent or not.
This stops the DQL in-flight count from leaking on each TX error.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index f087f278f9e56..b6105f915fe35 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -1428,6 +1426,12 @@ static void macb_tx_error_task(struct work_struct *work)
>  			desc->ctrl = ctrl | MACB_BIT(TX_USED);
>  		}
>  
> +		/* BQL has to see every frame leaving the ring, sent or not */
> +		if (skb) {
> +			packets++;
> +			bytes += skb->len;
> +		}
> +
>  		macb_tx_unmap(&bp->info, tx_skb, 0);
>  	}
>  

[Severity: High]
The commit message describes the problem in general terms: "The frames
dropped on a TX error are never returned to BQL." Does
macb_hresp_error_task() leak in the same way?

macb_interrupt_misc() queues it as BH work when the hardware reports
HRESP:

macb_interrupt_misc() {
    ...
	if (status & MACB_BIT(HRESP)) {
		queue_work(system_bh_wq, &bp->hresp_err_bh_work);
    ...
}

It then resets the TX rings and restarts the queues. It never calls
netdev_tx_completed_queue() or netdev_tx_reset_queue():

macb_hresp_error_task() {
    ...
	netif_tx_stop_all_queues(netdev);
	netif_carrier_off(netdev);

	bp->macbgem_ops.mog_init_rings(ctx);

	/* Initialize TX and RX buffers */
	macb_init_buffers(bp);
    ...
	netif_carrier_on(netdev);
	netif_tx_start_all_queues(netdev);
}

gem_init_rings() and macb_init_rings() mark every TX descriptor TX_USED
and set txq->head = txq->tail = 0. Every frame between tail and head was
already charged in macb_start_xmit():

	netdev_tx_sent_queue(netdev_get_tx_queue(bp->netdev, q),
			     skb->len);

So DQL would count those bytes as in flight forever. After enough HRESP
events, the queue would stay stopped with an empty ring. Only
macb_close() would recover it, through netdev_tx_reset_queue().

This commit carries the same Fixes: tag as the BQL introduction, and that
commit missed both paths. Could the HRESP ring reset path be covered here
too, or in a companion patch, so that backports get both fixes?

On a side note, this HRESP path also seems to reset head and tail
without unmapping the DMA buffers or freeing the tx_skb->skb entries. Are
those skbs and DMA mappings leaked as well?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107-nb-macb-tx-error-task-net-v1-0-a2aac5d64d52%40tipi-net.de