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 :)
Bjorn
