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

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

[Yun Zhou]

The mvneta driver uses the hardware Buffer Manager (BM) for RX buffer
allocation. During suspend, mvneta disables its clock, causing BM to
lose all buffer address state. On resume, mvneta_bm_port_init() re-
attaches the BM pool to the NIC, but BM hardware returns stale/garbage
buffer addresses. When NAPI poll processes these buffers, DMA cache
sync hits an invalid virtual address causing a kernel panic:

 Unable to handle kernel paging request at virtual address b0000080
 PC is at v7_dma_inv_range
 Call trace:
  v7_dma_inv_range from arch_sync_dma_for_cpu+0x94/0x158
  arch_sync_dma_for_cpu from __dma_sync_single_for_cpu+0xc4/0x15c
  __dma_sync_single_for_cpu from mvneta_rx_swbm+0x6c8/0xf48
  mvneta_rx_swbm from mvneta_poll+0x6fc/0x70c
  mvneta_poll from __napi_poll.constprop.0+0x2c/0x1e0
  __napi_poll.constprop.0 from net_rx_action+0x160/0x2c4
  net_rx_action from handle_softirqs+0xd8/0x2b8
  handle_softirqs from run_ksoftirqd+0x30/0x94
  run_ksoftirqd from smpboot_thread_fn+0x100/0x204
  smpboot_thread_fn from kthread+0xf4/0x110
  kthread from ret_from_fork+0x14/0x28

Fix by adding suspend/resume callbacks to the BM driver:

- suspend: drain all buffers (with DMA unmapping), free the BPPE
  regions, and reset pool state to FREE before stopping BM and gating
  the clock.

- resume: enable the clock and reinitialize BM defaults. Pool
  allocation and buffer refill are handled by mvneta_resume() through
  the normal mvneta_bm_port_init() path, which sees pools as FREE and
  performs full initialization identical to probe.

Add a device_link (DL_FLAG_AUTOREMOVE_CONSUMER) in mvneta_probe to
guarantee BM resumes before mvneta and suspends after mvneta.

Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
---
v2:
  - Drain buffers via mvneta_bm_bufs_free() in suspend instead of only
    stopping BM and gating the clock. This ensures proper DMA unmapping
    and avoids buffer leaks.
  - Free the BPPE DMA-coherent region in suspend so that resume takes
    the full probe-time initialization path (alloc + fill), eliminating
    the need to modify mvneta_bm_pool_create().
  - Reset pool type to MVNETA_BM_FREE in suspend so mvneta_bm_pool_use()
    correctly re-creates and refills pools on resume.
  - Check clk_prepare_enable() return value in resume.
  - Add device_link between mvneta (consumer) and mvneta_bm (supplier)
    to guarantee correct suspend/resume ordering.

 drivers/net/ethernet/marvell/mvneta.c    |  5 +++
 drivers/net/ethernet/marvell/mvneta_bm.c | 47 ++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

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);
 			}
 		}
 		/* 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)
 	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);
+	int i;
+
+	/* Drain buffers and free pool resources while BM is still clocked */
+	for (i = 0; i < MVNETA_BM_POOLS_NUM; i++) {
+		struct mvneta_bm_pool *bm_pool = &priv->bm_pools[i];
+		int size_bytes;
+
+		if (bm_pool->type == MVNETA_BM_FREE)
+			continue;
+
+		mvneta_bm_bufs_free(priv, bm_pool, bm_pool->port_map);
+
+		size_bytes = sizeof(u32) * bm_pool->hwbm_pool.size;
+		dma_free_coherent(&priv->pdev->dev, size_bytes,
+				  bm_pool->virt_addr, bm_pool->phys_addr);
+		bm_pool->virt_addr = NULL;
+		bm_pool->type = MVNETA_BM_FREE;
+	}
+
+	mvneta_bm_write(priv, MVNETA_BM_COMMAND_REG, MVNETA_BM_STOP_MASK);
+	clk_disable_unprepare(priv->clk);
+	return 0;
+}
+
+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;
+}
+#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 +535,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,
 	},
 };
 
-- 
2.43.0

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://sashiko.dev
---
net: mvneta_bm: add suspend/resume support to prevent crash after resume

