[RFC 0/5] device wakeup event support v2

[RFC 0/5] device wakeup event support v2

[Shaohua Li]

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

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

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

[RFC 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);
 };
 
 /**

-- 

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

[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

-- 

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

[RFC 3/5] pci wakeup handler

[Shaohua Li]

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

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

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

-- 

Re: [RFC 3/5] pci wakeup handler

[Rafael J. Wysocki]

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

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

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

Use kerneldoc format of the comment, please.

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

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

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

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

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

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

> +	return false;
> +}
> +

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

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

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

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

More details in the comment, please.

> +		device_receive_wakeup_event(&target->dev);

Why do we use device_receive_wakeup_event() here?

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

And here?  What's the idea?

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

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

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

What exactly is the role of device_receive_wakeup_event() here?

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

What exactly is the point of introducing this function?

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

Thanks,
Rafael

Re: [RFC 3/5] pci wakeup handler

[Shaohua Li]

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

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

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

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

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

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

Thanks,
Shaohua

Re: [RFC 3/5] pci wakeup handler

[Rafael J. Wysocki]

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

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

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

HTH

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

-- 

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

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

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

[RFC 0/5] device wakeup event support

[shaohua.li]

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

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

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

Comments and suggestions are welcome!

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

[RFC 4/5] PCIe native PME detection

[shaohua.li]

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

This implements PCIe native PME detection.

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

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

-- 

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

[Rafael J. Wysocki]

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

Could you please describe how this is done?

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

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

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

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

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

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

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

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

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

Same here?

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

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

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

I'd use

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

to simplify the references here.

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

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

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

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

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

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

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

and drop the two lines above.

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

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

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

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

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

bdf has been defined as u16.  Is that intentional?

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

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

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

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

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

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

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

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

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

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

Thanks,
Rafael

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

[Li, Shaohua]

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

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

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

Thanks,
Shaohua