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

[Mario Limonciello <mario.limonciello@amd.com>]

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