Re: [PATCH net-next v10 1/8] net: macb: introduce macb_context struct for buffer management

[netdev-bot+sashiko@kernel.org]

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

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

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 NAPI poll handlers read the context after
  napi_complete_done(), and nothing stops that context from being freed…
- [Low] The patch changes what `ethtool -g` reports on AT91 EMAC devices,
  and the commit message does not say so.
- [Low] On EMAC, at91ether_open() kzallocs bp->ctx and never sets
  ctx->rx_ring_size or ctx->tx_ring_size, so both stay 0.

Pre-existing issues:
- [Medium] macb_set_ringparam() calls macb_close(), updates
  configured_*_ring_size, and then calls macb_open(bp->netdev) without…

--

net: macb: introduce macb_context struct for buffer management

This commit moves the per-queue ring buffers and ring parameters out of
struct macb and struct macb_queue into a new struct macb_context. The
context is allocated in macb_open()/at91ether_open() and freed on close.
User-requested ring sizes are now kept in bp->configured_{rx,tx}_ring_size
so they persist while the interface is down.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 20fe307898343..73ee650e09060 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -2011,15 +2011,16 @@ static void macb_tx_restart(struct macb_queue *queue)
>  
>  static bool macb_tx_complete_pending(struct macb_queue *queue)
>  {
> +	struct macb_txq *txq = macb_txq(queue);
>  	bool retval = false;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&queue->tx_ptr_lock, flags);
> -	if (queue->tx_head != queue->tx_tail) {
> +	if (txq->head != txq->tail) {

[Severity: Medium]
Can bp->ctx be freed while this is running?

macb_tx_poll() calls this after napi_complete_done() has already released
NAPI ownership:

    if (work_done < budget && napi_complete_done(napi, work_done)) {
        queue_writel(queue, IER, MACB_BIT(TCOMP));
        ...
        if (macb_tx_complete_pending(queue)) {

napi_disable() waits only for ownership to be released. It does not wait
for the poll function to return. Documentation/networking/napi.rst says:

    napi_disable() and subsequent calls to the poll method only wait for
    the ownership of the instance to be released, not for the poll method
    to exit. This means that drivers should avoid accessing any data
    structures after calling napi_complete_done().

That seems to allow this interleaving:

CPU A                               CPU B
macb_tx_poll()
  napi_complete_done() == true
  <delayed>
                                    macb_close()
                                      macb_quiesce_start()
                                        napi_disable()  /* returns */
                                      ...
                                      macb_free(bp);
                                      kfree(bp->ctx);
                                      bp->ctx = NULL;
  macb_tx_complete_pending()
    macb_txq()
      &bp->ctx->txq[q]   /* NULL or freed */
    txq->head

Before this patch, queue->tx_head and queue->tx_tail lived in bp->queues
and were always valid. The freed ring was only touched when head != tail.
Now txq points into the ctx, so an empty TX ring also hits the freed or
NULL memory.

The RX side (macb_rx_poll()->macb_rx_pending()->macb_rxq()) already had a
similar window on rx_ring before this patch. Now it is on the ctx itself.

This window also seems to remain at the end of the series. There,
macb_close() and macb_context_swap_end() free the old ctx after
napi_disable(), with no further synchronization.

Should the poll handlers avoid touching ctx after napi_complete_done()?
Alternatively, should the ctx free wait for in-flight poll calls to return?

[ ... ]

> @@ -3822,8 +3912,8 @@ static int macb_set_ringparam(struct net_device *netdev,
>  		macb_close(bp->netdev);
>  	}
>  
> -	bp->rx_ring_size = new_rx_size;
> -	bp->tx_ring_size = new_tx_size;
> +	bp->configured_rx_ring_size = new_rx_size;
> +	bp->configured_tx_ring_size = new_tx_size;
>  
>  	if (reset)
>  		macb_open(bp->netdev);

[Severity: Medium]
This is a pre-existing issue, but the macb_open() return value is ignored
here.

With this patch, every macb_open() error path ends with bp->ctx == NULL:

    free_ctx:
        kfree(bp->ctx);
        bp->ctx = NULL;

Meanwhile the netdev stays IFF_UP. A later dev_close()->macb_close() calls
macb_quiesce_start()->napi_disable() on NAPI instances that were left
disabled, and that hangs. The same hang could already happen before this
patch when macb_alloc() failed on reopen. If that point were somehow
passed, macb_free() would then dereference the NULL bp->ctx.

This patch adds one more way for this path to fail: the kzalloc_obj() of
ctx.

The later commit "net: macb: use context swapping in .set_ringparam()" in
this series seems to fix this. It allocates the new context first and
returns the error with the interface untouched.

Should the macb_open() failure be handled here until then?

[ ... ]

> @@ -5177,6 +5267,13 @@ static int at91ether_open(struct net_device *netdev)
>  	if (ret < 0)
>  		return ret;
>  
> +	WRITE_ONCE(bp->ctx, kzalloc_obj(*bp->ctx));
> +	wmb(); /* ensure IRQ enabled below read the new context */
> +	if (!bp->ctx) {
> +		ret = -ENOMEM;
> +		goto pm_exit;
> +	}
> +

[Severity: Low]
Unlike macb_open(), at91ether_open() never sets ctx->rx_ring_size or
ctx->tx_ring_size. Both stay 0, while bp->configured_* holds 9/1.

Shared helpers still read these fields. For example,
at91ether_start()->macb_rx_desc()->macb_rx_ring_wrap() does:

    return index & (bp->ctx->rx_ring_size - 1);

With a size of 0 the mask is UINT_MAX, so indices pass through unchanged.
This matches the pre-patch behaviour, where bp->rx_ring_size was also 0
on EMAC.

It only works because at91ether_start() bounds its loop with
AT91ETHER_MAX_RX_DESCR and at91ether_rx() wraps rxq->tail by hand.

If the configured value were copied into ctx the way macb_open() does it,
there would be a problem. AT91ETHER_MAX_RX_DESCR (9) is not a power of
two, so the mask would be 8 and descriptors 1..7 would alias descriptor 0.

Is it intended that the EMAC ctx has ring sizes of 0 while configured_*
reports 9/1?

[ ... ]

> @@ -5998,6 +6101,15 @@ static int macb_probe(struct platform_device *pdev)
>  	/* setup capabilities */
>  	macb_configure_caps(bp, macb_config);
>  
> +	if (bp->caps & MACB_CAPS_MACB_IS_EMAC) {
> +		/* The name is lying on EMAC: sizes aren't configurable. */
> +		bp->configured_rx_ring_size = AT91ETHER_MAX_RX_DESCR;
> +		bp->configured_tx_ring_size = 1; /* tiny ring buffer */
> +	} else {

[Severity: Low]
This changes what ethtool -g reports on EMAC devices.

Before this patch, only macb_init_dflt() set bp->rx_ring_size and
bp->tx_ring_size. EMAC goes through at91ether_init() instead, but it still
installs macb_ethtool_ops. As a result, macb_get_ringparam() reported 0/0
on EMAC. With this hunk it reports 9/1.

The new values match the hardware: TX only ever uses rm9200_txq[0]. So
this looks like a fix to the reporting.

However, the commit message describes configured_{rx,tx}_ring_size only as
a way to keep the sizes across close. Could it also mention the
user-visible ethtool -g change on EMAC?

Also, at91ether_open() does not copy configured_* into the ctx (see the
comment above), so the reported values and the ctx disagree.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001-macb-context-v10-0-beb541bbb7df%40bootlin.com