| Message ID | 20231203041046.38655-3-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for drivers to decide bridge D3 policy | expand |
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 > >
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 >> >>
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 --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; /*
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(-)