On 17/06/2024 09:56, Sakari Ailus wrote: > Hi Hans, > > On Mon, Jun 17, 2024 at 09:27:43AM +0200, Hans Verkuil wrote: >> On 17/06/2024 01:58, Laurent Pinchart wrote: >>> Hi Tomasz, >>> >>> On Thu, Jun 06, 2024 at 06:57:50PM +0900, Tomasz Figa wrote: >>>> On Wed, Mar 27, 2024 at 5:24 PM Ricardo Ribalda wrote: >>>>> >>>>> uvc_unregister_video() can be called asynchronously from >>>>> uvc_disconnect(). If the device is still streaming when that happens, a >>>>> plethora of race conditions can happen. >>>>> >>>>> Make sure that the device has stopped streaming before exiting this >>>>> function. >>>>> >>>>> If the user still holds handles to the driver's file descriptors, any >>>>> ioctl will return -ENODEV from the v4l2 core. >>>>> >>>>> This change make uvc more consistent with the rest of the v4l2 drivers >>>>> using the vb2_fop_* and vb2_ioctl_* helpers. >>>>> >>>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx> >>>>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx> >>>>> --- >>>>> drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++ >>>>> 1 file changed, 11 insertions(+) >>>> >>>> First of all, thanks for the patch. I have a question about the >>>> problem being fixed here. >>>> >>>> Could you point out a specific race condition example that could >>>> happen without this change? >>>> From what I see in __video_do_ioctl((), no ioctls would be executed >>>> anymore after the video node is unregistered. >>>> Since the device is not present either, what asynchronous code paths >>>> could be still triggered? >>> >>> I believe the issue is that some ioctls can be in progress while the >>> device is unregistered. I'll let Ricardo confirm. >>> >>> I've tried to explain multiple times before that this should be handled >>> in the V4L2 core, ideally with fixes in the cdev core too, as this issue >>> affects all cdev drivers. I've pointed to related patches that have been >>> posted for the cdev core. They need to be wrapped in V4L2 functions to >>> make them easier to use for drivers. If we don't want to depend on those >>> cdev changes, we can implement the "wrappers" with fixes limited to >>> V4L2 until the cdev changes get merged (assuming someone would resurect >>> them). >> >> But there is already a V4L2 wrapper for that: vb2_video_unregister_device(). >> It safely unregisters the video device, ensuring any in-flight ioctls finish >> first, and it stops any video streaming. >> >> The only reason it can't be used in uvc for the video stream is that that >> vb2_queue doesn't set the lock field (i.e. uses the core V4L2 serialization >> mechanism). The metadata stream *does* set that field, so for that stream this >> function can be used. >> >> While it would be nice to have this fixed in the cdev core part, that will >> take very long, and we have a perfectly fine V4L2 helper for this already. > > It might not take *that* long to get there but it won't happen unless > someone does it. Dan Williams posted a patch but his immediate problem was > solved differently so there it remains > <URL:https://lore.kernel.org/all/161117153248.2853729.2452425259045172318.stgit@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/>. > > In the meantime vb_video_unregister_device() would seem to be the best > choice. Also note that even if these cdev improvements ever land, that doesn't remove the need for vb2_video_unregister_device, since that also explicitly stops any streaming that is in progress. Which is something you really want to do when the device is unbound. Regards, Hans
