[RFC 1/5] devcore introduce wakeup_event callback

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


>  };
>  
>  /**

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

[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

[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

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

[RFC 1/5] devcore introduce wakeup_event callback

[Shaohua Li]

[-- Attachment #1: devcore-introduce-wakeup_event.patch --]
[-- Type: text/plain, Size: 1431 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 |    7 +++++++
 1 file changed, 7 insertions(+)

Index: linux/include/linux/pm.h
===================================================================
--- linux.orig/include/linux/pm.h	2008-09-11 10:56:27.000000000 +0800
+++ linux/include/linux/pm.h	2008-09-11 10:56:29.000000000 +0800
@@ -125,6 +125,12 @@ 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. In bus level, the op might
+ *	check all devices under the bus and call device_receive_wakeup_event()
+ *	for devices which invoke wakeup event. In device level, the op just
+ *	returns if a wakeup event occurs. Note, if device follows standard
+ *	mechanism for wakeup which bus level can handle, device level op can be
+ *	empty.
  *
  * All of the above callbacks, except for @complete(), return error codes.
  * However, the error codes returned by the resume operations, @resume(),
@@ -151,6 +157,7 @@ struct pm_ops {
 	int (*thaw)(struct device *dev);
 	int (*poweroff)(struct device *dev);
 	int (*restore)(struct device *dev);
+	bool (*wakeup_event)(struct device *dev);
 };
 
 /**

-- 

[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-11 10:56:26.000000000 +0800
+++ linux/drivers/base/power/main.c	2008-09-11 10:56:39.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-11 10:56:29.000000000 +0800
+++ linux/include/linux/pm.h	2008-09-11 10:56:39.000000000 +0800
@@ -440,6 +440,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: 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;

-- 

[RFC 4/5] PCIe native PME detection

[Shaohua Li]

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

PCIe defines a native PME detection mechanism. When a PCIe endpoint invokes PME, PCIe root port has a set of regisets to detect the endpoint's bus/device/function number and root port will send out interrupt when PME is received. See PCIe spec for detail. This patch implements this feature.

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

Index: linux/drivers/pci/pcie/Kconfig
===================================================================
--- linux.orig/drivers/pci/pcie/Kconfig	2008-09-11 11:27:44.000000000 +0800
+++ linux/drivers/pci/pcie/Kconfig	2008-09-11 11:28:39.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-11 11:27:44.000000000 +0800
+++ linux/drivers/pci/pcie/Makefile	2008-09-11 11:28:39.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-11 11:30:26.000000000 +0800
@@ -0,0 +1,312 @@
+/*
+ * 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_enable_pme(struct pci_dev *pdev, bool enable)
+{
+	int pos;
+	u16 rtctl;
+
+	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
+
+	pci_read_config_word(pdev, pos + PCI_EXP_RTCTL, &rtctl);
+	if (!enable)
+		rtctl &= ~PCI_EXP_RTCTL_PMEIE;
+	else
+		rtctl |= PCI_EXP_RTCTL_PMEIE;
+	pci_write_config_word(pdev, pos + PCI_EXP_RTCTL, rtctl);
+}
+
+static inline void npme_clear_pme(struct pci_dev *pdev)
+{
+	int pos;
+	u32 rtsta;
+
+	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
+
+	pci_read_config_dword(pdev, pos + PCI_EXP_RTSTA, &rtsta);
+	rtsta |= PCI_EXP_RTSTA_PME;
+	pci_write_config_dword(pdev, pos + PCI_EXP_RTSTA, rtsta);
+}
+
+static bool npme_pme_target(struct pci_dev *target)
+{
+	bool ret = false;
+	if (target->dev.bus->pm && target->dev.bus->pm->base.wakeup_event)
+		ret = target->dev.bus->pm->base.wakeup_event(&target->dev);
+	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;
+	bool has_dev = false;
+
+	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)
+		has_dev = 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_enable_pme(dev->port, true);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	if (!has_dev)
+		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 rtsta;
+	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, &rtsta);
+	if (!(rtsta & PCI_EXP_RTSTA_PME)) {
+		spin_unlock_irqrestore(&data->lock, flags);
+		return IRQ_NONE;
+	}
+
+	data->bdf = (u16)rtsta;
+
+	/* disable PME to avoid interrupt flood */
+	npme_enable_pme(pdev, false);
+	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
+static inline int npme_osc_setup(struct pcie_device *pciedev)
+{
+	return 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;
+	set_service_data(dev, data);
+
+	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_enable_pme(pdev, true);
+
+	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_enable_pme(pdev, false);
+	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_enable_pme(pdev, false);
+
+	/* 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_enable_pme(pdev, true);
+	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-11 11:27:44.000000000 +0800
+++ linux/include/linux/pci_regs.h	2008-09-11 11:28:39.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)

-- 

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

[Shaohua Li]

[-- Attachment #1: acpi-gpe.patch --]
[-- Type: text/plain, Size: 4728 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 |   60 ++++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h     |    4 ++
 4 files changed, 88 insertions(+)

Index: linux/drivers/acpi/Kconfig
===================================================================
--- linux.orig/drivers/acpi/Kconfig	2008-09-11 10:56:25.000000000 +0800
+++ linux/drivers/acpi/Kconfig	2008-09-11 10:56:47.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-11 10:56:25.000000000 +0800
+++ linux/drivers/acpi/bus.c	2008-09-11 10:56:47.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-11 10:56:25.000000000 +0800
+++ linux/drivers/acpi/sleep/wakeup.c	2008-09-11 10:56:47.000000000 +0800
@@ -142,6 +142,64 @@ 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);
+
+	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 +225,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-11 10:56:25.000000000 +0800
+++ linux/include/acpi/acpi_bus.h	2008-09-11 10:56:47.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 2/5] devcore adds generic wakeup event handler

[Bjorn Helgaas]

On Thursday 11 September 2008 12:30:39 am Shaohua Li wrote:
> The default action to handle wakeup event. Currently just prints something,
> maybe we should call .resume(). The routine will be called in task context.
> ...

> +void device_receive_wakeup_event(struct device *dev)
> +{
> +	printk("Device %s invokes wakeup event\n", dev->bus_id);

Please use dev_printk(), e.g., dev_info() in this case.  Also
applies to patches 4 and 5.

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

[Rafael J. Wysocki]

On Thursday, 11 of September 2008, Shaohua Li 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.
> 
> 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>

Thanks for posting the update.

Unfortunately, I won't be able to review the patches until after I get back
from the Kernel Summit.

Thanks,
Rafael

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

[Shaohua Li]

On Mon, 2008-09-15 at 07:50 +0800, Rafael J. Wysocki wrote:
> On Thursday, 11 of September 2008, Shaohua Li 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.
> >
> > 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>
> 
> Thanks for posting the update.
> 
> Unfortunately, I won't be able to review the patches until after I get back
> from the Kernel Summit.
Any update on this?

Thanks,
Shaohua

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

[Rafael J. Wysocki]

On Thursday, 11 of September 2008, Shaohua Li wrote:
> 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 |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> Index: linux/include/linux/pm.h
> ===================================================================
> --- linux.orig/include/linux/pm.h	2008-09-11 10:56:27.000000000 +0800
> +++ linux/include/linux/pm.h	2008-09-11 10:56:29.000000000 +0800
> @@ -125,6 +125,12 @@ 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. In bus level, the op might
> + *	check all devices under the bus and call device_receive_wakeup_event()
> + *	for devices which invoke wakeup event. In device level, the op just
> + *	returns if a wakeup event occurs. Note, if device follows standard
> + *	mechanism for wakeup which bus level can handle, device level op can be
> + *	empty.
>   *
>   * All of the above callbacks, except for @complete(), return error codes.
>   * However, the error codes returned by the resume operations, @resume(),
> @@ -151,6 +157,7 @@ struct pm_ops {
>  	int (*thaw)(struct device *dev);
>  	int (*poweroff)(struct device *dev);
>  	int (*restore)(struct device *dev);
> +	bool (*wakeup_event)(struct device *dev);
>  };
>  
>  /**

I think it will be better to place wakeup_event() in 'struct device' itself
rather than here.

Generally, the 'struct pm_ops' thing (after the simplification patch queued up
for .29 that will be 'struct dev_pm_ops') will depend on PM_SLEEP and
wakeup_event() is a run-time thing.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Thursday, 11 of September 2008, Shaohua Li wrote:
> 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-11 10:56:26.000000000 +0800
> +++ linux/drivers/base/power/main.c	2008-09-11 10:56:39.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-11 10:56:29.000000000 +0800
> +++ linux/include/linux/pm.h	2008-09-11 10:56:39.000000000 +0800
> @@ -440,6 +440,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

Do you anticipate any particular use of this function?

Rafael

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

[Rafael J. Wysocki]

On Sunday, 19 of October 2008, Rafael J. Wysocki wrote:
> On Thursday, 11 of September 2008, Shaohua Li wrote:
> > 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 |    7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > Index: linux/include/linux/pm.h
> > ===================================================================
> > --- linux.orig/include/linux/pm.h	2008-09-11 10:56:27.000000000 +0800
> > +++ linux/include/linux/pm.h	2008-09-11 10:56:29.000000000 +0800
> > @@ -125,6 +125,12 @@ 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. In bus level, the op might
> > + *	check all devices under the bus and call device_receive_wakeup_event()
> > + *	for devices which invoke wakeup event. In device level, the op just
> > + *	returns if a wakeup event occurs. Note, if device follows standard
> > + *	mechanism for wakeup which bus level can handle, device level op can be
> > + *	empty.
> >   *
> >   * All of the above callbacks, except for @complete(), return error codes.
> >   * However, the error codes returned by the resume operations, @resume(),
> > @@ -151,6 +157,7 @@ struct pm_ops {
> >  	int (*thaw)(struct device *dev);
> >  	int (*poweroff)(struct device *dev);
> >  	int (*restore)(struct device *dev);
> > +	bool (*wakeup_event)(struct device *dev);
> >  };
> >  
> >  /**
> 
> I think it will be better to place wakeup_event() in 'struct device' itself
> rather than here.

That should be 'struct device_driver' actually, sorry.

Thanks,
Rafael

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 4/5] PCIe native PME detection

[Rafael J. Wysocki]

On Thursday, 11 of September 2008, Shaohua Li wrote:
> PCIe defines a native PME detection mechanism. When a PCIe endpoint invokes PME, PCIe root port has a set of regisets to detect the endpoint's bus/device/function number and root port will send out interrupt when PME is received. See PCIe spec for detail. This patch implements this feature.

Any details of the implementation?

> ---
>  drivers/pci/pcie/Kconfig  |    7 +
>  drivers/pci/pcie/Makefile |    2 
>  drivers/pci/pcie/npme.c   |  312 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci_regs.h  |    1 
>  4 files changed, 322 insertions(+)
> 
> Index: linux/drivers/pci/pcie/Kconfig
> ===================================================================
> --- linux.orig/drivers/pci/pcie/Kconfig	2008-09-11 11:27:44.000000000 +0800
> +++ linux/drivers/pci/pcie/Kconfig	2008-09-11 11:28:39.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-11 11:27:44.000000000 +0800
> +++ linux/drivers/pci/pcie/Makefile	2008-09-11 11:28:39.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-11 11:30:26.000000000 +0800
> @@ -0,0 +1,312 @@
> +/*
> + * 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_enable_pme(struct pci_dev *pdev, bool enable)

This works in analogy with pci_pme_active(), so it would seem reasonable to
call it npme_pme_active(), although pcie_npme_active() would be even better
IMO.

> +{
> +	int pos;
> +	u16 rtctl;
> +
> +	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
> +

The result of this call seems to be a good candidate for caching in
'struct pci_dev'.

> +	pci_read_config_word(pdev, pos + PCI_EXP_RTCTL, &rtctl);
> +	if (!enable)
> +		rtctl &= ~PCI_EXP_RTCTL_PMEIE;
> +	else
> +		rtctl |= PCI_EXP_RTCTL_PMEIE;
> +	pci_write_config_word(pdev, pos + PCI_EXP_RTCTL, rtctl);
> +}
> +
> +static inline void npme_clear_pme(struct pci_dev *pdev)

pcie_npme_clear_status() perhaps?

> +{
> +	int pos;
> +	u32 rtsta;
> +
> +	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);

Check if we found it?

> +
> +	pci_read_config_dword(pdev, pos + PCI_EXP_RTSTA, &rtsta);
> +	rtsta |= PCI_EXP_RTSTA_PME;
> +	pci_write_config_dword(pdev, pos + PCI_EXP_RTSTA, rtsta);
> +}
> +
> +static bool npme_pme_target(struct pci_dev *target)
> +{
> +	bool ret = false;
> +	if (target->dev.bus->pm && target->dev.bus->pm->base.wakeup_event)
> +		ret = target->dev.bus->pm->base.wakeup_event(&target->dev);
> +	return ret;
> +}

This apparently only calls the device bus type's wakeup_event() method, so
perhaps give it a better name (pcie_npme_bus_callback() maybe?).

> +
> +static void npme_work_handle(struct work_struct *work)
> +{
> +	struct npme_data *data = container_of(work, struct npme_data, work);

Is 'data' guaranteed to be not NULL?

> +	struct pcie_device *dev = data->dev;
> +	unsigned long flags;
> +	struct pci_dev *target;
> +	bool has_dev = false;
> +
> +	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);

Is 'dev' guaranteed to be not NULL?

> +		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)
> +		has_dev = npme_pme_target(target);

What's the meaning of 'has_dev'?  It seems to be the result of the bus type
callback.

> +	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_enable_pme(dev->port, true);

What does data->exit different from zero mean at this point?

> +
> +	spin_unlock_irqrestore(&data->lock, flags);
> +
> +	if (!has_dev)
> +		printk(KERN_ERR"Spurious Native PME interrupt %d received\n",
> +			dev->irq);
> +
> +	if (target)
> +		pci_dev_put(target);
> +}
> +

Add a kerneldoc comment, please.

> +static irqreturn_t npme_irq(int irq, void *context)
> +{
> +	int pos;
> +	struct pci_dev *pdev;
> +	u32 rtsta;
> +	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, &rtsta);
> +	if (!(rtsta & PCI_EXP_RTSTA_PME)) {
> +		spin_unlock_irqrestore(&data->lock, flags);
> +		return IRQ_NONE;
> +	}
> +
> +	data->bdf = (u16)rtsta;

Hm, couldn't we use pci_read_config_word() above instead?

> +
> +	/* disable PME to avoid interrupt flood */
> +	npme_enable_pme(pdev, false);
> +	spin_unlock_irqrestore(&data->lock, flags);
> +
> +	schedule_work(&data->work);

I'm not sure if the workqueue is exactly suitable for that.  Have you
considered using anything else?

> +
> +	return IRQ_HANDLED;
> +}
> +
> +#ifdef CONFIG_ACPI

