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 <[email protected]>
---
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
> From: Dexuan Cui <[email protected]>
> Sent: Tuesday, August 13, 2019 6:07 PM
> To: [email protected]; [email protected];
> [email protected]
> Cc: Michael Kelley <[email protected]>; [email protected];
> [email protected]; [email protected]; Sasha
> Levin <[email protected]>; Haiyang Zhang
> <[email protected]>; KY Srinivasan <[email protected]>;
> [email protected]; [email protected]; [email protected]; vkuznets
> <[email protected]>; [email protected]; Stephen Hemminger
> <[email protected]>; [email protected]; Dexuan Cui
> <[email protected]>
> 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 <[email protected]>
> ---
>
> 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 <[email protected]> On Behalf Of
> Dexuan Cui
> Sent: Monday, September 2, 2019 5:35 PM
> To: [email protected]; [email protected];
> [email protected]
> [..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 <[email protected]>
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 <[email protected]>
> Sent: Monday, October 7, 2019 6:24 AM
> To: Dexuan Cui <[email protected]>
> Cc: [email protected]; [email protected]; Michael Kelley
> <[email protected]>; [email protected];
> [email protected]; [email protected]; Sasha
> Levin <[email protected]>; Haiyang Zhang
> <[email protected]>; KY Srinivasan <[email protected]>;
> [email protected]; [email protected]; [email protected]; vkuznets
> <[email protected]>; [email protected]; Stephen Hemminger
> <[email protected]>; [email protected]
> 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 <[email protected]>
>
> 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 <[email protected]>
>> Sent: Monday, October 7, 2019 6:24 AM
>> To: Dexuan Cui <[email protected]>
>> Cc: [email protected]; [email protected]; Michael Kelley
>> <[email protected]>; [email protected];
>> [email protected]; [email protected]; Sasha
>> Levin <[email protected]>; Haiyang Zhang
>> <[email protected]>; KY Srinivasan <[email protected]>;
>> [email protected]; [email protected]; [email protected]; vkuznets
>> <[email protected]>; [email protected]; Stephen Hemminger
>> <[email protected]>; [email protected]
>> 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 <[email protected]>
>> 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 <[email protected]>
> > > Sent: Monday, October 7, 2019 6:24 AM
> > > To: Dexuan Cui <[email protected]>
> > > Cc: [email protected]; [email protected]; Michael Kelley
> > > <[email protected]>; [email protected];
> > > [email protected]; [email protected]; Sasha
> > > Levin <[email protected]>; Haiyang Zhang
> > > <[email protected]>; KY Srinivasan <[email protected]>;
> > > [email protected]; [email protected]; [email protected]; vkuznets
> > > <[email protected]>; [email protected]; Stephen Hemminger
> > > <[email protected]>; [email protected]
> > > 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 <[email protected]>
> > > 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 <[email protected]>
> 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 <[email protected]>
>>>> Sent: Monday, October 7, 2019 6:24 AM
>>>> To: Dexuan Cui <[email protected]>
>>>> Cc: [email protected]; [email protected]; Michael Kelley
>>>> <[email protected]>; [email protected];
>>>> [email protected]; [email protected]; Sasha
>>>> Levin <[email protected]>; Haiyang Zhang
>>>> <[email protected]>; KY Srinivasan <[email protected]>;
>>>> [email protected]; [email protected]; [email protected]; vkuznets
>>>> <[email protected]>; [email protected]; Stephen Hemminger
>>>> <[email protected]>; [email protected]
>>>> 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 <[email protected]>
>>>> 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.