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@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". > 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 -- மணிவண்ணன் சதாசிவம்
