Re: [PATCH net v2 2/3] net: macb: propagate RX ring refill errors

[netdev-bot+sashiko@kernel.org]

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

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

- [High] The patch adds a failure path to macb_open(): if
  mog_init_rings() (gem_init_rings()) finds any queue with zero prepared…
- [Medium] The patch treats a GEM RX queue with zero prepared descriptors
  as unrecoverable.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  macb_hresp_error_task() re-initialises every RX ring with no exclusion…

--

net: macb: propagate RX ring refill errors

This commit changes gem_rx_refill(), gem_init_rx_ring(), gem_init_rings(),
macb_init_rings() and the mog_init_rings op so they return errno-style ints.
gem_rx_refill() now returns -ENOMEM when a queue ends up with zero prepared
RX descriptors, and macb_open() fails in that case. The other refill callers
(resume, the HRESP error task and NAPI) still ignore the result.

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

[ ... ]

> @@ -1558,6 +1558,14 @@ static void gem_rx_refill(struct macb_queue *queue)
>  
>  	netdev_vdbg(bp->netdev, "rx ring: queue: %p, prepared head %d, tail %d\n",
>  		    queue, queue->rx_prepared_head, queue->rx_tail);
> +
> +	/* Fail if queue has zero prepared descriptors. This is critical because
> +	 * nothing will ever trigger a refill again.
> +	 */
> +	if (queue->rx_prepared_head == queue->rx_tail)
> +		return -ENOMEM;
> +
> +	return 0;
>  }

[Severity: Medium]

This comment says a queue with zero prepared descriptors can never be
refilled. What happens when the other callers reach that state? Only
macb_open() checks the new return value. The other three paths drop it:

gem_rx()
    gem_rx_refill(queue);

macb_hresp_error_task()
    bp->macbgem_ops.mog_init_rings(bp);

macb_resume()
    if (macb_is_gem(bp))
        gem_init_rx_ring(queue);

gem_init_rx_ring() resets rx_tail and rx_prepared_head to 0 and sets RX_USED
on every descriptor. gem_rx_refill() stops at the first failed
netdev_alloc_skb() or dma_map_single(). So resume and HRESP recovery end up
with zero armed descriptors whenever slot 0 has no skb and its allocation
fails.

In the NAPI path, gem_rx() can drain up to rx_prepared_head, and then the
end-of-poll refill can fail on the first empty slot.

Once there, macb_rx_pending() returns early:

    if (macb_is_gem(bp) && queue->rx_tail == queue->rx_prepared_head)
        return false;

That means macb_rx_poll() does not reschedule NAPI. RCOMP also cannot fire
while every descriptor is software-owned. On top of that,
macb_hresp_error_task() goes on to set NCR.RE|TE and call
netif_carrier_on() and netif_tx_start_all_queues() on that ring.

The commit message says:

  "Note that other callers of refill (resume, HRESP error task, NAPI)
   cannot do anything useful with that error and keep their best-effort
   refill, hoping it will improve."

Doesn't the new comment say the opposite, that nothing will trigger another
refill? Would the queue then stay dead until the interface is taken down and
brought back up, while carrier and TX still report up? On a single-queue GEM
that would be all RX.

One caveat: ISR_ROVR is part of rx_intr_mask, and macb_interrupt() schedules
napi_rx on it. If the hardware raises receive-overrun while no descriptors
are available, NAPI would run and retry the refill, which would contradict
the new comment. The code alone doesn't settle this.

[ ... ]

> @@ -2814,14 +2822,16 @@ static void gem_init_rx_ring(struct macb_queue *queue)
>  	for (i = 0; i < queue->bp->rx_ring_size; i++)
>  		macb_rx_desc(queue, i)->addr |= MACB_BIT(RX_USED);
>  
> -	gem_rx_refill(queue);
> +	return gem_rx_refill(queue);
>  }
>  
> -static void gem_init_rings(struct macb *bp)
> +static int gem_init_rings(struct macb *bp)

