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

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Friday, May 11, 2012, Huang Ying wrote:
> 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@sisk.pl> wrote:
> > > > On Friday, May 04, 2012, Huang Ying wrote:
> > > >> From: Zheng Yan <zheng.z.yan@intel.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@intel.com>
> > > >> Signed-off-by: Huang Ying <ying.huang@intel.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.

OK, I see.  You mean that pci_pme_list_scan() should avoid checking the
PME status for devices whose parents (bridges) are in low-power states, right?

I think that that check should be made by pci_pme_list_scan() itself, then.

Thanks,
Rafael