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: Mathias Nyman Message-Id: Date: Fri, 20 Jul 2018 14:46:20 +0300 To: Alan Stern 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: T24gMTkuMDcuMjAxOCAxNzo1NywgQWxhbiBTdGVybiB3cm90ZToKPiBPbiBUaHUsIDE5IEp1bCAy MDE4LCBNYXRoaWFzIE55bWFuIHdyb3RlOgo+IAo+PiB4aGNpIGRyaXZlciB3aWxsIHNldCB1cCBh bGwgdGhlIGVuZHBvaW50cyBmb3IgdGhlIG5ldyBhbHRzZXR0aW5nIGFscmVhZHkgaW4KPj4gdXNi X2hjZF9hbGxvY19iYW5kd2lkdGgoKS4KPj4KPj4gTmV3IGVuZHBvaW50cyB3aWxsIGJlIHJlYWR5 IGFuZCByaW5ncyBydW5uaW5nIGFmdGVyIHRoaXMuIEkgZG9uJ3Qga25vdyB0aGUgZXhhY3QKPj4g aGlzdG9yeSBiZWhpbmQgdGhpcywgYnV0IEkgYXNzdW1lIGl0IGlzIGJlY2F1c2UgeGhjaSBkb2Vz IGFsbCBvZiB0aGUgc3RlcHMgdG8KPj4gZHJvcC9hZGQsIGRpc2FibGUvZW5hYmxlIGVuZHBvaW50 cyBhbmQgY2hlY2sgYmFuZHdpZHRoIGluIGEgc2luZ2xlIGNvbmZpZ3VyZQo+PiBlbmRwb2ludCBj b21tYW5kLCB0aGF0IHdpbGwgcmV0dXJuIGVycm9ycyBpZiB0aGVyZSBpcyBub3QgZW5vdWdoIGJh bmR3aWR0aC4KPiAKPiBUaGF0J3MgcmlnaHQ7IFNhcmFoIGFuZCBJIHNwZW50IHNvbWUgdGltZSBn b2luZyBvdmVyIHRoaXMgd2hpbGUgc2hlIHdhcwo+IHdvcmtpbmcgb24gaXQuICBCdXQgaXQgbG9v a3MgbGlrZSB0aGUgYXBwcm9hY2ggaXNuJ3QgYWRlcXVhdGUuCj4gCj4+IFRoaXMgY29tbWFuZCBp cyBpc3N1ZWQgaW4gaGNkLT5kcml2ZXItPmNoZWNrX2JhbmR3aWR0aCgpCj4+IFRoaXMgbWVhbnMg dGhhdCB4aGNpIGRvZXNuJ3QgcmVhbGx5IGRvIG11Y2ggaW4gaGNkLT5kcml2ZXItPmVuZHBvaW50 X2Rpc2FibGUgb3IKPj4gaGNkLT5kcml2ZXItPmVuZHBvaW50X2VuYWJsZQo+Pgo+PiBJdCBhbHNv IG1lYW5zIHRoYXQgeGhjaSBkcml2ZXIgYXNzdW1lcyByaW5ncyBhcmUgZW1wdHkgd2hlbgo+PiBo Y2QtPmRyaXZlci0+Y2hlY2tfYmFuZHdpZHRoIGlzIGNhbGxlZC4gSXQgd2lsbCBibHVudGx5IGZy ZWUgZHJvcHBlZCByaW5ncy4KPj4gSWYgdGhlcmUgYXJlIFVSQnMgbGVmdCBvbiBhIGVuZHBvaW50 IHJpbmcgdGhhdCB3YXMgZHJvcHBlZCthZGRlZAo+PiAoZnJlZWQrcmVhbGxvY2F0ZWQpIHRoZW4g dGhvc2UgVVJCcyB3aWxsIGNvbnRhaW4gcG9pbnRlcnMgdG8gZnJlZWQgcmluZywKPj4gY2F1c2lu ZyBpc3N1ZXMgd2hlbiB1c2JfaGNkX2ZsdXNoX2VuZHBvaW50KCkgY2FuY2VscyB0aG9zZSBVUkJz Lgo+Pgo+PiB1c2Jfc2V0X2ludGVyZmFjZSgpCj4+ICAgICB1c2JfaGNkX2FsbG9jX2JhbmR3aWR0 aCgpCj4+ICAgICAgIGhjZC0+ZHJpdmVyLT5kcm9wX2VuZHBvaW50KCkKPj4gICAgICAgaGNkLT5k cml2ZXItPmFkZF9lbmRwb2ludCgpIC8vIGFsbG9jYXRlcyBuZXcgcmluZ3MKPj4gICAgICAgaGNk LT5kcml2ZXItPmNoZWNrX2JhbmR3aWR0aCgpIC8vIGlzc3VlcyBjb25maWd1cmUgZW5kcG9pbnQg Y29tbWFuZCwgZnJlZSByaW5ncy4KPj4gICAgIHVzYl9kaXNhYmxlX2ludGVyZmFjZShpZmFjZSwg dHJ1ZSkKPj4gICAgICAgdXNiX2Rpc2FibGVfZW5kcG9pbnQoKQo+PiAgICAgICAgIHVzYl9oY2Rf Zmx1c2hfZW5kcG9pbnQoKSAvLyB3aWxsIGFjY2VzcyBmcmVlZCByaW5nIGlmIFVSQnMgZm91bmQh IQo+PiAgICAgICAgIHVzYl9oY2RfZGlzYWJsZV9lbmRwb2ludCgpCj4+ICAgICAgICAgICBoY2Qt PmRyaXZlci0+ZW5kcG9pbnRfZGlzYWJsZSgpICAvLyB4aGNpIGRvZXMgbm90aGluZwo+PiAgICAg dXNiX2VuYWJsZV9pbnRlcmZhY2UoaWZhY2UsIHRydWUpCj4+ICAgICAgIHVzYl9lbmFibGVfZW5k cG9pbnQoZXBfYWRkcnNzLCB0cnVlKSAvLyBub3QgcmVhbGx5IGRvaW5nIG11Y2ggb24geGhjaSBz aWRlLgo+Pgo+PiBBcyBmaXJzdCBhaWQgSSBjb3VsZCB0cnkgdG8gaW1wbGVtZW50IGNoZWNrcyB0 aGF0IG1ha2Ugc3VyZSB0aGUgZmx1c2hlZCBVUkJzCj4+IHRyYiBwb2ludGVycyByZWFsbHkgYXJl IG9uIHRoZSBjdXJyZW50IGVuZHBvaW50IHJpbmcsIGFuZCBhbHNvIGFkZCBzb21lIHdhcm5pbmcK Pj4gaWYgd2UgYXJlIHdlIGFyZSBkcm9wcGluZyBlbmRwb2ludHMgd2l0aCBVUkJzIHN0aWxsIHF1 ZXVlZC4KPj4KPj4gQnV0IHdlIG5lZWQgdG8gZml4IHRoaXMgcHJvcGVybHkgYXMgd2VsbC4KPj4g eGhjaSBuZWVkcyB0byBiZSBtb3JlIGluIHN5bmMgd2l0aCB1c2IgY29yZSBpbiB1c2Jfc2V0X2lu dGVyZmFjZSgpLCBjdXJyZW50bHkgeGhjaQo+PiBoYXMgdGhlIGFsdHNzZXR0aW5nIHVwIGFuZCBy dW5uaW5nIHdoZW4gdXNiIGNvcmUgaGFzbid0IGV2ZW50IHN0YXJ0ZWQgZmx1c2hpbmcgZW5kcG9p bnRzLgo+IAo+IEFic29sdXRlbHkuICBUaGUgY29yZSB0cmllcyB0byBiZSBjb21wYXRpYmxlIHdp dGggaG9zdCBjb250cm9sbGVyCj4gZHJpdmVycyB0aGF0IGVpdGhlciBhbGxvY2F0ZSBiYW5kd2lk dGggYXMgaXQgaXMgcmVxdWVzdGVkIG9yIGVsc2UKPiBhbGxvY2F0ZSBiYW5kd2lkdGggYWxsIGF0 IG9uY2Ugd2hlbiBhbiBhbHRzZXR0aW5nIGlzIGluc3RhbGxlZC4KPiAKPiB4aGNpLWhjZCBmYWxs cyBpbnRvIHRoZSBzZWNvbmQgY2F0ZWdvcnkuICBIb3dldmVyLCB0aGlzIGFwcHJvYWNoCj4gcmVx dWlyZXMgdGhlIGJhbmR3aWR0aCB2ZXJpZmljYXRpb24gZm9yIHRoZSBuZXcgYWx0c2V0dGluZyB0 byBiZQo+IHBlcmZvcm1lZCBiZWZvcmUgdGhlIG9sZCBhbHRzZXR0aW5nIGhhcyBiZWVuIGRpc2Fi bGVkLCBhbmQgdGhlIHhIQ0kKPiBoYXJkd2FyZSBjYW4ndCBkbyB0aGlzLgo+IAo+IFdlIG1heSBu ZWVkIHRvIGNoYW5nZSB0aGUgY29yZSBzbyB0aGF0IHRoZSBvbGQgZW5kcG9pbnRzIGFyZSBkaXNh YmxlZAo+IGJlZm9yZSB0aGUgYmFuZHdpZHRoIGNoZWNrIGlzIGRvbmUsIGluc3RlYWQgb2YgYWZ0 ZXIuICBPZiBjb3Vyc2UsIHRoaXMKPiBsZWFkcyB0byBhbiBhd2t3YXJkIHNpdHVhdGlvbiBpZiB0 aGUgY2hlY2sgZmFpbHMgLS0gd2UnZCBwcm9iYWJseSBoYXZlCj4gdG8gZ28gYmFjayBhbmQgcmUt aW5zdGFsbCB0aGUgb2xkIGFsdHNldHRpbmcuCgpUaGF0IHdvdWxkIGhlbHAgeGhjaSBhIGxvdC4K CklmIHdlIHdhbnQgdG8gYXZvaWQgdGhlIGF3a3dhcmQgYWx0c2V0dGluZyByZS1pbnN0YWxsIGFm dGVyIGJhbmR3aWR0aCBmYWlsdXJlCnRoZW4gYWRkaW5nIGEgZXh0cmEgZW5kcG9pbnQgZmx1c2gg YmVmb3JlIGNoZWNraW5nIHRoZSBiYW5kd2lkdGggd291bGQgYWxyZWFkeSBoZWxwIGEgbG90LgoK VGhlIGVuZHBvaW50IGRpc2FibGluZyBjYW4gdGhlbiBiZSByZW1haW4gYWZ0ZXIgYmFuZHdpZHRo IGNoZWNraW5nLgpEb2VzIHRoYXQgd29yayBmb3Igb3RoZXIgaG9zdCBjb250cm9sbGVycz8KCi1N YXRoaWFzCi0tLQpUbyB1bnN1YnNjcmliZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAi dW5zdWJzY3JpYmUgbGludXgtdXNiIiBpbgp0aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jk b21vQHZnZXIua2VybmVsLm9yZwpNb3JlIG1ham9yZG9tbyBpbmZvIGF0ICBodHRwOi8vdmdlci5r ZXJuZWwub3JnL21ham9yZG9tby1pbmZvLmh0bWwK 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