[PATCH net v2] net: ag71xx: re-enable TX interrupts on RX OOM

[PATCH net v2] net: ag71xx: re-enable TX interrupts on RX OOM

[Rosen Penev]

When the RX ring runs out of buffers the OOM path calls
napi_complete() but does not re-enable interrupts.  Because
INT_POLL masks both RX and TX events, TX completions are
blocked until the OOM timer fires (~100 ms), which can cause
TX ring exhaustion and dropped packets.

Restore TX interrupt delivery so the engine can drain completed
descriptors while NAPI waits for RX buffer memory.

Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: add Fixes: . 
 drivers/net/ethernet/atheros/ag71xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
index d93fd91ff884..5a33759be3cf 100644
--- a/drivers/net/ethernet/atheros/ag71xx.c
+++ b/drivers/net/ethernet/atheros/ag71xx.c
@@ -1746,6 +1746,7 @@ static int ag71xx_poll(struct napi_struct *napi, int limit)
 
 	mod_timer(&ag->oom_timer, jiffies + AG71XX_OOM_REFILL);
 	napi_complete(napi);
+	ag71xx_int_enable(ag, AG71XX_INT_TX);
 	return 0;
 }
 
-- 
2.56.0

Re: [PATCH net v2] net: ag71xx: re-enable TX interrupts on RX OOM

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v2] net: ag71xx: re-enable TX interrupts on RX OOM

[Daniel Machon]

> When the RX ring runs out of buffers the OOM path calls
> napi_complete() but does not re-enable interrupts.  Because
> INT_POLL masks both RX and TX events, TX completions are
> blocked until the OOM timer fires (~100 ms), which can cause
> TX ring exhaustion and dropped packets.

Is that not exactly what the OOM timer is for? It schedules napi and tx buffers
will eventually be freed. I mean, it seems like it was designed to work like
this.

> 
> Restore TX interrupt delivery so the engine can drain completed
> descriptors while NAPI waits for RX buffer memory.
> 
> Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver")
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  v2: add Fixes: . 
>  drivers/net/ethernet/atheros/ag71xx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
> index d93fd91ff884..5a33759be3cf 100644
> --- a/drivers/net/ethernet/atheros/ag71xx.c
> +++ b/drivers/net/ethernet/atheros/ag71xx.c
> @@ -1746,6 +1746,7 @@ static int ag71xx_poll(struct napi_struct *napi, int limit)
>  
>  	mod_timer(&ag->oom_timer, jiffies + AG71XX_OOM_REFILL);
>  	napi_complete(napi);
> +	ag71xx_int_enable(ag, AG71XX_INT_TX);
>  	return 0;
>  }
>  
> -- 
> 2.56.0
> 

/Daniel

Re: [PATCH net v2] net: ag71xx: re-enable TX interrupts on RX OOM

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 3 (1)

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] The RX-OOM exit in ag71xx_poll()
  (drivers/net/ethernet/atheros/ag71xx.c, around line 1732) now calls…
- [Low] NAPI can now be scheduled with TX_PS unmasked, which the driver
  did not allow before.
- [Low] In ag71xx_poll()'s OOM exit (lines 1731-1732), the new TX unmask
  runs after an unchecked napi_complete().
- [Low] The concern says the unlocked INT_ENABLE read-modify-write
  (ag71xx_sb/ag71xx_cb, lines 417-435), together with the new TX unmask,…

Pre-existing issues:
- [Low] The concern says the new TX enable in the OOM path can set TX_PS
  in AG71XX_REG_INT_ENABLE again after ag71xx_hw_stop() has cleared it,…

--

net: ag71xx: re-enable TX interrupts on RX OOM

This commit makes the RX out-of-memory exit in ag71xx_poll() unmask the
TX interrupt after napi_complete(). Before this change, INT_POLL stayed
masked until the OOM timer fired, so TX completions were held off for
about 100ms.

> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
> index 4e4794c4dfdce..3e582a7fa314a 100644
> --- a/drivers/net/ethernet/atheros/ag71xx.c
> +++ b/drivers/net/ethernet/atheros/ag71xx.c
> @@ -1729,6 +1729,7 @@ static int ag71xx_poll(struct napi_struct *napi, int limit)
>  
>  	mod_timer(&ag->oom_timer, jiffies + AG71XX_OOM_REFILL);
>  	napi_complete(napi);
> +	ag71xx_int_enable(ag, AG71XX_INT_TX);

[Severity: Medium]
With TX_PS unmasked here, does every TX completion during RX OOM now run
a full poll that lands back on the oom label?

ag71xx_interrupt() schedules the whole NAPI poll for any INT_POLL bit:

    if (likely(status & AG71XX_INT_POLL)) {
        ag71xx_int_disable(ag, AG71XX_INT_POLL);
        ...
        napi_schedule(&ag->napi);
    }

