Hey all, I recently ran into a kernel panic when testing the UVC Gadget Driver. The device ramdumps with the following stack when removing the UVC config from configfs: KP: Oops - BUG: Fatal exception: comm:Thread-6 PC:__list_del_entry_valid+0xb0/0xc4 LR:__list_del_entry_valid+0xb0/0xc4 PC: __list_del_entry_valid+0xb0 <FFFFFFE685330294> LR: __list_del_entry_valid+0xb0 <FFFFFFE685330294> [<FFFFFFE685330294>] __list_del_entry_valid+0xb0 [<FFFFFFE6857E50AC>] v4l2_fh_del+0x78 [<FFFFFFE685769774>] uvc_v4l2_release+0xd0 [<FFFFFFE6857D9B10>] v4l2_release+0xcc [<FFFFFFE684EE192C>] __fput+0xf8 [<FFFFFFE684EE17CC>] ____fput+0xc [<FFFFFFE684B5C9E0>] task_work_run+0x138 This looks like a side effect of https://lore.kernel.org/lkml/20230608204517.105396-1-badhri@xxxxxxxxxx/. Effectively, UVC function tried to disconnect the gadget before cleaning up resources. However, usb_gadget_unregister_driver which is removing the function prevents the gadget from disconnecting until the function is unbound. As of the patch mentioned above, gadget_unbind_driver holds udc->connect_lock and calls both usb_gadget_disconnect_locked and udc->driver->unbind one after the other. usb_gadget_disconnect_locked calls into UVC Gadget driver as follows: 1. usb_gadget_disconnect_locked 2. configfs_composite_disconnect 3. __composite_disconnect 4. uvc_function_disable udc->driver->unbind calls into UVC Gadget driver as follows: 1. udc->driver->unbind 2. configfs_composite_unbind 3. purge_configs_funcs 4. uvc_function_unbind uvc_function_disable notifies the userspace application with UVC_EVENT_DISCONNECTED which causes the V4L2 node to be released (or unsubscribed to). Either on unsubscribe or on release, the UVC Gadget Driver calls uvc_function_disconnect before cleaning up resources. Following is the problematic stack from uvc_v4l2_disable. 1. uvc_v4l2_disable 2. uvc_function_disconnect 3. usb_function_deactivate 4. usb_gadget_deactivate usb_gadget_deactivate attempts to lock udc->connect_lock as of the patch mentioned above. This means that attempting to unregister the UVC Gadget Driver results in the V4L2 resource cleanup being stuck behind udc->connect_lock, which will only be released after uvc_function_unbind finishes. This results in either the gadget deactivating after the unbind process has finished, or in a Kernel Panic as it tries to cleanup a V4L2 node that has been purged. This leaves us with two options: 1. We "fix" the locking in core.c to restore old behavior, and let the usb_gadget_deactivate call go through without locking. However, I am not sure about the specifics of the patch were and what exact issue it was trying to fix. Badhri, would you know if relaxing the constraints on usb_gadget_deactivate is feasible? It is possible that other gadget drivers run into similar issues as UVC driver. 3. UVC Gadget Driver calls usb_function_deactivate to take down the gadget if the userspace application stops listening to the V4L2 node. However, AFAICT disable is called as a part of the gadget resetting. So, if the V4L2 node is released because of UVC_EVENT_DISCONNECT, we can skip calling usb_function_deactivate as the gadget will be reset anyway. usb_function documentation seems to agree that if 'disable' is called, the gadget will be reset/reconfigured shortly: @disable: (REQUIRED) Indicates the function should be disabled. Reasons * include host resetting or reconfiguring the gadget, and disconnection. A dirty Patch for option 2 is attached below which skips calling usb_function_deactivate if uvc_function_disable was called before. It seems to work okay in testing. Let me know if the analysis and solutions seems okay and I can upload a formal patch. Thank you! --- drivers/usb/gadget/function/f_uvc.c | 12 ++++++++++-- drivers/usb/gadget/function/uvc.h | 1 + 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index 5e919fb65833..cef92243f1f7 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -385,7 +385,7 @@ uvc_function_disable(struct usb_function *f) v4l2_event.type = UVC_EVENT_DISCONNECT; v4l2_event_queue(&uvc->vdev, &v4l2_event); - uvc->state = UVC_STATE_DISCONNECTED; + uvc->state = UVC_STATE_HOST_DISCONNECTED; usb_ep_disable(uvc->video.ep); if (uvc->enable_interrupt_ep) @@ -410,8 +410,16 @@ uvc_function_disconnect(struct uvc_device *uvc) { int ret; - if ((ret = usb_function_deactivate(&uvc->func)) < 0) + if (uvc->state == UVC_STATE_HOST_DISCONNECTED) { + /* + * Don't deactivate gadget as this is being called in + * response to the host resetting. Gadget will be deactivated + * anyway. Just update to state as acknowledgement + */ + uvc->state = UVC_STATE_DISCONNECTED; + } else if ((ret = usb_function_deactivate(&uvc->func)) < 0) { uvcg_info(&uvc->func, "UVC disconnect failed with %d\n", ret); + } } /* -------------------------------------------------------------------------- diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 100475b1363e..f1e2bc98dc61 100644 --- a/drivers/usb/gadget/function/uvc.h +++ b/drivers/usb/gadget/function/uvc.h @@ -120,6 +120,7 @@ struct uvc_video { }; enum uvc_state { + UVC_STATE_HOST_DISCONNECTED, UVC_STATE_DISCONNECTED, UVC_STATE_CONNECTED, UVC_STATE_STREAMING, --
