[PATCH] usb: Add support for runtime power management of the hcd

[PATCH] usb: Add support for runtime power management of the hcd

[Matthew Garrett]

The PCI runtime power management code means that we can implement support
for powering down PCI host controllers. This patchset adds core support code
along with a new hcd flag (HCD_RUNTIME_PM) that allows hosts to opt in if
they support this functionality. Successfully tested with Intel EHCI and
UHCI, though the UHCI code may need to pay more attention to vendor-specific
features.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/usb/core/hcd-pci.c  |   30 +++++++++++++++++++++++++++++-
 drivers/usb/core/hcd.c      |    9 +++++++++
 drivers/usb/core/hcd.h      |    1 +
 drivers/usb/host/ehci-pci.c |    2 +-
 drivers/usb/host/uhci-hcd.c |    2 +-
 5 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 91f2885..f32e954 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -232,7 +232,7 @@ static int hcd_pci_suspend(struct device *dev)
 	if (retval)
 		return retval;
 
-	/* We might already be suspended (runtime PM -- not yet written) */
+	/* We might already be suspended */
 	if (pci_dev->current_state != PCI_D0)
 		return retval;
 
@@ -363,6 +363,32 @@ static int hcd_pci_restore(struct device *dev)
 	return resume_common(dev, true);
 }
 
+static int hcd_pci_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct usb_hcd *hcd = pci_get_drvdata(pci_dev);
+	int retval;
+
+	if (!(hcd->driver->flags & HCD_RUNTIME_PM))
+		return -EINVAL;
+
+	retval = hcd_pci_suspend(dev);
+	if (retval)
+		return retval;
+
+	retval = hcd_pci_suspend_noirq(dev);
+	if (retval)
+		hcd_pci_resume(dev);
+
+	return retval;
+}
+
+static int hcd_pci_runtime_resume(struct device *dev)
+{
+	hcd_pci_resume_noirq(dev);
+	return resume_common(dev, false);
+}
+
 struct dev_pm_ops usb_hcd_pci_pm_ops = {
 	.suspend	= hcd_pci_suspend,
 	.suspend_noirq	= hcd_pci_suspend_noirq,
@@ -376,6 +402,8 @@ struct dev_pm_ops usb_hcd_pci_pm_ops = {
 	.poweroff_noirq	= hcd_pci_suspend_noirq,
 	.restore_noirq	= hcd_pci_resume_noirq,
 	.restore	= hcd_pci_restore,
+	.runtime_suspend = hcd_pci_runtime_suspend,
+	.runtime_resume	= hcd_pci_runtime_resume,
 };
 EXPORT_SYMBOL_GPL(usb_hcd_pci_pm_ops);
 
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 34de475..abd75c0 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -38,6 +38,7 @@
 #include <asm/unaligned.h>
 #include <linux/platform_device.h>
 #include <linux/workqueue.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/usb.h>
 
@@ -1764,6 +1765,7 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
 	if (status == 0) {
 		usb_set_device_state(rhdev, USB_STATE_SUSPENDED);
 		hcd->state = HC_STATE_SUSPENDED;
+		pm_runtime_put(hcd->self.controller);
 	} else {
 		hcd->state = old_state;
 		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
@@ -1785,6 +1787,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 	if (hcd->state == HC_STATE_RUNNING)
 		return 0;
 
+	pm_runtime_get_sync(hcd->self.controller);
 	hcd->state = HC_STATE_RESUMING;
 	status = hcd->driver->bus_resume(hcd);
 	if (status == 0) {
@@ -1798,6 +1801,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 		hcd->state = old_state;
 		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
 				"resume", status);
+		pm_runtime_put(hcd->self.controller);
 		if (status != -ESHUTDOWN)
 			usb_hc_died(hcd);
 	}
@@ -1985,6 +1989,9 @@ struct usb_hcd *usb_create_hcd (const struct hc_driver *driver,
 	INIT_WORK(&hcd->wakeup_work, hcd_resume_work);
 #endif
 
+	pm_runtime_enable(dev);
+	pm_runtime_get(dev);
+
 	hcd->driver = driver;
 	hcd->product_desc = (driver->product_desc) ? driver->product_desc :
 			"USB Host Controller";
@@ -1996,6 +2003,8 @@ static void hcd_release (struct kref *kref)
 {
 	struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);
 
+	pm_runtime_put_sync(hcd->self.controller);
+
 	kfree(hcd);
 }
 
diff --git a/drivers/usb/core/hcd.h b/drivers/usb/core/hcd.h
index 79782a1..182a8b7 100644
--- a/drivers/usb/core/hcd.h
+++ b/drivers/usb/core/hcd.h
@@ -171,6 +171,7 @@ struct hc_driver {
 	int	flags;
 #define	HCD_MEMORY	0x0001		/* HC regs use memory (else I/O) */
 #define	HCD_LOCAL_MEM	0x0002		/* HC needs local memory */
+#define	HCD_RUNTIME_PM	0x0004		/* HC supports handling runtime PM */
 #define	HCD_USB11	0x0010		/* USB 1.1 */
 #define	HCD_USB2	0x0020		/* USB 2.0 */
 #define	HCD_USB3	0x0040		/* USB 3.0 */
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 378861b..5602101 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -370,7 +370,7 @@ static const struct hc_driver ehci_pci_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq =			ehci_irq,
-	.flags =		HCD_MEMORY | HCD_USB2,
+	.flags =		HCD_MEMORY | HCD_USB2 | HCD_RUNTIME_PM,
 
 	/*
 	 * basic lifecycle operations
diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
index 5cd0e48..c6e50aa 100644
--- a/drivers/usb/host/uhci-hcd.c
+++ b/drivers/usb/host/uhci-hcd.c
@@ -900,7 +900,7 @@ static const struct hc_driver uhci_driver = {
 
 	/* Generic hardware linkage */
 	.irq =			uhci_irq,
-	.flags =		HCD_USB11,
+	.flags =		HCD_USB11 | HCD_RUNTIME_PM,
 
 	/* Basic lifecycle operations */
 	.reset =		uhci_init,
-- 
1.6.5.2

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Oliver Neukum]

Am Montag, 9. November 2009 23:43:30 schrieb Matthew Garrett:
> The PCI runtime power management code means that we can implement support
> for powering down PCI host controllers. This patchset adds core support
>  code along with a new hcd flag (HCD_RUNTIME_PM) that allows hosts to opt
>  in if they support this functionality. Successfully tested with Intel EHCI
>  and UHCI, though the UHCI code may need to pay more attention to
>  vendor-specific features.

Extremely cool. However it makes me wonder about remote wakeup.
Do you know whether that works?

	Regards
		Oliver

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Matthew Garrett]

On Tue, Nov 10, 2009 at 09:20:03AM +0100, Oliver Neukum wrote:

> Extremely cool. However it makes me wonder about remote wakeup.
> Do you know whether that works?

It certainly seems to on Intel hardware. I haven't tested anything else 
yet.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Oliver Neukum]

Am Dienstag, 10. November 2009 13:41:09 schrieben Sie:
> On Tue, Nov 10, 2009 at 09:20:03AM +0100, Oliver Neukum wrote:
> > Extremely cool. However it makes me wonder about remote wakeup.
> > Do you know whether that works?
> 
> It certainly seems to on Intel hardware. I haven't tested anything else
> yet.

I was wondering because a tester has reported a race in ehci that
can lead to a loss of wakeup events. Alan posted a patch to fix it.
Does it cooperate with your patch?
And secondly what happens if you suspend to RAM a system
with suspended controllers?

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

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Matthew Garrett]

On Tue, Nov 10, 2009 at 02:08:01PM +0100, Oliver Neukum wrote:

> I was wondering because a tester has reported a race in ehci that
> can lead to a loss of wakeup events. Alan posted a patch to fix it.
> Does it cooperate with your patch?

Hm. I'll look into that.

> And secondly what happens if you suspend to RAM a system
> with suspended controllers?

I believe that this currently results in them being woken up and then 
whatever would normally happen, but would need to check to be sure. 
Rafael?

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Alan Stern]

On Tue, 10 Nov 2009, Matthew Garrett wrote:

> On Tue, Nov 10, 2009 at 02:08:01PM +0100, Oliver Neukum wrote:
> 
> > I was wondering because a tester has reported a race in ehci that
> > can lead to a loss of wakeup events. Alan posted a patch to fix it.
> > Does it cooperate with your patch?
> 
> Hm. I'll look into that.

There should not be any interaction.  That particular race occurs when
the root hub is suspended, not when the controller is suspended.


But this does raise an interesting point.  We do still have a race 
between remote wakeup and system sleep.

