Re: [PATCH v7 2/2] PCI: Don't put non-power manageable PCIe root ports into D3

[Mario Limonciello]

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.