On Tue, Dec 12, 2023 at 8:41 PM Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
>
> On 12/12/2023 13:25, Rafael J. Wysocki wrote:
> > On Mon, Dec 4, 2023 at 7:07 AM Mario Limonciello
> > <mario.limonciello@xxxxxxx> 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@xxxxxxx>
> >> ---
> >> 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
> >>
> >>
>
