| Message ID | KU1P153MB016637CAEAD346F0AA8E3801BFAD0@KU1P153MB0166.APCP153.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2] PCI: PM: Move to D0 before calling pci_legacy_resume_early() | expand |
> From: Dexuan Cui <decui@microsoft.com> > Sent: Tuesday, August 13, 2019 6:07 PM > To: lorenzo.pieralisi@arm.com; bhelgaas@google.com; > linux-pci@vger.kernel.org > Cc: Michael Kelley <mikelley@microsoft.com>; linux-hyperv@vger.kernel.org; > linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; Sasha > Levin <Alexander.Levin@microsoft.com>; Haiyang Zhang > <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>; > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com; vkuznets > <vkuznets@redhat.com>; marcelo.cerri@canonical.com; Stephen Hemminger > <sthemmin@microsoft.com>; jackm@mellanox.com; Dexuan Cui > <decui@microsoft.com> > Subject: [PATCH v2] PCI: PM: Move to D0 before calling > pci_legacy_resume_early() > > > In pci_legacy_suspend_late(), the device state is moved to PCI_UNKNOWN. > > In pci_pm_thaw_noirq(), the state is supposed to be moved back to PCI_D0, > but the current code misses the pci_legacy_resume_early() path, so the > state remains in PCI_UNKNOWN in that path. As a result, in the resume > phase of hibernation, this causes an error for the Mellanox VF driver, > which fails to enable MSI-X because pci_msi_supported() is false due > to dev->current_state != PCI_D0: > > mlx4_core a6d1:00:02.0: Detected virtual function - running in slave mode > mlx4_core a6d1:00:02.0: Sending reset > mlx4_core a6d1:00:02.0: Sending vhcr0 > mlx4_core a6d1:00:02.0: HCA minimum page size:512 > mlx4_core a6d1:00:02.0: Timestamping is not supported in slave mode > mlx4_core a6d1:00:02.0: INTx is not supported in multi-function mode, > aborting > PM: dpm_run_callback(): pci_pm_thaw+0x0/0xd7 returns -95 > PM: Device a6d1:00:02.0 failed to thaw: error -95 > > To be more accurate, the "resume" phase means the "thaw" callbacks which > run before the system enters hibernation: when the user runs the command > "echo disk > /sys/power/state" for hibernation, first the kernel "freezes" > all the devices and creates a hibernation image, then the kernel "thaws" > the devices including the disk/NIC, writes the memory to the disk, and > powers down. This patch fixes the error message for the Mellanox VF driver > in this phase. > > When the system starts again, a fresh kernel starts to run, and when the > kernel detects that a hibernation image was saved, the kernel "quiesces" > the devices, and then "restores" the devices from the saved image. In this > path: > device_resume_noirq() -> ... -> > pci_pm_restore_noirq() -> > pci_pm_default_resume_early() -> > pci_power_up() moves the device states back to PCI_D0. This path is > not broken and doesn't need my patch. > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > --- > > changes in v2: > Updated the changelog with more details. > > drivers/pci/pci-driver.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index 36dbe960306b..27dfc68db9e7 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -1074,15 +1074,16 @@ static int pci_pm_thaw_noirq(struct device *dev) > return error; > } > > - if (pci_has_legacy_pm_support(pci_dev)) > - return pci_legacy_resume_early(dev); > - > /* > * pci_restore_state() requires the device to be in D0 (because of MSI > * restoration among other things), so force it into D0 in case the > * driver's "freeze" callbacks put it into a low-power state directly. > */ > pci_set_power_state(pci_dev, PCI_D0); > + > + if (pci_has_legacy_pm_support(pci_dev)) > + return pci_legacy_resume_early(dev); > + > pci_restore_state(pci_dev); > > if (drv && drv->pm && drv->pm->thaw_noirq) > -- Hi, Lorenzo, Bjorn, Can you please take a look at the v2 ? Thanks, -- Dexuan
> From: devel <driverdev-devel-bounces@linuxdriverproject.org> On Behalf Of > Dexuan Cui > Sent: Monday, September 2, 2019 5:35 PM > To: lorenzo.pieralisi@arm.com; bhelgaas@google.com; > linux-pci@vger.kernel.org > [..snipped...] > > --- > > > > changes in v2: > > Updated the changelog with more details. > > > > drivers/pci/pci-driver.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > > index 36dbe960306b..27dfc68db9e7 100644 > > --- a/drivers/pci/pci-driver.c > > +++ b/drivers/pci/pci-driver.c > > @@ -1074,15 +1074,16 @@ static int pci_pm_thaw_noirq(struct device > *dev) > > return error; > > } > > > > - if (pci_has_legacy_pm_support(pci_dev)) > > - return pci_legacy_resume_early(dev); > > - > > /* > > * pci_restore_state() requires the device to be in D0 (because of MSI > > * restoration among other things), so force it into D0 in case the > > * driver's "freeze" callbacks put it into a low-power state directly. > > */ > > pci_set_power_state(pci_dev, PCI_D0); > > + > > + if (pci_has_legacy_pm_support(pci_dev)) > > + return pci_legacy_resume_early(dev); > > + > > pci_restore_state(pci_dev); > > > > if (drv && drv->pm && drv->pm->thaw_noirq) > > -- > > Hi, Lorenzo, Bjorn, > > Can you please take a look at the v2 ? > > -- Dexuan Hi Lorenzo, Bjorn, and all, It looks this patch has not been acked by anyone. Should I resend it? There is no change since v2. Thanks, -- Dexuan
On Wed, Aug 14, 2019 at 01:06:55AM +0000, Dexuan Cui wrote: > > In pci_legacy_suspend_late(), the device state is moved to PCI_UNKNOWN. > > In pci_pm_thaw_noirq(), the state is supposed to be moved back to PCI_D0, > but the current code misses the pci_legacy_resume_early() path, so the > state remains in PCI_UNKNOWN in that path. As a result, in the resume > phase of hibernation, this causes an error for the Mellanox VF driver, > which fails to enable MSI-X because pci_msi_supported() is false due > to dev->current_state != PCI_D0: > > mlx4_core a6d1:00:02.0: Detected virtual function - running in slave mode > mlx4_core a6d1:00:02.0: Sending reset > mlx4_core a6d1:00:02.0: Sending vhcr0 > mlx4_core a6d1:00:02.0: HCA minimum page size:512 > mlx4_core a6d1:00:02.0: Timestamping is not supported in slave mode > mlx4_core a6d1:00:02.0: INTx is not supported in multi-function mode, aborting > PM: dpm_run_callback(): pci_pm_thaw+0x0/0xd7 returns -95 > PM: Device a6d1:00:02.0 failed to thaw: error -95 > > To be more accurate, the "resume" phase means the "thaw" callbacks which > run before the system enters hibernation: when the user runs the command > "echo disk > /sys/power/state" for hibernation, first the kernel "freezes" > all the devices and creates a hibernation image, then the kernel "thaws" > the devices including the disk/NIC, writes the memory to the disk, and > powers down. This patch fixes the error message for the Mellanox VF driver > in this phase. > > When the system starts again, a fresh kernel starts to run, and when the > kernel detects that a hibernation image was saved, the kernel "quiesces" > the devices, and then "restores" the devices from the saved image. In this > path: > device_resume_noirq() -> ... -> > pci_pm_restore_noirq() -> > pci_pm_default_resume_early() -> > pci_power_up() moves the device states back to PCI_D0. This path is > not broken and doesn't need my patch. > > Signed-off-by: Dexuan Cui <decui@microsoft.com> This looks like a bugfix for 5839ee7389e8 ("PCI / PM: Force devices to D0 in pci_pm_thaw_noirq()") so maybe it should be marked for stable as 5839ee7389e8 was? Rafael, could you confirm? > --- > > changes in v2: > Updated the changelog with more details. > > drivers/pci/pci-driver.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index 36dbe960306b..27dfc68db9e7 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -1074,15 +1074,16 @@ static int pci_pm_thaw_noirq(struct device *dev) > return error; > } > > - if (pci_has_legacy_pm_support(pci_dev)) > - return pci_legacy_resume_early(dev); > - > /* > * pci_restore_state() requires the device to be in D0 (because of MSI > * restoration among other things), so force it into D0 in case the > * driver's "freeze" callbacks put it into a low-power state directly. > */ > pci_set_power_state(pci_dev, PCI_D0); > + > + if (pci_has_legacy_pm_support(pci_dev)) > + return pci_legacy_resume_early(dev); > + > pci_restore_state(pci_dev); > > if (drv && drv->pm && drv->pm->thaw_noirq) > -- > 2.19.1 >
> -----Original Message----- > From: Bjorn Helgaas <helgaas@kernel.org> > Sent: Monday, October 7, 2019 6:24 AM > To: Dexuan Cui <decui@microsoft.com> > Cc: lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; Michael Kelley > <mikelley@microsoft.com>; linux-hyperv@vger.kernel.org; > linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; Sasha > Levin <Alexander.Levin@microsoft.com>; Haiyang Zhang > <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>; > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com; vkuznets > <vkuznets@redhat.com>; marcelo.cerri@canonical.com; Stephen Hemminger > <sthemmin@microsoft.com>; jackm@mellanox.com > Subject: Re: [PATCH v2] PCI: PM: Move to D0 before calling > pci_legacy_resume_early() > > On Wed, Aug 14, 2019 at 01:06:55AM +0000, Dexuan Cui wrote: > > > > In pci_legacy_suspend_late(), the device state is moved to PCI_UNKNOWN. > > > > In pci_pm_thaw_noirq(), the state is supposed to be moved back to PCI_D0, > > but the current code misses the pci_legacy_resume_early() path, so the > > state remains in PCI_UNKNOWN in that path. As a result, in the resume > > phase of hibernation, this causes an error for the Mellanox VF driver, > > which fails to enable MSI-X because pci_msi_supported() is false due > > to dev->current_state != PCI_D0: > > > > mlx4_core a6d1:00:02.0: Detected virtual function - running in slave mode > > mlx4_core a6d1:00:02.0: Sending reset > > mlx4_core a6d1:00:02.0: Sending vhcr0 > > mlx4_core a6d1:00:02.0: HCA minimum page size:512 > > mlx4_core a6d1:00:02.0: Timestamping is not supported in slave mode > > mlx4_core a6d1:00:02.0: INTx is not supported in multi-function mode, > aborting > > PM: dpm_run_callback(): pci_pm_thaw+0x0/0xd7 returns -95 > > PM: Device a6d1:00:02.0 failed to thaw: error -95 > > > > To be more accurate, the "resume" phase means the "thaw" callbacks which > > run before the system enters hibernation: when the user runs the command > > "echo disk > /sys/power/state" for hibernation, first the kernel "freezes" > > all the devices and creates a hibernation image, then the kernel "thaws" > > the devices including the disk/NIC, writes the memory to the disk, and > > powers down. This patch fixes the error message for the Mellanox VF driver > > in this phase. > > > > When the system starts again, a fresh kernel starts to run, and when the > > kernel detects that a hibernation image was saved, the kernel "quiesces" > > the devices, and then "restores" the devices from the saved image. In this > > path: > > device_resume_noirq() -> ... -> > > pci_pm_restore_noirq() -> > > pci_pm_default_resume_early() -> > > pci_power_up() moves the device states back to PCI_D0. This path is > > not broken and doesn't need my patch. > > > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > > This looks like a bugfix for 5839ee7389e8 ("PCI / PM: Force devices to > D0 in pci_pm_thaw_noirq()") so maybe it should be marked for stable as > 5839ee7389e8 was? > > Rafael, could you confirm? > > > --- > > > > changes in v2: > > Updated the changelog with more details. > > > > drivers/pci/pci-driver.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > > index 36dbe960306b..27dfc68db9e7 100644 > > --- a/drivers/pci/pci-driver.c > > +++ b/drivers/pci/pci-driver.c > > @@ -1074,15 +1074,16 @@ static int pci_pm_thaw_noirq(struct device > *dev) > > return error; > > } > > > > - if (pci_has_legacy_pm_support(pci_dev)) > > - return pci_legacy_resume_early(dev); > > - > > /* > > * pci_restore_state() requires the device to be in D0 (because of MSI > > * restoration among other things), so force it into D0 in case the > > * driver's "freeze" callbacks put it into a low-power state directly. > > */ > > pci_set_power_state(pci_dev, PCI_D0); > > + > > + if (pci_has_legacy_pm_support(pci_dev)) > > + return pci_legacy_resume_early(dev); > > + > > pci_restore_state(pci_dev); > > > > if (drv && drv->pm && drv->pm->thaw_noirq) > > -- > > 2.19.1 > > Added Rafael to "To". Thanks, -- Dexuan
On 10/7/2019 8:57 PM, Dexuan Cui wrote: >> -----Original Message----- >> From: Bjorn Helgaas <helgaas@kernel.org> >> Sent: Monday, October 7, 2019 6:24 AM >> To: Dexuan Cui <decui@microsoft.com> >> Cc: lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; Michael Kelley >> <mikelley@microsoft.com>; linux-hyperv@vger.kernel.org; >> linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; Sasha >> Levin <Alexander.Levin@microsoft.com>; Haiyang Zhang >> <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>; >> olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com; vkuznets >> <vkuznets@redhat.com>; marcelo.cerri@canonical.com; Stephen Hemminger >> <sthemmin@microsoft.com>; jackm@mellanox.com >> Subject: Re: [PATCH v2] PCI: PM: Move to D0 before calling >> pci_legacy_resume_early() >> >> On Wed, Aug 14, 2019 at 01:06:55AM +0000, Dexuan Cui wrote: >>> In pci_legacy_suspend_late(), the device state is moved to PCI_UNKNOWN. >>> >>> In pci_pm_thaw_noirq(), the state is supposed to be moved back to PCI_D0, >>> but the current code misses the pci_legacy_resume_early() path, so the >>> state remains in PCI_UNKNOWN in that path. As a result, in the resume >>> phase of hibernation, this causes an error for the Mellanox VF driver, >>> which fails to enable MSI-X because pci_msi_supported() is false due >>> to dev->current_state != PCI_D0: >>> >>> mlx4_core a6d1:00:02.0: Detected virtual function - running in slave mode >>> mlx4_core a6d1:00:02.0: Sending reset >>> mlx4_core a6d1:00:02.0: Sending vhcr0 >>> mlx4_core a6d1:00:02.0: HCA minimum page size:512 >>> mlx4_core a6d1:00:02.0: Timestamping is not supported in slave mode >>> mlx4_core a6d1:00:02.0: INTx is not supported in multi-function mode, >> aborting >>> PM: dpm_run_callback(): pci_pm_thaw+0x0/0xd7 returns -95 >>> PM: Device a6d1:00:02.0 failed to thaw: error -95 >>> >>> To be more accurate, the "resume" phase means the "thaw" callbacks which >>> run before the system enters hibernation: when the user runs the command >>> "echo disk > /sys/power/state" for hibernation, first the kernel "freezes" >>> all the devices and creates a hibernation image, then the kernel "thaws" >>> the devices including the disk/NIC, writes the memory to the disk, and >>> powers down. This patch fixes the error message for the Mellanox VF driver >>> in this phase. >>> >>> When the system starts again, a fresh kernel starts to run, and when the >>> kernel detects that a hibernation image was saved, the kernel "quiesces" >>> the devices, and then "restores" the devices from the saved image. In this >>> path: >>> device_resume_noirq() -> ... -> >>> pci_pm_restore_noirq() -> >>> pci_pm_default_resume_early() -> >>> pci_power_up() moves the device states back to PCI_D0. This path is >>> not broken and doesn't need my patch. >>> >>> Signed-off-by: Dexuan Cui <decui@microsoft.com> >> This looks like a bugfix for 5839ee7389e8 ("PCI / PM: Force devices to >> D0 in pci_pm_thaw_noirq()") so maybe it should be marked for stable as >> 5839ee7389e8 was? >> >> Rafael, could you confirm? No, it is not a bug fix for that commit. The underlying issue would be there without that commit too. >>> --- >>> >>> changes in v2: >>> Updated the changelog with more details. >>> >>> drivers/pci/pci-driver.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c >>> index 36dbe960306b..27dfc68db9e7 100644 >>> --- a/drivers/pci/pci-driver.c >>> +++ b/drivers/pci/pci-driver.c >>> @@ -1074,15 +1074,16 @@ static int pci_pm_thaw_noirq(struct device >> *dev) >>> return error; >>> } >>> >>> - if (pci_has_legacy_pm_support(pci_dev)) >>> - return pci_legacy_resume_early(dev); >>> - >>> /* >>> * pci_restore_state() requires the device to be in D0 (because of MSI >>> * restoration among other things), so force it into D0 in case the >>> * driver's "freeze" callbacks put it into a low-power state directly. >>> */ >>> pci_set_power_state(pci_dev, PCI_D0); >>> + >>> + if (pci_has_legacy_pm_support(pci_dev)) >>> + return pci_legacy_resume_early(dev); >>> + >>> pci_restore_state(pci_dev); >>> >>> if (drv && drv->pm && drv->pm->thaw_noirq) >>> -- >>> 2.19.1 >>> The patch looks reasonable to me, but the comment above the pci_set_power_state() call needs to be updated too IMO.
On Tue, Oct 08, 2019 at 07:32:27PM +0200, Rafael J. Wysocki wrote: > On 10/7/2019 8:57 PM, Dexuan Cui wrote: > > > -----Original Message----- > > > From: Bjorn Helgaas <helgaas@kernel.org> > > > Sent: Monday, October 7, 2019 6:24 AM > > > To: Dexuan Cui <decui@microsoft.com> > > > Cc: lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; Michael Kelley > > > <mikelley@microsoft.com>; linux-hyperv@vger.kernel.org; > > > linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; Sasha > > > Levin <Alexander.Levin@microsoft.com>; Haiyang Zhang > > > <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>; > > > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com; vkuznets > > > <vkuznets@redhat.com>; marcelo.cerri@canonical.com; Stephen Hemminger > > > <sthemmin@microsoft.com>; jackm@mellanox.com > > > Subject: Re: [PATCH v2] PCI: PM: Move to D0 before calling > > > pci_legacy_resume_early() > > > > > > On Wed, Aug 14, 2019 at 01:06:55AM +0000, Dexuan Cui wrote: > > > > In pci_legacy_suspend_late(), the device state is moved to PCI_UNKNOWN. > > > > > > > > In pci_pm_thaw_noirq(), the state is supposed to be moved back to PCI_D0, > > > > but the current code misses the pci_legacy_resume_early() path, so the > > > > state remains in PCI_UNKNOWN in that path. As a result, in the resume > > > > phase of hibernation, this causes an error for the Mellanox VF driver, > > > > which fails to enable MSI-X because pci_msi_supported() is false due > > > > to dev->current_state != PCI_D0: > > > > > > > > mlx4_core a6d1:00:02.0: Detected virtual function - running in slave mode > > > > mlx4_core a6d1:00:02.0: Sending reset > > > > mlx4_core a6d1:00:02.0: Sending vhcr0 > > > > mlx4_core a6d1:00:02.0: HCA minimum page size:512 > > > > mlx4_core a6d1:00:02.0: Timestamping is not supported in slave mode > > > > mlx4_core a6d1:00:02.0: INTx is not supported in multi-function mode, > > > aborting > > > > PM: dpm_run_callback(): pci_pm_thaw+0x0/0xd7 returns -95 > > > > PM: Device a6d1:00:02.0 failed to thaw: error -95 > > > > > > > > To be more accurate, the "resume" phase means the "thaw" callbacks which > > > > run before the system enters hibernation: when the user runs the command > > > > "echo disk > /sys/power/state" for hibernation, first the kernel "freezes" > > > > all the devices and creates a hibernation image, then the kernel "thaws" > > > > the devices including the disk/NIC, writes the memory to the disk, and > > > > powers down. This patch fixes the error message for the Mellanox VF driver > > > > in this phase. Wordsmithing nit: what the patch does is not "fix the error message"; what it does is fix the *problem*, i.e., the fact that we can't operate the device because we can't enable MSI-X. The message is only a symptom. IIUC the relevant part of the system hibernation sequence is: pci_pm_freeze_noirq pci_pm_thaw_noirq pci_pm_thaw And the execution flow is: pci_pm_freeze_noirq if (pci_has_legacy_pm_support(pci_dev)) # true for mlx4 pci_legacy_suspend_late(dev, PMSG_FREEZE) pci_pm_set_unknown_state dev->current_state = PCI_UNKNOWN # <--- pci_pm_thaw_noirq if (pci_has_legacy_pm_support(pci_dev)) # true pci_legacy_resume_early(dev) # noop; mlx4 doesn't implement pci_pm_thaw # returns -95 EOPNOTSUPP if (pci_has_legacy_pm_support(pci_dev)) # true pci_legacy_resume drv->resume mlx4_resume # mlx4_driver.resume (legacy) mlx4_load_one mlx4_enable_msi_x pci_enable_msix_range __pci_enable_msix_range __pci_enable_msix if (!pci_msi_supported()) if (dev->current_state != PCI_D0) # <--- return 0 return -EINVAL err = -EOPNOTSUPP "INTx is not supported ..." (These are just my notes; you don't need to put them all into the commit message. I'm just sharing them in case I'm not understanding correctly.) > > > > When the system starts again, a fresh kernel starts to run, and when the > > > > kernel detects that a hibernation image was saved, the kernel "quiesces" > > > > the devices, and then "restores" the devices from the saved image. In this > > > > path: > > > > device_resume_noirq() -> ... -> > > > > pci_pm_restore_noirq() -> > > > > pci_pm_default_resume_early() -> > > > > pci_power_up() moves the device states back to PCI_D0. This path is > > > > not broken and doesn't need my patch. > > > > The cc list suggests that this might be a fix for a user-reported problem. Is there a launchpad or similar link you could include here? Should this be marked for stable? > > > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > > > This looks like a bugfix for 5839ee7389e8 ("PCI / PM: Force devices to > > > D0 in pci_pm_thaw_noirq()") so maybe it should be marked for stable as > > > 5839ee7389e8 was? > > > > > > Rafael, could you confirm? > > No, it is not a bug fix for that commit. The underlying issue would be > there without that commit too. Oh, right, I dunno what I was thinking, sorry. > > > > --- a/drivers/pci/pci-driver.c > > > > +++ b/drivers/pci/pci-driver.c > > > > @@ -1074,15 +1074,16 @@ static int pci_pm_thaw_noirq(struct device > > > *dev) > > > > return error; > > > > } > > > > > > > > - if (pci_has_legacy_pm_support(pci_dev)) > > > > - return pci_legacy_resume_early(dev); > > > > - > > > > /* > > > > * pci_restore_state() requires the device to be in D0 (because of MSI > > > > * restoration among other things), so force it into D0 in case the > > > > * driver's "freeze" callbacks put it into a low-power state directly. > > > > */ > > > > pci_set_power_state(pci_dev, PCI_D0); > > > > + > > > > + if (pci_has_legacy_pm_support(pci_dev)) > > > > + return pci_legacy_resume_early(dev); > > > > + > > > > pci_restore_state(pci_dev); > > > > > > > > if (drv && drv->pm && drv->pm->thaw_noirq) > > > > -- > > > > 2.19.1 > > > > > The patch looks reasonable to me, but the comment above the > pci_set_power_state() call needs to be updated too IMO. Hmm. 1) pci_restore_state() mainly writes config space, which doesn't require the device to be in D0. The only thing I see that would require D0 is the MSI-X MMIO space, so to be more specific, the comment could say "restoring the MSI-X *MMIO* state requires the device to be in D0". But I think you meant some other comment change. Did you mean something along the lines of "a legacy drv->resume_early() callback and pci_restore_state() both require the device to be in D0"? If something else, maybe you could propose some text? 2) I assume pci_pm_thaw_noirq() should leave the device in a functionally equivalent state, whether it uses legacy PM or not. Do we want something like the patch below instead? If we *do* want to skip pci_restore_state() for legacy PM, maybe we should add a comment. 3) Documentation/power/pci.rst says: ... devices have to be brought back to the fully functional state ... pci_pm_thaw_noirq() ... doesn't put the device into the full power state and doesn't attempt to restore its standard configuration registers. That doesn't seem consistent, and it looks like pci_pm_thaw_noirq() actually *does* put the device in full power (D0) state and restore config registers. diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index a8124e47bf6e..30c721fd6bcf 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -1068,7 +1068,7 @@ static int pci_pm_thaw_noirq(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); struct device_driver *drv = dev->driver; - int error = 0; + int error; if (pcibios_pm_ops.thaw_noirq) { error = pcibios_pm_ops.thaw_noirq(dev); @@ -1076,9 +1076,6 @@ static int pci_pm_thaw_noirq(struct device *dev) return error; } - if (pci_has_legacy_pm_support(pci_dev)) - return pci_legacy_resume_early(dev); - /* * pci_restore_state() requires the device to be in D0 (because of MSI * restoration among other things), so force it into D0 in case the @@ -1087,10 +1084,13 @@ static int pci_pm_thaw_noirq(struct device *dev) pci_set_power_state(pci_dev, PCI_D0); pci_restore_state(pci_dev); + if (pci_has_legacy_pm_support(pci_dev)) + return pci_legacy_resume_early(dev); + if (drv && drv->pm && drv->pm->thaw_noirq) - error = drv->pm->thaw_noirq(dev); + return drv->pm->thaw_noirq(dev); - return error; + return 0; } static int pci_pm_thaw(struct device *dev)
> From: Bjorn Helgaas <helgaas@kernel.org> > Sent: Tuesday, October 8, 2019 12:56 PM > ... > Wordsmithing nit: what the patch does is not "fix the error message"; > what it does is fix the *problem*, i.e., the fact that we can't > operate the device because we can't enable MSI-X. The message is only > a symptom. I totally agree. :-) > IIUC the relevant part of the system hibernation sequence is: > > pci_pm_freeze_noirq > pci_pm_thaw_noirq > pci_pm_thaw > > And the execution flow is: > > pci_pm_freeze_noirq > if (pci_has_legacy_pm_support(pci_dev)) # true for mlx4 > pci_legacy_suspend_late(dev, PMSG_FREEZE) > pci_pm_set_unknown_state > dev->current_state = PCI_UNKNOWN # <--- > pci_pm_thaw_noirq > if (pci_has_legacy_pm_support(pci_dev)) # true > pci_legacy_resume_early(dev) # noop; mlx4 doesn't > implement > pci_pm_thaw # returns -95 > EOPNOTSUPP > if (pci_has_legacy_pm_support(pci_dev)) # true > pci_legacy_resume > drv->resume > mlx4_resume # mlx4_driver.resume (legacy) > mlx4_load_one > mlx4_enable_msi_x > pci_enable_msix_range > __pci_enable_msix_range > __pci_enable_msix > if (!pci_msi_supported()) > if (dev->current_state != PCI_D0) # <--- > return 0 > return -EINVAL > err = -EOPNOTSUPP > "INTx is not supported ..." > > (These are just my notes; you don't need to put them all into the > commit message. I'm just sharing them in case I'm not understanding > correctly.) Yes, these notes are accurate. > > > > > When the system starts again, a fresh kernel starts to run, and when the > > > > > kernel detects that a hibernation image was saved, the kernel > "quiesces" > > > > > the devices, and then "restores" the devices from the saved image. In > this > > > > > path: > > > > > device_resume_noirq() -> ... -> > > > > > pci_pm_restore_noirq() -> > > > > > pci_pm_default_resume_early() -> > > > > > pci_power_up() moves the device states back to PCI_D0. This > path is > > > > > not broken and doesn't need my patch. > > > > > > > The cc list suggests that this might be a fix for a user-reported > problem. Is there a launchpad or similar link you could include here? I guess I'm the first one to notice the issue and there is not any bug link AFAIK. The hibernation process usually saves the states into a local disk (before the system is powered off), and the Mellanox NIC is not needed during the process, so it's not a real issue that the NIC can not work between pci_pm_thaw() and power_down(). This may explain why nobody else noticed the issue. I happened to see the error message, and hence investigated the issue. > Should this be marked for stable? I think we should do it. > > > > > --- a/drivers/pci/pci-driver.c > > > > > +++ b/drivers/pci/pci-driver.c > > > > > @@ -1074,15 +1074,16 @@ static int pci_pm_thaw_noirq(struct device > > > > *dev) > > > > > return error; > > > > > } > > > > > > > > > > - if (pci_has_legacy_pm_support(pci_dev)) > > > > > - return pci_legacy_resume_early(dev); > > > > > - > > > > > /* > > > > > * pci_restore_state() requires the device to be in D0 (because > of MSI > > > > > * restoration among other things), so force it into D0 in case > the > > > > > * driver's "freeze" callbacks put it into a low-power state > directly. > > > > > */ > > > > > pci_set_power_state(pci_dev, PCI_D0); > > > > > + > > > > > + if (pci_has_legacy_pm_support(pci_dev)) > > > > > + return pci_legacy_resume_early(dev); > > > > > + > > > > > pci_restore_state(pci_dev); > > > > > > > > > > if (drv && drv->pm && drv->pm->thaw_noirq) > > > > > -- > > > > > 2.19.1 > > > > > > > The patch looks reasonable to me, but the comment above the > > pci_set_power_state() call needs to be updated too IMO. > > Hmm. > > 1) pci_restore_state() mainly writes config space, which doesn't > require the device to be in D0. The only thing I see that would > require D0 is the MSI-X MMIO space, so to be more specific, the > comment could say "restoring the MSI-X *MMIO* state requires the > device to be in D0". > > But I think you meant some other comment change. Did you mean > something along the lines of "a legacy drv->resume_early() callback > and pci_restore_state() both require the device to be in D0"? > > If something else, maybe you could propose some text? > > 2) I assume pci_pm_thaw_noirq() should leave the device in a > functionally equivalent state, whether it uses legacy PM or not. Do > we want something like the patch below instead? If we *do* want to > skip pci_restore_state() for legacy PM, maybe we should add a comment. > > 3) Documentation/power/pci.rst says: > > ... devices have to be brought back to the fully functional > state ... > > pci_pm_thaw_noirq() ... doesn't put the device into the full power > state and doesn't attempt to restore its standard configuration > registers. > > That doesn't seem consistent, and it looks like pci_pm_thaw_noirq() > actually *does* put the device in full power (D0) state and restore > config registers. I would leave these questions to Rafael. > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index a8124e47bf6e..30c721fd6bcf 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -1068,7 +1068,7 @@ static int pci_pm_thaw_noirq(struct device *dev) > { > struct pci_dev *pci_dev = to_pci_dev(dev); > struct device_driver *drv = dev->driver; > - int error = 0; > + int error; > > if (pcibios_pm_ops.thaw_noirq) { > error = pcibios_pm_ops.thaw_noirq(dev); > @@ -1076,9 +1076,6 @@ static int pci_pm_thaw_noirq(struct device *dev) > return error; > } > > - if (pci_has_legacy_pm_support(pci_dev)) > - return pci_legacy_resume_early(dev); > - > /* > * pci_restore_state() requires the device to be in D0 (because of MSI > * restoration among other things), so force it into D0 in case the > @@ -1087,10 +1084,13 @@ static int pci_pm_thaw_noirq(struct device *dev) > pci_set_power_state(pci_dev, PCI_D0); > pci_restore_state(pci_dev); > > + if (pci_has_legacy_pm_support(pci_dev)) > + return pci_legacy_resume_early(dev); > + > if (drv && drv->pm && drv->pm->thaw_noirq) > - error = drv->pm->thaw_noirq(dev); > + return drv->pm->thaw_noirq(dev); > > - return error; > + return 0; > } > > static int pci_pm_thaw(struct device *dev) The only real difference from my patch is that you moved + if (pci_has_legacy_pm_support(pci_dev)) + return pci_legacy_resume_early(dev); to after the line "pci_restore_state(pci_dev);" This change is good to me, and shoud also resolve the error message I saw. Thanks, -- Dexuan
On 10/8/2019 9:56 PM, Bjorn Helgaas wrote: > On Tue, Oct 08, 2019 at 07:32:27PM +0200, Rafael J. Wysocki wrote: >> On 10/7/2019 8:57 PM, Dexuan Cui wrote: >>>> -----Original Message----- >>>> From: Bjorn Helgaas <helgaas@kernel.org> >>>> Sent: Monday, October 7, 2019 6:24 AM >>>> To: Dexuan Cui <decui@microsoft.com> >>>> Cc: lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; Michael Kelley >>>> <mikelley@microsoft.com>; linux-hyperv@vger.kernel.org; >>>> linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; Sasha >>>> Levin <Alexander.Levin@microsoft.com>; Haiyang Zhang >>>> <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>; >>>> olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com; vkuznets >>>> <vkuznets@redhat.com>; marcelo.cerri@canonical.com; Stephen Hemminger >>>> <sthemmin@microsoft.com>; jackm@mellanox.com >>>> Subject: Re: [PATCH v2] PCI: PM: Move to D0 before calling >>>> pci_legacy_resume_early() >>>> >>>> On Wed, Aug 14, 2019 at 01:06:55AM +0000, Dexuan Cui wrote: >>>>> In pci_legacy_suspend_late(), the device state is moved to PCI_UNKNOWN. >>>>> >>>>> In pci_pm_thaw_noirq(), the state is supposed to be moved back to PCI_D0, >>>>> but the current code misses the pci_legacy_resume_early() path, so the >>>>> state remains in PCI_UNKNOWN in that path. As a result, in the resume >>>>> phase of hibernation, this causes an error for the Mellanox VF driver, >>>>> which fails to enable MSI-X because pci_msi_supported() is false due >>>>> to dev->current_state != PCI_D0: >>>>> >>>>> mlx4_core a6d1:00:02.0: Detected virtual function - running in slave mode >>>>> mlx4_core a6d1:00:02.0: Sending reset >>>>> mlx4_core a6d1:00:02.0: Sending vhcr0 >>>>> mlx4_core a6d1:00:02.0: HCA minimum page size:512 >>>>> mlx4_core a6d1:00:02.0: Timestamping is not supported in slave mode >>>>> mlx4_core a6d1:00:02.0: INTx is not supported in multi-function mode, >>>> aborting >>>>> PM: dpm_run_callback(): pci_pm_thaw+0x0/0xd7 returns -95 >>>>> PM: Device a6d1:00:02.0 failed to thaw: error -95 >>>>> >>>>> To be more accurate, the "resume" phase means the "thaw" callbacks which >>>>> run before the system enters hibernation: when the user runs the command >>>>> "echo disk > /sys/power/state" for hibernation, first the kernel "freezes" >>>>> all the devices and creates a hibernation image, then the kernel "thaws" >>>>> the devices including the disk/NIC, writes the memory to the disk, and >>>>> powers down. This patch fixes the error message for the Mellanox VF driver >>>>> in this phase. > Wordsmithing nit: what the patch does is not "fix the error message"; > what it does is fix the *problem*, i.e., the fact that we can't > operate the device because we can't enable MSI-X. The message is only > a symptom. > > IIUC the relevant part of the system hibernation sequence is: > > pci_pm_freeze_noirq > pci_pm_thaw_noirq > pci_pm_thaw > > And the execution flow is: > > pci_pm_freeze_noirq > if (pci_has_legacy_pm_support(pci_dev)) # true for mlx4 > pci_legacy_suspend_late(dev, PMSG_FREEZE) > pci_pm_set_unknown_state > dev->current_state = PCI_UNKNOWN # <--- > pci_pm_thaw_noirq > if (pci_has_legacy_pm_support(pci_dev)) # true > pci_legacy_resume_early(dev) # noop; mlx4 doesn't implement > pci_pm_thaw # returns -95 EOPNOTSUPP > if (pci_has_legacy_pm_support(pci_dev)) # true > pci_legacy_resume > drv->resume > mlx4_resume # mlx4_driver.resume (legacy) > mlx4_load_one > mlx4_enable_msi_x > pci_enable_msix_range > __pci_enable_msix_range > __pci_enable_msix > if (!pci_msi_supported()) > if (dev->current_state != PCI_D0) # <--- > return 0 > return -EINVAL > err = -EOPNOTSUPP > "INTx is not supported ..." > > (These are just my notes; you don't need to put them all into the > commit message. I'm just sharing them in case I'm not understanding > correctly.) > >>>>> When the system starts again, a fresh kernel starts to run, and when the >>>>> kernel detects that a hibernation image was saved, the kernel "quiesces" >>>>> the devices, and then "restores" the devices from the saved image. In this >>>>> path: >>>>> device_resume_noirq() -> ... -> >>>>> pci_pm_restore_noirq() -> >>>>> pci_pm_default_resume_early() -> >>>>> pci_power_up() moves the device states back to PCI_D0. This path is >>>>> not broken and doesn't need my patch. >>>>> > The cc list suggests that this might be a fix for a user-reported > problem. Is there a launchpad or similar link you could include here? > > Should this be marked for stable? > >>>>> Signed-off-by: Dexuan Cui <decui@microsoft.com> >>>> This looks like a bugfix for 5839ee7389e8 ("PCI / PM: Force devices to >>>> D0 in pci_pm_thaw_noirq()") so maybe it should be marked for stable as >>>> 5839ee7389e8 was? >>>> >>>> Rafael, could you confirm? >> No, it is not a bug fix for that commit. The underlying issue would be >> there without that commit too. > Oh, right, I dunno what I was thinking, sorry. > >>>>> --- a/drivers/pci/pci-driver.c >>>>> +++ b/drivers/pci/pci-driver.c >>>>> @@ -1074,15 +1074,16 @@ static int pci_pm_thaw_noirq(struct device >>>> *dev) >>>>> return error; >>>>> } >>>>> >>>>> - if (pci_has_legacy_pm_support(pci_dev)) >>>>> - return pci_legacy_resume_early(dev); >>>>> - >>>>> /* >>>>> * pci_restore_state() requires the device to be in D0 (because of MSI >>>>> * restoration among other things), so force it into D0 in case the >>>>> * driver's "freeze" callbacks put it into a low-power state directly. >>>>> */ >>>>> pci_set_power_state(pci_dev, PCI_D0); >>>>> + >>>>> + if (pci_has_legacy_pm_support(pci_dev)) >>>>> + return pci_legacy_resume_early(dev); >>>>> + >>>>> pci_restore_state(pci_dev); >>>>> >>>>> if (drv && drv->pm && drv->pm->thaw_noirq) >>>>> -- >>>>> 2.19.1 >>>>> >> The patch looks reasonable to me, but the comment above the >> pci_set_power_state() call needs to be updated too IMO. > Hmm. > > 1) pci_restore_state() mainly writes config space, which doesn't > require the device to be in D0. The only thing I see that would > require D0 is the MSI-X MMIO space, so to be more specific, the > comment could say "restoring the MSI-X *MMIO* state requires the > device to be in D0". > > But I think you meant some other comment change. Did you mean > something along the lines of "a legacy drv->resume_early() callback > and pci_restore_state() both require the device to be in D0"? Yes, I did.
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 36dbe960306b..27dfc68db9e7 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -1074,15 +1074,16 @@ static int pci_pm_thaw_noirq(struct device *dev) return error; } - if (pci_has_legacy_pm_support(pci_dev)) - return pci_legacy_resume_early(dev); - /* * pci_restore_state() requires the device to be in D0 (because of MSI * restoration among other things), so force it into D0 in case the * driver's "freeze" callbacks put it into a low-power state directly. */ pci_set_power_state(pci_dev, PCI_D0); + + if (pci_has_legacy_pm_support(pci_dev)) + return pci_legacy_resume_early(dev); + pci_restore_state(pci_dev); if (drv && drv->pm && drv->pm->thaw_noirq)
In pci_legacy_suspend_late(), the device state is moved to PCI_UNKNOWN. In pci_pm_thaw_noirq(), the state is supposed to be moved back to PCI_D0, but the current code misses the pci_legacy_resume_early() path, so the state remains in PCI_UNKNOWN in that path. As a result, in the resume phase of hibernation, this causes an error for the Mellanox VF driver, which fails to enable MSI-X because pci_msi_supported() is false due to dev->current_state != PCI_D0: mlx4_core a6d1:00:02.0: Detected virtual function - running in slave mode mlx4_core a6d1:00:02.0: Sending reset mlx4_core a6d1:00:02.0: Sending vhcr0 mlx4_core a6d1:00:02.0: HCA minimum page size:512 mlx4_core a6d1:00:02.0: Timestamping is not supported in slave mode mlx4_core a6d1:00:02.0: INTx is not supported in multi-function mode, aborting PM: dpm_run_callback(): pci_pm_thaw+0x0/0xd7 returns -95 PM: Device a6d1:00:02.0 failed to thaw: error -95 To be more accurate, the "resume" phase means the "thaw" callbacks which run before the system enters hibernation: when the user runs the command "echo disk > /sys/power/state" for hibernation, first the kernel "freezes" all the devices and creates a hibernation image, then the kernel "thaws" the devices including the disk/NIC, writes the memory to the disk, and powers down. This patch fixes the error message for the Mellanox VF driver in this phase. When the system starts again, a fresh kernel starts to run, and when the kernel detects that a hibernation image was saved, the kernel "quiesces" the devices, and then "restores" the devices from the saved image. In this path: device_resume_noirq() -> ... -> pci_pm_restore_noirq() -> pci_pm_default_resume_early() -> pci_power_up() moves the device states back to PCI_D0. This path is not broken and doesn't need my patch. Signed-off-by: Dexuan Cui <decui@microsoft.com> --- changes in v2: Updated the changelog with more details. drivers/pci/pci-driver.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)