On 8/1/23 05:15, Rafael J. Wysocki wrote:
On Tue, Aug 1, 2023 at 5:25 AM Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
On 7/14/23 19:46, Limonciello, Mario wrote:
On 7/14/2023 2:17 PM, Rafael J. Wysocki wrote:
Generally speaking, pci_bridge_d3_possible() is there to prevent
bridges (and PCIe ports in particular) from being put into D3hot/cold
if there are reasons to believe that it may not work.
acpi_pci_bridge_d3() is part of that.
Even if it returns 'true', the _SxD/_SxW check should still be applied
via pci_target_state() to determine whether or not the firmware allows
this particular bridge to go into D3hot/cold. So arguably, the _SxW
check in acpi_pci_bridge_d3() should not be necessary and if it makes
any functional difference, there is a bug somewhere else.
But only if it was power manageable would the _SxD/_SxW check be
applied. This issue is around the branch of pci_target_state() where
it's not power manageable and so it uses PME or it falls back to D3hot.
Well, this looks like a spec interpretation difference.
We thought that _SxD/_SxW would only be relevant for devices with ACPI
PM support, but the firmware people seem to think that those objects
are also relevant for PCI devices that don't have ACPI PM support
(because those devices are still power-manageable via PMCSR). If
Windows agrees with that viewpoint, we'll need to adjust, but not
through adding _SxW checks in random places.
I think that depends upon how you want to handle the lack of _S0W.
On these problematic devices there is no _S0W under the PCIe
root port. As I said; Windows puts them into D0 in this case though.
So acpi_dev_power_state_for_wake should return ACPI_STATE_UNKNOWN.
Can you suggest where you think adding a acpi_dev_power_state_for_wake()
does make sense?
Two areas that I think would work would be in: pci_pm_suspend_noirq()
(to avoid calling pci_prepare_to_sleep)
or
directly in pci_prepare_to_sleep() to check that value in lieu of
pci_target_state().
Rafael,
Did you have any more thoughts on this?
Reportedly, if there are no ACPI power management objects associated
with a Root Port, Windows will always leave it in D0.
In that case, acpi_pci_bridge_d3() will return false unless the
HotPlugSupportInD3 property is present AFAICS, so the ACPI code will
not allow the port to be put into D3hot.
Consequently, platform_pci_bridge_d3() will return false and the only
thing that may allow the port to go into D0 is the dmi_get_bios_year()
check at the end of pci_bridge_d3_possible().
However, that was added, because there are Intel platforms on which
Root Ports need to be programmed into D3hot on suspend (which allows
the whole platform to reduce power significantly) and there are no
ACPI device power management objects associated with them (Mika should
know the gory details related to this). It looks like under Windows
the additional power reduction would not be possible on those systems,
but that would be a problem, wouldn't it?
I've been thinking on this today, and I at least have a hypothesis about
this behavior. Perhaps Windows is actually utilizing enabled PEP
constraints to enforce what state device should be put into over Modern
Standby cycles in the absence of ACPI objects.
In the case of one of my problematic system the PEP constraints for the
root port are:
Package (0x04)
{
0x00,
"\\_SB.PCI0.GP17",
0x00,
0x00
},
That first 0x00 means the constraint isn't actually enabled for the root
port.
Mika,
Could you get an acpidump from one of these problematic Intel systems so
we can check the PEP constraints to see if this theory works? Or maybe
you have some other ideas why this is different?
So it looks like there are some systems on which programming Root
Ports into D3hot is needed to achieve additional power reduction of
the platform and there are systems on which programming Root Ports
into D3hot breaks things and there are no ACPI power management
objects associated with these Root Ports in both cases.
The only way to make progress that I can think about right now is to
limit the dmi_get_bios_year() check at the end of
pci_bridge_d3_possible() to Intel platforms.
Yeah if we can't come up with a method that works for both this might be
the only real option.