Add a kerneldoc comment, please.  Also, this should go into a separate file
with 'acpi' in its name IMO.

> +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
> +static inline int npme_osc_setup(struct pcie_device *pciedev)
> +{
> +	return 0;
> +}
> +#endif
> +

Add a kerneldoc comment, please.
Also, I'd call it differently, like pcie_npme_probe().

> +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;
> +	set_service_data(dev, data);
> +
> +	pdev = dev->port;
> +
> +	/* clear pending PME */
> +	npme_clear_pme(pdev);
> +
> +	status = request_irq(dev->irq, npme_irq, IRQF_SHARED, "npme", dev);

Who's going to set dev->irq?

> +	if (status) {
> +		kfree(data);
> +		return status;
> +	}
> +
> +	/* enable PME interrupt */
> +	npme_enable_pme(pdev, true);
> +
> +	return 0;
> +}
> +
> +static void npme_remove(struct pcie_device *dev)

pcie_npme_remove() ?

> +{
> +	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_enable_pme(pdev, false);
> +	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);
> +}
> +

pcie_npme_suspend() ?

> +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_enable_pme(pdev, false);

Won't this cause a regression on systems that use the native PME mechanism
for wake-up (I have one of these)?

> +	/* clear pending PME */
> +	npme_clear_pme(pdev);
> +	spin_unlock_irqrestore(&data->lock, flags);
> +
> +	return 0;
> +}
> +

