Re: USB hot-plug not working (ASUS TP301UA-C4028T)

[Lukas Wunner <lukas@wunner.de>]

On Thu, Oct 06, 2016 at 10:42:14AM -0400, Alan Stern wrote:
> On Wed, 5 Oct 2016, Lukas Wunner wrote:
> > On Wed, Oct 05, 2016 at 01:54:01PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Oct 05, 2016 at 10:45:22AM -0400, Alan Stern wrote:
> > > > In short, Pierre's USB host controller doesn't send wakeup signals from
> > > > runtime suspend, because the firmware limits the runtime-suspend state
> > > > to D0 and the controller can't issue PME# from the D0 state.  In this
> > > > situation we would prefer to avoid suspending the controller at all,
> > > > rather than have it go into runtime suspend and then stop working.
> > 
> > As Alan has correctly pointed out below, there are PCI devices which
> > do not support PME but should still be runtime suspended, e.g. because
> > they have some non-standard mechanism to sideband signal wakeup or
> > because they can detect when they need to resume even if they're in
> > a low-power state.
> > 
> > AFAIUI, this device should not be runtime suspended at all because it
> > doesn't generate a PME interrupt and thus stays suspended forever.
> 
> No, as Oliver said, the device can generate a PME# signal.  It just 
> can't do so from D0, and the firmware doesn't allow it to go into a 
> deeper power-savings state.

Okay.

> 
> > The PCI core doesn't allow runtime PM by default.  Rather it calls
> > pm_runtime_forbid() when the device is added (see pci_pm_init(), called
> > indirectly from pci_device_add()).  PCI drivers need to explicitly call
> > pm_runtime_allow(), typically from their ->probe hook.
> 
> No, pm_runtime_allow() is generally called by userspace, via writing 
> to the .../power/control file in sysfs.  Most drivers do not use it; it 
> is a policy mechanism.  And drivers can't use it to _enforce_ anything, 
> since the user can always override the setting.

Okay, I stand corrected.

> 
> > If this xHC cannot signal wakeup, it shouldn't allow runtime PM in the
> > first place.  Simple as that.
> > 
> I still think this belongs in the PCI core -- except for the difficulty
> of determining whether a device can use a non-PME method for wakeup
> signalling.  If that issue has a good solution then the PCI core could 
> call pm_runtime_get_noresume() for devices that are capable of 
> generating wakeup signals but not in any D-state that the firmware will 
> allow for runtime suspend.

The PCI core already calls pm_runtime_get_sync() before invoking the
->probe hook of a driver (see local_pci_probe()).  Drivers need to
explicitly release a runtime ref to allow their device to suspend.
For xhci-pci, this seems to happen in usb_hcd_pci_probe():

	if (pci_dev_run_wake(dev))
		pm_runtime_put_noidle(&dev->dev);

So you could either modify the if-condition if you want to change the
behaviour for XHCI devices only, or if you want to change it in general,
add something like this to pci_dev_run_wake():

	/* PME capable in principle, but not from the intended sleep state */
	if (dev->pme_support && !pci_pme_capable(dev, pci_target_state(dev)))
		return false;

I've briefly looked over the callers of pci_dev_run_wake() and the above
seems safe but you should double-check them.

Best regards,

Lukas