Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a012ce3d-90f3-48fb-caa5-476f6c9f7fe3@amd.com>
Date:   Fri, 2 Jun 2023 19:58:47 -0500
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH v5 2/2] PCI: Don't assume root ports are power manageable
Content-Language: en-US
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        S-k Shyam-sundar <Shyam-sundar.S-k@amd.com>,
        Natikar Basavaraj <Basavaraj.Natikar@amd.com>,
        Deucher Alexander <Alexander.Deucher@amd.com>,
        "Rafael J . Wysocki" <rafael@kernel.org>, linux-pm@vger.kernel.org,
        Lukas Wunner <lukas@wunner.de>,
        Iain Lane <iain@orangesquash.org.uk>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>
References: <ZHp45fNGuPUWPnO7@bhelgaas>
From:   "Limonciello, Mario" <mario.limonciello@amd.com>
In-Reply-To: <ZHp45fNGuPUWPnO7@bhelgaas>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cy8rOUNDLy9lblhhY3h4ZEZ5ZGtuR25jQUpCaXE1RWN0cDVFUlIwSG4vSjFt?=
 =?utf-8?B?NTk1WmxXcGoraDA5dUY5VVVKb1NuNjFxUXdkYVdBRElPWGNVL2gzTnNsMzYw?=
 =?utf-8?B?enpQdlZUbHNXK2NzZS9JT1Zzdnk2Z2t4SjBFNDRNS1ZOWmZnYkQwc0w5WlRp?=
 =?utf-8?B?MllKQURhK2s5OTVnUzdsY3A0ZGVzaDZnYVlyS2UvbytBOE1EcWFLSzZhb0Ur?=
 =?utf-8?B?VUVWMk1xc3dXaEJ3OEl2aGVuUG9aWEVRcmVFcUFoVlFxVkc4dFczOFNNNTlz?=
 =?utf-8?B?OWRUVWE2WTFFRTlhT0dWaTcveWc4VTZBY1pOWnZsK01GaW8xNEpoeWZTcW9z?=
 =?utf-8?B?cnRDYzN1c3ZYS2FTUjdiZC9yZlVGWnpuTnQxMHQ1alNqOEdWdCtzZE4yYVQ1?=
 =?utf-8?B?eklXQWliQlFGUU4ybUtsL2FjblFpTkttL1oxSDVSOERldTE2MTZOZlFRdEp6?=
 =?utf-8?B?VWhnT05QLzZBREdiV3VHU2FkajNnNUhPSU5DR3V6U2UzU25TczN6cFoxTFBB?=
 =?utf-8?B?c0s4N0dLckkvYzA1NVlXN1dFUk5XejVCODFUNTFXMXQ5QXljWERvcUlOSmV2?=
 =?utf-8?B?c080bVlFcFo3d3F0Tkl3Y0FLS2greS9VUzdjaDU4aXRLRm5VRTRFSG5nTVd1?=
 =?utf-8?B?U2M3NDhPb3ZudXBjdWtYcXRtOFF1dWZiU1BtUW5ycVl0VUgvbEJ2RU9TdXI0?=
 =?utf-8?B?eEViejV1N3Y3K2hPa3hWTWFQM2FxN296WWg5R0ZMWEpxTEs1dG1FU2lnU1lr?=
 =?utf-8?B?S2twN1lURjJwcmcwb3ppeWh5WnpHZVpueUJNMDRvTExqclJEQUtRU2NVYjI3?=
 =?utf-8?B?ZnIvMENZdnc1MHpZQk92TnUxMG5GcHlPTC9VVnQwRS9taHM2a2lVQlYrc2I4?=
 =?utf-8?B?eWdoZ3lBakh5VU13bVNmV2FTMXJMY0tRbmJ2UlRIcXpJQlIxc0dUWEZ0aDFE?=
 =?utf-8?B?dklrLzdpaGpRWktEM3BvcWJSYTZYUlNyYWUvZC96NFlYbVJyR3owVWFRV01X?=
 =?utf-8?B?SGEyamRxZWJBSmhSVE5sVFNETnpTNWh2TGJnWUlmNE8zdDQwZnVST1drMzN4?=
 =?utf-8?B?WmZIWnJPOGJvUDkyYjczZCszOUFRSk45YzVZNVpzUEhadkM0enh5Vk0zeURu?=
 =?utf-8?B?NDMvdFBsdWlXZWVxazVySXpOR2g0emtoSVVnejJCeGRXL0N1SzBacVV6YWd1?=
 =?utf-8?B?ZHVyL1VIcEkvRVJRTXRTWTNhaTlTYi9raUkycnJIWXJsL0VCRllic01OdjZ1?=
 =?utf-8?B?VmNBYWJ2VnBJL1lGaXRRaGdROGhEUnRwL2lLaUgvNDRGZFE4WGI0aFZ4bGhm?=
 =?utf-8?B?TkRDNEhLbmNMVmg3MmFFZXRiL21NMEJQbU9DSithbjRscWJCZUQ1S2tDajkv?=
 =?utf-8?B?c0Q3Q1BSSEMwdFVYRGUyeUxMRzRvWVVibmIyYWRJcllZQXc0WldjaCswL3da?=
 =?utf-8?B?NHRJdFZNTXNKcVBuTnM0WStMZmc1SSt0T2s0UDlLdldrbUVwd0dPT0VQMmFO?=
 =?utf-8?B?YTExVTZqZEk5VDVkTjFubTQ4dHFFNEFYZUo3bUZDZlpyOWtqRjZSTFc4QVlv?=
 =?utf-8?B?RllMN1RRREQ0Qjc3TndHd2VmSEsyMm4yczJWRWwzbUZLSHVnRTB6V0xtTjdW?=
 =?utf-8?B?enZTaGozYjJFcFE4VkZYQTd2ZkZEek0vcG5UY3RoVVYySVlxeWV1eGpMUGN5?=
 =?utf-8?B?V2psOC9IT000aDdmck9ZYVZzakZKSTJNY0JSbXU1bVNRNUVxMThOV0NXL3Bk?=
 =?utf-8?B?L3cxcTEwZ0NCU0xJcUUvRy9wL2NodDlaZEVUdmhPZ0lZVlkyVGZpMzVMYkto?=
 =?utf-8?B?Q2VRV3lwbGMzV1Q2Smg4ay8xMTBudkNpWmdQOHF2UGI4VEIzaG1DOGFLSjU0?=
 =?utf-8?B?NE51RmxMOURZNFZoZUJhNWRaMjRQdUJ1VWpkTUNVVGFUVmd3WjdpVkRJQ3Fs?=
 =?utf-8?B?djdwRE5zWGIxUVdpSGd6c0J1Q3lvaTVtc2ZGMUg4ZmtoNFU3c1ZRSGJ0ZE4y?=
 =?utf-8?B?cS96cmZYdGdzeTRrWDN0RkFWSm5FZG8zQlZISDR5eDBtd1JHUjVJUGoxWk9x?=
 =?utf-8?B?Wnk0a2VmZE9yZDdIZkcvOWhQS000cGxyaDcvNlQvejM3ckF0d2dOK3VKS1Ev?=
 =?utf-8?B?VHkyRHlwdkJ2OTdsNWFZdCtUaU9QM0xkV2dRckVmb3BObnlCaFlGN0M0UHJi?=
 =?utf-8?Q?yp93d4BwEPWE3CgOqEbpNpRMi8/TkbG5dpXJbfe+Auxd?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 607a1341-4160-426a-a438-08db63cdb168
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB6095.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jun 2023 00:58:50.2070
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: LZaB3XC2efdFIIJ8vXNGRXOD2v7pfP3MCLWpSOcivFlUZY6N1pPz7nbCvVffk/AadufNRNN6DU+TJVpCa8v+Zw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB6392
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 6/2/2023 6:19 PM, Bjorn Helgaas wrote:
> On Fri, Jun 02, 2023 at 05:38:37PM -0500, Limonciello, Mario wrote:
>> On 6/2/2023 5:20 PM, Bjorn Helgaas wrote:
>>> On Tue, May 30, 2023 at 11:39:47AM -0500, Mario Limonciello wrote:
>>>> Using a USB keyboard or mouse to wakeup the system from s2idle fails when
>>>> that xHCI device is connected to a USB-C port for an AMD USB4 router.
>>> It sounds like the real issue is that "Root Ports in D3hot/D3cold may
>>> not support wakeup", and the USB, xHCI, USB-C, AMD USB4 router bits
>>> are probably not really relevant.  And hopefully even the "AMD
>>> platforms" mentioned below is not relevant.
>> Yeah.  It comes down to how much you want in the commit
>> about how we got to this conclusion versus a generic
>> fix.  I generally like to be verbose about a specific case
>> something fixes so that when distros decide what to pull
>> in to their older maintenance kernels they can understand
>> what's important.
> That's actually my point.  I think this problem probably affects
> non-USB devices, non-xHCI devices, non-USB4 routers, etc.
>
> If we can say "Any device below a Root Port in D3hot/D3cold may not
> support wakeup if X, Y, Z.  Root Ports may be put in D3hot/D3cold for
> sleep/hibernate/s2idle/...",  that's much more actionable.
Completely agree.
>
>>>> Due to commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>>>> all PCIe ports go into D3 during s2idle.
>>>>
>>>> When specific root ports are put into D3 over s2idle on some AMD platforms
>>>> it is not possible for the platform to properly identify wakeup sources.
>>>> This happens whether the root port goes into D3hot or D3cold.
>>> Can we connect this to a spec so it's not just the empirical "some AMD
>>> platforms work like X" observation?
>>>
>>> "s2idle" is meaningful on the power management side of the house, but
>>> it doesn't appear in PCI or ACPI specs, so I don't know what it means
>>> here.  I assume the D3hot/D3cold state of the Root Port is the
>>> critical factor, regardless of how it got there.
>> Unfortunately (?) for this particular issue it's only a
>> critical factor when the system is in s2idle.
>>
>> PME works fine to wake up the device if the root port is
>> in either D3hot or D3cold when the system isn't in s2idle.
> So that must mean something other than the Root Port has to be in some
> specific state.  "System in s2idle" is not actionable in terms of PCI
> maintenance.  It sounds like we just haven't really gotten to the root
> cause yet.
The root cause of this behavior is deep in the platform
firmware.  This is why the platform firmware doesn't
advertise power management support for the root port.
>
>>>> Linux shouldn't assume root ports support D3 just because they're on a
>>>> machine newer than 2015, the ports should also be deemed power manageable.
>>>> Add an extra check explicitly for root ports to ensure D3 isn't selected
>>>> for them if they are not power-manageable through platform firmware.
>>> But I *would* like to know specifically what "power manageable" means
>>> here.  I might naively assume that a device with the PCI Power
>>> Management Capability is "power manageable", and that if PME_Support
>>> includes D3hot and D3cold, we're good to go.  But obviously it's more
>>> complicated than that, and I'd like to cite the spec that mentions the
>>> actual things we need here.
>> Power manageable through platform firmware means the device
>> has ACPI methods like like _PR0, _PS0.
> What's the connection to wakeup?
There is also no _PRW (power resources for wake) for this
root port.
>
>>>> +	 * It's not safe to put root ports that don't support power
>>>> +	 * management into D3.
>>> I assume "it's not safe" really means "Root Ports in D3hot/D3cold may
>>> not be able to signal PME interrupts unless ... <mumble> platform
>>> firmware <mumble> e.g., ACPI method <mumble> ..."
>>>
>>> Can we include some of those hints here?
>> I'm cautious about hardcoding logic used by
>> acpi_bus_get_power_flags() in a comment in case it changes.
>>
>> How about:
>>
>> "Root ports in D3 may not be able to reliably signal wakeup
>> events unless platform firmware signals power management
>> capabilities".
> I'm really looking hard for that spec citation :)  Without that, this
> just devolves into "this seems to work on these systems."
I mean that's exactly what the broken logic below this
fix is....