pcie_npme_resume() ?

> +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_enable_pme(pdev, true);
> +	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-11 11:27:44.000000000 +0800
> +++ linux/include/linux/pci_regs.h	2008-09-11 11:28:39.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)
> 

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Thursday, 11 of September 2008, Shaohua Li wrote:
> In ACPI platform, if native PME isn't enabled, GPE is used to report wakeup event.

Add more details here, please.

> ---
>  drivers/acpi/Kconfig        |    9 ++++++
>  drivers/acpi/bus.c          |   15 +++++++++++
>  drivers/acpi/sleep/wakeup.c |   60 ++++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/acpi_bus.h     |    4 ++
>  4 files changed, 88 insertions(+)
> 
> Index: linux/drivers/acpi/Kconfig
> ===================================================================
> --- linux.orig/drivers/acpi/Kconfig	2008-09-11 10:56:25.000000000 +0800
> +++ linux/drivers/acpi/Kconfig	2008-09-11 10:56:47.000000000 +0800
> @@ -45,6 +45,15 @@ config ACPI_SLEEP
>  	depends on PM_SLEEP
>  	default y
>  
> +config ACPI_GPE_WAKEUP

I'd call it ACPI_RUNTIME_WAKEUP

> +	bool "ACPI wakeup event support"
> +	depends on PM_SLEEP && EXPERIMENTAL
> +	help
> +	  Enable ACPI to detect wakeup event.

