[Linux-stm32] [Intel-wired-lan] [PATCH net-next 1/2] stmmac: xsk: fix underflow of budget in zerocopy mode

[Paul Menzel]

Dear Jason,


Thank you for your patch.

Am 21.07.25 um 10:33 schrieb Jason Xing:
> From: Jason Xing <kernelxing at tencent.com>
> 
> The issue can happen when the budget number of descs are consumed. As

Instead of “The issue”, I’d use “An underflow …”.

> long as the budget is decreased to zero, it will again go into
> while (budget-- > 0) statement and get decreased by one, so the
> underflow issue can happen. It will lead to returning true whereas the
> expected value should be false.

What is “it”?

> In this case where all the budget are used up, it means zc function

*is* used?

> should return false to let the poll run again because normally we
> might have more data to process.

Do you have a reproducer, you could add to the commit message?

> Fixes: 132c32ee5bc0 ("net: stmmac: Add TX via XDP zero-copy socket")
> Signed-off-by: Jason Xing <kernelxing at tencent.com>
> ---
>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index f350a6662880..ea5541f9e9a6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2596,7 +2596,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
>   
>   	budget = min(budget, stmmac_tx_avail(priv, queue));
>   
> -	while (budget-- > 0) {
> +	while (budget > 0) {

So, if the while loop should not be entered with budget being 0, then 
the line could  be changed to `while (--budget > 0) {`? But then it 
wouldn’t be called for budget being 1.

A for loop might be the better choice for a loop with budget as counting 
variable?

>   		struct stmmac_metadata_request meta_req;
>   		struct xsk_tx_metadata *meta = NULL;
>   		dma_addr_t dma_addr;
> @@ -2681,6 +2681,8 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
>   
>   		tx_q->cur_tx = STMMAC_GET_ENTRY(tx_q->cur_tx, priv->dma_conf.dma_tx_size);
>   		entry = tx_q->cur_tx;
> +
> +		budget--;
>   	}
>   	u64_stats_update_begin(&txq_stats->napi_syncp);
>   	u64_stats_add(&txq_stats->napi.tx_set_ic_bit, tx_set_ic_bit);

Excuse my ignorance, but I do not yet see the problem that the while 
loop is entered and buffer is set to 0. Is it later the return condition?

     return !!budget && work_done;


Kind regards,

Paul