But I think I can get you what you're looking for.

 From this failing system the problematic root port is GP19.

The DSDT has:

Device (GP19) {
     Method (_DSM,..)
     Method (_PRT,..)
     Device (NHI0)
     Device (NHI1)
}

The SSDT has:

Scope (\_SB.PCI0.GP19) {
Method (YS0W,..)
Method (YPRW,..)
Method (RPRM,..)
Method (WPRM,..)
Method (SPDP,..)
Method (SPCH,..)
Method (_STA,..)
Method (_INI,..)
Method (_REG,..)
}

Section 7.3 from the ACPI spec [1] says

"For a device that is power-managed using ACPI, a Definition Block 
contains one or more of the objects found in the table below. Power 
management of a device is done using Power Resource control"

The GP19 device has NONE of the objects mentioned in the table.

Outside of this change, I do think this means acpi_bus_get_power_flags() 
may want to also look for some of the other objects besides _PS0 and 
_PR0 to resolve that a device is power manageable though.

[1] 
https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/07_Power_and_Performance_Mgmt/device-power-management-objects.html?highlight=pr0#device-power-management-objects

>>>> +	 */
>>>> +	if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT &&
>>>> +	    !platform_pci_power_manageable(bridge))
>>>> +		return false;
>>>> +
>>>>    	/*
>>>>    	 * It should be safe to put PCIe ports from 2015 or newer
>>>>    	 * to D3.
>>>> -- 
>>>> 2.34.1
>>>>