Suppose we're in the middle of a system sleep transition, and device D
(such as a USB root hub) has been suspended as part of the normal
preparation.  But then D generates a remote wakeup IRQ.

D's driver will field the interrupt request and will call something
like pm_request_resume(), to schedule a workqueue item for a runtime
resume of D.  However, the runtime-PM workqueues are either frozen or
in some other way prevented from doing anything while the system sleep
transition is in progress.

Therefore the wakeup request will get lost.  The information is still
present, in the form of a work_struct, but it won't get acted on until
the system wakes up.  In particular, it won't prevent the sleep
transition from completing and it won't cause the system to wake up
immediately after going to sleep.

This suggests that pm_request_resume() should abort a sleep transition
if one is already in progress.  Rafael, what do you think?

Alan Stern

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

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Oliver Neukum]

Am Dienstag, 10. November 2009 16:04:12 schrieb Alan Stern:
> D's driver will field the interrupt request and will call something
> like pm_request_resume(), to schedule a workqueue item for a runtime
> resume of D.  However, the runtime-PM workqueues are either frozen or
> in some other way prevented from doing anything while the system sleep
> transition is in progress.
> 
> Therefore the wakeup request will get lost.  The information is still
> present, in the form of a work_struct, but it won't get acted on until
> the system wakes up.  In particular, it won't prevent the sleep
> transition from completing and it won't cause the system to wake up
> immediately after going to sleep.
> 
> This suggests that pm_request_resume() should abort a sleep transition
> if one is already in progress.  Rafael, what do you think?

That seems to be a bit harsh. If you do not specify that a device be
able to wake the whole system, why would it be good for a request
coming from it to abort a system sleep transition?

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

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Alan Stern]

On Wed, 11 Nov 2009, Oliver Neukum wrote:

> Am Dienstag, 10. November 2009 16:04:12 schrieb Alan Stern:
> > D's driver will field the interrupt request and will call something
> > like pm_request_resume(), to schedule a workqueue item for a runtime
> > resume of D.  However, the runtime-PM workqueues are either frozen or
> > in some other way prevented from doing anything while the system sleep
> > transition is in progress.
> > 
> > Therefore the wakeup request will get lost.  The information is still
> > present, in the form of a work_struct, but it won't get acted on until
> > the system wakes up.  In particular, it won't prevent the sleep
> > transition from completing and it won't cause the system to wake up
> > immediately after going to sleep.
> > 
> > This suggests that pm_request_resume() should abort a sleep transition
> > if one is already in progress.  Rafael, what do you think?
> 
> That seems to be a bit harsh. If you do not specify that a device be
> able to wake the whole system, why would it be good for a request
> coming from it to abort a system sleep transition?

The way you specify that a device be able to wake the whole system is
by enabling its remote wakeup attribute.  If this attribute is not
enabled then the device will not send remote wakeup requests, so the
scenario described above will not occur -- no IRQ will be generated.

Alan Stern

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

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Oliver Neukum]

Am Mittwoch, 11. November 2009 18:08:17 schrieb Alan Stern:
> On Wed, 11 Nov 2009, Oliver Neukum wrote:

> > That seems to be a bit harsh. If you do not specify that a device be
> > able to wake the whole system, why would it be good for a request
> > coming from it to abort a system sleep transition?
> 
> The way you specify that a device be able to wake the whole system is
> by enabling its remote wakeup attribute.  If this attribute is not
> enabled then the device will not send remote wakeup requests, so the
> scenario described above will not occur -- no IRQ will be generated.

But what happens if a driver has requested remote wakeup requests
to do runtime power management?

	Regards
		Oliver

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Alan Stern]

On Wed, 11 Nov 2009, Oliver Neukum wrote:

> Am Mittwoch, 11. November 2009 18:08:17 schrieb Alan Stern:
> > On Wed, 11 Nov 2009, Oliver Neukum wrote:
> 
> > > That seems to be a bit harsh. If you do not specify that a device be
> > > able to wake the whole system, why would it be good for a request
> > > coming from it to abort a system sleep transition?
> > 
> > The way you specify that a device be able to wake the whole system is
> > by enabling its remote wakeup attribute.  If this attribute is not
> > enabled then the device will not send remote wakeup requests, so the
> > scenario described above will not occur -- no IRQ will be generated.
> 
> But what happens if a driver has requested remote wakeup requests
> to do runtime power management?

