linux-kernel - Re: [PATCH v4 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] [day] [month] [year] [list]

Message-ID: <aou5e5mzcnt7iy5vlungtrodbvhp3fpx7ytrrgplmdxb25h5e5@fhyiiqnrd5cf>
Date: Fri, 4 Jul 2025 16:16:56 +0800
From: Xu Yang <xu.yang_2@....com>
To: Alan Stern <stern@...land.harvard.edu>
Cc: ezequiel@...guardiasur.com.ar, mchehab@...nel.org, 
	laurent.pinchart@...asonboard.com, 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
Subject: Re: [PATCH v4 1/3] usb: core: add dma-noncoherent buffer alloc and
 free API

On Thu, Jul 03, 2025 at 10:35:23AM -0400, Alan Stern wrote:
> On Thu, Jul 03, 2025 at 06:38:09PM +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>
> > 
> > ---
> > Changes in v4:
> >  - improve if-else logic
> > 
> > Changes in v3:
> >  - put Return section at the end of description
> >  - correct some abbreviations
> >  - remove usb_dma_noncoherent_sync_for_cpu() and
> >    usb_dma_noncoherent_sync_for_device()
> >  - do DMA sync in usb_hcd_map_urb_for_dma() and
> >    usb_hcd_unmap_urb_for_dma()
> >  - call flush_kernel_vmap_range() for OUT transfers
> >    and invalidate_kernel_vmap_range() for IN transfers
> > ---
> >  drivers/usb/core/hcd.c | 33 ++++++++++++-----
> >  drivers/usb/core/usb.c | 80 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/usb.h    | 11 ++++++
> >  3 files changed, 116 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index c22de97432a0..42d9d8db0968 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> 
> > @@ -1425,8 +1431,10 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> >  	}
> >  
> >  	dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> > -	if (urb->transfer_buffer_length != 0
> > -	    && !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) {
> > +	if (!(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) {
> > +		if (!urb->transfer_buffer_length)
> > +			return ret;
> > +
> >  		if (hcd->localmem_pool) {
> >  			ret = hcd_alloc_coherent(
> >  					urb->dev->bus, mem_flags,
> > @@ -1491,7 +1499,16 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> >  		if (ret && (urb->transfer_flags & (URB_SETUP_MAP_SINGLE |
> >  				URB_SETUP_MAP_LOCAL)))
> >  			usb_hcd_unmap_urb_for_dma(hcd, urb);
> > +	} else {
> > +		if (!urb->sgt)
> > +			return ret;
> > +
> > +		if (dir == 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);
> >  	}
> 
> This could be done a little more cleanly.  It's always awkward to read
> an "else" clause for a negated test.

Agree.

> 
> Instead, change the "else" to:
> 
> 	if (urb->transfer_flags & URB_NO_TRANFER_DMA_MAP) {
> 
> and move this whole section to the top of the big "if".  Then you can 
> change the test that's already there to:
> 
> 	} else if (urb->transfer_buffer_length != 0) {

Okay. It's a better choice.

Thanks,
Xu Yang

> 
> Alan Stern