Re: [PATCH 1/2] handle wakeup event in PCI

Re: [PATCH 1/2] handle wakeup event in PCI

[Rafael J. Wysocki]

On Monday 24 August 2009, Shaohua Li wrote:
> Add an implementation how to detect wakeup event for PCI. PCI device can
> invoke PME, platform or PCIe native approach can collect the event and
> report to OS. OS should identify exactly which device invokes PME as
> several devices can share PME pin.
> 
> In platform approach (ACPI in this case), some BIOS give exact device which
> invokes PME but others doesn't. 

I don't think the BIOS can reliably say which device signals PME# in the PCI
(non-PCIe) case, unless the PME# is routed separately from each device to the
chipset.  Also, two or more devices may signal PME# at the same time.

So, in this case we generally need to scan the entire hierarchy each time
we get a PME#.

> In PCIe native approach, if PME source device is a pcie endpoint, the device
> is the exact PME source. If the device is root port or pcie-to-pci bridge,
> we need scan the hierarchy under the device.

Why do we have to scan if the source is a root port itself?

> To identify PME source, the patch does:
> 1. if the source is a pci device, the device is the only source for PME

That need not be true in the non-PCIe case IMO.

> 2. if the source is a bridge, scan the hierarchy under the bridge. Several
> devices under the bridge could be the sources.

I think we need a function that will scan the hierarchy below a bridge
(that may be the root bridge in the non-PCIe case or a PCIe-to-PCI
bridge in the PCIe case) and if a device has PME_Status set, it will
(a) clear it and (b) call pm_request_resume() for the device.

> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> ---
>  drivers/pci/pci-driver.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h        |    2 +
>  2 files changed, 79 insertions(+)
> 
> Index: linux/drivers/pci/pci-driver.c
> ===================================================================
> --- linux.orig/drivers/pci/pci-driver.c	2009-08-24 10:50:12.000000000 +0800
> +++ linux/drivers/pci/pci-driver.c	2009-08-24 15:42:49.000000000 +0800
> @@ -17,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/sched.h>
>  #include <linux/cpu.h>
> +#include <linux/pm_runtime.h>
>  #include "pci.h"
>  
>  /*
> @@ -570,6 +571,82 @@ static void pci_pm_complete(struct devic
>  		drv->pm->complete(dev);
>  }
>  
> +/*
> + * Called when dev is suspected to invoke a wakeup event
> + * */

Please add a full kerneldoc comment.

> +static bool pci_pm_check_wakeup_event(struct pci_dev *pdev)
> +{
> +	int pme_pos = pdev->pm_cap;
> +	u16 pmcsr;
> +	bool spurious = false;
> +
> +	if (pme_pos == 0) {

if (!pme_pos) would be better IMO.  Also the braces are not necessary and I'd
move the comment somewhere else (that function is going to be called for
devices that don't have the PM capability other than the USB controllers).

> +		/*
> +		 * Some USB devices haven't PME, but have specific registers to
> +		 * control wakeup
> +		 */
> +		return false;
> +	}
> +
> +	/* 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;

Fold these three lines into one:

+ return !spurious;

Thus, I wouldn't call that variable 'spurious'.  I'd rather call it 'ret' and
make it so that we can just return its value at the end.

So, the above can be rewritten as

+ 	/* Check if PME status is set. */
+	pci_read_config_word(pdev, pme_pos + PCI_PM_CTRL, &pmcsr);
+	if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
+		return false;
+	/* Clear PME status and disable PME to avoid interrupt flood. */
+	pmcsr |= PCI_PM_CTRL_PME_STATUS;
+	if (pmcsr & PCI_PM_CTRL_PME_ENABLE)
+		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
+	else
+		ret = false;
+	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
+	return ret;

> +}
> +
> +/* @target is suspected to invoke a wakeup event. Return true if it's true */

Please add full kerneldoc comment.

> +bool pci_pm_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.
> +	 * if target is device, trust the device invokes PME. If target is a
> +	 * bridge, scan devices under the bridge and only trust device invokes
> +	 * PME which we can detect
> +	 **/
> +	ret = pci_pm_check_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 */
> +		pm_request_resume(&target->dev);

I don't think we can return here.  There may be other devices signalling
PME.

Well, perhaps move the pm_request_resume(&target->dev) to the function above
along with the conditional?  In that case you could call the function
pci_pm_wakeup() which would be nicer IMO.

Also I'm not sure about the conditional.  Namely, I don't think we'll need to
call this function in the PCIe case except for PCI-to-PCIe bridges, because
the root port will give us the requester ID of the device the interrupt was for
if that's an endpoint.  So, perhaps we can assume we scan over non-PCIe
devices here?

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

Do we want to do that for bridges?

> +
> +	/* scan devices under the bridge */
> +	domain_nr = pci_domain_nr(target->bus);
> +	bus_start = target->subordinate->secondary;
> +	bus_end = target->subordinate->subordinate;
> +	while ((tmp = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, tmp)) != NULL) {
> +		if (pci_domain_nr(tmp->bus) == domain_nr &&
> +		   tmp->bus->number >= bus_start &&
> +		   tmp->bus->number <= bus_end) {
> +			if (pci_pm_check_wakeup_event(tmp)) {
> +				ret = true;
> +				pm_request_resume(&tmp->dev);
> +			}
> +		}
> +	}
> +	return ret;
> +}
> +
>  #ifdef CONFIG_SUSPEND
>  
>  static int pci_pm_suspend(struct device *dev)
> Index: linux/drivers/pci/pci.h
> ===================================================================
> --- linux.orig/drivers/pci/pci.h	2009-08-24 11:04:22.000000000 +0800
> +++ linux/drivers/pci/pci.h	2009-08-24 11:05:24.000000000 +0800
> @@ -83,6 +83,8 @@ static inline void pci_vpd_release(struc
>  		dev->vpd->ops->release(dev);
>  }
>  
> +extern bool pci_pm_handle_wakeup_event(struct pci_dev *target);
> +
>  /* PCI /proc functions */
>  #ifdef CONFIG_PROC_FS
>  extern int pci_proc_attach_device(struct pci_dev *dev);

Thanks,
Rafael

Re: [PATCH 1/2] handle wakeup event in PCI

[Shaohua Li]

On Tue, Aug 25, 2009 at 05:02:27AM +0800, Rafael J. Wysocki wrote:
> On Monday 24 August 2009, Shaohua Li wrote:
> > Add an implementation how to detect wakeup event for PCI. PCI device can
> > invoke PME, platform or PCIe native approach can collect the event and
> > report to OS. OS should identify exactly which device invokes PME as
> > several devices can share PME pin.
> > 
> > In platform approach (ACPI in this case), some BIOS give exact device which
> > invokes PME but others doesn't. 
> 
> I don't think the BIOS can reliably say which device signals PME# in the PCI
> (non-PCIe) case, unless the PME# is routed separately from each device to the
> chipset.  Also, two or more devices may signal PME# at the same time.
> 
> So, in this case we generally need to scan the entire hierarchy each time
> we get a PME#.
The thing isn't that simple. Some BIOS in its AML code clear PME status before
sending notification to OS, which will make the 'scan the entire hierarchy each
time' broken.

So my assumption here is BIOS sent notification to a specific device only when
it makes sure the device signals PME#. Otherwise, BIOS should send notification
to a bridge.

Or do you have better solution?
> > In PCIe native approach, if PME source device is a pcie endpoint, the device
> > is the exact PME source. If the device is root port or pcie-to-pci bridge,
> > we need scan the hierarchy under the device.
> 
> Why do we have to scan if the source is a root port itself?
The spec says legacy pci PME can directly route to root port, so this is similar
with the pcie-to-pci bridge case.

> > To identify PME source, the patch does:
> > 1. if the source is a pci device, the device is the only source for PME
> 
> That need not be true in the non-PCIe case IMO.
See my first comment.

> > 2. if the source is a bridge, scan the hierarchy under the bridge. Several
> > devices under the bridge could be the sources.
> 
> I think we need a function that will scan the hierarchy below a bridge
> (that may be the root bridge in the non-PCIe case or a PCIe-to-PCI
> bridge in the PCIe case) and if a device has PME_Status set, it will
> (a) clear it and (b) call pm_request_resume() for the device.
Even the PCIe device can work like a legacy PCI device to send PME and ACPI GPE
fires when this happens. I'd like we have a general solution for both PCIe case
and non-PCIe case.

> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > ---
> >  drivers/pci/pci-driver.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/pci/pci.h        |    2 +
> >  2 files changed, 79 insertions(+)
> > 
> > Index: linux/drivers/pci/pci-driver.c
> > ===================================================================
> > --- linux.orig/drivers/pci/pci-driver.c	2009-08-24 10:50:12.000000000 +0800
> > +++ linux/drivers/pci/pci-driver.c	2009-08-24 15:42:49.000000000 +0800
> > @@ -17,6 +17,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/sched.h>
> >  #include <linux/cpu.h>
> > +#include <linux/pm_runtime.h>
> >  #include "pci.h"
> >  
> >  /*
> > @@ -570,6 +571,82 @@ static void pci_pm_complete(struct devic
> >  		drv->pm->complete(dev);
> >  }
> >  
> > +/*
> > + * Called when dev is suspected to invoke a wakeup event
> > + * */
> 
> Please add a full kerneldoc comment.
> 
> > +static bool pci_pm_check_wakeup_event(struct pci_dev *pdev)
> > +{
> > +	int pme_pos = pdev->pm_cap;
> > +	u16 pmcsr;
> > +	bool spurious = false;
> > +
> > +	if (pme_pos == 0) {
> 
> if (!pme_pos) would be better IMO.  Also the braces are not necessary and I'd
> move the comment somewhere else (that function is going to be called for
> devices that don't have the PM capability other than the USB controllers).
> 
> > +		/*
> > +		 * Some USB devices haven't PME, but have specific registers to
> > +		 * control wakeup
> > +		 */
> > +		return false;
> > +	}
> > +
> > +	/* 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;
> 
> Fold these three lines into one:
> 
> + return !spurious;
> 
> Thus, I wouldn't call that variable 'spurious'.  I'd rather call it 'ret' and
> make it so that we can just return its value at the end.
but it's really spurious interrupt :).

> So, the above can be rewritten as
> 
> + 	/* Check if PME status is set. */
> +	pci_read_config_word(pdev, pme_pos + PCI_PM_CTRL, &pmcsr);
> +	if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
> +		return false;
> +	/* Clear PME status and disable PME to avoid interrupt flood. */
> +	pmcsr |= PCI_PM_CTRL_PME_STATUS;
> +	if (pmcsr & PCI_PM_CTRL_PME_ENABLE)
> +		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
> +	else
> +		ret = false;
> +	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
> +	return ret;
> 
> > +}
> > +
> > +/* @target is suspected to invoke a wakeup event. Return true if it's true */
> 
> Please add full kerneldoc comment.
> 
> > +bool pci_pm_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.
> > +	 * if target is device, trust the device invokes PME. If target is a
> > +	 * bridge, scan devices under the bridge and only trust device invokes
> > +	 * PME which we can detect
> > +	 **/
> > +	ret = pci_pm_check_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 */
> > +		pm_request_resume(&target->dev);
> 
> I don't think we can return here.  There may be other devices signalling
> PME.
they will invoke another interrupt, right?

> Well, perhaps move the pm_request_resume(&target->dev) to the function above
> along with the conditional?  In that case you could call the function
> pci_pm_wakeup() which would be nicer IMO.
> 
> Also I'm not sure about the conditional.  Namely, I don't think we'll need to
> call this function in the PCIe case except for PCI-to-PCIe bridges, because
> the root port will give us the requester ID of the device the interrupt was for
> if that's an endpoint.  So, perhaps we can assume we scan over non-PCIe
> devices here?
As I said, PCIe device can invoke ACPI GPE if BIOS doesn't enable PCIe native
PME, so sounds not true to only scan non-PCIe devices.

> > +		return true;
> > +	}
> > +
> > +	if (ret)
> > +		pm_request_resume(&target->dev);
> 
> Do we want to do that for bridges?
birdges can send PME too, right? Seems not harmful.

Thanks,
Shaohua

Re: [PATCH 1/2] handle wakeup event in PCI

[Shaohua Li]

On Tue, Aug 25, 2009 at 09:58:38AM +0800, Shaohua Li wrote:
> On Tue, Aug 25, 2009 at 05:02:27AM +0800, Rafael J. Wysocki wrote:
> > On Monday 24 August 2009, Shaohua Li wrote:
> > > Add an implementation how to detect wakeup event for PCI. PCI device can
> > > invoke PME, platform or PCIe native approach can collect the event and
> > > report to OS. OS should identify exactly which device invokes PME as
> > > several devices can share PME pin.
> > > 
> > > In platform approach (ACPI in this case), some BIOS give exact device which
> > > invokes PME but others doesn't. 
> > 
> > I don't think the BIOS can reliably say which device signals PME# in the PCI
> > (non-PCIe) case, unless the PME# is routed separately from each device to the
> > chipset.  Also, two or more devices may signal PME# at the same time.
> > 
> > So, in this case we generally need to scan the entire hierarchy each time
> > we get a PME#.
> The thing isn't that simple. Some BIOS in its AML code clear PME status before
> sending notification to OS, which will make the 'scan the entire hierarchy each
> time' broken.
> 
> So my assumption here is BIOS sent notification to a specific device only when
> it makes sure the device signals PME#. Otherwise, BIOS should send notification
> to a bridge.
> 
> Or do you have better solution?
> > > In PCIe native approach, if PME source device is a pcie endpoint, the device
> > > is the exact PME source. If the device is root port or pcie-to-pci bridge,
> > > we need scan the hierarchy under the device.
> > 
> > Why do we have to scan if the source is a root port itself?
> The spec says legacy pci PME can directly route to root port, so this is similar
> with the pcie-to-pci bridge case.
> 
> > > To identify PME source, the patch does:
> > > 1. if the source is a pci device, the device is the only source for PME
> > 
> > That need not be true in the non-PCIe case IMO.
> See my first comment.
> 
> > > 2. if the source is a bridge, scan the hierarchy under the bridge. Several
> > > devices under the bridge could be the sources.
> > 
> > I think we need a function that will scan the hierarchy below a bridge
> > (that may be the root bridge in the non-PCIe case or a PCIe-to-PCI
> > bridge in the PCIe case) and if a device has PME_Status set, it will
> > (a) clear it and (b) call pm_request_resume() for the device.
> Even the PCIe device can work like a legacy PCI device to send PME and ACPI GPE
> fires when this happens. I'd like we have a general solution for both PCIe case
> and non-PCIe case.
Rafael,
how about the updated patch? I separate the wakeup event handling for ACPI and pcie.


Add an implementation how to detect wakeup event for PCI. PCI device can
invoke PME, platform or PCIe native approach can collect the event and
report to OS. OS should identify exactly which device invokes PME as
several devices can share PME pin.

In platform approach (ACPI in this case), some BIOS give exact device which
invokes PME but others doesn't. In either case, we always scan all devices
to check which one is the wakeup source. In the meantime, if the device isn't a
bridge, we trust the device is wakeup source even its PME status isn't set,
because BIOS can clear PME status before OS gets notification and we have no
reliable method to check if the device is the wakeup source.

In PCIe native approach, if PME source device is a pcie endpoint, the device
is the exact PME source. If the device is root port or pcie-to-pci bridge,
we need scan legacy devices in the hierarchy under the root port or bridge.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
 drivers/pci/pci-driver.c |  100 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h        |    3 +
 2 files changed, 103 insertions(+)

