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

[Simon Horman]

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 support to prevent crash after resume

This patch adds suspend/resume callbacks to the mvneta_bm driver so the
hardware Buffer Manager is drained on suspend and reinitialised on resume,
and adds a device_link from mvneta to mvneta_bm so the ordering of the two
drivers' PM transitions is enforced.

> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 0c061fb0ed07..cfaf5ea1db9e 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -5678,6 +5678,11 @@ static int mvneta_probe(struct platform_device *pdev)
>  					 "use SW buffer management\n");
>  				mvneta_bm_put(pp->bm_priv);
>  				pp->bm_priv = NULL;
> +			} else {
> +				/* Ensure BM suspends after us, resumes before us */
> +				device_link_add(&pdev->dev,
> +						&pp->bm_priv->pdev->dev,
> +						DL_FLAG_AUTOREMOVE_CONSUMER);

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

device_link_add() can return NULL on failure (allocation failure,
supplier-not-yet-bound, invalid flag combinations, or a pre-existing
reverse dependency). The commit message and the comment both indicate
that the suspend/resume ordering correctness depends on this link
existing.

If the link is not created, the system silently regresses to the
pre-patch ordering: BM may suspend before mvneta finishes its own
suspend, or resume after mvneta and hand back stale buffer pointers.
The surrounding block already falls back to SW buffer management when
mvneta_bm_port_init() fails; would a similar dev_warn (or fallback) be
appropriate here when device_link_add() returns NULL?

>  			}
>  		}
>  		/* Set RX packet offset correction for platforms, whose
> diff --git a/drivers/net/ethernet/marvell/mvneta_bm.c b/drivers/net/ethernet/marvell/mvneta_bm.c
> index 6bb380494919..ff2fe4020a45 100644
> --- a/drivers/net/ethernet/marvell/mvneta_bm.c
> +++ b/drivers/net/ethernet/marvell/mvneta_bm.c
> @@ -477,6 +477,52 @@ static void mvneta_bm_remove(struct platform_device *pdev)

[ ... ]

> +static int mvneta_bm_resume(struct device *dev)
> +{
> +	struct mvneta_bm *priv = dev_get_drvdata(dev);
> +	int err;
> +
> +	err = clk_prepare_enable(priv->clk);
> +	if (err)
> +		return err;
> +
> +	/* Reinitialize BM hardware; pools are refilled by mvneta_resume() */
> +	mvneta_bm_default_set(priv);
> +	mvneta_bm_write(priv, MVNETA_BM_COMMAND_REG, MVNETA_BM_START_MASK);
> +	return 0;
> +}

[High]
Is this hardware re-initialisation on resume complete?

The commit message says "mvneta disables its clock, causing BM to lose
all buffer address state". If clock gating loses BM state, the per-pool
size and ring pointer registers programmed in mvneta_bm_pools_init()
would also be lost:

	/* Reset read pointer */
	mvneta_bm_write(priv, MVNETA_BM_POOL_READ_PTR_REG(i), 0);

	/* Reset write pointer */
	mvneta_bm_write(priv, MVNETA_BM_POOL_WRITE_PTR_REG(i), 0);
	...
	mvneta_bm_write(priv, MVNETA_BM_POOL_SIZE_REG(i),
			bm_pool->hwbm_pool.size);

mvneta_bm_default_set() only touches MVNETA_BM_INTR_MASK_REG,
MVNETA_BM_INTR_CAUSE_REG, and the burst-size bits in
MVNETA_BM_CONFIG_REG.

The downstream mvneta_bm_pool_create() invoked via mvneta_resume() ->
mvneta_bm_port_init() -> mvneta_bm_pool_use() writes only
POOL_BASE_REG, the XBAR target/attr, and the ENABLE bit; it does not
program POOL_SIZE_REG, POOL_READ_PTR_REG, or POOL_WRITE_PTR_REG.

After resume, POOL_SIZE_REG would hold its hardware reset value while
hwbm_pool->size in software is non-zero. Could buffer puts via
mvneta_bm_pool_put_bp() then misuse the BPPE ring and recreate the
class of failure this patch is trying to fix?

Would it be more correct for mvneta_bm_resume() to call
mvneta_bm_pools_init() (or rewrite SIZE/READ_PTR/WRITE_PTR for every
pool) instead of only mvneta_bm_default_set()?

> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(mvneta_bm_pm_ops, mvneta_bm_suspend, mvneta_bm_resume);
> +