PATCH/RFC: usbcore wakeup updates (3/4)

PATCH/RFC: usbcore wakeup updates (3/4)

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 156 bytes --]

There were already some hooks in usbcore, but they were only
configurable for root hubs ... but not keyboards, mice, Ethernet
adapters, or other devices.



[-- Attachment #2: wake-usbcore.patch --]
[-- Type: text/x-diff, Size: 8356 bytes --]

This replaces USB core wakeup bits with new pmcore wakeup bits:

 - Initialized or disabled in usb_set_configuration(), depending
   on whether the configuration supports wakeup;
 - Lets userspace change the initial may_wakeup policy, eliminating
   some guesswork marked by a couple FIXMEs
 - Virtual root hub uses the driver model wakeup support, which can
   change the initial may_wakeup policy.
 - PCI bus glue updates, including using those pmcore wakeup bits.

Since I'm too lazy to split them out, the PCI bus glue updates include
some unrelated changes, notably:

 - Handles "suspend deeper" (e.g. PCI_D1 to PCI_D2) per spec, although
   nobody is likely to issue such requests lately.

 - Requests to suspend to unsupported D1 and D2 states are treated
   as just hints, where D3cold is OK too ... many systems need this
   to enter ACPI G1/S1 (standby).
   
 - Makes "legacy" systems act like they support PCI PM, so they can
   enter ACPI G1 states ... though of course without wakeup support.
   (There may be some wierdness lurking here though, since some of
   them can wakeup from S3/STR even though PCI PME# isn't involved.)

NOTE:  breaks both OHCI and EHCI builds.

[ against 2.6.9-rc3 ]


--- 1.31/drivers/usb/core/hcd-pci.c	Thu Sep 30 11:23:16 2004
+++ edited/drivers/usb/core/hcd-pci.c	Mon Oct  4 12:56:41 2004
@@ -266,6 +266,18 @@
 
 #ifdef	CONFIG_PM
 
+static char __attribute_used__ *pci_state(u32 state)
+{
+	switch (state) {
+	case 0:		return "D0";
+	case 1:		return "D1";
+	case 2:		return "D2";
+	case 3:		return "D3hot";
+	case 4:		return "D3cold";
+	}
+	return NULL;
+}
+
 /**
  * usb_hcd_pci_suspend - power management suspend of a PCI-based HCD
  * @dev: USB Host Controller being suspended
@@ -286,16 +298,34 @@
 	 * PM-sensitive HCDs may already have done this.
 	 */
 	has_pci_pm = pci_find_capability(dev, PCI_CAP_ID_PM);
-	if (has_pci_pm)
-		dev_dbg(hcd->self.controller, "suspend D%d --> D%d\n",
-			dev->current_state, state);
+	if (state > 4)
+		state = 4;
 
 	switch (hcd->state) {
 	case USB_STATE_HALT:
 		dev_dbg (hcd->self.controller, "halted; hcd not suspended\n");
 		break;
 	case HCD_STATE_SUSPENDED:
-		dev_dbg (hcd->self.controller, "hcd already suspended\n");
+		dev_dbg (hcd->self.controller, "PCI %s --> %s\n",
+				pci_state(dev->current_state),
+				pci_state(state));
+		if (state > 3)
+			state = 3;
+
+		if (state == dev->current_state)
+			break;
+		else if (state < dev->current_state)
+			retval = -EIO;
+		else if (has_pci_pm)
+			retval = pci_set_power_state (dev, state);
+		else
+			dev->current_state = state;
+
+		if (retval == 0)
+			dev->dev.power.power_state = state;
+		else
+			dev_dbg (hcd->self.controller, 
+					"re-suspend fail, %d\n", retval);
 		break;
 	default:
 		retval = hcd->driver->suspend (hcd, state);
@@ -306,22 +336,48 @@
 		else {
 			hcd->state = HCD_STATE_SUSPENDED;
 			pci_save_state (dev, hcd->pci_state);
-#ifdef	CONFIG_USB_SUSPEND
-			pci_enable_wake (dev, state, hcd->remote_wakeup);
-			pci_enable_wake (dev, 4, hcd->remote_wakeup);
-#endif
+
 			/* no DMA or IRQs except in D0 */
 			pci_disable_device (dev);
 			free_irq (hcd->irq, hcd);
 			
-			if (has_pci_pm)
+			if (has_pci_pm) {
 				retval = pci_set_power_state (dev, state);
-			dev->dev.power.power_state = state;
+
+				/* POLICY: treat D1/D2 as just hints; D3hot
+				 * always works (may take longer to resume).
+				 */
+				if (retval < 0 && state < 3) {
+					retval = pci_set_power_state (dev, 3);
+					if (retval == 0)
+						state = 3;
+				}
+				if (retval == 0) {
+#ifdef	CONFIG_USB_SUSPEND
+					int do_resume =
+						device_may_wakeup (&dev->dev);
+					pci_enable_wake (dev, state, do_resume);
+					pci_enable_wake (dev, 4, do_resume);
+#endif
+					dev->dev.power.power_state = state;
+				}
+			} else {
+				if (state > 3)
+					state = 3;
+				dev->current_state = state;
+				dev->dev.power.power_state = state;
+			}
 			if (retval < 0) {
 				dev_dbg (&dev->dev,
-						"PCI suspend fail, %d\n",
+						"PCI %s suspend fail, %d\n",
+						pci_state(state),
 						retval);
 				(void) usb_hcd_pci_resume (dev);
+			} else {
+				dev_dbg(hcd->self.controller,
+					"suspended to PCI %s%s\n",
+					pci_state(state),
+					has_pci_pm ? " (PM)" : "");
 			}
 		}
 	}
@@ -343,9 +399,10 @@
 
 	hcd = pci_get_drvdata(dev);
 	has_pci_pm = pci_find_capability(dev, PCI_CAP_ID_PM);
-	if (has_pci_pm)
-		dev_dbg(hcd->self.controller, "resume from state D%d\n",
-				dev->current_state);
+
+	/* D3cold resume isn't usually reported this way... */
+	dev_dbg(hcd->self.controller, "resume from PCI %s\n",
+			pci_state(dev->current_state));
 
 	if (hcd->state != HCD_STATE_SUSPENDED) {
 		dev_dbg (hcd->self.controller, 
@@ -356,6 +413,8 @@
 
 	if (has_pci_pm)
 		pci_set_power_state (dev, 0);
+	else
+		dev->current_state = 0;
 	dev->dev.power.power_state = 0;
 	retval = request_irq (dev->irq, usb_hcd_irq, SA_SHIRQ,
 				hcd->description, hcd);
--- 1.67/drivers/usb/core/message.c	Thu Aug 26 16:29:16 2004
+++ edited/drivers/usb/core/message.c	Mon Oct  4 12:53:40 2004
@@ -1297,10 +1297,16 @@
 		goto free_interfaces;
 
 	dev->actconfig = cp;
+	device_init_wakeup(&dev->dev, 0);
 	if (!cp)
 		usb_set_device_state(dev, USB_STATE_ADDRESS);
 	else {
 		usb_set_device_state(dev, USB_STATE_CONFIGURED);
+		if (cp->desc.bmAttributes & USB_CONFIG_ATT_WAKEUP) {
+			dev_dbg (&dev->dev, "config #%d can issue wakeup\n",
+				configuration);
+			device_init_wakeup(&dev->dev, 1);
+		}
 
 		/* Initialize the new interface structures and the
 		 * hc/hcd/usbcore interface/endpoint state.
--- 1.97/drivers/usb/core/hcd.c	Mon Sep 13 16:23:44 2004
+++ edited/drivers/usb/core/hcd.c	Mon Oct  4 12:53:40 2004
@@ -352,19 +352,21 @@
 	/* DEVICE REQUESTS */
 
 	case DeviceRequest | USB_REQ_GET_STATUS:
-		ubuf [0] = (hcd->remote_wakeup << USB_DEVICE_REMOTE_WAKEUP)
+		ubuf [0] = (device_may_wakeup(&urb->dev->dev)
+					<< USB_DEVICE_REMOTE_WAKEUP)
 				| (1 << USB_DEVICE_SELF_POWERED);
 		ubuf [1] = 0;
 		break;
 	case DeviceOutRequest | USB_REQ_CLEAR_FEATURE:
 		if (wValue == USB_DEVICE_REMOTE_WAKEUP)
-			hcd->remote_wakeup = 0;
+			device_set_wakeup_enable(&urb->dev->dev, 0);
 		else
 			goto error;
 		break;
 	case DeviceOutRequest | USB_REQ_SET_FEATURE:
-		if (hcd->can_wakeup && wValue == USB_DEVICE_REMOTE_WAKEUP)
-			hcd->remote_wakeup = 1;
+		if (wValue == USB_DEVICE_REMOTE_WAKEUP
+				&& device_can_wakeup(&urb->dev->dev))
+			device_set_wakeup_enable(&urb->dev->dev, 1);
 		else
 			goto error;
 		break;
@@ -392,7 +394,7 @@
 				bufp = fs_rh_config_descriptor;
 				len = sizeof fs_rh_config_descriptor;
 			}
-			if (hcd->can_wakeup)
+			if (device_can_wakeup(&urb->dev->dev))
 				patch_wakeup = 1;
 			break;
 		case USB_DT_STRING << 8:
--- 1.46/drivers/usb/core/hcd.h	Thu Sep 30 11:23:16 2004
+++ edited/drivers/usb/core/hcd.h	Mon Oct  4 12:55:19 2004
@@ -74,8 +74,6 @@
 	 */
 	struct hc_driver	*driver;	/* hw-specific hooks */
 	unsigned		saw_irq : 1;
-	unsigned		can_wakeup:1;	/* hw supports wakeup? */
-	unsigned		remote_wakeup:1;/* sw should use wakeup? */
 	int			irq;		/* irq allocated */
 	void __iomem		*regs;		/* device memory/io */
 
@@ -354,11 +352,13 @@
 {
 	/* hcd->driver->start() reported can_wakeup, probably with
 	 * assistance from board's boot firmware.
-	 * NOTE:  normal devices won't enable wakeup by default.
+	 * NOTE:  usb_set_configuration() enables wakeup by default;
+	 * it can then be disabled from userspace.
 	 */
-	if (hcd->can_wakeup)
+#ifdef	CONFIG_USB_SUSPEND
+	if (device_can_wakeup(hcd->self.controller))
 		dev_dbg (hcd->self.controller, "supports USB remote wakeup\n");
-	hcd->remote_wakeup = hcd->can_wakeup;
+#endif
 
 	return usb_register_root_hub (usb_dev, hcd->self.controller);
 }
--- 1.108/drivers/usb/core/hub.c	Mon Sep 13 11:47:25 2004
+++ edited/drivers/usb/core/hub.c	Mon Oct  4 12:53:40 2004
@@ -1422,11 +1422,7 @@
 	 * NOTE:  OTG devices may issue remote wakeup (or SRP) even when
 	 * we don't explicitly enable it here.
 	 */
-	if (udev->actconfig
-			// && FIXME (remote wakeup enabled on this bus)
-			// ... currently assuming it's always appropriate
-			&& (udev->actconfig->desc.bmAttributes
-				& USB_CONFIG_ATT_WAKEUP) != 0) {
+	if (device_may_wakeup(&udev->dev)) {
 		status = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
 				USB_REQ_SET_FEATURE, USB_RECIP_DEVICE,
 				USB_DEVICE_REMOTE_WAKEUP, 0,

Re: PATCH/RFC: usbcore wakeup updates (3/4)

[Pavel Machek]

Hi!

> There were already some hooks in usbcore, but they were only
> configurable for root hubs ... but not keyboards, mice, Ethernet
> adapters, or other devices.

That "when asked about D1 enter D3" bit worries me a bit, because
it is (ugly) workaround to core problems, but I can survive it.

Introducing enums where PCI suspend level is stored in u32 would be welcome...

				Pavel
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: PATCH/RFC: usbcore wakeup updates (3/4)

[David Brownell]

Hi Pavel,

On Wednesday 06 October 2004 3:51 am, Pavel Machek wrote:
> > There were already some hooks in usbcore, but they were only
> > configurable for root hubs ... but not keyboards, mice, Ethernet
> > adapters, or other devices.
> 
> That "when asked about D1 enter D3" bit worries me a bit, because
> it is (ugly) workaround to core problems, but I can survive it.

I'm not sure what a better fix would be though ... I think WIndows
doesn't bother entering a low power state at all in such cases.
Which seems particularly pointless for the typical USB controller,
which is probably idle at that point already, and which can't take
all that much longer to resume from D3hot than from D1.

But that part is peripheral to the thrust of this set of patches.

Do you think adding those two bits to per-device PM state
is basically a good way to introduce their wakeup capabilities
to the PM core?  Suggestions on the next step?


> Introducing enums where PCI suspend level is stored in u32
> would be welcome... 

I'm not averse to enums, especially once sparse does good things
with them, but I still think that sort of change would just be nibbling
around the edges of a larger problem.  (Which should be addressed
by different patches making device power states/policies, like G0/D1
or "idle yourself", be types that are fully distinct from system power
states like G1/S3, and for which abuse creates compiler errors.)

- Dave


> 
> 				Pavel
> -- 
> 64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         
> 
> 

Re: PATCH/RFC: usbcore wakeup updates (3/4)

[Pavel Machek]

Hi!

> > > There were already some hooks in usbcore, but they were only
> > > configurable for root hubs ... but not keyboards, mice, Ethernet
> > > adapters, or other devices.
> > 
> > That "when asked about D1 enter D3" bit worries me a bit, because
> > it is (ugly) workaround to core problems, but I can survive it.
> 
> I'm not sure what a better fix would be though ... I think WIndows
> doesn't bother entering a low power state at all in such cases.
> Which seems particularly pointless for the typical USB controller,
> which is probably idle at that point already, and which can't take
> all that much longer to resume from D3hot than from D1.

Hmm, perhaps it is wrong thing to tell the devices what state to enter
in the first place.

> Do you think adding those two bits to per-device PM state
> is basically a good way to introduce their wakeup capabilities
> to the PM core?  Suggestions on the next step?

It did not look overly ugly to me... so it is probably okay. Not sure
what the next step is -- you'll probably want some sysfs interface for
suspending single devices?

> > Introducing enums where PCI suspend level is stored in u32
> > would be welcome... 
> 
> I'm not averse to enums, especially once sparse does good things
> with them, but I still think that sort of change would just be nibbling
> around the edges of a larger problem.  (Which should be addressed
> by different patches making device power states/policies, like G0/D1
> or "idle yourself", be types that are fully distinct from system power
> states like G1/S3, and for which abuse creates compiler errors.)

I'm not sure we want to move to anything complicated than simple enum.

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: PATCH/RFC: usbcore wakeup updates (3/4)

[David Brownell]

On Thursday 07 October 2004 2:19 pm, Pavel Machek wrote:
> 
> Hmm, perhaps it is wrong thing to tell the devices what state to enter
> in the first place.

As a rule, yes.  (I think you've made that point before!)

During system suspend transitions, pmcore should just
be telling drivers to "get compatible with system state X",
after "idle everything" and (for swsusp/pmdisk) "write
this image to swap" steps.  PMcore has no business
telling drivers which device-specific modes to use; it
isn't supposed to know any hardware that well.  It
wouldn't know, for example, that when wakeup is
enabled, certain states are ruled out ...

Though I don't see anything obviously broken about
(for example) using sysfs to force some devices into
PCI_D3hot state ... or with maintaining compatibility
with today's PCI API, which talks in terms of power state.


> > Do you think adding those two bits to per-device PM state
> > is basically a good way to introduce their wakeup capabilities
> > to the PM core?  Suggestions on the next step?
> 
> It did not look overly ugly to me... so it is probably okay. Not sure
> what the next step is 

Merging into some tree... presumably after 2.6.10 opens!  :)


> -- you'll probably want some sysfs interface for 
> suspending single devices?

Other than "echo -n $(IDENTIFIER) > power/state" ?  No,
we have sysfs support already!  But problems with it include:

 - Minor annoyance:  "echo" doesn't work, "echo -n" does!

 - The sysfs code needs to handle suspending a _tree_ not
   just a single device ... it'll have children if the device is a
   bus adapter (HCD), bridge (hub), or just the floor of a stack
   of virtualized drivers (usb-storage hotplugging SCSI hosts and
   disks on the fly, network adapters, etc).  This needs to do
   bottom-up-suspend and top-down-resume -- but it doesn't.
   (And USB has some workarounds, but they may need to cross
   from USB into other driver stacks ...)

 - Semantics of $(IDENTIFIER).  I think those need to include
   driver-specific values, and that means ripping out code that
   "knows" otherwise.  That includes in PMcore code.  And
   what makes most sense to me there involves two different
   sets of state identifiers, identified as meaningful strings not
   cryptic digits:  (a) a handful of generic states like "idle",
   "lowpower", "on", and "off", plus (b) device-specific states
   that might borrow from the bus (PCI_D1, PCI_D3hot, etc)
   but which can be customized to match the hardware.

   For some devices, "idle" and "lowpower" may both map
   to "off"; for some, "lowpower" may mean "cut power when
   idle" (instead of "full power all the time").

Near term, I'd hope for tree suspend/resume but start to clean
up the mess involved with $(IDENTIFIER).  (Plus make the
wakeup mechanisms work better.)


> > > Introducing enums where PCI suspend level is stored in u32
> > > would be welcome... 
> > 
> > I'm not averse to enums, especially once sparse does good things
> > with them, but I still think that sort of change would just be nibbling
> > around the edges of a larger problem.  (Which should be addressed
> > by different patches making device power states/policies, like G0/D1
> > or "idle yourself", be types that are fully distinct from system power
> > states like G1/S3, and for which abuse creates compiler errors.)
> 
> I'm not sure we want to move to anything complicated than simple enum.

I'm pretty sure we should.  If for no other reason than to force
all the drivers to change.  They disagree about what $IDENTIFIER
means because enums are basically un-typechecked integers,
and that's unlikely to change.

But also, since a typed struct pointer can support lots of other
policy structures, including things analagous to "cpufreq"
governors.  A set of drivers can agree (maybe because they
share the same bus, or are otherwise related) that the
pointer gets container_of() treatment to morph to something
packaginging much more interesting power policies than a
simple PCI bus needs.  Like for example suspending four
devices, on different busses, together -- or not at all.  Or
understanding that when these devices all suspend, one
of the power supplies (or clocks) can be disabled.

- Dave


> 								Pavel
> -- 
> People were complaining that M$ turns users into beta-testers...
> ...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
> 

Re: PATCH/RFC: usbcore wakeup updates (3/4)

[Pavel Machek]

Hi!

> Though I don't see anything obviously broken about
> (for example) using sysfs to force some devices into
> PCI_D3hot state ... or with maintaining compatibility
> with today's PCI API, which talks in terms of power state.

Nothing is wrong with user telling us to go to specific state. But it is about the
only case where explicit PCI state is neccessary (AFAICS).

>  - The sysfs code needs to handle suspending a _tree_ not
>    just a single device ... it'll have children if the device is a
>    bus adapter (HCD), bridge (hub), or just the floor of a stack
>    of virtualized drivers (usb-storage hotplugging SCSI hosts and
>    disks on the fly, network adapters, etc).  This needs to do
>    bottom-up-suspend and top-down-resume -- but it doesn't.
>    (And USB has some workarounds, but they may need to cross
>    from USB into other driver stacks ...)

I believe nigel has patches...

>  - Semantics of _(IDENTIFIER).  I think those need to include
>    driver-specific values, and that means ripping out code that
>    "knows" otherwise.  That includes in PMcore code.  And
>    what makes most sense to me there involves two different
>    sets of state identifiers, identified as meaningful strings not
>    cryptic digits:  (a) a handful of generic states like "idle",
>    "lowpower", "on", and "off", plus (b) device-specific states
>    that might borrow from the bus (PCI_D1, PCI_D3hot, etc)
>    but which can be customized to match the hardware.

I do not think that having both generic and specific states at same interface is
nice... Perhaps we could make it exclusive? If generic state "ON" is same as "PCI_D0",
just forget about "PCI_D0" and always use "ON"...
But these are small details.

> > I'm not sure we want to move to anything complicated than simple enum.
> 
> I'm pretty sure we should.  If for no other reason than to force
> all the drivers to change.  They disagree about what _IDENTIFIER
> means because enums are basically un-typechecked integers,
> and that's unlikely to change.

Well, I am not sure we want "flag day" for drivers.
If we introduce enums than kill all the sparse warnings, we can
get there, too...

> But also, since a typed struct pointer can support lots of other
> policy structures, including things analagous to "cpufreq"
> governors.  A set of drivers can agree (maybe because they
> share the same bus, or are otherwise related) that the
> pointer gets container_of() treatment to morph to something
> packaginging much more interesting power policies than a
> simple PCI bus needs.  Like for example suspending four
> devices, on different busses, together -- or not at all.  Or
> understanding that when these devices all suspend, one
> of the power supplies (or clocks) can be disabled.

I don't see how this could work. Who would call the governors?
Core? Driver? How would you associate drivers with governors?

-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms