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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <z3lechp24hlgm7v55cyfpexnewgqibxf342txas7akkvkd3k7m@se4ww4vle64b>
Date: Mon, 30 Jun 2025 16:45:44 +0800
From: Xu Yang <xu.yang_2@....com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Alan Stern <stern@...land.harvard.edu>, ezequiel@...guardiasur.com.ar, 
	mchehab@...nel.org, hdegoede@...hat.com, gregkh@...uxfoundation.org, 
	mingo@...nel.org, tglx@...utronix.de, andriy.shevchenko@...ux.intel.com, 
	viro@...iv.linux.org.uk, thomas.weissschuh@...utronix.de, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org, imx@...ts.linux.dev, jun.li@....com, 
	Ricardo Ribalda <ribalda@...omium.org>
Subject: Re: [PATCH v2 1/3] usb: core: add dma-noncoherent buffer alloc and
 free API

Hi Laurent,

On Mon, Jun 30, 2025 at 02:39:24AM +0300, 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@....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@...omium.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.

Your concern sounds resonable. But I see some drivers also call
invalidate_kernel_vmap_range() before dma_sync_sgtable_for_cpu(). I'm not
sure what's the correct order :(

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/sound/core/memalloc.c?h=linux-6.15.y#n600
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/media/common/videobuf2/videobuf2-dma-contig.c?h=linux-6.15.y#n157

Thanks,
Xu Yang

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