+ Enable ACPI to detect run-time wake-up events.

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

+ at run time.

> +
> +
>  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-11 10:56:25.000000000 +0800
> +++ linux/drivers/acpi/bus.c	2008-09-11 10:56:47.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);
> +

We were talking about removing the notifier last time.  Please do that.

>  /**
>   * 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-11 10:56:25.000000000 +0800
> +++ linux/drivers/acpi/sleep/wakeup.c	2008-09-11 10:56:47.000000000 +0800
> @@ -142,6 +142,64 @@ void acpi_disable_wakeup_device(u8 sleep
>  	spin_unlock(&acpi_device_lock);
>  }

Please put that into a separate file.

> +#ifdef CONFIG_ACPI_GPE_WAKEUP

Add a kerneldoc comment, please.

> +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);
> +
> +	put_device(ldev);
> +	return 0;
> +}
> +

Ditto.

> +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 +225,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-11 10:56:25.000000000 +0800
> +++ linux/include/acpi/acpi_bus.h	2008-09-11 10:56:47.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
>   */
> 

I understand from the above that devices having their own wake-up GPEs will be
handled.  However, it still is completely unclear to me what happens with
devices that can generate PME# and for which there are no specific GPEs, like
any devices on add-in cards.

Thanks,
Rafael

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

