Re: [PATCH net v2 1/3] net: macb: never give hardware a NULL RX buffer

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (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] RX can stall permanently once no descriptor is armed (rx_tail ==
  rx_prepared_head) and refill has failed.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  widens it.

--

net: macb: never give hardware a NULL RX buffer

This patch sets RX_USED on every GEM RX descriptor in gem_init_rx_ring()
before the refill. A failed allocation or DMA mapping then cannot leave
hardware-owned slots pointing at NULL buffers. gem_rx() and
macb_rx_pending() now also stop once rx_tail reaches rx_prepared_head.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 8e5c034dc3a48..7f25574928d71 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -1859,6 +1866,10 @@ static bool macb_rx_pending(struct macb_queue *queue)
>  	struct macb_dma_desc *desc;
>  	unsigned int entry;
>  
> +	/* No armed descriptor left: nothing can be pending. */
> +	if (macb_is_gem(bp) && queue->rx_tail == queue->rx_prepared_head)
> +		return false;
> +

[Severity: High]
Can RX on a GEM queue stall for good once every armed descriptor has
been consumed and gem_rx_refill() keeps failing?

Under memory pressure, the sequence would be:

gem_rx()
    rx_tail == rx_prepared_head -> break
    gem_rx_refill()
        netdev_alloc_skb() or dma_map_single() fails -> break
macb_rx_poll()
    napi_complete_done()
    queue_writel(queue, IER, bp->rx_intr_mask)
    macb_rx_pending()
        rx_tail == rx_prepared_head -> return false
    (no napi_schedule())

At that point every descriptor has RX_USED set. The hardware cannot
complete a frame, so RCOMP cannot fire. gem_rx_refill() is only called
from gem_rx() and gem_init_rx_ring(), and no timer or work item retries
it.

bp->rx_intr_mask is MACB_RX_INT_FLAGS. RXUBR is only added for
MACB_CAPS_NEEDS_RSTONUBR parts:

    bp->rx_intr_mask = MACB_RX_INT_FLAGS;
    if (bp->caps & MACB_CAPS_NEEDS_RSTONUBR)
        bp->rx_intr_mask |= MACB_BIT(RXUBR);

Zynq and the EMAC would still get a retry from macb_interrupt() on each
incoming frame through RXUBR.

On other GEM variants, is anything left to restart NAPI apart from
ISR_ROVR, if the controller raises it at all in this state? If not, RX
on that queue would stay dead after memory recovers, until the
interface is closed and reopened.

Before this patch, macb_rx_pending() returned true for the stale
RX_USED slot at rx_tail. NAPI kept rescheduling itself and so kept
retrying gem_rx_refill().

Later in the series, "net: macb: propagate RX ring refill errors"
handles the open-time case where nothing gets armed, by making
macb_open() fail. That commit leaves NAPI, resume and HRESP refill as
best effort. Its message also notes that with zero armed descriptors
nothing would ever trigger a refill. So this steady-state case seems
to remain at the end of the series.

[ ... ]

> @@ -2795,9 +2806,14 @@ static int macb_alloc(struct macb *bp)
>  
>  static void gem_init_rx_ring(struct macb_queue *queue)
>  {
> +	unsigned int i;
> +
>  	queue->rx_tail = 0;
>  	queue->rx_prepared_head = 0;
>  
> +	for (i = 0; i < queue->bp->rx_ring_size; i++)
> +		macb_rx_desc(queue, i)->addr |= MACB_BIT(RX_USED);
> +
>  	gem_rx_refill(queue);
>  }

[Severity: High]
This is a pre-existing issue, but this loop seems to widen it.
macb_hresp_error_task() reinitialises every RX ring without disabling
or synchronising RX NAPI:

macb_hresp_error_task()
    queue_writel(queue, IDR, ...)
    macb_writel(bp, NCR, ctrl & ~(RE | TE))
    bp->macbgem_ops.mog_init_rings(bp)
        gem_init_rx_ring()

It calls neither napi_disable() nor napi_synchronize(), and takes no
lock that gem_rx() also takes. With per-queue IRQs on different CPUs,
or with threaded NAPI, a gem_rx() poll already running on another CPU
keeps touching rx_tail, rx_prepared_head, the descriptors and
rx_skbuff[] at the same time.

The new loop ORs RX_USED into descriptors the hardware still owns
(valid buffer, ctrl == 0). A concurrent gem_rx() may have passed the
cursor check using stale values. Could it then see RX_USED with
ctrl == 0, log "not whole frame pointed by descriptor", and do
rx_tail++?

Suppose that increment lands after rx_tail = 0 but before
gem_rx_refill() runs. CIRC_SPACE(0, 1, rx_ring_size) is then 0, so
refill arms nothing. The whole ring is left RX_USED and RX stalls.

The same race already existed in other forms before this patch. Both
CPUs can run gem_rx_refill() on the same NULL rx_skbuff[] entry, and
each allocates and maps an skb. That either leaks one skb or leaves
desc->addr and rx_skbuff[] out of sync.

The HRESP refill can also take the rx_skbuff[entry] != NULL branch in
gem_rx_refill() and clear RX_USED on a descriptor whose skb gem_rx() on
the other CPU is about to unmap and hand to napi_gro_receive(). Once RE
is re-enabled, wouldn't the hardware DMA into an skb that the stack
already owns?

Later in the series, "net: macb: quiesce IRQs and drain BH on interface
close" only covers close and the open error path.
macb_hresp_error_task() looks unchanged at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-macb-close-v2-0-360efa565914%40bootlin.com