The driver mustn't tell the device to send remote wakeup requests if
the remote wakeup attribute isn't set.  If this means it can't do 
effective runtime PM, then so be it.

Now, this does raise another point, which has been mentioned before.  
To wit: devices really ought to have _two_ remote-wakeup attributes, 
one for runtime PM and one for system sleep.  But that would introduce 
yet more complexity and opportunities for confusion.

Alan Stern

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Rafael J. Wysocki]

On Tuesday 10 November 2009, Matthew Garrett wrote:
> On Tue, Nov 10, 2009 at 02:08:01PM +0100, Oliver Neukum wrote:
> 
> > I was wondering because a tester has reported a race in ehci that
> > can lead to a loss of wakeup events. Alan posted a patch to fix it.
> > Does it cooperate with your patch?
> 
> Hm. I'll look into that.
> 
> > And secondly what happens if you suspend to RAM a system
> > with suspended controllers?
> 
> I believe that this currently results in them being woken up and then 
> whatever would normally happen,

That's correct AFAICS.

Rafael

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Rafael J. Wysocki]

[Sorry for the delayed response, I was distracted by the btusb regression
causing resume to fail on my box.]

On Tuesday 10 November 2009, Alan Stern wrote:
> On Tue, 10 Nov 2009, Matthew Garrett wrote:
> 
> > On Tue, Nov 10, 2009 at 02:08:01PM +0100, Oliver Neukum wrote:
> > 
> > > I was wondering because a tester has reported a race in ehci that
> > > can lead to a loss of wakeup events. Alan posted a patch to fix it.
> > > Does it cooperate with your patch?
> > 
> > Hm. I'll look into that.
> 
> There should not be any interaction.  That particular race occurs when
> the root hub is suspended, not when the controller is suspended.
> 
> 
> But this does raise an interesting point.  We do still have a race 
> between remote wakeup and system sleep.

We do to some extent.  There is the check in dpm_prepare() that will abort
system sleep transitions if run-time wake-up has been requested earlier,
but later requests will be discarded.

> Suppose we're in the middle of a system sleep transition, and device D
> (such as a USB root hub) has been suspended as part of the normal
> preparation.  But then D generates a remote wakeup IRQ.
> 
> D's driver will field the interrupt request and will call something
> like pm_request_resume(), to schedule a workqueue item for a runtime
> resume of D.  However, the runtime-PM workqueues are either frozen or
> in some other way prevented from doing anything while the system sleep
> transition is in progress.
> 
> Therefore the wakeup request will get lost.  The information is still
> present, in the form of a work_struct, but it won't get acted on until
> the system wakes up.  In particular, it won't prevent the sleep
> transition from completing and it won't cause the system to wake up
> immediately after going to sleep.
> 
> This suggests that pm_request_resume() should abort a sleep transition
> if one is already in progress.  Rafael, what do you think?

That's not an easy question, becuase there always will be a point after which
we can't handle a run-time resume request.  If we are deep enough into the
suspend process, we won't receive wakeup interrupts any more, at least on
some platforms.

Thanks,
Rafael

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Rafael J. Wysocki]

On Wednesday 11 November 2009, Alan Stern wrote:
> On Wed, 11 Nov 2009, Oliver Neukum wrote:
> 
> > Am Mittwoch, 11. November 2009 18:08:17 schrieb Alan Stern:
> > > On Wed, 11 Nov 2009, Oliver Neukum wrote:
> > 
> > > > That seems to be a bit harsh. If you do not specify that a device be
> > > > able to wake the whole system, why would it be good for a request
> > > > coming from it to abort a system sleep transition?
> > > 
> > > The way you specify that a device be able to wake the whole system is
> > > by enabling its remote wakeup attribute.  If this attribute is not
> > > enabled then the device will not send remote wakeup requests, so the
> > > scenario described above will not occur -- no IRQ will be generated.
> > 
> > But what happens if a driver has requested remote wakeup requests
> > to do runtime power management?
> 
> The driver mustn't tell the device to send remote wakeup requests if
> the remote wakeup attribute isn't set.  If this means it can't do 
> effective runtime PM, then so be it.
> 
> Now, this does raise another point, which has been mentioned before.  
> To wit: devices really ought to have _two_ remote-wakeup attributes, 
> one for runtime PM and one for system sleep.

Yes, they do and there's another reason.  Namely, there apparently are
devices which can wake up the system from a sleep state and that are
unable to generate runtime wakeup events.

