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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Wed, Mar 05, 2025 at 02:41:26PM +0100, Rafael J. Wysocki wrote:
> On Fri, Feb 28, 2025 at 7:40 PM Brian Norris <briannorris@chromium.org> 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@linaro.org>
> > > >
> > > > 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".
> 

Thanks for sharing the background. This could go in the commit message IMO.

> 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.

Looks good to me. Brian, could you please respin incorporating the comments?

- Mani

-- 
மணிவண்ணன் சதாசிவம்