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

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

On Mon, Jun 30, 2025 at 08:48:23AM +0200, Ricardo Ribalda wrote:
> On Mon, 30 Jun 2025 at 01:39, Laurent Pinchart wrote:
> > 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 ?
> 
> That change came from Christoph
> https://lore.kernel.org/linux-media/20210128150955.GA30563@lst.de/
> 
> """
> 
> Given that we vmap the addresses this also needs
> flush_kernel_vmap_range / invalidate_kernel_vmap_range calls for
> VIVT architectures.
> 
> """

Thank you, I looked for such a discussion in the list archive yesterday
but somehow missed it.

Christoph, you mentioned

  Right now we don't have a proper state machine for the
  *_kernel_vmap_range, but we should probably add one once usage of this
  grows.

Has there been any progress on that front ?

> > > > +   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