Hi Mika and Bjorn, > -----Original Message----- > From: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> > Sent: Wednesday, November 18, 2020 11:56 PM > To: Bjorn Helgaas <helgaas@xxxxxxxxxx> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; Rafael J. Wysocki > <rjw@xxxxxxxxxxxxx>; Lukas Wunner <lukas@xxxxxxxxx>; David Airlie > <airlied@xxxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>; Patel, Utkarsh H > <utkarsh.h.patel@xxxxxxxxx>; Maarten Lankhorst > <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard <mripard@xxxxxxxxxx>; > Thomas Zimmermann <tzimmermann@xxxxxxx>; linux-pci@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] PCI/PM: Do not generate wakeup event when runtime > resuming bus > > 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@xxxxxxxxx> > > > > 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? This is reported only Google partner bug which is private and I am not sure if I can share the link here. > > 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@xxxxxxxxxxxxxxx> > > > --- > > > 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. Sincerely, Utkarsh Patel.
