On Tue, 5 Feb 2019 guido@xxxxxxxxxxxxxxxxxx wrote: > Zitat von Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>: > > > On Mon, 4 Feb 2019, Guido Kiener wrote: > > > >> From: Guido Kiener <guido.kiener@xxxxxxxxxxxxxxxxx> > >> > >> A reset e.g. calling ep_reset_338x() can happen while endpoints > >> are enabled. > > > > How can this happen? That routine is called from only two places. > > One is in net2280_disable(), so after the endpoint has already been > > disabled. The other is in usb_reinit_338x() via usb_reinit() via > > stop_activity(), which disconnects the gadget driver and thereby > > disables all the endpoints. > > > Yes, the routine is called when I stop my application. See callstack: > Call Trace: > net2280_disable: Invalid ep=00000000341c7996 or ep->desc > usb_ep_disable+0x24/0xa0 [udc_core] > disableEndpoints+0x1c/0x80 [rsusbtmc] > tmc_func_disable+0x29/0xa0 [rsusbtmc] > reset_config+0x3e/0xa0 [libcomposite] > composite_disconnect+0x36/0x60 [libcomposite] > net2280_pullup+0x9e/0xf0 [net2280] > usb_gadget_disconnect+0x35/0xc0 [udc_core] > usb_gadget_remove_driver+0x29/0xa0 [udc_core] > usb_gadget_unregister_driver+0xc1/0xf0 [udc_core] > usb_composite_unregister+0x12/0x20 [libcomposite] > dev_release+0x3f/0x80 [rsusbtmc] > __fput+0x102/0x230 > ____fput+0xe/0x10 > task_work_run+0x90/0xc0 > exit_to_usermode_loop+0xf2/0x100 > do_syscall_64+0xfb/0x120 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > When I restart my application then the endpoints remains disabled. > A workaround is to reload our (legacy) gadget driver, libcomposite > and net2280. > > >> The ep_reset_338x() sets ep->desc = NULL to mark > >> endpoint being invalid. A subsequent call of net2280_disable will > >> fail and return -EINVAL to parent function usb_ep_disable(), > >> which will fail, too, and do not set the member ep->enabled = false. > > > > So maybe a better fix (assuming there really is a problem) is to make > > usb_ep_disable() clear ep->enabled always. After all, the only > > reasonable way for usb_ep_disable() to fail is if the endpoint isn't > > enabled in the first place. > > > I had the same idea. However the root cause is the net2280 driver. > We have not seen the problem with other function drivers. > The usb_ep_disable() implementation looks reasonable to me. When the > ret = ep->ops->disable(ep); function fails for any other fictitious > reason (e.g. conflicts, order of endpoints..) then it might be better to > set ep->enabled = false when the function driver really succeeds. All right, I withdraw the objection. Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> Alan Stern
