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

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

[Brian Norris]

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.

 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

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

[Manivannan Sadhasivam]

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
> 

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

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

[Brian Norris]

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

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

[Bjorn Helgaas]

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
> 

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

[Brian Norris]

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

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

[Manivannan Sadhasivam]

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

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

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

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

[Manivannan Sadhasivam]

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

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

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

[Brian Norris]

Hi Rafael, Manivannan,

On Thu, Mar 13, 2025 at 10:51:13AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 05, 2025 at 02:41:26PM +0100, Rafael J. Wysocki wrote:
> > 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.

Yes, thanks Rafael. This adds a bit more than the guesswork I've done so
far.

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

I suppose if this harms any non-x86 BIOS systems, we can just add to
bridge_d3_blacklist[]. This works for me too.

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

Sure, will send shortly.

Brian