PCI: Universal error recoverability of devices
v2 cover PCI: Universal error recoverability of devices
This series intends to replace commit 1dc302f7fccc ("PCI: Ensure error recoverability at all times") on the pci/err topic branch:
https://git.kernel.org/pci/pci/c/1dc302f7fccc
The commit is assigning "dev->state_saved = false" in pci_bus_add_device() and during review there were requests to explain the assignment more clearly in a code comment.
However the assignment is (only) necessitated by missing assignments in pci_legacy_suspend() and pci_pm_freeze(), so I propose to instead add *those* assignments (patch [1/3]) and thus avoid the need for the assignment in pci_bus_add_device(), together with its code comment.
Furthermore the commit is *removing* an assignment in pci_device_add(). I am separating that out to new patch [2/3].
So patch [3/3] is identical to the commit, but without the addition of an assignment in pci_bus_add_device() and without the removal of an assignment in pci_device_add().
I am looking into improving the documentation on pci_save_state() in a separate series.
Lukas Wunner (3):
PCI/PM: Reinstate clearing state_saved in legacy and !pm codepaths PCI/PM: Stop needlessly clearing state_saved on enumeration and thaw PCI/ERR: Ensure error recoverability at all times
drivers/pci/bus.c | 3 +++ drivers/pci/pci-driver.c | 6 ++++-- drivers/pci/pci.c | 3 --- drivers/pci/probe.c | 2 -- 4 files changed, 7 insertions(+), 7 deletions(-)
Signature · 1 line
2.51.0
PATCH v2 1/3 PCI/PM: Reinstate clearing state_saved in legacy and !pm codepaths
When a PCI device is suspended, it is normally the PCI core's job to save Config Space and put the device into a low power state. However drivers are allowed to assume these responsibilities. When they do, the PCI core can tell by looking at the state_saved flag in struct pci_dev: The flag is cleared before commencing the suspend sequence and it is set when pci_save_state() is called. If the PCI core finds the flag set late in the suspend sequence, it refrains from calling pci_save_state() itself.
But there are two corner cases where the PCI core neglects to clear the flag before commencing the suspend sequence:
* If a driver has legacy PCI PM callbacks, pci_legacy_suspend() neglects to clear the flag. The (stale) flag is subsequently queried by pci_legacy_suspend() itself and pci_legacy_suspend_late().
* If a device has no driver or its driver has no PCI PM callbacks, pci_pm_freeze() neglects to clear the flag. The (stale) flag is subsequently queried by pci_pm_freeze_noirq().
The flag may be set prior to suspend if the device went through error
recovery: Drivers commonly invoke pci_restore_state() + pci_save_state() to restore Config Space after reset.
The flag may also be set if drivers call pci_save_state() on probe to allow for recovery from subsequent errors.
The result is that pci_legacy_suspend_late() and pci_pm_freeze_noirq() don't call pci_save_state() and so the state that will be restored on resume is the one recorded on last error recovery or on probe, not the one that the device had on suspend. If the two states happen to be identical, there's no problem.
Reinstate clearing the flag in pci_legacy_suspend() and pci_pm_freeze(). The two functions used to do that until commit 4b77b0a2ba27 ("PCI: Clear saved_state after the state has been restored") deemed it unnecessary because it assumed that it's sufficient to clear the flag on resume in pci_restore_state(). The commit seemingly did not take into account that pci_save_state() and pci_restore_state() are not only used by power management code, but also for error recovery.
Devices without driver or whose driver has no PCI PM callbacks may be in runtime suspend when pci_pm_freeze() is called. Their state has already been saved, so don't clear the flag to skip a pointless pci_save_state() in pci_pm_freeze_noirq().
None of the drivers with legacy PCI PM callbacks seem to use runtime PM, so clear the flag unconditionally in their case.
--- drivers/pci/pci-driver.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/pci/pci-driver.c +4 -0
@@ -629,6 +629,8 @@ static int pci_legacy_suspend(struct device *dev, pm_message_t state)struct pci_dev *pci_dev =to_pci_dev (dev);struct pci_driver *drv = pci_dev->driver; + pci_dev->state_saved = false; +if (drv && drv->suspend) {pci_power_t prev = pci_dev->current_state;int error;
@@ -1036,6 +1038,8 @@ static int pci_pm_freeze(struct device *dev)if (!pm) {pci_pm_default_suspend (pci_dev); +if (!pm_runtime_suspended (dev)) + pci_dev->state_saved = false;return 0 ; }
Signature · 1 line
2.51.0
Re: [PATCH v2 1/3] PCI/PM: Reinstate clearing state_saved in legacy and !pm codepaths
Quoted 33 lines · Lukas Wunner
When a PCI device is suspended, it is normally the PCI core's job to save Config Space and put the device into a low power state. However drivers are allowed to assume these responsibilities. When they do, the PCI core can tell by looking at the
state_savedflag in structpci_dev: The flag is cleared before commencing the suspend sequence and it is set whenpci_save_state()is called. If the PCI core finds the flag set late in the suspend sequence, it refrains from callingpci_save_state()itself.But there are two corner cases where the PCI core neglects to clear the flag before commencing the suspend sequence:
* If a driver has legacy PCI PM callbacks,
pci_legacy_suspend()neglects to clear the flag. The (stale) flag is subsequently queried bypci_legacy_suspend()itself andpci_legacy_suspend_late().* If a device has no driver or its driver has no PCI PM callbacks,
pci_pm_freeze()neglects to clear the flag. The (stale) flag is subsequently queried bypci_pm_freeze_noirq().The flag may be set prior to suspend if the device went through error
recovery: Drivers commonly invokepci_restore_state()+pci_save_state()to restore Config Space after reset.The flag may also be set if drivers call
pci_save_state()on probe to allow for recovery from subsequent errors.The result is that
pci_legacy_suspend_late()andpci_pm_freeze_noirq()don't callpci_save_state()and so the state that will be restored on resume is the one recorded on last error recovery or on probe, not the one that the device had on suspend. If the two states happen to be identical, there's no problem.
Quoted 7 lines
Reinstate clearing the flag in
pci_legacy_suspend()andpci_pm_freeze(). The two functions used to do that until commit4b77b0a2ba27("PCI: Clearsaved_stateafter the state has been restored") deemed it unnecessary because it assumed that it's sufficient to clear the flag on resume inpci_restore_state(). The commit seemingly did not take into account thatpci_save_state()andpci_restore_state()are not only used by power management code, but also for error recovery.
That's right, it didn't.
Quoted 11 lines
Devices without driver or whose driver has no PCI PM callbacks may be in runtime suspend when
pci_pm_freeze()is called. Their state has already been saved, so don't clear the flag to skip a pointlesspci_save_state()inpci_pm_freeze_noirq().None of the drivers with legacy PCI PM callbacks seem to use runtime PM, so clear the flag unconditionally in their case.
Fixes 4b77b0a2ba27 ("PCI: Clear saved_state after the state has been restored")Signed-off-by Lukas Wunner <[email protected]>Cc [email protected] # v2.6.32+
Quoted 30 lines
--- drivers/pci/pci-driver.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/pci/pci-driver.c +4
-1@@ -629,6 +629,8 @@ static int pci_legacy_suspend(struct device *dev, pm_message_t state)struct pci_dev *pci_dev =to_pci_dev (dev);struct pci_driver *drv = pci_dev->driver; + pci_dev->state_saved = false; +if (drv && drv->suspend) {pci_power_t prev = pci_dev->current_state;int error;@@ -1036,6 +1038,8 @@ static int pci_pm_freeze(struct device *dev)if (!pm) {pci_pm_default_suspend (pci_dev); +if (!pm_runtime_suspended (dev)) + pci_dev->state_saved = false;return 0 ; } --2.51.0
Re: [PATCH v2 1/3] PCI/PM: Reinstate clearing state_saved in legacy and !pm codepaths
In reply to Lukas WunnerQuoted 7 lines · Lukas Wunner
When a PCI device is suspended, it is normally the PCI core's job to save Config Space and put the device into a low power state. However drivers are allowed to assume these responsibilities. When they do, the PCI core can tell by looking at the
state_savedflag in structpci_dev: The flag is cleared before commencing the suspend sequence and it is set whenpci_save_state()is called. If the PCI core finds the flag set late in the suspend sequence, it refrains from callingpci_save_state()itself.
This has been applied already, no issue there. I have a few questions to help come up with a short higher-level merge commit log.
Quoted 7 lines
But there are two corner cases where the PCI core neglects to clear the flag before commencing the suspend sequence:
* If a driver has legacy PCI PM callbacks,
pci_legacy_suspend()neglects to clear the flag. The (stale) flag is subsequently queried bypci_legacy_suspend()itself andpci_legacy_suspend_late().
Quoted 7 lines
* If a device has no driver or its driver has no PCI PM callbacks,
pci_pm_freeze()neglects to clear the flag. The (stale) flag is subsequently queried bypci_pm_freeze_noirq().The flag may be set prior to suspend if the device went through error
recovery: Drivers commonly invokepci_restore_state()+pci_save_state()to restore Config Space after reset.
I guess the only point of pci_save_state() in this case is to set state_saved again so a future pci_restore_state() will work, right?
The actual state being *saved* is pointless, assuming pci_save_state() saves exactly the same data that pci_restore_state() restored.
And these are the pci_save_state() calls you removed with "treewide:
Drop pci_save_state() after pci_restore_state()". Too bad we have to document the behavior we're about to change, but that's what we need to do. It's just a little clutter to keep in mind for this release.
Quoted 8 lines
The flag may also be set if drivers call
pci_save_state()on probe to allow for recovery from subsequent errors.The result is that
pci_legacy_suspend_late()andpci_pm_freeze_noirq()don't callpci_save_state()and so the state that will be restored on resume is the one recorded on last error recovery or on probe, not the one that the device had on suspend. If the two states happen to be identical, there's no problem.
So IIUC the effect is that after this change and the "treewide" change,
- If the driver uses legacy PM, the state restored on resume will be the state from suspend instead of the state on probe.
- For devices with no driver or a driver without PM, if the device has already been runtime-suspended, we avoid a pointless pci_save_state(), so it's an optimization and not logically related to the legacy PM case.
Right?
I'm thinking of something like this for the merge commit and eventual pull request; please correct me if this isn't right:
Restore the suspend config state, not the state from probe or error recovery, for drivers using legacy PCI suspend.
Avoid saving config state again for devices without driver PM if their state was already saved by runtime suspend.
Quoted 48 lines
Reinstate clearing the flag in
pci_legacy_suspend()andpci_pm_freeze(). The two functions used to do that until commit4b77b0a2ba27("PCI: Clearsaved_stateafter the state has been restored") deemed it unnecessary because it assumed that it's sufficient to clear the flag on resume inpci_restore_state(). The commit seemingly did not take into account thatpci_save_state()andpci_restore_state()are not only used by power management code, but also for error recovery.Devices without driver or whose driver has no PCI PM callbacks may be in runtime suspend when
pci_pm_freeze()is called. Their state has already been saved, so don't clear the flag to skip a pointlesspci_save_state()inpci_pm_freeze_noirq().None of the drivers with legacy PCI PM callbacks seem to use runtime PM, so clear the flag unconditionally in their case.
Fixes 4b77b0a2ba27 ("PCI: Clear saved_state after the state has been restored")Signed-off-by Lukas Wunner <[email protected]>Cc [email protected] # v2.6.32+--- drivers/pci/pci-driver.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/pci/pci-driver.c +4
-0@@ -629,6 +629,8 @@ static int pci_legacy_suspend(struct device *dev, pm_message_t state)struct pci_dev *pci_dev =to_pci_dev (dev);struct pci_driver *drv = pci_dev->driver; + pci_dev->state_saved = false; +if (drv && drv->suspend) {pci_power_t prev = pci_dev->current_state;int error;@@ -1036,6 +1038,8 @@ static int pci_pm_freeze(struct device *dev)if (!pm) {pci_pm_default_suspend (pci_dev); +if (!pm_runtime_suspended (dev)) + pci_dev->state_saved = false;return 0 ; }Signature · 1 line
2.51.0
Re: [PATCH v2 1/3] PCI/PM: Reinstate clearing state_saved in legacy and !pm codepaths
Quoted 19 lines · Bjorn Helgaas
Quoted 7 lines · Lukas Wunner
But there are two corner cases where the PCI core neglects to clear the flag before commencing the suspend sequence:
* If a driver has legacy PCI PM callbacks,
pci_legacy_suspend()neglects to clear the flag. The (stale) flag is subsequently queried bypci_legacy_suspend()itself andpci_legacy_suspend_late().Quoted 7 lines
* If a device has no driver or its driver has no PCI PM callbacks,
pci_pm_freeze()neglects to clear the flag. The (stale) flag is subsequently queried bypci_pm_freeze_noirq().The flag may be set prior to suspend if the device went through error
recovery: Drivers commonly invokepci_restore_state()+pci_save_state()to restore Config Space after reset.I guess the only point of
pci_save_state()in this case is to setstate_savedagain so a futurepci_restore_state()will work, right?
Quoted 7 lines
The actual state being *saved* is pointless, assuming
pci_save_state()saves exactly the same data thatpci_restore_state()restored.And these are the
pci_save_state()calls you removed with "treewide:
Droppci_save_state()afterpci_restore_state()". Too bad we have to document the behavior we're about to change, but that's what we need to do. It's just a little clutter to keep in mind for this release.
Yes. All of your comments above are correct.
Quoted 9 lines
Quoted 8 lines
The flag may also be set if drivers call
pci_save_state()on probe to allow for recovery from subsequent errors.The result is that
pci_legacy_suspend_late()andpci_pm_freeze_noirq()don't callpci_save_state()and so the state that will be restored on resume is the one recorded on last error recovery or on probe, not the one that the device had on suspend. If the two states happen to be identical, there's no problem.
Quoted 5 lines
So IIUC the effect is that after this change and the "treewide" change,
- If the driver uses legacy PM, the state restored on resume will be the state from suspend instead of the state on probe.
Right.
Quoted 5 lines
- For devices with no driver or a driver without PM, if the device has already been runtime-suspended, we avoid a pointless
pci_save_state(), so it's an optimization and not logically related to the legacy PM case.
It's slightly different:
- For devices with no driver or a driver without PM, the state restored on "thaw" and "restore" will be the state from "freeze" instead of the state on probe.
So the same problem that we have for drivers using legacy PM, we also have for devices with no driver or a driver without (modern) PM callbacks, but only in the "freeze" codepath (for hibernation).
In the patch, I made the "pci_dev->state_saved = false" assignment conditional on !pm_runtime_suspended() in the "freeze" codepath. I didn't do the same in the legacy codepath because none of the drivers using legacy PM callbacks seem to be using runtime PM.
The purpose of making it conditional on !pm_runtime_suspended() is just that we'd otherwise call pci_save_state() twice: Once in pci_pm_runtime_suspend() and once more in pci_pm_freeze(). That would be pointless.
In the commit message, I provided a rationale for the conditionality, but inadvertently caused confusion.
Quoted 8 lines
I'm thinking of something like this for the merge commit and eventual pull request; please correct me if this isn't right:
Restore the suspend config state, not the state from probe or error recovery, for drivers using legacy PCI suspend.
Avoid saving config state again for devices without driver PM if their state was already saved by runtime suspend.
I'd suggest instead (feel free to wordsmith as you see fit):
Restore the suspend config state, not the state from probe or error
recovery, for drivers using legacy PCI suspend. [ <- unmodified ]
Same for devices with no driver or a driver without PM callbacks
when the system is hibernated. [ <- replacement ]
Mentioning the runtime PM conditionality in the high-level changelog is probably not worth it.
Was I able to clarify all questions? Please ask again if not.
Also, in case the meaning of "freeze", "thaw", "restore" isn't clear, here's the order of a hibernation sequence (suspend to disk):
pci_pm_prepare()
pci_pm_freeze()
pci_pm_freeze_noirq()
<system image is generated> pci_pm_thaw_noirq()
pci_pm_thaw()
pci_pm_complete()
pci_pm_prepare()
pci_pm_poweroff()
pci_pm_poweroff_late() pci_pm_poweroff_noirq()
<system is asleep, then restarted with boot kernel> pci_pm_prepare()
pci_pm_freeze()
pci_pm_freeze_noirq()
<system image is restored, replacing the boot kernel> pci_pm_restore_noirq()
pci_pm_restore()
pci_pm_complete()
Note that "freeze" happens twice in the whole sequence.
Thanks,
Lukas
Re: [PATCH v2 1/3] PCI/PM: Reinstate clearing state_saved in legacy and !pm codepaths
Quoted 58 lines · Lukas Wunner
Quoted 19 lines · Bjorn Helgaas
Quoted 7 lines · Lukas Wunner
But there are two corner cases where the PCI core neglects to clear the flag before commencing the suspend sequence:
* If a driver has legacy PCI PM callbacks,
pci_legacy_suspend()neglects to clear the flag. The (stale) flag is subsequently queried bypci_legacy_suspend()itself andpci_legacy_suspend_late().Quoted 7 lines
* If a device has no driver or its driver has no PCI PM callbacks,
pci_pm_freeze()neglects to clear the flag. The (stale) flag is subsequently queried bypci_pm_freeze_noirq().The flag may be set prior to suspend if the device went through error
recovery: Drivers commonly invokepci_restore_state()+pci_save_state()to restore Config Space after reset.I guess the only point of
pci_save_state()in this case is to setstate_savedagain so a futurepci_restore_state()will work, right?Quoted 7 lines
The actual state being *saved* is pointless, assuming
pci_save_state()saves exactly the same data thatpci_restore_state()restored.And these are the
pci_save_state()calls you removed with "treewide:
Droppci_save_state()afterpci_restore_state()". Too bad we have to document the behavior we're about to change, but that's what we need to do. It's just a little clutter to keep in mind for this release.Yes. All of your comments above are correct.
Quoted 9 lines
Quoted 8 lines
The flag may also be set if drivers call
pci_save_state()on probe to allow for recovery from subsequent errors.The result is that
pci_legacy_suspend_late()andpci_pm_freeze_noirq()don't callpci_save_state()and so the state that will be restored on resume is the one recorded on last error recovery or on probe, not the one that the device had on suspend. If the two states happen to be identical, there's no problem.Quoted 5 lines
So IIUC the effect is that after this change and the "treewide" change,
- If the driver uses legacy PM, the state restored on resume will be the state from suspend instead of the state on probe.
Right.
Quoted 4 lines
- For devices with no driver or a driver without PM, if the device has already been runtime-suspended, we avoid a pointless
pci_save_state(), so it's an optimization and not logically related to the legacy PM case.It's slightly different:
- For devices with no driver or a driver without PM, the state restored on "thaw" and "restore" will be the state from "freeze" instead of the state on probe.
Quoted 8 lines
So the same problem that we have for drivers using legacy PM, we also have for devices with no driver or a driver without (modern) PM callbacks, but only in the "freeze" codepath (for hibernation).
In the patch, I made the "
pci_dev->state_saved= false" assignment conditional on !pm_runtime_suspended() in the "freeze" codepath. I didn't do the same in the legacy codepath because none of the drivers using legacy PM callbacks seem to be using runtime PM.
Maybe it's moot because we hope there will be no new users of PCI legacy PM with runtime PM, but I don't think there's anything to *prevent* that or to protect against out-of-tree drivers.
The implicit assumption that there are no such drivers makes it look like there's something magic involving state_saved, legacy PM, and runtime PM. It might be worth doing the same in the legacy PM path just for readability.
Quoted 17 lines
The purpose of making it conditional on !pm_runtime_suspended() is just that we'd otherwise call
pci_save_state()twice: Once inpci_pm_runtime_suspend()and once more inpci_pm_freeze(). That would be pointless.In the commit message, I provided a rationale for the conditionality, but inadvertently caused confusion.
Quoted 8 lines
I'm thinking of something like this for the merge commit and eventual pull request; please correct me if this isn't right:
Restore the suspend config state, not the state from probe or error recovery, for drivers using legacy PCI suspend.
Avoid saving config state again for devices without driver PM if their state was already saved by runtime suspend.
Quoted 7 lines
I'd suggest instead (feel free to wordsmith as you see fit):
Restore the suspend config state, not the state from probe or error
recovery, for drivers using legacy PCI suspend. [ <- unmodified ]Same for devices with no driver or a driver without PM callbacks
when the system is hibernated. [ <- replacement ]
Stepping back, I guess that when drivers use generic PM, we already save config state during suspend and restore that state during resume, and do the same when entering/leaving hibernation.
And the point of this patch is to do the same when drivers lack PM or use legacy PCI PM, or when devices have no driver?
Maybe third try is the charm?
For drivers using PCI legacy suspend, save config state at suspend so that state, not any earlier state from enumeration, probe, or error recovery, will be restored when resuming.
For devices with no driver or a driver that lacks PM, save config state at hibernate so that state, not any earlier state from enumeration, probe, or error recovery, will be restored when resuming.
IIUC, after "Ensure error recoverability", the PCI core will always save the state during enumeration, so drivers shouldn't use pci_save_state() at all unless they make config changes that they want restored during error recovery?
Or, I guess (sigh) if they do their own power management?
Quoted 28 lines
Mentioning the runtime PM conditionality in the high-level changelog is probably not worth it.
Was I able to clarify all questions? Please ask again if not.
Also, in case the meaning of "freeze", "thaw", "restore" isn't clear, here's the order of a hibernation sequence (suspend to disk):
pci_pm_prepare()
pci_pm_freeze()
pci_pm_freeze_noirq()
<system image is generated>pci_pm_thaw_noirq()
pci_pm_thaw()
pci_pm_complete()
pci_pm_prepare()
pci_pm_poweroff()
pci_pm_poweroff_late()pci_pm_poweroff_noirq()
<system is asleep, then restarted with boot kernel>pci_pm_prepare()
pci_pm_freeze()
pci_pm_freeze_noirq()
<system image is restored, replacing the boot kernel>pci_pm_restore_noirq()
pci_pm_restore()
pci_pm_complete()
Quoted 1 line
Note that "freeze" happens twice in the whole sequence.
Thanks, this is extremely helpful. Copied into my notes. I guess this is essentially condensed from Documentation/driver-api/pm/devices.rst, but it's very helpful to see the bare bones and your annotations.
Bjorn
Re: [PATCH v2 1/3] PCI/PM: Reinstate clearing state_saved in legacy and !pm codepaths
Quoted 6 lines · Bjorn Helgaas
Quoted 4 lines · Lukas Wunner
In the patch, I made the "
pci_dev->state_saved= false" assignment conditional on !pm_runtime_suspended() in the "freeze" codepath. I didn't do the same in the legacy codepath because none of the drivers using legacy PM callbacks seem to be using runtime PM.
Quoted 8 lines
Maybe it's moot because we hope there will be no new users of PCI legacy PM with runtime PM, but I don't think there's anything to *prevent* that or to protect against out-of-tree drivers.
The implicit assumption that there are no such drivers makes it look like there's something magic involving
state_saved, legacy PM, and runtime PM. It might be worth doing the same in the legacy PM path just for readability.
Drivers having both legacy callbacks and modern callbacks (including runtime PM callbacks) cause emission of a WARN splat in pci_has_legacy_pm_support().
Drivers need to activate runtime PM by dropping a runtime PM reference on probe (see the code comment in local_pci_probe()). In theory a driver could have legacy callbacks but no modern callbacks and still use runtime PM by calling pm_runtime_put_noidle() on probe. So I compiled a list of drivers implementing legacy callbacks (included at the end of this e-mail for reference), grep'ed through them for any "pm_runtime" occurrences and found none.
Hence it seems very unlikely that drivers using legacy callbacks and runtime PM exist. We probably shouldn't accommodate for such use cases but should rather try to incentivize conversion to modern callbacks.
When compiling the list I sadly noticed that new drivers do exist which use legacy callbacks. A case in point is:
drivers/net/ethernet/google/gve/gve_main.c
... which started using legacy callbacks in 2021 with commit 974365e51861 ("gve: Implement suspend/resume/shutdown").
I guess there is no real incentive to convert to modern PM callbacks and finding someone who has the hardware and can test patches is hard (most drivers are for ATA, some for really old 1990s hardware). Plus, a lot of detailed knowledge about PCI PM is necessary to avoid breakage, making this a task that can't easily be delegated to new contributors. And everyone with the knowledge is overworked already. So we keep dragging this tech debt along which complicates codepaths. :(
Quoted 6 lines
Stepping back, I guess that when drivers use generic PM, we already save config state during suspend and restore that state during resume, and do the same when entering/leaving hibernation.
And the point of this patch is to do the same when drivers lack PM or use legacy PCI PM, or when devices have no driver?
Right. To have a consistent logic that state_saved is always cleared before commencing the suspend sequence, in all codepaths.
Quoted 10 lines
Maybe third try is the charm?
For drivers using PCI legacy suspend, save config state at suspend so that state, not any earlier state from enumeration, probe, or error recovery, will be restored when resuming.
For devices with no driver or a driver that lacks PM, save config state at hibernate so that state, not any earlier state from enumeration, probe, or error recovery, will be restored when resuming.
Perfect.
Quoted 6 lines
IIUC, after "Ensure error recoverability", the PCI core will always save the state during enumeration, so drivers shouldn't use
pci_save_state()at all unless they make config changes that they want restored during error recovery?Or, I guess (sigh) if they do their own power management?
Exactly right.
Quoted 2 lines
Quoted 2 lines
Also, in case the meaning of "freeze", "thaw", "restore" isn't clear, here's the order of a hibernation sequence (suspend to disk):
[...]
Quoted 1 line
Thanks, this is extremely helpful.
FWIW, this is the sequence of suspend-to-memory, obviously much simpler:
pci_pm_prepare()
pci_pm_suspend()
pci_pm_suspend_late()
pci_pm_suspend_noirq()
pci_pm_resume_noirq()
pci_pm_resume_early()
pci_pm_resume()
pci_pm_complete()
And runtime PM:
pci_pm_runtime_suspend()
pci_pm_runtime_resume()
Thanks,
Lukas
-- >8 --
Drivers with legacy PCI PM callbacks:
drivers/ata/acard-ahci.c
drivers/ata/ata_generic.c
drivers/ata/ata_piix.c
drivers/ata/pata_acpi.c
drivers/ata/pata_ali.c
drivers/ata/pata_amd.c
drivers/ata/pata_artop.c
drivers/ata/pata_atiixp.c
drivers/ata/pata_atp867x.c
drivers/ata/pata_cmd640.c
drivers/ata/pata_cmd64x.c
drivers/ata/pata_cs5520.c
drivers/ata/pata_cs5530.c
drivers/ata/pata_cs5535.c
drivers/ata/pata_cs5536.c
drivers/ata/pata_cypress.c
drivers/ata/pata_efar.c
drivers/ata/pata_hpt366.c
drivers/ata/pata_hpt3x3.c
drivers/ata/pata_it8213.c
drivers/ata/pata_it821x.c
drivers/ata/pata_jmicron.c
drivers/ata/pata_macio.c
drivers/ata/pata_marvell.c
drivers/ata/pata_mpiix.c
drivers/ata/pata_netcell.c
drivers/ata/pata_ninja32.c
drivers/ata/pata_ns87410.c
drivers/ata/pata_ns87415.c
drivers/ata/pata_oldpiix.c
drivers/ata/pata_opti.c
drivers/ata/pata_optidma.c
drivers/ata/pata_pdc2027x.c
drivers/ata/pata_pdc202xx_old.c
drivers/ata/pata_piccolo.c
drivers/ata/pata_radisys.c
drivers/ata/pata_rdc.c
drivers/ata/pata_rz1000.c
drivers/ata/pata_sc1200.c
drivers/ata/pata_sch.c
drivers/ata/pata_serverworks.c
drivers/ata/pata_sil680.c
drivers/ata/pata_sis.c
drivers/ata/pata_sl82c105.c
drivers/ata/pata_triflex.c
drivers/ata/pata_via.c
drivers/ata/sata_inic162x.c
drivers/ata/sata_mv.c
drivers/ata/sata_nv.c
drivers/ata/sata_sil.c
drivers/ata/sata_sil24.c
drivers/ata/sata_sis.c
drivers/ata/sata_via.c
drivers/bluetooth/hci_bcm4377.c
drivers/gpio/gpio-bt8xx.c
drivers/message/fusion/mptfc.c
drivers/message/fusion/mptsas.c
drivers/message/fusion/mptspi.c
drivers/mtd/nand/raw/cafe_nand.c
drivers/net/ethernet/atheros/atl1e/atl1e_main.c
drivers/net/ethernet/atheros/atlx/atl2.c
drivers/net/ethernet/google/gve/gve_main.c
drivers/net/ethernet/microsoft/mana/gdma_main.c
drivers/net/ethernet/toshiba/tc35815.c
drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
drivers/net/wireless/mediatek/mt76/mt7615/pci.c
drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
drivers/net/wireless/mediatek/mt76/mt76x2/pci.c
drivers/scsi/nsp32.c
drivers/scsi/qedf/qedf_main.c
drivers/scsi/qedi/qedi_main.c
drivers/scsi/stex.c
drivers/tty/serial/serial_txx9.c
drivers/video/fbdev/chipsfb.c
drivers/video/fbdev/i810/i810_main.c
Re: [PATCH v2 1/3] PCI/PM: Reinstate clearing state_saved in legacy and !pm codepaths
Quoted 29 lines · Lukas Wunner
Quoted 6 lines · Bjorn Helgaas
Quoted 4 lines · Lukas Wunner
In the patch, I made the "
pci_dev->state_saved= false" assignment conditional on !pm_runtime_suspended() in the "freeze" codepath. I didn't do the same in the legacy codepath because none of the drivers using legacy PM callbacks seem to be using runtime PM.Quoted 8 lines
Maybe it's moot because we hope there will be no new users of PCI legacy PM with runtime PM, but I don't think there's anything to *prevent* that or to protect against out-of-tree drivers.
The implicit assumption that there are no such drivers makes it look like there's something magic involving
state_saved, legacy PM, and runtime PM. It might be worth doing the same in the legacy PM path just for readability.Drivers having both legacy callbacks and modern callbacks (including runtime PM callbacks) cause emission of a WARN splat in
pci_has_legacy_pm_support().Drivers need to activate runtime PM by dropping a runtime PM reference on probe (see the code comment in
local_pci_probe()). In theory a driver could have legacy callbacks but no modern callbacks and still use runtime PM by callingpm_runtime_put_noidle()on probe. So I compiled a list of drivers implementing legacy callbacks (included at the end of this e-mail for reference), grep'ed through them for any "pm_runtime" occurrences and found none.
Quoted 3 lines
Hence it seems very unlikely that drivers using legacy callbacks and runtime PM exist. We probably shouldn't accommodate for such use cases but should rather try to incentivize conversion to modern callbacks.
Agreed.
What about adding a WARN_ON(pm_runtime_enabled(dev)) to the "legacy" suspend/hibernation callback paths?
Quoted 8 lines
When compiling the list I sadly noticed that new drivers do exist which use legacy callbacks. A case in point is:
drivers/net/ethernet/google/gve/gve_main.c... which started using legacy callbacks in 2021 with commit
974365e51861("gve: Implement suspend/resume/shutdown").
Quoted 7 lines
I guess there is no real incentive to convert to modern PM callbacks and finding someone who has the hardware and can test patches is hard (most drivers are for ATA, some for really old 1990s hardware). Plus, a lot of detailed knowledge about PCI PM is necessary to avoid breakage, making this a task that can't easily be delegated to new contributors. And everyone with the knowledge is overworked already. So we keep dragging this tech debt along which complicates codepaths. :(
While I agree that this is the case, I'm not sure what can be done to address this problem, realistically.
PATCH v2 2/3 PCI/PM: Stop needlessly clearing state_saved on enumeration and thaw
In reply to Lukas WunnerThe state_saved flag tells the PCI core whether a driver assumes responsibility to save Config Space and put the device into a low power state on suspend.
The flag is currently initialized to false on enumeration, even though it already is false (because struct pci_dev is zeroed by kzalloc()) and even though it is set to false before commencing the suspend sequence (the only code path where it's relevant).
The flag is also set to false in pci_pm_thaw(), i.e. on resume, when it's no longer relevant.
Drop these two superfluous flag assignments for simplicity.
--- drivers/pci/pci-driver.c | 2 -- drivers/pci/probe.c | 2 -- 2 files changed, 4 deletions(-)
drivers/pci/pci-driver.c +0 -2
@@ -1133,8 +1133,6 @@ static int pci_pm_thaw(struct device *dev)pci_pm_reenable_device (pci_dev); } - pci_dev->state_saved = false; -return error; }
drivers/pci/probe.c +0 -2
@@ -2747,8 +2747,6 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)pci_reassigndev_resource_alignment (dev); - dev->state_saved = false; -pci_init_capabilities (dev);/*
Signature · 1 line
2.51.0
Re: [PATCH v2 2/3] PCI/PM: Stop needlessly clearing state_saved on enumeration and thaw
Quoted 10 lines · Lukas Wunner
The
state_savedflag tells the PCI core whether a driver assumes responsibility to save Config Space and put the device into a low power state on suspend.The flag is currently initialized to false on enumeration, even though it already is false (because struct
pci_devis zeroed bykzalloc()) and even though it is set to false before commencing the suspend sequence (the only code path where it's relevant).
Quoted 6 lines
The flag is also set to false in
pci_pm_thaw(), i.e. on resume, when it's no longer relevant.Drop these two superfluous flag assignments for simplicity.
Signed-off-by Lukas Wunner <[email protected]>
Quoted 35 lines
--- drivers/pci/pci-driver.c | 2 -- drivers/pci/probe.c | 2 -- 2 files changed, 4 deletions(-)drivers/pci/pci-driver.c +0
-2@@ -1133,8 +1133,6 @@ static int pci_pm_thaw(struct device *dev)pci_pm_reenable_device (pci_dev); } - pci_dev->state_saved = false; -return error; }drivers/pci/probe.c +0
-3@@ -2747,8 +2747,6 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)pci_reassigndev_resource_alignment (dev); - dev->state_saved = false; -pci_init_capabilities (dev);/* -- 2.51.0
PATCH v2 3/3 PCI/ERR: Ensure error recoverability at all times
In reply to Lukas WunnerWhen the PCI core gained power management support in 2002, it introduced pci_save_state() and pci_restore_state() helpers to restore Config Space after a D3hot or D3cold transition, which implies a Soft or Fundamental Reset (PCIe r7.0 sec 5.8):
https://git.kernel.org/tglx/history/c/a5287abe398b
In 2006, EEH and AER were introduced to recover from errors by performing a reset. Because errors can occur at any time, drivers began calling pci_save_state() on probe to ensure recoverability.
In 2009, recoverability was foiled by commit c82f63e411f1 ("PCI: check saved state before restore"): It amended pci_restore_state() to bail out if the "state_saved" flag has been cleared. The flag is cleared by pci_restore_state() itself, hence a saved state is now allowed to be restored only once and is then invalidated. That doesn't seem to make sense because the saved state should be good enough to be reused.
Soon after, drivers began to work around this behavior by calling pci_save_state() immediately after pci_restore_state(), see e.g. commit b94f2d775a71 ("igb: call pci_save_state after pci_restore_state"). Hilariously, two drivers even set the "saved_state" flag to true before invoking pci_restore_state(), see ipr_reset_restore_cfg_space() and e1000_io_slot_reset().
Despite these workarounds, recoverability at all times is not guaranteed:
E.g. when a PCIe port goes through a runtime suspend and resume cycle, the "saved_state" flag is cleared by:
pci_pm_runtime_resume()
pci_pm_default_resume_early()
pci_restore_state()
... and hence on a subsequent AER event, the port's Config Space cannot be restored. Riana reports a recovery failure of a GPU-integrated PCIe switch and has root-caused it to the behavior of pci_restore_state(). Another workaround would be necessary, namely calling pci_save_state() in pcie_port_device_runtime_resume().
The motivation of commit c82f63e411f1 was to prevent restoring state if pci_save_state() hasn't been called before. But that can be achieved by saving state already on device addition, after Config Space has been initialized. A desirable side effect is that devices become recoverable even if no driver gets bound. This renders the commit unnecessary, so revert it.
--- drivers/pci/bus.c | 3 +++ drivers/pci/pci.c | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/pci/bus.c +3 -0
@@ -357,6 +357,9 @@ void pci_bus_add_device(struct pci_dev *dev)pci_proc_attach_device (dev);pci_bridge_d3_update (dev); +/* Save config space for error recoverability */ +pci_save_state (dev); +/* * If the PCI device is associated with a pwrctrl device with a * power supply, create a device link between the PCI device and
drivers/pci/pci.c +0 -3
@@ -1855,9 +1855,6 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)*/ void pci_restore_state (struct pci_dev *dev) { -if (!dev->state_saved) -return ; -pci_restore_pcie_state (dev);pci_restore_pasid_state (dev);pci_restore_pri_state (dev);
Signature · 1 line
2.51.0
Quoted 29 lines · Lukas Wunner
This series intends to replace commit
1dc302f7fccc("PCI: Ensure error recoverability at all times") on the pci/err topic branch:https://git.kernel.org/pci/pci/c/1dc302f7fccc
The commit is assigning "
dev->state_saved= false" inpci_bus_add_device()and during review there were requests to explain the assignment more clearly in a code comment.However the assignment is (only) necessitated by missing assignments in
pci_legacy_suspend()andpci_pm_freeze(), so I propose to instead add *those* assignments (patch [1/3]) and thus avoid the need for the assignment inpci_bus_add_device(), together with its code comment.Furthermore the commit is *removing* an assignment in
pci_device_add(). I am separating that out to new patch [2/3].So patch [3/3] is identical to the commit, but without the addition of an assignment in
pci_bus_add_device()and without the removal of an assignment inpci_device_add().I am looking into improving the documentation on
pci_save_state()in a separate series.Lukas Wunner (3):
PCI/PM: Reinstate clearingstate_savedin legacy and !pm codepaths PCI/PM: Stop needlessly clearingstate_savedon enumeration and thaw PCI/ERR: Ensure error recoverability at all times
Quoted 5 lines
drivers/pci/bus.c | 3 +++ drivers/pci/pci-driver.c | 6 ++++-- drivers/pci/pci.c | 3 --- drivers/pci/probe.c | 2 -- 4 files changed, 7 insertions(+), 7 deletions(-)
Applied on pci/err for v6.19, thanks!