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
