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