[RFC 0/5] device wakeup event support

[RFC 0/5] device wakeup event support

[shaohua.li]

This series add device wakeup event detection support. This is the base to
implement runtime device suspend/resume, though we don't support it now.
But David said USB is approaching to this. See this bugzilla
http://bugzilla.kernel.org/show_bug.cgi?id=6892 for detail.

The current process to handle wakeup event is:
1. driver enable wakeup event line pme and suspend
2. NPME or ACPI receives wakeup event
3. NPME or ACPI call .wakeup_event() to clear and disable wakeup event. Driver can do extra things in .wakeup_event() too.
4. NPME or ACPI call generic wakeup event handler (device_receive_wakeup_event())
5. device resumes, and goto 1 for next round of suspend

There are somethings we need discuss:
1. is this generic for other platforms?
2. what should the generic wakeup event handler do?

Comments and suggestions are welcome!

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
-- 

[RFC 1/5] devcore introduce wakeup_event callback

[shaohua.li]

[-- Attachment #1: devcore-introduce-wakeup_event.patch --]
[-- Type: text/plain, Size: 1143 bytes --]

Introduce .wakeup_event(). When a device gets a wakeup event,
the callback is called. The callback usually should disable wakeup event.

---
 include/linux/pm.h |    3 +++
 1 file changed, 3 insertions(+)

Index: linux/include/linux/pm.h
===================================================================
--- linux.orig/include/linux/pm.h	2008-09-08 13:55:57.000000000 +0800
+++ linux/include/linux/pm.h	2008-09-08 13:56:51.000000000 +0800
@@ -125,6 +125,8 @@ typedef struct pm_message {
  *	make ANY assumptions about the hardware state right prior to @restore().
  *	On most platforms, there are no restrictions on availability of
  *	resources like clocks during @restore().
+ * @wakeup_event: Checks if a wakeup event occurs. If yes, wakeup event should
+ *      be disabled.
  *
  * All of the above callbacks, except for @complete(), return error codes.
  * However, the error codes returned by the resume operations, @resume(),
@@ -151,6 +153,7 @@ struct pm_ops {
 	int (*thaw)(struct device *dev);
 	int (*poweroff)(struct device *dev);
 	int (*restore)(struct device *dev);
+	int (*wakeup_event)(struct device *dev);
 };
 
 /**

-- 

Re: [RFC 1/5] devcore introduce wakeup_event callback

[David Brownell]

On Monday 08 September 2008, shaohua.li@intel.com wrote:
> --- linux.orig/include/linux/pm.h       2008-09-08 13:55:57.000000000 +0800
> +++ linux/include/linux/pm.h    2008-09-08 13:56:51.000000000 +0800
> @@ -125,6 +125,8 @@ typedef struct pm_message {
>   *     make ANY assumptions about the hardware state right prior to @restore().
>   *     On most platforms, there are no restrictions on availability of
>   *     resources like clocks during @restore().
> + * @wakeup_event: Checks if a wakeup event occurs. If yes, wakeup event should
> + *      be disabled.

And ... what else??  What does the return value indicate?
Should anything be done with it other than printing it out
if it's nonzero and we're debugging?


>   *
>   * All of the above callbacks, except for @complete(), return error codes.
>   * However, the error codes returned by the resume operations, @resume(),
> @@ -151,6 +153,7 @@ struct pm_ops {
>         int (*thaw)(struct device *dev);
>         int (*poweroff)(struct device *dev);
>         int (*restore)(struct device *dev);
> +       int (*wakeup_event)(struct device *dev);

My reaction to adding this method is:  why do it here rather
than at the bus level?

In my particular experience there are two basic types of wakeup
event:

  - Regular IRQs.  Common on SOC systems; the IRQ comes in,
    the driver knows it must wake up.  Does not need any
    driver model hook; enable_irq_wake()/disable_irq_wake()
    suffice.

    I've seen USB remote wakeup working just fine on several
    different embedded Linuxes using only regular IRQ hooks.

  - Side-band signaling.  Think of this as an IRQ mechanism
    that's not used for "normal" driver operation, and you
    won't be far off.  Examples:

     * PCI PME# (and its PCIE analogue).  Bus-specific; it's
       pretty much a kind of shared IRQ line coupled with a
       special config-space register protocol.

     * ACPI GPEs.  Bus-specific ... and similar to GPIO IRQs.
       Also sharable; bytecode is used to map the GPE and
       some register state to the ACPI device(s) which
       issued that GPE.

     * Pin-change events.  Not quite isomorphic with (GPIO)
       IRQs; sometimes used with pins that aren't used for
       events (or even GPIOs!) during normal operation.
       Device-specific.

That is, I don't see why these events should expect to be
filtered through the driver core.  If there's a reason to
do that, please enlighten me!

- Dave

p.s. Related to this, I don't see the point behind
     patch 2/5 ...


>  };
>  
>  /**


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

RE: [RFC 1/5] devcore introduce wakeup_event callback

[Li, Shaohua]

>-----Original Message-----
>From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>owner@vger.kernel.org] On Behalf Of David Brownell
>Sent: Tuesday, September 09, 2008 10:56 AM
>To: Li, Shaohua
>Cc: linux-pm@lists.linux-foundation.org; linux-acpi@vger.kernel.org;
>stern@rowland.harvard.edu
>Subject: Re: [RFC 1/5] devcore introduce wakeup_event callback
>
>On Monday 08 September 2008, shaohua.li@intel.com wrote:
>> --- linux.orig/include/linux/pm.h       2008-09-08 13:55:57.000000000
>+0800
>> +++ linux/include/linux/pm.h    2008-09-08 13:56:51.000000000 +0800
>> @@ -125,6 +125,8 @@ typedef struct pm_message {
>>   *     make ANY assumptions about the hardware state right prior to
>@restore().
>>   *     On most platforms, there are no restrictions on availability of
>>   *     resources like clocks during @restore().
>> + * @wakeup_event: Checks if a wakeup event occurs. If yes, wakeup event
>should
>> + *      be disabled.
>
>And ... what else??  What does the return value indicate?
>Should anything be done with it other than printing it out
>if it's nonzero and we're debugging?
Return 0 if the device invokes a wakeup event. In this case, driver should clear/disable wakeup event.
Return < 0 if device didn't invoke wakeup.

If device follows standard wakeup mechanism which bus level can handle, driver isn't required to provide this callback.

>>   *
>>   * All of the above callbacks, except for @complete(), return error
>codes.
>>   * However, the error codes returned by the resume operations, @resume(),
>> @@ -151,6 +153,7 @@ struct pm_ops {
>>         int (*thaw)(struct device *dev);
>>         int (*poweroff)(struct device *dev);
>>         int (*restore)(struct device *dev);
>> +       int (*wakeup_event)(struct device *dev);
>
>My reaction to adding this method is:  why do it here rather
>than at the bus level?
>
>In my particular experience there are two basic types of wakeup
>event:
>
>  - Regular IRQs.  Common on SOC systems; the IRQ comes in,
>    the driver knows it must wake up.  Does not need any
>    driver model hook; enable_irq_wake()/disable_irq_wake()
>    suffice.
>
>    I've seen USB remote wakeup working just fine on several
>    different embedded Linuxes using only regular IRQ hooks.
Ok, in such case, driver can just ignore the callback.

>  - Side-band signaling.  Think of this as an IRQ mechanism
>    that's not used for "normal" driver operation, and you
>    won't be far off.  Examples:
>
>     * PCI PME# (and its PCIE analogue).  Bus-specific; it's
>       pretty much a kind of shared IRQ line coupled with a
>       special config-space register protocol.
Right, in this case, device doesn't require .wakeup_event too.
>
>     * ACPI GPEs.  Bus-specific ... and similar to GPIO IRQs.
>       Also sharable; bytecode is used to map the GPE and
>       some register state to the ACPI device(s) which
>       issued that GPE.
This isn't bus specific. ACPI devices can map to any physical devices, like PCI, IDE drive, PNP device. In this case, a bus specific mechanism can't handle all.
For example the UHCI case. A GPE is fired, we need to clear/disable the wakeup event. PCI bus can't handle it, as UHCI has special registers for this, so we need call into device specific handling.

>     * Pin-change events.  Not quite isomorphic with (GPIO)
>       IRQs; sometimes used with pins that aren't used for
>       events (or even GPIOs!) during normal operation.
>       Device-specific.
>
>That is, I don't see why these events should expect to be
>filtered through the driver core.  If there's a reason to
>do that, please enlighten me!
The UHCI case.

Thanks,
Shaohua

Re: [RFC 1/5] devcore introduce wakeup_event callback

[David Brownell]

On Monday 08 September 2008, Li, Shaohua wrote:
> 
> >> + * @wakeup_event: Checks if a wakeup event occurs.
> >> + *    If yes, wakeup event should be disabled.
> >
> >And ... what else??  What does the return value indicate?
> >Should anything be done with it other than printing it out
> >if it's nonzero and we're debugging?
>
> Return 0 if the device invokes a wakeup event. In this case, driver 
> should clear/disable wakeup event. 
> Return < 0 if device didn't invoke wakeup.

So it's effectively "bool"?  I'd declare it as "bool" then...
and rename it to sound like a predicate; "is_wakedev()" maybe.


> If device follows standard wakeup mechanism which bus level can
> handle, driver isn't required to provide this callback.

There seems to be a semi-hidden distinction between bus-level
code and device-level code.  Somewhat apparent in later code;
but not at all clear in this initial set-the-stage patch.


> >> @@ -151,6 +153,7 @@ struct pm_ops {
> >>         int (*thaw)(struct device *dev);
> >>         int (*poweroff)(struct device *dev);
> >>         int (*restore)(struct device *dev);
> >> +       int (*wakeup_event)(struct device *dev);
> >
> >My reaction to adding this method is:  why do it here rather
> >than at the bus level?

Your answer probably should have been:  "The recent method
reshuffling makes that method work both at the level of the
'struct bus_type' and at the level of 'struct device'.  So
this _does_ add it at the bus level."

:)


> >In my particular experience there are two basic types of wakeup
> >event:
> >
> > 	...
> >
> >     * ACPI GPEs.  Bus-specific ... and similar to GPIO IRQs.
> >       Also sharable; bytecode is used to map the GPE and
> >       some register state to the ACPI device(s) which
> >       issued that GPE.
>
> This isn't bus specific.

It's bus-specific in the sense of "ACPI bus" (/sys/bus/acpi).
If the DSDT defines a GPE and maps it to an ACPI device, it will
follow those rules.

Devices not on the "ACPI bus" -- like add-on PCI cards, USB
peripherals, and other devices which have some such ACPI device
higher in the driver model tre -- don't use GPEs to wake up.
(An ancestral device may well do so:  a PCI bridge, a USB
host controller, etc.)


> ACPI devices can map to any physical devices, like PCI, IDE
> drive, PNP device. In this case, a bus specific mechanism can't
> handle all.

Sure it can.  And it'd be more natural to do it that way too.
I'd be tempted to call them from ACPI like

	struct device *dev;
	acpi_handle adev;

	/* adev received a wake event notification */

	...
	dev = acpi_get_physical_device(adev);
	if (dev) {
		/* this chunk of code should probably live in
		 * drivers/base/power/... somewhere and be
		 * part of this patch 1/5 so it's more clear
		 * what the infrastructure is doing.
		 */
		if (... && dev->bus->pm.base->is_wakedev(dev)) {
			... call dev->driver->pm.resume(dev)
			... else if resume is null, issue diagnostic
		} else {
			... report suitable generic confusion ...
		}
	} else {
		... report ACPI-specific confusion ...
	}
	...

When that's a PCI device -- bridge or otherwise -- that
would call something resembling your patch #3 (but fixed
to handle the case of PCI bridges, so add-in cards will
work properly).

And for a non-ACPI system, whatever IRQ handler receives the
PME# signal would just call that chunk of code for the PCI
root bridge(s) it handles.  All done.  :)


> For example the UHCI case. A GPE is fired, we need to clear/disable
> the wakeup event. PCI bus can't handle it, as UHCI has special
> registers for this, so we need call into device specific handling.  

So the bus-level PCI operation would end up with a device
that's either (a) known to have fired PME#, or (b) known
to be a device with legacy PM -- like UHCI -- which it's
presuming has been wakeup-enabled.

Then in the (a) case it can clear PME# the normal way, and
would rarely need to do anything else.

While in the (b) case the UHCI driver would need to have
provided some device-specific -- not bus-generic -- op
called like dev->driver->pm.is_wakeup(), to diddle those
registers (only for Intel silicon, not UHCI from VIA or
Genesys or anyone else).


I think I'm getting a bit more clear on what you're trying
to do with this.  Restructure it a bit and I will be able
to see that from reading the code (as will others).  :)

- Dave

RE: [RFC 1/5] devcore introduce wakeup_event callback

[Li, Shaohua]

Ok, you comments on pci bridge makes sense. I missed that case, will change the code to support it.

>On Monday 08 September 2008, Li, Shaohua wrote:
>>
>> >> + * @wakeup_event: Checks if a wakeup event occurs.
>> >> + *    If yes, wakeup event should be disabled.
>> >
>> >And ... what else??  What does the return value indicate?
>> >Should anything be done with it other than printing it out
>> >if it's nonzero and we're debugging?
>>
>> Return 0 if the device invokes a wakeup event. In this case, driver
>> should clear/disable wakeup event.
>> Return < 0 if device didn't invoke wakeup.
>
>So it's effectively "bool"?  I'd declare it as "bool" then...
>and rename it to sound like a predicate; "is_wakedev()" maybe.
Right.

>> >> @@ -151,6 +153,7 @@ struct pm_ops {
>> >>         int (*thaw)(struct device *dev);
>> >>         int (*poweroff)(struct device *dev);
>> >>         int (*restore)(struct device *dev);
>> >> +       int (*wakeup_event)(struct device *dev);
>> >
>> >My reaction to adding this method is:  why do it here rather
>> >than at the bus level?
>
>Your answer probably should have been:  "The recent method
>reshuffling makes that method work both at the level of the
>'struct bus_type' and at the level of 'struct device'.  So
>this _does_ add it at the bus level."
Right.


>> >In my particular experience there are two basic types of wakeup
>> >event:
>> >
>> >     ...
>> >
>> >     * ACPI GPEs.  Bus-specific ... and similar to GPIO IRQs.
>> >       Also sharable; bytecode is used to map the GPE and
>> >       some register state to the ACPI device(s) which
>> >       issued that GPE.
>>
>> This isn't bus specific.
>
>It's bus-specific in the sense of "ACPI bus" (/sys/bus/acpi).
>If the DSDT defines a GPE and maps it to an ACPI device, it will
>follow those rules.
>
>Devices not on the "ACPI bus" -- like add-on PCI cards, USB
>peripherals, and other devices which have some such ACPI device
>higher in the driver model tre -- don't use GPEs to wake up.
>(An ancestral device may well do so:  a PCI bridge, a USB
>host controller, etc.)
>
>
>> ACPI devices can map to any physical devices, like PCI, IDE
>> drive, PNP device. In this case, a bus specific mechanism can't
>> handle all.
>
>Sure it can.  And it'd be more natural to do it that way too.
>I'd be tempted to call them from ACPI like
>
>        struct device *dev;
>        acpi_handle adev;
>
>        /* adev received a wake event notification */
>
>        ...
>        dev = acpi_get_physical_device(adev);
>        if (dev) {
>                /* this chunk of code should probably live in
>                 * drivers/base/power/... somewhere and be
>                 * part of this patch 1/5 so it's more clear
>                 * what the infrastructure is doing.
>                 */
>                if (... && dev->bus->pm.base->is_wakedev(dev)) {
>                        ... call dev->driver->pm.resume(dev)
>                        ... else if resume is null, issue diagnostic
>                } else {
>                        ... report suitable generic confusion ...
>                }
>        } else {
>                ... report ACPI-specific confusion ...
>        }
>        ...
>
>When that's a PCI device -- bridge or otherwise -- that
>would call something resembling your patch #3 (but fixed
>to handle the case of PCI bridges, so add-in cards will
>work properly).
>
>And for a non-ACPI system, whatever IRQ handler receives the
>PME# signal would just call that chunk of code for the PCI
>root bridge(s) it handles.  All done.  :)
I can move my ' device_receive_wakeup_event()' into ' pci_pm_wakeup_event()', and export the routine. Then IRQ handler can call it. Assume this is what you want.

>> For example the UHCI case. A GPE is fired, we need to clear/disable
>> the wakeup event. PCI bus can't handle it, as UHCI has special
>> registers for this, so we need call into device specific handling.
>
>So the bus-level PCI operation would end up with a device
>that's either (a) known to have fired PME#, or (b) known
>to be a device with legacy PM -- like UHCI -- which it's
>presuming has been wakeup-enabled.
>
>Then in the (a) case it can clear PME# the normal way, and
>would rarely need to do anything else.
>
>While in the (b) case the UHCI driver would need to have
>provided some device-specific -- not bus-generic -- op
>called like dev->driver->pm.is_wakeup(), to diddle those
>registers (only for Intel silicon, not UHCI from VIA or
>Genesys or anyone else).
>
>
>I think I'm getting a bit more clear on what you're trying
>to do with this.  Restructure it a bit and I will be able
>to see that from reading the code (as will others).  :)
So we are on the same page here both bus and device requires a op like .is_wakeup(). And what I need to do is to address the pci bridge case.

Thanks,
Shaohua

Re: [RFC 1/5] devcore introduce wakeup_event callback

[Rafael J. Wysocki]