[ ... ]

> @@ -2834,11 +2844,15 @@ static void gem_init_rings(struct macb *bp)
>  		queue->tx_head = 0;
>  		queue->tx_tail = 0;
>  
> -		gem_init_rx_ring(queue);
> +		err = gem_init_rx_ring(queue);
> +		if (err)
> +			last_err = err;
>  	}
> +
> +	return last_err;
>  }

[Severity: High]

This is a pre-existing issue, not one introduced by this patch.
macb_hresp_error_task() re-initialises every RX ring through this helper,
and nothing stops a NAPI RX poll from running at the same time.

macb_interrupt_misc() queues the task on system_bh_wq, on whichever CPU took
the HRESP interrupt:

    queue_work(system_bh_wq, &bp->hresp_err_bh_work);

The task masks interrupts through IDR and clears NCR.RE/TE. It then calls
mog_init_rings() -> gem_init_rx_ring() -> gem_rx_refill(). That resets
rx_tail and rx_prepared_head and re-arms every slot whose rx_skbuff[entry]
is non-NULL.

Meanwhile, gem_rx() for another queue (or threaded NAPI) can be running on
another CPU:

    skb = queue->rx_skbuff[entry];
    ...
    queue->rx_skbuff[entry] = NULL;

After that it unmaps the buffer, passes the skb to napi_gro_receive(), and
calls gem_rx_refill() itself.

Neither path takes bp->lock, and the task never calls napi_disable() or
otherwise synchronises with NAPI. Can the HRESP refill see a stale non-NULL
rx_skbuff[entry] and clear RX_USED on a descriptor whose buffer was just
unmapped and handed to the stack?

If so, the MAC would DMA into memory owned by the network stack once NCR.RE
is set again. The two refills could also both allocate for the same NULL
slot, leaking one skb and its DMA mapping.

This looks unresolved at the end of the series too. "net: macb: quiesce
IRQs and drain BH on interface close" drains this work only in close and in
the open error path. macb_hresp_error_task() still does not disable or
synchronise with NAPI.

[ ... ]

> @@ -3183,7 +3199,9 @@ static int macb_open(struct net_device *netdev)
>  		goto pm_exit;
>  	}
>  
> -	bp->macbgem_ops.mog_init_rings(bp);
> +	err = bp->macbgem_ops.mog_init_rings(bp);
> +	if (err)
> +		goto free_rings;
>  	macb_init_buffers(bp);
>  
>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {

[Severity: High]

Could this new failure path turn a ring size change into a hang on the next
ifdown? On a running interface, macb_set_ringparam() does:

    if (netif_running(bp->netdev)) {
        reset = 1;
        macb_close(bp->netdev);
    }
    ...
    if (reset)
        macb_open(bp->netdev);

    return 0;

It ignores the return value of macb_open(). By this point macb_close() has
already called napi_disable() on every napi_rx/napi_tx, plus macb_free()
and pm_runtime_put().

If mog_init_rings() now fails here, goto free_rings skips the napi_enable()
loop. NAPI stays disabled, but the netdev still has IFF_UP and
__LINK_STATE_START set.

The next "ip link set dev X down" calls macb_close() again. Its
napi_disable() ends up in napi_disable_locked():

    while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
        usleep_range(20, 200);
        val = READ_ONCE(n->state);
    }

Won't this loop forever while holding RTNL? The second macb_close() would
also call pm_runtime_put() again and underflow the runtime PM usage count.

Before this patch, the same GFP_ATOMIC netdev_alloc_skb() failure let
macb_open() succeed with one degraded queue. The unchecked macb_open() in
macb_set_ringparam() could already be hit through macb_alloc() failures,
but this new failure path makes it much more likely.

This also looks unresolved at the end of the series. macb_set_ringparam()
still ignores the return value of macb_open(), and macb_close() ->
macb_quiesce_start() still calls napi_disable() on the already-disabled
NAPI.

[ ... ]

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