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

[Leon Romanovsky]

On Thu, Jul 11, 2024 at 07:02:39PM +0100, Robin Murphy wrote:
> 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.

I'll add, however the plan is to get rid of dummy ops in the near future.

Thanks

> 
> 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);