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.