On Tuesday, 9 of September 2008, Li, Shaohua wrote:
> Ok, you comments on pci bridge makes sense. I missed that case, will change the code to support it.
> 
> >On Monday 08 September 2008, Li, Shaohua wrote:
> >>
> >> >> + * @wakeup_event: Checks if a wakeup event occurs.
> >> >> + *    If yes, wakeup event should be disabled.
> >> >
> >> >And ... what else??  What does the return value indicate?
> >> >Should anything be done with it other than printing it out
> >> >if it's nonzero and we're debugging?
> >>
> >> Return 0 if the device invokes a wakeup event. In this case, driver
> >> should clear/disable wakeup event.
> >> Return < 0 if device didn't invoke wakeup.
> >
> >So it's effectively "bool"?  I'd declare it as "bool" then...
> >and rename it to sound like a predicate; "is_wakedev()" maybe.
> Right.
> 
> >> >> @@ -151,6 +153,7 @@ struct pm_ops {
> >> >>         int (*thaw)(struct device *dev);
> >> >>         int (*poweroff)(struct device *dev);
> >> >>         int (*restore)(struct device *dev);
> >> >> +       int (*wakeup_event)(struct device *dev);
> >> >
> >> >My reaction to adding this method is:  why do it here rather
> >> >than at the bus level?
> >
> >Your answer probably should have been:  "The recent method
> >reshuffling makes that method work both at the level of the
> >'struct bus_type' and at the level of 'struct device'.  So
> >this _does_ add it at the bus level."
> Right.

Still, I'm not sure if we need the method at the device core level at all.

The suspend/resume callbacks are needed at the device core level, because the
_core_ initiates the operation (suspend, resume etc.), but in this case the
operation will be initiated in a core-independent way (for example, by an
interrupt handler in case of the PCI Express native PME).

Surely, we'll need such a callback on the PCI core level that may have to
invoke the device driver to handle wake-up after identifying the device that
caused the event to happen.

> >> >In my particular experience there are two basic types of wakeup
> >> >event:
> >> >
> >> >     ...
> >> >
> >> >     * ACPI GPEs.  Bus-specific ... and similar to GPIO IRQs.
> >> >       Also sharable; bytecode is used to map the GPE and
> >> >       some register state to the ACPI device(s) which
> >> >       issued that GPE.
> >>
> >> This isn't bus specific.
> >
> >It's bus-specific in the sense of "ACPI bus" (/sys/bus/acpi).
> >If the DSDT defines a GPE and maps it to an ACPI device, it will
> >follow those rules.
> >
> >Devices not on the "ACPI bus" -- like add-on PCI cards, USB
> >peripherals, and other devices which have some such ACPI device
> >higher in the driver model tre -- don't use GPEs to wake up.
> >(An ancestral device may well do so:  a PCI bridge, a USB
> >host controller, etc.)

Well, I'm not exactly sure how PME# is supposed to be handled for add-on cards.
My understanding so far has been that all of the PME# signals are supposed to
be line ORed and routed (around all of the bridges) to the host bridge that
should generate an interrupt of some sort (eg. ACPI SCI).  Isn't that correct?

> >> ACPI devices can map to any physical devices, like PCI, IDE
> >> drive, PNP device. In this case, a bus specific mechanism can't
> >> handle all.
> >
> >Sure it can.  And it'd be more natural to do it that way too.
> >I'd be tempted to call them from ACPI like
> >
> >        struct device *dev;
> >        acpi_handle adev;
> >
> >        /* adev received a wake event notification */
> >
> >        ...
> >        dev = acpi_get_physical_device(adev);
> >        if (dev) {
> >                /* this chunk of code should probably live in
> >                 * drivers/base/power/... somewhere and be
> >                 * part of this patch 1/5 so it's more clear
> >                 * what the infrastructure is doing.
> >                 */
> >                if (... && dev->bus->pm.base->is_wakedev(dev)) {
> >                        ... call dev->driver->pm.resume(dev)
> >                        ... else if resume is null, issue diagnostic
> >                } else {
> >                        ... report suitable generic confusion ...
> >                }
> >        } else {
> >                ... report ACPI-specific confusion ...
> >        }
> >        ...
> >
> >When that's a PCI device -- bridge or otherwise -- that
> >would call something resembling your patch #3 (but fixed
> >to handle the case of PCI bridges, so add-in cards will
> >work properly).
> >
> >And for a non-ACPI system, whatever IRQ handler receives the
> >PME# signal would just call that chunk of code for the PCI
> >root bridge(s) it handles.  All done.  :)
> I can move my ' device_receive_wakeup_event()' into ' pci_pm_wakeup_event()',
> and export the routine. Then IRQ handler can call it. Assume this is what you
> want.  

Would pci_pm_wakeup_event() be responsible for invoking a device driver's
callback in that case?

Thanks,
Rafael

Re: [RFC 1/5] devcore introduce wakeup_event callback

[Alan Stern]

On Tue, 9 Sep 2008, Rafael J. Wysocki wrote:

> Still, I'm not sure if we need the method at the device core level at all.
> 
> The suspend/resume callbacks are needed at the device core level, because the
> _core_ initiates the operation (suspend, resume etc.), but in this case the
> operation will be initiated in a core-independent way (for example, by an
> interrupt handler in case of the PCI Express native PME).

Not only the wakeup (resume) operation, but also the original suspend 
operation.

> Surely, we'll need such a callback on the PCI core level that may have to
> invoke the device driver to handle wake-up after identifying the device that
> caused the event to happen.

Absolutely.  This could involve calling the normal PCI resume method or 
it could involve a new method.  Presumably it will mirror whatever 
means we choose for implementing runtime suspend (e.g., a pair of new 
methods).

Alan Stern

Re: [RFC 1/5] devcore introduce wakeup_event callback

[Alan Stern]

On Mon, 8 Sep 2008, David Brownell wrote:

> > For example the UHCI case. A GPE is fired, we need to clear/disable
> > the wakeup event. PCI bus can't handle it, as UHCI has special
> > registers for this, so we need call into device specific handling.  
> 
> So the bus-level PCI operation would end up with a device
> that's either (a) known to have fired PME#, or (b) known
> to be a device with legacy PM -- like UHCI -- which it's
> presuming has been wakeup-enabled.
> 
> Then in the (a) case it can clear PME# the normal way, and
> would rarely need to do anything else.
> 
> While in the (b) case the UHCI driver would need to have
> provided some device-specific -- not bus-generic -- op
> called like dev->driver->pm.is_wakeup(), to diddle those
> registers (only for Intel silicon, not UHCI from VIA or
> Genesys or anyone else).

What about case (c): The device uses legacy PM but the wakeup settings 
have already been handled by the ACPI AML code?  There's no need to 
clear any extra wakeup-related stuff, but it still is necessary to tell 
the driver about the event.

(As a matter of fact, I have no idea whether or not Intel's legacy PM 
for UHCI is implemented in AML.  It may vary from one BIOS to another.)

Alan Stern

Re: [RFC 1/5] devcore introduce wakeup_event callback

[David Brownell]

On Tuesday 09 September 2008, Alan Stern wrote:
> What about case (c): The device uses legacy PM but the wakeup settings 
> have already been handled by the ACPI AML code?  There's no need to 
> clear any extra wakeup-related stuff, but it still is necessary to tell 
> the driver about the event.

Good question ... best if Intel answers it.  :0


> (As a matter of fact, I have no idea whether or not Intel's legacy PM 
> for UHCI is implemented in AML.  It may vary from one BIOS to another.)

One data point:  on the laptop used to type this message,
the GPE code block includes stuff like this for each UHCI:

        Method (_L0C, 0, NotSerialized)
        {
            Notify (\_SB.PCI0.USB3, 0x02)
        }

