On Fri, Feb 21, 2014 at 11:45 AM, Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx> wrote: > On Fri, Feb 21, 2014 at 01:57:25PM -0500, Alan Stern wrote: >> On Fri, 21 Feb 2014, Mathias Nyman wrote: >> > This is how I gather it works: >> > >> > Round 1: add usb2 hcd >> > >> > usb_add_hcd() // primary usb2 hcd >> > hcd->self.root_hub = usb_alloc_dev() >> > hcd->driver->reset(hcd) -> xhci_gen_setup() >> > if(primary_hcd) // true >> > pm_runtime_get_noresume(hcd->self.controller) // +1 usage >> >> At this point, we know the controller's usage counter is already > 0. >> Therefore it doesn't matter whether the _sync or _noresume version is >> used. >> >> But is this really what you want to do? That is, do you want to >> prevent the controller from being suspended, or do you want to prevent >> the root hub from being suspended? > > Either one would work. Mathias is just trying to prevent xhci_suspend > from being called before the xHCI driver finishes registering both > roothubs. It could be better to prevent the root hub from suspending, > in case there's other issues with bus suspend we haven't found yet. > >> > register_root_hub(hcd) -> usb_new_device(usb_dev) >> > pm_runtime_set_active(&udev->dev); >> > pm_runtime_get_noresume(&udev->dev); >> > pm_runtime_use_autosuspend(&udev->dev); >> > pm_runtime_enable(&udev->dev); >> > ... >> > pm_runtime_put_sync_autosuspend(&udev->dev); this would trigger >> > xhci_suspend if all usages were 0. (for both roothub and controller) >> > which oopses as usb3 stuff is not yet initialized >> >> Why would this trigger xhci_suspend? Isn't we still running in the >> context of the probe routine? The controller won't be suspended until >> the probe call returns. > > Maybe it helps to look at the context of the bug report Mathias is > trying to fix? > > http://marc.info/?l=linux-usb&m=138914518219334&w=2 > http://marc.info/?l=linux-kernel&m=138919609832200&w=2 > > It's pretty clear from the oops that xhci_suspend gets called before the > second roothub is registered. From the group it came from, I suspect > they have lots of random patches applied on top of their 3.10 kernel, > but let's assume that's not the issue and explore whether xhci_suspend > can be called before probe completes. > > The xHCI driver registers its own PCI probe function, and then it calls > usb_hcd_pci_probe(), and then registers the second roothub. So if the > USB core is somehow preventing PCI suspend in usb_hcd_pci_probe(), it > won't help for our special init flow. If the PCI core is supposed to > prevent PCI suspend until probe completes, then yes, xhci_suspend > shouldn't ever be called until both buses are registered. > > What about the xHCI platform case? xhci_plat_suspend is registered as a > system sleep PM operation, and it calls xhci_suspend: > > static int xhci_plat_suspend(struct device *dev) > { > struct usb_hcd *hcd = dev_get_drvdata(dev); > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > > return xhci_suspend(xhci); > } > > static int xhci_plat_resume(struct device *dev) > { > struct usb_hcd *hcd = dev_get_drvdata(dev); > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > > return xhci_resume(xhci, 0); > } > > static const struct dev_pm_ops xhci_plat_pm_ops = { > SET_SYSTEM_SLEEP_PM_OPS(xhci_plat_suspend, xhci_plat_resume) > }; > > So could we have a system sleep event in the middle of platform probe? No, not system sleep. dpm_suspend() is performed under the device_lock and so is ->probe(). The driver core does not hold the lock for runtime pm operations. -- 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