[Shaohua Li]

On Mon, Oct 20, 2008 at 03:04:16AM +0800, Rafael J. Wysocki wrote:
> On Thursday, 11 of September 2008, Shaohua Li wrote:
> > 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 |    7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > Index: linux/include/linux/pm.h
> > ===================================================================
> > --- linux.orig/include/linux/pm.h     2008-09-11 10:56:27.000000000 +0800
> > +++ linux/include/linux/pm.h  2008-09-11 10:56:29.000000000 +0800
> > @@ -125,6 +125,12 @@ 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. In bus level, the op might
> > + *   check all devices under the bus and call device_receive_wakeup_event()
> > + *   for devices which invoke wakeup event. In device level, the op just
> > + *   returns if a wakeup event occurs. Note, if device follows standard
> > + *   mechanism for wakeup which bus level can handle, device level op can be
> > + *   empty.
> >   *
> >   * All of the above callbacks, except for @complete(), return error codes.
> >   * However, the error codes returned by the resume operations, @resume(),
> > @@ -151,6 +157,7 @@ struct pm_ops {
> >       int (*thaw)(struct device *dev);
> >       int (*poweroff)(struct device *dev);
> >       int (*restore)(struct device *dev);
> > +     bool (*wakeup_event)(struct device *dev);
> >  };
> >
> >  /**
> 
> I think it will be better to place wakeup_event() in 'struct device' itself
> rather than here.
> 
> Generally, the 'struct pm_ops' thing (after the simplification patch queued up
> for .29 that will be 'struct dev_pm_ops') will depend on PM_SLEEP and
> wakeup_event() is a run-time thing.
ok

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

[Shaohua Li]

On Mon, Oct 20, 2008 at 03:06:58AM +0800, Rafael J. Wysocki wrote:
> On Thursday, 11 of September 2008, Shaohua Li wrote:
> > 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-11 10:56:26.000000000 +0800
> > +++ linux/drivers/base/power/main.c   2008-09-11 10:56:39.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-11 10:56:29.000000000 +0800
> > +++ linux/include/linux/pm.h  2008-09-11 10:56:39.000000000 +0800
> > @@ -440,6 +440,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
> 
> Do you anticipate any particular use of this function?
maybe calls .resume() to wakeup a device.

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 4/5] PCIe native PME detection

[Shaohua Li]

On Mon, Oct 20, 2008 at 04:30:02AM +0800, Rafael J. Wysocki wrote:
> On Thursday, 11 of September 2008, Shaohua Li wrote:
> > PCIe defines a native PME detection mechanism. When a PCIe endpoint invokes PME, PCIe root port has a set of regisets to detect the endpoint's bus/device/function number and root port will send out interrupt when PME is received. See PCIe spec for detail. This patch implements this feature.
> 
> Any details of the implementation?
> 
> > ---
> >  drivers/pci/pcie/Kconfig  |    7 +
> >  drivers/pci/pcie/Makefile |    2
> >  drivers/pci/pcie/npme.c   |  312 ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci_regs.h  |    1
> >  4 files changed, 322 insertions(+)
> >
> > Index: linux/drivers/pci/pcie/Kconfig
> > ===================================================================
> > --- linux.orig/drivers/pci/pcie/Kconfig       2008-09-11 11:27:44.000000000 +0800
> > +++ linux/drivers/pci/pcie/Kconfig    2008-09-11 11:28:39.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-11 11:27:44.000000000 +0800
> > +++ linux/drivers/pci/pcie/Makefile   2008-09-11 11:28:39.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-11 11:30:26.000000000 +0800
> > @@ -0,0 +1,312 @@
> > +/*
> > + * 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_enable_pme(struct pci_dev *pdev, bool enable)
> 
> This works in analogy with pci_pme_active(), so it would seem reasonable to
> call it npme_pme_active(), although pcie_npme_active() would be even better
> IMO.
ok, I'll change the name of the function and blow as you suggested.
  
> > +{
> > +     int pos;
> > +     u16 rtctl;
> > +
> > +     pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
> > +
> 
> The result of this call seems to be a good candidate for caching in
> 'struct pci_dev'.
this isn't frequently called, doesn't need cache.

> > +{
> > +     int pos;
> > +     u32 rtsta;
> > +
> > +     pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
> 
> Check if we found it?
this is a pcie root port, so it always has the PCI_CAP_ID_EXP 

> > +static bool npme_pme_target(struct pci_dev *target)
> > +{
> > +     bool ret = false;
> > +     if (target->dev.bus->pm && target->dev.bus->pm->base.wakeup_event)
> > +             ret = target->dev.bus->pm->base.wakeup_event(&target->dev);
> > +     return ret;
> > +}
> 
> This apparently only calls the device bus type's wakeup_event() method, so
> perhaps give it a better name (pcie_npme_bus_callback() maybe?).
ok 
> > +
> > +static void npme_work_handle(struct work_struct *work)
> > +{
> > +     struct npme_data *data = container_of(work, struct npme_data, work);
> 
> Is 'data' guaranteed to be not NULL?
should be
> > +     struct pcie_device *dev = data->dev;
> > +     unsigned long flags;
> > +     struct pci_dev *target;
> > +     bool has_dev = false;
> > +
> > +     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);
> 
> Is 'dev' guaranteed to be not NULL?
should be
> > +             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)
> > +             has_dev = npme_pme_target(target);
> 
> What's the meaning of 'has_dev'?  It seems to be the result of the bus type
> callback.
maybe I should rename it as found_dev
> > +     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_enable_pme(dev->port, true);
> 
> What does data->exit different from zero mean at this point?
the driver is exitting. I'll rename it

> > +static irqreturn_t npme_irq(int irq, void *context)
> > +{
> > +     int pos;
> > +     struct pci_dev *pdev;
> > +     u32 rtsta;
> > +     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, &rtsta);
> > +     if (!(rtsta & PCI_EXP_RTSTA_PME)) {
> > +             spin_unlock_irqrestore(&data->lock, flags);
> > +             return IRQ_NONE;
> > +     }
> > +
> > +     data->bdf = (u16)rtsta;
> 
> Hm, couldn't we use pci_read_config_word() above instead?
but it actually is a dword. 
> > +
> > +     /* disable PME to avoid interrupt flood */
> > +     npme_enable_pme(pdev, false);
> > +     spin_unlock_irqrestore(&data->lock, flags);
> > +
> > +     schedule_work(&data->work);
> 
> I'm not sure if the workqueue is exactly suitable for that.  Have you
> considered using anything else?
we will call driver's .wakeup_event(), which might call into .resume() from device_receive_wakeup_event()
so workqueue is best fit here.
> > +     /* clear pending PME */
> > +     npme_clear_pme(pdev);
> > +
> > +     status = request_irq(dev->irq, npme_irq, IRQF_SHARED, "npme", dev);
> 
> Who's going to set dev->irq?
the pcie port driver.
> > +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_enable_pme(pdev, false);
> 
> Won't this cause a regression on systems that use the native PME mechanism
> for wake-up (I have one of these)?
good point. currently I don't know if a npme interrupt can wakeup system from
suspend/resume, because npme interrupt looks like usual device interrupt.
Need more invistigation here.

