[Public]
> -----Original Message-----
> From: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> Sent: Thursday, March 24, 2022 14:10
> To: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Cc: Rafael J. Wysocki <rafael@xxxxxxxxxx>; Limonciello, Mario
> <Mario.Limonciello@xxxxxxx>; Bjorn Helgaas <bhelgaas@xxxxxxxxxx>;
> open list:PCI SUBSYSTEM <linux-pci@xxxxxxxxxxxxxxx>; Linux PM <linux-
> pm@xxxxxxxxxxxxxxx>; Mehta, Sanju <Sanju.Mehta@xxxxxxx>; Mika
> Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v4] PCI / ACPI: Assume `HotPlugSupportInD3` only if
> device can wake from D3
>
> On Thu, Mar 24, 2022 at 7:52 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> >
> > On Thu, Mar 24, 2022 at 07:31:56PM +0100, Rafael J. Wysocki wrote:
> > > On Thu, Mar 24, 2022 at 6:15 PM Limonciello, Mario
> > > <Mario.Limonciello@xxxxxxx> wrote:
> > > > > -----Original Message-----
> > > > > From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > > > > Sent: Thursday, March 24, 2022 11:35
> > > > > To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> > > > > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; open list:PCI SUBSYSTEM
> <linux-
> > > > > pci@xxxxxxxxxxxxxxx>; Linux PM <linux-pm@xxxxxxxxxxxxxxx>;
> Mehta, Sanju
> > > > > <Sanju.Mehta@xxxxxxx>; Rafael J. Wysocki <rafael@xxxxxxxxxx>;
> Mika
> > > > > Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > > > > Subject: Re: [PATCH v4] PCI / ACPI: Assume `HotPlugSupportInD3`
> only if
> > > > > device can wake from D3
> > > > >
> > > > > [+cc Mika, "HotPlugSupportInD3" scope question below]
> > > >
> > > > FYI - Mika had approved some earlier versions of this, so I expect
> conceptual
> > > > Alignment at least with the latest one.
> > > >
> > > > <snip>
> > > >
> > > > > > > Can we at least list some common scenarios? E.g., it affects
> > > > > > > kernels after commit X, or it affects machines with CPUs newer
> > > > > > > than Y, or it affects a certain kind of tunneling, etc? Distros
> > > > > > > need this information so they can figure whether and how far to
> > > > > > > backport things like this.
> > > > > >
> > > > > > It's going to affect any retail machine with the SOC we refer to in
> > > > > > the kernel as "Yellow Carp". This is one of the first non-Intel
> > > > > > USB4 hosts and will be using the USB4 SW CM in the kernel.
> > > > > >
> > > > > > Without this change, effectively PCIe tunneling will not work when
> > > > > > any downstream PCIe device is hotplugged. In the right
> > > > > > circumstances it might work if it's coldbooted (if the paths
> > > > > > selected by the pre-boot firmware connection manager are
> identical
> > > > > > to that selected by SW CM).
> > > > >
> > > > > Thanks a lot for this context! As far as I can tell from grubbing
> > > > > through the git history, there are no PCI, USB4, or Thunderbolt
> > > > > changes related to Yellow Carp, so I assume this has to do with Yellow
> > > > > Carp firmware doing things differently than previous platforms.
> > > >
> > > > There have been a variety of Thunderbolt/USB4 changes as a result of
> > > > Yellow Carp development and findings, but they have not been quirks;
> > > > they have been done as generic changes that still make sense for all
> > > > USB4 devices.
> > > >
> > > > Sanju (on CC) has submitted a majority of these, so if you want to see
> > > > a sense of what these are you can look for his commits in
> drivers/thunderbolt.
> > > >
> > > > > Previously, if a Root Port implemented the HotPlugSupportInD3
> > > > > property, we assumed that the Root Port and any downstream
> bridges
> > > > > could handle hot-plug events while in D3hot.
> > > > >
> > > > > I guess the difference here is that on Yellow Carp firmware, even if
> > > > > there is a HotPlugSupportInD3 property on the Root Port, the Root
> Port
> > > > > cannot handle hot-plug events in D3hot UNLESS there is also an _S0W
> > > > > method AND that _S0W says the Root Port can wakeup from D3hot or
> > > > > D3cold, right?
> > > >
> > > > Yes, correct!
> > > >
> > > > > I have some heartburn about this that's only partly related to this
> > > > > patch. The Microsoft doc clearly says "HotPlugSupportInD3" must be
> > > > > implemented on Root Ports and its presence tells us that the *Root
> > > > > Port* can handle hot-plug events while in D3.
> > > > >
> > > > > But acpi_pci_bridge_d3() looks up the Root Port at the top of the
> > > > > hierarchy and assume that its "HotPlugSupportInD3" applies to any
> > > > > switch ports that may be below that Root Port (added by
> 26ad34d510a8
> > > > > ("PCI / ACPI: Whitelist D3 for more PCIe hotplug ports") [1]).
> > >
> > > Not really.
> > >
> > > "HotPlugSupportInD3" applies to the root port only and the platform
> > > firmware may not know about any ports below it.
> > >
> > > However, the presence of "HotPlugSupportInD3" is used as an indicator
> > > that the entire hierarchy is "D3cold-aware", so to speak.
> > > Essentially, this boils down to the "Is the hardware modern enough?"
> > > consideration and the answer is assumed to be "yes" if the property in
> > > question is present for the root port.
> >
> > Seems weird to me since we're talking about a hot-plug Root Port and
> > anything at all could be plugged into it. We're basically saying that
> > we can assume a property of an arbitrary downstream device based on
> > something we know about the upstream device. I'm still not
> > comfortable with that.
> >
> > At a minimum we should add a comment about this assumption. The
> > existing "... we know the hierarchy behind it supports D3 just fine"
> > seems a little too strong.
> >
> > > But if "HotPlugSupportInD3" is not consistent with the other pieces of
> > > information regarding the root port available from the firmware (_PRW
> > > and _S0W in this particular case), the presence of it is questionable
> > > in the first place, so IMO the approach here makes sense.
> >
> > This part seems reasonable to me, as long as we have good confidence
> > that requiring "HotPlugSupportInD3" + _PRW + _S0W where we used to
> > require only "HotPlugSupportInD3" is unlikely to break anything.
> >
> > I can't judge that, but I assume you know that we don't use the
> > acpi_pci_bridge_d3() result unless _PRW and _S0W exist, so I'll take
> > your word for it :)
>
> Actually, that is an extremely good point I didn't think about.
>
> Thinking about it now, one thing is missing: a check if _S0W is
> present, because the lack of it means "any power state should be
> fine".
>
> With this check in place we would only avoid taking
> "HotPlugSupportInD3" into account if it were not consistent with the
> other settings and if that didn't work, we would end up in the quirks
> territory this way or another.
In that case something like this instead?
+ if (!adev->wakeup.flags.valid)
+ return false;
+
+ if (ACPI_SUCCESS(acpi_evaluate_integer(adev->handle, "_S0W", NULL, &ret)))
+ return ret >= ACPI_STATE_D3_HOT;
+
+ return true;
