Date:   Tue, 5 Sep 2023 14:57:33 -0500
From:   Mario Limonciello <mario.limonciello@....com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Shyam Sundar S K <Shyam-sundar.S-k@....com>,
        bhelgaas@...gle.com
Cc:     linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        platform-driver-x86@...r.kernel.org,
        Iain Lane <iain@...ngesquash.org.uk>, rafael@...nel.org
Subject: Re: [PATCH v16 3/3] platform/x86/amd: pmc: Don't let PCIe root ports
 go into D3

On 9/5/2023 05:15, Hans de Goede wrote:
> 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@amd.com/
>>> Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>>> Reported-by: Iain Lane <iain@...ngesquash.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@....com>
>>
>> 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@....com>
> 
> 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 ?
> 

I was thinking more about Hans' comments to the cover letter as well as 
Shyam's comments to move it into pmc-quirks.c.

Perhaps it would better be conveying what's going on by having a 
dedicated step that amd-pmc calls pci_choose_state() for each PCIe 
device and checks the value against the constraints.  If "any" of them 
are mismatched it could emit a message.  This is a little bit of 
duplication though because drivers/acpi/x86/s2idle.c already also emits 
a similar message for some devices when pm_debug_messages is enabled.

Then the special case would be for PCIe root ports that are mismatched 
the driver overrides it.  If this logic change is wouldn't make sense 
for it to be moved into pmc-quirks.c.

I don't think using pci_d3cold_disable() / pci_d3cold_enable() is 
correct though.  If PCI core stays the same it should still be setting 
pdev->bridge_d3 to zero.  The problem isn't with D3cold on the PCIe RP 
at s2didle, it's with D3hot.


> 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;
>>>   	}
>>>
>>
> 

Powered by blists - more mailing lists