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

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

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.

> +
>  	mbus = ubus->mon_bus;
>  	if (mbus != NULL)
>  		mon_bus_complete(mbus, urb, status);
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 1b2545b4363b..d31baee4ffa6 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1368,6 +1368,7 @@ extern int usb_disabled(void);
>  #define URB_ISO_ASAP		0x0002	/* iso-only; use the first unexpired
>  					 * slot in the schedule */
>  #define URB_NO_TRANSFER_DMA_MAP	0x0004	/* urb->transfer_dma valid on submit */
> +#define URB_USBMON_NEED_SYNC	0x0008	/* usb monitor need do dma sync for cpu read */
>  #define URB_ZERO_PACKET		0x0040	/* Finish bulk OUT with short packet */
>  #define URB_NO_INTERRUPT	0x0080	/* HINT: no non-error interrupt
>  					 * needed */

-- 
Regards,

Laurent Pinchart