Thanks,
Shaohua

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

[Shaohua Li]

On Mon, Oct 20, 2008 at 04:39:47AM +0800, Rafael J. Wysocki wrote:
> On Thursday, 11 of September 2008, Shaohua Li wrote:
> > In ACPI platform, if native PME isn't enabled, GPE is used to report wakeup event.
> 
> Add more details here, please.
> 
> > ---
> >  drivers/acpi/Kconfig        |    9 ++++++
> >  drivers/acpi/bus.c          |   15 +++++++++++
> >  drivers/acpi/sleep/wakeup.c |   60 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/acpi/acpi_bus.h     |    4 ++
> >  4 files changed, 88 insertions(+)
> >
> > Index: linux/drivers/acpi/Kconfig
> > ===================================================================
> > --- linux.orig/drivers/acpi/Kconfig   2008-09-11 10:56:25.000000000 +0800
> > +++ linux/drivers/acpi/Kconfig        2008-09-11 10:56:47.000000000 +0800
> > @@ -45,6 +45,15 @@ config ACPI_SLEEP
> >       depends on PM_SLEEP
> >       default y
> >
> > +config ACPI_GPE_WAKEUP
> 
> I'd call it ACPI_RUNTIME_WAKEUP
ok

> > +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);
> > +
> 
> We were talking about removing the notifier last time.  Please do that.
Did you see my comments on the issue last time? ACPI test tree already introduces
the mechanism for other purpose, and we can just use it.