Without diving into the ICHx specs (which I believe DO have
such details, thanks be!), my first reaction is that this is
not a "case (c)".  Of course, the rest of the AML code is,
as usual, cryptic (I'd rather have C code), and such stuff
might be hidden elsewhere in the event sequence.

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

Re: [RFC 1/5] devcore introduce wakeup_event callback

[Alan Stern]

On Tue, 9 Sep 2008, David Brownell wrote:

> > (As a matter of fact, I have no idea whether or not Intel's legacy PM 
> > for UHCI is implemented in AML.  It may vary from one BIOS to another.)
> 
> One data point:  on the laptop used to type this message,
> the GPE code block includes stuff like this for each UHCI:
> 
>         Method (_L0C, 0, NotSerialized)
>         {
>             Notify (\_SB.PCI0.USB3, 0x02)
>         }
> 
> Without diving into the ICHx specs (which I believe DO have
> such details, thanks be!), my first reaction is that this is
> not a "case (c)".  Of course, the rest of the AML code is,
> as usual, cryptic (I'd rather have C code), and such stuff
> might be hidden elsewhere in the event sequence.

Fortunately the interface is extremely simple.  It's an 8-bit register 
in the PCI config space, where the two low-order bits enable 
wakeup detection for each of the two ports.  It won't matter if the 
driver and the AML both try to disable wakeup; the end result should 
work regardless.

Anyway, the real problem here is not to enable or disable wakeup
detection.  It is to determine whether or not the device really did
issue a wakeup request.  With Intel's UHCI this involves reading an I/O
register.  On all the systems I have tried it works even in D3hot, so
we should be okay.

Alan Stern

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

[RFC 2/5] devcore adds generic wakeup event handler

[shaohua.li]

[-- Attachment #1: devcore-introduce-default-pme-action.patch --]
[-- Type: text/plain, Size: 1289 bytes --]

The default action to handle wakeup event. Currently just prints something,
maybe we should call .resume(). The routine will be called in task context.

---
 drivers/base/power/main.c |    6 ++++++
 include/linux/pm.h        |    2 ++
 2 files changed, 8 insertions(+)

Index: linux/drivers/base/power/main.c
===================================================================
--- linux.orig/drivers/base/power/main.c	2008-09-08 13:55:56.000000000 +0800
+++ linux/drivers/base/power/main.c	2008-09-08 14:22:12.000000000 +0800
@@ -785,3 +785,9 @@ void __suspend_report_result(const char 
 	}
 }
 EXPORT_SYMBOL_GPL(__suspend_report_result);
+
+void device_receive_wakeup_event(struct device *dev)
+{
+	printk("Device %s invokes wakeup event\n", dev->bus_id);
+}
+EXPORT_SYMBOL(device_receive_wakeup_event);
Index: linux/include/linux/pm.h
===================================================================
--- linux.orig/include/linux/pm.h	2008-09-08 13:56:51.000000000 +0800
+++ linux/include/linux/pm.h	2008-09-08 14:22:12.000000000 +0800
@@ -436,6 +436,8 @@ static inline int device_suspend(pm_mess
 
 #endif /* !CONFIG_PM_SLEEP */
 
+void device_receive_wakeup_event(struct device *dev);
+
 /*
  * Global Power Management flags
  * Used to keep APM and ACPI from both being active

-- 

[RFC 3/5] pci wakeup handler

[shaohua.li]

[-- Attachment #1: pci-wakeup-event.patch --]
[-- Type: text/plain, Size: 2250 bytes --]

pci subsystem wakeup handler.
---
 drivers/pci/pci-driver.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

Index: linux/drivers/pci/pci-driver.c
===================================================================
--- linux.orig/drivers/pci/pci-driver.c	2008-09-08 13:55:56.000000000 +0800
+++ linux/drivers/pci/pci-driver.c	2008-09-08 14:24:42.000000000 +0800
@@ -472,12 +472,57 @@ static int pci_pm_resume_noirq(struct de
 	return error;
 }
 
+/*
+ * Called when dev is suspected to invoke a wakeup event, return 0 if yes
+ * */
+static int pci_pm_wakeup_event(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	int pme_pos = pci_find_capability(pdev, PCI_CAP_ID_PM);
+	struct pci_driver *drv = pdev->driver;
+	u16 reg16;
+	int spurious = 0;
+	int ret = -ENODEV;
+
+	if (pme_pos == 0) {
+		/*
+		 * Some USB devices haven't PME, but have specific registers to
+		 * control wakeup
+		 */
+		goto out;
+	}
+
+	/* clear PME status and disable PME to avoid interrupt flood */
+	pci_read_config_word(pdev, pme_pos + PCI_PM_CTRL, &reg16);
+	if (!(reg16 & PCI_PM_CTRL_PME_STATUS))
+		return -ENODEV;
+	/* I see spurious GPE here, just ignore it for now */
+	if (!(reg16 & PCI_PM_CTRL_PME_ENABLE))
+		spurious = 1;
+	reg16 &= ~PCI_PM_CTRL_PME_ENABLE;
+	reg16 |= PCI_PM_CTRL_PME_STATUS;
+	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, reg16);
+
+	if (spurious)
+		return -ENODEV;
+	ret = 0;
+	/* This device invokes PME, gives driver a chance to do something */
+out:
+	if (drv && drv->pm && drv->pm->base.wakeup_event) {
+		if (!ret) /* ignore return value in this case */
+			drv->pm->base.wakeup_event(&pdev->dev);
+		else
+			return drv->pm->base.wakeup_event(&pdev->dev);
+	}
+	return ret;
+}
 #else /* !CONFIG_SUSPEND */
 
 #define pci_pm_suspend		NULL
 #define pci_pm_suspend_noirq	NULL
 #define pci_pm_resume		NULL
 #define pci_pm_resume_noirq	NULL
+#define pci_pm_wakeup_event	NULL
 
 #endif /* !CONFIG_SUSPEND */
 
@@ -651,6 +696,7 @@ struct pm_ext_ops pci_pm_ops = {
 		.thaw = pci_pm_thaw,
 		.poweroff = pci_pm_poweroff,
 		.restore = pci_pm_restore,
+		.wakeup_event = pci_pm_wakeup_event,
 	},
 	.suspend_noirq = pci_pm_suspend_noirq,
 	.resume_noirq = pci_pm_resume_noirq,

-- 

Re: [RFC 3/5] pci wakeup handler

[Rafael J. Wysocki]

On Monday, 8 of September 2008, shaohua.li@intel.com wrote:
> pci subsystem wakeup handler.
> ---
>  drivers/pci/pci-driver.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> Index: linux/drivers/pci/pci-driver.c
> ===================================================================
> --- linux.orig/drivers/pci/pci-driver.c	2008-09-08 13:55:56.000000000 +0800
> +++ linux/drivers/pci/pci-driver.c	2008-09-08 14:24:42.000000000 +0800
> @@ -472,12 +472,57 @@ static int pci_pm_resume_noirq(struct de
>  	return error;
>  }
>  
> +/*
> + * Called when dev is suspected to invoke a wakeup event, return 0 if yes
> + * */
> +static int pci_pm_wakeup_event(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int pme_pos = pci_find_capability(pdev, PCI_CAP_ID_PM);

Pleae use dev->pm_cap instead.

> +	struct pci_driver *drv = pdev->driver;
> +	u16 reg16;
> +	int spurious = 0;

Please use 'bool' for boolean variables.

> +	int ret = -ENODEV;
> +
> +	if (pme_pos == 0) {
> +		/*
> +		 * Some USB devices haven't PME, but have specific registers to
> +		 * control wakeup
> +		 */
> +		goto out;
> +	}
> +
> +	/* clear PME status and disable PME to avoid interrupt flood */
> +	pci_read_config_word(pdev, pme_pos + PCI_PM_CTRL, &reg16);

The variable used for that is called 'pmcsr' in the other functions.  Perhaps
call it 'pmcsr' instead of 'reg16' here too?

> +	if (!(reg16 & PCI_PM_CTRL_PME_STATUS))
> +		return -ENODEV;
> +	/* I see spurious GPE here, just ignore it for now */
> +	if (!(reg16 & PCI_PM_CTRL_PME_ENABLE))
> +		spurious = 1;
> +	reg16 &= ~PCI_PM_CTRL_PME_ENABLE;
> +	reg16 |= PCI_PM_CTRL_PME_STATUS;
> +	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, reg16);

I think you can use pci_pm_active() for clearing PME# and status.  That'll
read the register once more, but that shouldn't be a problem.  Actually, you
can do:

	pci_read_config_word(pdev, pme_pos + PCI_PM_CTRL, &pmcsr);
	if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
		/* No event (why have we been called, actually? */
		return -EINVAL;
	pci_pm_active(dev, false);
	if (!(pmcsr & PCI_PM_CTRL_PME_ENABLE))
		/* Spurious event */
		return -ENODEV;
> +
> +	if (spurious)
> +		return -ENODEV;
> +	ret = 0;
> +	/* This device invokes PME, gives driver a chance to do something */
> +out:
> +	if (drv && drv->pm && drv->pm->base.wakeup_event) {
> +		if (!ret) /* ignore return value in this case */
> +			drv->pm->base.wakeup_event(&pdev->dev);
> +		else
> +			return drv->pm->base.wakeup_event(&pdev->dev);

Hm, I'd do:

	if (drv && drv->pm && drv->pm->base.wakeup_event) {
		int dev_ret = drv->pm->base.wakeup_event(&pdev->dev);
		if (ret)
			ret = dev_ret;
	}

Also, why do you think we should ignore the returned value if ret is zero?

> +	}
> +	return ret;
> +}
>  #else /* !CONFIG_SUSPEND */
>  
>  #define pci_pm_suspend		NULL
>  #define pci_pm_suspend_noirq	NULL
>  #define pci_pm_resume		NULL
>  #define pci_pm_resume_noirq	NULL
> +#define pci_pm_wakeup_event	NULL
>  
>  #endif /* !CONFIG_SUSPEND */
>  
> @@ -651,6 +696,7 @@ struct pm_ext_ops pci_pm_ops = {
>  		.thaw = pci_pm_thaw,
>  		.poweroff = pci_pm_poweroff,
>  		.restore = pci_pm_restore,
> +		.wakeup_event = pci_pm_wakeup_event,
>  	},
>  	.suspend_noirq = pci_pm_suspend_noirq,
>  	.resume_noirq = pci_pm_resume_noirq,
> 

Thanks,
Rafael

RE: [RFC 3/5] pci wakeup handler

[Li, Shaohua]

>-----Original Message-----
>From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
>Sent: Monday, September 08, 2008 9:10 PM
>To: Li, Shaohua
>Cc: linux-pm@lists.linux-foundation.org; linux-acpi@vger.kernel.org;
>stern@rowland.harvard.edu; dbrownell@users.sourceforge.net; Jesse Barnes
>Subject: Re: [RFC 3/5] pci wakeup handler
>
>On Monday, 8 of September 2008, shaohua.li@intel.com wrote:
>> pci subsystem wakeup handler.
>> ---
>>  drivers/pci/pci-driver.c |   46
>++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> Index: linux/drivers/pci/pci-driver.c
>> ===================================================================
>> --- linux.orig/drivers/pci/pci-driver.c       2008-09-08
>13:55:56.000000000 +0800
>> +++ linux/drivers/pci/pci-driver.c    2008-09-08 14:24:42.000000000 +0800
>> @@ -472,12 +472,57 @@ static int pci_pm_resume_noirq(struct de
>>       return error;
>>  }
>>
>> +/*
>> + * Called when dev is suspected to invoke a wakeup event, return 0 if
>yes
>> + * */
>> +static int pci_pm_wakeup_event(struct device *dev)
>> +{
>> +     struct pci_dev *pdev = to_pci_dev(dev);
>> +     int pme_pos = pci_find_capability(pdev, PCI_CAP_ID_PM);
>
>Pleae use dev->pm_cap instead.
Ok, I'll address your other comments too.

>        if (drv && drv->pm && drv->pm->base.wakeup_event) {
>                int dev_ret = drv->pm->base.wakeup_event(&pdev->dev);
>                if (ret)
>                        ret = dev_ret;
>        }
>
>Also, why do you think we should ignore the returned value if ret is zero?
Because we already identified the device which invokes PME. Even the .wakeup_event() returns an error, we should populate It's this device which has wakeup event. In my mind, driver isn't required to provide .wakeup_event() unless device has non-standard regs for wakeup event or some special to handle. Generic PME handling should be fine for most devices (for PCI devices).

Thanks,
Shaohua

Re: [RFC 3/5] pci wakeup handler

[David Brownell]

On Monday 08 September 2008, Li, Shaohua wrote:
> >        if (drv && drv->pm && drv->pm->base.wakeup_event) {
> >                int dev_ret = drv->pm->base.wakeup_event(&pdev->dev);
> >                if (ret)
> >                        ret = dev_ret;
> >        }
> >
> > Also, why do you think we should ignore the returned value if ret is zero?
> 
> Because we already identified the device which invokes PME.

Well, we *may* have identified *one* of them.  If the device
supports the PCI PM standards, we did.  If it uses legacy PM
(like Intel's UHCI controllers), that's not guaranteed ...


> Even the .wakeup_event() returns an error, we should populate
> It's this device which has wakeup event.

You haven't exactly defined the semantics of the return value,
or what a caller would do with it.  


> In my mind, driver isn't 
> required to provide .wakeup_event() unless device has non-standard
> regs for wakeup event or some special to handle. Generic PME handling
> should be fine for most devices (for PCI devices).      

I don't follow this then.  You're saying it will suffice to
just clear the PCI PM status, and the driver is expected to
work fine without even being notified about its wake event??

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

RE: [RFC 3/5] pci wakeup handler

[Li, Shaohua]

>-----Original Message-----
>From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>owner@vger.kernel.org] On Behalf Of David Brownell
>Sent: Tuesday, September 09, 2008 10:57 AM
>To: Li, Shaohua
>Cc: Rafael J. Wysocki; linux-pm@lists.linux-foundation.org; linux-
>acpi@vger.kernel.org; stern@rowland.harvard.edu; Jesse Barnes
>Subject: Re: [RFC 3/5] pci wakeup handler
>
>On Monday 08 September 2008, Li, Shaohua wrote:
>> >        if (drv && drv->pm && drv->pm->base.wakeup_event) {
>> >                int dev_ret = drv->pm->base.wakeup_event(&pdev->dev);
>> >                if (ret)
>> >                        ret = dev_ret;
>> >        }
>> >
>> > Also, why do you think we should ignore the returned value if ret is
>zero?
>>
>> Because we already identified the device which invokes PME.
>
>Well, we *may* have identified *one* of them.  If the device
>supports the PCI PM standards, we did.  If it uses legacy PM
>(like Intel's UHCI controllers), that's not guaranteed ...
Right, so the patch always 0 if device follows PM standards. Otherwise, return .wakeup_event().

>> Even the .wakeup_event() returns an error, we should populate
>> It's this device which has wakeup event.
>
>You haven't exactly defined the semantics of the return value,
>or what a caller would do with it.
I'll comment this on another mail

>> In my mind, driver isn't
>> required to provide .wakeup_event() unless device has non-standard
>> regs for wakeup event or some special to handle. Generic PME handling
>> should be fine for most devices (for PCI devices).
>
>I don't follow this then.  You're saying it will suffice to
>just clear the PCI PM status, and the driver is expected to
>work fine without even being notified about its wake event??
NPME or ACPI will call ' device_receive_wakeup_event()', which will do anything you want.

Thanks,
Shaohua

Re: [RFC 3/5] pci wakeup handler

[David Brownell]

On Monday 08 September 2008, shaohua.li@intel.com wrote:
> --- linux.orig/drivers/pci/pci-driver.c	2008-09-08 13:55:56.000000000 +0800
> +++ linux/drivers/pci/pci-driver.c	2008-09-08 14:24:42.000000000 +0800
> @@ -472,12 +472,57 @@ static int pci_pm_resume_noirq(struct de
>  	return error;
>  }
>  
> +/*
> + * Called when dev is suspected to invoke a wakeup event, return 0 if yes
> + * */
> +static int pci_pm_wakeup_event(struct device *dev)

If wakeup notifications were bus-specific this would
take "struct pci_dev *pdev" ... and we're missing a
key part of this stuff, namely the code to sort out
which devices get this call.

I think you're assuming ACPI can just use its event
notification scheme to map from GPE through AML to
the particular device.  That's partly OK; except, see
below about bridge nodes.


> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int pme_pos = pci_find_capability(pdev, PCI_CAP_ID_PM);

Yes, use the cached value here (like Rafael said)...


> +	struct pci_driver *drv = pdev->driver;
> +	u16 reg16;
> +	int spurious = 0;
> +	int ret = -ENODEV;
> +
> +	if (pme_pos == 0) {
> +		/*
> +		 * Some USB devices haven't PME, but have specific registers to
> +		 * control wakeup

The PCI PM spec has words some like:  "Some PCI devices support legacy
wakeup mechanisms instead of supporting PCI PM capabilities."  Today
the best example of that is Intel's UHCI controllers, but it's not
restricted to USB at all.

I suspect that this particular path won't often need to handle anything
other than those Intel controllers, however.  And so I hope that they
never share GPEs.  ;)

Another rather important case is bridges.  I observe that with ACPI
the way PME# is handled for add-in cards _seems_ to be that the bridge
for that PCI bus segment gets a GPE (presumably matching the PME#
signal for that bus segment, and maybe for its subsidiaries).  That
suggests the bridge will need to scan its children to find out which
one(s) issued PME#.  You didn't include such code here, where that
notification will be received...


> +		 */
> +		goto out;
> +	}
> +
> +	/* clear PME status and disable PME to avoid interrupt flood */
> +	pci_read_config_word(pdev, pme_pos + PCI_PM_CTRL, &reg16);
> +	if (!(reg16 & PCI_PM_CTRL_PME_STATUS))
> +		return -ENODEV;

Whitespace between paragraphs please ...

> +	/* I see spurious GPE here, just ignore it for now */

Comments about GPEs shouldn't be included here; they're ACPI-specific.
This is PCI-generic code!

But if you're going by GPEs, remember that several different
PCI devices could be hanging off the same GPE, with only one
of them issuing a wake event.  Maybe the "spurious" bit is an
artifact of an AML bug, issuing extra device notifications.


> +	if (!(reg16 & PCI_PM_CTRL_PME_ENABLE))
> +		spurious = 1;
> +	reg16 &= ~PCI_PM_CTRL_PME_ENABLE;
> +	reg16 |= PCI_PM_CTRL_PME_STATUS;
> +	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, reg16);

If this device didn't issue PME, then don't clear PME_ENABLE.

By doing that you'd be preventing runtime power management
from working as effectively as it could.  (Example, several
PCI devices could enable PME and enter D3hot to shrink their
power demands while the system as a whole is in some G0/active
state rather than a G1/sleeping ACPI state.  If one issues a
wake event, that shouldn't disable PME on any others...


> +
> +	if (spurious)
> +		return -ENODEV;
> +	ret = 0;
> +	/* This device invokes PME, gives driver a chance to do something */
> +out:
> +	if (drv && drv->pm && drv->pm->base.wakeup_event) {
> +		if (!ret) /* ignore return value in this case */
> +			drv->pm->base.wakeup_event(&pdev->dev);
> +		else
> +			return drv->pm->base.wakeup_event(&pdev->dev);

I'm also puzzled why you want the legacy PM case (UHCI etc)
to work differently from the "normal" one.

And style-wise I'd really prefer to see

	return pdev->driver->wakeup(pdev);

Or perhaps more generically -- since I still have yet to
hear an argument why resume() shouldn't suffice to handle
the wakeup event processing, at least for PCI -- just

	if (pdev->driver->wakeup)
		return pdev->driver->wakeup(pdev);
	if (pdev->driver->resume)
		return pdev->driver->resume(pdev);

Although I don't know what the return value here should
be interpreted to mean.  Would it be better to return
void, and just log all "interesting"/error cases?


> +	}
> +	return ret;
> +}
>  #else /* !CONFIG_SUSPEND */
>  
>  #define pci_pm_suspend		NULL
>  #define pci_pm_suspend_noirq	NULL
>  #define pci_pm_resume		NULL
>  #define pci_pm_resume_noirq	NULL
> +#define pci_pm_wakeup_event	NULL
>  
>  #endif /* !CONFIG_SUSPEND */
>  
> @@ -651,6 +696,7 @@ struct pm_ext_ops pci_pm_ops = {
>  		.thaw = pci_pm_thaw,
>  		.poweroff = pci_pm_poweroff,
>  		.restore = pci_pm_restore,
> +		.wakeup_event = pci_pm_wakeup_event,
>  	},
>  	.suspend_noirq = pci_pm_suspend_noirq,
>  	.resume_noirq = pci_pm_resume_noirq,
> 
> -- 
> 
> 

RE: [RFC 3/5] pci wakeup handler

[Li, Shaohua]

>-----Original Message-----
>From: David Brownell [mailto:david-b@pacbell.net]
>Sent: Tuesday, September 09, 2008 10:57 AM
>To: Li, Shaohua
>Cc: linux-pm@lists.linux-foundation.org; linux-acpi@vger.kernel.org;
>stern@rowland.harvard.edu
>Subject: Re: [RFC 3/5] pci wakeup handler
>
>On Monday 08 September 2008, shaohua.li@intel.com wrote:
>> --- linux.orig/drivers/pci/pci-driver.c       2008-09-08
>13:55:56.000000000 +0800
>> +++ linux/drivers/pci/pci-driver.c    2008-09-08 14:24:42.000000000 +0800
>> @@ -472,12 +472,57 @@ static int pci_pm_resume_noirq(struct de
>>       return error;
>>  }
>>
>> +/*
>> + * Called when dev is suspected to invoke a wakeup event, return 0 if
>yes
>> + * */
>> +static int pci_pm_wakeup_event(struct device *dev)
>
>If wakeup notifications were bus-specific this would
>take "struct pci_dev *pdev" ... and we're missing a
>key part of this stuff, namely the code to sort out
>which devices get this call.
This device is suspected to invoke a wakeup event, might not be true. I suppose other bus requires the same mechanism, so should be a 'struct device'

>> +     struct pci_driver *drv = pdev->driver;
>> +     u16 reg16;
>> +     int spurious = 0;
>> +     int ret = -ENODEV;
>> +
>> +     if (pme_pos == 0) {
>> +             /*
>> +              * Some USB devices haven't PME, but have specific
>registers to
>> +              * control wakeup
>
>The PCI PM spec has words some like:  "Some PCI devices support legacy
>wakeup mechanisms instead of supporting PCI PM capabilities."  Today
>the best example of that is Intel's UHCI controllers, but it's not
>restricted to USB at all.
>
>I suspect that this particular path won't often need to handle anything
>other than those Intel controllers, however.  And so I hope that they
>never share GPEs.  ;)
This is one of the reason calling drv->wakeup_event().

>Another rather important case is bridges.  I observe that with ACPI
>the way PME# is handled for add-in cards _seems_ to be that the bridge
>for that PCI bus segment gets a GPE (presumably matching the PME#
>signal for that bus segment, and maybe for its subsidiaries).  That
>suggests the bridge will need to scan its children to find out which
>one(s) issued PME#.  You didn't include such code here, where that
>notification will be received...
In GPE case, it appears BIOS will detect the exact wakeup device.

In native PME case, if a device is a pcie device, npme will detect the exact device too. If the device is a legacy device, then npme driver will check devices under bridges, please see the npme_pme_target().

We can directly scan children in pci_pm_wakeup_event() too, but GPE case doesn't require it and actually is broken in GPE case as duplication will be added.

I thought this covers all cases in IA platform, right?
>
>> +     /* I see spurious GPE here, just ignore it for now */
>
>Comments about GPEs shouldn't be included here; they're ACPI-specific.
>This is PCI-generic code!
A typo, it should be PME. And actually I found this in npme case instead of ACPI.

>> +     if (!(reg16 & PCI_PM_CTRL_PME_ENABLE))
>> +             spurious = 1;
>> +     reg16 &= ~PCI_PM_CTRL_PME_ENABLE;
>> +     reg16 |= PCI_PM_CTRL_PME_STATUS;
>> +     pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, reg16);
>
>If this device didn't issue PME, then don't clear PME_ENABLE.
Ok, this is reasonable.


>> +     if (drv && drv->pm && drv->pm->base.wakeup_event) {
>> +             if (!ret) /* ignore return value in this case */
>> +                     drv->pm->base.wakeup_event(&pdev->dev);
>> +             else
>> +                     return drv->pm->base.wakeup_event(&pdev->dev);
>
>I'm also puzzled why you want the legacy PM case (UHCI etc)
>to work differently from the "normal" one.
>
>And style-wise I'd really prefer to see
>
>        return pdev->driver->wakeup(pdev);
>
>Or perhaps more generically -- since I still have yet to
>hear an argument why resume() shouldn't suffice to handle
>the wakeup event processing, at least for PCI -- just
>
>        if (pdev->driver->wakeup)
>                return pdev->driver->wakeup(pdev);
>        if (pdev->driver->resume)
>                return pdev->driver->resume(pdev);
>
>Although I don't know what the return value here should
>be interpreted to mean.  Would it be better to return
>void, and just log all "interesting"/error cases?
In my mind, .wakeup_event() just returns if the device invokes a wakeup event, ACPI or NPME will call corresponding .resume method. Suspected device might not invoke wakeup event as you said the bridge case.

Thanks,
Shaohua

Re: [RFC 3/5] pci wakeup handler

[David Brownell]

On Monday 08 September 2008, Li, Shaohua wrote:
> >> +static int pci_pm_wakeup_event(struct device *dev)
> >
> >If wakeup notifications were bus-specific this would
> >take "struct pci_dev *pdev" ... and we're missing a
> >key part of this stuff, namely the code to sort out
> >which devices get this call.
> 
> This device is suspected to invoke a wakeup event, might
> not be true. I suppose other bus requires the same
> mechanism, so should be a 'struct device'.

But *THIS* call is PCI-specific, and will need to be
called from PCI bridge code which knows that it only
has to cope with PCI devices as sources of the PCI
specific wake events.  Nothing generic there...

But I'd rather see that addressed in the context of
my comments to your patch [1/5].



> >Another rather important case is bridges.  I observe that with ACPI
> >the way PME# is handled for add-in cards _seems_ to be that the bridge
> >for that PCI bus segment gets a GPE (presumably matching the PME#
> >signal for that bus segment, and maybe for its subsidiaries).  That
> >suggests the bridge will need to scan its children to find out which
> >one(s) issued PME#.  You didn't include such code here, where that
> >notification will be received...
>
> In GPE case, it appears BIOS will detect the exact wakeup device.

The laptop on which I'm typing this response has two GPEs which
are shared by several devices each.  (Not PCI devices though.)

And ... read what I wrote more closely.  BIOS doesn't know anything
about add-in cards, just mainboard devices.  If I have a board with
six PCI slots, ACPI delivers a notification to the bridgesince that
is the only mainboard device.  The bridge then has to sort out which
add-in card issued the wakeup event.


> In native PME case, if a device is a pcie device, npme will
> detect the exact device too.  If the device is a legacy device,
> then npme driver will check devices under bridges, please see
> the npme_pme_target().

This file relates to PCI not PCIE; I referred to PCI bridges
not PCIE ones.  Are you saying the PCIE code is what I have
to look at to see PCI bridge support?  Extremely confusing if
that's the case!  Not to mention wrong ... since I have several
systems here that have PCI support but not PCIE.  Linux will
be supporting such things for a LONG time to come.


> We can directly scan children in pci_pm_wakeup_event() too,
> but GPE case doesn't require it and actually is broken in GPE
> case as duplication will be added.  
> 
> I thought this covers all cases in IA platform, right?

Not the case I introduced above:  PCI bridges, where there's
no PCIE in the house.  I've not looked at other cases.


> >> +     /* I see spurious GPE here, just ignore it for now */
> >
> >Comments about GPEs shouldn't be included here; they're ACPI-specific.
> >This is PCI-generic code!
>
> A typo, it should be PME. And actually I found this in npme case instead of ACPI.

OK, I guess.  But in that case are you sure it was really
spurious?  Rather than just not being able to tell which
device issued the PME until you checked the PME status bit?

It'd really be routine to get a PME# event and then need
to scan several devices to find which one raised it.  Not
all those devices would have enabled PME# either...


> >Or perhaps more generically -- since I still have yet to
> >hear an argument why resume() shouldn't suffice to handle
> >the wakeup event processing, at least for PCI -- just
> >
> >        if (pdev->driver->wakeup)
> >                return pdev->driver->wakeup(pdev);
> >        if (pdev->driver->resume)
> >                return pdev->driver->resume(pdev);
> >
> >Although I don't know what the return value here should
> >be interpreted to mean.  Would it be better to return
> >void, and just log all "interesting"/error cases?
> 
> In my mind, .wakeup_event() just returns if the device
> invokes a wakeup event, ACPI or NPME will call corresponding
> .resume method. Suspected device might not invoke wakeup
> event as you said the bridge case.

I think I see some of what's going on here.  This routine
is getting more attention than I think it deserves, because
you have placed what I'd call a PCI-internal utility -- to
find which devices have issued PME# signals -- into a driver
model method rather than hiding it in the internals of code
that dispatches PME# notifications (or some ACPI GPEs).

I thought you were providing a set of patches grouped by
functionality:  PCI, PCIE, ACPI.  Evidently not...

PCI support for PME# would have an entry for use by an IRQ
handler (on most non-ACPI hardware) or an ACPI GPE ... and
everything else should be internal to that bus, except for
the driver notification callback (which I'm happy to think
is just the existing bus-specific resume method).

You've almost made my case that there shouldn't be such a
hook in the driver model PM core.  :)

- Dave

Re: [RFC 3/5] pci wakeup handler

[Rafael J. Wysocki]

On Tuesday, 9 of September 2008, David Brownell wrote:
> On Monday 08 September 2008, shaohua.li@intel.com wrote:
> > --- linux.orig/drivers/pci/pci-driver.c	2008-09-08 13:55:56.000000000 +0800
> > +++ linux/drivers/pci/pci-driver.c	2008-09-08 14:24:42.000000000 +0800
> > @@ -472,12 +472,57 @@ static int pci_pm_resume_noirq(struct de
> >  	return error;
> >  }
> >  
> > +/*
> > + * Called when dev is suspected to invoke a wakeup event, return 0 if yes
> > + * */
> > +static int pci_pm_wakeup_event(struct device *dev)
> 
> If wakeup notifications were bus-specific this would
> take "struct pci_dev *pdev" ... and we're missing a
> key part of this stuff, namely the code to sort out
> which devices get this call.
> 
> I think you're assuming ACPI can just use its event
> notification scheme to map from GPE through AML to
> the particular device.  That's partly OK; except, see
> below about bridge nodes.
> 
> 
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	int pme_pos = pci_find_capability(pdev, PCI_CAP_ID_PM);
> 
> Yes, use the cached value here (like Rafael said)...
> 
> 
> > +	struct pci_driver *drv = pdev->driver;
> > +	u16 reg16;
> > +	int spurious = 0;
> > +	int ret = -ENODEV;
> > +
> > +	if (pme_pos == 0) {
> > +		/*
> > +		 * Some USB devices haven't PME, but have specific registers to
> > +		 * control wakeup
> 
> The PCI PM spec has words some like:  "Some PCI devices support legacy
> wakeup mechanisms instead of supporting PCI PM capabilities."  Today
> the best example of that is Intel's UHCI controllers, but it's not
> restricted to USB at all.
> 
> I suspect that this particular path won't often need to handle anything
> other than those Intel controllers, however.  And so I hope that they
> never share GPEs.  ;)
> 
> Another rather important case is bridges.  I observe that with ACPI
> the way PME# is handled for add-in cards _seems_ to be that the bridge
> for that PCI bus segment gets a GPE (presumably matching the PME#
> signal for that bus segment, and maybe for its subsidiaries).  That
> suggests the bridge will need to scan its children to find out which
> one(s) issued PME#.  You didn't include such code here, where that
> notification will be received...

AFAICS, the 'original' PCI PME# (as opposed to the PCI Express native PME)
signal is supposed to be routed _around_ bridges and (presumably) the entire
PME# network would share one GPE in that case.  Then, we'd actually have to
check all of the PCI devices to see which of them caused the event to happen.

Thanks,
Rafael

Re: [RFC 3/5] pci wakeup handler

[David Brownell]

On Tuesday 09 September 2008, Rafael J. Wysocki wrote:
> > 
> > Another rather important case is bridges.  I observe that with ACPI
> > the way PME# is handled for add-in cards _seems_ to be that the bridge
> > for that PCI bus segment gets a GPE (presumably matching the PME#
> > signal for that bus segment, and maybe for its subsidiaries).  That
> > suggests the bridge will need to scan its children to find out which
> > one(s) issued PME#.  You didn't include such code here, where that
> > notification will be received...
> 
> AFAICS, the 'original' PCI PME# (as opposed to the PCI Express native PME)
> signal is supposed to be routed _around_ bridges and (presumably) the entire
> PME# network would share one GPE in that case.

To the extent that board designers are expected to follow
such rules, but have the power not to do so, we know that
relying on such expectations would be ... unwise.  ;)

Isn't the notion of a GPE pretty much an x86-ism?
For non-x86 systems I'd just think "IRQ".  And for
x86, I regularly wonder if maybe chaining through
genirq wouldn't make SCI interrupts, including GPEs,
become a lot more robust.  :(

I suspect folk maintaining non-ACPI platforms with PCI
wiill have to help sort out this particular issue.  I
suspect their board designs will provide at least one
PME# indication.  ISTR multiple PME# domains were OK,
at least in the "one per root bridge" sense, but I've
not looked at the relevant spec recently.


> Then, we'd actually have to 
> check all of the PCI devices to see which of them caused the event to happen.

That emphasizes my point:  that patch #3 needs to include code
to handle bridges, scanning wakeup-eligible devices until it
finds the relevant one(s).

Of course there should be a few ways to speed that scanning:

 - First, bridges shouldn't need to scan devices for which PME#
   was not enabled.  Make pci_enable_wake() record that state
   for use in scanning ... look at that handful of devices first.
   (And possibly the rest later, in case of errors.)

 - Second, ACPI-specific, is to recognize that the mainboard
   devices all seem to be associated with private GPEs so they
   can get direct wake event notifications.  So when the PCI
   device getting this notification/probe isn't a bridge, it
   won't need to scan other devices.

EVERY system I have here seems to have PCI devices, including
ones in add-in slots, which will require that scanning ...
because they don't show up in the ACPI tables (second case).

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

[RFC 4/5] PCIe native PME detection

[shaohua.li]

[-- Attachment #1: pcie-native-pme.patch --]
[-- Type: text/plain, Size: 10438 bytes --]

This implements PCIe native PME detection.

---
 drivers/pci/pcie/Kconfig  |    7 
 drivers/pci/pcie/Makefile |    2 
 drivers/pci/pcie/npme.c   |  341 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci_regs.h  |    1 
 4 files changed, 351 insertions(+)

Index: linux/drivers/pci/pcie/Kconfig
===================================================================
--- linux.orig/drivers/pci/pcie/Kconfig	2008-09-08 13:55:55.000000000 +0800
+++ linux/drivers/pci/pcie/Kconfig	2008-09-08 14:26:13.000000000 +0800
@@ -46,3 +46,10 @@ config PCIEASPM_DEBUG
 	help
 	  This enables PCI Express ASPM debug support. It will add per-device
 	  interface to control ASPM.
+
+config PCIENPME
+	bool "PCIE Native PME support(Experimental)"
+	depends on PCIEPORTBUS && EXPERIMENTAL
+	help
+	  This enables PCI Express Native PME Reporting.
+
Index: linux/drivers/pci/pcie/Makefile
===================================================================
--- linux.orig/drivers/pci/pcie/Makefile	2008-09-08 13:55:55.000000000 +0800
+++ linux/drivers/pci/pcie/Makefile	2008-09-08 14:26:13.000000000 +0800
@@ -11,3 +11,5 @@ obj-$(CONFIG_PCIEPORTBUS)	+= pcieportdrv
 
 # Build PCI Express AER if needed
 obj-$(CONFIG_PCIEAER)		+= aer/
+
+obj-$(CONFIG_PCIENPME) += npme.o
Index: linux/drivers/pci/pcie/npme.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux/drivers/pci/pcie/npme.c	2008-09-08 14:26:13.000000000 +0800
@@ -0,0 +1,341 @@
+/*
+ * PCIE Native PME support
+ *
+ * Copyright (C) 2007 - 2008 Intel Corp
+ *  Shaohua Li <shaohua.li@intel.com>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/pcieport_if.h>
+#include <linux/acpi.h>
+#include <linux/pci-acpi.h>
+
+static int disabled;
+module_param(disabled, bool, 0);
+static int force = 1;
+module_param(force, bool, 0);
+
+static struct pcie_port_service_id npme_id[] = {
+	{
+	.vendor 	= PCI_ANY_ID,
+	.device 	= PCI_ANY_ID,
+	.port_type 	= PCIE_RC_PORT,
+	.service_type 	= PCIE_PORT_SERVICE_PME,
+	},
+	{ /* end: all zeroes */ }
+};
+
+struct npme_data {
+	spinlock_t lock;
+	struct pcie_device *dev;
+	struct work_struct work;
+	u16 bdf; /* device which invokes PME */
+	int exit;
+};
+
+static inline void npme_set_pme(struct pci_dev *pdev, int enable)
+{
+	int pos;
+	u16 reg16;
+
+	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
+
+	pci_read_config_word(pdev, pos + PCI_EXP_RTCTL, &reg16);
+	if (!enable)
+		reg16 &= ~PCI_EXP_RTCTL_PMEIE;
+	else
+		reg16 |= PCI_EXP_RTCTL_PMEIE;
+	pci_write_config_word(pdev, pos + PCI_EXP_RTCTL, reg16);
+}
+
+static inline void npme_clear_pme(struct pci_dev *pdev)
+{
+	int pos;
+	u32 reg32;
+
+	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
+
+	pci_read_config_dword(pdev, pos + PCI_EXP_RTSTA, &reg32);
+	reg32 |= PCI_EXP_RTSTA_PME;
+	pci_write_config_dword(pdev, pos + PCI_EXP_RTSTA, reg32);
+}
+
+static int npme_pme_target_one(struct pci_dev *target)
+{
+	int ret = -ENODEV;
+	if (target->dev.bus->pm && target->dev.bus->pm->base.wakeup_event)
+		ret = target->dev.bus->pm->base.wakeup_event(&target->dev);
+	if (!ret)
+		device_receive_wakeup_event(&target->dev);
+	return ret;
+}
+
+static int npme_pme_target(struct pci_dev *target)
+{
+	int ret;
+	struct pci_dev *tmp = NULL;
+	int domain_nr, bus_start, bus_end;
+
+	ret = npme_pme_target_one(target);
+	/*
+	 * Because PCIe-PCI bridge or pci root port can take ownership, legacy
+	 * devices under the bridges might share a BDF. The BDF can be root
+	 * port's BDF or (PCIe-PCI bridge's secondary bus number, 0, 0)
+	 */
+	if (!target->subordinate || !target->is_pcie ||
+		(target->pcie_type != PCI_EXP_TYPE_ROOT_PORT &&
+		target->pcie_type != PCI_EXP_TYPE_PCI_BRIDGE))
+		return ret;
+
+	/* check all legacy devices under bridge */
+	domain_nr = pci_domain_nr(target->bus);
+	bus_start = target->subordinate->secondary;
+	bus_end = target->subordinate->subordinate;
+	while ((tmp = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, tmp)) != NULL) {
+		if (!tmp->is_pcie && pci_domain_nr(tmp->bus) == domain_nr &&
+		   tmp->bus->number >= bus_start &&
+		   tmp->bus->number <= bus_end)
+			ret &= npme_pme_target_one(tmp);
+	}
+	if (ret)
+		ret = -ENODEV;
+	return ret;
+}
+
+static void npme_work_handle(struct work_struct *work)
+{
+	struct npme_data *data = container_of(work, struct npme_data, work);
+	struct pcie_device *dev = data->dev;
+	unsigned long flags;
+	struct pci_dev *target;
+	int ret = -ENODEV;
+
+	target = pci_get_bus_and_slot(data->bdf >> 8, data->bdf & 0xff);
+	/* PCIe-PCI bridge might change bdf to (secondary bus numer, 0, 0) */
+	if (!target && (data->bdf & 0xff) == 0) {
+		struct pci_bus *bus;
+
+		bus = pci_find_bus(pci_domain_nr(dev->port->bus),
+			data->bdf >> 8);
+		if (bus) {
+			target = bus->self;
+			if (!target->is_pcie || target->pcie_type !=
+					PCI_EXP_TYPE_PCI_BRIDGE)
+				target = NULL;
+		}
+		if (target)
+			pci_dev_get(target);
+	}
+
+	if (target)
+		ret = npme_pme_target(target);
+	else
+		printk(KERN_ERR"Can't find device %02d:%d.%d which invokes PME\n",
+			data->bdf >> 8, PCI_SLOT(data->bdf), PCI_FUNC(data->bdf));
+
+	spin_lock_irqsave(&data->lock, flags);
+	/* clear pending PME */
+	npme_clear_pme(dev->port);
+	/* reenable native PME */
+	if (!data->exit)
+		npme_set_pme(dev->port, 1);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	if (ret)
+		printk(KERN_ERR"Spurious Native PME interrupt %d received\n", dev->irq);
+
+	if (target)
+		pci_dev_put(target);
+}
+
+static irqreturn_t npme_irq(int irq, void *context)
+{
+	int pos;
+	struct pci_dev *pdev;
+	u32 reg32;
+	struct npme_data *data;
+	unsigned long flags;
+
+	pdev = ((struct pcie_device *)context)->port;
+	data = get_service_data((struct pcie_device *)context);
+
+	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
+
+	spin_lock_irqsave(&data->lock, flags);
+	pci_read_config_dword(pdev, pos + PCI_EXP_RTSTA, &reg32);
+	if (!(reg32 & PCI_EXP_RTSTA_PME)) {
+		spin_unlock_irqrestore(&data->lock, flags);
+		return IRQ_NONE;
+	}
+
+	data->bdf = reg32;
+
+	/* disable PME to avoid interrupt flood */
+	npme_set_pme(pdev, 0);
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	schedule_work(&data->work);
+
+	return IRQ_HANDLED;
+}
+
+#ifdef CONFIG_ACPI
+static int npme_osc_setup(struct pcie_device *pciedev)
+{
+	acpi_status status = AE_NOT_FOUND;
+	struct pci_dev *pdev = pciedev->port;
+	acpi_handle handle = DEVICE_ACPI_HANDLE(&pdev->dev);
+
+	if (!handle)
+		return -EINVAL;
+	pcie_osc_support_set(OSC_EXT_PCI_CONFIG_SUPPORT);
+	status = pci_osc_control_set(handle,
+			OSC_PCI_EXPRESS_NATIVE_HP_CONTROL |
+			OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL);
+
+	if (ACPI_FAILURE(status)) {
+		printk(KERN_DEBUG "Native PME service couldn't init device "
+			"%s - %s\n", pciedev->device.bus_id,
+			(status == AE_SUPPORT || status == AE_NOT_FOUND) ?
+			"no _OSC support" : "Run ACPI _OSC fails");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+#else
+#define npme_osc_setup(e) (0)
+#endif
+
+static int __devinit npme_probe (struct pcie_device *dev,
+				const struct pcie_port_service_id *id )
+{
+	struct pci_dev *pdev;
+	int status;
+	struct npme_data *data;
+
+	if (npme_osc_setup(dev) && !force)
+		return -EINVAL;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	spin_lock_init(&data->lock);
+	INIT_WORK(&data->work, npme_work_handle);
+	data->dev = dev;
+
+	pdev = dev->port;
+
+	/* clear pending PME */
+	npme_clear_pme(pdev);
+
+	status = request_irq(dev->irq, npme_irq, IRQF_SHARED, "npme", dev);
+	if (status) {
+		kfree(data);
+		return status;
+	}
+
+	/* enable PME interrupt */
+	npme_set_pme(pdev, 1);
+
+	set_service_data(dev, data);
+	return 0;
+}
+
+static void npme_remove(struct pcie_device *dev)
+{
+	struct pci_dev *pdev;
+	unsigned long flags;
+	struct npme_data *data = get_service_data(dev);
+
+	pdev = dev->port;
+
+	/* disable PME interrupt */
+	spin_lock_irqsave(&data->lock, flags);
+	data->exit = 1;
+	npme_set_pme(pdev, 0);
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	flush_scheduled_work();
+	free_irq(dev->irq, dev);
+
+	/* clear pending PME */
+	npme_clear_pme(pdev);
+
+	set_service_data(dev, NULL);
+	kfree(data);
+}
+
+static int npme_suspend(struct pcie_device *dev, pm_message_t state)
+{
+	struct pci_dev *pdev;
+	struct npme_data *data;
+	unsigned long flags;
+
+	pdev = dev->port;
+	data = get_service_data(dev);
+
+	spin_lock_irqsave(&data->lock, flags);
+	/* disable PME to avoid further interrupt */
+	npme_set_pme(pdev, 0);
+
+	/* clear pending PME */
+	npme_clear_pme(pdev);
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	return 0;
+}
+
+static int npme_resume(struct pcie_device *dev)
+{
+	struct pci_dev *pdev = dev->port;
+	unsigned long flags;
+	struct npme_data *data = get_service_data(dev);
+
+	spin_lock_irqsave(&data->lock, flags);
+	npme_clear_pme(pdev);
+	npme_set_pme(pdev, 1);
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	return 0;
+}
+
+static struct pcie_port_service_driver npme_driver = {
+	.name		= "npme",
+	.id_table	= &npme_id[0],
+
+	.probe		= npme_probe,
+	.remove		= npme_remove,
+	.suspend	= npme_suspend,
+	.resume		= npme_resume,
+};
+
+
+static int __init npme_service_init(void)
+{
+	if (disabled)
+		return -EINVAL;
+	return pcie_port_service_register(&npme_driver);
+}
+
+static void __exit npme_service_exit(void)
+{
+	pcie_port_service_unregister(&npme_driver);
+}
+
+module_init(npme_service_init);
+module_exit(npme_service_exit);
+
+MODULE_AUTHOR("Shaohua Li<shaohua.li@intel.com>");
+MODULE_LICENSE("GPL");
Index: linux/include/linux/pci_regs.h
===================================================================
--- linux.orig/include/linux/pci_regs.h	2008-09-08 13:55:55.000000000 +0800
+++ linux/include/linux/pci_regs.h	2008-09-08 14:26:13.000000000 +0800
@@ -419,6 +419,7 @@
 #define  PCI_EXP_RTCTL_CRSSVE	0x10	/* CRS Software Visibility Enable */
 #define PCI_EXP_RTCAP		30	/* Root Capabilities */
 #define PCI_EXP_RTSTA		32	/* Root Status */
+#define  PCI_EXP_RTSTA_PME	0x10000	/* PME status */
 
 /* Extended Capabilities (PCI-X 2.0 and Express) */
 #define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)

-- 

Re: [RFC 4/5] PCIe native PME detection

[Rafael J. Wysocki]

On Monday, 8 of September 2008, shaohua.li@intel.com wrote:
> This implements PCIe native PME detection.

Could you please describe how this is done?

Generally, please add kerneldoc comments to all of the functions and make it
clear what they are for.

> ---
>  drivers/pci/pcie/Kconfig  |    7 
>  drivers/pci/pcie/Makefile |    2 
>  drivers/pci/pcie/npme.c   |  341 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci_regs.h  |    1 
>  4 files changed, 351 insertions(+)
> 
> Index: linux/drivers/pci/pcie/Kconfig
> ===================================================================
> --- linux.orig/drivers/pci/pcie/Kconfig	2008-09-08 13:55:55.000000000 +0800
> +++ linux/drivers/pci/pcie/Kconfig	2008-09-08 14:26:13.000000000 +0800
> @@ -46,3 +46,10 @@ config PCIEASPM_DEBUG
>  	help
>  	  This enables PCI Express ASPM debug support. It will add per-device
>  	  interface to control ASPM.
> +
> +config PCIENPME
> +	bool "PCIE Native PME support(Experimental)"
> +	depends on PCIEPORTBUS && EXPERIMENTAL
> +	help
> +	  This enables PCI Express Native PME Reporting.
> +
> Index: linux/drivers/pci/pcie/Makefile
> ===================================================================
> --- linux.orig/drivers/pci/pcie/Makefile	2008-09-08 13:55:55.000000000 +0800
> +++ linux/drivers/pci/pcie/Makefile	2008-09-08 14:26:13.000000000 +0800
> @@ -11,3 +11,5 @@ obj-$(CONFIG_PCIEPORTBUS)	+= pcieportdrv
>  
>  # Build PCI Express AER if needed
>  obj-$(CONFIG_PCIEAER)		+= aer/
> +
> +obj-$(CONFIG_PCIENPME) += npme.o
> Index: linux/drivers/pci/pcie/npme.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux/drivers/pci/pcie/npme.c	2008-09-08 14:26:13.000000000 +0800
> @@ -0,0 +1,341 @@
> +/*
> + * PCIE Native PME support
> + *
> + * Copyright (C) 2007 - 2008 Intel Corp
> + *  Shaohua Li <shaohua.li@intel.com>
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/pcieport_if.h>
> +#include <linux/acpi.h>
> +#include <linux/pci-acpi.h>
> +
> +static int disabled;
> +module_param(disabled, bool, 0);
> +static int force = 1;
> +module_param(force, bool, 0);
> +
> +static struct pcie_port_service_id npme_id[] = {
> +	{
> +	.vendor 	= PCI_ANY_ID,
> +	.device 	= PCI_ANY_ID,
> +	.port_type 	= PCIE_RC_PORT,
> +	.service_type 	= PCIE_PORT_SERVICE_PME,
> +	},
> +	{ /* end: all zeroes */ }
> +};
> +
> +struct npme_data {
> +	spinlock_t lock;
> +	struct pcie_device *dev;

It looks like 'dev' will always be the root complex.  Is that correct?

> +	struct work_struct work;
> +	u16 bdf; /* device which invokes PME */
> +	int exit;
> +};
> +

