Re: [PATCH 1/2] usbmon: do dma sync for cpu read if the buffer is not dma coherent

[Alan Stern <stern@rowland.harvard.edu>]

On Sat, Jun 14, 2025 at 05:16:47PM +0300, Laurent Pinchart wrote:
> On Sat, Jun 14, 2025 at 09:24:45PM +0800, Xu Yang wrote:
> > The urb->transfer_dma may not be dma coherent, in this case usb monitor
> > may get old data. For example, commit "20e1dbf2bbe2 media: uvcvideo:
> > Use dma_alloc_noncontiguous API" is allocating non-coherent buffer.
> > 
> > To make usbmon result more reliable, this will add a flag
> > URB_USBMON_NEED_SYNC to indicate that usb monitor need do dma sync
> > before reading buffer data.
> > 
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  drivers/usb/mon/mon_main.c | 7 +++++++
> >  include/linux/usb.h        | 1 +
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/usb/mon/mon_main.c b/drivers/usb/mon/mon_main.c
> > index af852d53aac6..b9d5c1b46b85 100644
> > --- a/drivers/usb/mon/mon_main.c
> > +++ b/drivers/usb/mon/mon_main.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/notifier.h>
> >  #include <linux/mutex.h>
> > +#include <linux/dma-mapping.h>
> >  
> >  #include "usb_mon.h"
> >  
> > @@ -142,6 +143,12 @@ static void mon_complete(struct usb_bus *ubus, struct urb *urb, int status)
> >  {
> >  	struct mon_bus *mbus;
> >  
> > +	if (urb->transfer_flags & URB_USBMON_NEED_SYNC)
> > +		dma_sync_single_for_cpu(ubus->sysdev,
> > +					urb->transfer_dma,
> > +					urb->transfer_buffer_length,
> > +					DMA_FROM_DEVICE);
> 
> This will result in a double sync, impacting performance. Futhermore,
> the sync code in uvc_video.c reads as
> 
> 	/* Sync DMA and invalidate vmap range. */
> 	dma_sync_sgtable_for_cpu(uvc_stream_to_dmadev(uvc_urb->stream),
> 				 uvc_urb->sgt, uvc_stream_dir(stream));
> 	invalidate_kernel_vmap_range(uvc_urb->buffer,
> 				     uvc_urb->stream->urb_size);
> 
> The difference makes me think something has been overlooked here.
> Finally, uvcvideo supports output devices, so the DMA_FROM_DEVICE
> direction doesn't seem universally applicable.
> 
> It seems there are quite a few issues to solve to merge this feature. If
> the DMA sync operation can be done in a generic way in usbmon, then we
> should consider moving it to the USB core and avoid the sync in the
> driver. That may remove too much flexibility from drivers though, in
> which case it may be best to let the driver handle the sync in a way
> that guarantees it gets performed before usbmon inspects the data.
> 
> The issue doesn't seem specific to the uvcvideo driver. All drivers that
> set URB_NO_TRANSFER_DMA_MAP seem to be affected. A more generic
> mechanism to handle that would also be good.

Handling this in the core does seem like the best approach.  Waiting 
until the class driver's callback does a DMA sync won't work for usbmon, 
because the usbmon callback occurs first.

It also won't work right with OUT transfers, because usbmon will read 
data from the buffer after it has been synced by the driver.  (While 
this might not actually hurt anything, I think it's still a violation of 
the DMA API.)

The places where usbcore does normal DMA mapping for URBs are okay:
after usbmon during submission, before usbmon during giveback.  But it 
doesn't handle unusual mapping schemes, only the commonly used ones.

Alan Stern