[PATCH v3 1/1] xhci: Prevent runtime pm from autosuspending during initialization

[PATCH v3 1/1] xhci: Prevent runtime pm from autosuspending during initialization

[Mathias Nyman]

xHCI driver has its own pci probe function that will call usb_hcd_pci_probe
to register its usb-2 bus, and then continue to manually register the
usb-3 bus. usb_hcd_pci_probe does a pm_runtime_put_noidle at the end and
might thus trigger a runtime suspend before the usb-3 bus is ready.

Prevent the runtime suspend by increasing the usage count in the
beginning of xhci_pci_probe, and decrease it once the usb-3 bus is
ready.

xhci-platform driver is not using usb_hcd_pci_probe to set up
busses and should not need to have it's usage count increased during probe.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Acked-by: Dan Williams <dan.j.williams@intel.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/usb/host/xhci-pci.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 04f986d..753857f 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -190,6 +190,10 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	struct usb_hcd *hcd;
 
 	driver = (struct hc_driver *)id->driver_data;
+
+	/* Prevent runtime suspending between USB-2 and USB-3 initialization */
+	pm_runtime_get_noresume(&dev->dev);
+
 	/* Register the USB 2.0 roothub.
 	 * FIXME: USB core must know to register the USB 2.0 roothub first.
 	 * This is sort of silly, because we could just set the HCD driver flags
@@ -199,7 +203,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	retval = usb_hcd_pci_probe(dev, id);
 
 	if (retval)
-		return retval;
+		goto put_runtime_pm;
 
 	/* USB 2.0 roothub is stored in the PCI device now. */
 	hcd = dev_get_drvdata(&dev->dev);
@@ -228,12 +232,17 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (xhci->quirks & XHCI_LPM_SUPPORT)
 		hcd_to_bus(xhci->shared_hcd)->root_hub->lpm_capable = 1;
 
+	/* USB-2 and USB-3 roothubs initialized, allow runtime pm suspend */
+	pm_runtime_put_noidle(&dev->dev);
+
 	return 0;
 
 put_usb3_hcd:
 	usb_put_hcd(xhci->shared_hcd);
 dealloc_usb2_hcd:
 	usb_hcd_pci_remove(dev);
+put_runtime_pm:
+	pm_runtime_put_noidle(&dev->dev);
 	return retval;
 }
 
-- 
1.8.1.2

Re: [PATCH v3 1/1] xhci: Prevent runtime pm from autosuspending during initialization

[Mathias Nyman]

On 03/03/2014 08:37 PM, Greg KH wrote:
> On Mon, Mar 03, 2014 at 07:30:17PM +0200, Mathias Nyman wrote:
>> xHCI driver has its own pci probe function that will call usb_hcd_pci_probe
>> to register its usb-2 bus, and then continue to manually register the
>> usb-3 bus. usb_hcd_pci_probe does a pm_runtime_put_noidle at the end and
>> might thus trigger a runtime suspend before the usb-3 bus is ready.
>
> What is the result if that happens?

Crashes. Null pointer dereference in xhci_suspend() when touching 
xhci->shared_hcd before it's initialized. More info here:

http://marc.info/?l=linux-usb&m=138914518219334&w=2

>
> Is this a regression from 3.13?  Or something new for 3.14?
>

According to reporter its been around since 3.7

commit 596d789a211d134dc5f94d1e5957248c204ef850
USB: set hub's default autosuspend delay as 0

But nobody else than the reporter is able to trigger it.

> What platform(s) are affected by this?

David, the reporter (added to cc), mentioned
"This bug happened in a platform with 1 usb3 host controller + 1 usb3 
OTG controller" run by some Intel internal group

http://marc.info/?l=linux-usb&m=138915969822029&w=2

David, can you elaborate on the platform?

>> xhci-platform driver is not using usb_hcd_pci_probe to set up
>> busses and should not need to have it's usage count increased during probe.
>
> I didn't think we had any in-kernel users of the xhci-platform driver,
> has that changed and I missed a new platform being added?
>

Not that I'm aware of. I just wanted to point out that this issue is 
only a matter on pci enumerated xhci hosts.

-Mathias

Re: [PATCH v3 1/1] xhci: Prevent runtime pm from autosuspending during initialization

[Greg KH]

On Tue, Mar 04, 2014 at 01:50:55PM +0200, Mathias Nyman wrote:
> On 03/03/2014 08:37 PM, Greg KH wrote:
> >On Mon, Mar 03, 2014 at 07:30:17PM +0200, Mathias Nyman wrote:
> >>xHCI driver has its own pci probe function that will call usb_hcd_pci_probe
> >>to register its usb-2 bus, and then continue to manually register the
> >>usb-3 bus. usb_hcd_pci_probe does a pm_runtime_put_noidle at the end and
> >>might thus trigger a runtime suspend before the usb-3 bus is ready.
> >
> >What is the result if that happens?
> 
> Crashes. Null pointer dereference in xhci_suspend() when touching
> xhci->shared_hcd before it's initialized. More info here:
> 
> http://marc.info/?l=linux-usb&m=138914518219334&w=2
> 
> >
> >Is this a regression from 3.13?  Or something new for 3.14?
> >
> 
> According to reporter its been around since 3.7
> 
> commit 596d789a211d134dc5f94d1e5957248c204ef850
> USB: set hub's default autosuspend delay as 0
> 
> But nobody else than the reporter is able to trigger it.

