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

Robin Murphy
2024-07-11T18:02:56Z

On 11/07/2024 11:38 am, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
>
> 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@nvidia.com>
> ---
> 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);