Index: linux/drivers/pci/pci-driver.c
===================================================================
--- linux.orig/drivers/pci/pci-driver.c	2009-08-25 13:30:47.000000000 +0800
+++ linux/drivers/pci/pci-driver.c	2009-08-25 14:37:57.000000000 +0800
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/cpu.h>
+#include <linux/pm_runtime.h>
 #include "pci.h"
 
 /*
@@ -570,6 +571,105 @@ static void pci_pm_complete(struct devic
 		drv->pm->complete(dev);
 }
 
+/*
+ * pci_pm_check_wakeup_event - pdev is suspected to invoke a wakeup event
+ * @pdev: the suspected pci device
+ *
+ * Clear PME status and disable PME, return if the pci device @pdev really
+ * invokes PME
+ **/
+static bool pci_pm_check_wakeup_event(struct pci_dev *pdev)
+{
+	int pme_pos = pdev->pm_cap;
+	u16 pmcsr;
+	bool spurious_pme = false;
+
+	if (!pme_pos)
+		return false;
+
+	/* 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)
+		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
+	else
+		spurious_pme = true;
+	pmcsr |= PCI_PM_CTRL_PME_STATUS;
+	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
+
+	return !spurious_pme;
+}
+
+/*
+ * pci_pm_handle_wakeup_event_native - handle PCIe native PME event
+ * @target is suspected to invoke a wakeup event (PME)
+ * Note @target should be a PCIe device
+ */
+bool pci_pm_handle_wakeup_event_native(struct pci_dev *target)
+{
+	bool ret;
+	struct pci_dev *tmp = NULL;
+	int domain_nr, bus_start, bus_end;
+
+	BUG_ON(!target->is_pcie);
+
+	/* For PCIe PME, if the target isn't the source, do nothing */
+	ret = pci_pm_check_wakeup_event(target);
+	if (ret)
+		pm_request_resume(&target->dev);
+
+	if (!target->subordinate || (target->pcie_type != PCI_EXP_TYPE_ROOT_PORT
+		&& target->pcie_type != PCI_EXP_TYPE_PCI_BRIDGE))
+		return ret;
+
+	/* scan legacy PCI devices under the bridge or root port */
+	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 (!tmp->is_pcie && pci_pm_check_wakeup_event(tmp)) {
+				ret = true;
+				pm_request_resume(&tmp->dev);
+			}
+		}
+	}
+	return ret;
+}
+
+/*
+ * pci_pm_handle_wakeup_event_platform - handle platform PCI wakeup event
+ * @target is suspected to invoke a wakeup event
+ */
+bool pci_pm_handle_wakeup_event_platform(struct pci_dev *target)
+{
+	bool ret = false;
+	struct pci_dev *tmp = NULL;
+
+	/*
+	 * In the platform approach (ACPI), target PME status might get cleared
+	 * by BIOS before OS receives a notification, so we haven't reliable
+	 * method to detect if target is the wakeup source, just trust it is
+	 */
+	pci_pm_check_wakeup_event(target);
+	pm_request_resume(&target->dev);
+	if (!target->subordinate)
+		ret = true;
+
+	/* scan all pci devices to find the wakeup source */
+	while ((tmp = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, tmp)) != NULL) {
+		if (tmp != target && pci_pm_check_wakeup_event(tmp)) {
+			ret = true;
+			pm_request_resume(&tmp->dev);
+		}
+	}
+	return ret;
+}
+
 #ifdef CONFIG_SUSPEND
 
 static int pci_pm_suspend(struct device *dev)
Index: linux/drivers/pci/pci.h
===================================================================
--- linux.orig/drivers/pci/pci.h	2009-08-25 13:30:47.000000000 +0800
+++ linux/drivers/pci/pci.h	2009-08-25 14:30:26.000000000 +0800
@@ -83,6 +83,9 @@ static inline void pci_vpd_release(struc
 		dev->vpd->ops->release(dev);
 }
 
+extern bool pci_pm_handle_wakeup_event_native(struct pci_dev *target);
+extern bool pci_pm_handle_wakeup_event_platform(struct pci_dev *target);
+
 /* PCI /proc functions */
 #ifdef CONFIG_PROC_FS
 extern int pci_proc_attach_device(struct pci_dev *dev);

Re: [PATCH 1/2] handle wakeup event in PCI

[Rafael J. Wysocki]

On Tuesday 25 August 2009, Shaohua Li wrote:
> On Tue, Aug 25, 2009 at 05:02:27AM +0800, Rafael J. Wysocki wrote:
> > On Monday 24 August 2009, Shaohua Li wrote:
> > > Add an implementation how to detect wakeup event for PCI. PCI device can
> > > invoke PME, platform or PCIe native approach can collect the event and
> > > report to OS. OS should identify exactly which device invokes PME as
> > > several devices can share PME pin.
> > > 
> > > In platform approach (ACPI in this case), some BIOS give exact device which
> > > invokes PME but others doesn't. 
> > 
> > I don't think the BIOS can reliably say which device signals PME# in the PCI
> > (non-PCIe) case, unless the PME# is routed separately from each device to the
> > chipset.  Also, two or more devices may signal PME# at the same time.
> > 
> > So, in this case we generally need to scan the entire hierarchy each time
> > we get a PME#.
> The thing isn't that simple. Some BIOS in its AML code clear PME status before
> sending notification to OS, which will make the 'scan the entire hierarchy each
> time' broken.
> 
> So my assumption here is BIOS sent notification to a specific device only when
> it makes sure the device signals PME#. Otherwise, BIOS should send notification
> to a bridge.
> 
> Or do you have better solution?
> > > In PCIe native approach, if PME source device is a pcie endpoint, the device
> > > is the exact PME source. If the device is root port or pcie-to-pci bridge,
> > > we need scan the hierarchy under the device.
> > 
> > Why do we have to scan if the source is a root port itself?
> The spec says legacy pci PME can directly route to root port, so this is similar
> with the pcie-to-pci bridge case.
> 
> > > To identify PME source, the patch does:
> > > 1. if the source is a pci device, the device is the only source for PME
> > 
> > That need not be true in the non-PCIe case IMO.
> See my first comment.
> 
> > > 2. if the source is a bridge, scan the hierarchy under the bridge. Several
> > > devices under the bridge could be the sources.
> > 
> > I think we need a function that will scan the hierarchy below a bridge
> > (that may be the root bridge in the non-PCIe case or a PCIe-to-PCI
> > bridge in the PCIe case) and if a device has PME_Status set, it will
> > (a) clear it and (b) call pm_request_resume() for the device.
> Even the PCIe device can work like a legacy PCI device to send PME and ACPI GPE
> fires when this happens. I'd like we have a general solution for both PCIe case
> and non-PCIe case.
> 
> > > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > > ---
> > >  drivers/pci/pci-driver.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/pci/pci.h        |    2 +
> > >  2 files changed, 79 insertions(+)
> > > 
> > > Index: linux/drivers/pci/pci-driver.c
> > > ===================================================================
> > > --- linux.orig/drivers/pci/pci-driver.c	2009-08-24 10:50:12.000000000 +0800
> > > +++ linux/drivers/pci/pci-driver.c	2009-08-24 15:42:49.000000000 +0800
> > > @@ -17,6 +17,7 @@
> > >  #include <linux/slab.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/cpu.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include "pci.h"
> > >  
> > >  /*
> > > @@ -570,6 +571,82 @@ static void pci_pm_complete(struct devic
> > >  		drv->pm->complete(dev);
> > >  }
> > >  
> > > +/*
> > > + * Called when dev is suspected to invoke a wakeup event
> > > + * */
> > 
> > Please add a full kerneldoc comment.
> > 
> > > +static bool pci_pm_check_wakeup_event(struct pci_dev *pdev)
> > > +{
> > > +	int pme_pos = pdev->pm_cap;
> > > +	u16 pmcsr;
> > > +	bool spurious = false;
> > > +
> > > +	if (pme_pos == 0) {
> > 
> > if (!pme_pos) would be better IMO.  Also the braces are not necessary and I'd
> > move the comment somewhere else (that function is going to be called for
> > devices that don't have the PM capability other than the USB controllers).
> > 
> > > +		/*
> > > +		 * Some USB devices haven't PME, but have specific registers to
> > > +		 * control wakeup
> > > +		 */
> > > +		return false;
> > > +	}
> > > +
> > > +	/* 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;
> > 
> > Fold these three lines into one:
> > 
> > + return !spurious;
> > 
> > Thus, I wouldn't call that variable 'spurious'.  I'd rather call it 'ret' and
> > make it so that we can just return its value at the end.
> but it's really spurious interrupt :).
> 
> > So, the above can be rewritten as
> > 
> > + 	/* Check if PME status is set. */
> > +	pci_read_config_word(pdev, pme_pos + PCI_PM_CTRL, &pmcsr);
> > +	if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
> > +		return false;
> > +	/* Clear PME status and disable PME to avoid interrupt flood. */
> > +	pmcsr |= PCI_PM_CTRL_PME_STATUS;
> > +	if (pmcsr & PCI_PM_CTRL_PME_ENABLE)
> > +		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
> > +	else
> > +		ret = false;
> > +	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
> > +	return ret;
> > 
> > > +}
> > > +
> > > +/* @target is suspected to invoke a wakeup event. Return true if it's true */
> > 
> > Please add full kerneldoc comment.
> > 
> > > +bool pci_pm_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.
> > > +	 * if target is device, trust the device invokes PME. If target is a
> > > +	 * bridge, scan devices under the bridge and only trust device invokes
> > > +	 * PME which we can detect
> > > +	 **/
> > > +	ret = pci_pm_check_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 */
> > > +		pm_request_resume(&target->dev);
> > 
> > I don't think we can return here.  There may be other devices signalling
> > PME.
> they will invoke another interrupt, right?

If there are two PCI devices asserting PME# at the same time, there may be no
means to distinguish between them.  We can return after checking the first one,
but if we're guaranteed to receive the interrupt just as soon as we reenable
the notification mechanism, it may be better to check that beforehand.

> > Well, perhaps move the pm_request_resume(&target->dev) to the function above
> > along with the conditional?  In that case you could call the function
> > pci_pm_wakeup() which would be nicer IMO.
> > 
> > Also I'm not sure about the conditional.  Namely, I don't think we'll need to
> > call this function in the PCIe case except for PCI-to-PCIe bridges, because
> > the root port will give us the requester ID of the device the interrupt was for
> > if that's an endpoint.  So, perhaps we can assume we scan over non-PCIe
> > devices here?
> As I said, PCIe device can invoke ACPI GPE if BIOS doesn't enable PCIe native
> PME, so sounds not true to only scan non-PCIe devices.
> 
> > > +		return true;
> > > +	}
> > > +
> > > +	if (ret)
> > > +		pm_request_resume(&target->dev);
> > 
> > Do we want to do that for bridges?
> birdges can send PME too, right? Seems not harmful.

