From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lukas Wunner Subject: Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound Date: Sun, 25 Feb 2018 09:59:00 +0100 Message-ID: <20180225085900.GA923@wunner.de> References: <20180220212922.GC32228@bhelgaas-glaptop.roam.corp.google.com> <6131920.sr0PL0JerP@aspire.rjw.lan> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from bmailout1.hostsharing.net (bmailout1.hostsharing.net [83.223.95.100]) by alsa0.perex.cz (Postfix) with ESMTP id 88978267048 for ; Sun, 25 Feb 2018 09:59:01 +0100 (CET) Content-Disposition: inline In-Reply-To: <6131920.sr0PL0JerP@aspire.rjw.lan> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: "Rafael J. Wysocki" Cc: Pierre Moreau , "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." , Lyude Paul , Linux PM , nouveau@lists.freedesktop.org, "Rafael J. Wysocki" , Takashi Iwai , dri-devel , Hans de Goede , Bjorn Helgaas , Peter Wu , Bastien Nocera , Linux PCI , Alex Deucher , Dave Airlie , Bjorn Helgaas , Ben Skeggs List-Id: alsa-devel@alsa-project.org On Wed, Feb 21, 2018 at 01:39:34PM +0100, Rafael J. Wysocki wrote: > On Wednesday, February 21, 2018 10:57:14 AM CET Rafael J. Wysocki wrote: > > So if pci_pm_runtime_suspend() is modified to call pci_save_state() > > before returning 0 in the !dev->driver case, we can just move the > > pci_restore_standard_config() invocation in pci_pm_runtime_resume() up > > to the very top and check dev->driver later. > > I mean something like the patch below, overall (untested). > > Tentatively-signed-off-by: Rafael J. Wysocki Okay I've tested this successfully now. I'll have to respin the series at least one more time to address the unnecessary initialization Bjorn spotted in patch [5/7] and will then replace patch [1/7] with this one. I'll wait a few more days before respinning to allow for further comments, in particular I'm hoping for feedback from Takashi and someone testing this on Optimus/ATPX. Thanks! Lukas > --- > drivers/pci/pci-driver.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > Index: linux-pm/drivers/pci/pci-driver.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci-driver.c > +++ linux-pm/drivers/pci/pci-driver.c > @@ -1224,11 +1224,14 @@ static int pci_pm_runtime_suspend(struct > int error; > > /* > - * If pci_dev->driver is not set (unbound), the device should > - * always remain in D0 regardless of the runtime PM status > + * If pci_dev->driver is not set (unbound), the device may go to D3cold, > + * but only if the bridge above it is suspended. In case that happens, > + * save its config space. > */ > - if (!pci_dev->driver) > + if (!pci_dev->driver) { > + pci_save_state(pci_dev); > return 0; > + } > > if (!pm || !pm->runtime_suspend) > return -ENOSYS; > @@ -1276,16 +1279,17 @@ static int pci_pm_runtime_resume(struct > const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL; > > /* > - * If pci_dev->driver is not set (unbound), the device should > - * always remain in D0 regardless of the runtime PM status > + * Regardless of whether or not the driver is there, the device might > + * have been put into D3cold as a result of suspending the bridge above > + * it, so restore the config spaces of all devices here. > */ > + pci_restore_standard_config(pci_dev); > if (!pci_dev->driver) > return 0; > > if (!pm || !pm->runtime_resume) > return -ENOSYS; > > - pci_restore_standard_config(pci_dev); > pci_fixup_device(pci_fixup_resume_early, pci_dev); > pci_enable_wake(pci_dev, PCI_D0, false); > pci_fixup_device(pci_fixup_resume, pci_dev); > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Date: Sun, 25 Feb 2018 09:59:00 +0100 From: Lukas Wunner To: "Rafael J. Wysocki" Cc: Bjorn Helgaas , dri-devel , Peter Wu , Alex Deucher , Dave Airlie , nouveau@lists.freedesktop.org, Ben Skeggs , Lyude Paul , Hans de Goede , "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." , Takashi Iwai , Jaroslav Kysela , Linux PM , "Rafael J. Wysocki" , Pierre Moreau , Bastien Nocera , Bjorn Helgaas , Linux PCI Subject: Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound Message-ID: <20180225085900.GA923@wunner.de> References: <20180220212922.GC32228@bhelgaas-glaptop.roam.corp.google.com> <6131920.sr0PL0JerP@aspire.rjw.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <6131920.sr0PL0JerP@aspire.rjw.lan> List-ID: On Wed, Feb 21, 2018 at 01:39:34PM +0100, Rafael J. Wysocki wrote: > On Wednesday, February 21, 2018 10:57:14 AM CET Rafael J. Wysocki wrote: > > So if pci_pm_runtime_suspend() is modified to call pci_save_state() > > before returning 0 in the !dev->driver case, we can just move the > > pci_restore_standard_config() invocation in pci_pm_runtime_resume() up > > to the very top and check dev->driver later. > > I mean something like the patch below, overall (untested). > > Tentatively-signed-off-by: Rafael J. Wysocki Okay I've tested this successfully now. I'll have to respin the series at least one more time to address the unnecessary initialization Bjorn spotted in patch [5/7] and will then replace patch [1/7] with this one. I'll wait a few more days before respinning to allow for further comments, in particular I'm hoping for feedback from Takashi and someone testing this on Optimus/ATPX. Thanks! Lukas > --- > drivers/pci/pci-driver.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > Index: linux-pm/drivers/pci/pci-driver.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci-driver.c > +++ linux-pm/drivers/pci/pci-driver.c > @@ -1224,11 +1224,14 @@ static int pci_pm_runtime_suspend(struct > int error; > > /* > - * If pci_dev->driver is not set (unbound), the device should > - * always remain in D0 regardless of the runtime PM status > + * If pci_dev->driver is not set (unbound), the device may go to D3cold, > + * but only if the bridge above it is suspended. In case that happens, > + * save its config space. > */ > - if (!pci_dev->driver) > + if (!pci_dev->driver) { > + pci_save_state(pci_dev); > return 0; > + } > > if (!pm || !pm->runtime_suspend) > return -ENOSYS; > @@ -1276,16 +1279,17 @@ static int pci_pm_runtime_resume(struct > const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL; > > /* > - * If pci_dev->driver is not set (unbound), the device should > - * always remain in D0 regardless of the runtime PM status > + * Regardless of whether or not the driver is there, the device might > + * have been put into D3cold as a result of suspending the bridge above > + * it, so restore the config spaces of all devices here. > */ > + pci_restore_standard_config(pci_dev); > if (!pci_dev->driver) > return 0; > > if (!pm || !pm->runtime_resume) > return -ENOSYS; > > - pci_restore_standard_config(pci_dev); > pci_fixup_device(pci_fixup_resume_early, pci_dev); > pci_enable_wake(pci_dev, PCI_D0, false); > pci_fixup_device(pci_fixup_resume, pci_dev); >