On Tue, Feb 19, 2013 at 11:51:46AM -0500, Alan Stern wrote: > On Tue, 19 Feb 2013, Felipe Balbi wrote: > > > > > we will still see the warning since usbcore will continue to call > > > > ->drop_endpoint() for disabled eps. > > > > > > But if xhci-hcd is fixed properly, it won't print out those warnings > > > when drop_endpoint is called for eps that were disabled by a device > > > reset. > > > > why not fixing at usbcore level and we will never have such an issue > > again with any new drivers. Currently the only user for > > ->drop_endpoint() is xhci, but what if another driver decides to use it > > and makes a similar 'mistake' ? > > > > It might be just me, but it sounds better that usbcore doesn't even call > > ->drop_endpoint() for endpoints which it knows are disabled. > > Part of the problem here is that ep->enabled is owned by usbcore, not > by the HCD. The HCD should not be allowed to change it. > > Furthermore, enabling an endpoint involves changes to the core data > structures as well as to the HCD's structures and the hardware. > Resetting a device may affect the hardware and the HCD, but it doesn't > affect the core structures. The second paragraph sounds right, but the message isn't actually about ep->enabled. I think it's about how the USB core manages the usb_device's ep_in and ep_out arrays. When usb_reset_and_verify_device() is called, it goes through port initialization (including resetting the device). At that point, all endpoints (except the default control endpoint) are marked as disabled by the xHCI host, and both the host and driver drop any bandwidth resources allocated to non-default endpoints. Then usb_reset_and_verify_device() attempts to reinstate the old configuration and alt settings. usb_hcd_alloc_bandwidth() gets called with udev->actconfig, so that the configuration and all alt setting 0 interfaces are restored. That calls into the xHCI driver to drop any endpoints in the udev->ep_out[] or udev->ep_in[] arrays. I think that's what triggers the warning. The USB core still keeps the old usb_host_endpoint structure pointers after the device has been reset. The function usb_disable_interface() clears out the ep_out and ep_in arrays, but that isn't called by usb_reset_and_verify_device() until after the configuration has been re-installed in the xHCI host. It's called just before attempting to re-install the old alt settings. I'm not sure why the code is written that way. Perhaps the USB core needs those endpoints to stick around because of later error paths? Alan? > > But I have no strong feelings either way, it just feels/sounds better > > what I suggested ;-) > > I think we should wait to hear what Sarah has to say. We could just remove the warning. It was mostly put in to help me debug the new bandwidth allocation code, and just in case there was a host controller who accidentially mis-managed the endpoint context state. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