This commit adds suspend and resume callbacks to the mvneta_bm driver
to handle hardware Buffer Manager state correctly across power cycles.
It drains and frees buffers on suspend, then reinitializes the BM hardware
defaults on resume.

> diff --git a/drivers/net/ethernet/marvell/mvneta_bm.c b/drivers/net/ethernet/marvell/mvneta_bm.c
> index 6bb3804949190..ff2fe4020a456 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)
>  	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);
> +	int i;
> +
> +	/* Drain buffers and free pool resources while BM is still clocked */
> +	for (i = 0; i < MVNETA_BM_POOLS_NUM; i++) {
> +		struct mvneta_bm_pool *bm_pool = &priv->bm_pools[i];
> +		int size_bytes;
> +
> +		if (bm_pool->type == MVNETA_BM_FREE)
> +			continue;
> +
> +		mvneta_bm_bufs_free(priv, bm_pool, bm_pool->port_map);
> +
> +		size_bytes = sizeof(u32) * bm_pool->hwbm_pool.size;
> +		dma_free_coherent(&priv->pdev->dev, size_bytes,
> +				  bm_pool->virt_addr, bm_pool->phys_addr);

[Severity: Critical]
Can this dma_free_coherent() call cause a panic or double-free if pool
creation failed during initialization?

If mvneta_bm_pool_create() fails in mvneta_bm_pool_use(), the pool type is
left as non-FREE. For example, if the failure is due to an alignment issue
in mvneta_bm_pool_create():

    if (!IS_ALIGNED((u32)bm_pool->virt_addr, MVNETA_BM_POOL_PTR_ALIGN)) {
            dma_free_coherent(&pdev->dev, size_bytes, bm_pool->virt_addr,
                              bm_pool->phys_addr);
            ...
            return -ENOMEM;
    }

This frees virt_addr but leaves the dangling pointer in bm_pool->virt_addr.
Since the type is no longer MVNETA_BM_FREE, the suspend loop will not
skip it, leading to a second dma_free_coherent() call on the same pointer.

If the allocation itself failed, virt_addr is NULL, which could also lead
to a panic here.

> +		bm_pool->virt_addr = NULL;
> +		bm_pool->type = MVNETA_BM_FREE;

[Severity: Medium]
Does setting the pool type to MVNETA_BM_FREE cause lockdep tracking issues
on resume?

When the device resumes, mvneta_resume() will call mvneta_bm_pool_use()
to recreate the pool. Because the type was reset to MVNETA_BM_FREE here,
mvneta_bm_pool_use() will execute its initialization block:

    if (new_pool->type == MVNETA_BM_FREE) {
        ...
        mutex_init(&hwbm_pool->buf_lock);
    }

Calling mutex_init() on the existing statically-allocated lock structure
wipes its wait list and lockdep state. Is there a way to avoid
re-initializing locks that are already set up?

> +	}
> +
> +	mvneta_bm_write(priv, MVNETA_BM_COMMAND_REG, MVNETA_BM_STOP_MASK);
> +	clk_disable_unprepare(priv->clk);
> +	return 0;
> +}
> +
> +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);

[Severity: Critical]
Is the hardware pool state completely restored during resume?

The commit message notes that mvneta_bm_resume() and mvneta_resume()
perform full initialization identical to probe. However, mvneta_resume()
recreates the pool via mvneta_bm_pool_create(), which only restores the
base address and target attributes:

    mvneta_bm_write(priv, MVNETA_BM_POOL_BASE_REG(bm_pool->id),
                    bm_pool->phys_addr);
    mvneta_bm_pool_target_set(priv, bm_pool->id, target_id,  attr);
    mvneta_bm_pool_enable(priv, bm_pool->id);

It appears the pool capacity (MVNETA_BM_POOL_SIZE_REG) and the read/write
pointers (MVNETA_BM_POOL_READ_PTR_REG and MVNETA_BM_POOL_WRITE_PTR_REG)
are missing, as they are normally configured in mvneta_bm_pools_init()
which isn't called during resume.

Since the BM loses its register state during suspend, won't the hardware
operate with uninitialized or zero capacity after resume? Could this lead
to incorrectly wrapped pointers or dropped buffers when refilling the pool?

> +	return 0;
> +}
> +#endif

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);
> +