Re: [PATCH] net: mvneta_bm: add suspend/resume to prevent crash after resume

[Simon Horman <horms@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.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: mvneta_bm: add suspend/resume to prevent crash after resume

This patch adds suspend/resume PM callbacks to the mvneta_bm driver to
avoid a NAPI-time panic after system resume caused by BM hardware
returning stale buffer addresses. The suspend handler stops BM and gates
the clock; the resume handler re-enables the clock, calls
mvneta_bm_default_set() and mvneta_bm_pools_init().

> diff --git a/drivers/net/ethernet/marvell/mvneta_bm.c b/drivers/net/ethernet/marvell/mvneta_bm.c
> index 6bb380494919..a0800b99c007 100644
> --- a/drivers/net/ethernet/marvell/mvneta_bm.c
> +++ b/drivers/net/ethernet/marvell/mvneta_bm.c
> @@ -477,6 +477,30 @@ static void mvneta_bm_remove(struct platform_device *pdev)
>  	clk_disable_unprepare(priv->clk);
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int mvneta_bm_suspend(struct device *dev)
> +{
> +	struct mvneta_bm *priv = dev_get_drvdata(dev);
> +
> +	mvneta_bm_write(priv, MVNETA_BM_COMMAND_REG, MVNETA_BM_STOP_MASK);
> +	clk_disable_unprepare(priv->clk);
> +	return 0;
> +}

[High]
Does this suspend path leak every RX buffer currently parked in the BM
hardware?

Buffers allocated via mvneta_bm_construct() are tracked only by the BM
unit itself: their virtual address is stored in the first 4 bytes of
the buffer, while only the phys address is pushed to BM through
mvneta_bm_pool_put_bp(). There is no kernel-side shadow list.

Once MVNETA_BM_STOP_MASK is written and the clock is gated, the commit
message itself says BM "loses all buffer address state". On a later
teardown, mvneta_bm_bufs_free() will read zero/garbage out of the now
empty BPPI/BPPE and bail on:

	if (buf_phys_addr == 0)
		continue;

so dma_unmap_single() / hwbm_buf_free() are never called for the
original allocations. Should suspend drain BM via mvneta_bm_bufs_free()
while BM still knows the addresses, and resume refill via
hwbm_pool_add()?

> +
> +static int mvneta_bm_resume(struct device *dev)
> +{
> +	struct mvneta_bm *priv = dev_get_drvdata(dev);
> +
> +	clk_prepare_enable(priv->clk);

[Medium]
Should the return value of clk_prepare_enable() be checked here?

mvneta_bm_probe() checks the same call and propagates the error. If
the call fails on the resume path, the immediately-following readl/
writel accesses in mvneta_bm_default_set() (MVNETA_BM_INTR_MASK_REG,
MVNETA_BM_INTR_CAUSE_REG, MVNETA_BM_CONFIG_REG) and
mvneta_bm_pools_init() (MVNETA_BM_COMMAND_REG and per-pool registers)
target an unclocked peripheral, which on Armada 380 typically raises a
synchronous external abort.

> +	mdelay(1);

[Low]
Resume callbacks for system sleep run in process context. Could
usleep_range() be used here instead of the busy-wait mdelay(1)?

Also, what is the 1ms delay waiting for? The probe path does the same
clk_prepare_enable() followed by mvneta_bm_default_set() with no
delay, and there is no comment or datasheet citation explaining what
this is for.

> +	mvneta_bm_default_set(priv);
> +	mvneta_bm_pools_init(priv);

[High, High]
Two questions about reusing mvneta_bm_pools_init() on the resume path.

First, does resume leave BM hardware with no buffer addresses, in spite
of the commit log saying the resume should "restart pools"?

mvneta_bm_pools_init() resets bm_pool->type to MVNETA_BM_FREE but does
not reset hwbm_pool->buf_num. The original probe-time fill set
buf_num = hwbm_pool->size, so the post-resume call chain
mvneta_bm_port_init() -> mvneta_bm_pool_use() -> hwbm_pool_add() takes
this early-return in net/core/hwbm.c:

	int hwbm_pool_add(struct hwbm_pool *bm_pool, unsigned int buf_num)
	{
		...
		if (bm_pool->buf_num == bm_pool->size) {
			pr_warn("pool already filled\n");
			return bm_pool->buf_num;
		}

hwbm_pool_refill() is never called, so no buffers are constructed and
no phys addresses are pushed via mvneta_bm_pool_put_bp(). Meanwhile,
mvneta_bm_pool_create() reprograms MVNETA_BM_POOL_BASE_REG to a fresh
dma_alloc_coherent() region. Doesn't this leave BM started with an
empty pool, recreating the same class of failure the patch claims to
fix the next time mvneta_rx_hwbm() runs?

Second, can this leak the previously allocated BPPE region per
suspend/resume cycle?

mvneta_bm_pools_init() unconditionally sets bm_pool->type =
MVNETA_BM_FREE for every pool but does not free
bm_pool->virt_addr / bm_pool->phys_addr (the DMA-coherent BPPE region
allocated by mvneta_bm_pool_create() during probe). On resume,
mvneta_bm_port_init() -> mvneta_bm_pool_use() observes
new_pool->type == MVNETA_BM_FREE and re-enters
mvneta_bm_pool_create(), which calls dma_alloc_coherent() and
overwrites virt_addr/phys_addr without dma_free_coherent() on the
previous allocation. mvneta_bm_pool_destroy() is the only freer of
virt_addr and is not called from the suspend path.

> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(mvneta_bm_pm_ops, mvneta_bm_suspend, mvneta_bm_resume);
> +
>  static const struct of_device_id mvneta_bm_match[] = {
>  	{ .compatible = "marvell,armada-380-neta-bm" },
>  	{ }
> @@ -489,6 +513,7 @@ static struct platform_driver mvneta_bm_driver = {
>  	.driver = {
>  		.name = MVNETA_BM_DRIVER_NAME,
>  		.of_match_table = mvneta_bm_match,
> +		.pm = &mvneta_bm_pm_ops,
>  	},
>  };

[Medium]
Is the suspend/resume ordering between the mvneta_bm supplier and the
mvneta consumer guaranteed by anything beyond dpm_list registration
order?

mvneta_bm_get() does of_find_device_by_node() and
platform_get_drvdata(), but does not call device_link_add(). Without a
device link or equivalent dpm dependency, ordering between two
independent platform_drivers is determined by probe completion order.
Under deferred-probe scenarios (e.g., mvneta probes first with the
bm_priv == NULL fallback, then mvneta_bm probes later), would the
ordering still keep mvneta_bm_resume() before any mvneta_resume() ->
mvneta_bm_port_init() touching BM registers, and the mvneta consumer
quiesced before mvneta_bm_suspend() stops BM and gates the clock?

Would adding a device_link_add(supplier=bm_pdev, consumer=mvneta_pdev,
DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER) when a consumer
attaches make this ordering explicit?
-- 
pw-bot: changes-requested