Re: [PATCH 01/10] dmapool: remove checks for dev == NULL

[Robin Murphy]

On 2022-05-31 19:12, Tony Battersby wrote:
> dmapool originally tried to support pools without a device because
> dma_alloc_coherent() supports allocations without a device.  But nobody
> ended up using dma pools without a device, so the current checks in
> dmapool.c for pool->dev == NULL are both insufficient and causing bloat.
> Remove them.

Furthermore, dma_pool_create() already dereferences the dev argument 
unconditionally, so there's no way we could possibly get as far as these 
checks even if a caller did ever pass NULL.

Reviewed-by: Robin Murphy <robin.murphy@xxxxxxx>

> Signed-off-by: Tony Battersby <tonyb@xxxxxxxxxxxxxxx>
> ---
>   mm/dmapool.c | 42 +++++++++++-------------------------------
>   1 file changed, 11 insertions(+), 31 deletions(-)
>
> diff --git a/mm/dmapool.c b/mm/dmapool.c
> index a7eb5d0eb2da..0f89de408cbe 100644
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -275,7 +275,7 @@ void dma_pool_destroy(struct dma_pool *pool)
>   	mutex_lock(&pools_reg_lock);
>   	mutex_lock(&pools_lock);
>   	list_del(&pool->pools);
> -	if (pool->dev && list_empty(&pool->dev->dma_pools))
> +	if (list_empty(&pool->dev->dma_pools))
>   		empty = true;
>   	mutex_unlock(&pools_lock);
>   	if (empty)
> @@ -284,12 +284,8 @@ void dma_pool_destroy(struct dma_pool *pool)
>   
>   	list_for_each_entry_safe(page, tmp, &pool->page_list, page_list) {
>   		if (is_page_busy(page)) {
> -			if (pool->dev)
> -				dev_err(pool->dev, "%s %s, %p busy\n", __func__,
> -					pool->name, page->vaddr);
> -			else
> -				pr_err("%s %s, %p busy\n", __func__,
> -				       pool->name, page->vaddr);
> +			dev_err(pool->dev, "%s %s, %p busy\n", __func__,
> +				pool->name, page->vaddr);
>   			/* leak the still-in-use consistent memory */
>   			list_del(&page->page_list);
>   			kfree(page);
> @@ -351,12 +347,8 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
>   		for (i = sizeof(page->offset); i < pool->size; i++) {
>   			if (data[i] == POOL_POISON_FREED)
>   				continue;
> -			if (pool->dev)
> -				dev_err(pool->dev, "%s %s, %p (corrupted)\n",
> -					__func__, pool->name, retval);
> -			else
> -				pr_err("%s %s, %p (corrupted)\n",
> -					__func__, pool->name, retval);
> +			dev_err(pool->dev, "%s %s, %p (corrupted)\n",
> +				__func__, pool->name, retval);
>   
>   			/*
>   			 * Dump the first 4 bytes even if they are not
> @@ -411,12 +403,8 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
>   	page = pool_find_page(pool, dma);
>   	if (!page) {
>   		spin_unlock_irqrestore(&pool->lock, flags);
> -		if (pool->dev)
> -			dev_err(pool->dev, "%s %s, %p/%pad (bad dma)\n",
> -				__func__, pool->name, vaddr, &dma);
> -		else
> -			pr_err("%s %s, %p/%pad (bad dma)\n",
> -			       __func__, pool->name, vaddr, &dma);
> +		dev_err(pool->dev, "%s %s, %p/%pad (bad dma)\n",
> +			__func__, pool->name, vaddr, &dma);
>   		return;
>   	}
>   
> @@ -426,12 +414,8 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
>   #ifdef	DMAPOOL_DEBUG
>   	if ((dma - page->dma) != offset) {
>   		spin_unlock_irqrestore(&pool->lock, flags);
> -		if (pool->dev)
> -			dev_err(pool->dev, "%s %s, %p (bad vaddr)/%pad\n",
> -				__func__, pool->name, vaddr, &dma);
> -		else
> -			pr_err("%s %s, %p (bad vaddr)/%pad\n",
> -			       __func__, pool->name, vaddr, &dma);
> +		dev_err(pool->dev, "%s %s, %p (bad vaddr)/%pad\n",
> +			__func__, pool->name, vaddr, &dma);
>   		return;
>   	}
>   	{
> @@ -442,12 +426,8 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
>   				continue;
>   			}
>   			spin_unlock_irqrestore(&pool->lock, flags);
> -			if (pool->dev)
> -				dev_err(pool->dev, "%s %s, dma %pad already free\n",
> -					__func__, pool->name, &dma);
> -			else
> -				pr_err("%s %s, dma %pad already free\n",
> -				       __func__, pool->name, &dma);
> +			dev_err(pool->dev, "%s %s, dma %pad already free\n",
> +				__func__, pool->name, &dma);
>   			return;
>   		}
>   	}