Re: [PATCH 0/5] usb: gadget: uvc: fix racing between uvc_function_set_alt and streamon/off

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Paul,

Thank you for the patch series. I believe you've already noticed that the 
subject line of the cover letter should have mentioned 0/4 instead of 0/5.

On Wednesday, 18 April 2018 06:20:15 EEST Paul Elder wrote:
> Down the call stack from the ioctl handler for VIDIOC_STREAMON,
> uvc_video_alloc_requests contains a BUG_ON, which in the high level,
> triggers when VIDIOC_STREAMON ioctl is issued without VIDIOC_STREAMOFF
> being issued previously.
> 
> This can happen in a few ways, such as if the userspace uvc gadget
> application simply doesn't issue VIDIOC_STREAMOFF. Another way is if
> uvc_function_set_alt with alt 0 is called after it is called with 1 but
> before VIDIOC_STREAMON is called; in this case, UVC_EVENT_STREAMON will

I assume you meant UVC_EVENT_STREAMOFF here.

> not be queued to userspace, and therefore userspace will never call
> VIDIOC_STREAMOFF.
> 
> To fix this, add two more uvc states: starting and stopping. The
> starting state starts when uvc_function_set_alt 1 is called, and ends
> in uvc_v4l2_streamon, when the state is changed to streaming.

Nitpicking, as we're talking about states, I would say "is entered" instead of 
"starts" and "is exited" instead of "ends".

> The stopping state starts when uvc_function_set_alt 0 is called, and ends
> in uvc_v4l2_streamoff, when the state is changed to connected.
> 
> Along with this fix, the completion of the usb status phase needs to be
> delayed until uvc_v4l2_streamon/off. This is already the case for
> uvc_function_set_alt 1, so add this to when alt is 0. However, the
> delayed_status is only incremented when this function returns, so if
> uvc_v4l2_streamon/off is called in between returning and incrementing,
> then uvc_function_setup_continue will WARN that delayed_status is zero.
> 
> To fix this, add and use usb_composite_setup_delay,
> which increments the delayed_status. Then, uvc_function_set_alt
> returns 0 instead of USB_GADGET_DELAYED_STATUS.
> 
> Finally, there is another way to trigger the aforementioned BUG: start
> streaming and (physically) disconnect usb. To fix this, call
> uvcg_video_enable 0 in uvc_function_disable.

This is a clear and detailed cover letter, good job ! I'll review the 
individual patches now.

> Paul Elder (4):
>   usb: gadget: uvc: synchronize streamon/off with uvc_function_set_alt
>   usb: gadget: composite: add function to increment delayed_status
>   usb: gadget: uvc: synchronize usb status phase delay for
>     uvc_function_set_alt
>   usb: gadget: uvc: disable stream when disconnected
> 
>  drivers/usb/gadget/composite.c         |  6 ++++++
>  drivers/usb/gadget/function/f_uvc.c    | 24 +++++++++++++++++++++---
>  drivers/usb/gadget/function/uvc.h      |  2 ++
>  drivers/usb/gadget/function/uvc_v4l2.c | 21 +++++++++++++++++++--
>  include/linux/usb/composite.h          |  2 ++
>  5 files changed, 50 insertions(+), 5 deletions(-)

-- 
Regards,

Laurent Pinchart



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux