Re: [PATCH 1/2] [media] uvcvideo: Refactor teardown of uvc on USB disconnect

[Daniel Axtens]

Hi Laurent,

>> To fix this, we need to make sure the devices are always unregistered
>> before the end of uvc_disconnect(). To this, move the unregistration
>> into the disconnect path:
>>
>>  - split uvc_status_cleanup() into two parts, one on disconnect that
>>    unregisters and one on delete that frees.
>> 
>>  - move media_device_unregister() into the disconnect path.
>
> While the patch looks reasonable to me (with one comment below though), isn't 
> this an issue with the USB core, or possibly the device core ? It's a common 
> practice to create device nodes as children of physical devices. Does the 
> device core really require all device nodes to be unregistered synchronously 
> with physical device hot-unplug ? If so, shouldn't it warn somehow when a 
> device is deleted and still has children, instead of accepting that silently 
> and later complaining due to sysfs issues ?

Probably! I might have a look at this in a bit - I was initially drawn
into this area because of a misbehaving USB3 dock + webcam combo; once I
sort out my more pressing issues there I will have a look at putting
that extra sanity checking in the device core.

>
>> [0]: https://lkml.org/lkml/2016/12/8/657
>> 
>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>> Cc: Dave Stevenson <linux-media@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
>> Cc: Greg KH <greg@xxxxxxxxx>
>> Signed-off-by: Daniel Axtens <dja@xxxxxxxxxx>
>> 
>> ---
>> 
>> Tested with cheese and yavta.
>> ---
>>  drivers/media/usb/uvc/uvc_driver.c | 8 ++++++--
>>  drivers/media/usb/uvc/uvc_status.c | 8 ++++++--
>>  drivers/media/usb/uvc/uvcvideo.h   | 1 +
>>  3 files changed, 13 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/media/usb/uvc/uvc_driver.c
>> b/drivers/media/usb/uvc/uvc_driver.c index 46d6be0bb316..2390592f78e0
>> 100644
>> --- a/drivers/media/usb/uvc/uvc_driver.c
>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>> @@ -1815,8 +1815,6 @@ static void uvc_delete(struct uvc_device *dev)
>>  	if (dev->vdev.dev)
>>  		v4l2_device_unregister(&dev->vdev);
>>  #ifdef CONFIG_MEDIA_CONTROLLER
>> -	if (media_devnode_is_registered(dev->mdev.devnode))
>> -		media_device_unregister(&dev->mdev);
>
> media_device_unregister() will now be called before v4l2_device_unregister() 
> which, unless I'm mistaken, will now result in 
> media_device_unregister_entity() being called twice for every entity, the 
> first time by media_device_unregister(), and the second time by 
> v4l2_device_unregister_subdev() through v4l2_device_unregister(). Looking at 
> media_device_unregister() I don't think that's safe.
>
> We could move to v4l2_device_unregister() call to uvc_unregister_video(), but 
> that worries me (perhaps unnecessarily though) due to the race conditions it 
> could introduce. Would you still be able to give it a try ?
>

That's a good catch. I have moved v4l2_device_unregister() into the
unregister path, and turned on a bunch of debugging, including KASan. It
looks good so far, but I will plug and unplug the webcam a few more
times before sending v2 :)

> Note that your patch isn't really at fault here, the media controller and V4L2 
> core code have been broken for a long time when it comes to entity lifetime 
> management. That might be fixed some day, but I won't hold my breath given the 
> bad track record of the previous year and a half.

This is my first real foray into lifecycle managment in Linux. I've
found it quite confusing, so it's comforting to know that it's not just me!

Regards,
Daniel

>
>>  	media_device_cleanup(&dev->mdev);
>>  #endif
>> 
>> @@ -1884,6 +1882,12 @@ static void uvc_unregister_video(struct uvc_device
>> *dev) uvc_debugfs_cleanup_stream(stream);
>>  	}
>> 
>> +	uvc_status_unregister(dev);
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> +	if (media_devnode_is_registered(dev->mdev.devnode))
>> +		media_device_unregister(&dev->mdev);
>> +#endif
>> +
>>  	/* Decrement the stream count and call uvc_delete explicitly if there
>>  	 * are no stream left.
>>  	 */
>> diff --git a/drivers/media/usb/uvc/uvc_status.c
>> b/drivers/media/usb/uvc/uvc_status.c index f552ab997956..95709b23d3b4
>> 100644
>> --- a/drivers/media/usb/uvc/uvc_status.c
>> +++ b/drivers/media/usb/uvc/uvc_status.c
>> @@ -198,12 +198,16 @@ int uvc_status_init(struct uvc_device *dev)
>>  	return 0;
>>  }
>> 
>> -void uvc_status_cleanup(struct uvc_device *dev)
>> +void uvc_status_unregister(struct uvc_device *dev)
>>  {
>>  	usb_kill_urb(dev->int_urb);
>> +	uvc_input_cleanup(dev);
>> +}
>> +
>> +void uvc_status_cleanup(struct uvc_device *dev)
>> +{
>>  	usb_free_urb(dev->int_urb);
>>  	kfree(dev->status);
>> -	uvc_input_cleanup(dev);
>>  }
>> 
>>  int uvc_status_start(struct uvc_device *dev, gfp_t flags)
>> diff --git a/drivers/media/usb/uvc/uvcvideo.h
>> b/drivers/media/usb/uvc/uvcvideo.h index 15e415e32c7f..4b4814df35cd 100644
>> --- a/drivers/media/usb/uvc/uvcvideo.h
>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>> @@ -712,6 +712,7 @@ void uvc_video_clock_update(struct uvc_streaming
>> *stream,
>> 
>>  /* Status */
>>  extern int uvc_status_init(struct uvc_device *dev);
>> +extern void uvc_status_unregister(struct uvc_device *dev);
>>  extern void uvc_status_cleanup(struct uvc_device *dev);
>>  extern int uvc_status_start(struct uvc_device *dev, gfp_t flags);
>>  extern void uvc_status_stop(struct uvc_device *dev);
>
> -- 
> Regards,
>
> Laurent Pinchart