This one is analogous to pci_pme_active(), isn't it?

Why don't you want it to clear the status bit as well?

> +static inline void npme_set_pme(struct pci_dev *pdev, int enable)
> +{
> +	int pos;
> +	u16 reg16;
> +
> +	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);

OK, so we ask if this is a PCI Express device.  Shouldn't we also check if
this actually is a root port?  Also, why don't we use ->is_pcie and
->pcie_type here?

> +	pci_read_config_word(pdev, pos + PCI_EXP_RTCTL, &reg16);
> +	if (!enable)
> +		reg16 &= ~PCI_EXP_RTCTL_PMEIE;
> +	else
> +		reg16 |= PCI_EXP_RTCTL_PMEIE;
> +	pci_write_config_word(pdev, pos + PCI_EXP_RTCTL, reg16);
> +}
> +
> +static inline void npme_clear_pme(struct pci_dev *pdev)
> +{
> +	int pos;
> +	u32 reg32;
> +
> +	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);

Same here?

> +	pci_read_config_dword(pdev, pos + PCI_EXP_RTSTA, &reg32);
> +	reg32 |= PCI_EXP_RTSTA_PME;
> +	pci_write_config_dword(pdev, pos + PCI_EXP_RTSTA, reg32);
> +}

The names npme_clear_pme() and npme_set_pme() are quite confusing, because
they suggest the functions should just do opposite things to the same bit in a
register, which is not the case.

