From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: usb HC busted? From: Alan Stern Message-Id: Date: Thu, 19 Jul 2018 10:57:12 -0400 (EDT) To: Mathias Nyman Cc: Sudip Mukherjee , Greg KH , Andy Shevchenko , Andy Shevchenko , Mathias Nyman , linux-usb@vger.kernel.org, lukaszx.szulc@intel.com, Christoph Hellwig , Marek Szyprowski , iommu@lists.linux-foundation.org List-ID: T24gVGh1LCAxOSBKdWwgMjAxOCwgTWF0aGlhcyBOeW1hbiB3cm90ZToKCj4geGhjaSBkcml2ZXIg d2lsbCBzZXQgdXAgYWxsIHRoZSBlbmRwb2ludHMgZm9yIHRoZSBuZXcgYWx0c2V0dGluZyBhbHJl YWR5IGluCj4gdXNiX2hjZF9hbGxvY19iYW5kd2lkdGgoKS4KPiAKPiBOZXcgZW5kcG9pbnRzIHdp bGwgYmUgcmVhZHkgYW5kIHJpbmdzIHJ1bm5pbmcgYWZ0ZXIgdGhpcy4gSSBkb24ndCBrbm93IHRo ZSBleGFjdAo+IGhpc3RvcnkgYmVoaW5kIHRoaXMsIGJ1dCBJIGFzc3VtZSBpdCBpcyBiZWNhdXNl IHhoY2kgZG9lcyBhbGwgb2YgdGhlIHN0ZXBzIHRvCj4gZHJvcC9hZGQsIGRpc2FibGUvZW5hYmxl IGVuZHBvaW50cyBhbmQgY2hlY2sgYmFuZHdpZHRoIGluIGEgc2luZ2xlIGNvbmZpZ3VyZQo+IGVu ZHBvaW50IGNvbW1hbmQsIHRoYXQgd2lsbCByZXR1cm4gZXJyb3JzIGlmIHRoZXJlIGlzIG5vdCBl bm91Z2ggYmFuZHdpZHRoLgoKVGhhdCdzIHJpZ2h0OyBTYXJhaCBhbmQgSSBzcGVudCBzb21lIHRp bWUgZ29pbmcgb3ZlciB0aGlzIHdoaWxlIHNoZSB3YXMgCndvcmtpbmcgb24gaXQuICBCdXQgaXQg bG9va3MgbGlrZSB0aGUgYXBwcm9hY2ggaXNuJ3QgYWRlcXVhdGUuCgo+IFRoaXMgY29tbWFuZCBp cyBpc3N1ZWQgaW4gaGNkLT5kcml2ZXItPmNoZWNrX2JhbmR3aWR0aCgpCj4gVGhpcyBtZWFucyB0 aGF0IHhoY2kgZG9lc24ndCByZWFsbHkgZG8gbXVjaCBpbiBoY2QtPmRyaXZlci0+ZW5kcG9pbnRf ZGlzYWJsZSBvcgo+IGhjZC0+ZHJpdmVyLT5lbmRwb2ludF9lbmFibGUKPiAKPiBJdCBhbHNvIG1l YW5zIHRoYXQgeGhjaSBkcml2ZXIgYXNzdW1lcyByaW5ncyBhcmUgZW1wdHkgd2hlbgo+IGhjZC0+ ZHJpdmVyLT5jaGVja19iYW5kd2lkdGggaXMgY2FsbGVkLiBJdCB3aWxsIGJsdW50bHkgZnJlZSBk cm9wcGVkIHJpbmdzLgo+IElmIHRoZXJlIGFyZSBVUkJzIGxlZnQgb24gYSBlbmRwb2ludCByaW5n IHRoYXQgd2FzIGRyb3BwZWQrYWRkZWQKPiAoZnJlZWQrcmVhbGxvY2F0ZWQpIHRoZW4gdGhvc2Ug VVJCcyB3aWxsIGNvbnRhaW4gcG9pbnRlcnMgdG8gZnJlZWQgcmluZywKPiBjYXVzaW5nIGlzc3Vl cyB3aGVuIHVzYl9oY2RfZmx1c2hfZW5kcG9pbnQoKSBjYW5jZWxzIHRob3NlIFVSQnMuCj4gCj4g dXNiX3NldF9pbnRlcmZhY2UoKQo+ICAgIHVzYl9oY2RfYWxsb2NfYmFuZHdpZHRoKCkKPiAgICAg IGhjZC0+ZHJpdmVyLT5kcm9wX2VuZHBvaW50KCkKPiAgICAgIGhjZC0+ZHJpdmVyLT5hZGRfZW5k cG9pbnQoKSAvLyBhbGxvY2F0ZXMgbmV3IHJpbmdzCj4gICAgICBoY2QtPmRyaXZlci0+Y2hlY2tf YmFuZHdpZHRoKCkgLy8gaXNzdWVzIGNvbmZpZ3VyZSBlbmRwb2ludCBjb21tYW5kLCBmcmVlIHJp bmdzLgo+ICAgIHVzYl9kaXNhYmxlX2ludGVyZmFjZShpZmFjZSwgdHJ1ZSkKPiAgICAgIHVzYl9k aXNhYmxlX2VuZHBvaW50KCkKPiAgICAgICAgdXNiX2hjZF9mbHVzaF9lbmRwb2ludCgpIC8vIHdp bGwgYWNjZXNzIGZyZWVkIHJpbmcgaWYgVVJCcyBmb3VuZCEhCj4gICAgICAgIHVzYl9oY2RfZGlz YWJsZV9lbmRwb2ludCgpCj4gICAgICAgICAgaGNkLT5kcml2ZXItPmVuZHBvaW50X2Rpc2FibGUo KSAgLy8geGhjaSBkb2VzIG5vdGhpbmcKPiAgICB1c2JfZW5hYmxlX2ludGVyZmFjZShpZmFjZSwg dHJ1ZSkKPiAgICAgIHVzYl9lbmFibGVfZW5kcG9pbnQoZXBfYWRkcnNzLCB0cnVlKSAvLyBub3Qg cmVhbGx5IGRvaW5nIG11Y2ggb24geGhjaSBzaWRlLgo+IAo+IEFzIGZpcnN0IGFpZCBJIGNvdWxk IHRyeSB0byBpbXBsZW1lbnQgY2hlY2tzIHRoYXQgbWFrZSBzdXJlIHRoZSBmbHVzaGVkIFVSQnMK PiB0cmIgcG9pbnRlcnMgcmVhbGx5IGFyZSBvbiB0aGUgY3VycmVudCBlbmRwb2ludCByaW5nLCBh bmQgYWxzbyBhZGQgc29tZSB3YXJuaW5nCj4gaWYgd2UgYXJlIHdlIGFyZSBkcm9wcGluZyBlbmRw b2ludHMgd2l0aCBVUkJzIHN0aWxsIHF1ZXVlZC4KPiAKPiBCdXQgd2UgbmVlZCB0byBmaXggdGhp cyBwcm9wZXJseSBhcyB3ZWxsLgo+IHhoY2kgbmVlZHMgdG8gYmUgbW9yZSBpbiBzeW5jIHdpdGgg dXNiIGNvcmUgaW4gdXNiX3NldF9pbnRlcmZhY2UoKSwgY3VycmVudGx5IHhoY2kKPiBoYXMgdGhl IGFsdHNzZXR0aW5nIHVwIGFuZCBydW5uaW5nIHdoZW4gdXNiIGNvcmUgaGFzbid0IGV2ZW50IHN0 YXJ0ZWQgZmx1c2hpbmcgZW5kcG9pbnRzLgoKQWJzb2x1dGVseS4gIFRoZSBjb3JlIHRyaWVzIHRv IGJlIGNvbXBhdGlibGUgd2l0aCBob3N0IGNvbnRyb2xsZXIKZHJpdmVycyB0aGF0IGVpdGhlciBh bGxvY2F0ZSBiYW5kd2lkdGggYXMgaXQgaXMgcmVxdWVzdGVkIG9yIGVsc2UKYWxsb2NhdGUgYmFu ZHdpZHRoIGFsbCBhdCBvbmNlIHdoZW4gYW4gYWx0c2V0dGluZyBpcyBpbnN0YWxsZWQuICAKCnho Y2ktaGNkIGZhbGxzIGludG8gdGhlIHNlY29uZCBjYXRlZ29yeS4gIEhvd2V2ZXIsIHRoaXMgYXBw cm9hY2gKcmVxdWlyZXMgdGhlIGJhbmR3aWR0aCB2ZXJpZmljYXRpb24gZm9yIHRoZSBuZXcgYWx0 c2V0dGluZyB0byBiZQpwZXJmb3JtZWQgYmVmb3JlIHRoZSBvbGQgYWx0c2V0dGluZyBoYXMgYmVl biBkaXNhYmxlZCwgYW5kIHRoZSB4SENJCmhhcmR3YXJlIGNhbid0IGRvIHRoaXMuCgpXZSBtYXkg bmVlZCB0byBjaGFuZ2UgdGhlIGNvcmUgc28gdGhhdCB0aGUgb2xkIGVuZHBvaW50cyBhcmUgZGlz YWJsZWQgCmJlZm9yZSB0aGUgYmFuZHdpZHRoIGNoZWNrIGlzIGRvbmUsIGluc3RlYWQgb2YgYWZ0 ZXIuICBPZiBjb3Vyc2UsIHRoaXMgCmxlYWRzIHRvIGFuIGF3a3dhcmQgc2l0dWF0aW9uIGlmIHRo ZSBjaGVjayBmYWlscyAtLSB3ZSdkIHByb2JhYmx5IGhhdmUgCnRvIGdvIGJhY2sgYW5kIHJlLWlu c3RhbGwgdGhlIG9sZCBhbHRzZXR0aW5nLgoKQWxhbiBTdGVybgotLS0KVG8gdW5zdWJzY3JpYmUg ZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LXVzYiIgaW4K dGhlIGJvZHkgb2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2VyLmtlcm5lbC5vcmcKTW9yZSBt YWpvcmRvbW8gaW5mbyBhdCAgaHR0cDovL3ZnZXIua2VybmVsLm9yZy9tYWpvcmRvbW8taW5mby5o dG1sCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alan Stern Subject: Re: usb HC busted? Date: Thu, 19 Jul 2018 10:57:12 -0400 (EDT) Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: 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: Mathias Nyman 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 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. Alan Stern