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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: sparx5/lan969x: stop the FDMA channels properly

This commit adds the FDMA_CH_ACTIVE register. sparx5_fdma_stop() now
disables the RX and TX channels through FDMA_CH_DISABLE and waits for
them to become inactive. It also sets INJ_STOP and waits for
XTR_BUF_IS_EMPTY to become 1 instead of 0.

> The stop sequence is used by the FDMA deinit, and by the FDMA buffer
> resize added later in this series.

[Severity: Low]
Should this patch carry a Fixes: tag? These three problems all seem to
come from 10615907e9b5 ("net: sparx5: switchdev: adding frame DMA
functionality"):

  - the FDMA_CH_ACTIVATE read-modify-write that does nothing
  - INJ_STOP is never set
  - the inverted XTR_BUF_IS_EMPTY test

lan969x inherits them through the shared sparx5_fdma_stop().

sparx5_fdma_deinit() and lan969x_fdma_deinit() both free the DCB rings
and data buffers right after this stop. Before this patch the channels
could therefore still be active when that memory was freed. The old code
did set XTR_STOP before the free, though, so actual DMA into freed
memory is plausible but not proven.

The commit message presents the change as groundwork for the resize
patch and does not describe any user-visible impact. Would it make sense
to state the impact, add the tag, and perhaps send this to net
separately from the net-next feature series?

> 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 poll times out, the RX or TX channel may still be active.
sparx5_fdma_stop() only warns and still returns 0, and both callers
ignore the result and free the DMA memory right away:

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);
	...
}

Neither deinit path resets the FDMA with FDMA_CTRL.NRESET after the
stop. Later in the series, only sparx5_fdma_pause() does that.

If a channel does not go inactive within 10 ms, can the FDMA keep
fetching DCBs from the freed rings? Can it keep writing frame data and
DCB status into freed memory, or keep transmitting stale buffer
contents?

Should the error be propagated to the callers, or the FDMA be reset
before the memory is released?

> +
> +	/* 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: Medium]
The extraction stop order is now the reverse of the old code. The old
code set XTR_STOP in sparx5_fdma_rx_deactivate() first and then polled
the extraction buffer. The new sequence is:

sparx5_fdma_stop()
    spx5_wr(... FDMA_CH_DISABLE)
    read_poll_timeout(... FDMA_CH_ACTIVE ...)     up to 10 ms
    sparx5_fdma_rx_deactivate()
        spx5_rmw(FDMA_PORT_CTRL_XTR_STOP_SET(1), ...)
    read_poll_timeout(... XTR_BUF_IS_EMPTY ...)

XTR_STOP is still clear during the CH_ACTIVE wait. Can the queue system
keep pushing CPU-bound frames into the extraction buffer during that
window?

By the time XTR_BUF_IS_EMPTY is polled, the extraction channel is
already inactive. Is anything left that can move the buffered data to
memory?

If not, this poll will hit its full 10 ms timeout whenever frames arrive
in that window. That is the same "wait always runs until it times out"
symptom the commit message says it fixes. The timeout is also dropped
silently here, while the CH_ACTIVE poll above warns.

No extraction buffer reset or DB discard follows the stop either. The
lan966x driver uses the same FDMA IP and does this in
lan966x_fdma_rx_disable() after the channel goes inactive:

	lan_rmw(FDMA_CH_DB_DISCARD_DB_DISCARD_SET(BIT(fdma->channel_id)),
		FDMA_CH_DB_DISCARD_DB_DISCARD,
		lan966x, FDMA_CH_DB_DISCARD);

Later in the series, sparx5_fdma_pause() drains the CPU queues before
the stop. After it, it pulses FDMA_CTRL.NRESET and calls
sparx5_xtr_flush(), which covers the resize path. The deinit path
(sparx5_cleanup_fdma()->fdma_deinit) gets no reset, though. The
ordering and the ignored drain timeout are still in sparx5_fdma_stop()
at the end of the series.

Would it be better to set XTR_STOP before disabling the extraction
channel, and to warn when this drain poll times out?
-- 
pw-bot: cr