Re: [PATCH net-next v2 1/9] net: sparx5/lan969x: stop the FDMA channels properly

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 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] If the FDMA_CH_ACTIVE poll in sparx5_fdma_stop()
  (sparx5_fdma.c:486-493) times out, the function only calls…
- [Low] In sparx5_fdma_stop()
  (drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c:472-509), the…

--

net: sparx5/lan969x: stop the FDMA channels properly

This commit changes sparx5_fdma_stop() to disable the FDMA channels
through FDMA_CH_DISABLE and to poll the new FDMA_CH_ACTIVE register until
they are inactive. It also stops the injection port through INJ_STOP. The
XTR_BUF_IS_EMPTY poll now waits for the buffer to be empty instead of
non-empty.

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> index 15ec9b053824f..bcae0bbd5e44e 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c

[ ... ]

> @@ -468,20 +471,39 @@ int sparx5_fdma_start(struct sparx5 *sparx5)
>  
>  int sparx5_fdma_stop(struct sparx5 *sparx5)
>  {
> +	u32 channels = BIT(sparx5->rx.fdma.channel_id) |
> +		       BIT(sparx5->tx.fdma.channel_id);
>  	struct sparx5_rx *rx = &sparx5->rx;
>  	struct sparx5_tx *tx = &sparx5->tx;
>  	u32 val;
>  
>  	napi_disable(&rx->napi);
>  
> -	/* Stop the fdma and channel interrupts */
> +	/* Disable the channels, and wait for them to finish the current DCB */
> +	spx5_wr(FDMA_CH_DISABLE_CH_DISABLE_SET(channels), sparx5,
> +		FDMA_CH_DISABLE);
> +
> +	if (read_poll_timeout(sparx5_fdma_ch_active,
> +			      val,
> +			      !(FDMA_CH_ACTIVE_CH_ACTIVE_GET(val) & channels),
> +			      SPX5_FDMA_STOP_SLEEP_US,
> +			      SPX5_FDMA_STOP_TIMEOUT_US,
> +			      false,
> +			      sparx5))
> +		dev_warn(sparx5->dev, "Failed to stop FDMA channels\n");

[Severity: Medium]
If this FDMA_CH_ACTIVE poll times out, what stops the channels from
continuing to DMA into memory that the callers free right afterwards?

In that case sparx5_fdma_stop() only warns and still returns 0. Both
deinit callers ignore the return value anyway, and free the DMA memory
without resetting the FDMA:

sparx5_fdma_deinit() {
	sparx5_fdma_stop(sparx5);
	fdma_free_phys(&sparx5->rx.fdma);
	fdma_free_phys(&sparx5->tx.fdma);
	...
}

lan969x_fdma_deinit() {
	...
	sparx5_fdma_stop(sparx5);
	fdma_free_coherent(sparx5->dev, &tx->fdma);
	fdma_free_coherent(sparx5->dev, &rx->fdma);
	lan969x_fdma_free_pages(rx);
	page_pool_destroy(rx->page_pool);
	...
}

A channel that is still active keeps the DCB addresses in its LLP
registers. Could it then fetch DCBs from the freed coherent rings, or
write extracted frames into RX pages or skbs that have already gone back
to the allocator? Unbind/remove reaches this through
sparx5_frame_io_deinit()->ops->fdma_deinit()->sparx5_fdma_stop().

The code alone doesn't show whether a channel can really stay active
longer than SPX5_FDMA_STOP_TIMEOUT_US after CH_DISABLE. Still, the timeout
branch exists, and the frees after it run no matter what the stop returns.

Later in this series, "net: sparx5/lan969x: add support for resizing the
FDMA buffers" adds sparx5_fdma_pause(). It follows sparx5_fdma_stop() with
an FDMA_CTRL NRESET before the buffers are freed. That covers the resize
path, but the deinit paths still have no reset.

Would it make sense to return -ETIMEDOUT from sparx5_fdma_stop(), or to
assert FDMA_CTRL NRESET in the deinit paths before releasing the DMA
memory, or both?

> +
> +	/* Stop the fdma and the RX channel interrupt */
>  	sparx5_fdma_rx_deactivate(sparx5, rx);
>  	sparx5_fdma_tx_deactivate(sparx5, tx);
>  
> -	/* Wait for the RX channel to stop */
> -	read_poll_timeout(sparx5_fdma_port_ctrl, val,
> -			  FDMA_PORT_CTRL_XTR_BUF_IS_EMPTY_GET(val) == 0,
> -			  500, 10000, 0, sparx5);
> +	/* Wait for the extraction buffer to be empty */
> +	read_poll_timeout(sparx5_fdma_port_ctrl,
> +			  val,
> +			  FDMA_PORT_CTRL_XTR_BUF_IS_EMPTY_GET(val),
> +			  SPX5_FDMA_STOP_SLEEP_US,
> +			  SPX5_FDMA_STOP_TIMEOUT_US,
> +			  false,
> +			  sparx5);

[Severity: Low]
Can this poll still time out while RX traffic is arriving?

The extraction channel is disabled and polled inactive first, while the
extraction port is still running. XTR_STOP is only set after that, in
sparx5_fdma_rx_deactivate():

	/* Stop RX fdma */
	spx5_rmw(FDMA_PORT_CTRL_XTR_STOP_SET(1), FDMA_PORT_CTRL_XTR_STOP,
		 sparx5, FDMA_PORT_CTRL(0));

Frames may keep reaching the CPU extraction queue during that window, and
CPU forwarding (QFWD_SWITCH_PORT_MODE_PORT_ENA) stays enabled on the
deinit path. If so, there may be no active channel left to move data out
of the extraction buffer, and XTR_BUF_IS_EMPTY might never become 1.

The result of this read_poll_timeout() is also discarded and nothing is
printed. A non-empty buffer therefore looks the same as a clean stop.

The commit message says the old wait "always runs until it times out".
Could the new wait still do that in this case?

Later in the series, sparx5_fdma_pause() disables CPU forwarding and
drains the CPU queues before it calls sparx5_fdma_stop(). That suggests
the stop sequence alone does not handle extraction that is still in
progress.

Would it help to set XTR_STOP before disabling the channel and poll
XTR_BUF_IS_EMPTY while the channel is still active? Another option is to
flush the buffer with FDMA_PORT_CTRL_XTR_BUF_RST, which is defined in
sparx5_main_regs.h but never used. At the very least, checking and
reporting the poll result would keep the diagnostic.

>  
>  	return 0;
>  }

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