OK

Thanks,
Rafael

Re: [PATCH 1/2] handle wakeup event in PCI

[Rafael J. Wysocki]

On Tuesday 25 August 2009, Shaohua Li wrote:
> On Tue, Aug 25, 2009 at 09:58:38AM +0800, Shaohua Li wrote:
> > On Tue, Aug 25, 2009 at 05:02:27AM +0800, Rafael J. Wysocki wrote:
> > > On Monday 24 August 2009, Shaohua Li wrote:
> > > > Add an implementation how to detect wakeup event for PCI. PCI device can
> > > > invoke PME, platform or PCIe native approach can collect the event and
> > > > report to OS. OS should identify exactly which device invokes PME as
> > > > several devices can share PME pin.
> > > > 
> > > > In platform approach (ACPI in this case), some BIOS give exact device which
> > > > invokes PME but others doesn't. 
> > > 
> > > I don't think the BIOS can reliably say which device signals PME# in the PCI
> > > (non-PCIe) case, unless the PME# is routed separately from each device to the
> > > chipset.  Also, two or more devices may signal PME# at the same time.
> > > 
> > > So, in this case we generally need to scan the entire hierarchy each time
> > > we get a PME#.
> > The thing isn't that simple. Some BIOS in its AML code clear PME status before
> > sending notification to OS, which will make the 'scan the entire hierarchy each
> > time' broken.
> > 
> > So my assumption here is BIOS sent notification to a specific device only when
> > it makes sure the device signals PME#. Otherwise, BIOS should send notification
> > to a bridge.
> > 
> > Or do you have better solution?
> > > > In PCIe native approach, if PME source device is a pcie endpoint, the device
> > > > is the exact PME source. If the device is root port or pcie-to-pci bridge,
> > > > we need scan the hierarchy under the device.
> > > 
> > > Why do we have to scan if the source is a root port itself?
> > The spec says legacy pci PME can directly route to root port, so this is similar
> > with the pcie-to-pci bridge case.
> > 
> > > > To identify PME source, the patch does:
> > > > 1. if the source is a pci device, the device is the only source for PME
> > > 
> > > That need not be true in the non-PCIe case IMO.
> > See my first comment.
> > 
> > > > 2. if the source is a bridge, scan the hierarchy under the bridge. Several
> > > > devices under the bridge could be the sources.
> > > 
> > > I think we need a function that will scan the hierarchy below a bridge
> > > (that may be the root bridge in the non-PCIe case or a PCIe-to-PCI
> > > bridge in the PCIe case) and if a device has PME_Status set, it will
> > > (a) clear it and (b) call pm_request_resume() for the device.
> > Even the PCIe device can work like a legacy PCI device to send PME and ACPI GPE
> > fires when this happens. I'd like we have a general solution for both PCIe case
> > and non-PCIe case.
> Rafael,
> how about the updated patch? I separate the wakeup event handling for ACPI and pcie.

Hmm.  It seems we can do that more elegantly, so to speak, but I need to
think about it when I'm less tired.

