Re: [PATCH 1/2] dma: call unconditionally to unmap_page and unmap_sg callbacks

[Robin Murphy]

On 11/07/2024 11:38 am, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@xxxxxxxxxx>
>
> Almost all users of ->map_page()/map_sg() callbacks implement
> ->unmap_page()/unmap_sg() callbacks too. One user which doesn't do it,
> is dummy DMA ops interface, and the use of this interface is to fail
> the operation and in such case, there won't be any call to
> ->unmap_page()/unmap_sg().
>
> This patch removes the existence checks of ->unmap_page()/unmap_sg()
> and calls to it directly to create symmetrical interface to
> ->map_page()/map_sg().

Now that all the common cases have been mopped up by dma-direct, I'm 
inclined to agree that this seems reasonable. For sure there is code out 
there still abusing random DMA API calls as a cache maintenance 
interface because it thinks it knows better, but even that's going to be 
running on platforms where it expects unmap to have the desired effect 
anyway, so the chance of somehow ending up with the dummy ops and 
crashing seems sufficiently unlikely.

However, I'm a little wary of the static checker brigade noticing that 
and trying to "fix" it by reinstating these tests, so perhaps it's worth 
just adding unmaps to the dummy ops (with a WARN() in them) as well for 
the sake of cleanliness and avoidance of any doubt.

Thanks,
Robin.

> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxx>
> ---
>   kernel/dma/mapping.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index 81de84318ccc..6832fd6f0796 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -177,7 +177,7 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size,
>   	if (dma_map_direct(dev, ops) ||
>   	    arch_dma_unmap_page_direct(dev, addr + size))
>   		dma_direct_unmap_page(dev, addr, size, dir, attrs);
> -	else if (ops->unmap_page)
> +	else
>   		ops->unmap_page(dev, addr, size, dir, attrs);
>   	debug_dma_unmap_page(dev, addr, size, dir);
>   }
> @@ -291,7 +291,7 @@ void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
>   	if (dma_map_direct(dev, ops) ||
>   	    arch_dma_unmap_sg_direct(dev, sg, nents))
>   		dma_direct_unmap_sg(dev, sg, nents, dir, attrs);
> -	else if (ops->unmap_sg)
> +	else
>   		ops->unmap_sg(dev, sg, nents, dir, attrs);
>   }
>   EXPORT_SYMBOL(dma_unmap_sg_attrs);