| Message ID | 20241126151711.v5.1.Id0a0e78ab0421b6bce51c4b0b87e6aebdfc69ec7@changeid (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v5] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms | expand |
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. Past discussions (Link > [1]) determined the restrictions around D3 should be relaxed for all > Device Tree systems. So let's allow the PCI bridges to go to D3Hot > during runtime. > > To match devm_pci_alloc_host_bridge() -> devm_of_pci_bridge_init(), we > look at the host bridge's parent when determining whether this is a > Device Tree based platform. Not all bridges have their own node, but the > parent (controller) should. > > Link: https://lore.kernel.org/linux-pci/20240227225442.GA249898@bhelgaas/ [1] > Link: https://lore.kernel.org/linux-pci/20240828210705.GA37859@bhelgaas/ [2] > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > [Brian: look at host bridge's parent, not bridge node; rewrite > description] > Signed-off-by: Brian Norris <briannorris@chromium.org> Thanks for picking it up! - Mani > --- > Based on prior work by Manivannan Sadhasivam that was part of a bigger > series that stalled: > > [PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms > https://lore.kernel.org/linux-pci/20240802-pci-bridge-d3-v5-4-2426dd9e8e27@linaro.org/ > > I'm resubmitting this single patch, since it's useful and seemingly had > agreement. I massaged it a bit to relax some restrictions on how the > Device Tree should look. > > Changes in v5: > - Pulled out of the larger series, as there were more controversial > changes in there, while this one had agreement (Link [2]). > - Rewritten with a relaxed set of rules, because the above patch > required us to modify many device trees to add bridge nodes. > > drivers/pci/pci.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index e278861684bc..5d898f5ea155 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3018,6 +3018,8 @@ static const struct dmi_system_id bridge_d3_blacklist[] = { > */ > bool pci_bridge_d3_possible(struct pci_dev *bridge) > { > + struct pci_host_bridge *host_bridge; > + > if (!pci_is_pcie(bridge)) > return false; > > @@ -3038,6 +3040,15 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge) > if (pci_bridge_d3_force) > return true; > > + /* > + * Allow D3 for all Device Tree based systems. We assume a host > + * bridge's parent will have a device node, even if this bridge > + * may not have its own. > + */ > + host_bridge = pci_find_host_bridge(bridge->bus); > + if (dev_of_node(host_bridge->dev.parent)) > + return true; > + > /* Even the oldest 2010 Thunderbolt controller supports D3. */ > if (bridge->is_thunderbolt) > return true; > -- > 2.47.0.338 >
Hi Bjorn, 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. Past discussions (Link > [1]) determined the restrictions around D3 should be relaxed for all > Device Tree systems. So let's allow the PCI bridges to go to D3Hot > during runtime. > > To match devm_pci_alloc_host_bridge() -> devm_of_pci_bridge_init(), we > look at the host bridge's parent when determining whether this is a > Device Tree based platform. Not all bridges have their own node, but the > parent (controller) should. > > Link: https://lore.kernel.org/linux-pci/20240227225442.GA249898@bhelgaas/ [1] > Link: https://lore.kernel.org/linux-pci/20240828210705.GA37859@bhelgaas/ [2] > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > [Brian: look at host bridge's parent, not bridge node; rewrite > description] > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > Based on prior work by Manivannan Sadhasivam that was part of a bigger > series that stalled: > > [PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms > https://lore.kernel.org/linux-pci/20240802-pci-bridge-d3-v5-4-2426dd9e8e27@linaro.org/ > > I'm resubmitting this single patch, since it's useful and seemingly had > agreement. I massaged it a bit to relax some restrictions on how the > Device Tree should look. > > Changes in v5: > - Pulled out of the larger series, as there were more controversial > changes in there, while this one had agreement (Link [2]). > - Rewritten with a relaxed set of rules, because the above patch > required us to modify many device trees to add bridge nodes. I'm wondering if you have any thoughts on this. Manivannan seemed happy with this in his reply. I'd like to see this land in mainline, so I can avoid the hacks that everyone seems to be picking up (such as adding "pcie_port_pm=force" to their command lines). (While I'm at it ... apologies for the poor versioning. The subject here should probably be "v6", since I'm clearly quoting above that the prior version from Manivannan was v5.) Thanks, Brian
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. 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." > Past discussions (Link [1]) determined the restrictions around D3 > should be relaxed for all Device Tree systems. This is far too generic a statement for me to sign up to, especially since "all Device Tree systems" doesn't say anything at all about how any particular hardware works or what behavior we're relying on. We need to say something about what D3hot means (i.e., only message and type 0 config requests accepted) and that we know anything below the bridge is inaccessible in D3hot and why that's OK. E.g., maybe we only care about wakeup requests and we know those still work with the bridge in D3hot because XYZ. > So let's allow the PCI bridges to go to D3Hot during runtime. > > To match devm_pci_alloc_host_bridge() -> devm_of_pci_bridge_init(), we > look at the host bridge's parent when determining whether this is a > Device Tree based platform. Not all bridges have their own node, but the > parent (controller) should. > > Link: https://lore.kernel.org/linux-pci/20240227225442.GA249898@bhelgaas/ [1] > Link: https://lore.kernel.org/linux-pci/20240828210705.GA37859@bhelgaas/ [2] > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > [Brian: look at host bridge's parent, not bridge node; rewrite > description] > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > Based on prior work by Manivannan Sadhasivam that was part of a bigger > series that stalled: > > [PATCH v5 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms > https://lore.kernel.org/linux-pci/20240802-pci-bridge-d3-v5-4-2426dd9e8e27@linaro.org/ > > I'm resubmitting this single patch, since it's useful and seemingly had > agreement. I massaged it a bit to relax some restrictions on how the > Device Tree should look. > > Changes in v5: > - Pulled out of the larger series, as there were more controversial > changes in there, while this one had agreement (Link [2]). > - Rewritten with a relaxed set of rules, because the above patch > required us to modify many device trees to add bridge nodes. > > drivers/pci/pci.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index e278861684bc..5d898f5ea155 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3018,6 +3018,8 @@ static const struct dmi_system_id bridge_d3_blacklist[] = { > */ > bool pci_bridge_d3_possible(struct pci_dev *bridge) > { > + struct pci_host_bridge *host_bridge; > + > if (!pci_is_pcie(bridge)) > return false; > > @@ -3038,6 +3040,15 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge) > if (pci_bridge_d3_force) > return true; > > + /* > + * Allow D3 for all Device Tree based systems. We assume a host > + * bridge's parent will have a device node, even if this bridge > + * may not have its own. > + */ > + host_bridge = pci_find_host_bridge(bridge->bus); > + if (dev_of_node(host_bridge->dev.parent)) > + return true; > + > /* Even the oldest 2010 Thunderbolt controller supports D3. */ > if (bridge->is_thunderbolt) > return true; > -- > 2.47.0.338 >
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. 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. > > Past discussions (Link [1]) determined the restrictions around D3 > > should be relaxed for all Device Tree systems. > > This is far too generic a statement for me to sign up to, especially > since "all Device Tree systems" doesn't say anything at all about how > any particular hardware works or what behavior we're relying on. > > We need to say something about what D3hot means (i.e., only message > and type 0 config requests accepted) and that we know anything below > the bridge is inaccessible in D3hot and why that's OK. E.g., maybe we > only care about wakeup requests and we know those still work with the > bridge in D3hot because XYZ. The context of this function is that it applies to situations where we are considering runtime all of the PCI hierarchy beneath the port. So yes, it applies in situations where we will wake device(s) by PMCSR, or they will request wakeup via PME, WAKE#, etc. Beyond that ... I don't really know what to say. This is how the spec says things should work, and AFAICT, the only reason we don't do that is because the feature writer was being conservative and happened to assume DMI is always present. Brian
On Fri, Feb 28, 2025 at 10:39:57AM -0800, Brian Norris 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. > I would say 'Majority of the DT systems' since there are exceptions like Qcom compute platforms where developers put devicetree in an ACPI based laptop with a BIOS/DMI (for a reason). > 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. > > 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. > Right. I guess the commit tried to be less invasive so the author decided to limit it to DMI based systems. I couldn't think of any other reasons. > > > Past discussions (Link [1]) determined the restrictions around D3 > > > should be relaxed for all Device Tree systems. > > > > This is far too generic a statement for me to sign up to, especially > > since "all Device Tree systems" doesn't say anything at all about how > > any particular hardware works or what behavior we're relying on. > > > > We need to say something about what D3hot means (i.e., only message > > and type 0 config requests accepted) and that we know anything below > > the bridge is inaccessible in D3hot and why that's OK. E.g., maybe we > > only care about wakeup requests and we know those still work with the > > bridge in D3hot because XYZ. > This patch's main motive is to enable D3hot for bridges that was disabled for no good reasons, other than the one mentioned above. Maybe adding a bit more about bridge D3hot handling could be of help, but your comment sounds like you are questioning why allowing D3hot is OK? But we already enable it for newer DMI based systems. I think the question should be asked is, why PCI core decided to not allow D3hot for DT based platforms without any user reported issues. - Mani
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". 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.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index e278861684bc..5d898f5ea155 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3018,6 +3018,8 @@ static const struct dmi_system_id bridge_d3_blacklist[] = { */ bool pci_bridge_d3_possible(struct pci_dev *bridge) { + struct pci_host_bridge *host_bridge; + if (!pci_is_pcie(bridge)) return false; @@ -3038,6 +3040,15 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge) if (pci_bridge_d3_force) return true; + /* + * Allow D3 for all Device Tree based systems. We assume a host + * bridge's parent will have a device node, even if this bridge + * may not have its own. + */ + host_bridge = pci_find_host_bridge(bridge->bus); + if (dev_of_node(host_bridge->dev.parent)) + return true; + /* Even the oldest 2010 Thunderbolt controller supports D3. */ if (bridge->is_thunderbolt) return true;