| Message ID | 20230913040832.114610-3-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add quirk for PCIe root port on AMD systems | expand |
On 9/12/2023 23:25, Lukas Wunner wrote: > On Tue, Sep 12, 2023 at 11:08:32PM -0500, Mario Limonciello wrote: >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -2752,6 +2752,11 @@ int pci_prepare_to_sleep(struct pci_dev *dev) >> if (target_state == PCI_POWER_ERROR) >> return -EIO; >> >> + /* quirk to avoid setting D3 */ >> + if (wakeup && dev->dev_flags & PCI_DEV_FLAGS_NO_WAKE_D3 && >> + (target_state == PCI_D3hot || target_state == PCI_D3cold)) >> + target_state = PCI_D0; >> + >> pci_enable_wake(dev, target_state, wakeup); >> >> error = pci_set_power_state(dev, target_state); > > Would it be possible to just add the affected system to > bridge_d3_blacklist[]? It's initially reported on Lenovo Z13, but it affects all Rembrandt and Phoenix machines that have USB4 controller enabled. It's reproduced on every OEM system I have access to. > > Or would that defeat power management of other (non-affected) > Root Ports in the same machine? > > There's already PCI_DEV_FLAGS_NO_D3, would it be possible to just > reuse that instead of adding another codepath for D3 quirks? > The root port can handle D3 (including wakeup) at runtime fine. Issue occurs only during s2idle w/ hardware sleep. In v16/v17 (see cover letter for links) Rafael suggested to tie this specifically to suspend behavior and when wakeup flag is set. I didn't think it was appropriate to overload the existing flag because of this difference. > Thanks, > > Lukas
Hi Mario, kernel test robot noticed the following build warnings: [auto build test WARNING on 0bb80ecc33a8fb5a682236443c1e740d5c917d1d] url: https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/PCI-Move-the-PCI_CLASS_SERIAL_USB_USB4-definition-to-common-header/20230913-121559 base: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d patch link: https://lore.kernel.org/r/20230913040832.114610-3-mario.limonciello%40amd.com patch subject: [PATCH v18 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers config: i386-randconfig-r023-20230913 (https://download.01.org/0day-ci/archive/20230913/202309131834.q68yWKdZ-lkp@intel.com/config) compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230913/202309131834.q68yWKdZ-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202309131834.q68yWKdZ-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/pci/quirks.c:3890:15: warning: using the result of an assignment as a condition without parentheses [-Wparentheses] while (child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child)) { ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/pci/quirks.c:3890:15: note: place parentheses around the assignment to silence this warning while (child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child)) { ^ ( ) drivers/pci/quirks.c:3890:15: note: use '==' to turn this assignment into an equality comparison while (child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child)) { ^ == 1 warning generated. vim +3890 drivers/pci/quirks.c 3775 3776 /* 3777 * Some AMD/ATI GPUS (HD8570 - Oland) report that a D3hot->D0 transition 3778 * causes a reset (i.e., they advertise NoSoftRst-). This transition seems 3779 * to have no effect on the device: it retains the framebuffer contents and 3780 * monitor sync. Advertising this support makes other layers, like VFIO, 3781 * assume pci_reset_function() is viable for this device. Mark it as 3782 * unavailable to skip it when testing reset methods. 3783 */ 3784 DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_ATI, PCI_ANY_ID, 3785 PCI_CLASS_DISPLAY_VGA, 8, quirk_no_pm_reset); 3786 3787 /* 3788 * Thunderbolt controllers with broken MSI hotplug signaling: 3789 * Entire 1st generation (Light Ridge, Eagle Ridge, Light Peak) and part 3790 * of the 2nd generation (Cactus Ridge 4C up to revision 1, Port Ridge). 3791 */ 3792 static void quirk_thunderbolt_hotplug_msi(struct pci_dev *pdev) 3793 { 3794 if (pdev->is_hotplug_bridge && 3795 (pdev->device != PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C || 3796 pdev->revision <= 1)) 3797 pdev->no_msi = 1; 3798 } 3799 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_RIDGE, 3800 quirk_thunderbolt_hotplug_msi); 3801 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EAGLE_RIDGE, 3802 quirk_thunderbolt_hotplug_msi); 3803 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_PEAK, 3804 quirk_thunderbolt_hotplug_msi); 3805 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C, 3806 quirk_thunderbolt_hotplug_msi); 3807 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE, 3808 quirk_thunderbolt_hotplug_msi); 3809 3810 #ifdef CONFIG_ACPI 3811 /* 3812 * Apple: Shutdown Cactus Ridge Thunderbolt controller. 3813 * 3814 * On Apple hardware the Cactus Ridge Thunderbolt controller needs to be 3815 * shutdown before suspend. Otherwise the native host interface (NHI) will not 3816 * be present after resume if a device was plugged in before suspend. 3817 * 3818 * The Thunderbolt controller consists of a PCIe switch with downstream 3819 * bridges leading to the NHI and to the tunnel PCI bridges. 3820 * 3821 * This quirk cuts power to the whole chip. Therefore we have to apply it 3822 * during suspend_noirq of the upstream bridge. 3823 * 3824 * Power is automagically restored before resume. No action is needed. 3825 */ 3826 static void quirk_apple_poweroff_thunderbolt(struct pci_dev *dev) 3827 { 3828 acpi_handle bridge, SXIO, SXFP, SXLV; 3829 3830 if (!x86_apple_machine) 3831 return; 3832 if (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) 3833 return; 3834 3835 /* 3836 * SXIO/SXFP/SXLF turns off power to the Thunderbolt controller. 3837 * We don't know how to turn it back on again, but firmware does, 3838 * so we can only use SXIO/SXFP/SXLF if we're suspending via 3839 * firmware. 3840 */ 3841 if (!pm_suspend_via_firmware()) 3842 return; 3843 3844 bridge = ACPI_HANDLE(&dev->dev); 3845 if (!bridge) 3846 return; 3847 3848 /* 3849 * SXIO and SXLV are present only on machines requiring this quirk. 3850 * Thunderbolt bridges in external devices might have the same 3851 * device ID as those on the host, but they will not have the 3852 * associated ACPI methods. This implicitly checks that we are at 3853 * the right bridge. 3854 */ 3855 if (ACPI_FAILURE(acpi_get_handle(bridge, "DSB0.NHI0.SXIO", &SXIO)) 3856 || ACPI_FAILURE(acpi_get_handle(bridge, "DSB0.NHI0.SXFP", &SXFP)) 3857 || ACPI_FAILURE(acpi_get_handle(bridge, "DSB0.NHI0.SXLV", &SXLV))) 3858 return; 3859 pci_info(dev, "quirk: cutting power to Thunderbolt controller...\n"); 3860 3861 /* magic sequence */ 3862 acpi_execute_simple_method(SXIO, NULL, 1); 3863 acpi_execute_simple_method(SXFP, NULL, 0); 3864 msleep(300); 3865 acpi_execute_simple_method(SXLV, NULL, 0); 3866 acpi_execute_simple_method(SXIO, NULL, 0); 3867 acpi_execute_simple_method(SXLV, NULL, 0); 3868 } 3869 DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL, 3870 PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C, 3871 quirk_apple_poweroff_thunderbolt); 3872 3873 3874 /* 3875 * Putting PCIe root ports on Ryzen SoCs with USB4 controllers into D3 power 3876 * states may cause problems when the system attempts wake up from s2idle. 3877 * 3878 * This manifests as a missing wakeup interrupt on the following systems: 3879 * Family 19h model 44h (PCI 0x14b9) 3880 * Family 19h model 74h (PCI 0x14eb) 3881 * Family 19h model 78h (PCI 0x14eb) 3882 * 3883 * Add a quirk to the root port if a USB4 controller is connected to it 3884 * to avoid D3 power states. 3885 */ 3886 static void quirk_ryzen_rp_d3(struct pci_dev *pdev) 3887 { 3888 struct pci_dev *child = NULL; 3889 > 3890 while (child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child)) { 3891 if (pci_upstream_bridge(child) != pdev) 3892 continue; 3893 pdev->dev_flags |= PCI_DEV_FLAGS_NO_WAKE_D3; 3894 pci_info(pdev, "quirk: disabling D3 for wakeup\n"); 3895 break; 3896 } 3897 } 3898 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x14b9, quirk_ryzen_rp_d3); 3899 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x14eb, quirk_ryzen_rp_d3); 3900 #endif 3901
On Tue, Sep 12, 2023 at 11:43:53PM -0500, Mario Limonciello wrote: > On 9/12/2023 23:25, Lukas Wunner wrote: > > There's already PCI_DEV_FLAGS_NO_D3, would it be possible to just > > reuse that instead of adding another codepath for D3 quirks? > > > > The root port can handle D3 (including wakeup) at runtime fine. > Issue occurs only during s2idle w/ hardware sleep. I see. If this only affects system sleep, not runtime PM, what you can do is define a DECLARE_PCI_FIXUP_SUSPEND_LATE() which calls pci_d3cold_disable() and also define a DECLARE_PCI_FIXUP_CLASS_RESUME_EARLY() which calls pci_d3cold_enable(). And I think you can make those calls conditional on pm_suspend_no_platform() to constrain to s2idle. User space should still be able to influence runtime PM via the d3cold_allowed flag (unless I'm missing something). Thanks, Lukas
On 9/13/2023 09:31, Lukas Wunner wrote: > On Tue, Sep 12, 2023 at 11:43:53PM -0500, Mario Limonciello wrote: >> On 9/12/2023 23:25, Lukas Wunner wrote: >>> There's already PCI_DEV_FLAGS_NO_D3, would it be possible to just >>> reuse that instead of adding another codepath for D3 quirks? >>> >> >> The root port can handle D3 (including wakeup) at runtime fine. >> Issue occurs only during s2idle w/ hardware sleep. > > I see. > > If this only affects system sleep, not runtime PM, what you can do is > define a DECLARE_PCI_FIXUP_SUSPEND_LATE() which calls pci_d3cold_disable() > and also define a DECLARE_PCI_FIXUP_CLASS_RESUME_EARLY() which calls > pci_d3cold_enable(). > > And I think you can make those calls conditional on pm_suspend_no_platform() > to constrain to s2idle. > > User space should still be able to influence runtime PM via the > d3cold_allowed flag (unless I'm missing something). > > Thanks, > > Lukas The part you're missing is that D3hot is affected by this issue too, otherwise it would be a good proposal.
On Wed, Sep 13, 2023 at 11:36:49AM -0500, Mario Limonciello wrote: > On 9/13/2023 09:31, Lukas Wunner wrote: > > If this only affects system sleep, not runtime PM, what you can do is > > define a DECLARE_PCI_FIXUP_SUSPEND_LATE() which calls pci_d3cold_disable() > > and also define a DECLARE_PCI_FIXUP_CLASS_RESUME_EARLY() which calls > > pci_d3cold_enable(). > > > > And I think you can make those calls conditional on pm_suspend_no_platform() > > to constrain to s2idle. > > > > User space should still be able to influence runtime PM via the > > d3cold_allowed flag (unless I'm missing something). > > The part you're missing is that D3hot is affected by this issue too, > otherwise it would be a good proposal. I recall that in an earlier version of the patch, you solved the issue by amending pci_bridge_d3_possible(). Changing the dev->no_d3cold flag indirectly influences the bridge_d3 flag (through pci_dev_check_d3cold() and pci_bridge_d3_update()). If dev->no_d3cold is set on a device below a port, that port is prevented from entring D3hot because it would result in the device effectively being in D3cold. So you might want to take a closer look at this approach despite the flag suggesting that it only influences D3cold. Thanks, Lukas
On 9/14/2023 09:17, Lukas Wunner wrote: > On Wed, Sep 13, 2023 at 11:36:49AM -0500, Mario Limonciello wrote: >> On 9/13/2023 09:31, Lukas Wunner wrote: >>> If this only affects system sleep, not runtime PM, what you can do is >>> define a DECLARE_PCI_FIXUP_SUSPEND_LATE() which calls pci_d3cold_disable() >>> and also define a DECLARE_PCI_FIXUP_CLASS_RESUME_EARLY() which calls >>> pci_d3cold_enable(). >>> >>> And I think you can make those calls conditional on pm_suspend_no_platform() >>> to constrain to s2idle. >>> >>> User space should still be able to influence runtime PM via the >>> d3cold_allowed flag (unless I'm missing something). >> >> The part you're missing is that D3hot is affected by this issue too, >> otherwise it would be a good proposal. > > I recall that in an earlier version of the patch, you solved the issue > by amending pci_bridge_d3_possible(). > > Changing the dev->no_d3cold flag indirectly influences the bridge_d3 > flag (through pci_dev_check_d3cold() and pci_bridge_d3_update()). > > If dev->no_d3cold is set on a device below a port, that port is > prevented from entring D3hot because it would result in the > device effectively being in D3cold. > > So you might want to take a closer look at this approach despite > the flag suggesting that it only influences D3cold. > Ah; I hadn't considered setting it on a device below the port. In this particular situation the only devices below the root port are USB controllers. If those devices don't go into D3 the system can't enter hardware sleep.
On Thu, Sep 14, 2023 at 09:31:38AM -0500, Mario Limonciello wrote: > On 9/14/2023 09:17, Lukas Wunner wrote: > > On Wed, Sep 13, 2023 at 11:36:49AM -0500, Mario Limonciello wrote: > > > On 9/13/2023 09:31, Lukas Wunner wrote: > > > > If this only affects system sleep, not runtime PM, what you can do is > > > > define a DECLARE_PCI_FIXUP_SUSPEND_LATE() which calls pci_d3cold_disable() > > > > and also define a DECLARE_PCI_FIXUP_CLASS_RESUME_EARLY() which calls > > > > pci_d3cold_enable(). > > > > > > > > And I think you can make those calls conditional on pm_suspend_no_platform() > > > > to constrain to s2idle. > > > > > > > > User space should still be able to influence runtime PM via the > > > > d3cold_allowed flag (unless I'm missing something). > > > > > > The part you're missing is that D3hot is affected by this issue too, > > > otherwise it would be a good proposal. > > > > I recall that in an earlier version of the patch, you solved the issue > > by amending pci_bridge_d3_possible(). > > > > Changing the dev->no_d3cold flag indirectly influences the bridge_d3 > > flag (through pci_dev_check_d3cold() and pci_bridge_d3_update()). > > > > If dev->no_d3cold is set on a device below a port, that port is > > prevented from entring D3hot because it would result in the > > device effectively being in D3cold. > > > > So you might want to take a closer look at this approach despite > > the flag suggesting that it only influences D3cold. > > > > Ah; I hadn't considered setting it on a device below the port. In this > particular situation the only devices below the root port are USB > controllers. > > If those devices don't go into D3 the system can't enter hardware sleep. If you set dev->no_d3cold on the USB controllers, they should still be able to go to D3hot, but not D3cold, which perhaps might be sufficient. It should prevent D3cold *and* D3hot on the Root Port above. And if you set that on system sleep in a quirk and clear it on resume, runtime PM shouldn't be affected. Thanks, Lukas
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 59c01d68c6d5..a113b8941d09 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -2752,6 +2752,11 @@ int pci_prepare_to_sleep(struct pci_dev *dev) if (target_state == PCI_POWER_ERROR) return -EIO; + /* quirk to avoid setting D3 */ + if (wakeup && dev->dev_flags & PCI_DEV_FLAGS_NO_WAKE_D3 && + (target_state == PCI_D3hot || target_state == PCI_D3cold)) + target_state = PCI_D0; + pci_enable_wake(dev, target_state, wakeup); error = pci_set_power_state(dev, target_state); diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index eeec1d6f9023..c6f2c301f62a 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3869,6 +3869,34 @@ static void quirk_apple_poweroff_thunderbolt(struct pci_dev *dev) DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C, quirk_apple_poweroff_thunderbolt); + + +/* + * Putting PCIe root ports on Ryzen SoCs with USB4 controllers into D3 power + * states may cause problems when the system attempts wake up from s2idle. + * + * This manifests as a missing wakeup interrupt on the following systems: + * Family 19h model 44h (PCI 0x14b9) + * Family 19h model 74h (PCI 0x14eb) + * Family 19h model 78h (PCI 0x14eb) + * + * Add a quirk to the root port if a USB4 controller is connected to it + * to avoid D3 power states. + */ +static void quirk_ryzen_rp_d3(struct pci_dev *pdev) +{ + struct pci_dev *child = NULL; + + while (child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child)) { + if (pci_upstream_bridge(child) != pdev) + continue; + pdev->dev_flags |= PCI_DEV_FLAGS_NO_WAKE_D3; + pci_info(pdev, "quirk: disabling D3 for wakeup\n"); + break; + } +} +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x14b9, quirk_ryzen_rp_d3); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x14eb, quirk_ryzen_rp_d3); #endif /* diff --git a/include/linux/pci.h b/include/linux/pci.h index 8c7c2c3c6c65..2292fbc20630 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -245,6 +245,8 @@ enum pci_dev_flags { PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11), /* Device does honor MSI masking despite saying otherwise */ PCI_DEV_FLAGS_HAS_MSI_MASKING = (__force pci_dev_flags_t) (1 << 12), + /* Wakeup events don't work over D3 */ + PCI_DEV_FLAGS_NO_WAKE_D3 = (__force pci_dev_flags_t) (1 << 13), }; enum pci_irq_reroute_variant {
Iain reports that USB devices can't be used to wake a Lenovo Z13 from suspend. This is because the PCIe root port has been put into D3hot and AMD's platform can't handle USB devices waking the platform from a hardware sleep state in this case. This problem only occurs on Linux, when waking from a platform hardware sleep state. Comparing the behavior on Windows and Linux, Windows doesn't put the root ports into D3. In Windows systems that support Modern Standby specify hardware pre-conditions for the SoC to achieve the lowest power state by device constraints in a SOC specific "Power Engine Plugin" (PEP) [1] [2]. They can be marked as disabled or enabled and when enabled can specify the minimum power state required for an ACPI device. The policy on Linux does not take constraints into account to decide what state to put the device into at suspend like Windows does. Rather for devices that support D3, the target state is selected by this policy: 1. If platform_pci_power_manageable(): Use platform_pci_choose_state() 2. If the device is armed for wakeup: Select the deepest D-state that supports a PME. 3. Else: Use D3hot. Devices are considered power manageable by the platform when they have one or more objects described in the table in section 7.3 of the ACPI 6.5 specification [3]. If devices are not considered power manageable; specs are ambiguous as to what should happen. In this situation Windows 11 leaves PCIe ports in D0 while Linux puts them into D3 due to the policy introduced by commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend"). As the Windows PEP driver uses constraints to express the desired state that should be selected for suspend but Linux doesn't introduce a quirk for the problematic root ports. When selecting a target state specifically for sleep in `pci_prepare_to_sleep` this quirk will prevent the root ports from selecting D3hot or D3cold if they have been configured as a wakeup source. Cc: stable@vger.kernel.org Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/platform-design-for-modern-standby#low-power-core-silicon-cpu-soc-dram [1] Link: https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf [2] Link: https://uefi.org/specs/ACPI/6.5/07_Power_and_Performance_Mgmt.html#device-power-management-objects [3] Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend") Reported-by: Iain Lane <iain@orangesquash.org.uk> Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121 Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- The same PCI ID is used for multiple different root ports. This problem only affects the root port that the USB4 controller is connected to. drivers/pci/pci.c | 5 +++++ drivers/pci/quirks.c | 28 ++++++++++++++++++++++++++++ include/linux/pci.h | 2 ++ 3 files changed, 35 insertions(+)