On Sun, Nov 17, 2024 at 08:44:16PM +0800, Markus Rechberger wrote: > Basically the issue comes from hub_port_connect. > > drivers/usb/core/hub.c > > hub_port_init returns -71 -EPROTO and jumps to loop > https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L5450 > > I'd question if usb_ep0_reinit is really required in loop which is > running following functions: You mean that usb_ep0_reinit() runs the following, not that the loop does. > usb_disable_endpoint(udev, 0 + USB_DIR_IN, true); > usb_disable_endpoint(udev, 0 + USB_DIR_OUT, true); > usb_enable_endpoint(udev, &udev->ep0, true); > > this is something only experience over the past decades can tell? It _is_ necessary, because the maxpacket size of ep0 may change from one loop iteration to the next. Therefore the endpoint must be disabled and re-enabled each time the loop repeats. [Now that I go back through the git log, it appears the only reason for exporting usb_ep0_reinit was so that the WUSB driver could call it -- see commit fc721f5194dc ("wusb: make ep0_reinit available for modules"). Since the kernel doesn't support WUSB any more, we should be able to stop exporting that function.] > usb_enable_endpoint will trigger xhci_endpoint_reset which doesn't do > much, but crashes the entire system with the upstream kernel when it > triggers xhci_check_bw_table). > > I removed usb_ep0_reinit here and devices are still workable under > various conditions (again I shorted and pulled D+/D- to ground for > testing). > The NULL PTR check in xhci_check_bw_table would be a second line of > defense but as indicated in the first mail it shouldn't even get there. > > > > As a second issue I found in usb_reset_and_verify device > https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L6131 > > ret = hub_port_init(parent_hub, udev, port1, i, &descriptor); > if (ret >= 0 || ret == -ENOTCONN || ret == -ENODEV) { > break; > } > > hub_port_init can also return -71 / -EPROTO, the cases should be very > rare when usb_reset_and_verify_device is triggered and that happens. If that happens, the loop which this code sits inside will simply perform another iteration. That's what it's supposed to do, not an issue at all. Alan Stern
