| Message ID | 20220907215818.2670097-1-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9b91a65230784a9ef644b8bdbb82a79ba4ae9456 |
| Headers | show |
| Series | [v4] usb: gadget: uvc: increase worker prio to WQ_HIGHPRI | expand |
Hi Michael, On Wed, Sep 07, 2022 at 11:58:18PM +0200, Michael Grzeschik wrote: > This patch is changing the simple workqueue in the gadget driver to be > allocated as async_wq with a higher priority. The pump worker, that is > filling the usb requests, will have a higher priority and will not be > scheduled away so often while the video stream is handled. This will > lead to fewer streaming underruns. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> ... > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > index 58e383afdd4406..1a31e6c6a5ffb8 100644 > --- a/drivers/usb/gadget/function/uvc.h > +++ b/drivers/usb/gadget/function/uvc.h > @@ -88,6 +88,7 @@ struct uvc_video { > struct usb_ep *ep; > > struct work_struct pump; > + struct workqueue_struct *async_wq; > > /* Frame parameters */ > u8 bpp; I am commenting here because this is the most recent change but after this showed up in -next as commit 9b91a6523078 ("usb: gadget: uvc: increase worker prio to WQ_HIGHPRI"), I see the following warning/error when building s390 allmodconfig: In file included from ../include/linux/string.h:253, from ../include/linux/bitmap.h:11, from ../include/linux/cpumask.h:12, from ../include/linux/smp.h:13, from ../include/linux/lockdep.h:14, from ../include/linux/rcupdate.h:29, from ../include/linux/rculist.h:11, from ../include/linux/pid.h:5, from ../include/linux/sched.h:14, from ../include/linux/ratelimit.h:6, from ../include/linux/dev_printk.h:16, from ../include/linux/device.h:15, from ../drivers/usb/gadget/function/f_uvc.c:9: In function ‘fortify_memset_chk’, inlined from ‘uvc_register_video’ at ../drivers/usb/gadget/function/f_uvc.c:424:2: ../include/linux/fortify-string.h:301:25: error: call to ‘__write_overflow_field’ declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] 301 | __write_overflow_field(p_size_field, size); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This commit did not directly cause this, it just made the issue more obvious. In commit e4ce9ed835bc ("usb: gadget: uvc: ensure the vdev is unset"), also authored by you, the size parameter appears to be wrong? It is using the size of 'struct uvc_video', instead of the size of 'struct video_device'. It appears to be pure luck that everything worked up until this point, as those two types had the same size (1400 bytes) before this change but now 'struct uvc_video' is 1408 bytes, meaning there is now an overwrite. Any reason this is not the fix? Cheers, Nathan diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index e6948cf8def3..836601227155 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -421,7 +421,7 @@ uvc_register_video(struct uvc_device *uvc) int ret; /* TODO reference counting. */ - memset(&uvc->vdev, 0, sizeof(uvc->video)); + memset(&uvc->vdev, 0, sizeof(uvc->vdev)); uvc->vdev.v4l2_dev = &uvc->v4l2_dev; uvc->vdev.v4l2_dev->dev = &cdev->gadget->dev; uvc->vdev.fops = &uvc_v4l2_fops;
Hi Nathan! On Wed, Sep 28, 2022 at 09:29:55AM -0700, Nathan Chancellor wrote: >On Wed, Sep 07, 2022 at 11:58:18PM +0200, Michael Grzeschik wrote: >> This patch is changing the simple workqueue in the gadget driver to be >> allocated as async_wq with a higher priority. The pump worker, that is >> filling the usb requests, will have a higher priority and will not be >> scheduled away so often while the video stream is handled. This will >> lead to fewer streaming underruns. >> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > >... > >> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h >> index 58e383afdd4406..1a31e6c6a5ffb8 100644 >> --- a/drivers/usb/gadget/function/uvc.h >> +++ b/drivers/usb/gadget/function/uvc.h >> @@ -88,6 +88,7 @@ struct uvc_video { >> struct usb_ep *ep; >> >> struct work_struct pump; >> + struct workqueue_struct *async_wq; >> >> /* Frame parameters */ >> u8 bpp; > >I am commenting here because this is the most recent change but after >this showed up in -next as commit 9b91a6523078 ("usb: gadget: uvc: >increase worker prio to WQ_HIGHPRI"), I see the following warning/error >when building s390 allmodconfig: > > In file included from ../include/linux/string.h:253, > from ../include/linux/bitmap.h:11, > from ../include/linux/cpumask.h:12, > from ../include/linux/smp.h:13, > from ../include/linux/lockdep.h:14, > from ../include/linux/rcupdate.h:29, > from ../include/linux/rculist.h:11, > from ../include/linux/pid.h:5, > from ../include/linux/sched.h:14, > from ../include/linux/ratelimit.h:6, > from ../include/linux/dev_printk.h:16, > from ../include/linux/device.h:15, > from ../drivers/usb/gadget/function/f_uvc.c:9: > In function ‘fortify_memset_chk’, > inlined from ‘uvc_register_video’ at ../drivers/usb/gadget/function/f_uvc.c:424:2: > ../include/linux/fortify-string.h:301:25: error: call to ‘__write_overflow_field’ declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] > 301 | __write_overflow_field(p_size_field, size); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >This commit did not directly cause this, it just made the issue more >obvious. In commit e4ce9ed835bc ("usb: gadget: uvc: ensure the vdev is >unset"), also authored by you, the size parameter appears to be wrong? >It is using the size of 'struct uvc_video', instead of the size of >'struct video_device'. It appears to be pure luck that everything worked >up until this point, as those two types had the same size (1400 bytes) >before this change but now 'struct uvc_video' is 1408 bytes, meaning >there is now an overwrite. Any reason this is not the fix? > >Cheers, >Nathan > >diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c >index e6948cf8def3..836601227155 100644 >--- a/drivers/usb/gadget/function/f_uvc.c >+++ b/drivers/usb/gadget/function/f_uvc.c >@@ -421,7 +421,7 @@ uvc_register_video(struct uvc_device *uvc) > int ret; > > /* TODO reference counting. */ >- memset(&uvc->vdev, 0, sizeof(uvc->video)); >+ memset(&uvc->vdev, 0, sizeof(uvc->vdev)); > uvc->vdev.v4l2_dev = &uvc->v4l2_dev; > uvc->vdev.v4l2_dev->dev = &cdev->gadget->dev; > uvc->vdev.fops = &uvc_v4l2_fops; > This sounds right. Do you send a proper patch? Michael
On Wed, Sep 28, 2022 at 09:45:29PM +0200, Michael Grzeschik wrote: > Hi Nathan! > > On Wed, Sep 28, 2022 at 09:29:55AM -0700, Nathan Chancellor wrote: > > On Wed, Sep 07, 2022 at 11:58:18PM +0200, Michael Grzeschik wrote: > > > This patch is changing the simple workqueue in the gadget driver to be > > > allocated as async_wq with a higher priority. The pump worker, that is > > > filling the usb requests, will have a higher priority and will not be > > > scheduled away so often while the video stream is handled. This will > > > lead to fewer streaming underruns. > > > > > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > > > ... > > > > > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > > > index 58e383afdd4406..1a31e6c6a5ffb8 100644 > > > --- a/drivers/usb/gadget/function/uvc.h > > > +++ b/drivers/usb/gadget/function/uvc.h > > > @@ -88,6 +88,7 @@ struct uvc_video { > > > struct usb_ep *ep; > > > > > > struct work_struct pump; > > > + struct workqueue_struct *async_wq; > > > > > > /* Frame parameters */ > > > u8 bpp; > > > > I am commenting here because this is the most recent change but after > > this showed up in -next as commit 9b91a6523078 ("usb: gadget: uvc: > > increase worker prio to WQ_HIGHPRI"), I see the following warning/error > > when building s390 allmodconfig: > > > > In file included from ../include/linux/string.h:253, > > from ../include/linux/bitmap.h:11, > > from ../include/linux/cpumask.h:12, > > from ../include/linux/smp.h:13, > > from ../include/linux/lockdep.h:14, > > from ../include/linux/rcupdate.h:29, > > from ../include/linux/rculist.h:11, > > from ../include/linux/pid.h:5, > > from ../include/linux/sched.h:14, > > from ../include/linux/ratelimit.h:6, > > from ../include/linux/dev_printk.h:16, > > from ../include/linux/device.h:15, > > from ../drivers/usb/gadget/function/f_uvc.c:9: > > In function ‘fortify_memset_chk’, > > inlined from ‘uvc_register_video’ at ../drivers/usb/gadget/function/f_uvc.c:424:2: > > ../include/linux/fortify-string.h:301:25: error: call to ‘__write_overflow_field’ declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] > > 301 | __write_overflow_field(p_size_field, size); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > This commit did not directly cause this, it just made the issue more > > obvious. In commit e4ce9ed835bc ("usb: gadget: uvc: ensure the vdev is > > unset"), also authored by you, the size parameter appears to be wrong? > > It is using the size of 'struct uvc_video', instead of the size of > > 'struct video_device'. It appears to be pure luck that everything worked > > up until this point, as those two types had the same size (1400 bytes) > > before this change but now 'struct uvc_video' is 1408 bytes, meaning > > there is now an overwrite. Any reason this is not the fix? > > > > Cheers, > > Nathan > > > > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c > > index e6948cf8def3..836601227155 100644 > > --- a/drivers/usb/gadget/function/f_uvc.c > > +++ b/drivers/usb/gadget/function/f_uvc.c > > @@ -421,7 +421,7 @@ uvc_register_video(struct uvc_device *uvc) > > int ret; > > > > /* TODO reference counting. */ > > - memset(&uvc->vdev, 0, sizeof(uvc->video)); > > + memset(&uvc->vdev, 0, sizeof(uvc->vdev)); > > uvc->vdev.v4l2_dev = &uvc->v4l2_dev; > > uvc->vdev.v4l2_dev->dev = &cdev->gadget->dev; > > uvc->vdev.fops = &uvc_v4l2_fops; > > > > This sounds right. Do you send a proper patch? Yup, I sent https://lore.kernel.org/20220928201921.3152163-1-nathan@kernel.org/ which should be in your mailbox now :) Cheers, Nathan
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index f4f6cf75930beb..09961f4ca981da 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -897,10 +897,14 @@ static void uvc_function_unbind(struct usb_configuration *c, { struct usb_composite_dev *cdev = c->cdev; struct uvc_device *uvc = to_uvc(f); + struct uvc_video *video = &uvc->video; long wait_ret = 1; uvcg_info(f, "%s()\n", __func__); + if (video->async_wq) + destroy_workqueue(video->async_wq); + /* * If we know we're connected via v4l2, then there should be a cleanup * of the device from userspace either via UVC_EVENT_DISCONNECT or diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 58e383afdd4406..1a31e6c6a5ffb8 100644 --- a/drivers/usb/gadget/function/uvc.h +++ b/drivers/usb/gadget/function/uvc.h @@ -88,6 +88,7 @@ struct uvc_video { struct usb_ep *ep; struct work_struct pump; + struct workqueue_struct *async_wq; /* Frame parameters */ u8 bpp; diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c index 511f106f984375..d6dbf9b763b2e1 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -170,7 +170,7 @@ uvc_v4l2_qbuf(struct file *file, void *fh, struct v4l2_buffer *b) return ret; if (uvc->state == UVC_STATE_STREAMING) - schedule_work(&video->pump); + queue_work(video->async_wq, &video->pump); return ret; } diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index c00ce0e91f5d5c..bb037fcc90e69e 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -277,7 +277,7 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) spin_unlock_irqrestore(&video->req_lock, flags); if (uvc->state == UVC_STATE_STREAMING) - schedule_work(&video->pump); + queue_work(video->async_wq, &video->pump); } static int @@ -485,7 +485,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable) video->req_int_count = 0; - schedule_work(&video->pump); + queue_work(video->async_wq, &video->pump); return ret; } @@ -499,6 +499,11 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc) spin_lock_init(&video->req_lock); INIT_WORK(&video->pump, uvcg_video_pump); + /* Allocate a work queue for asynchronous video pump handler. */ + video->async_wq = alloc_workqueue("uvcgadget", WQ_UNBOUND | WQ_HIGHPRI, 0); + if (!video->async_wq) + return -EINVAL; + video->uvc = uvc; video->fcc = V4L2_PIX_FMT_YUYV; video->bpp = 16;
This patch is changing the simple workqueue in the gadget driver to be allocated as async_wq with a higher priority. The pump worker, that is filling the usb requests, will have a higher priority and will not be scheduled away so often while the video stream is handled. This will lead to fewer streaming underruns. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- v3 -> v4: - improved commit message to be more clear v2 -> v3: - renamed workqueue to "uvcgadget" v1 -> v2: - added destroy_workqueue in uvc_function_unbind - reworded comment above allow_workqueue drivers/usb/gadget/function/f_uvc.c | 4 ++++ drivers/usb/gadget/function/uvc.h | 1 + drivers/usb/gadget/function/uvc_v4l2.c | 2 +- drivers/usb/gadget/function/uvc_video.c | 9 +++++++-- 4 files changed, 13 insertions(+), 3 deletions(-)