| Message ID | 20220402233914.3625405-2-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: uvc: fixes and improvements | expand |
Hello! On 4/3/22 2:39 AM, Michael Grzeschik wrote: > On uvcg_queue_cancel the buf_used counter has to be reset. Since the > encode function uses the variable to decide if the encoded data has > reached the end of frame. Intermediate calls of uvcg_queue_cancel can > therefor lead to miscalculations in the encode functions, if buf_used Therefore? > was not properly reset. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> [...] MBR, Sergey
Hi Michael, Looks like we found the same issue, I submitted the same change the other week here: https://lore.kernel.org/all/20220331184024.23918-1-w36195@motorola.com/ One difference is you had the reset outside of the queue lock. I figured to keep it within the lock since we can get a cancel while the pump worker is running and this can negate the reset. Do you agree? Thanks, Dan On Tue, Apr 05, 2022 at 11:43:16AM +0300, Sergey Shtylyov wrote: > Hello! > > On 4/3/22 2:39 AM, Michael Grzeschik wrote: > > > On uvcg_queue_cancel the buf_used counter has to be reset. Since the > > encode function uses the variable to decide if the encoded data has > > reached the end of frame. Intermediate calls of uvcg_queue_cancel can > > therefor lead to miscalculations in the encode functions, if buf_used > > Therefore? > > > was not properly reset. > > > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > [...] > > MBR, Sergey
Hi Dan, On Tue, Apr 05, 2022 at 10:01:34AM -0500, Dan Vacura wrote: >Looks like we found the same issue, I submitted the same change the >other week here: >https://lore.kernel.org/all/20220331184024.23918-1-w36195@motorola.com/ > >One difference is you had the reset outside of the queue lock. I figured >to keep it within the lock since we can get a cancel while the pump >worker is running and this can negate the reset. Do you agree? Yes! Your patch is to favour and mine can be dropped from this series. Thanks, Michael > >On Tue, Apr 05, 2022 at 11:43:16AM +0300, Sergey Shtylyov wrote: >> Hello! >> >> On 4/3/22 2:39 AM, Michael Grzeschik wrote: >> >> > On uvcg_queue_cancel the buf_used counter has to be reset. Since the >> > encode function uses the variable to decide if the encoded data has >> > reached the end of frame. Intermediate calls of uvcg_queue_cancel can >> > therefor lead to miscalculations in the encode functions, if buf_used >> >> Therefore? >> >> > was not properly reset. >> > >> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> [...] >> >> MBR, Sergey >
diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c index d852ac9e47e72c..cfa0ac4adb04d5 100644 --- a/drivers/usb/gadget/function/uvc_queue.c +++ b/drivers/usb/gadget/function/uvc_queue.c @@ -256,6 +256,8 @@ void uvcg_queue_cancel(struct uvc_video_queue *queue, int disconnect) struct uvc_buffer *buf; unsigned long flags; + queue->buf_used = 0; + spin_lock_irqsave(&queue->irqlock, flags); while (!list_empty(&queue->irqqueue)) { buf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
On uvcg_queue_cancel the buf_used counter has to be reset. Since the encode function uses the variable to decide if the encoded data has reached the end of frame. Intermediate calls of uvcg_queue_cancel can therefor lead to miscalculations in the encode functions, if buf_used was not properly reset. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- drivers/usb/gadget/function/uvc_queue.c | 2 ++ 1 file changed, 2 insertions(+)