diff mbox series

[v2,2/4] PCI: Refresh root ports in pci_bridge_d3_update()

Message ID 20231203041046.38655-3-mario.limonciello@amd.com
State New
Headers show
Series Add support for drivers to decide bridge D3 policy | expand

Commit Message

Mario Limonciello Dec. 3, 2023, 4:10 a.m. UTC
If pci_d3cold_enable() or pci_d3cold_disable() is called on a root
port it is ignored because there is no upstream bridge.

If called on a root port, use `no_d3cold` variable to decide policy
and also immediately refresh whether D3 is possible.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/pci.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Rafael J. Wysocki Dec. 12, 2023, 7:25 p.m. UTC | #1
On Mon, Dec 4, 2023 at 7:07 AM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> If pci_d3cold_enable() or pci_d3cold_disable() is called on a root
> port it is ignored because there is no upstream bridge.

The kerneldoc comment of pci_bridge_d3_update() explains what that
function is for which also covers why it does not take effect when
called on root ports.

> If called on a root port, use `no_d3cold` variable to decide policy

It is unclear that this is about pci_bridge_d3_possible() which
applies to both D3hot and D3cold, not just D3cold AFAICS.  I don't
think that no_d3cold should affect the D3hot behavior.

> and also immediately refresh whether D3 is possible.

Which isn't correct AFAICS.

> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/pci/pci.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 72505794cc72..3d4aaecda457 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3023,6 +3023,9 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>                 if (pci_bridge_d3_disable)
>                         return false;
>
> +               if (bridge->no_d3cold)
> +                       return false;
> +
>                 /*
>                  * Hotplug ports handled by firmware in System Management Mode
>                  * may not be put into D3 by the OS (Thunderbolt on non-Macs).
> @@ -3098,7 +3101,11 @@ void pci_bridge_d3_update(struct pci_dev *dev)
>         bool d3cold_ok = true;
>
>         bridge = pci_upstream_bridge(dev);
> -       if (!bridge || !pci_bridge_d3_possible(bridge))
> +       if (!bridge) {
> +               dev->bridge_d3 = pci_bridge_d3_possible(dev);
> +               return;
> +       }
> +       if (!pci_bridge_d3_possible(bridge))
>                 return;
>
>         /*
> --
> 2.34.1
>
>
Mario Limonciello Dec. 12, 2023, 7:41 p.m. UTC | #2
On 12/12/2023 13:25, Rafael J. Wysocki wrote:
> On Mon, Dec 4, 2023 at 7:07 AM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> If pci_d3cold_enable() or pci_d3cold_disable() is called on a root
>> port it is ignored because there is no upstream bridge.
> 
> The kerneldoc comment of pci_bridge_d3_update() explains what that
> function is for which also covers why it does not take effect when
> called on root ports.

I'm sorry but can you clarify the intent of your comment?

Are you suggesting we should introduce a different function/logic for 
root ports, kernel doc should be updated, or root ports should be 
special cased in that function?

> 
>> If called on a root port, use `no_d3cold` variable to decide policy
> 
> It is unclear that this is about pci_bridge_d3_possible() which
> applies to both D3hot and D3cold, not just D3cold AFAICS.  I don't
> think that no_d3cold should affect the D3hot behavior.

IMO the semantics are confusing depending upon what device you called 
pci_d3cold_disable()/pci_d3cold_enable() with as an argument.

Both devices and root ports are used by existing driver in the kernel.

If you called pci_d3cold_disable() with a device, that actually prevents 
the /bridge above it/ from going to D3hot as well (bridge_d3 is set to 
the result)

> 
>> and also immediately refresh whether D3 is possible.
> 
> Which isn't correct AFAICS.

Why?

> 
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/pci/pci.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 72505794cc72..3d4aaecda457 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3023,6 +3023,9 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>>                  if (pci_bridge_d3_disable)
>>                          return false;
>>
>> +               if (bridge->no_d3cold)
>> +                       return false;
>> +
>>                  /*
>>                   * Hotplug ports handled by firmware in System Management Mode
>>                   * may not be put into D3 by the OS (Thunderbolt on non-Macs).
>> @@ -3098,7 +3101,11 @@ void pci_bridge_d3_update(struct pci_dev *dev)
>>          bool d3cold_ok = true;
>>
>>          bridge = pci_upstream_bridge(dev);
>> -       if (!bridge || !pci_bridge_d3_possible(bridge))
>> +       if (!bridge) {
>> +               dev->bridge_d3 = pci_bridge_d3_possible(dev);
>> +               return;
>> +       }
>> +       if (!pci_bridge_d3_possible(bridge))
>>                  return;
>>
>>          /*
>> --
>> 2.34.1
>>
>>
Rafael J. Wysocki Dec. 12, 2023, 7:53 p.m. UTC | #3
On Tue, Dec 12, 2023 at 8:41 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 12/12/2023 13:25, Rafael J. Wysocki wrote:
> > On Mon, Dec 4, 2023 at 7:07 AM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> If pci_d3cold_enable() or pci_d3cold_disable() is called on a root
> >> port it is ignored because there is no upstream bridge.
> >
> > The kerneldoc comment of pci_bridge_d3_update() explains what that
> > function is for which also covers why it does not take effect when
> > called on root ports.
>
> I'm sorry but can you clarify the intent of your comment?
>
> Are you suggesting we should introduce a different function/logic for
> root ports, kernel doc should be updated, or root ports should be
> special cased in that function?

They are special-cased in that function already, because it updates an
upstream port for a change in a downstream device.

There are only 2 places really affected by no_d3cold:
pci_dev_check_d3cold() and the ACPI power state selection for PCI
devices. where the former is used for checking whether or not it is
valid to put an upstream bridge into D3hot/cold (which depends on
whether or not the downstream devices below it are allowed to use
D3cold).

The only place where no_d3cold affects root ports is the ACPI power
state selection, because the only way to program a root port into
D3cold is via ACPI.

> >
> >> If called on a root port, use `no_d3cold` variable to decide policy
> >
> > It is unclear that this is about pci_bridge_d3_possible() which
> > applies to both D3hot and D3cold, not just D3cold AFAICS.  I don't
> > think that no_d3cold should affect the D3hot behavior.
>
> IMO the semantics are confusing depending upon what device you called
> pci_d3cold_disable()/pci_d3cold_enable() with as an argument.
>
> Both devices and root ports are used by existing driver in the kernel.
>
> If you called pci_d3cold_disable() with a device, that actually prevents
> the /bridge above it/ from going to D3hot as well (bridge_d3 is set to
> the result)

Right, because (as per the PCI PM spec) putting an upstream bridge
into D3hot/cold effectively removes power from the bus segment below
it, so the devices on that bus segment go into D3cold.  If they are
not allowed to go into D3cold, the bridge needs to stay in a shallower
power state either.

> >
> >> and also immediately refresh whether D3 is possible.
> >
> > Which isn't correct AFAICS.
>
> Why?

Because it makes no_d3cold affect the ability of the given root port
to be programmed into D3hot via PMCSR.

> >
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> ---
> >>   drivers/pci/pci.c | 9 ++++++++-
> >>   1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> index 72505794cc72..3d4aaecda457 100644
> >> --- a/drivers/pci/pci.c
> >> +++ b/drivers/pci/pci.c
> >> @@ -3023,6 +3023,9 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> >>                  if (pci_bridge_d3_disable)
> >>                          return false;
> >>
> >> +               if (bridge->no_d3cold)
> >> +                       return false;
> >> +
> >>                  /*
> >>                   * Hotplug ports handled by firmware in System Management Mode
> >>                   * may not be put into D3 by the OS (Thunderbolt on non-Macs).
> >> @@ -3098,7 +3101,11 @@ void pci_bridge_d3_update(struct pci_dev *dev)
> >>          bool d3cold_ok = true;
> >>
> >>          bridge = pci_upstream_bridge(dev);
> >> -       if (!bridge || !pci_bridge_d3_possible(bridge))
> >> +       if (!bridge) {
> >> +               dev->bridge_d3 = pci_bridge_d3_possible(dev);
> >> +               return;
> >> +       }
> >> +       if (!pci_bridge_d3_possible(bridge))
> >>                  return;
> >>
> >>          /*
> >> --
> >> 2.34.1
> >>
> >>
>
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 72505794cc72..3d4aaecda457 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3023,6 +3023,9 @@  bool pci_bridge_d3_possible(struct pci_dev *bridge)
 		if (pci_bridge_d3_disable)
 			return false;
 
+		if (bridge->no_d3cold)
+			return false;
+
 		/*
 		 * Hotplug ports handled by firmware in System Management Mode
 		 * may not be put into D3 by the OS (Thunderbolt on non-Macs).
@@ -3098,7 +3101,11 @@  void pci_bridge_d3_update(struct pci_dev *dev)
 	bool d3cold_ok = true;
 
 	bridge = pci_upstream_bridge(dev);
-	if (!bridge || !pci_bridge_d3_possible(bridge))
+	if (!bridge) {
+		dev->bridge_d3 = pci_bridge_d3_possible(dev);
+		return;
+	}
+	if (!pci_bridge_d3_possible(bridge))
 		return;
 
 	/*