Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes: > As a holdover from the old g_file_storage gadget, the g_mass_storage > legacy gadget driver attempts to unregister itself when its main > operating thread terminates (if it hasn't been unregistered already). > This is not strictly necessary; it was never more than an attempt to > have the gadget fail cleanly if something went wrong and the main > thread was killed. > > However, now that the UDC core manages gadget drivers independently of > UDC drivers, this scheme doesn't work any more. A simple test: > > modprobe dummy-hcd > modprobe g-mass-storage file=... > rmmod dummy-hcd > > ends up in a deadlock with the following backtrace: > > sysrq: SysRq : Show Blocked State > task PC stack pid father > file-storage D 0 1130 2 0x00000000 > Call Trace: > __schedule+0x53e/0x58c > schedule+0x6e/0x77 > schedule_preempt_disabled+0xd/0xf > __mutex_lock.isra.1+0x129/0x224 > ? _raw_spin_unlock_irqrestore+0x12/0x14 > __mutex_lock_slowpath+0x12/0x14 > mutex_lock+0x28/0x2b > usb_gadget_unregister_driver+0x29/0x9b [udc_core] > usb_composite_unregister+0x10/0x12 [libcomposite] > msg_cleanup+0x1d/0x20 [g_mass_storage] > msg_thread_exits+0xd/0xdd7 [g_mass_storage] > fsg_main_thread+0x1395/0x13d6 [usb_f_mass_storage] > ? __schedule+0x573/0x58c > kthread+0xd9/0xdb > ? do_set_interface+0x25c/0x25c [usb_f_mass_storage] > ? init_completion+0x1e/0x1e > ret_from_fork+0x19/0x24 > rmmod D 0 1155 683 0x00000000 > Call Trace: > __schedule+0x53e/0x58c > schedule+0x6e/0x77 > schedule_timeout+0x26/0xbc > ? __schedule+0x573/0x58c > do_wait_for_common+0xb3/0x128 > ? usleep_range+0x81/0x81 > ? wake_up_q+0x3f/0x3f > wait_for_common+0x2e/0x45 > wait_for_completion+0x17/0x19 > fsg_common_put+0x34/0x81 [usb_f_mass_storage] > fsg_free_inst+0x13/0x1e [usb_f_mass_storage] > usb_put_function_instance+0x1a/0x25 [libcomposite] > msg_unbind+0x2a/0x42 [g_mass_storage] > __composite_unbind+0x4a/0x6f [libcomposite] > composite_unbind+0x12/0x14 [libcomposite] > usb_gadget_remove_driver+0x4f/0x77 [udc_core] > usb_del_gadget_udc+0x52/0xcc [udc_core] > dummy_udc_remove+0x27/0x2c [dummy_hcd] > platform_drv_remove+0x1d/0x31 > device_release_driver_internal+0xe9/0x16d > device_release_driver+0x11/0x13 > bus_remove_device+0xd2/0xe2 > device_del+0x19f/0x221 > ? selinux_capable+0x22/0x27 > platform_device_del+0x21/0x63 > platform_device_unregister+0x10/0x1a > cleanup+0x20/0x817 [dummy_hcd] > SyS_delete_module+0x10c/0x197 > ? ____fput+0xd/0xf > ? task_work_run+0x55/0x62 > ? prepare_exit_to_usermode+0x65/0x75 > do_fast_syscall_32+0x86/0xc3 > entry_SYSENTER_32+0x4e/0x7c > > What happens is that removing the dummy-hcd driver causes the UDC core > to unbind the gadget driver, which it does while holding the udc_lock > mutex. The unbind routine in g_mass_storage tells the main thread to > exit and waits for it to terminate. > > But as mentioned above, when the main thread exits it tries to > unregister the mass-storage function driver. Via the composite > framework this ends up calling usb_gadget_unregister_driver(), which > tries to acquire the udc_lock mutex. The result is deadlock. > > The simplest way to fix the problem is not to be so clever: The main > thread doesn't have to unregister the function driver. The side > effects won't be so terrible; if the gadget is still attached to a USB > host when the main thread is killed, it will appear to the host as > though the gadget's firmware has crashed -- a reasonably accurate > interpretation, and an all-too-common occurrence for USB mass-storage > devices. > > In fact, the code to unregister the driver when the main thread exits > is specific to g-mass-storage; it is not used when f-mass-storage is > included as a function in a larger composite device. Therefore the > entire mechanism responsible for this (the fsg_operations structure > with its ->thread_exits method, the fsg_common_set_ops() routine, and > the msg_thread_exits() callback routine) can all be eliminated. Even > the msg_registered bitflag can be removed, because now the driver is > unregistered in only one place rather than in two places. > > Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > CC: Michal Nazarewicz <mina86@xxxxxxxxxx> > CC: <stable@xxxxxxxxxxxxxxx> Acked-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> -- balbi
Attachment:
signature.asc
Description: PGP signature
