Re: [PATCH v2 1/3] usb: core: add dma-noncoherent buffer alloc and free API

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Fri, Jun 27, 2025 at 10:23:36AM -0400, Alan Stern wrote:
> On Fri, Jun 27, 2025 at 06:19:37PM +0800, Xu Yang wrote:
> > This will add usb_alloc_noncoherent() and usb_free_noncoherent()
> > functions to support alloc and free buffer in a dma-noncoherent way.
> > 
> > To explicit manage the memory ownership for the kernel and device,
> > this will also add usb_dma_noncoherent_sync_for_cpu/device() functions
> > and call it at proper time.  The management requires the user save
> > sg_table returned by usb_alloc_noncoherent() to urb->sgt.
> > 
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  drivers/usb/core/hcd.c | 30 ++++++++++++++++
> >  drivers/usb/core/usb.c | 80 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/usb.h    |  9 +++++
> >  3 files changed, 119 insertions(+)
> > 
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index c22de97432a0..5fa00d32afb8 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -1496,6 +1496,34 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> >  }
> >  EXPORT_SYMBOL_GPL(usb_hcd_map_urb_for_dma);
> >  
> > +static void usb_dma_noncoherent_sync_for_cpu(struct usb_hcd *hcd,
> > +					     struct urb *urb)
> > +{
> > +	enum dma_data_direction dir;
> > +
> > +	if (!urb->sgt)
> > +		return;
> > +
> > +	dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> 
> Are the following operations really necessary if the direction is OUT?  
> There are no bidirectional URBs, and an OUT transfer never modifies the 
> contents of the transfer buffer so the buffer contents will be the same 
> after the URB completes as they were when the URB was submitted.

The arch part of dma_sync_sgtable_for_cpu(DMA_TO_DEVICE) is a no-op on
all architectures but microblaze, mips, parisc and powerpc (at least in
some configurations of those architectures).

The IOMMU DMA mapping backend calls into the arch-specific code, and
also handles swiotlb, which is a no-op for DMA_TO_DEVICE. There's also
some IOMMU-related arch-specific handling for sparc.

I think dma_sync_sgtable_for_cpu() should be called for the
DMA_TO_DEVICE direction, to ensure proper operation in those uncommon
but real cases where platforms need to perform some operation. It has a
non-zero cost on other platforms, as the CPU will need to go through a
few function calls to end up in no-ops and then go back up the call
stack.

invalidate_kernel_vmap_range() may not be needed. I don't recall why it
was added. The call was introduced in

commit 20e1dbf2bbe2431072571000ed31dfef09359c08
Author: Ricardo Ribalda <ribalda@chromium.org>
Date:   Sat Mar 13 00:55:20 2021 +0100

    media: uvcvideo: Use dma_alloc_noncontiguous API

Ricardo, do we need to invalidate the vmap range in the DMA_TO_DEVICE
case ?

> > +	invalidate_kernel_vmap_range(urb->transfer_buffer,
> > +				     urb->transfer_buffer_length);
> > +	dma_sync_sgtable_for_cpu(hcd->self.sysdev, urb->sgt, dir);

In the DMA_FROM_DEVICE case, shouldn't the vmap range should be
invalidated after calling dma_sync_sgtable_for_cpu() ? Otherwise I think
speculative reads coming between invalidation and dma sync could result
in data corruption.

> > +}
> 
> This entire routine should be inserted at the appropriate place in 
> usb_hcd_unmap_urb_for_dma() instead of being standalone.
> 
> > +static void usb_dma_noncoherent_sync_for_device(struct usb_hcd *hcd,
> > +						struct urb *urb)
> > +{
> > +	enum dma_data_direction dir;
> > +
> > +	if (!urb->sgt)
> > +		return;
> > +
> > +	dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> > +	flush_kernel_vmap_range(urb->transfer_buffer,
> > +				urb->transfer_buffer_length);
> > +	dma_sync_sgtable_for_device(hcd->self.sysdev, urb->sgt, dir);
> > +}
> 
> Likewise, this code belongs inside usb_hcd_map_urb_for_dma().
> 
> Also, the material that this routine replaces in the uvc and stk1160 
> drivers do not call flush_kernel_vmap_range().  Why did you add that 
> here?  Was this omission a bug in those drivers?
> 
> Alan Stern

-- 
Regards,

Laurent Pinchart