> Add an implementation how to detect wakeup event for PCI. PCI device can
> invoke PME, platform or PCIe native approach can collect the event and
> report to OS. OS should identify exactly which device invokes PME as
> several devices can share PME pin.
> 
> In platform approach (ACPI in this case), some BIOS give exact device which
> invokes PME but others doesn't. In either case, we always scan all devices
> to check which one is the wakeup source. In the meantime, if the device isn't a
> bridge, we trust the device is wakeup source even its PME status isn't set,
> because BIOS can clear PME status before OS gets notification and we have no
> reliable method to check if the device is the wakeup source.
> 
> In PCIe native approach, if PME source device is a pcie endpoint, the device
> is the exact PME source. If the device is root port or pcie-to-pci bridge,
> we need scan legacy devices in the hierarchy under the root port or bridge.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> ---
>  drivers/pci/pci-driver.c |  100 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h        |    3 +
>  2 files changed, 103 insertions(+)
> 
> Index: linux/drivers/pci/pci-driver.c
> ===================================================================
> --- linux.orig/drivers/pci/pci-driver.c	2009-08-25 13:30:47.000000000 +0800
> +++ linux/drivers/pci/pci-driver.c	2009-08-25 14:37:57.000000000 +0800
> @@ -17,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/sched.h>
>  #include <linux/cpu.h>
> +#include <linux/pm_runtime.h>
>  #include "pci.h"
>  
>  /*
> @@ -570,6 +571,105 @@ static void pci_pm_complete(struct devic
>  		drv->pm->complete(dev);
>  }
>  
> +/*
> + * pci_pm_check_wakeup_event - pdev is suspected to invoke a wakeup event
> + * @pdev: the suspected pci device
> + *
> + * Clear PME status and disable PME, return if the pci device @pdev really
> + * invokes PME
> + **/
> +static bool pci_pm_check_wakeup_event(struct pci_dev *pdev)
> +{
> +	int pme_pos = pdev->pm_cap;
> +	u16 pmcsr;
> +	bool spurious_pme = false;
> +
> +	if (!pme_pos)
> +		return false;
> +
> +	/* 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)
> +		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
> +	else
> +		spurious_pme = true;
> +	pmcsr |= PCI_PM_CTRL_PME_STATUS;
> +	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
> +
> +	return !spurious_pme;
> +}

This looks good, although I'm still thinking that "return something" is a bit
more natural to read than "return !something".  The code just means we return
'false' if there's a spurious PME and the name of the temp variable doesn't
really matter.  Well, whatever.

> +
> +/*
> + * pci_pm_handle_wakeup_event_native - handle PCIe native PME event
> + * @target is suspected to invoke a wakeup event (PME)
> + * Note @target should be a PCIe device
> + */
> +bool pci_pm_handle_wakeup_event_native(struct pci_dev *target)
> +{
> +	bool ret;
> +	struct pci_dev *tmp = NULL;
> +	int domain_nr, bus_start, bus_end;
> +
> +	BUG_ON(!target->is_pcie);
> +
> +	/* For PCIe PME, if the target isn't the source, do nothing */
> +	ret = pci_pm_check_wakeup_event(target);
> +	if (ret)
> +		pm_request_resume(&target->dev);
> +

This needs some more work IMO.  Namely, if we're sure that the device is not
a bridge that forwards the PME (BTW, the spec 2.0 says the PME Status won't
be set for the bridge itself in that case), we can just call

pm_runtime_resume(&target->dev);

and return, because there's no need to delay the execution of
->runtime_resume() by putting it into a separate work item.  So, IMO we should
do something like this:

* check if 'target' is a bridge forwarding the PME
* if not, execute pm_runtime_resume(&target->dev) and return
* otherwise, look for device(s) that assert PME# under the bridge, clear PME
  Status and execute pm_request_resume() for each of them

Thanks,
Rafael

Re: [PATCH 1/2] handle wakeup event in PCI

[Shaohua Li]

On Wed, Aug 26, 2009 at 08:16:24AM +0800, Rafael J. Wysocki wrote:
> On Tuesday 25 August 2009, Shaohua Li wrote:
> > On Tue, Aug 25, 2009 at 09:58:38AM +0800, Shaohua Li wrote:
> > > On Tue, Aug 25, 2009 at 05:02:27AM +0800, Rafael J. Wysocki wrote:
> > > > On Monday 24 August 2009, Shaohua Li wrote:
> > > > > Add an implementation how to detect wakeup event for PCI. PCI device can
> > > > > invoke PME, platform or PCIe native approach can collect the event and
> > > > > report to OS. OS should identify exactly which device invokes PME as
> > > > > several devices can share PME pin.
> > > > > 
> > > > > In platform approach (ACPI in this case), some BIOS give exact device which
> > > > > invokes PME but others doesn't. 
> > > > 
> > > > I don't think the BIOS can reliably say which device signals PME# in the PCI
> > > > (non-PCIe) case, unless the PME# is routed separately from each device to the
> > > > chipset.  Also, two or more devices may signal PME# at the same time.
> > > > 
> > > > So, in this case we generally need to scan the entire hierarchy each time
> > > > we get a PME#.
> > > The thing isn't that simple. Some BIOS in its AML code clear PME status before
> > > sending notification to OS, which will make the 'scan the entire hierarchy each
> > > time' broken.
> > > 
> > > So my assumption here is BIOS sent notification to a specific device only when
> > > it makes sure the device signals PME#. Otherwise, BIOS should send notification
> > > to a bridge.
> > > 
> > > Or do you have better solution?
> > > > > In PCIe native approach, if PME source device is a pcie endpoint, the device
> > > > > is the exact PME source. If the device is root port or pcie-to-pci bridge,
> > > > > we need scan the hierarchy under the device.
> > > > 
> > > > Why do we have to scan if the source is a root port itself?
> > > The spec says legacy pci PME can directly route to root port, so this is similar
> > > with the pcie-to-pci bridge case.
> > > 
> > > > > To identify PME source, the patch does:
> > > > > 1. if the source is a pci device, the device is the only source for PME
> > > > 
> > > > That need not be true in the non-PCIe case IMO.
> > > See my first comment.
> > > 
> > > > > 2. if the source is a bridge, scan the hierarchy under the bridge. Several
> > > > > devices under the bridge could be the sources.
> > > > 
> > > > I think we need a function that will scan the hierarchy below a bridge
> > > > (that may be the root bridge in the non-PCIe case or a PCIe-to-PCI
> > > > bridge in the PCIe case) and if a device has PME_Status set, it will
> > > > (a) clear it and (b) call pm_request_resume() for the device.
> > > Even the PCIe device can work like a legacy PCI device to send PME and ACPI GPE
> > > fires when this happens. I'd like we have a general solution for both PCIe case
> > > and non-PCIe case.
> > Rafael,
> > how about the updated patch? I separate the wakeup event handling for ACPI and pcie.
> 
> Hmm.  It seems we can do that more elegantly, so to speak, but I need to
> think about it when I'm less tired.
> 
> > Add an implementation how to detect wakeup event for PCI. PCI device can
> > invoke PME, platform or PCIe native approach can collect the event and
> > report to OS. OS should identify exactly which device invokes PME as
> > several devices can share PME pin.
> > 
> > In platform approach (ACPI in this case), some BIOS give exact device which
> > invokes PME but others doesn't. In either case, we always scan all devices
> > to check which one is the wakeup source. In the meantime, if the device isn't a
> > bridge, we trust the device is wakeup source even its PME status isn't set,
> > because BIOS can clear PME status before OS gets notification and we have no
> > reliable method to check if the device is the wakeup source.
> > 
> > In PCIe native approach, if PME source device is a pcie endpoint, the device
> > is the exact PME source. If the device is root port or pcie-to-pci bridge,
> > we need scan legacy devices in the hierarchy under the root port or bridge.
> > 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > ---
> >  drivers/pci/pci-driver.c |  100 +++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/pci/pci.h        |    3 +
> >  2 files changed, 103 insertions(+)
> > 
> > Index: linux/drivers/pci/pci-driver.c
> > ===================================================================
> > --- linux.orig/drivers/pci/pci-driver.c	2009-08-25 13:30:47.000000000 +0800
> > +++ linux/drivers/pci/pci-driver.c	2009-08-25 14:37:57.000000000 +0800
> > @@ -17,6 +17,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/sched.h>
> >  #include <linux/cpu.h>
> > +#include <linux/pm_runtime.h>
> >  #include "pci.h"
> >  
> >  /*
> > @@ -570,6 +571,105 @@ static void pci_pm_complete(struct devic
> >  		drv->pm->complete(dev);
> >  }
> >  
> > +/*
> > + * pci_pm_check_wakeup_event - pdev is suspected to invoke a wakeup event
> > + * @pdev: the suspected pci device
> > + *
> > + * Clear PME status and disable PME, return if the pci device @pdev really
> > + * invokes PME
> > + **/
> > +static bool pci_pm_check_wakeup_event(struct pci_dev *pdev)
> > +{
> > +	int pme_pos = pdev->pm_cap;
> > +	u16 pmcsr;
> > +	bool spurious_pme = false;
> > +
> > +	if (!pme_pos)
> > +		return false;
> > +
> > +	/* 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)
> > +		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
> > +	else
> > +		spurious_pme = true;
> > +	pmcsr |= PCI_PM_CTRL_PME_STATUS;
> > +	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
> > +
> > +	return !spurious_pme;
> > +}
> 
> This looks good, although I'm still thinking that "return something" is a bit
> more natural to read than "return !something".  The code just means we return
> 'false' if there's a spurious PME and the name of the temp variable doesn't
> really matter.  Well, whatever.
> 
> > +
> > +/*
> > + * pci_pm_handle_wakeup_event_native - handle PCIe native PME event
> > + * @target is suspected to invoke a wakeup event (PME)
> > + * Note @target should be a PCIe device
> > + */
> > +bool pci_pm_handle_wakeup_event_native(struct pci_dev *target)
> > +{
> > +	bool ret;
> > +	struct pci_dev *tmp = NULL;
> > +	int domain_nr, bus_start, bus_end;
> > +
> > +	BUG_ON(!target->is_pcie);
> > +
> > +	/* For PCIe PME, if the target isn't the source, do nothing */
> > +	ret = pci_pm_check_wakeup_event(target);
> > +	if (ret)
> > +		pm_request_resume(&target->dev);
> > +
> 
> This needs some more work IMO.  Namely, if we're sure that the device is not
> a bridge that forwards the PME (BTW, the spec 2.0 says the PME Status won't
> be set for the bridge itself in that case), we can just call
> 
> pm_runtime_resume(&target->dev);
> 
> and return, because there's no need to delay the execution of
> ->runtime_resume() by putting it into a separate work item.  So, IMO we should
> do something like this:
> 
> * check if 'target' is a bridge forwarding the PME
> * if not, execute pm_runtime_resume(&target->dev) and return
> * otherwise, look for device(s) that assert PME# under the bridge, clear PME
>   Status and execute pm_request_resume() for each of them
Fixed the two issues you pointed out above.


Add an implementation how to detect wakeup event for PCI. PCI device can
invoke PME, platform or PCIe native approach can collect the event and
report to OS. OS should identify exactly which device invokes PME as
several devices can share PME pin.

In platform approach (ACPI in this case), some BIOS give exact device which
invokes PME but others doesn't. In either case, we always scan all devices
to check which one is the wakeup source. In the meantime, if the device isn't a
bridge, we trust the device is wakeup source even its PME status isn't set,
because BIOS can clear PME status before OS gets notification and we have no
reliable method to check if the device is the wakeup source.

In PCIe native approach, if PME source device is a pcie endpoint, the device
is the exact PME source. If the device is root port or pcie-to-pci bridge,
we need scan legacy devices in the hierarchy under the root port or bridge.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
 drivers/pci/pci-driver.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h        |    3 +
 2 files changed, 109 insertions(+)

Index: linux/drivers/pci/pci-driver.c
===================================================================
--- linux.orig/drivers/pci/pci-driver.c	2009-08-25 15:39:52.000000000 +0800
+++ linux/drivers/pci/pci-driver.c	2009-08-27 14:15:34.000000000 +0800
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/cpu.h>
+#include <linux/pm_runtime.h>
 #include "pci.h"
 
 /*
@@ -570,6 +571,111 @@ static void pci_pm_complete(struct devic
 		drv->pm->complete(dev);
 }
 
+/*
+ * pci_pm_check_wakeup_event - pdev is suspected to invoke a wakeup event
+ * @pdev: the suspected pci device
+ *
+ * Clear PME status and disable PME, return if the pci device @pdev really
+ * invokes PME
+ **/
+static bool pci_pm_check_wakeup_event(struct pci_dev *pdev)
+{
+	int pme_pos = pdev->pm_cap;
+	u16 pmcsr;
+	bool ret = false;
+
+	if (!pme_pos)
+		return false;
+
+	/* 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) {
+		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
+		ret = true;
+	}
+
+	pmcsr |= PCI_PM_CTRL_PME_STATUS;
+	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
+
+	return ret;
+}
+
+/*
+ * pci_pm_handle_wakeup_event_native - handle PCIe native PME event
+ * @target is suspected to invoke a wakeup event (PME)
+ * Note @target should be a PCIe device
+ */
+bool pci_pm_handle_wakeup_event_native(struct pci_dev *target)
+{
+	bool ret;
+	struct pci_dev *tmp = NULL;
+	int domain_nr, bus_start, bus_end;
+
+	BUG_ON(!target->is_pcie);
+
+	/* For PCIe PME, if the target isn't the source, do nothing */
+	ret = pci_pm_check_wakeup_event(target);
+
+	if (!target->subordinate || (target->pcie_type != PCI_EXP_TYPE_ROOT_PORT
+		&& target->pcie_type != PCI_EXP_TYPE_PCI_BRIDGE)) {
+		if (ret)
+			pm_runtime_resume(&target->dev);
+		return ret;
+	}
+
+	if (ret)
+		pm_request_resume(&target->dev);
+
+	/* scan legacy PCI devices under the bridge or root port */
+	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 (!tmp->is_pcie && pci_pm_check_wakeup_event(tmp)) {
+				ret = true;
+				pm_request_resume(&tmp->dev);
+			}
+		}
+	}
+	return ret;
+}
+
+/*
+ * pci_pm_handle_wakeup_event_platform - handle platform PCI wakeup event
+ * @target is suspected to invoke a wakeup event
+ */
+bool pci_pm_handle_wakeup_event_platform(struct pci_dev *target)
+{
+	bool ret = false;
+	struct pci_dev *tmp = NULL;
+
+	/*
+	 * In the platform approach (ACPI), target PME status might get cleared
+	 * by BIOS before OS receives a notification, so we haven't reliable
+	 * method to detect if target is the wakeup source, just trust it is
+	 */
+	pci_pm_check_wakeup_event(target);
+	pm_request_resume(&target->dev);
+	if (!target->subordinate)
+		ret = true;
+
+	/* scan all pci devices to find the wakeup source */
+	while ((tmp = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, tmp)) != NULL) {
+		if (tmp != target && pci_pm_check_wakeup_event(tmp)) {
+			ret = true;
+			pm_request_resume(&tmp->dev);
+		}
+	}
+	return ret;
+}
+
 #ifdef CONFIG_SUSPEND
 
 static int pci_pm_suspend(struct device *dev)
Index: linux/drivers/pci/pci.h
===================================================================
--- linux.orig/drivers/pci/pci.h	2009-08-25 15:38:50.000000000 +0800
+++ linux/drivers/pci/pci.h	2009-08-26 09:07:42.000000000 +0800
@@ -83,6 +83,9 @@ static inline void pci_vpd_release(struc
 		dev->vpd->ops->release(dev);
 }
 
+extern bool pci_pm_handle_wakeup_event_native(struct pci_dev *target);
+extern bool pci_pm_handle_wakeup_event_platform(struct pci_dev *target);
+
 /* PCI /proc functions */
 #ifdef CONFIG_PROC_FS
 extern int pci_proc_attach_device(struct pci_dev *dev);

Re: [PATCH 1/2] handle wakeup event in PCI

[Rafael J. Wysocki]

On Thursday 27 August 2009, Shaohua Li wrote:
Hi,

> Add an implementation how to detect wakeup event for PCI. PCI device can
> invoke PME, platform or PCIe native approach can collect the event and
> report to OS. OS should identify exactly which device invokes PME as
> several devices can share PME pin.
> 
> In platform approach (ACPI in this case), some BIOS give exact device which
> invokes PME but others doesn't. In either case, we always scan all devices
> to check which one is the wakeup source. In the meantime, if the device isn't a
> bridge, we trust the device is wakeup source even its PME status isn't set,
> because BIOS can clear PME status before OS gets notification and we have no
> reliable method to check if the device is the wakeup source.
> 
> In PCIe native approach, if PME source device is a pcie endpoint, the device
> is the exact PME source. If the device is root port or pcie-to-pci bridge,
> we need scan legacy devices in the hierarchy under the root port or bridge.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> ---
>  drivers/pci/pci-driver.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h        |    3 +
>  2 files changed, 109 insertions(+)
> 
> Index: linux/drivers/pci/pci-driver.c
> ===================================================================
> --- linux.orig/drivers/pci/pci-driver.c	2009-08-25 15:39:52.000000000 +0800
> +++ linux/drivers/pci/pci-driver.c	2009-08-27 14:15:34.000000000 +0800
> @@ -17,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/sched.h>
>  #include <linux/cpu.h>
> +#include <linux/pm_runtime.h>
>  #include "pci.h"
>  
>  /*
> @@ -570,6 +571,111 @@ static void pci_pm_complete(struct devic
>  		drv->pm->complete(dev);
>  }
>  
> +/*
> + * pci_pm_check_wakeup_event - pdev is suspected to invoke a wakeup event
> + * @pdev: the suspected pci device
> + *
> + * Clear PME status and disable PME, return if the pci device @pdev really
> + * invokes PME
> + **/
> +static bool pci_pm_check_wakeup_event(struct pci_dev *pdev)
> +{
> +	int pme_pos = pdev->pm_cap;
> +	u16 pmcsr;
> +	bool ret = false;
> +
> +	if (!pme_pos)
> +		return false;
> +
> +	/* 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) {
> +		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
> +		ret = true;
> +	}
> +
> +	pmcsr |= PCI_PM_CTRL_PME_STATUS;
> +	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
> +
> +	return ret;
> +}

The function above looks good.

I think it's better to move the one below into the PCIe PME code.

> +
> +/*
> + * pci_pm_handle_wakeup_event_native - handle PCIe native PME event
> + * @target is suspected to invoke a wakeup event (PME)
> + * Note @target should be a PCIe device
> + */
> +bool pci_pm_handle_wakeup_event_native(struct pci_dev *target)
> +{
> +	bool ret;
> +	struct pci_dev *tmp = NULL;
> +	int domain_nr, bus_start, bus_end;
> +
> +	BUG_ON(!target->is_pcie);
> +
> +	/* For PCIe PME, if the target isn't the source, do nothing */
> +	ret = pci_pm_check_wakeup_event(target);
> +
> +	if (!target->subordinate || (target->pcie_type != PCI_EXP_TYPE_ROOT_PORT
> +		&& target->pcie_type != PCI_EXP_TYPE_PCI_BRIDGE)) {
> +		if (ret)
> +			pm_runtime_resume(&target->dev);
> +		return ret;
> +	}
> +
> +	if (ret)
> +		pm_request_resume(&target->dev);

Here, if you want to busy loop in pcie_pme_work_handle(), it's better not to
use pm_runtime_resume() (sorry, I overlooked that before), because we should
first clear all PME_Status flags set and _then_ try to resume the devices.

So, the code you had here before was better:

+	ret = pci_pm_check_wakeup_event(target);
+	if (ret)
+		pm_request_resume(&target->dev);
+	if (is_regular_device(target))
+		return ret;

> +
> +	/* scan legacy PCI devices under the bridge or root port */
> +	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 (!tmp->is_pcie && pci_pm_check_wakeup_event(tmp)) {
> +				ret = true;
> +				pm_request_resume(&tmp->dev);
> +			}

Do we have to check ->is_pcie here?

Anyway, I'd do

+		if (pci_domain_nr(tmp->bus) != domain_nr
+		    || tmp->bus->number < bus_start || tmp->bus->number > bus_end)
+			continue;

and then the rest.

> +		}
> +	}
> +	return ret;
> +}
> +
> +/*
> + * pci_pm_handle_wakeup_event_platform - handle platform PCI wakeup event
> + * @target is suspected to invoke a wakeup event
> + */
> +bool pci_pm_handle_wakeup_event_platform(struct pci_dev *target)
> +{
> +	bool ret = false;
> +	struct pci_dev *tmp = NULL;
> +
> +	/*
> +	 * In the platform approach (ACPI), target PME status might get cleared
> +	 * by BIOS before OS receives a notification, so we haven't reliable
> +	 * method to detect if target is the wakeup source, just trust it is
> +	 */
> +	pci_pm_check_wakeup_event(target);
> +	pm_request_resume(&target->dev);
> +	if (!target->subordinate)
> +		ret = true;
> +
> +	/* scan all pci devices to find the wakeup source */
> +	while ((tmp = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, tmp)) != NULL) {

Doing

+		if (tmp == target)
+			continue;

separately would be cleaner IMO.

> +		if (tmp != target && pci_pm_check_wakeup_event(tmp)) {
> +			ret = true;
> +			pm_request_resume(&tmp->dev);
> +		}
> +	}
> +	return ret;
> +}
> +
>  #ifdef CONFIG_SUSPEND
>  
>  static int pci_pm_suspend(struct device *dev)
> Index: linux/drivers/pci/pci.h
> ===================================================================
> --- linux.orig/drivers/pci/pci.h	2009-08-25 15:38:50.000000000 +0800
> +++ linux/drivers/pci/pci.h	2009-08-26 09:07:42.000000000 +0800
> @@ -83,6 +83,9 @@ static inline void pci_vpd_release(struc
>  		dev->vpd->ops->release(dev);
>  }
>  
> +extern bool pci_pm_handle_wakeup_event_native(struct pci_dev *target);
> +extern bool pci_pm_handle_wakeup_event_platform(struct pci_dev *target);
> +
>  /* PCI /proc functions */
>  #ifdef CONFIG_PROC_FS
>  extern int pci_proc_attach_device(struct pci_dev *dev);
> 
> 

