Re: [PATCH net] net: bgmac: don't process RX with a zero NAPI budget

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] In drivers/net/ethernet/broadcom/bgmac.c, bgmac_poll() still
  acknowledges every interrupt source, including BGMAC_IS_RX, with…

--

net: bgmac: don't process RX with a zero NAPI budget

When bgmac_poll() is called with a zero budget, this patch makes it
return right after TX completion cleanup. The goal is to stop netpoll
from processing an RX frame and tripping the "exceeded budget in poll"
WARN_ONCE() in poll_one_napi().

> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 3e9c57196a397..a545c0cc25977 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -1154,6 +1154,11 @@ static int bgmac_poll(struct napi_struct *napi, int weight)
>  	bgmac_write(bgmac, BGMAC_INT_STATUS, ~0);
>  
>  	bgmac_dma_tx_free(bgmac, &bgmac->tx_ring[0]);
> +
> +	/* A zero budget (netpoll) means only TX may be cleaned */
> +	if (unlikely(!weight))
> +		return 0;

[Severity: Medium]
Can this leave received frames stuck in the RX ring?

Before it reaches the new zero budget return, bgmac_poll() still clears
every interrupt status bit, including BGMAC_IS_RX, with a write-1-clear:

    bgmac_write(bgmac, BGMAC_INT_STATUS, ~0);

The commit message says:

    NAPI stays scheduled, so the pending RX frames are picked up by the
    next regular poll.

Is that guaranteed?

poll_napi() calls poll_one_napi() for every NAPI on dev->napi_list
whose poll_owner it can take. poll_one_napi() then calls
napi->poll(napi, 0) without checking NAPI_STATE_SCHED.

bgmac_netdev_ops has no ndo_poll_controller, so bgmac_interrupt() is not
run first. As a result, bgmac_poll(napi, 0) can run while the NAPI is
idle and chip interrupts are on.

Suppose a frame arrives while netpoll is transmitting. One example is
__netpoll_send_skb() running with IRQs disabled while the TX queue is
stopped. The sequence would be:

netpoll_poll_dev()->poll_napi()->poll_one_napi()->bgmac_poll(napi, 0)
    bgmac_write(bgmac, BGMAC_INT_STATUS, ~0);   /* clears BGMAC_IS_RX */
    bgmac_dma_tx_free(bgmac, &bgmac->tx_ring[0]);
    return 0;                                   /* NAPI not scheduled */

After that, bgmac_interrupt() finds nothing pending:

    u32 int_status = bgmac_read(bgmac, BGMAC_INT_STATUS);
    int_status &= bgmac->int_mask;

    if (!int_status)
        return IRQ_NONE;

bgmac_interrupt() is the only caller of napi_schedule() here. Frames
already in the RX ring would sit there until another frame raises
BGMAC_IS_RX. On a quiet link, for example after a single ARP or ping
reply, that wait has no upper bound.

The unconditional ack existed before this patch. However, one RX slot
used to be processed in this window, and now none are.

Would it work to ack only BGMAC_IS_TX0 when the budget is 0? That would
leave BGMAC_IS_RX set, so the interrupt handler still schedules NAPI.

The new comment says "only TX may be cleaned". The NAPI documentation
says a zero budget means processing "only skb Tx completions and no Rx".
This path still consumes RX interrupt state, which doesn't match either
statement.

> +
>  	handled += bgmac_dma_rx_read(bgmac, &bgmac->rx_ring[0], weight);
>  
>  	/* Poll again if more events arrived in the meantime */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014001912.141532-1-rosenp%40gmail.com