Re: [PATCH net-next v2 2/9] net: sparx5/lan969x: free the FDMA buffers on init errors and deinit

[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] On lan969x, the ownership tracking for RX pages is still
  incomplete in the free_page error path of lan969x_fdma_rx_get_frame().

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but both NAPI
  handlers ignore the return value of fdma_dcb_add() when they refill RX…

--

net: sparx5/lan969x: free the FDMA buffers on init errors and deinit

This patch adds sparx5_fdma_rx_free(), lan969x_fdma_rx_free() and
lan969x_fdma_tx_free(). They release the RX skbs/pages, the page_pool,
the TX buffer state and the in-flight TX buffers on init error paths and
on deinit. It also checks the return value of fdma_dcbs_init() for RX,
and stores the mapped TX length so it no longer reads skb->len.

> diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
> index 053d125eda658..8e0f1ddcbf2c4 100644
> --- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
> +++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c

[ ... ]

> @@ -90,12 +90,29 @@ static void lan969x_fdma_free_pages(struct sparx5_rx *rx)
>  	struct fdma *fdma = &rx->fdma;
>  
>  	for (int i = 0; i < fdma->n_dcbs; ++i) {
> -		for (int j = 0; j < fdma->n_dbs; ++j)
> +		for (int j = 0; j < fdma->n_dbs; ++j) {
> +			if (!rx->page[i][j])
> +				continue;
> +
>  			page_pool_put_full_page(rx->page_pool,
>  						rx->page[i][j], false);
> +			rx->page[i][j] = NULL;
> +		}
>  	}
>  }

[Severity: High]
Is a non-NULL slot here always a page that the driver still owns?

lan969x_fdma_rx_get_frame() takes its free_page error path when
WARN_ON(!port) fires or build_skb() fails:

free_page:
	page_pool_recycle_direct(rx->page_pool, page);

	return NULL;

That returns the page to the pool, but
rx->page[fdma->dcb_index][fdma->db_index] still points at it.
lan969x_fdma_napi_poll() then breaks out of the loop without advancing
or refilling that DB:

		skb = lan969x_fdma_rx_get_frame(sparx5, rx);
		if (!skb)
			break;

On a later teardown through
lan969x_fdma_deinit()->lan969x_fdma_rx_free()->lan969x_fdma_free_pages(),
wouldn't page_pool_put_full_page() run on that stale slot a second time?

A page in the pool cache still has pp_ref_count == 1. So
page_pool_unref_netmem() takes the second put as the last reference and
recycles the page again, this time into the ptr_ring, while it also
sits in the alloc cache. page_pool_destroy() would then DMA-unmap the
page twice and call put_page() on it twice.

The commit message says:

  "To know which RX buffers are still owned by the driver, clear the RX
  buffer slot once the buffer has been passed to the stack on Sparx5, and
  make lan969x_fdma_free_pages() skip and clear the empty slots of the
  ring."

However, lan969x does not clear any slot at this commit. A later patch
in the series, "net: sparx5/lan969x: add support for resizing the FDMA
buffers", fixes the success path: it sets rx->page[...] = NULL before
returning the skb. The free_page path still leaves the slot set there.

That later patch also adds lan969x_fdma_resize(), which calls
lan969x_fdma_rx_free() on an MTU change. After it, the double put can
happen without an unbind.

Separately, and this part predates the patch: the stuck DB stays DONE,
so the next poll processes the same, already recycled page again. If
build_skb() succeeds that time, the stack gets an skb on a page that
the refill can hand straight back to the hardware.

Would it work to set rx->page[fdma->dcb_index][fdma->db_index] = NULL
after page_pool_recycle_direct()? The poll loop would also need to
consume or refill that DB, or reuse the page in place, instead of
recycling the page while the DB still references it.

[ ... ]

> @@ -169,14 +186,18 @@ static int lan969x_fdma_rx_alloc(struct sparx5 *sparx5)
>  		return PTR_ERR(rx->page_pool);
>  
>  	err = fdma_alloc_coherent(sparx5->dev, fdma);
> -	if (err)
> +	if (err) {
> +		page_pool_destroy(rx->page_pool);
>  		return err;
> +	}
>  
> -	fdma_dcbs_init(fdma,
> -		       FDMA_DCB_INFO_DATAL(fdma->db_size),
> -		       FDMA_DCB_STATUS_INTR);
> +	err = fdma_dcbs_init(fdma,
> +			     FDMA_DCB_INFO_DATAL(fdma->db_size),
> +			     FDMA_DCB_STATUS_INTR);
> +	if (err)
> +		lan969x_fdma_rx_free(sparx5);
>  
> -	return 0;
> +	return err;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but the same allocation
failure is still unchecked when the RX DCBs are refilled at runtime. The
patch only adds the check at init time.

lan969x_fdma_napi_poll() ignores the return value of fdma_dcb_add():

		fdma_dcb_add(fdma,
			     old_dcb,
			     FDMA_DCB_INFO_DATAL(fdma->db_size),
			     FDMA_DCB_STATUS_INTR);

So does sparx5_fdma_napi_callback() in sparx5_fdma.c:

		fdma_dcb_add(fdma, fdma->dcb_index,
			     FDMA_DCB_INFO_DATAL(fdma->db_size),
			     FDMA_DCB_STATUS_INTR);

__fdma_db_add() in fdma_api.c writes the status, with DONE cleared,
before it calls the dataptr callback:

	db->status = cpu_to_le64(status);

	err = cb(fdma, dcb_idx, db_idx, &dataptr);
	if (unlikely(err))
		return err;

Suppose page_pool_dev_alloc_pages() in lan969x_fdma_rx_dataptr_cb() or
the GFP_ATOMIC __netdev_alloc_skb() in sparx5_fdma_rx_dataptr_cb()
fails. __fdma_dcb_add() then returns before it links the DCB or updates
fdma->last_dcb. The next successful add links around that DCB, so the
hardware skips it.

Once dcb_index wraps around to that DCB, fdma_has_frames() and
fdma_db_is_done() return false. Would both NAPI handlers then stop at
that DCB on every poll? The hardware would fill the remaining DCBs, but
nothing would consume or refill them, and CPU RX would stay stalled
until the driver is rebound.

This code is unchanged through the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-sparx5-mtu-change-v2-0-59c91357e8f6%40microchip.com