Re: [PATCH v5] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

[Rafael J. Wysocki]

On Fri, Feb 28, 2025 at 7:40 PM Brian Norris <briannorris@xxxxxxxxxxxx> wrote:
>
> Hi Bjorn,
>
> On Fri, Feb 28, 2025 at 11:45:09AM -0600, Bjorn Helgaas wrote:
> > On Tue, Nov 26, 2024 at 03:17:11PM -0800, Brian Norris wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > >
> > > Unlike ACPI based platforms, there are no known issues with D3Hot for
> > > the PCI bridges in Device Tree based platforms.
> >
> > Can we elaborate on this a little bit?  Referring to "known issues
> > with ACPI-based platforms" depends on a lot of domain-specific history
> > that most readers (including me) don't know.
>
> Well, to me, the background here is simply the surrounding code context,
> and the past discussions that I linked:
>
> https://lore.kernel.org/linux-pci/20240227225442.GA249898@bhelgaas/
>
> The whole reason we need this patch is that:
> (a) there's some vaguely specified reason this function (which prevents
>     standard-specified behavior) exists; and
> (b) that function includes a condition that allows all systems with a
>     DMI/BIOS newer than year 2015 to use this feature.
>
> Digging a bit further, it seems like maybe the only reason this feature
> is prevented on DT systems is from commit ("9d26d3a8f1b0 PCI: Put PCIe
> ports into D3 during suspend"), where the subtext is that it was written
> by and for Intel in 2016, with an arbitrary time-based cutoff ("year
> this was being developed") that only works for DMI systems. DT systems
> do not tend to support DMI.
>
> If any of this is what you're looking for, I can try to
> copy/paste/summarize a few more of those bits, if it helps.
>
> > I don't think "ACPI-based" or "devicetree-based" are good
> > justifications for changing the behavior because they don't identify
> > any specific reasons.  It's like saying "we can enable this feature
> > because the platform spec is written in French."
>
> AIUI, It's involved because of the general strategy of this function
> (per its comments, "recent enough PCIe ports"). So far, it sounds like
> that reason (presumably, old BIOS with poor power management code)
> doesn't really apply to a system based on device tree, where the power
> management code is mostly/entirely in the OS.

No, it was about PCIe hardware failing to handle PM correctly on ports.

> But really, the original commit doesn't actually state reasons, so maybe
> the "known issues" phrasing could be weakened a bit, to avoid implying
> there were any stated reasons.

There were hardware issues related to PM on x86 platforms predating
the introduction of Connected Standby in Windows.  For instance,
programming a port into D3hot by writing to its PMCSR might cause the
PCIe link behind it to go down and the only way to revive it was to
power cycle the Root Complex.  And similar.

Also, PM has never really worked correctly on PCI (non-PCIe) bridges
and there is this case where the platform firmware handles hotplug and
doesn't want the OS to get in the way (the bridge->is_hotplug_bridge
&& !pciehp_is_native(bridge) check in pci_bridge_d3_possible()).

The DMI check at the end of pci_bridge_d3_possible() is really
something to the effect of "there is no particular reason to prevent
this bridge from going into D3, but try to avoid platforms where it
may not work".

Basically, as far as I'm concerned, this check can be changed into
something like

if (!IS_ENABLED(CONFIG_X86) || dmi_get_bios_year() >= 2015)
        return true;

which also requires updating the comment above it accordingly.

This would have been better than the check added by the $subject patch IMV.