>   But that would introduce  yet more complexity and opportunities for
>   confusion.

Nevertheless.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Alan Stern]

On Wed, 11 Nov 2009, Rafael J. Wysocki wrote:

> > But this does raise an interesting point.  We do still have a race 
> > between remote wakeup and system sleep.
> 
> We do to some extent.  There is the check in dpm_prepare() that will abort
> system sleep transitions if run-time wake-up has been requested earlier,
> but later requests will be discarded.

Exactly.  The race I was referring to doesn't involve runtime PM; it 
occurs when a device is suspended as part of a sleep transition but 
then generates a wakeup request before the transition has finished.

> > Suppose we're in the middle of a system sleep transition, and device D
> > (such as a USB root hub) has been suspended as part of the normal
> > preparation.  But then D generates a remote wakeup IRQ.
> > 
> > D's driver will field the interrupt request and will call something
> > like pm_request_resume(), to schedule a workqueue item for a runtime
> > resume of D.  However, the runtime-PM workqueues are either frozen or
> > in some other way prevented from doing anything while the system sleep
> > transition is in progress.
> > 
> > Therefore the wakeup request will get lost.  The information is still
> > present, in the form of a work_struct, but it won't get acted on until
> > the system wakes up.  In particular, it won't prevent the sleep
> > transition from completing and it won't cause the system to wake up
> > immediately after going to sleep.
> > 
> > This suggests that pm_request_resume() should abort a sleep transition
> > if one is already in progress.  Rafael, what do you think?
> 
> That's not an easy question, becuase there always will be a point after which
> we can't handle a run-time resume request.  If we are deep enough into the
> suspend process, we won't receive wakeup interrupts any more, at least on
> some platforms.

I don't know how to fix this in general...  The ultimate answer has to 
be a synchronization point of some sort.  Wakeup requests received 
before this point should cause the sleep to be aborted.  Requests 
received later should remain pending in the hardware and cause the 
system to wake up as soon as it goes to sleep.  But where this point is 
will depend on the platform.

On regular PCs, a likely spot is the place where we disable the IRQ 
lines.  For other types of system, I don't know.

Alan Stern

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Alan Stern]

On Wed, 11 Nov 2009, Rafael J. Wysocki wrote:

> > Now, this does raise another point, which has been mentioned before.  
> > To wit: devices really ought to have _two_ remote-wakeup attributes, 
> > one for runtime PM and one for system sleep.
> 
> Yes, they do and there's another reason.  Namely, there apparently are
> devices which can wake up the system from a sleep state and that are
> unable to generate runtime wakeup events.

That's okay.  There's no harm in trying to enable remote wakeup on a 
device which doesn't support it.

> >   But that would introduce  yet more complexity and opportunities for
> >   confusion.
> 
> Nevertheless.

The real question is whether there are devices for which we explicitly
want to enable remote wakeup during runtime PM but not system sleep (or
vice versa, but this seems rather unlikely).  USB host controllers
might be a good example; as a rule people don't want their laptops to
wake up when they unplug a USB mouse after suspending the machine.

Alan Stern

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Matthew Garrett]

On Wed, Nov 11, 2009 at 06:09:24PM -0500, Alan Stern wrote:

> That's okay.  There's no harm in trying to enable remote wakeup on a 
> device which doesn't support it.

There's harm if we're using the ability to generate remote waleups as a 
condition for whether we can perform runtime pm on the device...

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Oliver Neukum]

Am Donnerstag, 12. November 2009 01:33:12 schrieb Matthew Garrett:
> On Wed, Nov 11, 2009 at 06:09:24PM -0500, Alan Stern wrote:
> > That's okay.  There's no harm in trying to enable remote wakeup on a 
> > device which doesn't support it.
> 
> There's harm if we're using the ability to generate remote waleups as a 
> condition for whether we can perform runtime pm on the device...

That I would consider backwards. If the user enables runtime power management
of a device, he has to live with remote wakeup being enabled. At most
the kernel might resume and switch off remote wakeup on the device,
but we might also leave this to user space.

	Regards
		Oliver

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Oliver Neukum]

Am Mittwoch, 11. November 2009 23:24:57 schrieb Rafael J. Wysocki:
> > Now, this does raise another point, which has been mentioned before.  
> > To wit: devices really ought to have two remote-wakeup attributes, 
> > one for runtime PM and one for system sleep.
> 
> Yes, they do and there's another reason.  Namely, there apparently are
> devices which can wake up the system from a sleep state and that are
> unable to generate runtime wakeup events.

