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

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

[Xu Yang]

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);
+
 	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 */
-- 
2.34.1

[PATCH 2/2] media: uvcvideo: add URB_USBMON_NEED_SYNC urb flag

[Xu Yang]

Since commit "20e1dbf2bbe2 media: uvcvideo: Use dma_alloc_noncontiguous
API", the driver is allocating non-coherent buffer for urb. This will
add URB_USBMON_NEED_SYNC flag to inform usb monitor needs do dma sync
when record data.

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/media/usb/uvc/uvc_video.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index e3567aeb0007..446b3f16545d 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1946,7 +1946,8 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
 		urb->context = uvc_urb;
 		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
 				ep->desc.bEndpointAddress);
-		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
+		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP |
+				      URB_USBMON_NEED_SYNC;
 		urb->transfer_dma = uvc_urb->dma;
 		urb->interval = ep->desc.bInterval;
 		urb->transfer_buffer = uvc_urb->buffer;
-- 
2.34.1

Re: [PATCH 2/2] media: uvcvideo: add URB_USBMON_NEED_SYNC urb flag

[Hans de Goede]

Hi,

On 14-Jun-25 15:24, Xu Yang wrote:
> Since commit "20e1dbf2bbe2 media: uvcvideo: Use dma_alloc_noncontiguous
> API", the driver is allocating non-coherent buffer for urb. This will
> add URB_USBMON_NEED_SYNC flag to inform usb monitor needs do dma sync
> when record data.
> 
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans


> ---
>  drivers/media/usb/uvc/uvc_video.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index e3567aeb0007..446b3f16545d 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1946,7 +1946,8 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
>  		urb->context = uvc_urb;
>  		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
>  				ep->desc.bEndpointAddress);
> -		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
> +		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP |
> +				      URB_USBMON_NEED_SYNC;
>  		urb->transfer_dma = uvc_urb->dma;
>  		urb->interval = ep->desc.bInterval;
>  		urb->transfer_buffer = uvc_urb->buffer;

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

[Laurent Pinchart]

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

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

[Alan Stern]

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

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

[Xu Yang]

Hi Laurent,

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.

Thanks for your comments and suggestion. I'll try to handle these things
in usb core layer to remove above concerns.

Thanks,
Xu Yang

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