diff mbox

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

Message ID Pine.LNX.4.44L0.1610111117040.1827-100000@iolanthe.rowland.org
State Not Applicable
Headers show

Commit Message

Alan Stern Oct. 11, 2016, 3:18 p.m. UTC
On Sat, 8 Oct 2016, Lukas Wunner wrote:

> 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.

That seems like a good suggestion.  The patch is below; Pierre, can you 
test it?  This should remove the need to set the USB autosuspend module 
parameter to -1.

Alan Stern




--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Pierre de Villemereuil Oct. 11, 2016, 8:27 p.m. UTC | #1
Hi!

I'm sorry, I'm not savvy enough to know what to do with this (I know 
basics on how to code and compile, but I'm no dev). Could someone 
guide me through it?

I gather this patch needs to be applied to some kernel module code 
which needs to be compiled and reloaded into my current kernel, 
right?

Sorry to let you down...

Cheers,
Pierre.

Le mardi 11 octobre 2016, 11:18:28 NZDT Alan Stern a écrit :
> On Sat, 8 Oct 2016, Lukas Wunner wrote:
> > 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.
> 
> That seems like a good suggestion.  The patch is below; Pierre, 
can you
> test it?  This should remove the need to set the USB autosuspend 
module
> parameter to -1.
> 
> Alan Stern
> 
> 
> 
> Index: usb-4.x/drivers/pci/pci.c
> 
===================================================================
> --- usb-4.x.orig/drivers/pci/pci.c
> +++ usb-4.x/drivers/pci/pci.c
> @@ -2064,6 +2064,10 @@ bool pci_dev_run_wake(struct pci_dev *de
>  	if (!dev->pme_support)
>  		return false;
> 
> +	/* PME-capable in principle, but not from the intended sleep 
state */
> +	if (!pci_pme_capable(dev, pci_target_state(dev)))
> +		return false;
> +
>  	while (bus->parent) {
>  		struct pci_dev *bridge = bus->self;
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern Oct. 12, 2016, 6:23 p.m. UTC | #2
On Wed, 12 Oct 2016, Pierre de Villemereuil wrote:

> Hi!
> 
> I'm sorry, I'm not savvy enough to know what to do with this (I know 
> basics on how to code and compile, but I'm no dev). Could someone 
> guide me through it?
> 
> I gather this patch needs to be applied to some kernel module code 
> which needs to be compiled and reloaded into my current kernel, 
> right?

More likely you'll have to rebuild an entire new kernel, not just a 
single module.

Search the support site for the distribution you are using.  It ought
to contain reasonably thorough instructions on how to build and install
your own version of their kernel.

Once you know how to do all that, I can tell you how the patch should
be applied -- that's the easy part.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pierre de Villemereuil Oct. 13, 2016, 8:58 p.m. UTC | #3
Hi!

I've branched the kernel package on the OpenSUSE Build Server, I'll 
try to apply the patch there (this ought to be cleanest method).

Starting from the root of the kernel tarball, the patch should be 
applied to drivers/pci/pci.c, am I right?

Cheers,
Pierre.


Le mercredi 12 octobre 2016, 14:23:35 NZDT Alan Stern a écrit :
> On Wed, 12 Oct 2016, Pierre de Villemereuil wrote:
> > Hi!
> > 
> > I'm sorry, I'm not savvy enough to know what to do with this (I 
know
> > basics on how to code and compile, but I'm no dev). Could 
someone
> > guide me through it?
> > 
> > I gather this patch needs to be applied to some kernel module 
code
> > which needs to be compiled and reloaded into my current kernel,
> > right?
> 
> More likely you'll have to rebuild an entire new kernel, not just 
a
> single module.
> 
> Search the support site for the distribution you are using.  It 
ought
> to contain reasonably thorough instructions on how to build and 
install
> your own version of their kernel.
> 
> Once you know how to do all that, I can tell you how the patch 
should
> be applied -- that's the easy part.
> 
> Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Wunner Oct. 20, 2016, 10:01 a.m. UTC | #4
On Tue, Oct 11, 2016 at 11:18:28AM -0400, Alan Stern wrote:
> On Sat, 8 Oct 2016, Lukas Wunner wrote:
> > 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.
> 
> That seems like a good suggestion.  The patch is below; Pierre, can you 
> test it?  This should remove the need to set the USB autosuspend module 
> parameter to -1.

Alan, how do we proceed with this?  Are you going to submit a patch
(with commit message, tags and all) to linux-pci@ or would you prefer
me to do that?  I just went over the callers of pci_dev_run_wake()
once more and the patch still looks safe to me.

Thanks,

Lukas

> 
> Index: usb-4.x/drivers/pci/pci.c
> ===================================================================
> --- usb-4.x.orig/drivers/pci/pci.c
> +++ usb-4.x/drivers/pci/pci.c
> @@ -2064,6 +2064,10 @@ bool pci_dev_run_wake(struct pci_dev *de
>  	if (!dev->pme_support)
>  		return false;
>  
> +	/* PME-capable in principle, but not from the intended sleep state */
> +	if (!pci_pme_capable(dev, pci_target_state(dev)))
> +		return false;
> +
>  	while (bus->parent) {
>  		struct pci_dev *bridge = bus->self;
>  
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern Oct. 20, 2016, 1:57 p.m. UTC | #5
On Thu, 20 Oct 2016, Lukas Wunner wrote:

> On Tue, Oct 11, 2016 at 11:18:28AM -0400, Alan Stern wrote:
> > On Sat, 8 Oct 2016, Lukas Wunner wrote:
> > > 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.
> > 
> > That seems like a good suggestion.  The patch is below; Pierre, can you 
> > test it?  This should remove the need to set the USB autosuspend module 
> > parameter to -1.
> 
> Alan, how do we proceed with this?  Are you going to submit a patch
> (with commit message, tags and all) to linux-pci@ or would you prefer
> me to do that?  I just went over the callers of pci_dev_run_wake()
> once more and the patch still looks safe to me.

Thanks for checking.  I intend to submit it soon; there just hasn't 
been enough free time this week.  :-(

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

Index: usb-4.x/drivers/pci/pci.c
===================================================================
--- usb-4.x.orig/drivers/pci/pci.c
+++ usb-4.x/drivers/pci/pci.c
@@ -2064,6 +2064,10 @@  bool pci_dev_run_wake(struct pci_dev *de
 	if (!dev->pme_support)
 		return false;
 
+	/* PME-capable in principle, but not from the intended sleep state */
+	if (!pci_pme_capable(dev, pci_target_state(dev)))
+		return false;
+
 	while (bus->parent) {
 		struct pci_dev *bridge = bus->self;