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

[Rafael J. Wysocki]

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?

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.