> > --- linux.orig/include/acpi/acpi_bus.h        2008-09-11 10:56:25.000000000 +0800
> > +++ linux/include/acpi/acpi_bus.h     2008-09-11 10:56:47.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
> >   */
> >
> 
> I understand from the above that devices having their own wake-up GPEs will be
> handled.  However, it still is completely unclear to me what happens with
> devices that can generate PME# and for which there are no specific GPEs, like
> any devices on add-in cards.
As we discussed last time, pci bus for add-in cards will invoke a gpe, and
this new implementation will check all pci devices under a bridge to try to find
a device generating PME. This should work for add-in cards.

Thanks,
Shaohua

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

[Rafael J. Wysocki]

On Wednesday, 22 of October 2008, Shaohua Li wrote:
> On Mon, Oct 20, 2008 at 03:06:58AM +0800, Rafael J. Wysocki wrote:
> > On Thursday, 11 of September 2008, Shaohua Li wrote:
> > > 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-11 10:56:26.000000000 +0800
> > > +++ linux/drivers/base/power/main.c   2008-09-11 10:56:39.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-11 10:56:29.000000000 +0800
> > > +++ linux/include/linux/pm.h  2008-09-11 10:56:39.000000000 +0800
> > > @@ -440,6 +440,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
> > 
> > Do you anticipate any particular use of this function?
> maybe calls .resume() to wakeup a device.

Well, resume() is specific to system sleep state transitions, so it seems we
need a separate driver callback here.

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

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

[Rafael J. Wysocki]

On Wednesday, 22 of October 2008, Shaohua Li wrote:
> On Mon, Oct 20, 2008 at 04:30:02AM +0800, Rafael J. Wysocki wrote:
> > On Thursday, 11 of September 2008, Shaohua Li wrote:
> > > PCIe defines a native PME detection mechanism. When a PCIe endpoint invokes PME, PCIe root port has a set of regisets to detect the endpoint's bus/device/function number and root port will send out interrupt when PME is received. See PCIe spec for detail. This patch implements this feature.
> > 
> > Any details of the implementation?
> > 
> > > ---
> > >  drivers/pci/pcie/Kconfig  |    7 +
> > >  drivers/pci/pcie/Makefile |    2
> > >  drivers/pci/pcie/npme.c   |  312 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/pci_regs.h  |    1
> > >  4 files changed, 322 insertions(+)
> > >
> > > Index: linux/drivers/pci/pcie/Kconfig
> > > ===================================================================
> > > --- linux.orig/drivers/pci/pcie/Kconfig       2008-09-11 11:27:44.000000000 +0800
> > > +++ linux/drivers/pci/pcie/Kconfig    2008-09-11 11:28:39.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-11 11:27:44.000000000 +0800
> > > +++ linux/drivers/pci/pcie/Makefile   2008-09-11 11:28:39.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-11 11:30:26.000000000 +0800
> > > @@ -0,0 +1,312 @@
> > > +/*
> > > + * 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_enable_pme(struct pci_dev *pdev, bool enable)
> > 
> > This works in analogy with pci_pme_active(), so it would seem reasonable to
> > call it npme_pme_active(), although pcie_npme_active() would be even better
> > IMO.
> ok, I'll change the name of the function and blow as you suggested.
>   
> > > +{
> > > +     int pos;
> > > +     u16 rtctl;
> > > +
> > > +     pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
> > > +
> > 
> > The result of this call seems to be a good candidate for caching in
> > 'struct pci_dev'.
> this isn't frequently called, doesn't need cache.

But the code to read it is duplicated in several places.  Perhaps add a helper
function, then?

> > > +{
> > > +     int pos;
> > > +     u32 rtsta;
> > > +
> > > +     pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
> > 
> > Check if we found it?
> this is a pcie root port, so it always has the PCI_CAP_ID_EXP 

OK
 
> > > +static bool npme_pme_target(struct pci_dev *target)
> > > +{
> > > +     bool ret = false;
> > > +     if (target->dev.bus->pm && target->dev.bus->pm->base.wakeup_event)
> > > +             ret = target->dev.bus->pm->base.wakeup_event(&target->dev);
> > > +     return ret;
> > > +}
> > 
> > This apparently only calls the device bus type's wakeup_event() method, so
> > perhaps give it a better name (pcie_npme_bus_callback() maybe?).
> ok 
> > > +
> > > +static void npme_work_handle(struct work_struct *work)
> > > +{
> > > +     struct npme_data *data = container_of(work, struct npme_data, work);
> > 
> > Is 'data' guaranteed to be not NULL?
> should be
> > > +     struct pcie_device *dev = data->dev;
> > > +     unsigned long flags;
> > > +     struct pci_dev *target;
> > > +     bool has_dev = false;
> > > +
> > > +     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);
> > 
> > Is 'dev' guaranteed to be not NULL?
> should be
> > > +             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)
> > > +             has_dev = npme_pme_target(target);
> > 
> > What's the meaning of 'has_dev'?  It seems to be the result of the bus type
> > callback.
> maybe I should rename it as found_dev
> > > +     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_enable_pme(dev->port, true);
> > 
> > What does data->exit different from zero mean at this point?
> the driver is exitting. I'll rename it
> 
> > > +static irqreturn_t npme_irq(int irq, void *context)
> > > +{
> > > +     int pos;
> > > +     struct pci_dev *pdev;
> > > +     u32 rtsta;
> > > +     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, &rtsta);
> > > +     if (!(rtsta & PCI_EXP_RTSTA_PME)) {
> > > +             spin_unlock_irqrestore(&data->lock, flags);
> > > +             return IRQ_NONE;
> > > +     }
> > > +
> > > +     data->bdf = (u16)rtsta;
> > 
> > Hm, couldn't we use pci_read_config_word() above instead?
> but it actually is a dword.

So we have a 32-bit register that we take only a half of.  Please use a mask
explicitly, then.
 
> > > +
> > > +     /* disable PME to avoid interrupt flood */
> > > +     npme_enable_pme(pdev, false);
> > > +     spin_unlock_irqrestore(&data->lock, flags);
> > > +
> > > +     schedule_work(&data->work);
> > 
> > I'm not sure if the workqueue is exactly suitable for that.  Have you
> > considered using anything else?
> we will call driver's .wakeup_event(), which might call into .resume() from device_receive_wakeup_event()
> so workqueue is best fit here.
> > > +     /* clear pending PME */
> > > +     npme_clear_pme(pdev);
> > > +
> > > +     status = request_irq(dev->irq, npme_irq, IRQF_SHARED, "npme", dev);
> > 
> > Who's going to set dev->irq?
> the pcie port driver.
> > > +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_enable_pme(pdev, false);
> > 
> > Won't this cause a regression on systems that use the native PME mechanism
> > for wake-up (I have one of these)?
> good point. currently I don't know if a npme interrupt can wakeup system from
> suspend/resume, because npme interrupt looks like usual device interrupt.
> Need more invistigation here.

