| Message ID | 20260930141914.6678-2-fbeltranmillalen@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI/PM: Do not save the config space of an inaccessible device | expand |
> hcd_pci_suspend_noirq() ignores the return value of pci_save_state() and > goes on to pci_prepare_to_sleep(). > > The next patch makes pci_save_state() fail, without marking the state as > saved, when the config space of the device is not accessible. For such > a controller pci_prepare_to_sleep() does not work either: > pci_set_low_power_state() reads the PM control register as all ones and > marks the device D3cold. pci_pm_suspend_noirq() then finds a device > whose power state changed without its state being saved, and warns. > With an earlier version of the next patch on a MacBookPro14,3, whose > Thunderbolt xHCI controllers can be inaccessible at this point: > > xhci_hcd 0000:7d:00.0: PCI PM: State of device not saved by hcd_pci_suspend_noirq+0x0/0x180 > WARNING: CPU: 7 PID: 96793 at drivers/pci/pci-driver.c:888 pci_pm_suspend_noirq+0x2f4/0x300 > > [ ... ] > in D0 instead of being put into a low-power state with an incomplete > saved state. > > Assisted-by: LLM > Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com> Sashiko has reviewed this patch and found no issues. It looks great!
diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c index cd2234759..7f8a56425 100644 --- a/drivers/usb/core/hcd-pci.c +++ b/drivers/usb/core/hcd-pci.c @@ -541,8 +541,6 @@ static int hcd_pci_suspend_noirq(struct device *dev) if (retval) return retval; - pci_save_state(pci_dev); - /* If the root hub is dead rather than suspended, disallow remote * wakeup. usb_hc_died() should ensure that both hosts are marked as * dying, so we only need to check the primary roothub. @@ -551,6 +549,19 @@ static int hcd_pci_suspend_noirq(struct device *dev) device_set_wakeup_enable(dev, 0); dev_dbg(dev, "wakeup: %d\n", device_may_wakeup(dev)); + /* + * If the state could not be saved, most likely because the controller + * is no longer accessible, putting it into a low-power state would + * fail as well and leave it marked as D3cold, and the PCI core would + * then warn that its state was not saved. Leave it to the PCI core + * instead, as for a driver without a suspend_noirq callback. + */ + retval = pci_save_state(pci_dev); + if (retval) { + dev_dbg(dev, "--> not suspending, state not saved\n"); + return 0; + } + /* Possibly enable remote wakeup, * choose the appropriate low-power state, and go to that state. */
hcd_pci_suspend_noirq() ignores the return value of pci_save_state() and goes on to pci_prepare_to_sleep(). The next patch makes pci_save_state() fail, without marking the state as saved, when the config space of the device is not accessible. For such a controller pci_prepare_to_sleep() does not work either: pci_set_low_power_state() reads the PM control register as all ones and marks the device D3cold. pci_pm_suspend_noirq() then finds a device whose power state changed without its state being saved, and warns. With an earlier version of the next patch on a MacBookPro14,3, whose Thunderbolt xHCI controllers can be inaccessible at this point: xhci_hcd 0000:7d:00.0: PCI PM: State of device not saved by hcd_pci_suspend_noirq+0x0/0x180 WARNING: CPU: 7 PID: 96793 at drivers/pci/pci-driver.c:888 pci_pm_suspend_noirq+0x2f4/0x300 Check the return value and, if the state could not be saved, return 0 without putting the controller into a low-power state. The PCI core then handles it as it handles a driver without a suspend_noirq callback: it tries to save the state and to put the device into a low-power state itself, which fails in the same way, but without the warning, as the power state did not change inside the driver callback. Move the check that disables wakeup for a dead root hub ahead of it, so that it is not skipped. Today pci_save_state() only fails if a capability save buffer is missing or the VC state cannot be saved. In that case the controller is now left in D0 instead of being put into a low-power state with an incomplete saved state. Assisted-by: LLM Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com> --- v2: - Rewrote the commit message: the failure handled here comes from the next patch, not from pci_save_state() as it is today. - Disable wakeup for a dead root hub before the early return. - Dropped Alan's Acked-by because of both changes. drivers/usb/core/hcd-pci.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)