Re: [PATCH] PCI/PM: Do not generate wakeup event when runtime resuming bus

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi Bjorn,

On Wed, Nov 18, 2020 at 03:22:00PM -0600, Bjorn Helgaas wrote:
> On Thu, Oct 29, 2020 at 12:24:53PM +0300, Mika Westerberg wrote:
> > When a PCI bridge is runtime resumed from D3cold the underlying bus is
> > walked and the attached devices are runtime resumed as well. However, in
> > addition to that we also generate a wakeup event for these devices even
> > though this actually is not a real wakeup event coming from the
> > hardware.
> > 
> > Normally this does not cause problems but when combined with
> > /sys/power/wakeup_count like using the steps below:
> > 
> >   # count=$(cat /sys/power/wakeup_count)
> >   # echo $count > /sys/power/wakeup_count
> >   # echo mem > /sys/power/state
> > 
> > The system suspend cycle might get aborted at this point if a PCI bridge
> > that was runtime suspended (D3cold) was runtime resumed for any reason.
> > The runtime resume calls pci_wakeup_bus() and that generates wakeup
> > event increasing wakeup_count.
> > 
> > Since this is not a real wakeup event we can prevent the above from
> > happening by removing the call to pci_wakeup_event() in
> > pci_wakeup_bus(). While there rename pci_wakeup_bus() to
> > pci_resume_bus() to better reflect what it does.
> > 
> > Reported-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
> 
> Is there a URL to a report on a mailing list or a bugzilla that we can
> include here?  What was the actual user-visible issue?  If we can
> mention it here, it may be useful to others who encounter the same
> issue.  I guess maybe a system suspend fails?

I'm not sure if there is bugzilla entry about this. There might be a
Google partner bug but not sure if it is public.

@Utkarsh, if there is one can you share that link with Bjorn?

There are two user visible issues, first is that if you do the above
steps manually the suspend gets aborted (as the above commit log tries
to explain).

The second "user visible" issue is that the ChromeOS suspend stress test
script below fails (as it does the same steps):

  https://chromium.googlesource.com/chromiumos/platform/power_manager/+/refs/heads/master/tools/suspend_stress_test

> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/gpu/vga/vga_switcheroo.c |  2 +-
> >  drivers/pci/pci.c                | 16 +++++-----------
> >  include/linux/pci.h              |  2 +-
> >  3 files changed, 7 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> > index 087304b1a5d7..8843b078ad4e 100644
> > --- a/drivers/gpu/vga/vga_switcheroo.c
> > +++ b/drivers/gpu/vga/vga_switcheroo.c
> > @@ -1039,7 +1039,7 @@ static int vga_switcheroo_runtime_resume(struct device *dev)
> >  	mutex_lock(&vgasr_mutex);
> >  	vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_ON);
> >  	mutex_unlock(&vgasr_mutex);
> > -	pci_wakeup_bus(pdev->bus);
> > +	pci_resume_bus(pdev->bus);
> >  	ret = dev->bus->pm->runtime_resume(dev);
> >  	if (ret)
> >  		return ret;
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 6d4d5a2f923d..b25dfa63eeb9 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1174,26 +1174,20 @@ int pci_platform_power_transition(struct pci_dev *dev, pci_power_t state)
> >  }
> >  EXPORT_SYMBOL_GPL(pci_platform_power_transition);
> >  
> > -/**
> > - * pci_wakeup - Wake up a PCI device
> > - * @pci_dev: Device to handle.
> > - * @ign: ignored parameter
> > - */
> > -static int pci_wakeup(struct pci_dev *pci_dev, void *ign)
> > +static int pci_resume_one(struct pci_dev *pci_dev, void *ign)
> >  {
> > -	pci_wakeup_event(pci_dev);
> 
> IIUC this is the critical change, and all the rest of this patch is
> no-op renames.  Can you split this into two patches so the important
> change is more obvious?

Sure.

> Then the obvious questions will be why it is safe to remove this, and
> where the desired call for the real wakeup is.

This is only called on runtime resume path to turn on devices below this
one. However, wakeup is only relevant on system sleep path.

For ACPI backed devices the real wakeup is handled in the
pci_acpi_wake_dev() and in case of PME it is pcie_pme_handle_request().
And then there is the pme_poll thread as well.