While allocation keeps failing, ag71xx_poll() goes straight back to oom:

    rx_done = ag71xx_rx_packets(ag, limit);

    if (!rx_ring->buf[rx_ring->dirty % rx_ring_size].rx.rx_buf)
        goto oom;

Each of these passes does three things:

  - ag71xx_rx_packets()->ag71xx_ring_rx_refill() tries another atomic
    napi_alloc_frag().
  - It prints netif_err(ag, rx_err, ndev, "out of memory\n") again, with
    no ratelimit. NETIF_MSG_RX_ERR is in AG71XX_DEFAULT_MSG_ENABLE, so
    this message shows with default settings.
  - mod_timer() pushes oom_timer forward again.

Before this change the path ran at most once per AG71XX_OOM_REFILL. Now
it can run once per TX IRQ for as long as memory stays short and TX
traffic keeps flowing.

The commit message says TX delivery is restored "while NAPI waits for RX
buffer memory". It looks like NAPI actually keeps retrying at the TX
interrupt rate, and the OOM_REFILL backoff no longer applies. Is that
intended?

[Severity: Low]
Does this break the driver's rule of masking interrupts before NAPI is
scheduled?

ag71xx_interrupt() always masks AG71XX_INT_POLL before napi_schedule().
ag71xx_oom_timer_handler() schedules NAPI without masking anything:

    static void ag71xx_oom_timer_handler(struct timer_list *t)
    {
        struct ag71xx *ag = timer_container_of(ag, t, oom_timer);

        napi_schedule(&ag->napi);
    }

That was fine while the OOM exit left all of INT_POLL masked. With TX_PS
now unmasked, a poll started by the timer can run with TX interrupts
live. A TX IRQ during that poll masks POLL partway through and sets
NAPI_STATE_MISSED.

The reverse case can also happen. A poll driven by a TX IRQ can leave
the OOM state through the normal exit, which calls
ag71xx_int_enable(ag, AG71XX_INT_POLL). The oom_timer armed above is
never cancelled, so it later schedules a spurious poll with all POLL
interrupts enabled.

Neither case seems to corrupt state. Is the extra poll and interrupt
traffic expected, though?

[Severity: Low]
This isn't a bug introduced by this patch, but can this new enable set
TX_PS in AG71XX_REG_INT_ENABLE again after ag71xx_hw_stop() has cleared
it?

ag71xx_hw_disable() stops the hardware before it quiesces NAPI and the
timer:

    ag71xx_hw_stop(ag);
    ag71xx_dma_reset(ag);

    napi_disable(&ag->napi);
    timer_delete_sync(&ag->oom_timer);

ag71xx_fast_reset() does not quiesce NAPI at all. The normal exit in
ag71xx_poll() already re-enables AG71XX_INT_POLL in the same window, so
the window predates this change.

The new line's effect there looks negligible:

  - ag71xx_dma_reset() clears the pending PS count.
  - TX DMA is stopped.
  - ag71xx_hw_start() later overwrites INT_ENABLE with AG71XX_INT_INIT.

[Severity: Low]
Should this unmask depend on what napi_complete() returns?

include/linux/netdevice.h documents napi_complete_done() as returning
"false if device should avoid rearming interrupts".
Documentation/networking/napi.rst says "IRQ should only be unmasked
after a successful call to napi_complete_done()".

napi_complete_done() returns false in these cases:

  - NAPIF_STATE_NPSVC or NAPIF_STATE_IN_BUSY_POLL is set.
  - NAPIF_STATE_MISSED was set, and NAPI has been rescheduled.

TX_PS can now be unmasked during timer-driven polls, so MISSED can
happen here. In those cases TX is unmasked while NAPI is still
scheduled, which gives a spurious IRQ.

This would follow the documented contract:

    if (napi_complete(napi))
        ag71xx_int_enable(ag, AG71XX_INT_TX);

The existing normal exit path has the same unchecked pattern, but this
patch adds a new instance of it.

[Severity: Low]
ag71xx_int_enable() does an unlocked read-modify-write of
AG71XX_REG_INT_ENABLE through ag71xx_sb():

    iowrite32(ioread32(r) | mask, r);

Could the new TX unmask race with ag71xx_int_disable() in
ag71xx_interrupt() and lose an update?

As far as I can tell this is harmless in practice:

  - AG71XX depends on ATH79 || COMPILE_TEST, and ATH79 is uniprocessor.
    Two polls can't interleave their read-modify-writes.
  - A hardirq that preempts this read-modify-write can only lose its own
    mask. TX_PS stays asserted and the line is level-sensitive, so the
    interrupt fires again and ag71xx_interrupt() masks POLL again.

The worst case looks like one extra interrupt.

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