Thanks,
Rafael

Re: [PATCH 1/2] handle wakeup event in PCI

[Shaohua Li]

On Sun, Aug 30, 2009 at 04:26:33AM +0800, Rafael J. Wysocki wrote:
> On Thursday 27 August 2009, Shaohua Li wrote:
> Hi,
> 
> > Add an implementation how to detect wakeup event for PCI. PCI device can
> > invoke PME, platform or PCIe native approach can collect the event and
> > report to OS. OS should identify exactly which device invokes PME as
> > several devices can share PME pin.
> > 
> > In platform approach (ACPI in this case), some BIOS give exact device which
> > invokes PME but others doesn't. In either case, we always scan all devices
> > to check which one is the wakeup source. In the meantime, if the device isn't a
> > bridge, we trust the device is wakeup source even its PME status isn't set,
> > because BIOS can clear PME status before OS gets notification and we have no
> > reliable method to check if the device is the wakeup source.
> > 
> > In PCIe native approach, if PME source device is a pcie endpoint, the device
> > is the exact PME source. If the device is root port or pcie-to-pci bridge,
> > we need scan legacy devices in the hierarchy under the root port or bridge.
> > 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > ---
> >  drivers/pci/pci-driver.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/pci/pci.h        |    3 +
> >  2 files changed, 109 insertions(+)
> > 
> > Index: linux/drivers/pci/pci-driver.c
> > ===================================================================
> > --- linux.orig/drivers/pci/pci-driver.c	2009-08-25 15:39:52.000000000 +0800
> > +++ linux/drivers/pci/pci-driver.c	2009-08-27 14:15:34.000000000 +0800
> > @@ -17,6 +17,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/sched.h>
> >  #include <linux/cpu.h>
> > +#include <linux/pm_runtime.h>
> >  #include "pci.h"
> >  
> >  /*
> > @@ -570,6 +571,111 @@ static void pci_pm_complete(struct devic
> >  		drv->pm->complete(dev);
> >  }
> >  
> > +/*
> > + * pci_pm_check_wakeup_event - pdev is suspected to invoke a wakeup event
> > + * @pdev: the suspected pci device
> > + *
> > + * Clear PME status and disable PME, return if the pci device @pdev really
> > + * invokes PME
> > + **/
> > +static bool pci_pm_check_wakeup_event(struct pci_dev *pdev)
> > +{
> > +	int pme_pos = pdev->pm_cap;
> > +	u16 pmcsr;
> > +	bool ret = false;
> > +
> > +	if (!pme_pos)
> > +		return false;
> > +
> > +	/* 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) {
> > +		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
> > +		ret = true;
> > +	}
> > +
> > +	pmcsr |= PCI_PM_CTRL_PME_STATUS;
> > +	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
> > +
> > +	return ret;
> > +}
> 
> The function above looks good.
> 
> I think it's better to move the one below into the PCIe PME code.
> 
> > +
> > +/*
> > + * pci_pm_handle_wakeup_event_native - handle PCIe native PME event
> > + * @target is suspected to invoke a wakeup event (PME)
> > + * Note @target should be a PCIe device
> > + */
> > +bool pci_pm_handle_wakeup_event_native(struct pci_dev *target)
> > +{
> > +	bool ret;
> > +	struct pci_dev *tmp = NULL;
> > +	int domain_nr, bus_start, bus_end;
> > +
> > +	BUG_ON(!target->is_pcie);
> > +
> > +	/* For PCIe PME, if the target isn't the source, do nothing */
> > +	ret = pci_pm_check_wakeup_event(target);
> > +
> > +	if (!target->subordinate || (target->pcie_type != PCI_EXP_TYPE_ROOT_PORT
> > +		&& target->pcie_type != PCI_EXP_TYPE_PCI_BRIDGE)) {
> > +		if (ret)
> > +			pm_runtime_resume(&target->dev);
> > +		return ret;
> > +	}
> > +
> > +	if (ret)
> > +		pm_request_resume(&target->dev);
> 
> Here, if you want to busy loop in pcie_pme_work_handle(), it's better not to
> use pm_runtime_resume() (sorry, I overlooked that before), because we should
> first clear all PME_Status flags set and _then_ try to resume the devices.
> 
> So, the code you had here before was better:
> 
> +	ret = pci_pm_check_wakeup_event(target);
> +	if (ret)
> +		pm_request_resume(&target->dev);
> +	if (is_regular_device(target))
> +		return ret;
> 
> > +
> > +	/* scan legacy PCI devices under the bridge or root port */
> > +	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 (!tmp->is_pcie && pci_pm_check_wakeup_event(tmp)) {
> > +				ret = true;
> > +				pm_request_resume(&tmp->dev);
> > +			}
> 
> Do we have to check ->is_pcie here?
> 
> Anyway, I'd do
> 
> +		if (pci_domain_nr(tmp->bus) != domain_nr
> +		    || tmp->bus->number < bus_start || tmp->bus->number > bus_end)
> +			continue;
> 
> and then the rest.
> 
> > +		}
> > +	}
> > +	return ret;
> > +}
> > +
> > +/*
> > + * pci_pm_handle_wakeup_event_platform - handle platform PCI wakeup event
> > + * @target is suspected to invoke a wakeup event
> > + */
> > +bool pci_pm_handle_wakeup_event_platform(struct pci_dev *target)
> > +{
> > +	bool ret = false;
> > +	struct pci_dev *tmp = NULL;
> > +
> > +	/*
> > +	 * In the platform approach (ACPI), target PME status might get cleared
> > +	 * by BIOS before OS receives a notification, so we haven't reliable
> > +	 * method to detect if target is the wakeup source, just trust it is
> > +	 */
> > +	pci_pm_check_wakeup_event(target);
> > +	pm_request_resume(&target->dev);
> > +	if (!target->subordinate)
> > +		ret = true;
> > +
> > +	/* scan all pci devices to find the wakeup source */
> > +	while ((tmp = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, tmp)) != NULL) {
> 
> Doing
> 
> +		if (tmp == target)
> +			continue;
> 
> separately would be cleaner IMO.
> 
> > +		if (tmp != target && pci_pm_check_wakeup_event(tmp)) {
> > +			ret = true;
> > +			pm_request_resume(&tmp->dev);
> > +		}
> > +	}
> > +	return ret;
> > +}
> > +
> >  #ifdef CONFIG_SUSPEND
> >  
> >  static int pci_pm_suspend(struct device *dev)
> > Index: linux/drivers/pci/pci.h
> > ===================================================================
> > --- linux.orig/drivers/pci/pci.h	2009-08-25 15:38:50.000000000 +0800
> > +++ linux/drivers/pci/pci.h	2009-08-26 09:07:42.000000000 +0800
> > @@ -83,6 +83,9 @@ static inline void pci_vpd_release(struc
> >  		dev->vpd->ops->release(dev);
> >  }
> >  
> > +extern bool pci_pm_handle_wakeup_event_native(struct pci_dev *target);
> > +extern bool pci_pm_handle_wakeup_event_platform(struct pci_dev *target);
> > +
> >  /* PCI /proc functions */
> >  #ifdef CONFIG_PROC_FS
> >  extern int pci_proc_attach_device(struct pci_dev *dev);
Updated the patch.


