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: Thu, 19 Jul 2018 18:42:19 +0300 To: Sudip Mukherjee Cc: Alan Stern , 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: Pj4gQXMgZmlyc3QgYWlkIEkgY291bGQgdHJ5IHRvIGltcGxlbWVudCBjaGVja3MgdGhhdCBtYWtl IHN1cmUgdGhlIGZsdXNoZWQgVVJCcwo+PiB0cmIgcG9pbnRlcnMgcmVhbGx5IGFyZSBvbiB0aGUg Y3VycmVudCBlbmRwb2ludCByaW5nLCBhbmQgYWxzbyBhZGQgc29tZSB3YXJuaW5nCj4+IGlmIHdl IGFyZSB3ZSBhcmUgZHJvcHBpbmcgZW5kcG9pbnRzIHdpdGggVVJCcyBzdGlsbCBxdWV1ZWQuCj4g Cj4gWWVzLCBwbGVhc2UuIEkgdGhpbmsgeW91ciBmaXJzdC1haWQgd2lsbCBiZSBhIG11Y2ggYmV0 dGVyIG9wdGlvbiB0aGFuCj4gdGhlIGhhY2t5IHBhdGNoIEkgYW0gdXNpbmcgYXRtLgo+IAoKQXR0 YWNoZWQgYSBwYXRjaCB0aGF0IGNoZWNrcyBjYW5jZWxlZCBVUkIgdGQvdHJiIHBvaW50ZXJzLgpJ IGhhdmVuJ3QgdGVzdGVkIGl0IGF0IGFsbCAod2VsbCBjb21waWxlcyBhbmQgYm9vdHMsIGJ1dCBu ZXcgY29kZSBuZXZlciBleGVyY2lzZWQpCgpEb2VzIGl0IHdvcmsgZm9yIHlvdT8KCj4+Cj4+IEJ1 dCB3ZSBuZWVkIHRvIGZpeCB0aGlzIHByb3Blcmx5IGFzIHdlbGwuCj4+IHhoY2kgbmVlZHMgdG8g YmUgbW9yZSBpbiBzeW5jIHdpdGggdXNiIGNvcmUgaW4gdXNiX3NldF9pbnRlcmZhY2UoKSwgY3Vy cmVudGx5IHhoY2kKPj4gaGFzIHRoZSBhbHRzc2V0dGluZyB1cCBhbmQgcnVubmluZyB3aGVuIHVz YiBjb3JlIGhhc24ndCBldmVudCBzdGFydGVkIGZsdXNoaW5nIGVuZHBvaW50cy4KPiAKPiBJIGFt IGFibGUgdG8gcmVwcm9kdWNlIHRoaXMgb24gYWxtb3N0IGFsbCBjeWNsZXMsIHNvIEkgY2FuIGFs d2F5cyB0ZXN0Cj4gdGhlIGZpeCBmb3IgeW91IGFmdGVyIHlvdSBhcmUgZnVsbHkgYmFjayBmcm9t IHlvdXIgaG9saWRheS4KCk5pY2UsIHRoYW5rcwoKLU1hdGhpYXMKCkZyb20gYTdkNGFmMzEyOWE5 MTgxMWM5NWVhNjQyZjZjOTE2YjFjMWNhNmQ0NiBNb24gU2VwIDE3IDAwOjAwOjAwIDIwMDEKRnJv bTogTWF0aGlhcyBOeW1hbiA8bWF0aGlhcy5ueW1hbkBsaW51eC5pbnRlbC5jb20+CkRhdGU6IFRo dSwgMTkgSnVsIDIwMTggMTg6MDY6MTggKzAzMDAKU3ViamVjdDogW1BBVENIXSB4aGNpOiB3aGVu IGRlcXVlaW5nIGEgVVJCIG1ha2Ugc3VyZSBpdCBleGlzdHMgb24gdGhlIGN1cnJlbnQKIGVuZHBv aW50IHJpbmcuCgpJZiB0aGUgZW5kcG9pbnQgcmluZyBoYXMgYmVlbiByZWFsbG9jYXRlZCBzaW5j ZSB0aGUgVVJCIHdhcyBlbnF1ZXVlZCwKdGhlbiBVUkIgbWF5IGNvbnRhaW4gVEQgYW5kIFRSQiBw b2ludGVycyB0byBhIGFscmVhZHkgZnJlZWQgcmluZy4KSWYgdGhpcyB0aGUgY2FzZSB0aGVuIG1h bnVhbGx0IHJldHVybiB0aGUgVVJCLCBhbmQgZG9uJ3QgdHJ5IHRvIHN0b3AKdGhlIHJpbmcuIEl0 IHdvdWxkIGJlIHVzZWxlc3MuCgpUaGlzIGNhbiBoYXBwZW5lZCBpZiBlbmRwb2ludCBpcyBub3Qg Zmx1c2hlZCBiZWZvcmUgaXQgaXMgZHJvcHBlZCBhbmQKcmUtYWRkZWQsIHdoaWNoIGlzIHRoZSBj YXNlIGluIHVzYl9zZXRfaW50ZXJmYWNlKCkgYXMgeGhjaSBkb2VzCnRoaW5ncyBpbiBhbiBvZGQg b3JkZXIuCgpTaWduZWQtb2ZmLWJ5OiBNYXRoaWFzIE55bWFuIDxtYXRoaWFzLm55bWFuQGxpbnV4 LmludGVsLmNvbT4KLS0tCiBkcml2ZXJzL3VzYi9ob3N0L3hoY2kuYyB8IDQzICsrKysrKysrKysr KysrKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tLS0KIDEgZmlsZSBjaGFuZ2VkLCAzMiBpbnNl cnRpb25zKCspLCAxMSBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9kcml2ZXJzL3VzYi9ob3N0 L3hoY2kuYyBiL2RyaXZlcnMvdXNiL2hvc3QveGhjaS5jCmluZGV4IDcxMWRhMzMuLjViZWRhYjcg MTAwNjQ0Ci0tLSBhL2RyaXZlcnMvdXNiL2hvc3QveGhjaS5jCisrKyBiL2RyaXZlcnMvdXNiL2hv c3QveGhjaS5jCkBAIC0zNyw2ICszNywyMSBAQCBzdGF0aWMgdW5zaWduZWQgaW50IHF1aXJrczsK IG1vZHVsZV9wYXJhbShxdWlya3MsIHVpbnQsIFNfSVJVR08pOwogTU9EVUxFX1BBUk1fREVTQyhx dWlya3MsICJCaXQgZmxhZ3MgZm9yIHF1aXJrcyB0byBiZSBlbmFibGVkIGFzIGRlZmF1bHQiKTsK IAorc3RhdGljIGJvb2wgdGRfb25fcmluZyhzdHJ1Y3QgeGhjaV90ZCAqdGQsIHN0cnVjdCB4aGNp X3JpbmcgKnJpbmcpCit7CisJc3RydWN0IHhoY2lfc2VnbWVudCAqc2VnID0gcmluZy0+Zmlyc3Rf c2VnOworCisJaWYgKCF0ZCB8fCAhdGQtPnN0YXJ0X3NlZykKKwkJcmV0dXJuIGZhbHNlOworCWRv IHsKKwkJaWYgKHNlZyA9PSB0ZC0+c3RhcnRfc2VnKQorCQkJcmV0dXJuIHRydWU7CisJCXNlZyA9 IHNlZy0+bmV4dDsKKwl9IHdoaWxlIChzZWcgJiYgc2VnICE9IHJpbmctPmZpcnN0X3NlZyk7CisK KwlyZXR1cm4gZmFsc2U7Cit9CisKIC8qIFRPRE86IGNvcGllZCBmcm9tIGVoY2ktaGNkLmMgLSBj YW4gdGhpcyBiZSByZWZhY3RvcmVkPyAqLwogLyoKICAqIHhoY2lfaGFuZHNoYWtlIC0gc3BpbiBy ZWFkaW5nIGhjIHVudGlsIGhhbmRzaGFrZSBjb21wbGV0ZXMgb3IgZmFpbHMKQEAgLTE0NjcsMTkg KzE0ODIsMTYgQEAgc3RhdGljIGludCB4aGNpX3VyYl9kZXF1ZXVlKHN0cnVjdCB1c2JfaGNkICpo Y2QsIHN0cnVjdCB1cmIgKnVyYiwgaW50IHN0YXR1cykKIAkJZ290byBkb25lOwogCX0KIAorCS8q IGNoZWNrIHJpbmcgaXMgbm90IHJlLWFsbG9jYXRlZCBzaW5jZSBVUkIgd2FzIGVucXVldWVkICov CisJaWYgKCF0ZF9vbl9yaW5nKCZ1cmJfcHJpdi0+dGRbMF0sIGVwX3JpbmcpKSB7CisJCXhoY2lf ZXJyKHhoY2ksICJDYW5jZWxlZCBVUkIgdGQgbm90IGZvdW5kIG9uIGVuZHBvaW50IHJpbmciKTsK KwkJZ290byBlcnJfdW5saW5rX2dpdmViYWNrOworCX0KKwogCWlmICh4aGNpLT54aGNfc3RhdGUg JiBYSENJX1NUQVRFX0hBTFRFRCkgewogCQl4aGNpX2RiZ190cmFjZSh4aGNpLCB0cmFjZV94aGNp X2RiZ19jYW5jZWxfdXJiLAotCQkJCSJIQyBoYWx0ZWQsIGZyZWVpbmcgVEQgbWFudWFsbHkuIik7 Ci0JCWZvciAoaSA9IHVyYl9wcml2LT5udW1fdGRzX2RvbmU7Ci0JCSAgICAgaSA8IHVyYl9wcml2 LT5udW1fdGRzOwotCQkgICAgIGkrKykgewotCQkJdGQgPSAmdXJiX3ByaXYtPnRkW2ldOwotCQkJ aWYgKCFsaXN0X2VtcHR5KCZ0ZC0+dGRfbGlzdCkpCi0JCQkJbGlzdF9kZWxfaW5pdCgmdGQtPnRk X2xpc3QpOwotCQkJaWYgKCFsaXN0X2VtcHR5KCZ0ZC0+Y2FuY2VsbGVkX3RkX2xpc3QpKQotCQkJ CWxpc3RfZGVsX2luaXQoJnRkLT5jYW5jZWxsZWRfdGRfbGlzdCk7Ci0JCX0KLQkJZ290byBlcnJf Z2l2ZWJhY2s7CisJCQkgICAgICAgIkhDIGhhbHRlZCwgZnJlZWluZyBURCBtYW51YWxseS4iKTsK KwkJZ290byBlcnJfdW5saW5rX2dpdmViYWNrOwogCX0KIAogCWkgPSB1cmJfcHJpdi0+bnVtX3Rk c19kb25lOwpAQCAtMTUxOSw2ICsxNTMxLDE1IEBAIHN0YXRpYyBpbnQgeGhjaV91cmJfZGVxdWV1 ZShzdHJ1Y3QgdXNiX2hjZCAqaGNkLCBzdHJ1Y3QgdXJiICp1cmIsIGludCBzdGF0dXMpCiAJc3Bp bl91bmxvY2tfaXJxcmVzdG9yZSgmeGhjaS0+bG9jaywgZmxhZ3MpOwogCXJldHVybiByZXQ7CiAK K2Vycl91bmxpbmtfZ2l2ZWJhY2s6CisJZm9yIChpID0gdXJiX3ByaXYtPm51bV90ZHNfZG9uZTsg aSA8IHVyYl9wcml2LT5udW1fdGRzOyBpKyspIHsKKwkJdGQgPSAmdXJiX3ByaXYtPnRkW2ldOwor CQlpZiAoIWxpc3RfZW1wdHkoJnRkLT50ZF9saXN0KSkKKwkJCWxpc3RfZGVsX2luaXQoJnRkLT50 ZF9saXN0KTsKKwkJaWYgKCFsaXN0X2VtcHR5KCZ0ZC0+Y2FuY2VsbGVkX3RkX2xpc3QpKQorCQkJ bGlzdF9kZWxfaW5pdCgmdGQtPmNhbmNlbGxlZF90ZF9saXN0KTsKKwl9CisKIGVycl9naXZlYmFj azoKIAlpZiAodXJiX3ByaXYpCiAJCXhoY2lfdXJiX2ZyZWVfcHJpdih1cmJfcHJpdik7Ci0tIAoy LjcuNAoK From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mathias Nyman Subject: Re: usb HC busted? Date: Thu, 19 Jul 2018 18:42:19 +0300 Message-ID: References: <20180717114104.irgdb5rmg2qxclgp@debian> <20180717144022.4wabhdhysori3mvg@debian> <20180717144918.7ymzbhijxfcc3fhb@debian> <20180717151017.yk5d4glzwqcrxpqc@debian> <20180719113455.urktqxijzoyhfoou@debian> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------CCF90E165C54D8B35E62BA11" Return-path: In-Reply-To: <20180719113455.urktqxijzoyhfoou@debian> 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: Sudip Mukherjee Cc: Mathias Nyman , Greg KH , linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, Christoph Hellwig , Andy Shevchenko , Alan Stern , Andy Shevchenko , lukaszx.szulc-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org List-Id: iommu@lists.linux-foundation.org This is a multi-part message in MIME format. --------------CCF90E165C54D8B35E62BA11 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit >> 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. > > Yes, please. I think your first-aid will be a much better option than > the hacky patch I am using atm. > Attached a patch that checks canceled URB td/trb pointers. I haven't tested it at all (well compiles and boots, but new code never exercised) Does it work for you? >> >> 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. > > I am able to reproduce this on almost all cycles, so I can always test > the fix for you after you are fully back from your holiday. Nice, thanks -Mathias --------------CCF90E165C54D8B35E62BA11 Content-Type: text/x-patch; name="0001-xhci-when-dequeing-a-URB-make-sure-it-exists-on-the-.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0001-xhci-when-dequeing-a-URB-make-sure-it-exists-on-the-.pa"; filename*1="tch" >>From a7d4af3129a91811c95ea642f6c916b1c1ca6d46 Mon Sep 17 00:00:00 2001 From: Mathias Nyman Date: Thu, 19 Jul 2018 18:06:18 +0300 Subject: [PATCH] xhci: when dequeing a URB make sure it exists on the current endpoint ring. If the endpoint ring has been reallocated since the URB was enqueued, then URB may contain TD and TRB pointers to a already freed ring. If this the case then manuallt return the URB, and don't try to stop the ring. It would be useless. This can happened if endpoint is not flushed before it is dropped and re-added, which is the case in usb_set_interface() as xhci does things in an odd order. Signed-off-by: Mathias Nyman --- drivers/usb/host/xhci.c | 43 ++++++++++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 711da33..5bedab7 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -37,6 +37,21 @@ static unsigned int quirks; module_param(quirks, uint, S_IRUGO); MODULE_PARM_DESC(quirks, "Bit flags for quirks to be enabled as default"); +static bool td_on_ring(struct xhci_td *td, struct xhci_ring *ring) +{ + struct xhci_segment *seg = ring->first_seg; + + if (!td || !td->start_seg) + return false; + do { + if (seg == td->start_seg) + return true; + seg = seg->next; + } while (seg && seg != ring->first_seg); + + return false; +} + /* TODO: copied from ehci-hcd.c - can this be refactored? */ /* * xhci_handshake - spin reading hc until handshake completes or fails @@ -1467,19 +1482,16 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) goto done; } + /* check ring is not re-allocated since URB was enqueued */ + if (!td_on_ring(&urb_priv->td[0], ep_ring)) { + xhci_err(xhci, "Canceled URB td not found on endpoint ring"); + goto err_unlink_giveback; + } + if (xhci->xhc_state & XHCI_STATE_HALTED) { xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb, - "HC halted, freeing TD manually."); - for (i = urb_priv->num_tds_done; - i < urb_priv->num_tds; - i++) { - td = &urb_priv->td[i]; - if (!list_empty(&td->td_list)) - list_del_init(&td->td_list); - if (!list_empty(&td->cancelled_td_list)) - list_del_init(&td->cancelled_td_list); - } - goto err_giveback; + "HC halted, freeing TD manually."); + goto err_unlink_giveback; } i = urb_priv->num_tds_done; @@ -1519,6 +1531,15 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) spin_unlock_irqrestore(&xhci->lock, flags); return ret; +err_unlink_giveback: + for (i = urb_priv->num_tds_done; i < urb_priv->num_tds; i++) { + td = &urb_priv->td[i]; + if (!list_empty(&td->td_list)) + list_del_init(&td->td_list); + if (!list_empty(&td->cancelled_td_list)) + list_del_init(&td->cancelled_td_list); + } + err_giveback: if (urb_priv) xhci_urb_free_priv(urb_priv); -- 2.7.4 --------------CCF90E165C54D8B35E62BA11 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline --------------CCF90E165C54D8B35E62BA11--