Hi Shyam,
On 9/5/23 12:08, Shyam Sundar S K wrote:
>
>
> On 8/29/2023 10:42 PM, Mario Limonciello wrote:
>> commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>> changed pci_bridge_d3_possible() so that any vendor's PCIe ports
>> from modern machines (>=2015) are allowed to be put into D3.
>>
>> 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 D3 and AMD's platform can't handle USB devices waking from
>> a hardware sleep state in this case.
>>
>> This problem only occurs on Linux, and only when the AMD PMC driver
>> is utilized to put the device into a hardware sleep state. Comparing
>> the behavior on Windows and Linux, Windows doesn't put the root ports
>> into D3.
>>
>> A variety of approaches were discussed to change PCI core to handle this
>> case generically but no consensus was reached. To limit the scope of
>> effect only to the affected machines introduce a workaround into the
>> amd-pmc driver to only apply to the PCI root ports in affected machines
>> when going into hardware sleep.
>>
>> Link: https://lore.kernel.org/linux-pci/20230818193932.27187-1-mario.limonciello@xxxxxxx/
>> Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>> Reported-by: Iain Lane <iain@xxxxxxxxxxxxxxxxxxx>
>> 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@xxxxxxx>
>
> See if this change can be moved to pmc-quirks.c, besides that change
> looks good to me. Thank you.
>
> Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
Thank you for the review.
I also just replied to this series (to the cover-letter)
with an alternative approach based on making the
XHCI driver call pci_d3cold_disable() on the XHCI
PCIe-device on affected AMD chipsets.
That seems like a cleaner approach to me. I wonder
if you have any remarks about that approach ?
Regards,
Hans
>
>> ---
>> v15->v16:
>> * Only match PCIe root ports with ACPI companions
>> * Use constraints when workaround activated
>> ---
>> drivers/platform/x86/amd/pmc/pmc.c | 39 ++++++++++++++++++++++++++++++
>> 1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>> index eb2a4263814c..6a037447ec5a 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -741,6 +741,41 @@ static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev)
>> return 0;
>> }
>>
>> +/* only allow PCIe root ports with a LPS0 constraint configured to go to D3 */
>> +static int amd_pmc_rp_wa(struct amd_pmc_dev *pdev)
>> +{
>> + struct pci_dev *pci_dev = NULL;
>> +
>> + while ((pci_dev = pci_get_device(PCI_VENDOR_ID_AMD, PCI_ANY_ID, pci_dev))) {
>> + struct acpi_device *adev;
>> + int constraint;
>> +
>> + if (!pci_is_pcie(pci_dev) ||
>> + !(pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT))
>> + continue;
>> +
>> + if (pci_dev->current_state == PCI_D3hot ||
>> + pci_dev->current_state == PCI_D3cold)
>> + continue;
>> +
>> + adev = ACPI_COMPANION(&pci_dev->dev);
>> + if (!adev)
>> + continue;
>> +
>> + constraint = acpi_get_lps0_constraint(adev);
>> + if (constraint != ACPI_STATE_UNKNOWN &&
>> + constraint >= ACPI_STATE_S3)
>> + continue;
>> +
>> + if (pci_dev->bridge_d3 == 0)
>> + continue;
>> + pci_dev->bridge_d3 = 0;
>> + dev_info(&pci_dev->dev, "Disabling D3 on PCIe root port due lack of constraint\n");
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
>> {
>> struct rtc_device *rtc_device;
>> @@ -893,6 +928,10 @@ static int amd_pmc_suspend_handler(struct device *dev)
>> case AMD_CPU_ID_CZN:
>> rc = amd_pmc_czn_wa_irq1(pdev);
>> break;
>> + case AMD_CPU_ID_YC:
>> + case AMD_CPU_ID_PS:
>> + rc = amd_pmc_rp_wa(pdev);
>> + break;
>> default:
>> break;
>> }
>>
>