Add an implementation how to detect wakeup event for PCI. PCI device can
invoke PME, platform or PCIe native approach can collect the event and
report to OS. OS should identify exactly which device invokes PME as
several devices can share PME pin.

In platform approach (ACPI in this case), some BIOS give exact device which
invokes PME but others doesn't. In either case, we always scan all devices
to check which one is the wakeup source. In the meantime, if the device isn't a
bridge, we trust the device is wakeup source even its PME status isn't set,
because BIOS can clear PME status before OS gets notification and we have no
reliable method to check if the device is the wakeup source.

In PCIe native approach, if PME source device is a pcie endpoint, the device
is the exact PME source. If the device is root port or pcie-to-pci bridge,
we need scan legacy devices in the hierarchy under the root port or bridge.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
 drivers/pci/pci-driver.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h        |    3 ++
 2 files changed, 68 insertions(+)

Index: linux/drivers/pci/pci-driver.c
===================================================================
--- linux.orig/drivers/pci/pci-driver.c	2009-08-31 17:26:23.000000000 +0800
+++ linux/drivers/pci/pci-driver.c	2009-08-31 17:28:04.000000000 +0800
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/cpu.h>
+#include <linux/pm_runtime.h>
 #include "pci.h"
 
 /*
@@ -570,6 +571,70 @@ static void pci_pm_complete(struct devic
 		drv->pm->complete(dev);
 }
 
+/*
+ * pci_pm_check_wakeup_event - pdev is suspected to invoke a wakeup event
+ * @pdev: the suspected pci device
+ *
+ * Clear PME status and disable PME, return if the pci device @pdev really
+ * invokes PME
+ **/
+bool pci_pm_check_wakeup_event(struct pci_dev *pdev)
+{
+	int pme_pos = pdev->pm_cap;
+	u16 pmcsr;
+	bool ret = false;
+
+	if (!pme_pos)
+		return false;
+
+	/* 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) {
+		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
+		ret = true;
+	}
+
+	pmcsr |= PCI_PM_CTRL_PME_STATUS;
+	pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
+
+	return ret;
+}
+
+/*
+ * pci_pm_handle_wakeup_event_platform - handle platform PCI wakeup event
+ * @target is suspected to invoke a wakeup event
+ */
+bool pci_pm_handle_wakeup_event_platform(struct pci_dev *target)
+{
+	bool ret = false;
+	struct pci_dev *tmp = NULL;
+
+	/*
+	 * In the platform approach (ACPI), target PME status might get cleared
+	 * by BIOS before OS receives a notification, so we haven't reliable
+	 * method to detect if target is the wakeup source, just trust it is
+	 */
+	pci_pm_check_wakeup_event(target);
+	pm_request_resume(&target->dev);
+	if (!target->subordinate)
+		ret = true;
+
+	/* scan all pci devices to find the wakeup source */
+	while ((tmp = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, tmp)) != NULL) {
+		if (tmp == target)
+			continue;
+		if (pci_pm_check_wakeup_event(tmp)) {
+			ret = true;
+			pm_request_resume(&tmp->dev);
+		}
+	}
+	return ret;
+}
+
 #ifdef CONFIG_SUSPEND
 
 static int pci_pm_suspend(struct device *dev)