> +static int npme_pme_target_one(struct pci_dev *target)
> +{
> +	int ret = -ENODEV;
> +	if (target->dev.bus->pm && target->dev.bus->pm->base.wakeup_event)
> +		ret = target->dev.bus->pm->base.wakeup_event(&target->dev);

I'd use

	struct device *dev = &target->dev;
	struct device_driver *drv = dev->driver;

to simplify the references here.

> +	if (!ret)
> +		device_receive_wakeup_event(&target->dev);

Do you have any potential use of device_receive_wakeup_event() in mind?

> +	return ret;
> +}
> +
> +static int npme_pme_target(struct pci_dev *target)
> +{
> +	int ret;
> +	struct pci_dev *tmp = NULL;

Why not to call it 'pdev' or just 'dev'?

> +	int domain_nr, bus_start, bus_end;
> +
> +	ret = npme_pme_target_one(target);
> +	/*
> +	 * Because PCIe-PCI bridge or pci root port can take ownership, legacy
> +	 * devices under the bridges might share a BDF. The BDF can be root
> +	 * port's BDF or (PCIe-PCI bridge's secondary bus number, 0, 0)
> +	 */
> +	if (!target->subordinate || !target->is_pcie ||
> +		(target->pcie_type != PCI_EXP_TYPE_ROOT_PORT &&
> +		target->pcie_type != PCI_EXP_TYPE_PCI_BRIDGE))
> +		return ret;
> +
> +	/* check all legacy devices under bridge */
> +	domain_nr = pci_domain_nr(target->bus);
> +	bus_start = target->subordinate->secondary;
> +	bus_end = target->subordinate->subordinate;
> +	while ((tmp = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, tmp)) != NULL) {
> +		if (!tmp->is_pcie && pci_domain_nr(tmp->bus) == domain_nr &&
> +		   tmp->bus->number >= bus_start &&
> +		   tmp->bus->number <= bus_end)
> +			ret &= npme_pme_target_one(tmp);

I'd do 
			int dev_ret = npme_pme_target_one(dev);
			if (!ret)
				ret = dev_ret;

> +	}
> +	if (ret)
> +		ret = -ENODEV;

and drop the two lines above.

> +	return ret;
> +}
> +
> +static void npme_work_handle(struct work_struct *work)
> +{
> +	struct npme_data *data = container_of(work, struct npme_data, work);
> +	struct pcie_device *dev = data->dev;
> +	unsigned long flags;
> +	struct pci_dev *target;
> +	int ret = -ENODEV;
> +
> +	target = pci_get_bus_and_slot(data->bdf >> 8, data->bdf & 0xff);
> +	/* PCIe-PCI bridge might change bdf to (secondary bus numer, 0, 0) */
> +	if (!target && (data->bdf & 0xff) == 0) {
> +		struct pci_bus *bus;
> +
> +		bus = pci_find_bus(pci_domain_nr(dev->port->bus),
> +			data->bdf >> 8);
> +		if (bus) {
> +			target = bus->self;
> +			if (!target->is_pcie || target->pcie_type !=
> +					PCI_EXP_TYPE_PCI_BRIDGE)
> +				target = NULL;
> +		}
> +		if (target)
> +			pci_dev_get(target);
> +	}
> +
> +	if (target)
> +		ret = npme_pme_target(target);
> +	else
> +		printk(KERN_ERR"Can't find device %02d:%d.%d which invokes PME\n",
> +			data->bdf >> 8, PCI_SLOT(data->bdf), PCI_FUNC(data->bdf));
> +
> +	spin_lock_irqsave(&data->lock, flags);
> +	/* clear pending PME */
> +	npme_clear_pme(dev->port);
> +	/* reenable native PME */
> +	if (!data->exit)
> +		npme_set_pme(dev->port, 1);
> +
> +	spin_unlock_irqrestore(&data->lock, flags);
> +
> +	if (ret)
> +		printk(KERN_ERR"Spurious Native PME interrupt %d received\n", dev->irq);
> +
> +	if (target)
> +		pci_dev_put(target);
> +}
> +
> +static irqreturn_t npme_irq(int irq, void *context)
> +{
> +	int pos;
> +	struct pci_dev *pdev;
> +	u32 reg32;

More meaningful name (eg. the name of the register it's used to read), please?

> +	struct npme_data *data;
> +	unsigned long flags;
> +
> +	pdev = ((struct pcie_device *)context)->port;
> +	data = get_service_data((struct pcie_device *)context);
> +
> +	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);

Again, that has to be PCI Express root port IMO.

> +	spin_lock_irqsave(&data->lock, flags);
> +	pci_read_config_dword(pdev, pos + PCI_EXP_RTSTA, &reg32);
> +	if (!(reg32 & PCI_EXP_RTSTA_PME)) {
> +		spin_unlock_irqrestore(&data->lock, flags);
> +		return IRQ_NONE;
> +	}
> +
> +	data->bdf = reg32;

bdf has been defined as u16.  Is that intentional?

> +
> +	/* disable PME to avoid interrupt flood */
> +	npme_set_pme(pdev, 0);
> +	spin_unlock_irqrestore(&data->lock, flags);
> +
> +	schedule_work(&data->work);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +#ifdef CONFIG_ACPI
> +static int npme_osc_setup(struct pcie_device *pciedev)
> +{
> +	acpi_status status = AE_NOT_FOUND;
> +	struct pci_dev *pdev = pciedev->port;
> +	acpi_handle handle = DEVICE_ACPI_HANDLE(&pdev->dev);
> +
> +	if (!handle)
> +		return -EINVAL;
> +	pcie_osc_support_set(OSC_EXT_PCI_CONFIG_SUPPORT);
> +	status = pci_osc_control_set(handle,
> +			OSC_PCI_EXPRESS_NATIVE_HP_CONTROL |
> +			OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL);
> +
> +	if (ACPI_FAILURE(status)) {
> +		printk(KERN_DEBUG "Native PME service couldn't init device "
> +			"%s - %s\n", pciedev->device.bus_id,
> +			(status == AE_SUPPORT || status == AE_NOT_FOUND) ?
> +			"no _OSC support" : "Run ACPI _OSC fails");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +#else
> +#define npme_osc_setup(e) (0)

I'd prefer a static inline.  Also, is that safe to return 0 here?

> +#endif
> +
> +static int __devinit npme_probe (struct pcie_device *dev,
> +				const struct pcie_port_service_id *id )
> +{
> +	struct pci_dev *pdev;
> +	int status;
> +	struct npme_data *data;

Why don't we check if dev is a root complex?

> +	if (npme_osc_setup(dev) && !force)
> +		return -EINVAL;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +	spin_lock_init(&data->lock);
> +	INIT_WORK(&data->work, npme_work_handle);
> +	data->dev = dev;

Isn't it necessary to do set_service_data(dev, data) before requesting the IRQ?

> +	pdev = dev->port;
> +
> +	/* clear pending PME */
> +	npme_clear_pme(pdev);
> +
> +	status = request_irq(dev->irq, npme_irq, IRQF_SHARED, "npme", dev);

How exactly is dev->irq determined and where does that happen?

> +	if (status) {
> +		kfree(data);
> +		return status;
> +	}
> +
> +	/* enable PME interrupt */
> +	npme_set_pme(pdev, 1);
> +
> +	set_service_data(dev, data);
> +	return 0;
> +}
> +
> +static void npme_remove(struct pcie_device *dev)
> +{
> +	struct pci_dev *pdev;
> +	unsigned long flags;
> +	struct npme_data *data = get_service_data(dev);
> +
> +	pdev = dev->port;
> +
> +	/* disable PME interrupt */
> +	spin_lock_irqsave(&data->lock, flags);
> +	data->exit = 1;
> +	npme_set_pme(pdev, 0);
> +	spin_unlock_irqrestore(&data->lock, flags);
> +
> +	flush_scheduled_work();
> +	free_irq(dev->irq, dev);
> +
> +	/* clear pending PME */
> +	npme_clear_pme(pdev);
> +
> +	set_service_data(dev, NULL);
> +	kfree(data);
> +}
> +
> +static int npme_suspend(struct pcie_device *dev, pm_message_t state)
> +{
> +	struct pci_dev *pdev;
> +	struct npme_data *data;
> +	unsigned long flags;
> +
> +	pdev = dev->port;
> +	data = get_service_data(dev);
> +
> +	spin_lock_irqsave(&data->lock, flags);
> +	/* disable PME to avoid further interrupt */
> +	npme_set_pme(pdev, 0);
> +
> +	/* clear pending PME */
> +	npme_clear_pme(pdev);
> +	spin_unlock_irqrestore(&data->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int npme_resume(struct pcie_device *dev)
> +{
> +	struct pci_dev *pdev = dev->port;
> +	unsigned long flags;
> +	struct npme_data *data = get_service_data(dev);
> +
> +	spin_lock_irqsave(&data->lock, flags);
> +	npme_clear_pme(pdev);
> +	npme_set_pme(pdev, 1);
> +	spin_unlock_irqrestore(&data->lock, flags);
> +
> +	return 0;
> +}
> +
> +static struct pcie_port_service_driver npme_driver = {
> +	.name		= "npme",
> +	.id_table	= &npme_id[0],
> +
> +	.probe		= npme_probe,
> +	.remove		= npme_remove,
> +	.suspend	= npme_suspend,
> +	.resume		= npme_resume,
> +};
> +
> +
> +static int __init npme_service_init(void)
> +{
> +	if (disabled)
> +		return -EINVAL;
> +	return pcie_port_service_register(&npme_driver);
> +}
> +
> +static void __exit npme_service_exit(void)
> +{
> +	pcie_port_service_unregister(&npme_driver);
> +}
> +
> +module_init(npme_service_init);
> +module_exit(npme_service_exit);
> +
> +MODULE_AUTHOR("Shaohua Li<shaohua.li@intel.com>");
> +MODULE_LICENSE("GPL");
> Index: linux/include/linux/pci_regs.h
> ===================================================================
> --- linux.orig/include/linux/pci_regs.h	2008-09-08 13:55:55.000000000 +0800
> +++ linux/include/linux/pci_regs.h	2008-09-08 14:26:13.000000000 +0800
> @@ -419,6 +419,7 @@
>  #define  PCI_EXP_RTCTL_CRSSVE	0x10	/* CRS Software Visibility Enable */
>  #define PCI_EXP_RTCAP		30	/* Root Capabilities */
>  #define PCI_EXP_RTSTA		32	/* Root Status */
> +#define  PCI_EXP_RTSTA_PME	0x10000	/* PME status */
>  
>  /* Extended Capabilities (PCI-X 2.0 and Express) */
>  #define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)
> 

Apart from the above, I'd start the names of all functions from
'native_pme' rather than from 'npme'.

Thanks,
Rafael

RE: [RFC 4/5] PCIe native PME detection

[Li, Shaohua]

>On Monday, 8 of September 2008, shaohua.li@intel.com wrote:
>> This implements PCIe native PME detection.
>
>Could you please describe how this is done?
>
>Generally, please add kerneldoc comments to all of the functions and make
>it
>clear what they are for.
Sorry, I'll do it later. Currently the main goal is to discuss the device core changes. We need generic implementation not just for x86.

>It looks like 'dev' will always be the root complex.  Is that correct?
Right, and the port driver is just for root complex.

>> +     struct work_struct work;
>> +     u16 bdf; /* device which invokes PME */
>> +     int exit;
>> +};
>> +
>
>This one is analogous to pci_pme_active(), isn't it?
>
>Why don't you want it to clear the status bit as well?
No, they are some pcie root complex's native pme bits, I'll document them later. Also, I'll address your other comments later.

Thanks,
Shaohua

[RFC 5/5] ACPI GPE based wakeup event detection

[shaohua.li]

[-- Attachment #1: acpi-gpe.patch --]
[-- Type: text/plain, Size: 4885 bytes --]

In ACPI platform, if native PME isn't enabled, GPE is used to report wakeup event.
---
 drivers/acpi/Kconfig        |    9 ++++++
 drivers/acpi/bus.c          |   15 ++++++++++
 drivers/acpi/sleep/wakeup.c |   66 ++++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h     |    4 ++
 4 files changed, 94 insertions(+)

Index: linux/drivers/acpi/Kconfig
===================================================================
--- linux.orig/drivers/acpi/Kconfig	2008-09-08 14:28:50.000000000 +0800
+++ linux/drivers/acpi/Kconfig	2008-09-08 14:29:08.000000000 +0800
@@ -45,6 +45,15 @@ config ACPI_SLEEP
 	depends on PM_SLEEP
 	default y
 
+config ACPI_GPE_WAKEUP
+	bool "ACPI wakeup event support"
+	depends on PM_SLEEP && EXPERIMENTAL
+	help
+	  Enable ACPI to detect wakeup event. For example, PCI device can
+	  invoke PME, and in ACPI platform, the PME will invoke a GPE. With
+	  the option, we can detect which device invokes wakeup event.
+
+
 config ACPI_PROCFS
 	bool "Deprecated /proc/acpi files"
 	depends on PROC_FS
Index: linux/drivers/acpi/bus.c
===================================================================
--- linux.orig/drivers/acpi/bus.c	2008-09-08 14:28:32.000000000 +0800
+++ linux/drivers/acpi/bus.c	2008-09-08 14:29:03.000000000 +0800
@@ -496,6 +496,19 @@ static int acpi_bus_check_scope(struct a
 	return 0;
 }
 
+static BLOCKING_NOTIFIER_HEAD(acpi_bus_notify_list);
+int register_acpi_bus_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&acpi_bus_notify_list, nb);
+}
+EXPORT_SYMBOL_GPL(register_acpi_bus_notifier);
+
+void unregister_acpi_bus_notifier(struct notifier_block *nb)
+{
+	blocking_notifier_chain_unregister(&acpi_bus_notify_list, nb);
+}
+EXPORT_SYMBOL_GPL(unregister_acpi_bus_notifier);
+
 /**
  * acpi_bus_notify
  * ---------------
@@ -506,6 +519,8 @@ static void acpi_bus_notify(acpi_handle 
 	int result = 0;
 	struct acpi_device *device = NULL;
 
+	blocking_notifier_call_chain(&acpi_bus_notify_list,
+			type, (void *)handle);
 
 	if (acpi_bus_get_device(handle, &device))
 		return;
Index: linux/drivers/acpi/sleep/wakeup.c
===================================================================
--- linux.orig/drivers/acpi/sleep/wakeup.c	2008-09-08 14:28:55.000000000 +0800
+++ linux/drivers/acpi/sleep/wakeup.c	2008-09-08 15:04:23.000000000 +0800
@@ -142,6 +142,70 @@ void acpi_disable_wakeup_device(u8 sleep
 	spin_unlock(&acpi_device_lock);
 }
 
+#ifdef CONFIG_ACPI_GPE_WAKEUP
+static int acpi_gpe_pme_check(struct acpi_device *dev)
+{
+	struct device *ldev;
+
+	ldev = acpi_get_physical_device(dev->handle);
+	if (!ldev)
+		return -ENODEV;
+	/*
+	 * AML code might already clear the event, so ignore the return value.
+	 * Actually we can't correctly detect which device invokes GPE if the
+	 * event is cleared.
+	 */
+	if (ldev->bus->pm && ldev->bus->pm->base.wakeup_event)
+		ldev->bus->pm->base.wakeup_event(ldev);
+
+	/*
+	 * We always send the event. AML code usually identifies the exact
+	 * device of the GPE, let's trust it
+	 */
+	device_receive_wakeup_event(ldev);
+
+	put_device(ldev);
+	return 0;
+}
+
+static int acpi_gpe_pme_handler(struct notifier_block *nb,
+	unsigned long type, void *data)
+{
+	int ret;
+	acpi_handle handle = data;
+	struct acpi_device *dev;
+
+	if (type != ACPI_NOTIFY_DEVICE_WAKE)
+		return NOTIFY_DONE;
+
+	if (acpi_bus_get_device(handle, &dev))
+		return NOTIFY_DONE;
+
+	ret = acpi_gpe_pme_check(dev);
+
+	acpi_disable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
+		ACPI_NOT_ISR);
+
+	/* FIXME: spurious interrupt, disables it? */
+	if (ret)
+		printk(KERN_ERR"Spurious GPE %d detected\n",
+			dev->wakeup.gpe_number);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block acpi_gpe_pme_nb = {
+	.notifier_call = acpi_gpe_pme_handler,
+};
+
+static void acpi_init_gpe_pme(void)
+{
+	register_acpi_bus_notifier(&acpi_gpe_pme_nb);
+}
+#else
+static inline void acpi_init_gpe_pme(void) {}
+#endif
+
 static int __init acpi_wakeup_device_init(void)
 {
 	struct list_head *node, *next;
@@ -167,6 +231,8 @@ static int __init acpi_wakeup_device_ini
 		spin_lock(&acpi_device_lock);
 	}
 	spin_unlock(&acpi_device_lock);
+
+	acpi_init_gpe_pme();
 	return 0;
 }
 
Index: linux/include/acpi/acpi_bus.h
===================================================================
--- linux.orig/include/acpi/acpi_bus.h	2008-09-08 14:28:42.000000000 +0800
+++ linux/include/acpi/acpi_bus.h	2008-09-08 14:29:03.000000000 +0800
@@ -327,6 +327,10 @@ int acpi_bus_get_private_data(acpi_handl
 extern int acpi_notifier_call_chain(struct acpi_device *, u32, u32);
 extern int register_acpi_notifier(struct notifier_block *);
 extern int unregister_acpi_notifier(struct notifier_block *);
+
+extern int register_acpi_bus_notifier(struct notifier_block *nb);
+extern void unregister_acpi_bus_notifier(struct notifier_block *nb);
+
 /*
  * External Functions
  */

-- 

Re: [RFC 5/5] ACPI GPE based wakeup event detection

[Rafael J. Wysocki]

On Monday, 8 of September 2008, shaohua.li@intel.com wrote:
> In ACPI platform, if native PME isn't enabled, GPE is used to report wakeup event.
> ---
>  drivers/acpi/Kconfig        |    9 ++++++
>  drivers/acpi/bus.c          |   15 ++++++++++
>  drivers/acpi/sleep/wakeup.c |   66 ++++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/acpi_bus.h     |    4 ++
>  4 files changed, 94 insertions(+)
> 
> Index: linux/drivers/acpi/Kconfig
> ===================================================================
> --- linux.orig/drivers/acpi/Kconfig	2008-09-08 14:28:50.000000000 +0800
> +++ linux/drivers/acpi/Kconfig	2008-09-08 14:29:08.000000000 +0800
> @@ -45,6 +45,15 @@ config ACPI_SLEEP
>  	depends on PM_SLEEP
>  	default y
>  
> +config ACPI_GPE_WAKEUP
> +	bool "ACPI wakeup event support"
> +	depends on PM_SLEEP && EXPERIMENTAL
> +	help
> +	  Enable ACPI to detect wakeup event. For example, PCI device can
> +	  invoke PME, and in ACPI platform, the PME will invoke a GPE. With
> +	  the option, we can detect which device invokes wakeup event.
> +
> +
>  config ACPI_PROCFS
>  	bool "Deprecated /proc/acpi files"
>  	depends on PROC_FS
> Index: linux/drivers/acpi/bus.c
> ===================================================================
> --- linux.orig/drivers/acpi/bus.c	2008-09-08 14:28:32.000000000 +0800
> +++ linux/drivers/acpi/bus.c	2008-09-08 14:29:03.000000000 +0800
> @@ -496,6 +496,19 @@ static int acpi_bus_check_scope(struct a
>  	return 0;
>  }
>  
> +static BLOCKING_NOTIFIER_HEAD(acpi_bus_notify_list);
> +int register_acpi_bus_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&acpi_bus_notify_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(register_acpi_bus_notifier);
> +
> +void unregister_acpi_bus_notifier(struct notifier_block *nb)
> +{
> +	blocking_notifier_chain_unregister(&acpi_bus_notify_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(unregister_acpi_bus_notifier);
> +
>  /**
>   * acpi_bus_notify
>   * ---------------
> @@ -506,6 +519,8 @@ static void acpi_bus_notify(acpi_handle 
>  	int result = 0;
>  	struct acpi_device *device = NULL;
>  
> +	blocking_notifier_call_chain(&acpi_bus_notify_list,
> +			type, (void *)handle);

Hm, perhaps I'm too tired and I'm missing something obvious, but can you
tell me please why that has to be a notifier chain?  It looks like you add only
one notifier to it, so seemingly it could be replaced by a direct call to a
function like acpi_gpe_pme_handler() (with modified list of arguments).

>  	if (acpi_bus_get_device(handle, &device))
>  		return;
> Index: linux/drivers/acpi/sleep/wakeup.c
> ===================================================================
> --- linux.orig/drivers/acpi/sleep/wakeup.c	2008-09-08 14:28:55.000000000 +0800
> +++ linux/drivers/acpi/sleep/wakeup.c	2008-09-08 15:04:23.000000000 +0800
> @@ -142,6 +142,70 @@ void acpi_disable_wakeup_device(u8 sleep
>  	spin_unlock(&acpi_device_lock);
>  }
>  
> +#ifdef CONFIG_ACPI_GPE_WAKEUP
> +static int acpi_gpe_pme_check(struct acpi_device *dev)
> +{
> +	struct device *ldev;
> +
> +	ldev = acpi_get_physical_device(dev->handle);
> +	if (!ldev)
> +		return -ENODEV;
> +	/*
> +	 * AML code might already clear the event, so ignore the return value.
> +	 * Actually we can't correctly detect which device invokes GPE if the
> +	 * event is cleared.
> +	 */
> +	if (ldev->bus->pm && ldev->bus->pm->base.wakeup_event)
> +		ldev->bus->pm->base.wakeup_event(ldev);
> +
> +	/*
> +	 * We always send the event. AML code usually identifies the exact
> +	 * device of the GPE, let's trust it
> +	 */
> +	device_receive_wakeup_event(ldev);
> +
> +	put_device(ldev);
> +	return 0;
> +}
> +
> +static int acpi_gpe_pme_handler(struct notifier_block *nb,
> +	unsigned long type, void *data)
> +{
> +	int ret;
> +	acpi_handle handle = data;
> +	struct acpi_device *dev;
> +
> +	if (type != ACPI_NOTIFY_DEVICE_WAKE)
> +		return NOTIFY_DONE;
> +
> +	if (acpi_bus_get_device(handle, &dev))
> +		return NOTIFY_DONE;
> +
> +	ret = acpi_gpe_pme_check(dev);
> +
> +	acpi_disable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
> +		ACPI_NOT_ISR);
> +

At which point are dev->wakeup.gpe_device and dev->wakeup.gpe_number
determined, for example, for PCI devices, and how?

> +	/* FIXME: spurious interrupt, disables it? */
> +	if (ret)
> +		printk(KERN_ERR"Spurious GPE %d detected\n",
> +			dev->wakeup.gpe_number);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block acpi_gpe_pme_nb = {
> +	.notifier_call = acpi_gpe_pme_handler,
> +};
> +
> +static void acpi_init_gpe_pme(void)
> +{
> +	register_acpi_bus_notifier(&acpi_gpe_pme_nb);
> +}
> +#else
> +static inline void acpi_init_gpe_pme(void) {}
> +#endif
> +
>  static int __init acpi_wakeup_device_init(void)
>  {
>  	struct list_head *node, *next;
> @@ -167,6 +231,8 @@ static int __init acpi_wakeup_device_ini
>  		spin_lock(&acpi_device_lock);
>  	}
>  	spin_unlock(&acpi_device_lock);
> +
> +	acpi_init_gpe_pme();
>  	return 0;
>  }
>  
> Index: linux/include/acpi/acpi_bus.h
> ===================================================================
> --- linux.orig/include/acpi/acpi_bus.h	2008-09-08 14:28:42.000000000 +0800
> +++ linux/include/acpi/acpi_bus.h	2008-09-08 14:29:03.000000000 +0800
> @@ -327,6 +327,10 @@ int acpi_bus_get_private_data(acpi_handl
>  extern int acpi_notifier_call_chain(struct acpi_device *, u32, u32);
>  extern int register_acpi_notifier(struct notifier_block *);
>  extern int unregister_acpi_notifier(struct notifier_block *);
> +
> +extern int register_acpi_bus_notifier(struct notifier_block *nb);
> +extern void unregister_acpi_bus_notifier(struct notifier_block *nb);
> +
>  /*
>   * External Functions
>   */

Thanks,
Rafael

Re: [RFC 5/5] ACPI GPE based wakeup event detection

[Zhao Yakui]

On Mon, 2008-09-08 at 22:57 +0200, Rafael J. Wysocki wrote:
> On Monday, 8 of September 2008, shaohua.li@intel.com wrote:
> > In ACPI platform, if native PME isn't enabled, GPE is used to report wakeup event.
> > ---
> >  drivers/acpi/Kconfig        |    9 ++++++
> >  drivers/acpi/bus.c          |   15 ++++++++++
> >  drivers/acpi/sleep/wakeup.c |   66 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/acpi/acpi_bus.h     |    4 ++
> >  4 files changed, 94 insertions(+)
> > 
> > Index: linux/drivers/acpi/Kconfig
> > ===================================================================
> > --- linux.orig/drivers/acpi/Kconfig	2008-09-08 14:28:50.000000000 +0800
> > +++ linux/drivers/acpi/Kconfig	2008-09-08 14:29:08.000000000 +0800
> > @@ -45,6 +45,15 @@ config ACPI_SLEEP
> >  	depends on PM_SLEEP
> >  	default y
> >  
> > +config ACPI_GPE_WAKEUP
> > +	bool "ACPI wakeup event support"
> > +	depends on PM_SLEEP && EXPERIMENTAL
> > +	help
> > +	  Enable ACPI to detect wakeup event. For example, PCI device can
> > +	  invoke PME, and in ACPI platform, the PME will invoke a GPE. With
> > +	  the option, we can detect which device invokes wakeup event.
> > +
> > +
> >  config ACPI_PROCFS
> >  	bool "Deprecated /proc/acpi files"
> >  	depends on PROC_FS
> > Index: linux/drivers/acpi/bus.c
> > ===================================================================
> > --- linux.orig/drivers/acpi/bus.c	2008-09-08 14:28:32.000000000 +0800
> > +++ linux/drivers/acpi/bus.c	2008-09-08 14:29:03.000000000 +0800
> > @@ -496,6 +496,19 @@ static int acpi_bus_check_scope(struct a
> >  	return 0;
> >  }
> >  
> > +static BLOCKING_NOTIFIER_HEAD(acpi_bus_notify_list);
> > +int register_acpi_bus_notifier(struct notifier_block *nb)
> > +{
> > +	return blocking_notifier_chain_register(&acpi_bus_notify_list, nb);
> > +}
> > +EXPORT_SYMBOL_GPL(register_acpi_bus_notifier);
> > +
> > +void unregister_acpi_bus_notifier(struct notifier_block *nb)
> > +{
> > +	blocking_notifier_chain_unregister(&acpi_bus_notify_list, nb);
> > +}
> > +EXPORT_SYMBOL_GPL(unregister_acpi_bus_notifier);
> > +
> >  /**
> >   * acpi_bus_notify
> >   * ---------------
> > @@ -506,6 +519,8 @@ static void acpi_bus_notify(acpi_handle 
> >  	int result = 0;
> >  	struct acpi_device *device = NULL;
> >  
> > +	blocking_notifier_call_chain(&acpi_bus_notify_list,
> > +			type, (void *)handle);
> 
> Hm, perhaps I'm too tired and I'm missing something obvious, but can you
> tell me please why that has to be a notifier chain?  It looks like you add only
> one notifier to it, so seemingly it could be replaced by a direct call to a
> function like acpi_gpe_pme_handler() (with modified list of arguments).
When the notifier chain is used, it can work regardless of whether the
CONFIG_ACPI_GPE_WAKEUP is set.
If the CONFIG_ACPI_GPE_WAKEUP is set, what you said is also OK. 

> 
> >  	if (acpi_bus_get_device(handle, &device))
> >  		return;
> > Index: linux/drivers/acpi/sleep/wakeup.c
> > ===================================================================
> > --- linux.orig/drivers/acpi/sleep/wakeup.c	2008-09-08 14:28:55.000000000 +0800
> > +++ linux/drivers/acpi/sleep/wakeup.c	2008-09-08 15:04:23.000000000 +0800
> > @@ -142,6 +142,70 @@ void acpi_disable_wakeup_device(u8 sleep
> >  	spin_unlock(&acpi_device_lock);
> >  }
> >  
> > +#ifdef CONFIG_ACPI_GPE_WAKEUP
> > +static int acpi_gpe_pme_check(struct acpi_device *dev)
> > +{
> > +	struct device *ldev;
> > +
> > +	ldev = acpi_get_physical_device(dev->handle);
> > +	if (!ldev)
> > +		return -ENODEV;
> > +	/*
> > +	 * AML code might already clear the event, so ignore the return value.
> > +	 * Actually we can't correctly detect which device invokes GPE if the
> > +	 * event is cleared.
> > +	 */
> > +	if (ldev->bus->pm && ldev->bus->pm->base.wakeup_event)
> > +		ldev->bus->pm->base.wakeup_event(ldev);
> > +
> > +	/*
> > +	 * We always send the event. AML code usually identifies the exact
> > +	 * device of the GPE, let's trust it
> > +	 */
> > +	device_receive_wakeup_event(ldev);
> > +
> > +	put_device(ldev);
> > +	return 0;
> > +}
> > +
> > +static int acpi_gpe_pme_handler(struct notifier_block *nb,
> > +	unsigned long type, void *data)
> > +{
> > +	int ret;
> > +	acpi_handle handle = data;
> > +	struct acpi_device *dev;
> > +
> > +	if (type != ACPI_NOTIFY_DEVICE_WAKE)
> > +		return NOTIFY_DONE;
> > +
> > +	if (acpi_bus_get_device(handle, &dev))
> > +		return NOTIFY_DONE;
> > +
> > +	ret = acpi_gpe_pme_check(dev);
> > +
> > +	acpi_disable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
> > +		ACPI_NOT_ISR);
> > +
> 
> At which point are dev->wakeup.gpe_device and dev->wakeup.gpe_number
> determined, for example, for PCI devices, and how?
In the boot phase a PCI device will be bound with an ACPI device. In the
system based on ACPI(GPE) when a PCI device generates a wakeup event, an
ACPI interrupt will be triggered and the ACPI AML code will send the
device wakeup event to the corresponding ACPI device. In such case we
can get the gpe number for the PCI device.
> 
> > +	/* FIXME: spurious interrupt, disables it? */
> > +	if (ret)
> > +		printk(KERN_ERR"Spurious GPE %d detected\n",
> > +			dev->wakeup.gpe_number);
> > +
> > +	return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block acpi_gpe_pme_nb = {
> > +	.notifier_call = acpi_gpe_pme_handler,
> > +};
> > +
> > +static void acpi_init_gpe_pme(void)
> > +{
> > +	register_acpi_bus_notifier(&acpi_gpe_pme_nb);
> > +}
> > +#else
> > +static inline void acpi_init_gpe_pme(void) {}
> > +#endif
> > +
> >  static int __init acpi_wakeup_device_init(void)
> >  {
> >  	struct list_head *node, *next;
> > @@ -167,6 +231,8 @@ static int __init acpi_wakeup_device_ini
> >  		spin_lock(&acpi_device_lock);
> >  	}
> >  	spin_unlock(&acpi_device_lock);
> > +
> > +	acpi_init_gpe_pme();
> >  	return 0;
> >  }
> >  
> > Index: linux/include/acpi/acpi_bus.h
> > ===================================================================
> > --- linux.orig/include/acpi/acpi_bus.h	2008-09-08 14:28:42.000000000 +0800
> > +++ linux/include/acpi/acpi_bus.h	2008-09-08 14:29:03.000000000 +0800
> > @@ -327,6 +327,10 @@ int acpi_bus_get_private_data(acpi_handl
> >  extern int acpi_notifier_call_chain(struct acpi_device *, u32, u32);
> >  extern int register_acpi_notifier(struct notifier_block *);
> >  extern int unregister_acpi_notifier(struct notifier_block *);
> > +
> > +extern int register_acpi_bus_notifier(struct notifier_block *nb);
> > +extern void unregister_acpi_bus_notifier(struct notifier_block *nb);
> > +
> >  /*
> >   * External Functions
> >   */
> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC 5/5] ACPI GPE based wakeup event detection

[Li, Shaohua]

>> > +
>> >  /**
>> >   * acpi_bus_notify
>> >   * ---------------
>> > @@ -506,6 +519,8 @@ static void acpi_bus_notify(acpi_handle
>> >     int result = 0;
>> >     struct acpi_device *device = NULL;
>> >
>> > +   blocking_notifier_call_chain(&acpi_bus_notify_list,
>> > +                   type, (void *)handle);
>>
>> Hm, perhaps I'm too tired and I'm missing something obvious, but can you
>> tell me please why that has to be a notifier chain?  It looks like you
>add only
>> one notifier to it, so seemingly it could be replaced by a direct call to
>a
>> function like acpi_gpe_pme_handler() (with modified list of arguments).
>When the notifier chain is used, it can work regardless of whether the
>CONFIG_ACPI_GPE_WAKEUP is set.
>If the CONFIG_ACPI_GPE_WAKEUP is set, what you said is also OK.
I actually had another usage for this (I sent a patchset for docking station, which uses it)

>> >     if (acpi_bus_get_device(handle, &device))
>> >             return;
>> > Index: linux/drivers/acpi/sleep/wakeup.c
>> > ===================================================================
>> > --- linux.orig/drivers/acpi/sleep/wakeup.c  2008-09-08
>14:28:55.000000000 +0800
>> > +++ linux/drivers/acpi/sleep/wakeup.c       2008-09-08
>15:04:23.000000000 +0800
>> > @@ -142,6 +142,70 @@ void acpi_disable_wakeup_device(u8 sleep
>> >     spin_unlock(&acpi_device_lock);
>> >  }
>> >
>> > +#ifdef CONFIG_ACPI_GPE_WAKEUP
>> > +static int acpi_gpe_pme_check(struct acpi_device *dev)
>> > +{
>> > +   struct device *ldev;
>> > +
>> > +   ldev = acpi_get_physical_device(dev->handle);
>> > +   if (!ldev)
>> > +           return -ENODEV;
>> > +   /*
>> > +    * AML code might already clear the event, so ignore the return
>value.
>> > +    * Actually we can't correctly detect which device invokes GPE if
>the
>> > +    * event is cleared.
>> > +    */
>> > +   if (ldev->bus->pm && ldev->bus->pm->base.wakeup_event)
>> > +           ldev->bus->pm->base.wakeup_event(ldev);
>> > +
>> > +   /*
>> > +    * We always send the event. AML code usually identifies the exact
>> > +    * device of the GPE, let's trust it
>> > +    */
>> > +   device_receive_wakeup_event(ldev);
>> > +
>> > +   put_device(ldev);
>> > +   return 0;
>> > +}
>> > +
>> > +static int acpi_gpe_pme_handler(struct notifier_block *nb,
>> > +   unsigned long type, void *data)
>> > +{
>> > +   int ret;
>> > +   acpi_handle handle = data;
>> > +   struct acpi_device *dev;
>> > +
>> > +   if (type != ACPI_NOTIFY_DEVICE_WAKE)
>> > +           return NOTIFY_DONE;
>> > +
>> > +   if (acpi_bus_get_device(handle, &dev))
>> > +           return NOTIFY_DONE;
>> > +
>> > +   ret = acpi_gpe_pme_check(dev);
>> > +
>> > +   acpi_disable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
>> > +           ACPI_NOT_ISR);
>> > +
>>
>> At which point are dev->wakeup.gpe_device and dev->wakeup.gpe_number
>> determined, for example, for PCI devices, and how?
>In the boot phase a PCI device will be bound with an ACPI device. In the
>system based on ACPI(GPE) when a PCI device generates a wakeup event, an
>ACPI interrupt will be triggered and the ACPI AML code will send the
>device wakeup event to the corresponding ACPI device. In such case we
>can get the gpe number for the PCI device.
This is correct. For example:

            Method (_L07, 0, NotSerialized)
            {
                Notify (\_SB.PCI0.SMBS, 0x02)
            }

            Method (_L05, 0, NotSerialized)
            {
                Notify (\_SB.PCI0.MODM, 0x02)
            }

