On Thu, Mar 31, 2022 at 02:33:12PM -0500, Limonciello, Mario wrote: > On 3/31/2022 14:04, Bjorn Helgaas wrote: > > [+cc Rafael, Mika, linux-pm] > > On Mon, Mar 28, 2022 at 03:55:18PM -0500, Mario Limonciello wrote: > > ... > > On some platforms, e.g., AMD Yellow Carp, firmware may supply > > "HotPlugSupportInD3" even though _S0W tells us the device cannot > > wake from D3hot. With the previous code, these devices could be put > > in D3hot and hotplugged devices would not be recognized. > > > > If _S0W exists and says the Root Port cannot wake itself from D3hot, > > return "false" to indicate that "dev" cannot handle hotplug events > > while in D3. > > > > 1) "dev" has a _PS0 or _PR0 method, or > > > > 2a) The Root Port above "dev" has _PRW and > > > > 2b) If the Root Port above "dev" has _S0W, it can wake from D3hot or > > D3cold and > > > > 2c) The Root Port above "dev" has a _DSD with a > > "HotPlugSupportInD3" property with value 1. > > Very well, I'll incorporate into the commit message and scrap some of the > old stuff. > > > The _S0W part makes sense to me. The _PRW part hasn't been explained > > yet. We didn't depend on it before, but we think it's safe to depend > > on it now? > > An earlier version of this patch actually was only checking this rather than > _S0W alone. It was suggested that both should be checked together by > Rafael. > > https://lore.kernel.org/linux-pci/CAJZ5v0grj=vE1wGJpMxh-Hy7=ommfFUh5hw++nmQdLVxVtCSWw@xxxxxxxxxxxxxx/ > > FWIW at least some earlier versions Rafael and Mika both agreed towards that > direction (and presumably weren't worried about existing systems that this > code was used for). OK. I think it's worth mentioning _PRW even if we don't have a good explanation for it. For someone bringing up a platform like this, that will be a better hint than "adev->wakeup.flags.valid" alone. > > In the commit log and comments, can we be more explicit about whether > > "D3" means "D3hot" or "D3cold"? > > The check for _S0W return is looking for "3", so it's really D3hot "or" > D3cold. In the problematic case on Yellow Carp, it was D3hot. I'll add > this detail. Linux advertises OSC_SB_PR3_SUPPORT, so I think we are checking for D3hot. Of course, if a device can't wake from D3hot, it can't wake from D3cold either. You didn't *add* any D3 comments and only quoted the Microsoft doc, so maybe nothing to change in this patch. It's confusing to me that the existing code, like acpi_pci_bridge_d3(), pci_bridge_d3_possible(), pci_bridge_d3_force, etc., are all ambiguous. Bjorn
