linux-kernel - Re: [RFC v2 3/5] PCIe, Add runtime PM support to PCIe port

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1336723068.6487.8.camel@yhuang-dev>
Date:	Fri, 11 May 2012 15:57:48 +0800
From:	Huang Ying <ying.huang@...el.com>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	huang ying <huang.ying.caritas@...il.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>, ming.m.lin@...el.com,
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	Zheng Yan <zheng.z.yan@...el.com>
Subject: Re: [RFC v2 3/5] PCIe, Add runtime PM support to PCIe port

On Mon, 2012-05-07 at 23:00 +0200, Rafael J. Wysocki wrote:
> On Saturday, May 05, 2012, huang ying wrote:
> > On Sat, May 5, 2012 at 3:43 AM, Rafael J. Wysocki <rjw@...k.pl> wrote:
> > > On Friday, May 04, 2012, Huang Ying wrote:
> > >> From: Zheng Yan <zheng.z.yan@...el.com>
> > >>
> > >> This patch adds runtime PM support to PCIe port.  This is needed by
> > >> PCIe D3cold support, where PCIe device in slot may be powered on/off
> > >> by PCIe port.
> > >>
> > >> Because runtime suspend is broken for some chipset, a white list is
> > >> used to enable runtime PM support for only chipset known to work.
> > >>
> > >> Signed-off-by: Zheng Yan <zheng.z.yan@...el.com>
> > >> Signed-off-by: Huang Ying <ying.huang@...el.com>
> > >> ---
> > >>  drivers/pci/pci.c              |    9 +++++++++
> > >>  drivers/pci/pcie/portdrv_pci.c |   40 ++++++++++++++++++++++++++++++++++++++++
> > >>  2 files changed, 49 insertions(+)
> > >>
> > >> --- a/drivers/pci/pci.c
> > >> +++ b/drivers/pci/pci.c
> > >> @@ -1476,6 +1476,15 @@ bool pci_check_pme_status(struct pci_dev
> > >>   */
> > >>  static int pci_pme_wakeup(struct pci_dev *dev, void *pme_poll_reset)
> > >>  {
> > >> +     struct pci_dev *bridge = dev->bus->self;
> > >> +
> > >> +     /*
> > >> +      * If bridge is in low power state, the configuration space of
> > >> +      * subordinate devices may be not accessible
> > >
> > > Please also say in the comment _when_ this is possible.  That's far from
> > > obvious, because the runtime PM framework generally ensures that parents are
> > > resumed before the children, so the comment should describe the particular
> > > scenario leading to this situation.
> > 
> > OK.  I will add something like below into comments.
> > 
> > This is possible when doing PME poll.
> 
> Well, that doesn't really explain much. :-)
> 
> I _think_ the situation is when a device causes WAKE# to be generated and
> the platform receives a GPE as a result and we get an ACPI_NOTIFY_DEVICE_WAKE
> notification for the device, which may be under a bridge (PCIe port really)
> in D3_cold.  Is that the case?

That is not the case now.  Because we do not use pci_pme_wakeup_bus() in
ACPI handler now.

But for PME poll, if some subordinate devices under a bridge is put in
low power state, we still need to access configuration space of
subordinate devices to poll PME status even when bridge is in low power
state.  That may not work.

> > >> +      */
> > >> +     if (bridge && bridge->current_state != PCI_D0)
> > >> +             return 0;
> > >> +
> > >>       if (pme_poll_reset && dev->pme_poll)
> > >>               dev->pme_poll = false;
> > >>
> > >> --- a/drivers/pci/pcie/portdrv_pci.c
> > >> +++ b/drivers/pci/pcie/portdrv_pci.c
> > >> @@ -11,6 +11,7 @@
> > >>  #include <linux/kernel.h>
> > >>  #include <linux/errno.h>
> > >>  #include <linux/pm.h>
> > >> +#include <linux/pm_runtime.h>
> > >>  #include <linux/init.h>
> > >>  #include <linux/pcieport_if.h>
> > >>  #include <linux/aer.h>
> > >> @@ -99,6 +100,27 @@ static int pcie_port_resume_noirq(struct
> > >>       return 0;
> > >>  }
> > >>
> > >> +#ifdef CONFIG_PM_RUNTIME
> > >> +static int pcie_port_runtime_suspend(struct device *dev)
> > >> +{
> > >> +     struct pci_dev *pdev = to_pci_dev(dev);
> > >> +
> > >
> > > A comment explaining why this is needed here would be welcome.
> > 
> > Sorry, do not catch your idea exactly.  What is needed?  Why do we
> > need to add PCIe port runtime suspend support?
> 
> No, why we need to call pci_save_state() from here and pci_restore_state()
> from the corresponding resume routine.
> 
> In theory that shouldn't be necessary, because the PCI bus type's
> runtime suspend/resume routines do the save/restore of the PCI config space.

I found maybe we do not need this special processing, will test more.

Best Regards,
Huang Ying


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/