GPE 5,7 are wakeup GPEs

Thanks,
Shaohua

Re: [RFC 5/5] ACPI GPE based wakeup event detection

[Rafael J. Wysocki]

On Tuesday, 9 of September 2008, Li, Shaohua wrote:
> >> > +
> >> >  /**
> >> >   * acpi_bus_notify
> >> >   * ---------------
> >> > @@ -506,6 +519,8 @@ static void acpi_bus_notify(acpi_handle
> >> >     int result = 0;
> >> >     struct acpi_device *device = NULL;
> >> >
> >> > +   blocking_notifier_call_chain(&acpi_bus_notify_list,
> >> > +                   type, (void *)handle);
> >>
> >> Hm, perhaps I'm too tired and I'm missing something obvious, but can you
> >> tell me please why that has to be a notifier chain?  It looks like you
> >add only
> >> one notifier to it, so seemingly it could be replaced by a direct call to
> >a
> >> function like acpi_gpe_pme_handler() (with modified list of arguments).
> >When the notifier chain is used, it can work regardless of whether the
> >CONFIG_ACPI_GPE_WAKEUP is set.
> >If the CONFIG_ACPI_GPE_WAKEUP is set, what you said is also OK.
> I actually had another usage for this (I sent a patchset for docking station, which uses it)
> 
> >> >     if (acpi_bus_get_device(handle, &device))
> >> >             return;
> >> > Index: linux/drivers/acpi/sleep/wakeup.c
> >> > ===================================================================
> >> > --- linux.orig/drivers/acpi/sleep/wakeup.c  2008-09-08
> >14:28:55.000000000 +0800
> >> > +++ linux/drivers/acpi/sleep/wakeup.c       2008-09-08
> >15:04:23.000000000 +0800
> >> > @@ -142,6 +142,70 @@ void acpi_disable_wakeup_device(u8 sleep
> >> >     spin_unlock(&acpi_device_lock);
> >> >  }
> >> >
> >> > +#ifdef CONFIG_ACPI_GPE_WAKEUP
> >> > +static int acpi_gpe_pme_check(struct acpi_device *dev)
> >> > +{
> >> > +   struct device *ldev;
> >> > +
> >> > +   ldev = acpi_get_physical_device(dev->handle);
> >> > +   if (!ldev)
> >> > +           return -ENODEV;
> >> > +   /*
> >> > +    * AML code might already clear the event, so ignore the return
> >value.
> >> > +    * Actually we can't correctly detect which device invokes GPE if
> >the
> >> > +    * event is cleared.
> >> > +    */
> >> > +   if (ldev->bus->pm && ldev->bus->pm->base.wakeup_event)
> >> > +           ldev->bus->pm->base.wakeup_event(ldev);
> >> > +
> >> > +   /*
> >> > +    * We always send the event. AML code usually identifies the exact
> >> > +    * device of the GPE, let's trust it
> >> > +    */
> >> > +   device_receive_wakeup_event(ldev);
> >> > +
> >> > +   put_device(ldev);
> >> > +   return 0;
> >> > +}
> >> > +
> >> > +static int acpi_gpe_pme_handler(struct notifier_block *nb,
> >> > +   unsigned long type, void *data)
> >> > +{
> >> > +   int ret;
> >> > +   acpi_handle handle = data;
> >> > +   struct acpi_device *dev;
> >> > +
> >> > +   if (type != ACPI_NOTIFY_DEVICE_WAKE)
> >> > +           return NOTIFY_DONE;
> >> > +
> >> > +   if (acpi_bus_get_device(handle, &dev))
> >> > +           return NOTIFY_DONE;
> >> > +
> >> > +   ret = acpi_gpe_pme_check(dev);
> >> > +
> >> > +   acpi_disable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
> >> > +           ACPI_NOT_ISR);
> >> > +
> >>
> >> At which point are dev->wakeup.gpe_device and dev->wakeup.gpe_number
> >> determined, for example, for PCI devices, and how?
> >In the boot phase a PCI device will be bound with an ACPI device. In the
> >system based on ACPI(GPE) when a PCI device generates a wakeup event, an
> >ACPI interrupt will be triggered and the ACPI AML code will send the
> >device wakeup event to the corresponding ACPI device. In such case we
> >can get the gpe number for the PCI device.
> This is correct. For example:
> 
>             Method (_L07, 0, NotSerialized)
>             {
>                 Notify (\_SB.PCI0.SMBS, 0x02)
>             }
> 
>             Method (_L05, 0, NotSerialized)
>             {
>                 Notify (\_SB.PCI0.MODM, 0x02)
>             }
> 
> GPE 5,7 are wakeup GPEs

Well, it is not clear to me, though, which one of them is associated with the
PME# signal.

Also, the question was what piece of code in the kernel was responsible for the
identification of the wake-up GPEs and for setting dev->wakeup.gpe_device
and dev->wakeup.gpe_number as appropriate.  In particular, how do we figure
out which wake-up GPE will be used for signalling PME# for devices that are
not on-board?

Thanks,
Rafael

Re: [RFC 5/5] ACPI GPE based wakeup event detection

[Alan Stern]

On Tue, 9 Sep 2008, Zhao Yakui wrote:

> > > --- linux.orig/drivers/acpi/bus.c	2008-09-08 14:28:32.000000000 +0800
> > > +++ linux/drivers/acpi/bus.c	2008-09-08 14:29:03.000000000 +0800
> > > @@ -496,6 +496,19 @@ static int acpi_bus_check_scope(struct a
> > >  	return 0;
> > >  }
> > >  
> > > +static BLOCKING_NOTIFIER_HEAD(acpi_bus_notify_list);
> > > +int register_acpi_bus_notifier(struct notifier_block *nb)
> > > +{
> > > +	return blocking_notifier_chain_register(&acpi_bus_notify_list, nb);
> > > +}
> > > +EXPORT_SYMBOL_GPL(register_acpi_bus_notifier);
> > > +
> > > +void unregister_acpi_bus_notifier(struct notifier_block *nb)
> > > +{
> > > +	blocking_notifier_chain_unregister(&acpi_bus_notify_list, nb);
> > > +}
> > > +EXPORT_SYMBOL_GPL(unregister_acpi_bus_notifier);
> > > +
> > >  /**
> > >   * acpi_bus_notify
> > >   * ---------------
> > > @@ -506,6 +519,8 @@ static void acpi_bus_notify(acpi_handle 
> > >  	int result = 0;
> > >  	struct acpi_device *device = NULL;
> > >  
> > > +	blocking_notifier_call_chain(&acpi_bus_notify_list,
> > > +			type, (void *)handle);
> > 
> > Hm, perhaps I'm too tired and I'm missing something obvious, but can you
> > tell me please why that has to be a notifier chain?  It looks like you add only
> > one notifier to it, so seemingly it could be replaced by a direct call to a
> > function like acpi_gpe_pme_handler() (with modified list of arguments).
> When the notifier chain is used, it can work regardless of whether the
> CONFIG_ACPI_GPE_WAKEUP is set.
> If the CONFIG_ACPI_GPE_WAKEUP is set, what you said is also OK. 

If CONFIG_ACPI_GPE_WAKEUP is not set then you should define 
acpi_gpe_pme_handler() as a do-nothing function.  Then you could call 
it always, regardless of the kconfig setting, with no need for a 
notifier chain.

Alan Stern

Re: [RFC 0/5] device wakeup event support

[David Brownell]

On Monday 08 September 2008, shaohua.li@intel.com wrote:
> This series add device wakeup event detection support. This is the base to
> implement runtime device suspend/resume, though we don't support it now.
> But David said USB is approaching to this. See this bugzilla
> http://bugzilla.kernel.org/show_bug.cgi?id=6892 for detail.

Glad to see more attention here.  :)

Note that this issue is specific to systems using PCI (at core),
with additional nuances for PCI systems which use ACPI.

Device wakeup through USB works fine on various non-PCI and
non-ACPI embedded systems, without needing these patches.
It's done so for several years now...


> The current process to handle wakeup event is:
> 1. driver enable wakeup event line pme and suspend
> 2. NPME or ACPI receives wakeup event
> 3. NPME or ACPI call .wakeup_event() to clear and disable wakeup
>    event. Driver can do extra things in .wakeup_event() too. 
> 4. NPME or ACPI call generic wakeup event handler (device_receive_wakeup_event())
> 5. device resumes, and goto 1 for next round of suspend
> 
> There are somethings we need discuss:
> 1. is this generic for other platforms?
> 2. what should the generic wakeup event handler do?
> 
> Comments and suggestions are welcome!

My reaction re "generic" is that these mechanisms should
be specific to the busses and platforms involved; see my
comments on patch #1.

However this means that for PCI devices, or PCIE, this can
and should be generic enough to work on non-x86 systems.

It's not quite that generic yet, and there are interactions
between PCI/PCIE and ACPI that will need work.  But having
those layers clean is important; and you've got a decent
start on that.

Plus there are various other wake-capable devices, like
the PS2 devices and UART in /proc/acpi/wakeup on one of
my systems:

Device	S-state	  Status   Sysfs node
PCI0	  S4	 disabled  no-bus:pci0000:00
PS2M	  S4	 disabled  pnp:00:05
PS2K	  S4	 disabled  pnp:00:06
UAR1	  S4	 disabled  pnp:00:08
USB1	  S3	 disabled  pci:0000:00:03.0
USB2	  S3	 disabled  pci:0000:00:03.1
USB3	  S3	 disabled  	<-- BIOS bug: no such hardware
USB4	  S3	 disabled  pci:0000:00:03.3
S139	  S4	 disabled  	<-- BIOS bug: no such hardware
LAN	  S4	 disabled  pci:0000:00:04.0
MDM	  S4	 disabled  	<-- MDM and AUD are the same HW??
AUD	  S4	 disabled  pci:0000:00:02.7
SLPB	  S4	*enabled   

But getting a good start on the PCI and ACPI runtime wake
framework will help a lot.

- Dave

RE: [RFC 0/5] device wakeup event support

[Li, Shaohua]

>-----Original Message-----
>From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>owner@vger.kernel.org] On Behalf Of David Brownell
>Sent: Tuesday, September 09, 2008 10:41 AM
>To: Li, Shaohua
>Cc: linux-pm@lists.linux-foundation.org; linux-acpi@vger.kernel.org;
>stern@rowland.harvard.edu
>Subject: Re: [RFC 0/5] device wakeup event support
>
>On Monday 08 September 2008, shaohua.li@intel.com wrote:
>> This series add device wakeup event detection support. This is the base
>to
>> implement runtime device suspend/resume, though we don't support it now.
>> But David said USB is approaching to this. See this bugzilla
>> http://bugzilla.kernel.org/show_bug.cgi?id=6892 for detail.
>
>Glad to see more attention here.  :)
>
>Note that this issue is specific to systems using PCI (at core),
>with additional nuances for PCI systems which use ACPI.
>
>Device wakeup through USB works fine on various non-PCI and
>non-ACPI embedded systems, without needing these patches.
>It's done so for several years now...
>
>
>> The current process to handle wakeup event is:
>> 1. driver enable wakeup event line pme and suspend
>> 2. NPME or ACPI receives wake
>> There are somethings we need discuss:up event
>> 3. NPME or ACPI call .wakeup_event() to clear and disable wakeup
>>    event. Driver can do extra things in .wakeup_event() too.
>> 4. NPME or ACPI call generic wakeup event handler
>(device_receive_wakeup_event())
>> 5. device resumes, and goto 1 for next round of suspend
>>
>> 1. is this generic for other platforms?
>> 2. what should the generic wakeup event handler do?
>>
>> Comments and suggestions are welcome!
>
>My reaction re "generic" is that these mechanisms should
>be specific to the busses and platforms involved; see my
>comments on patch #1.
>
>However this means that for PCI devices, or PCIE, this can
>and should be generic enough to work on non-x86 systems.
>
>It's not quite that generic yet, and there are interactions
>between PCI/PCIE and ACPI that will need work.  But having
>those layers clean is important; and you've got a decent
>start on that.
>
>Plus there are various other wake-capable devices, like
>the PS2 devices and UART in /proc/acpi/wakeup on one of
>my systems:
>
>Device  S-state   Status   Sysfs node
>PCI0      S4     disabled  no-bus:pci0000:00
>PS2M      S4     disabled  pnp:00:05
>PS2K      S4     disabled  pnp:00:06
>UAR1      S4     disabled  pnp:00:08
>USB1      S3     disabled  pci:0000:00:03.0
>USB2      S3     disabled  pci:0000:00:03.1
>USB3      S3     disabled       <-- BIOS bug: no such hardware
>USB4      S3     disabled  pci:0000:00:03.3
>S139      S4     disabled       <-- BIOS bug: no such hardware
>LAN       S4     disabled  pci:0000:00:04.0
>MDM       S4     disabled       <-- MDM and AUD are the same HW??
>AUD       S4     disabled  pci:0000:00:02.7
>SLPB      S4    *enabled
>
>But getting a good start on the PCI and ACPI runtime wake
>framework will help a lot.
The ACPI mechanism works for all devices you mentioned here. It's not just for ACPI device.

Thanks,
Shaohua

[RFC 0/5] device wakeup event support v2

[Shaohua Li]

This series add device wakeup event detection support. This is the base to
implement runtime device suspend/resume, though we don't support it now.
But David said USB is approaching to this. See this bugzilla
http://bugzilla.kernel.org/show_bug.cgi?id=6892 for detail.

changes v1 -> v2:
1. scan pci bridge for PME. Current implementation is just doing scan if target device is a bridge
2. move device_receive_wakeup_event() call to pci, and provide an API (pci_handle_wakeup_event()) for non-ACPI & non-PCIe platform
3. fixed a lot of coding style issues

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
-- 

[RFC 3/5] pci wakeup handler

[Shaohua Li]

[-- Attachment #1: pci-wakeup-event.patch --]
[-- Type: text/plain, Size: 4779 bytes --]

pci subsystem wakeup handler.
---
 drivers/pci/pci-driver.c |   95 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h      |    6 ++
 2 files changed, 101 insertions(+)

Index: linux/drivers/pci/pci-driver.c
===================================================================
--- linux.orig/drivers/pci/pci-driver.c	2008-09-11 10:56:26.000000000 +0800
+++ linux/drivers/pci/pci-driver.c	2008-09-11 11:15:20.000000000 +0800
@@ -472,12 +472,106 @@ static int pci_pm_resume_noirq(struct de
 	return error;
 }
 
+/*
+ * Called when dev is suspected to invoke a wakeup event, return 0 if yes
+ * */
+static bool pci_handle_one_wakeup_event(struct pci_dev *pdev)
+{
+	int pme_pos = pdev->pm_cap;
+	struct pci_driver *drv = pdev->driver;
+	u16 pmcsr;
+	bool spurious = false;
+
+	if (pme_pos == 0) {
+		/*
+		 * Some USB devices haven't PME, but have specific registers to
+		 * control wakeup
+		 */
+		goto out;
+	}
+
+	/* clear PME status and disable PME to avoid interrupt flood */
+	pci_read_config_word(pdev, pme_pos + PCI_PM_CTRL, &pmcsr);
+	if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
+		return false;
+	/* I see spurious PME here, just ignore it for now */
+	if (!(pmcsr & PCI_PM_CTRL_PME_ENABLE))
+		spurious = true;
+	else
+		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
+	pmcsr |= PCI_PM_CTRL_PME_STATUS;
+	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
+
+	if (spurious)
+		return false;
+	return true;
+out:
+	if (drv && drv->pm && drv->pm->base.wakeup_event)
+		return drv->pm->base.wakeup_event(&pdev->dev);
+	return false;
+}
+
+bool pci_handle_wakeup_event(struct pci_dev *target)
+{
+	bool ret;
+	struct pci_dev *tmp = NULL;
+	int domain_nr, bus_start, bus_end;
+
+	/*
+	 * @target could be a bridge or a device.
+	 * PCIe native PME case:
+	 *   @target is device - @target must be the exact device invoking PME
+	 *   @target is a root port or pcie-pci bridge - should scan legacy pci
+	 *	devices under the bridge
+	 * ACPI GPE case:
+	 *   @target is device - AML code could clear PME status before this
+	 *	routine is called, so we can't detect if @target invokes PME.
+	 *	Let's trust AML code
+	 *   @target is bridge - scan devices under the bridge
+	 * So: if target is device, trust the device invokes PME. If target is
+	 * bridge, scan devices under the bridge and only trust device invokes
+	 * PME which we can detect
+	 **/
+	ret = pci_handle_one_wakeup_event(target);
+	if (!target->subordinate || (target->is_pcie &&
+	    target->pcie_type != PCI_EXP_TYPE_ROOT_PORT &&
+	    target->pcie_type != PCI_EXP_TYPE_PCI_BRIDGE)) {
+		/* always trust the device invokes PME even we can't detect */
+		device_receive_wakeup_event(&target->dev);
+		return ret;
+	}
+
+	if (ret)
+		device_receive_wakeup_event(&target->dev);
+
+	domain_nr = pci_domain_nr(target->bus);
+	bus_start = target->subordinate->secondary;
+	bus_end = target->subordinate->subordinate;
+	while ((tmp = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, tmp)) != NULL) {
+		if (pci_domain_nr(tmp->bus) == domain_nr &&
+		   tmp->bus->number >= bus_start &&
+		   tmp->bus->number <= bus_end) {
+			if (pci_handle_one_wakeup_event(tmp)) {
+				ret = true;
+				device_receive_wakeup_event(&tmp->dev);
+			}
+		}
+	}
+	return ret;
+}
+EXPORT_SYMBOL(pci_handle_wakeup_event);
+
+static bool pci_pm_wakeup_event(struct device *dev)
+{
+	return pci_handle_wakeup_event(to_pci_dev(dev));
+}
 #else /* !CONFIG_SUSPEND */
 
 #define pci_pm_suspend		NULL
 #define pci_pm_suspend_noirq	NULL
 #define pci_pm_resume		NULL
 #define pci_pm_resume_noirq	NULL
