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

[Shaohua Li <shaohua.li@intel.com>]

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