On Fri, Mar 25, 2022 at 12:13 AM Limonciello, Mario
<Mario.Limonciello@xxxxxxx> wrote:
>
> [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?
Works for me.
> + 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;