Well, I have a setup option for enabling that in the system I was talking
about. :-)

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Wednesday, 22 of October 2008, Shaohua Li wrote:
> On Mon, Oct 20, 2008 at 04:39:47AM +0800, Rafael J. Wysocki wrote:
> > On Thursday, 11 of September 2008, Shaohua Li wrote:
> > > In ACPI platform, if native PME isn't enabled, GPE is used to report wakeup event.
> > 
> > Add more details here, please.
> > 
> > > ---
> > >  drivers/acpi/Kconfig        |    9 ++++++
> > >  drivers/acpi/bus.c          |   15 +++++++++++
> > >  drivers/acpi/sleep/wakeup.c |   60 ++++++++++++++++++++++++++++++++++++++++++++
> > >  include/acpi/acpi_bus.h     |    4 ++
> > >  4 files changed, 88 insertions(+)
> > >
> > > Index: linux/drivers/acpi/Kconfig
> > > ===================================================================
> > > --- linux.orig/drivers/acpi/Kconfig   2008-09-11 10:56:25.000000000 +0800
> > > +++ linux/drivers/acpi/Kconfig        2008-09-11 10:56:47.000000000 +0800
> > > @@ -45,6 +45,15 @@ config ACPI_SLEEP
> > >       depends on PM_SLEEP
> > >       default y
> > >
> > > +config ACPI_GPE_WAKEUP
> > 
> > I'd call it ACPI_RUNTIME_WAKEUP
> ok
> 
> > > +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);
> > > +
> > 
> > We were talking about removing the notifier last time.  Please do that.
> Did you see my comments on the issue last time? ACPI test tree already introduces
> the mechanism for other purpose, and we can just use it.
> 
> > > --- linux.orig/include/acpi/acpi_bus.h        2008-09-11 10:56:25.000000000 +0800
> > > +++ linux/include/acpi/acpi_bus.h     2008-09-11 10:56:47.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
> > >   */
> > >
> > 
> > I understand from the above that devices having their own wake-up GPEs will be
> > handled.  However, it still is completely unclear to me what happens with
> > devices that can generate PME# and for which there are no specific GPEs, like
> > any devices on add-in cards.
> As we discussed last time, pci bus for add-in cards will invoke a gpe, and
> this new implementation will check all pci devices under a bridge to try to find
> a device generating PME. This should work for add-in cards.

Well, can you please describe this mechanism to me or point me to
documents/code where I can read about it?

The question is how we can learn which GPE will be used for signalling the PME#
events.

Thanks,
Rafael