On 20.04.2015 23:39, Alan Stern wrote: > On Mon, 20 Apr 2015, Joe Lawrence wrote: > >> On Mon, Apr 20, 2015 at 01:35:40PM -0400, Alan Stern wrote: >>> On Mon, 20 Apr 2015, Joe Lawrence wrote: >>> >>>> So -ESHUTDOWN = -108 (0xffffff94) provoked bad_action_ret into reporting >>>> a bogus return value and stack trace above. >>> >>> As far as I know, -Eanything is never a valid return code for an IRQ >>> handler. Shouldn't this always return either IRQ_NONE or IRQ_HANDLED? >> >> Hi Alan -- I would think so, though the stack trace in the STS_FATAL >> case might interesting to somebody? (Not sure what info in such trace >> is useful since it's an irq handler.) Even then, it should probably be >> replaced with a WARN_ONCE or similar instead of inadvertently through >> the bogus irq return value. >> >> How about the following one-liner? >> >> -- Joe >> >> -->8-- -->8-- -->8-- >> >> From f8030d1cabbab1e7e5b0a0ba67fa4cd0a944d416 Mon Sep 17 00:00:00 2001 >> From: Joe Lawrence <joe.lawrence@xxxxxxxxxxx> >> Date: Mon, 20 Apr 2015 15:40:47 -0400 >> Subject: [PATCH] xhci: gracefully handle xhci_irq dead device >> >> If the xHCI host controller has died (ie, device removed) or suffered >> other serious fatal error (STS_FATAL), then xhci_irq should handle this >> condition with IRQ_HANDLED instead of -ESHUTDOWN. >> >> Signed-off-by: Joe Lawrence <joe.lawrence@xxxxxxxxxxx> >> --- >> drivers/usb/host/xhci-ring.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c >> index f5397a517c54..0402b80d2c85 100644 >> --- a/drivers/usb/host/xhci-ring.c >> +++ b/drivers/usb/host/xhci-ring.c >> @@ -2640,7 +2640,7 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd) >> xhci_halt(xhci); >> hw_died: >> spin_unlock(&xhci->lock); >> - return -ESHUTDOWN; >> + return IRQ_HANDLED; >> } >> >> /* > > That seems good to me, and I assume it prevents your warning. Mathias > knows a lot more about xhci-hcd than I do, though. Looks good to me, at least on STS_FATAL we know the interrupt is from xhci and should return IRQ_HANDLED. Probably also when we read 0xffffffff from the xhci status reg it's due to a removed xhci. On the other hand if we just removed xhci, and share the interrupt with somebody else who is also generating an interrupts, then we would probably continue to read 0xffffffff from the status reg and should return IRQ_NONE. Anyways, not a very likely situation and anything is better than -ESHUTDOWN. I tried to figure out the reason behind the -ESHUTDOWN, but looks like it has always just been there. It was added with the interrupt handler code and wasn't mentioned in the commit message. I'll add you patch and send it forward once rc1 is out -Mathias -- 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
