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

[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: mvneta_bm: add suspend/resume support to prevent crash after resume
    
This commit adds suspend and resume callbacks to the mvneta Buffer Manager
driver to safely drain and reinitialize buffers across power cycles. It also
adds a device link to guarantee correct PM ordering between mvneta and the BM
hardware.

> diff --git a/drivers/net/ethernet/marvell/mvneta_bm.c b/drivers/net/ethernet/marvell/mvneta_bm.c
> index 6bb3804949190..c23982bfc20b8 100644
> --- a/drivers/net/ethernet/marvell/mvneta_bm.c
> +++ b/drivers/net/ethernet/marvell/mvneta_bm.c

[ ... ]

> @@ -477,6 +477,68 @@ static void mvneta_bm_remove(struct platform_device *pdev)
>  	clk_disable_unprepare(priv->clk);
>  }
>  
> +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);
> +		if (bm_pool->hwbm_pool.buf_num)
> +			dev_warn(&priv->pdev->dev,
> +				 "pool %d: %d buffers not freed\n",
> +				 bm_pool->id, bm_pool->hwbm_pool.buf_num);
> +
> +		mvneta_bm_pool_disable(priv, bm_pool->id);
> +
> +		if (bm_pool->virt_addr) {
> +			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;
> +		}

[Severity: High]
This is a pre-existing issue, but does this introduce a double free during
suspend if mvneta_bm_pool_create() partially failed?

If mvneta_bm_pool_create() succeeds at dma_alloc_coherent() but fails a
subsequent check, the error path calls dma_free_coherent() but leaves
bm_pool->virt_addr as a dangling pointer:

drivers/net/ethernet/marvell/mvneta_bm.c:mvneta_bm_pool_create() {
    ...
    dma_free_coherent(&pdev->dev, size_bytes, bm_pool->virt_addr,
                      bm_pool->phys_addr);
    return err;
}

Since the caller mvneta_bm_pool_use() already set the pool type to something
other than MVNETA_BM_FREE, won't this new suspend handler see the non-NULL
virt_addr and call dma_free_coherent() on it a second time?

> +		bm_pool->type = MVNETA_BM_FREE;

[Severity: Medium]
Will this cause a lockdep warning by re-initializing an active lock class?

By setting the type to MVNETA_BM_FREE here, the resume path (which calls
mvneta_resume() -> mvneta_bm_port_init() -> mvneta_bm_pool_use()) will treat
this as a completely uninitialized pool.

This causes mvneta_bm_pool_use() to unconditionally call:

    mutex_init(&hwbm_pool->buf_lock);

Because the priv->bm_pools array is preserved across suspend, the mutex
was already fully initialized during probe. Doesn't re-initializing the
mutex across suspend/resume cycles reset its internal lockdep state and
wait queue?

> +	}
> +
> +	mvneta_bm_write(priv, MVNETA_BM_COMMAND_REG, MVNETA_BM_STOP_MASK);
> +	clk_disable_unprepare(priv->clk);
> +	return 0;
> +}
-- 
pw-bot: cr