That is outright disgusting. Which devices do this?

It seems to me that if the kernel cannot tell in these cases, we
really need two attributes and there's no alternative.

	Regards
		Oliver

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Oliver Neukum]

Am Donnerstag, 12. November 2009 00:09:24 schrieb Alan Stern:
> > Yes, they do and there's another reason.  Namely, there apparently are
> > devices which can wake up the system from a sleep state and that are
> > unable to generate runtime wakeup events.
> 
> That's okay.  There's no harm in trying to enable remote wakeup on a 
> device which doesn't support it.

That's a bold statement. If Windows doesn't do it, it is most likely a
false statement.

	Regards
		Oliver

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Matthew Garrett]

On Thu, Nov 12, 2009 at 11:31:05AM +0100, Oliver Neukum wrote:
> Am Mittwoch, 11. November 2009 23:24:57 schrieb Rafael J. Wysocki:
> > > Now, this does raise another point, which has been mentioned before.  
> > > To wit: devices really ought to have two remote-wakeup attributes, 
> > > one for runtime PM and one for system sleep.
> > 
> > Yes, they do and there's another reason.  Namely, there apparently are
> > devices which can wake up the system from a sleep state and that are
> > unable to generate runtime wakeup events.
> 
> That is outright disgusting. Which devices do this?

The ability for a device to raise an ACPI GPE and trigger a wakeup does 
not imply that there's runtime handling for that device's GPE in the 
system's ACPI tables. The default GPE handling code I posted for Intel 
helps here, but there's ample opportunity for us to find machines which 
have system wakeup support but no runtime wakeup support.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Matthew Garrett]

On Thu, Nov 12, 2009 at 08:41:11AM +0100, Oliver Neukum wrote:
> Am Donnerstag, 12. November 2009 01:33:12 schrieb Matthew Garrett:
> > On Wed, Nov 11, 2009 at 06:09:24PM -0500, Alan Stern wrote:
> > > That's okay.  There's no harm in trying to enable remote wakeup on a 
> > > device which doesn't support it.
> > 
> > There's harm if we're using the ability to generate remote waleups as a 
> > condition for whether we can perform runtime pm on the device...
> 
> That I would consider backwards. If the user enables runtime power management
> of a device, he has to live with remote wakeup being enabled. At most
> the kernel might resume and switch off remote wakeup on the device,
> but we might also leave this to user space.

My point was that if we don't have remote wakeup support at runtime, we 
can't enable runtime PM of the device. So we need to be able to 
distinguish between runtime remote wakeup and system sleep remote 
wakeup.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Alan Stern]

On Thu, 12 Nov 2009, Matthew Garrett wrote:

> On Wed, Nov 11, 2009 at 06:09:24PM -0500, Alan Stern wrote:
> 
> > That's okay.  There's no harm in trying to enable remote wakeup on a 
> > device which doesn't support it.
> 
> There's harm if we're using the ability to generate remote waleups as a 
> condition for whether we can perform runtime pm on the device...

You misunderstood me.  There's no harm in _trying_ to enable remote 
wakeup.  The harm comes when the attempt fails but the drivers thinks 
it succeeded.

Alan Stern

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

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Alan Stern]

On Thu, 12 Nov 2009, Oliver Neukum wrote:

> Am Donnerstag, 12. November 2009 00:09:24 schrieb Alan Stern:
> > > Yes, they do and there's another reason.  Namely, there apparently are
> > > devices which can wake up the system from a sleep state and that are
> > > unable to generate runtime wakeup events.
> > 
> > That's okay.  There's no harm in trying to enable remote wakeup on a 
> > device which doesn't support it.
> 
> That's a bold statement. If Windows doesn't do it, it is most likely a
> false statement.

Let's rephrase it: There's no harm in trying to enable remote wakeup on 
a device which can wake up the system from a sleep state but can't 
generate runtime wakeup events.

Alan Stern

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

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Alan Stern]

On Thu, 12 Nov 2009, Matthew Garrett wrote:

