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@xxxxxxxxx>
> > > + *
> > > + * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
