| Message ID | 20240325-guenter-mini-v3-0-c4bc61d84e03@chromium.org |
|---|---|
| Headers | show |
| Series | uvcvideo: Attempt N to land UVC race conditions fixes | expand |
On 3/25/24 09:31, Ricardo Ribalda wrote: > If there is no in_urb there is no need to do a clean stop. > Nit: int_urb Guenter > Also we avoid calling usb_kill_urb(NULL). It is properly handled by the > usb framework, but it is not polite. > > Now that we are at it, fix the code style in uvc_status_start() for > consistency. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_status.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c > index ff66482346dde..f644ac7e67427 100644 > --- a/drivers/media/usb/uvc/uvc_status.c > +++ b/drivers/media/usb/uvc/uvc_status.c > @@ -308,7 +308,7 @@ static int _uvc_status_start(struct uvc_device *dev, gfp_t flags) > { > lockdep_assert_held(&dev->status_lock); > > - if (dev->int_urb == NULL) > + if (!dev->int_urb) > return 0; > > return usb_submit_urb(dev->int_urb, flags); > @@ -320,6 +320,9 @@ static void _uvc_status_stop(struct uvc_device *dev) > > lockdep_assert_held(&dev->status_lock); > > + if (!dev->int_urb) > + return; > + > /* > * Prevent the asynchronous control handler from requeing the URB. The > * barrier is needed so the flush_status change is visible to other >
Back in 2020 Guenter published a set of patches to fix some race conditions on UVC: https://lore.kernel.org/all/20200917022547.198090-5-linux@roeck-us.net/ That kind of race conditions are not only seen on UVC, but are a common sin on almost all the kernel, so this is what it was decided back then that we should try to fix them at higher levels. After that. A lot of video_is_registered() were added to the core: ``` ribalda@alco:~/work/linux$ git grep is_registered drivers/media/v4l2-core/ drivers/media/v4l2-core/v4l2-compat-ioctl32.c: if (!video_is_registered(vdev)) drivers/media/v4l2-core/v4l2-dev.c: if (video_is_registered(vdev)) drivers/media/v4l2-core/v4l2-dev.c: if (video_is_registered(vdev)) drivers/media/v4l2-core/v4l2-dev.c: if (video_is_registered(vdev)) { drivers/media/v4l2-core/v4l2-dev.c: if (video_is_registered(vdev)) drivers/media/v4l2-core/v4l2-dev.c: if (!video_is_registered(vdev)) drivers/media/v4l2-core/v4l2-dev.c: if (video_is_registered(vdev)) drivers/media/v4l2-core/v4l2-dev.c: if (vdev == NULL || !video_is_registered(vdev)) { drivers/media/v4l2-core/v4l2-dev.c: if (video_is_registered(vdev)) drivers/media/v4l2-core/v4l2-dev.c: if (!vdev || !video_is_registered(vdev)) drivers/media/v4l2-core/v4l2-ioctl.c: if (!video_is_registered(vfd)) { drivers/media/v4l2-core/v4l2-subdev.c: if (video_is_registered(vdev)) { ``` And recently Sakari is trying to land: https://lore.kernel.org/linux-media/20230201214535.347075-1-sakari.ailus@linux.intel.com/ Which will make obsolete a lot off (all?) of the video_is_registered() checks on Guenter's patches. Besides those checks, there were some other valid races fixed on his patches. This patchset tries to fix the races still present in our code. Thanks! Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- Changes in v3: Thanks Hans! - Stop streaming during uvc_unregister() - Refactor the uvc_status code - Link to v2: https://lore.kernel.org/r/20230309-guenter-mini-v2-0-e6410d590d43@chromium.org Changes in v2: - Actually send the series to the ML an not only to individuals. - Link to v1: https://lore.kernel.org/r/20230309-guenter-mini-v1-0-627d10cf6e96@chromium.org --- Ricardo Ribalda (3): media: uvcvideo: stop stream during unregister media: uvcvideo: Refactor the status irq API media: uvcvideo: Exit early if there is not int_urb drivers/media/usb/uvc/uvc_driver.c | 24 ++++++++------- drivers/media/usb/uvc/uvc_status.c | 62 +++++++++++++++++++++++++++++++++++--- drivers/media/usb/uvc/uvc_v4l2.c | 22 ++++---------- drivers/media/usb/uvc/uvcvideo.h | 10 +++--- 4 files changed, 83 insertions(+), 35 deletions(-) --- base-commit: b14257abe7057def6127f6fb2f14f9adc8acabdb change-id: 20230309-guenter-mini-89861b084ef1 Best regards,