From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mathias Nyman Subject: Re: usb HC busted? Date: Fri, 20 Jul 2018 14:46:20 +0300 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Alan Stern Cc: Mathias Nyman , Greg KH , linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, lukaszx.szulc-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, Andy Shevchenko , Andy Shevchenko , Christoph Hellwig , Sudip Mukherjee List-Id: iommu@lists.linux-foundation.org On 19.07.2018 17:57, Alan Stern wrote: > On Thu, 19 Jul 2018, Mathias Nyman wrote: > >> xhci driver will set up all the endpoints for the new altsetting already in >> usb_hcd_alloc_bandwidth(). >> >> New endpoints will be ready and rings running after this. I don't know the exact >> history behind this, but I assume it is because xhci does all of the steps to >> drop/add, disable/enable endpoints and check bandwidth in a single configure >> endpoint command, that will return errors if there is not enough bandwidth. > > That's right; Sarah and I spent some time going over this while she was > working on it. But it looks like the approach isn't adequate. > >> This command is issued in hcd->driver->check_bandwidth() >> This means that xhci doesn't really do much in hcd->driver->endpoint_disable or >> hcd->driver->endpoint_enable >> >> It also means that xhci driver assumes rings are empty when >> hcd->driver->check_bandwidth is called. It will bluntly free dropped rings. >> If there are URBs left on a endpoint ring that was dropped+added >> (freed+reallocated) then those URBs will contain pointers to freed ring, >> causing issues when usb_hcd_flush_endpoint() cancels those URBs. >> >> usb_set_interface() >> usb_hcd_alloc_bandwidth() >> hcd->driver->drop_endpoint() >> hcd->driver->add_endpoint() // allocates new rings >> hcd->driver->check_bandwidth() // issues configure endpoint command, free rings. >> usb_disable_interface(iface, true) >> usb_disable_endpoint() >> usb_hcd_flush_endpoint() // will access freed ring if URBs found!! >> usb_hcd_disable_endpoint() >> hcd->driver->endpoint_disable() // xhci does nothing >> usb_enable_interface(iface, true) >> usb_enable_endpoint(ep_addrss, true) // not really doing much on xhci side. >> >> As first aid I could try to implement checks that make sure the flushed URBs >> trb pointers really are on the current endpoint ring, and also add some warning >> if we are we are dropping endpoints with URBs still queued. >> >> But we need to fix this properly as well. >> xhci needs to be more in sync with usb core in usb_set_interface(), currently xhci >> has the altssetting up and running when usb core hasn't event started flushing endpoints. > > Absolutely. The core tries to be compatible with host controller > drivers that either allocate bandwidth as it is requested or else > allocate bandwidth all at once when an altsetting is installed. > > xhci-hcd falls into the second category. However, this approach > requires the bandwidth verification for the new altsetting to be > performed before the old altsetting has been disabled, and the xHCI > hardware can't do this. > > We may need to change the core so that the old endpoints are disabled > before the bandwidth check is done, instead of after. Of course, this > leads to an awkward situation if the check fails -- we'd probably have > to go back and re-install the old altsetting. That would help xhci a lot. If we want to avoid the awkward altsetting re-install after bandwidth failure then adding a extra endpoint flush before checking the bandwidth would already help a lot. The endpoint disabling can then be remain after bandwidth checking. Does that work for other host controllers? -Mathias