Then it can wait for 3.15-rc1, and then go back to the stable trees at
that time, right?  I'd prefer that as it's not a regression and not
common.

> >What platform(s) are affected by this?
> 
> David, the reporter (added to cc), mentioned
> "This bug happened in a platform with 1 usb3 host controller + 1 usb3 OTG
> controller" run by some Intel internal group
> 
> http://marc.info/?l=linux-usb&m=138915969822029&w=2

That sounds like a "not shipping platform" to me :)

Please resend this with patches for 3.15-rc1.

thanks,

greg k-h

Re: [PATCH v3 1/1] xhci: Prevent runtime pm from autosuspending during initialization

[David Cohen]

Hi Greg,

On Tue, Mar 04, 2014 at 09:04:58AM -0800, Greg KH wrote:
> On Tue, Mar 04, 2014 at 01:50:55PM +0200, Mathias Nyman wrote:
> > On 03/03/2014 08:37 PM, Greg KH wrote:
> > >On Mon, Mar 03, 2014 at 07:30:17PM +0200, Mathias Nyman wrote:
> > >>xHCI driver has its own pci probe function that will call usb_hcd_pci_probe
> > >>to register its usb-2 bus, and then continue to manually register the
> > >>usb-3 bus. usb_hcd_pci_probe does a pm_runtime_put_noidle at the end and
> > >>might thus trigger a runtime suspend before the usb-3 bus is ready.
> > >
> > >What is the result if that happens?
> > 
> > Crashes. Null pointer dereference in xhci_suspend() when touching
> > xhci->shared_hcd before it's initialized. More info here:
> > 
> > http://marc.info/?l=linux-usb&m=138914518219334&w=2
> > 
> > >
> > >Is this a regression from 3.13?  Or something new for 3.14?
> > >
> > 
> > According to reporter its been around since 3.7
> > 
> > commit 596d789a211d134dc5f94d1e5957248c204ef850
> > USB: set hub's default autosuspend delay as 0
> > 
> > But nobody else than the reporter is able to trigger it.
> 
> Then it can wait for 3.15-rc1, and then go back to the stable trees at
> that time, right?  I'd prefer that as it's not a regression and not
> common.
> 
> > >What platform(s) are affected by this?
> > 
> > David, the reporter (added to cc), mentioned
> > "This bug happened in a platform with 1 usb3 host controller + 1 usb3 OTG
> > controller" run by some Intel internal group
> > 
> > http://marc.info/?l=linux-usb&m=138915969822029&w=2
> 
> That sounds like a "not shipping platform" to me :)

Not quite. You'd find this configuration on Asus T100T. I said "not
quite" because I don't think we can run Linux smoothly on it yet. But...

> 
> Please resend this with patches for 3.15-rc1.

...that sounds fine :)

Br, David

> 
> thanks,
> 
> greg k-h

Re: [PATCH v3 1/1] xhci: Prevent runtime pm from autosuspending during initialization

[Sarah Sharp]

On Tue, Mar 04, 2014 at 09:04:58AM -0800, Greg KH wrote:
> On Tue, Mar 04, 2014 at 01:50:55PM +0200, Mathias Nyman wrote:
> > On 03/03/2014 08:37 PM, Greg KH wrote:
> > >On Mon, Mar 03, 2014 at 07:30:17PM +0200, Mathias Nyman wrote:
> > >>xHCI driver has its own pci probe function that will call usb_hcd_pci_probe
> > >>to register its usb-2 bus, and then continue to manually register the
> > >>usb-3 bus. usb_hcd_pci_probe does a pm_runtime_put_noidle at the end and
> > >>might thus trigger a runtime suspend before the usb-3 bus is ready.
> > >
> > >What is the result if that happens?
> > 
> > Crashes. Null pointer dereference in xhci_suspend() when touching
> > xhci->shared_hcd before it's initialized. More info here:
> > 
> > http://marc.info/?l=linux-usb&m=138914518219334&w=2
> > 
> > >
> > >Is this a regression from 3.13?  Or something new for 3.14?
> > >
> > 
> > According to reporter its been around since 3.7
> > 
> > commit 596d789a211d134dc5f94d1e5957248c204ef850
> > USB: set hub's default autosuspend delay as 0
> > 
> > But nobody else than the reporter is able to trigger it.
> 
> Then it can wait for 3.15-rc1, and then go back to the stable trees at
> that time, right?  I'd prefer that as it's not a regression and not
> common.

Ok, I'll queue it with the other features for 3.15.

Sarah Sharp