+#define pci_pm_wakeup_event	NULL
 
 #endif /* !CONFIG_SUSPEND */
 
@@ -651,6 +745,7 @@ struct pm_ext_ops pci_pm_ops = {
 		.thaw = pci_pm_thaw,
 		.poweroff = pci_pm_poweroff,
 		.restore = pci_pm_restore,
+		.wakeup_event = pci_pm_wakeup_event,
 	},
 	.suspend_noirq = pci_pm_suspend_noirq,
 	.resume_noirq = pci_pm_resume_noirq,
Index: linux/include/linux/pci.h
===================================================================
--- linux.orig/include/linux/pci.h	2008-09-11 10:56:26.000000000 +0800
+++ linux/include/linux/pci.h	2008-09-11 10:56:42.000000000 +0800
@@ -646,6 +646,7 @@ int pci_enable_wake(struct pci_dev *dev,
 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_handle_wakeup_event(struct pci_dev *target);
 
 /* Functions for PCI Hotplug drivers to use */
 int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap);
@@ -949,6 +950,11 @@ static inline int pci_enable_wake(struct
 	return 0;
 }
 
+static inline bool pci_handle_wakeup_event(struct pci_dev *target)
+{
+	return false;
+}
+
 static inline int pci_request_regions(struct pci_dev *dev, const char *res_name)
 {
 	return -EIO;

-- 

Re: [RFC 3/5] pci wakeup handler

[Rafael J. Wysocki]

On Thursday, 11 of September 2008, Shaohua Li wrote:
> pci subsystem wakeup handler.

Perhaps add a bit more explanation here - what is introduced, why and why this
particular way.

> ---
>  drivers/pci/pci-driver.c |   95 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h      |    6 ++
>  2 files changed, 101 insertions(+)
> 
> Index: linux/drivers/pci/pci-driver.c
> ===================================================================
> --- linux.orig/drivers/pci/pci-driver.c	2008-09-11 10:56:26.000000000 +0800
> +++ linux/drivers/pci/pci-driver.c	2008-09-11 11:15:20.000000000 +0800
> @@ -472,12 +472,106 @@ static int pci_pm_resume_noirq(struct de
>  	return error;
>  }
>  
> +/*
> + * Called when dev is suspected to invoke a wakeup event, return 0 if yes
> + * */

Use kerneldoc format of the comment, please.

> +static bool pci_handle_one_wakeup_event(struct pci_dev *pdev)
> +{

I don't really like that being a boolean function.  I'd make it return 0 on
success and error code on failure.

> +	int pme_pos = pdev->pm_cap;
> +	struct pci_driver *drv = pdev->driver;
> +	u16 pmcsr;
> +	bool spurious = false;
> +
> +	if (pme_pos == 0) {
> +		/*
> +		 * Some USB devices haven't PME, but have specific registers to
> +		 * control wakeup
> +		 */
> +		goto out;
> +	}
> +
> +	/* clear PME status and disable PME to avoid interrupt flood */
> +	pci_read_config_word(pdev, pme_pos + PCI_PM_CTRL, &pmcsr);
> +	if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
> +		return false;
> +	/* I see spurious PME here, just ignore it for now */
> +	if (!(pmcsr & PCI_PM_CTRL_PME_ENABLE))
> +		spurious = true;
> +	else
> +		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;

If you do this unconditionally, you'll be able to use pci_pme_active() for it.
Actually, you can use pci_pme_enabled() for checking if PME is enabled
and pci_pme_status() for checking if the PME status is set.  Then,
you can remove the reference to the config space from here and use
those low-level callbacks instead of them. 

> +	pmcsr |= PCI_PM_CTRL_PME_STATUS;
> +	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
> +
> +	if (spurious)
> +		return false;
> +	return true;
> +out:
> +	if (drv && drv->pm && drv->pm->base.wakeup_event)
> +		return drv->pm->base.wakeup_event(&pdev->dev);

I'd move this into the 'if (!pme_pos)' block.  And is this what we want really?
In this case the driver's wakeup_event() will be responsible for checking
if the wake-up event is valid etc.

> +	return false;
> +}
> +

Please add a kerneldoc comment and I don't like bool here too.

> +bool pci_handle_wakeup_event(struct pci_dev *target)
> +{
> +	bool ret;
> +	struct pci_dev *tmp = NULL;
> +	int domain_nr, bus_start, bus_end;
> +
> +	/*
> +	 * @target could be a bridge or a device.
> +	 * PCIe native PME case:
> +	 *   @target is device - @target must be the exact device invoking PME
> +	 *   @target is a root port or pcie-pci bridge - should scan legacy pci
> +	 *	devices under the bridge
> +	 * ACPI GPE case:
> +	 *   @target is device - AML code could clear PME status before this
> +	 *	routine is called, so we can't detect if @target invokes PME.
> +	 *	Let's trust AML code
> +	 *   @target is bridge - scan devices under the bridge
> +	 * So: if target is device, trust the device invokes PME. If target is
> +	 * bridge, scan devices under the bridge and only trust device invokes
> +	 * PME which we can detect
> +	 **/

Change this comment into a kerneldoc one before the function, please.

> +	ret = pci_handle_one_wakeup_event(target);
> +	if (!target->subordinate || (target->is_pcie &&
> +	    target->pcie_type != PCI_EXP_TYPE_ROOT_PORT &&
> +	    target->pcie_type != PCI_EXP_TYPE_PCI_BRIDGE)) {
> +		/* always trust the device invokes PME even we can't detect */

More details in the comment, please.

> +		device_receive_wakeup_event(&target->dev);

Why do we use device_receive_wakeup_event() here?

> +		return ret;
> +	}
> +
> +	if (ret)
> +		device_receive_wakeup_event(&target->dev);

And here?  What's the idea?

> +
> +	domain_nr = pci_domain_nr(target->bus);
> +	bus_start = target->subordinate->secondary;
> +	bus_end = target->subordinate->subordinate;
> +	while ((tmp = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, tmp)) != NULL) {
> +		if (pci_domain_nr(tmp->bus) == domain_nr &&
> +		   tmp->bus->number >= bus_start &&
> +		   tmp->bus->number <= bus_end) {

This cascading 'if ()'s don't look good.  I'd probably use 'continue'.

> +			if (pci_handle_one_wakeup_event(tmp)) {
> +				ret = true;
> +				device_receive_wakeup_event(&tmp->dev);

What exactly is the role of device_receive_wakeup_event() here?

> +			}
> +		}
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL(pci_handle_wakeup_event);
> +
> +static bool pci_pm_wakeup_event(struct device *dev)
> +{
> +	return pci_handle_wakeup_event(to_pci_dev(dev));
> +}

What exactly is the point of introducing this function?

>  #else /* !CONFIG_SUSPEND */
>  
>  #define pci_pm_suspend		NULL
>  #define pci_pm_suspend_noirq	NULL
>  #define pci_pm_resume		NULL
>  #define pci_pm_resume_noirq	NULL
> +#define pci_pm_wakeup_event	NULL
>  
>  #endif /* !CONFIG_SUSPEND */
>  
> @@ -651,6 +745,7 @@ struct pm_ext_ops pci_pm_ops = {
>  		.thaw = pci_pm_thaw,
>  		.poweroff = pci_pm_poweroff,
>  		.restore = pci_pm_restore,
> +		.wakeup_event = pci_pm_wakeup_event,
>  	},
>  	.suspend_noirq = pci_pm_suspend_noirq,
>  	.resume_noirq = pci_pm_resume_noirq,
> Index: linux/include/linux/pci.h
> ===================================================================
> --- linux.orig/include/linux/pci.h	2008-09-11 10:56:26.000000000 +0800
> +++ linux/include/linux/pci.h	2008-09-11 10:56:42.000000000 +0800
> @@ -646,6 +646,7 @@ int pci_enable_wake(struct pci_dev *dev,
>  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_handle_wakeup_event(struct pci_dev *target);
>  
>  /* Functions for PCI Hotplug drivers to use */
>  int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap);
> @@ -949,6 +950,11 @@ static inline int pci_enable_wake(struct
>  	return 0;
>  }
>  
> +static inline bool pci_handle_wakeup_event(struct pci_dev *target)
> +{
> +	return false;
> +}
> +
>  static inline int pci_request_regions(struct pci_dev *dev, const char *res_name)
>  {
>  	return -EIO;
> 

Thanks,
Rafael

Re: [RFC 3/5] pci wakeup handler

[Shaohua Li]

On Mon, Oct 20, 2008 at 03:50:40AM +0800, Rafael J. Wysocki wrote:
> On Thursday, 11 of September 2008, Shaohua Li wrote:
> > pci subsystem wakeup handler.
> 
> Perhaps add a bit more explanation here - what is introduced, why and why this
> particular way.
I'll add a kernel doc in later post.

> > +static bool pci_handle_one_wakeup_event(struct pci_dev *pdev)
> > +{
> 
> I don't really like that being a boolean function.  I'd make it return 0 on
> success and error code on failure.
Oh, in my previous post, somebody like a boolean and then you like an int
in the mail list. Either is ok to me, but I'd like to have a reason
instead of a 'like' or 'unlike'.

> > +     /* clear PME status and disable PME to avoid interrupt flood */
> > +     pci_read_config_word(pdev, pme_pos + PCI_PM_CTRL, &pmcsr);
> > +     if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
> > +             return false;
> > +     /* I see spurious PME here, just ignore it for now */
> > +     if (!(pmcsr & PCI_PM_CTRL_PME_ENABLE))
> > +             spurious = true;
> > +     else
> > +             pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
> 
> If you do this unconditionally, you'll be able to use pci_pme_active() for it.
> Actually, you can use pci_pme_enabled() for checking if PME is enabled
> and pci_pme_status() for checking if the PME status is set.  Then,
> you can remove the reference to the config space from here and use
> those low-level callbacks instead of them.
ok.
> > +     pmcsr |= PCI_PM_CTRL_PME_STATUS;
> > +     pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
> > +
> > +     if (spurious)
> > +             return false;
> > +     return true;
> > +out:
> > +     if (drv && drv->pm && drv->pm->base.wakeup_event)
> > +             return drv->pm->base.wakeup_event(&pdev->dev);
> 
> I'd move this into the 'if (!pme_pos)' block.  And is this what we want really?
> In this case the driver's wakeup_event() will be responsible for checking
> if the wake-up event is valid etc.
Yes, device driver should check if a wake-up event is valid.

> > +             device_receive_wakeup_event(&target->dev);
> 
> Why do we use device_receive_wakeup_event() here?
the device receives wakeup event, so it should do something.

> 
> > +             return ret;
> > +     }
> > +
> > +     if (ret)
> > +             device_receive_wakeup_event(&target->dev);
> 
> And here?  What's the idea?
ditto
 
> > +                             device_receive_wakeup_event(&tmp->dev);
> 
> What exactly is the role of device_receive_wakeup_event() here?
ditto

> > +                     }
> > +             }
> > +     }
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(pci_handle_wakeup_event);
> > +
> > +static bool pci_pm_wakeup_event(struct device *dev)
> > +{
> > +     return pci_handle_wakeup_event(to_pci_dev(dev));
> > +}
> 
> What exactly is the point of introducing this function?
David said other archs (embedded system) might require it.

Thanks,
Shaohua

Re: [RFC 3/5] pci wakeup handler

[Rafael J. Wysocki]

On Wednesday, 22 of October 2008, Shaohua Li wrote:
> On Mon, Oct 20, 2008 at 03:50:40AM +0800, Rafael J. Wysocki wrote:
> > On Thursday, 11 of September 2008, Shaohua Li wrote:
> > > pci subsystem wakeup handler.
> > 
> > Perhaps add a bit more explanation here - what is introduced, why and why this
> > particular way.
> I'll add a kernel doc in later post.
> 
> > > +static bool pci_handle_one_wakeup_event(struct pci_dev *pdev)
> > > +{
> > 
> > I don't really like that being a boolean function.  I'd make it return 0 on
> > success and error code on failure.
> Oh, in my previous post, somebody like a boolean and then you like an int
> in the mail list. Either is ok to me, but I'd like to have a reason
> instead of a 'like' or 'unlike'.

That was probably me, but in a different context. ;-)

Use 'bool' for functions that are intended as boolean, eg.
'system_entering_hibernation()' will return 'true' if the system is entering
hibernation at the moment and 'false' otherwise, but for functions like
pci_handle_one_wakeup_event() the standard it to return 0 on success, so IMO
we should follow the standard.

HTH