Runtime PM: Improve support for PCI devices

Runtime PM: Improve support for PCI devices

[Matthew Garrett]

Right now we only support runtime PCI PM on devices that provide GPE methods.
That's not strictly necessary - we can find GPE associations from the _PRW
data and then install our own handlers. This patchset provides support for
allowing ACPI handlers to be installed for PCI devices and enables runtime
PM on a wider range of machines (Thinkpads, for instance). It also adds
support for polling legacy PCI devices to see if their PME flag has gone
high, since some vendors appear to have decided that copper is a sufficiently
precious resource that attaching that to anything would be an unnecessary
extravagence.

[PATCH 1/5] ACPI: Allow handlers to be installed at the same time as methods

[Matthew Garrett]

There are circumstances under which it may be desirable for GPE handlers
to be installable without displacing the existing GPE method. Add support
for this via a boolean argument to acpi_install_gpe_handler, and fix up the
existing users to ensure that their behaviour doesn't change.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/acpica/aclocal.h    |    7 ++--
 drivers/acpi/acpica/evgpe.c      |   75 ++++++++++++++++++--------------------
 drivers/acpi/acpica/evgpeinit.c  |   11 +-----
 drivers/acpi/acpica/evgpeutil.c  |    5 +--
 drivers/acpi/acpica/evxface.c    |   23 ++++++------
 drivers/acpi/ec.c                |    2 +-
 drivers/char/ipmi/ipmi_si_intf.c |    2 +-
 include/acpi/acpixf.h            |    3 +-
 8 files changed, 56 insertions(+), 72 deletions(-)

diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
index df85b53..66e3dff 100644
--- a/drivers/acpi/acpica/aclocal.h
+++ b/drivers/acpi/acpica/aclocal.h
@@ -406,16 +406,15 @@ struct acpi_predefined_data {
  *
  ****************************************************************************/
 
-/* Dispatch info for each GPE -- either a method or handler, cannot be both */
+/* Dispatch info for each GPE */
 
 struct acpi_handler_info {
 	acpi_event_handler address;	/* Address of handler, if any */
 	void *context;		/* Context to be passed to handler */
-	struct acpi_namespace_node *method_node;	/* Method node for this GPE level (saved) */
 	u8 orig_flags;		/* Original misc info about this GPE */
 };
 
-union acpi_gpe_dispatch_info {
+struct acpi_gpe_dispatch_info {
 	struct acpi_namespace_node *method_node;	/* Method node for this GPE level */
 	struct acpi_handler_info *handler;
 };
@@ -425,7 +424,7 @@ union acpi_gpe_dispatch_info {
  * NOTE: Important to keep this struct as small as possible.
  */
 struct acpi_gpe_event_info {
-	union acpi_gpe_dispatch_info dispatch;	/* Either Method or Handler */
+	struct acpi_gpe_dispatch_info dispatch;
 	struct acpi_gpe_register_info *register_info;	/* Backpointer to register info */
 	u8 flags;		/* Misc info about this GPE */
 	u8 gpe_number;		/* This GPE */
diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
index f226eac..c4b1c4c 100644
--- a/drivers/acpi/acpica/evgpe.c
+++ b/drivers/acpi/acpica/evgpe.c
@@ -474,9 +474,7 @@ static void ACPI_SYSTEM_XFACE acpi_ev_asynch_execute_gpe_method(void *context)
 	 * Must check for control method type dispatch one more time to avoid a
 	 * race with ev_gpe_install_handler
 	 */
-	if ((local_gpe_event_info.flags & ACPI_GPE_DISPATCH_MASK) ==
-	    ACPI_GPE_DISPATCH_METHOD) {
-
+	if (local_gpe_event_info.flags & ACPI_GPE_DISPATCH_METHOD) {
 		/* Allocate the evaluation information block */
 
 		info = ACPI_ALLOCATE_ZEROED(sizeof(struct acpi_evaluate_info));
@@ -575,41 +573,15 @@ acpi_ev_gpe_dispatch(struct acpi_gpe_event_info *gpe_event_info, u32 gpe_number)
 	}
 
 	/*
-	 * Dispatch the GPE to either an installed handler, or the control method
-	 * associated with this GPE (_Lxx or _Exx). If a handler exists, we invoke
-	 * it and do not attempt to run the method. If there is neither a handler
-	 * nor a method, we disable this GPE to prevent further such pointless
-	 * events from firing.
+	 * Dispatch the GPE to either any installed handler or control
+	 * method associated with this GPE (_Lxx or _Exx). We invoke
+	 * the method first in case it has side effects that would be
+	 * interfered with if the handler has already altered hardware
+	 * state. If there is neither a handler nor a method, we
+	 * disable this GPE to prevent further such pointless events
+	 * from firing.
 	 */
-	switch (gpe_event_info->flags & ACPI_GPE_DISPATCH_MASK) {
-	case ACPI_GPE_DISPATCH_HANDLER:
-
-		/*
-		 * Invoke the installed handler (at interrupt level)
-		 * Ignore return status for now.
-		 * TBD: leave GPE disabled on error?
-		 */
-		(void)gpe_event_info->dispatch.handler->address(gpe_event_info->
-								dispatch.
-								handler->
-								context);
-
-		/* It is now safe to clear level-triggered events. */
-
-		if ((gpe_event_info->flags & ACPI_GPE_XRUPT_TYPE_MASK) ==
-		    ACPI_GPE_LEVEL_TRIGGERED) {
-			status = acpi_hw_clear_gpe(gpe_event_info);
-			if (ACPI_FAILURE(status)) {
-				ACPI_EXCEPTION((AE_INFO, status,
-					"Unable to clear GPE[0x%2X]",
-						gpe_number));
-				return_UINT32(ACPI_INTERRUPT_NOT_HANDLED);
-			}
-		}
-		break;
-
-	case ACPI_GPE_DISPATCH_METHOD:
-
+	if (gpe_event_info->flags & ACPI_GPE_DISPATCH_METHOD) {
 		/*
 		 * Disable the GPE, so it doesn't keep firing before the method has a
 		 * chance to run (it runs asynchronously with interrupts enabled).
@@ -634,10 +606,34 @@ acpi_ev_gpe_dispatch(struct acpi_gpe_event_info *gpe_event_info, u32 gpe_number)
 					"Unable to queue handler for GPE[0x%2X] - event disabled",
 					gpe_number));
 		}
-		break;
+	}
 
-	default:
+	if (gpe_event_info->flags & ACPI_GPE_DISPATCH_HANDLER) {
+		/*
+		 * Invoke the installed handler (at interrupt level)
+		 * Ignore return status for now.
+		 * TBD: leave GPE disabled on error?
+		 */
+		(void)gpe_event_info->dispatch.handler->address(gpe_event_info->
+								dispatch.
+								handler->
+								context);
 
+		/* It is now safe to clear level-triggered events. */
+
+		if ((gpe_event_info->flags & ACPI_GPE_XRUPT_TYPE_MASK) ==
+		    ACPI_GPE_LEVEL_TRIGGERED) {
+			status = acpi_hw_clear_gpe(gpe_event_info);
+			if (ACPI_FAILURE(status)) {
+				ACPI_EXCEPTION((AE_INFO, status,
+					"Unable to clear GPE[0x%2X]",
+						gpe_number));
+				return_UINT32(ACPI_INTERRUPT_NOT_HANDLED);
+			}
+		}
+	}
+
+	if (!(gpe_event_info->flags & ACPI_GPE_DISPATCH_MASK)) {
 		/*
 		 * No handler or method to run!
 		 * 03/2010: This case should no longer be possible. We will not allow
@@ -658,7 +654,6 @@ acpi_ev_gpe_dispatch(struct acpi_gpe_event_info *gpe_event_info, u32 gpe_number)
 					gpe_number));
 			return_UINT32(ACPI_INTERRUPT_NOT_HANDLED);
 		}
-		break;
 	}
 
 	return_UINT32(ACPI_INTERRUPT_HANDLED);
diff --git a/drivers/acpi/acpica/evgpeinit.c b/drivers/acpi/acpica/evgpeinit.c
index 3084c5d..4c3687e 100644
--- a/drivers/acpi/acpica/evgpeinit.c
+++ b/drivers/acpi/acpica/evgpeinit.c
@@ -392,16 +392,7 @@ acpi_ev_match_gpe_method(acpi_handle obj_handle,
 		return_ACPI_STATUS(AE_OK);
 	}
 
-	if ((gpe_event_info->flags & ACPI_GPE_DISPATCH_MASK) ==
-	    ACPI_GPE_DISPATCH_HANDLER) {
-
-		/* If there is already a handler, ignore this GPE method */
-
-		return_ACPI_STATUS(AE_OK);
-	}
-
-	if ((gpe_event_info->flags & ACPI_GPE_DISPATCH_MASK) ==
-	    ACPI_GPE_DISPATCH_METHOD) {
+	if (gpe_event_info->flags & ACPI_GPE_DISPATCH_METHOD) {
 		/*
 		 * If there is already a method, ignore this method. But check
 		 * for a type mismatch (if both the _Lxx AND _Exx exist)
diff --git a/drivers/acpi/acpica/evgpeutil.c b/drivers/acpi/acpica/evgpeutil.c
index 19a0e51..434ad1b 100644
--- a/drivers/acpi/acpica/evgpeutil.c
+++ b/drivers/acpi/acpica/evgpeutil.c
@@ -323,12 +323,11 @@ acpi_ev_delete_gpe_handlers(struct acpi_gpe_xrupt_info *gpe_xrupt_info,
 								 ACPI_GPE_REGISTER_WIDTH)
 								+ j];
 
-			if ((gpe_event_info->flags & ACPI_GPE_DISPATCH_MASK) ==
-			    ACPI_GPE_DISPATCH_HANDLER) {
+			if (gpe_event_info->flags & ACPI_GPE_DISPATCH_HANDLER) {
 				ACPI_FREE(gpe_event_info->dispatch.handler);
 				gpe_event_info->dispatch.handler = NULL;
 				gpe_event_info->flags &=
-				    ~ACPI_GPE_DISPATCH_MASK;
+				    ~ACPI_GPE_DISPATCH_HANDLER;
 			}
 		}
 	}
diff --git a/drivers/acpi/acpica/evxface.c b/drivers/acpi/acpica/evxface.c
index 14e48ad..e618e43 100644
--- a/drivers/acpi/acpica/evxface.c
+++ b/drivers/acpi/acpica/evxface.c
@@ -662,6 +662,8 @@ ACPI_EXPORT_SYMBOL(acpi_remove_notify_handler)
  *                                edge- or level-triggered interrupt.
  *              Address         - Address of the handler
  *              Context         - Value passed to the handler on each GPE
+ *		keep_method	- Whether the existing method should be
+ *				  displaced or kept
  *
  * RETURN:      Status
  *
@@ -671,7 +673,8 @@ ACPI_EXPORT_SYMBOL(acpi_remove_notify_handler)
 acpi_status
 acpi_install_gpe_handler(acpi_handle gpe_device,
 			 u32 gpe_number,
-			 u32 type, acpi_event_handler address, void *context)
+			 u32 type, acpi_event_handler address, void *context,
+			 bool keep_method)
 {
 	struct acpi_gpe_event_info *gpe_event_info;
 	struct acpi_handler_info *handler;
@@ -711,8 +714,7 @@ acpi_install_gpe_handler(acpi_handle gpe_device,
 
 	/* Make sure that there isn't a handler there already */
 
-	if ((gpe_event_info->flags & ACPI_GPE_DISPATCH_MASK) ==
-	    ACPI_GPE_DISPATCH_HANDLER) {
+	if (gpe_event_info->flags & ACPI_GPE_DISPATCH_HANDLER) {
 		status = AE_ALREADY_EXISTS;
 		goto free_and_exit;
 	}
@@ -721,7 +723,6 @@ acpi_install_gpe_handler(acpi_handle gpe_device,
 
 	handler->address = address;
 	handler->context = context;
-	handler->method_node = gpe_event_info->dispatch.method_node;
 	handler->orig_flags = gpe_event_info->flags &
 			(ACPI_GPE_XRUPT_TYPE_MASK | ACPI_GPE_DISPATCH_MASK);
 
@@ -733,17 +734,17 @@ acpi_install_gpe_handler(acpi_handle gpe_device,
 	 */
 
 	if ((handler->orig_flags & ACPI_GPE_DISPATCH_METHOD)
-	    && !(gpe_event_info->flags & ACPI_GPE_CAN_WAKE))
+	    && !(gpe_event_info->flags & ACPI_GPE_CAN_WAKE) && !keep_method)
 		(void)acpi_raw_disable_gpe(gpe_event_info);
 
 	/* Install the handler */
 
 	gpe_event_info->dispatch.handler = handler;
 
-	/* Setup up dispatch flags to indicate handler (vs. method) */
+	if (!keep_method)
+		gpe_event_info->flags &=
+			~(ACPI_GPE_XRUPT_TYPE_MASK | ACPI_GPE_DISPATCH_MASK);
 
-	gpe_event_info->flags &=
-	    ~(ACPI_GPE_XRUPT_TYPE_MASK | ACPI_GPE_DISPATCH_MASK);
 	gpe_event_info->flags |= (u8) (type | ACPI_GPE_DISPATCH_HANDLER);
 
 	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
@@ -812,8 +813,7 @@ acpi_remove_gpe_handler(acpi_handle gpe_device,
 
 	/* Make sure that a handler is indeed installed */
 
-	if ((gpe_event_info->flags & ACPI_GPE_DISPATCH_MASK) !=
-	    ACPI_GPE_DISPATCH_HANDLER) {
+	if (!(gpe_event_info->flags & ACPI_GPE_DISPATCH_HANDLER)) {
 		status = AE_NOT_EXIST;
 		goto unlock_and_exit;
 	}
@@ -829,9 +829,8 @@ acpi_remove_gpe_handler(acpi_handle gpe_device,
 
 	handler = gpe_event_info->dispatch.handler;
 
-	/* Restore Method node (if any), set dispatch flags */
+	/* Set dispatch flags */
 
-	gpe_event_info->dispatch.method_node = handler->method_node;
 	gpe_event_info->flags &=
 		~(ACPI_GPE_XRUPT_TYPE_MASK | ACPI_GPE_DISPATCH_MASK);
 	gpe_event_info->flags |= handler->orig_flags;
diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index f31291b..04df824 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -735,7 +735,7 @@ static int ec_install_handlers(struct acpi_ec *ec)
 		return 0;
 	status = acpi_install_gpe_handler(NULL, ec->gpe,
 				  ACPI_GPE_EDGE_TRIGGERED,
-				  &acpi_ec_gpe_handler, ec);
+				  &acpi_ec_gpe_handler, ec, false);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 7bd7c45..5fdaa0d 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -1959,7 +1959,7 @@ static int acpi_gpe_irq_setup(struct smi_info *info)
 					  info->irq,
 					  ACPI_GPE_LEVEL_TRIGGERED,
 					  &ipmi_acpi_gpe,
-					  info);
+					  info, false);
 	if (status != AE_OK) {
 		dev_warn(info->dev, "%s unable to claim ACPI GPE %d,"
 			 " running polled\n", DEVICE_NAME, info->irq);
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index c0786d4..215fbd1 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -253,7 +253,8 @@ acpi_remove_address_space_handler(acpi_handle device,
 acpi_status
 acpi_install_gpe_handler(acpi_handle gpe_device,
 			 u32 gpe_number,
-			 u32 type, acpi_event_handler address, void *context);
+			 u32 type, acpi_event_handler address, void *context,
+			 bool keep_method);
 
 acpi_status
 acpi_remove_gpe_handler(acpi_handle gpe_device,
-- 
1.7.3.1

Re: [PATCH 1/5] ACPI: Allow handlers to be installed at the same time as methods

[Rafael J. Wysocki]

On Monday, October 04, 2010, Matthew Garrett wrote:
> There are circumstances under which it may be desirable for GPE handlers
> to be installable without displacing the existing GPE method. Add support
> for this via a boolean argument to acpi_install_gpe_handler, and fix up the
> existing users to ensure that their behaviour doesn't change.

Hmm.  I'm not sure this is the best way to do that.

In fact, what we need to handle is the case in which a GPE is pointed to by
a _PRW method somewhere and we presume that it's necessary to execute
Notify() for it regardless of whether or not it has a method, right?

So, this GPE will have ACPI_GPE_CAN_WAKE, so can we just put something like

if (gpe_event_info->flags & ACPI_GPE_CAN_WAKE)
         execute Notify()

somewhere around the switch statement in acpi_ev_gpe_dispatch()?

Or perhaps replace acpi_ev_asynch_enable_gpe() with something that will
execute Notify() and then do what the original acpi_ev_asynch_enable_gpe() does?

That should be easier to implement after the changes we've been discussing with
Bob recently (basically, handle both GPE handlers and _Lxx/_Exx analogously).

Rafael

RE: [PATCH 1/5] ACPI: Allow handlers to be installed at the same time as methods

[Moore, Robert]

>In fact, what we need to handle is the case in which a GPE is pointed to by
>a _PRW method somewhere and we presume that it's necessary to execute
>Notify() for it regardless of whether or not it has a method, right?

Is this the "windows compatibility" case where windows claims to implement a "implicit notify" if the GPE method does not exist for a GPE referenced by a _PRW?



>-----Original Message-----
>From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
>Sent: Tuesday, October 05, 2010 4:19 PM
>To: Matthew Garrett
>Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>pci@vger.kernel.org; Moore, Robert
>Subject: Re: [PATCH 1/5] ACPI: Allow handlers to be installed at the same
>time as methods
>
>On Monday, October 04, 2010, Matthew Garrett wrote:
>> There are circumstances under which it may be desirable for GPE handlers
>> to be installable without displacing the existing GPE method. Add support
>> for this via a boolean argument to acpi_install_gpe_handler, and fix up
>the
>> existing users to ensure that their behaviour doesn't change.
>
>Hmm.  I'm not sure this is the best way to do that.
>
>In fact, what we need to handle is the case in which a GPE is pointed to by
>a _PRW method somewhere and we presume that it's necessary to execute
>Notify() for it regardless of whether or not it has a method, right?
>
>So, this GPE will have ACPI_GPE_CAN_WAKE, so can we just put something like
>
>if (gpe_event_info->flags & ACPI_GPE_CAN_WAKE)
>         execute Notify()
>
>somewhere around the switch statement in acpi_ev_gpe_dispatch()?
>
>Or perhaps replace acpi_ev_asynch_enable_gpe() with something that will
>execute Notify() and then do what the original acpi_ev_asynch_enable_gpe()
>does?
>
>That should be easier to implement after the changes we've been discussing
>with
>Bob recently (basically, handle both GPE handlers and _Lxx/_Exx
>analogously).
>
>Rafael

RE: [PATCH 1/5] ACPI: Allow handlers to be installed at the same time as methods

[Moore, Robert]

>In fact, what we need to handle is the case in which a GPE is pointed to by
>a _PRW method somewhere and we presume that it's necessary to execute
>Notify() for it regardless of whether or not it has a method, right?



Although there exists a single Windows document mentions this behavior as a windows "feature", I'm not 100% convinced that this is actually true. AFAIK, we've never seen a machine that depends on an "implicit notify" on a device when a wake GPE happens.

That being said, we should probably think about how to implement this if and when it becomes a requirement.

It appears to me that the biggest issue right now is the fact that a Notify() must be performed on a Device object, and the problem is how to associate the GPE with the device object.

In the past, when ACPICA was executing the _PRW methods during initialization, it would have been simple to detect the condition where a _PRW method referenced a GPE that had no GPE method -- and then make an association of the _PRW device with the GPE for use later to generate an implicit notify.

Now that ACPICA no longer executes the _PRW methods, there are two pieces of information that it no longer has:

1) It does not know which GPEs can be used to wake the system. The new GPE model allows the host to tell ACPICA about these GPEs as they are discovered during the _PRW method execution phase.

2) It has no knowledge of which devices are associated with the wake GPEs.

Perhaps we should add some ability for the host to inform ACPICA of the device object associated with the _PRW (in addition to the actual GPE).

Bob



>-----Original Message-----
>From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
>Sent: Tuesday, October 05, 2010 4:19 PM
>To: Matthew Garrett
>Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>pci@vger.kernel.org; Moore, Robert
>Subject: Re: [PATCH 1/5] ACPI: Allow handlers to be installed at the same
>time as methods
>
>On Monday, October 04, 2010, Matthew Garrett wrote:
>> There are circumstances under which it may be desirable for GPE handlers
>> to be installable without displacing the existing GPE method. Add support
>> for this via a boolean argument to acpi_install_gpe_handler, and fix up
>the
>> existing users to ensure that their behaviour doesn't change.
>
>Hmm.  I'm not sure this is the best way to do that.
>
>In fact, what we need to handle is the case in which a GPE is pointed to by
>a _PRW method somewhere and we presume that it's necessary to execute
>Notify() for it regardless of whether or not it has a method, right?
>
>So, this GPE will have ACPI_GPE_CAN_WAKE, so can we just put something like
>
>if (gpe_event_info->flags & ACPI_GPE_CAN_WAKE)
>         execute Notify()
>
>somewhere around the switch statement in acpi_ev_gpe_dispatch()?
>
>Or perhaps replace acpi_ev_asynch_enable_gpe() with something that will
>execute Notify() and then do what the original acpi_ev_asynch_enable_gpe()
>does?
>
>That should be easier to implement after the changes we've been discussing
>with
>Bob recently (basically, handle both GPE handlers and _Lxx/_Exx
>analogously).
>
>Rafael

Re: [PATCH 1/5] ACPI: Allow handlers to be installed at the same time as methods

[Matthew Garrett]

On Wed, Oct 06, 2010 at 09:14:07AM -0700, Moore, Robert wrote:

> Although there exists a single Windows document mentions this behavior as a windows "feature", I'm not 100% convinced that this is actually true. AFAIK, we've never seen a machine that depends on an "implicit notify" on a device when a wake GPE happens.

Such behaviour would be irrelevant for system sleep/wake - the only 
requirement is in runtime power management.

> It appears to me that the biggest issue right now is the fact that a Notify() must be performed on a Device object, and the problem is how to associate the GPE with the device object.

The other important aspect of this is that a single GPE may correspond 
to multiple devices. The methods will generally cope with this by either 
sending multiple notifies, executing some SMM code to identify the 
relevant device or reading PCI configuration registers to identify the 
source of the wakeup. We need to handle that case as well.

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

Re: [PATCH 1/5] ACPI: Allow handlers to be installed at the same time as methods

[Rafael J. Wysocki]

On Wednesday, October 06, 2010, Matthew Garrett wrote:
> On Wed, Oct 06, 2010 at 09:14:07AM -0700, Moore, Robert wrote:
> 
> > Although there exists a single Windows document mentions this behavior as a windows "feature", I'm not 100% convinced that this is actually true. AFAIK, we've never seen a machine that depends on an "implicit notify" on a device when a wake GPE happens.
> 
> Such behaviour would be irrelevant for system sleep/wake - the only 
> requirement is in runtime power management.
> 
> > It appears to me that the biggest issue right now is the fact that a Notify() must be performed on a Device object, and the problem is how to associate the GPE with the device object.
> 
> The other important aspect of this is that a single GPE may correspond 
> to multiple devices. The methods will generally cope with this by either 
> sending multiple notifies, executing some SMM code to identify the 
> relevant device or reading PCI configuration registers to identify the 
> source of the wakeup. We need to handle that case as well.

So, as I said, we can modify acpi_gpe_can_wake() to pass a device object to
ACPICA.  Then, the device object will be added to the list of devices to Notify()
if the GPE is signaled.  Plain and simple.

Thanks,
Rafael

Re: [PATCH 1/5] ACPI: Allow handlers to be installed at the same time as methods

[Matthew Garrett]

On Wed, Oct 06, 2010 at 09:30:27PM +0200, Rafael J. Wysocki wrote:

> So, as I said, we can modify acpi_gpe_can_wake() to pass a device object to
> ACPICA.  Then, the device object will be added to the list of devices to Notify()
> if the GPE is signaled.  Plain and simple.

Yeah, that does seem to be the simplest solution.

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

RE: [PATCH 1/5] ACPI: Allow handlers to be installed at the same time as methods

[Moore, Robert]

Yes, I would suggest we add a device object to can_wake. This allows the acpica core to handle this "implicit notify" with no additional overhead from the host.



>-----Original Message-----
>From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
>Sent: Wednesday, October 06, 2010 12:30 PM
>To: Matthew Garrett
>Cc: Moore, Robert; linux-acpi@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-pci@vger.kernel.org
>Subject: Re: [PATCH 1/5] ACPI: Allow handlers to be installed at the same
>time as methods
>
>On Wednesday, October 06, 2010, Matthew Garrett wrote:
>> On Wed, Oct 06, 2010 at 09:14:07AM -0700, Moore, Robert wrote:
>>
>> > Although there exists a single Windows document mentions this behavior
>as a windows "feature", I'm not 100% convinced that this is actually true.
>AFAIK, we've never seen a machine that depends on an "implicit notify" on a
>device when a wake GPE happens.
>>
>> Such behaviour would be irrelevant for system sleep/wake - the only
>> requirement is in runtime power management.
>>
>> > It appears to me that the biggest issue right now is the fact that a
>Notify() must be performed on a Device object, and the problem is how to
>associate the GPE with the device object.
>>
>> The other important aspect of this is that a single GPE may correspond
>> to multiple devices. The methods will generally cope with this by either
>> sending multiple notifies, executing some SMM code to identify the
>> relevant device or reading PCI configuration registers to identify the
>> source of the wakeup. We need to handle that case as well.
>
>So, as I said, we can modify acpi_gpe_can_wake() to pass a device object to
>ACPICA.  Then, the device object will be added to the list of devices to
>Notify()
>if the GPE is signaled.  Plain and simple.
>
>Thanks,
>Rafael

[PATCH 2/5] pci: Export some PCI PM functionality

[Matthew Garrett]

It's helpful to have some extra PCI power management functions available to
platform code, so move the declarations to an exported header.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/pci/pci.h   |    3 ---
 include/linux/pci.h |    3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6beb11b..f5c7c38 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -63,11 +63,8 @@ struct pci_platform_pm_ops {
 extern int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
 extern void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 extern void pci_disable_enabled_device(struct pci_dev *dev);
-extern bool pci_check_pme_status(struct pci_dev *dev);
 extern int pci_finish_runtime_suspend(struct pci_dev *dev);
-extern void pci_wakeup_event(struct pci_dev *dev);
 extern int __pci_pme_wakeup(struct pci_dev *dev, void *ign);
-extern void pci_pme_wakeup_bus(struct pci_bus *bus);
 extern void pci_pm_init(struct pci_dev *dev);
 extern void platform_pci_wakeup_init(struct pci_dev *dev);
 extern void pci_allocate_cap_save_buffers(struct pci_dev *dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c8d95e3..af03f47 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -819,6 +819,9 @@ pci_power_t pci_target_state(struct pci_dev *dev);
 int pci_prepare_to_sleep(struct pci_dev *dev);
 int pci_back_from_sleep(struct pci_dev *dev);
 bool pci_dev_run_wake(struct pci_dev *dev);
+bool pci_check_pme_status(struct pci_dev *dev);
+void pci_wakeup_event(struct pci_dev *dev);
+void pci_pme_wakeup_bus(struct pci_bus *bus);
 
 static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 				  bool enable)
-- 
1.7.3.1

Re: [PATCH 2/5] pci: Export some PCI PM functionality

[Rafael J. Wysocki]

On Monday, October 04, 2010, Matthew Garrett wrote:
> It's helpful to have some extra PCI power management functions available to
> platform code, so move the declarations to an exported header.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

> ---
>  drivers/pci/pci.h   |    3 ---
>  include/linux/pci.h |    3 +++
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6beb11b..f5c7c38 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -63,11 +63,8 @@ struct pci_platform_pm_ops {
>  extern int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
>  extern void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
>  extern void pci_disable_enabled_device(struct pci_dev *dev);
> -extern bool pci_check_pme_status(struct pci_dev *dev);
>  extern int pci_finish_runtime_suspend(struct pci_dev *dev);
> -extern void pci_wakeup_event(struct pci_dev *dev);
>  extern int __pci_pme_wakeup(struct pci_dev *dev, void *ign);
> -extern void pci_pme_wakeup_bus(struct pci_bus *bus);
>  extern void pci_pm_init(struct pci_dev *dev);
>  extern void platform_pci_wakeup_init(struct pci_dev *dev);
>  extern void pci_allocate_cap_save_buffers(struct pci_dev *dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c8d95e3..af03f47 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -819,6 +819,9 @@ pci_power_t pci_target_state(struct pci_dev *dev);
>  int pci_prepare_to_sleep(struct pci_dev *dev);
>  int pci_back_from_sleep(struct pci_dev *dev);
>  bool pci_dev_run_wake(struct pci_dev *dev);
> +bool pci_check_pme_status(struct pci_dev *dev);
> +void pci_wakeup_event(struct pci_dev *dev);
> +void pci_pme_wakeup_bus(struct pci_bus *bus);
>  
>  static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
>  				  bool enable)
> 

Re: [PATCH 2/5] pci: Export some PCI PM functionality

[Jesse Barnes]

On Mon,  4 Oct 2010 14:22:26 -0400
Matthew Garrett <mjg@redhat.com> wrote:

> It's helpful to have some extra PCI power management functions available to
> platform code, so move the declarations to an exported header.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  drivers/pci/pci.h   |    3 ---
>  include/linux/pci.h |    3 +++
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6beb11b..f5c7c38 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -63,11 +63,8 @@ struct pci_platform_pm_ops {
>  extern int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
>  extern void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
>  extern void pci_disable_enabled_device(struct pci_dev *dev);
> -extern bool pci_check_pme_status(struct pci_dev *dev);
>  extern int pci_finish_runtime_suspend(struct pci_dev *dev);
> -extern void pci_wakeup_event(struct pci_dev *dev);
>  extern int __pci_pme_wakeup(struct pci_dev *dev, void *ign);
> -extern void pci_pme_wakeup_bus(struct pci_bus *bus);
>  extern void pci_pm_init(struct pci_dev *dev);
>  extern void platform_pci_wakeup_init(struct pci_dev *dev);
>  extern void pci_allocate_cap_save_buffers(struct pci_dev *dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c8d95e3..af03f47 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -819,6 +819,9 @@ pci_power_t pci_target_state(struct pci_dev *dev);
>  int pci_prepare_to_sleep(struct pci_dev *dev);
>  int pci_back_from_sleep(struct pci_dev *dev);
>  bool pci_dev_run_wake(struct pci_dev *dev);
> +bool pci_check_pme_status(struct pci_dev *dev);
> +void pci_wakeup_event(struct pci_dev *dev);
> +void pci_pme_wakeup_bus(struct pci_bus *bus);
>  
>  static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
>  				  bool enable)

Applied, thanks.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 3/5] ACPI: Bind implicit GPE dependencies to PCI devices

[Matthew Garrett]

Several platforms provide GPE information about devices in ACPI, but provide
no ACPI methods to handle these at runtime. Provide a default handler for
this case and bind it to PCI devices as they're discovered in ACPI space.
Keep the methods associated in case they have side effects.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/pci_bind.c |   86 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 86 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/pci_bind.c b/drivers/acpi/pci_bind.c
index 2ef0409..8b3cc6a 100644
--- a/drivers/acpi/pci_bind.c
+++ b/drivers/acpi/pci_bind.c
@@ -28,6 +28,7 @@
 #include <linux/pci.h>
 #include <linux/pci-acpi.h>
 #include <linux/acpi.h>
+#include <linux/list.h>
 #include <linux/pm_runtime.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
@@ -35,6 +36,43 @@
 #define _COMPONENT		ACPI_PCI_COMPONENT
 ACPI_MODULE_NAME("pci_bind");
 
+static LIST_HEAD(acpi_pci_gpe_devs);
+
+struct pci_gpe_dev {
+	struct list_head node;
+	struct pci_dev *dev;
+	acpi_handle gpe_device;
+	int gpe_number;
+	struct work_struct work;
+};
+
+static void acpi_pci_wake_handler_work(struct work_struct *work)
+{
+	struct pci_gpe_dev *gpe_dev = container_of(work, struct pci_gpe_dev,
+						   work);
+
+	pci_check_pme_status(gpe_dev->dev);
+	pm_runtime_resume(&gpe_dev->dev->dev);
+	pci_wakeup_event(gpe_dev->dev);
+	if (gpe_dev->dev->subordinate)
+		pci_pme_wakeup_bus(gpe_dev->dev->subordinate);
+}
+
+static u32 acpi_pci_wake_handler(void *data)
+{
+	long gpe_number = (long) data;
+	struct pci_gpe_dev *gpe_dev;
+
+	list_for_each_entry(gpe_dev, &acpi_pci_gpe_devs, node) {
+		if (gpe_number != gpe_dev->gpe_number)
+			continue;
+
+		schedule_work(&gpe_dev->work);
+	}
+
+	return ACPI_INTERRUPT_HANDLED;
+}
+
 static int acpi_pci_unbind(struct acpi_device *device)
 {
 	struct pci_dev *dev;
@@ -43,6 +81,30 @@ static int acpi_pci_unbind(struct acpi_device *device)
 	if (!dev)
 		goto out;
 
+	if (device->wakeup.flags.valid) {
+		struct pci_gpe_dev *gpe_dev;
+		struct pci_gpe_dev *tmp;
+		int gpe_count = 0;
+		int gpe_number = device->wakeup.gpe_number;
+		acpi_handle gpe_device = device->wakeup.gpe_device;
+
+		list_for_each_entry_safe(gpe_dev, tmp, &acpi_pci_gpe_devs, node) {
+			if (gpe_dev->dev == dev) {
+				flush_work(&gpe_dev->work);
+				list_del(&gpe_dev->node);
+				kfree(gpe_dev);
+			} else if (gpe_dev->gpe_number == gpe_number &&
+				   gpe_dev->gpe_device == gpe_device) {
+				gpe_count++;
+			}
+		}
+
+		if (gpe_count == 0) {
+			acpi_remove_gpe_handler(gpe_device, gpe_number,
+						&acpi_pci_wake_handler);
+		}
+	}
+
 	device_set_run_wake(&dev->dev, false);
 	pci_acpi_remove_pm_notifier(device);
 
@@ -71,6 +133,30 @@ static int acpi_pci_bind(struct acpi_device *device)
 		return 0;
 
 	pci_acpi_add_pm_notifier(device, dev);
+	if (device->wakeup.flags.valid) {
+		struct pci_gpe_dev *gpe_dev;
+		acpi_handle gpe_device = device->wakeup.gpe_device;
+		long gpe_number = device->wakeup.gpe_number;
+
+		gpe_dev = kmalloc(sizeof(struct pci_gpe_dev), GFP_KERNEL);
+		if (gpe_dev) {
+			gpe_dev->dev = dev;
+			gpe_dev->gpe_device = gpe_device;
+			gpe_dev->gpe_number = gpe_number;
+			INIT_WORK(&gpe_dev->work, acpi_pci_wake_handler_work);
+
+			acpi_install_gpe_handler(gpe_device, gpe_number,
+						 ACPI_GPE_LEVEL_TRIGGERED,
+						 &acpi_pci_wake_handler,
+						 (void *)gpe_number,
+						 true);
+			acpi_gpe_can_wake(device->wakeup.gpe_device,
+					  device->wakeup.gpe_number);
+			device->wakeup.flags.run_wake = 1;
+			list_add_tail(&gpe_dev->node, &acpi_pci_gpe_devs);
+		}
+	}
+
 	if (device->wakeup.flags.run_wake)
 		device_set_run_wake(&dev->dev, true);
 
-- 
1.7.3.1

Re: [PATCH 3/5] ACPI: Bind implicit GPE dependencies to PCI devices

[Rafael J. Wysocki]

On Monday, October 04, 2010, Matthew Garrett wrote:
> Several platforms provide GPE information about devices in ACPI, but provide
> no ACPI methods to handle these at runtime. Provide a default handler for
> this case and bind it to PCI devices as they're discovered in ACPI space.
> Keep the methods associated in case they have side effects.

I think I'd prefer it if ACPICA maintained a list of devices to Notify() if
the given GPE triggers.  That would require us to redefine
acpi_gpe_can_wake() a bit, but that should be generally more
efficient.

Thanks,
Rafael

[PATCH 4/5] ACPI: Missing _S0W shouldn't disable runtime PM

[Matthew Garrett]

A failure to evaluate _S0W will effectively result in the suspend state
for PCI devices being set to D0. We should limit that to genuine failures
rather than doing so if the method isn't present.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/sleep.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index cf82989..ab0ba78 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -609,9 +609,9 @@ int acpi_pm_device_sleep_state(struct device *dev, int *d_min_p)
 		acpi_method[3] = 'W';
 		status = acpi_evaluate_integer(handle, acpi_method, NULL,
 						&d_max);
-		if (ACPI_FAILURE(status)) {
+		if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 			d_max = d_min;
-		} else if (d_max < d_min) {
+		} else if (ACPI_SUCCESS(status) && d_max < d_min) {
 			/* Warn the user of the broken DSDT */
 			printk(KERN_WARNING "ACPI: Wrong value from %s\n",
 				acpi_method);
-- 
1.7.3.1

[PATCH 5/5] pci: Add support for polling PME state on suspended legacy PCI devices

[Matthew Garrett]

Not all hardware vendors hook up the PME line for legacy PCI devices,
meaning that wakeup events get lost. The only way around this is to poll
the devices to see if their state has changed, so add support for doing
that on legacy PCI devices that aren't part of the core chipset.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/pci/pci.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7fa3cbd..0b61263 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -38,6 +38,19 @@ EXPORT_SYMBOL(pci_pci_problems);
 
 unsigned int pci_pm_d3_delay;
 
+static void pci_pme_list_scan(struct work_struct *work);
+
+static LIST_HEAD(pci_pme_list);
+static DEFINE_MUTEX(pci_pme_list_mutex);
+static DECLARE_DELAYED_WORK(pci_pme_work, pci_pme_list_scan);
+
+struct pci_pme_device {
+	struct list_head list;
+	struct pci_dev *dev;
+};
+
+#define PME_TIMEOUT 1000 /* How long between PME checks */
+
 static void pci_dev_d3_sleep(struct pci_dev *dev)
 {
 	unsigned int delay = dev->d3_delay;
@@ -1331,6 +1344,32 @@ bool pci_pme_capable(struct pci_dev *dev, pci_power_t state)
 	return !!(dev->pme_support & (1 << state));
 }
 
+static void pci_pme_list_scan(struct work_struct *work)
+{
+	struct pci_pme_device *pme_dev;
+
+	mutex_lock(&pci_pme_list_mutex);
+	if (!list_empty(&pci_pme_list)) {
+		list_for_each_entry(pme_dev, &pci_pme_list, list)
+			pci_pme_wakeup(pme_dev->dev, NULL);
+		schedule_delayed_work(&pci_pme_work, msecs_to_jiffies(PME_TIMEOUT));
+	}
+	mutex_unlock(&pci_pme_list_mutex);
+}
+
+/**
+ * pci_external_pme - is a device an external PCI PME source?
+ * @dev: PCI device to check
+ *
+ */
+
+static bool pci_external_pme(struct pci_dev *dev)
+{
+	if (pci_is_pcie(dev) || dev->bus->number == 0)
+		return false;
+	return true;
+}
+
 /**
  * pci_pme_active - enable or disable PCI device's PME# function
  * @dev: PCI device to handle.
@@ -1354,6 +1393,44 @@ void pci_pme_active(struct pci_dev *dev, bool enable)
 
 	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
 
+	/* PCI (as opposed to PCIe) PME requires that the device have
+	   its PME# line hooked up correctly. Not all hardware vendors
+	   do this, so the PME never gets delivered and the device
+	   remains asleep. The easiest way around this is to
+	   periodically walk the list of suspended devices and check
+	   whether any have their PME flag set. The assumption is that
+	   we'll wake up often enough anyway that this won't be a huge
+	   hit, and the power savings from the devices will still be a
+	   win. */
+
+	if (pci_external_pme(dev)) {
+		struct pci_pme_device *pme_dev;
+		if (enable) {
+			pme_dev = kmalloc(sizeof(struct pci_pme_device),
+					  GFP_KERNEL);
+			if (!pme_dev)
+				goto out;
+			pme_dev->dev = dev;
+			mutex_lock(&pci_pme_list_mutex);
+			list_add(&pme_dev->list, &pci_pme_list);
+			if (list_is_singular(&pci_pme_list))
+				schedule_delayed_work(&pci_pme_work,
+						      msecs_to_jiffies(PME_TIMEOUT));
+			mutex_unlock(&pci_pme_list_mutex);
+		} else {
+			mutex_lock(&pci_pme_list_mutex);
+			list_for_each_entry(pme_dev, &pci_pme_list, list) {
+				if (pme_dev->dev == dev) {
+					list_del(&pme_dev->list);
+					kfree(pme_dev);
+					break;
+				}
+			}
+			mutex_unlock(&pci_pme_list_mutex);
+		}
+	}
+
+out:
 	dev_printk(KERN_DEBUG, &dev->dev, "PME# %s\n",
 			enable ? "enabled" : "disabled");
 }
-- 
1.7.3.1

Re: [PATCH 5/5] pci: Add support for polling PME state on suspended legacy PCI devices

[Rafael J. Wysocki]

On Monday, October 04, 2010, Matthew Garrett wrote:
> Not all hardware vendors hook up the PME line for legacy PCI devices,
> meaning that wakeup events get lost. The only way around this is to poll
> the devices to see if their state has changed, so add support for doing
> that on legacy PCI devices that aren't part of the core chipset.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

> ---
>  drivers/pci/pci.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 77 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7fa3cbd..0b61263 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -38,6 +38,19 @@ EXPORT_SYMBOL(pci_pci_problems);
>  
>  unsigned int pci_pm_d3_delay;
>  
> +static void pci_pme_list_scan(struct work_struct *work);
> +
> +static LIST_HEAD(pci_pme_list);
> +static DEFINE_MUTEX(pci_pme_list_mutex);
> +static DECLARE_DELAYED_WORK(pci_pme_work, pci_pme_list_scan);
> +
> +struct pci_pme_device {
> +	struct list_head list;
> +	struct pci_dev *dev;
> +};
> +
> +#define PME_TIMEOUT 1000 /* How long between PME checks */
> +
>  static void pci_dev_d3_sleep(struct pci_dev *dev)
>  {
>  	unsigned int delay = dev->d3_delay;
> @@ -1331,6 +1344,32 @@ bool pci_pme_capable(struct pci_dev *dev, pci_power_t state)
>  	return !!(dev->pme_support & (1 << state));
>  }
>  
> +static void pci_pme_list_scan(struct work_struct *work)
> +{
> +	struct pci_pme_device *pme_dev;
> +
> +	mutex_lock(&pci_pme_list_mutex);
> +	if (!list_empty(&pci_pme_list)) {
> +		list_for_each_entry(pme_dev, &pci_pme_list, list)
> +			pci_pme_wakeup(pme_dev->dev, NULL);
> +		schedule_delayed_work(&pci_pme_work, msecs_to_jiffies(PME_TIMEOUT));
> +	}
> +	mutex_unlock(&pci_pme_list_mutex);
> +}
> +
> +/**
> + * pci_external_pme - is a device an external PCI PME source?
> + * @dev: PCI device to check
> + *
> + */
> +
> +static bool pci_external_pme(struct pci_dev *dev)
> +{
> +	if (pci_is_pcie(dev) || dev->bus->number == 0)
> +		return false;
> +	return true;
> +}
> +
>  /**
>   * pci_pme_active - enable or disable PCI device's PME# function
>   * @dev: PCI device to handle.
> @@ -1354,6 +1393,44 @@ void pci_pme_active(struct pci_dev *dev, bool enable)
>  
>  	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
>  
> +	/* PCI (as opposed to PCIe) PME requires that the device have
> +	   its PME# line hooked up correctly. Not all hardware vendors
> +	   do this, so the PME never gets delivered and the device
> +	   remains asleep. The easiest way around this is to
> +	   periodically walk the list of suspended devices and check
> +	   whether any have their PME flag set. The assumption is that
> +	   we'll wake up often enough anyway that this won't be a huge
> +	   hit, and the power savings from the devices will still be a
> +	   win. */
> +
> +	if (pci_external_pme(dev)) {
> +		struct pci_pme_device *pme_dev;
> +		if (enable) {
> +			pme_dev = kmalloc(sizeof(struct pci_pme_device),
> +					  GFP_KERNEL);
> +			if (!pme_dev)
> +				goto out;
> +			pme_dev->dev = dev;
> +			mutex_lock(&pci_pme_list_mutex);
> +			list_add(&pme_dev->list, &pci_pme_list);
> +			if (list_is_singular(&pci_pme_list))
> +				schedule_delayed_work(&pci_pme_work,
> +						      msecs_to_jiffies(PME_TIMEOUT));
> +			mutex_unlock(&pci_pme_list_mutex);
> +		} else {
> +			mutex_lock(&pci_pme_list_mutex);
> +			list_for_each_entry(pme_dev, &pci_pme_list, list) {
> +				if (pme_dev->dev == dev) {
> +					list_del(&pme_dev->list);
> +					kfree(pme_dev);
> +					break;
> +				}
> +			}
> +			mutex_unlock(&pci_pme_list_mutex);
> +		}
> +	}
> +
> +out:
>  	dev_printk(KERN_DEBUG, &dev->dev, "PME# %s\n",
>  			enable ? "enabled" : "disabled");
>  }
> 

Re: [PATCH 5/5] pci: Add support for polling PME state on suspended legacy PCI devices

[Jesse Barnes]

On Mon,  4 Oct 2010 14:22:29 -0400
Matthew Garrett <mjg@redhat.com> wrote:

> Not all hardware vendors hook up the PME line for legacy PCI devices,
> meaning that wakeup events get lost. The only way around this is to poll
> the devices to see if their state has changed, so add support for doing
> that on legacy PCI devices that aren't part of the core chipset.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  drivers/pci/pci.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 77 insertions(+), 0 deletions(-)

Yuck, polling.  But I guess we don't have an alternative short of
rewiring all the cheap platforms out there!

So, applied.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 5/5] pci: Add support for polling PME state on suspended legacy PCI devices

[Rafael J. Wysocki]

On Friday, October 15, 2010, Jesse Barnes wrote:
> On Mon,  4 Oct 2010 14:22:29 -0400
> Matthew Garrett <mjg@redhat.com> wrote:
> 
> > Not all hardware vendors hook up the PME line for legacy PCI devices,
> > meaning that wakeup events get lost. The only way around this is to poll
> > the devices to see if their state has changed, so add support for doing
> > that on legacy PCI devices that aren't part of the core chipset.
> > 
> > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> > ---
> >  drivers/pci/pci.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 77 insertions(+), 0 deletions(-)
> 
> Yuck, polling.  But I guess we don't have an alternative short of
> rewiring all the cheap platforms out there!
> 
> So, applied.

Actually, the next step should be to avoid polling for the devices that
do signal wakeup normally.  That is, if wakeup interrupts or notifications are
coming for the device, we'll drop it from the list.  This looks doable.

Thanks,
Rafael