Subject: Re: Should xhci_irq() call usb_hc_died()?
To: Felipe Balbi <felipe.balbi@linux.intel.com>,
        Bjorn Helgaas <helgaas@kernel.org>,
        Mathias Nyman <mathias.nyman@intel.com>
References: <20161210002616.GA8381@bhelgaas-glaptop.roam.corp.google.com>
 <87fultill8.fsf@linux.intel.com>
Cc: Alan Stern <stern@rowland.harvard.edu>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
From: Mathias Nyman <mathias.nyman@linux.intel.com>
Message-ID: <584E8090.5080103@linux.intel.com>
Date: Mon, 12 Dec 2016 12:48:48 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.8.0
MIME-Version: 1.0
In-Reply-To: <87fultill8.fsf@linux.intel.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1886
Lines: 61

On 12.12.2016 10:43, Felipe Balbi wrote:
>
> Hi,
>
> Bjorn Helgaas <helgaas@kernel.org> writes:
>> Hi Mathias,
>>
>> ehci_irq(), ohci_irq(), fotg210_irq(), and oxu210_hcd_irq() contain code
>> equivalent to this:
>>
>>    status = ehci_readl(...);
>>    if (status == ~(u32) 0) {
>>      ...
>>      usb_hc_died(hcd);
>>      ...
>>      return IRQ_HANDLED;
>>    }
>>
>> xhci_irq() has a similar check, but does not call usb_hc_died():
>>
>>    status = readl(...);
>>    if (status = 0xffffffff) {
>>      ...
>>      return IRQ_HANDLED;
>>    }
>>
>> Should xhci_irq() also call usb_hc_died()?  Maybe there's some reason
>> for it to be different than the others, but it wasn't obvious to this
>> casual observer :)

It probably should, I'm not aware of any reason why not, and a quick look at the
logs didn't reveal anything.

Currently we are calling usb_hcd_died() in a couple of timeout cases if we read
0xffffffff from the pci registers, So eventually usb_hc_died() will be called.

I'll take a look at this in more detail

>
> you might just have fixed several bugs in dealing with a dead HC :-)
>
> Can you provide a patch? (well, unless Mathias has a strong reason not
> to call usb_hc_died(), of course).

I don't think this is the worst case, there are a couple of other reasons such as
normal pci remove case we halt the host and reset the hardware after first HCD (USB2)
is removed, with all the secondary HCD (USB3) sand all its devices still connected,

Or then the abnormal case where HC disappears, we may time out while giving back a
killed URB, and  may end up never returning it. USB core waits with the roothub device
lock held for the URB, and we try to tear down xhci, which also requires the roothub
device lock at some point -> deadlock.

I'm am looking at these, but I need to make sure i fix it properly and not cause even
more issues.

-Mathias