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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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

The function above looks good.

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

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

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

So, the code you had here before was better:

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

> +
> +	/* scan legacy PCI devices under the bridge or root port */
> +	domain_nr = pci_domain_nr(target->bus);
> +	bus_start = target->subordinate->secondary;
> +	bus_end = target->subordinate->subordinate;
> +	while ((tmp = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, tmp)) != NULL) {
> +		if (pci_domain_nr(tmp->bus) == domain_nr &&
> +		   tmp->bus->number >= bus_start &&
> +		   tmp->bus->number <= bus_end) {
> +			if (!tmp->is_pcie && pci_pm_check_wakeup_event(tmp)) {
> +				ret = true;
> +				pm_request_resume(&tmp->dev);
> +			}

Do we have to check ->is_pcie here?

Anyway, I'd do

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

and then the rest.

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

Doing

+		if (tmp == target)
+			continue;

separately would be cleaner IMO.

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

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux