[RFC 3/5] pci wakeup handler

[RFC 3/5] pci wakeup handler

[shaohua.li]

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

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

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

-- 

Re: [RFC 3/5] pci wakeup handler

[Rafael J. Wysocki]

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

Pleae use dev->pm_cap instead.

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

Please use 'bool' for boolean variables.

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

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

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

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

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

Hm, I'd do:

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

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

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

Thanks,
Rafael

Re: [RFC 3/5] pci wakeup handler

[Li, Shaohua]

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

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

Thanks,
Shaohua

Re: [RFC 3/5] pci wakeup handler

[David Brownell]

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

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

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


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

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


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

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

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

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


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

Whitespace between paragraphs please ...

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

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

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


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

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

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


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

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

And style-wise I'd really prefer to see

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

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

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

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


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

Re: [RFC 3/5] pci wakeup handler

[David Brownell]

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

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


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

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


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

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

Re: [RFC 3/5] pci wakeup handler

[Li, Shaohua]

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

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

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

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

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

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

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


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

Thanks,
Shaohua

Re: [RFC 3/5] pci wakeup handler

[Li, Shaohua]

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

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

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

Thanks,
Shaohua

Re: [RFC 3/5] pci wakeup handler

[David Brownell]

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

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

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



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

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

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


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

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


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

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


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

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

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


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

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

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

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

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

- Dave

Re: [RFC 3/5] pci wakeup handler

[Rafael J. Wysocki]

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

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

Thanks,
Rafael

Re: [RFC 3/5] pci wakeup handler

[David Brownell]

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

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

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

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


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

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

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

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

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

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

- Dave

[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