Index: linux/drivers/pci/pci.h
===================================================================
--- linux.orig/drivers/pci/pci.h	2009-08-31 17:26:23.000000000 +0800
+++ linux/drivers/pci/pci.h	2009-08-31 17:28:04.000000000 +0800
@@ -83,6 +83,9 @@ static inline void pci_vpd_release(struc
 		dev->vpd->ops->release(dev);
 }
 
+extern bool pci_pm_check_wakeup_event(struct pci_dev *target);
+extern bool pci_pm_handle_wakeup_event_platform(struct pci_dev *target);
+
 /* PCI /proc functions */
 #ifdef CONFIG_PROC_FS
 extern int pci_proc_attach_device(struct pci_dev *dev);

[PATCH 2] PCI PM: Add function for checking PME status of devices (was: Re: [PATCH 1/2] handle wakeup event in PCI)

[Rafael J. Wysocki]

On Tuesday 01 September 2009, Shaohua Li wrote:
...
> Updated the patch.

I think it's better to add pci_pm_handle_wakeup_event_platform() along with the
remaining platform support code and the other function should go into pci.c
rather than into pci-driver.c.

So, for now, I think we only need the patch below.

Thanks,
Rafael

---
Subject: PCI PM: Add function for checking PME status of devices

Add function pci_check_pme_status() that will check the PME status
bit of given device and clear it along with the PME enable bit.  It
will be necessary for PCI run-time power management.

Based on a patch from Shaohua Li <shaohua.li@intel.com>

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c |   35 +++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h |    1 +
 2 files changed, 36 insertions(+)

Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -48,6 +48,7 @@ struct pci_platform_pm_ops {
 extern int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
 extern void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 extern void pci_disable_enabled_device(struct pci_dev *dev);
+extern bool pci_check_pme_status(struct pci_dev *dev);
 extern void pci_pm_init(struct pci_dev *dev);
 extern void platform_pci_wakeup_init(struct pci_dev *dev);
 extern void pci_allocate_cap_save_buffers(struct pci_dev *dev);
Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -1158,6 +1158,41 @@ int pci_set_pcie_reset_state(struct pci_
 }
 
 /**
+ * pci_check_pme_status - Check if given device has generated PME.
+ * @dev: Device to check.
+ *
+ * Check the PME status of the device, clear PME status and PME enable.  Return
+ * 'true' if PME has been generated by the device (and hasn't been spurious) or
+ * 'false' otherwise.
+ */
+bool pci_check_pme_status(struct pci_dev *dev)
+{
+	int pmcsr_pos;
+	u16 pmcsr;
+	bool ret = false;
+
+	if (!dev->pm_cap)
+		return false;
+
+	pmcsr_pos = dev->pm_cap + PCI_PM_CTRL;
+	/* clear PME status and disable PME to avoid interrupt flood */
+	pci_read_config_word(dev, pmcsr_pos, &pmcsr);
+	if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
+		return false;
+
+	pmcsr |= PCI_PM_CTRL_PME_STATUS;
+	/* Ignore spurious PME or clear PME enable if it's not spurious. */
+	if (pmcsr & PCI_PM_CTRL_PME_ENABLE) {
+		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
+		ret = true;
+	}
+
+	pci_write_config_word(dev, pmcsr_pos, pmcsr);
+
+	return ret;
+}
+
+/**
  * pci_pme_capable - check the capability of PCI device to generate PME#
  * @dev: PCI device to handle.
  * @state: PCI state from which device will issue PME#.