> On Thu, Nov 12, 2009 at 08:41:11AM +0100, Oliver Neukum wrote:
> > Am Donnerstag, 12. November 2009 01:33:12 schrieb Matthew Garrett:
> > > On Wed, Nov 11, 2009 at 06:09:24PM -0500, Alan Stern wrote:
> > > > That's okay.  There's no harm in trying to enable remote wakeup on a 
> > > > device which doesn't support it.
> > > 
> > > There's harm if we're using the ability to generate remote waleups as a 
> > > condition for whether we can perform runtime pm on the device...
> > 
> > That I would consider backwards. If the user enables runtime power management
> > of a device, he has to live with remote wakeup being enabled. At most
> > the kernel might resume and switch off remote wakeup on the device,
> > but we might also leave this to user space.
> 
> My point was that if we don't have remote wakeup support at runtime, we 
> can't enable runtime PM of the device. So we need to be able to 
> distinguish between runtime remote wakeup and system sleep remote 
> wakeup.

You are mixing up two different scenarios.  The real trouble arises if
we can't tell whether runtime remote wakeup is available, or we think
it is available when in fact it isn't.

But if we know accurately that remote wakeup isn't available at runtime
then there's no problem -- the driver simply doesn't use runtime PM.
Even if the "wakeup" attribute in sysfs is enabled.

Alan Stern

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Matthew Garrett]

On Thu, Nov 12, 2009 at 06:00:02PM +0100, Oliver Neukum wrote:

> Exactly, but that isn't the same as needing two attributes exported to user
> space. That is an absolute requirement only if we can't tell what a device's
> capabilities are.

Right. I don't see any reason for runtime wakeup to be exposed to 
userspace - it's an entirely orthogonal concept to system wakeup. The 
relevant userspace policy is whether or not runtime pm is enabled.

> We could introduce a second attribute if we want to prevent drivers from
> using remote wakeup to do runtime power management, but it is not
> required. We could simply not give user space that choice.

Absolutely.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Oliver Neukum]

Am Donnerstag, 12. November 2009 15:51:22 schrieb Matthew Garrett:
> On Thu, Nov 12, 2009 at 08:41:11AM +0100, Oliver Neukum wrote:

> > That I would consider backwards. If the user enables runtime power
> > management of a device, he has to live with remote wakeup being enabled.
> > At most the kernel might resume and switch off remote wakeup on the
> > device, but we might also leave this to user space.
> 
> My point was that if we don't have remote wakeup support at runtime, we
> can't enable runtime PM of the device. So we need to be able to
> distinguish between runtime remote wakeup and system sleep remote
> wakeup.

Exactly, but that isn't the same as needing two attributes exported to user
space. That is an absolute requirement only if we can't tell what a device's
capabilities are.
We could introduce a second attribute if we want to prevent drivers from
using remote wakeup to do runtime power management, but it is not
required. We could simply not give user space that choice.

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

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Oliver Neukum]

Am Donnerstag, 12. November 2009 17:59:58 schrieb Matthew Garrett:
> On Thu, Nov 12, 2009 at 06:00:02PM +0100, Oliver Neukum wrote:
> > Exactly, but that isn't the same as needing two attributes exported to
> > user space. That is an absolute requirement only if we can't tell what a
> > device's capabilities are.
> 
> Right. I don't see any reason for runtime wakeup to be exposed to 
> userspace - it's an entirely orthogonal concept to system wakeup. The 
> relevant userspace policy is whether or not runtime pm is enabled.

That brings me to a possibly useless, wild idea. Do we want to tristate
this? Is there any use in a "don't care" setting?

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

Re: [PATCH] usb: Add support for runtime power management of the hcd

[Rafael J. Wysocki]

On Monday 16 November 2009, Oliver Neukum wrote:
> Am Donnerstag, 12. November 2009 17:59:58 schrieb Matthew Garrett:
> > On Thu, Nov 12, 2009 at 06:00:02PM +0100, Oliver Neukum wrote:
> > > Exactly, but that isn't the same as needing two attributes exported to
> > > user space. That is an absolute requirement only if we can't tell what a
> > > device's capabilities are.
> > 
> > Right. I don't see any reason for runtime wakeup to be exposed to 
> > userspace - it's an entirely orthogonal concept to system wakeup. The 
> > relevant userspace policy is whether or not runtime pm is enabled.
> 
> That brings me to a possibly useless, wild idea. Do we want to tristate
> this? Is there any use in a "don't care" setting?

I'm not sure what the "don't care" value would be useful for.

Besides, I don't really like tristates. :-)

Thanks,
Rafael