Re: [Nouveau] [PATCH v3 05/12] PCI: Detect root port of internal USB4 devices by `usb4-host-interface`
Hi Bjorn,
On Fri, Feb 11, 2022 at 03:45:46PM -0600, Bjorn Helgaas wrote:
> On Fri, Feb 11, 2022 at 01:32:43PM -0600, Mario Limonciello wrote:
> > The root port used for PCIe tunneling should be marked as removable to
> > ensure that the entire chain is marked removable.
> >
> > This can be done by looking for the device property specified in
> > the ACPI tables `usb4-host-interface`.
> >
> > Suggested-by: Mika Westerberg
> > Link:
> > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#mapping-native-protocols-pcie-displayport-tunneled-through-usb4-to-usb4-host-routers
> > Signed-off-by: Mario Limonciello
> > ---
> > drivers/pci/pci-acpi.c | 10 ++
> > drivers/pci/pci.h | 5 +
> > drivers/pci/probe.c| 1 +
> > 3 files changed, 16 insertions(+)
> >
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index a42dbf448860..6368e5633b1b 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -1391,6 +1391,16 @@ void pci_acpi_cleanup(struct device *dev, struct
> > acpi_device *adev)
> > }
> > }
> >
> > +bool pci_acpi_is_usb4(struct pci_dev *dev)
> > +{
> > + struct acpi_device *adev = ACPI_COMPANION(>dev);
> > +
> > + if (!adev)
> > + return false;
> > + return fwnode_property_present(acpi_fwnode_handle(adev),
> > + "usb4-host-interface");
>
> Maybe it's obvious to everybody but me that "USB4" means this device
> is removable. The Microsoft reference above doesn't say anything
> about removability.
>
> My expectation is that "USB" (like "PCI" and "PCIe") tells me
> something about how a device is electrically connected and how
> software can operate it. It doesn't really tell me anything about
> whether those electrical connections are permanent, made through an
> internal slot, or made through an external connector and cable.
It is used to identify "tunneled" ports (whether PCIe, USB 3.x or
DisplayPort). Tunnels are created by software (in Linux it is the
Thunderbolt driver) and are dynamic in nature. The USB4 links go over
USB Type-C cable which also is something user can plug/unplug freely.
I would say it is reasonable expectation that anything behind these
ports can be assumed as "removable".
Re: [Nouveau] [PATCH v3 04/12] PCI: Drop the `is_thunderbolt` attribute from PCI core
Hi Mario, On Sun, Feb 13, 2022 at 11:26:56AM -0600, Limonciello, Mario wrote: > On 2/13/2022 02:20, Lukas Wunner wrote: > > On Fri, Feb 11, 2022 at 01:32:42PM -0600, Mario Limonciello wrote: > > > The `is_thunderbolt` attribute is currently a dumping ground for a > > > variety of things. > > > > It's not as arbitrary as it may seem. Quite a bit of thought went into > > the current design. > > > > > > > Instead use the driver core removable attribute to indicate the > > > detail a device is attached to a thunderbolt or USB4 chain. > > > > You're missing the point that "is_thunderbolt" is set on the *controller* > > (i.e. its upstream and downstream ports). > > > > The controller itself is *not* removable if it's the host controller. > > > > However a device can be assumed to be removable if it has an ancestor > > which has the "is_thunderbolt" flag set. > > > > Ah right... I wonder if really what this series should be about then is > setting up the the PCIe endpoints for PCIe tunneling and XHCI tunneling to > be marked as "external" instead then. It would mean that existing code will > apply the removable attribute to everything downstream (and presumably at > least some of those drivers it will continue to make sense to drop > "pcie_is_thunderbolt_attached" and instead check dev_is_removable. Yes, I think this is the right thing to do. Anything connected over PCIe/USB 3.x tunnel is pretty much "removable" whereas the host controllers may or may not. Typically they are not.
Re: [Nouveau] [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk
Hi Lukas, On Sun, Feb 13, 2022 at 10:19:20AM +0100, Lukas Wunner wrote: > On Fri, Feb 11, 2022 at 01:32:41PM -0600, Mario Limonciello wrote: > > `pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt > > controller to indicate that D3 is possible. As this is used solely > > for older Apple systems, move it into a quirk that enumerates across > > all Intel TBT controllers. > > I'm not so sure if it is only needed on Apple systems. > > > > @@ -2954,10 +2960,6 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge) > > if (pci_bridge_d3_force) > > return true; > > > > - /* Even the oldest 2010 Thunderbolt controller supports D3. */ > > - if (bridge->is_thunderbolt) > > - return true; > > - > > /* Platform might know better if the bridge supports D3 */ > > if (platform_pci_bridge_d3(bridge)) > > return true; > > The fact that Thunderbolt PCIe ports support D3 is a property of those > devices. It's not a property of the platform or a quirk of a particular > vendor. It is in fact platform specific. For instance the non-Apple systems without "HotPlugSupportInD3" property have not been wired to support entering/exiting D3 runtime so putting these ports into D3 may actually lead into problems as we are in non-validated territory. > Hence in my view the current location of the check (pci_bridge_d3_possible()) > makes sense wheras the location you're moving it to does not. > > > > +/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers, > > but don't specify > > + * it in the ACPI tables > > + */ > > Apple started shipping Thunderbolt in 2011. > Intel brought the first chips to market in 2010. > > The date is meaningful at the code's current location in > pci_bridge_d3_possible() because a few lines further down > there's a 2015 BIOS cut-off date. > > Microsoft came up with an ACPI property that BIOS vendors may set > so that Windows knows it may put a Thunderbolt controller into D3cold. > I'm not even sure if that property was ever officially adopted by the > ACPI spec or if it's just a Microsoft-defined "standard". AFAIK It was invented by Intel Windows folks but Microsoft "documented" it. We use the same property in ChromeOS (and therefore Linux) too so it can be thought of as "de facto" way of declaring such port.
Re: [Nouveau] [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk
Hi,
On Fri, Feb 11, 2022 at 04:06:20PM -0600, Limonciello, Mario wrote:
> On 2/11/2022 15:35, Bjorn Helgaas wrote:
> > On Fri, Feb 11, 2022 at 01:32:41PM -0600, Mario Limonciello wrote:
> > > `pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt
> > > controller to indicate that D3 is possible. As this is used solely
> > > for older Apple systems, move it into a quirk that enumerates across
> > > all Intel TBT controllers.
> > >
> > > Suggested-by: Mika Westerberg
> > > Signed-off-by: Mario Limonciello
> > > ---
> > > drivers/pci/pci.c| 12 +-
> > > drivers/pci/quirks.c | 53
> > > 2 files changed, 60 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 9ecce435fb3f..5002e214c9a6 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -1064,7 +1064,13 @@ static inline bool platform_pci_bridge_d3(struct
> > > pci_dev *dev)
> > > if (pci_use_mid_pm())
> > > return false;
> > > - return acpi_pci_bridge_d3(dev);
> > > + if (acpi_pci_bridge_d3(dev))
> > > + return true;
> > > +
> > > + if (device_property_read_bool(>dev, "HotPlugSupportInD3"))
> > > + return true;
> >
> > Why do we need this? acpi_pci_bridge_d3() already looks for
> > "HotPlugSupportInD3".
>
> The Apple machines don't have ACPI companion devices that specify this
> property.
>
> I guess this probes a different question; can `device_property_read_bool` be
> used in `acpi_pci_bridge_d3` instead of:
>
> if (acpi_dev_get_property(adev, "HotPlugSupportInD3",
> ACPI_TYPE_INTEGER, ) < 0)
> return false;
>
> return obj->integer.value == 1;
>
> If so, then yeah this can probably be simplified.
Unfortunately the code in acpi_pci_bridge_d3() expects the device to
have an ACPI_COMPANION() which may not be the case with software nodes.
> >
> > > + return false;
> > > }
> > > /**
> > > @@ -2954,10 +2960,6 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> > > if (pci_bridge_d3_force)
> > > return true;
> > > - /* Even the oldest 2010 Thunderbolt controller supports D3. */
> > > - if (bridge->is_thunderbolt)
> > > - return true;
> > > -
> > > /* Platform might know better if the bridge supports D3
> > > */
> > > if (platform_pci_bridge_d3(bridge))
> > > return true;
> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > index 6d3c88edde00..aaf098ca7d54 100644
> > > --- a/drivers/pci/quirks.c
> > > +++ b/drivers/pci/quirks.c
> > > @@ -3756,6 +3756,59 @@ DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL,
> > > quirk_apple_poweroff_thunderbolt);
> > > #endif
> > > +/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers,
> > > but don't specify
> > > + * it in the ACPI tables
> >
> > Wrap to fit in 80 columns like the rest of the file. Also use the:
> >
> >/*
> > * comment ...
> > */
> >
> > style if it's more than one line.
> >
> > I don't think "as old as 2010" is helpful here -- I assume 2010 is
> > there because there *were* no Thunderbolt controllers before 2010, but
> > the code doesn't check any dates, so we basically assume all Apple
> > machines of any age with the listed controllers can do this.
>
> The old comment was saying that, which is where I got it from. Yeah, I'll
> update it.
>
> >
> > > + */
> > > +static void quirk_apple_d3_thunderbolt(struct pci_dev *dev)
> > > +{
> > > + struct property_entry properties[] = {
> > > + PROPERTY_ENTRY_BOOL("HotPlugSupportInD3"),
> > > + {},
> > > + };
> > > +
> > > + if (!x86_apple_machine)
> > > + return;
> >
> > The current code doesn't check x86_apple_machine, so this needs some
> > justification. How do I know this works the same as before?
>
> Mika and Lucas were saying the only reason for this codepath was Apple
> machines in the first place, which is where this idea came from.
Yes, that's the reason.
Nobody else is going to need this except Apple machines with Intel
Thunderbolt controller.
> Something specifically relevant is that the Apple machines use a SW
> connection manager, whereas everyone else up until USB4 devices use a
> firmware based connection manager with varying behaviors on generation
> (ICM).
Yup.
> > > +
> > > + if (device_create_managed_software_node(>dev, properties, NULL))
> > > + pci_warn(dev, "could not add HotPlugSupportInD3 property");
> > > +}
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> > > PCI_DEVICE_ID_INTEL_LIGHT_RIDGE,
> > > + quirk_apple_d3_thunderbolt);
> >
> > The current code assumes *all* Thunderbolt controllers support D3, so
> > it would assume a controller released next year would support D3, but
> >
Re: [Nouveau] [PATCH v2 3/9] PCI: drop `is_thunderbolt` attribute
Hi, On Sun, Feb 13, 2022 at 09:39:28AM +0100, Lukas Wunner wrote: > On Fri, Feb 11, 2022 at 12:23:51PM +0200, Mika Westerberg wrote: > > On Thu, Feb 10, 2022 at 04:43:23PM -0600, Mario Limonciello wrote: > > > @@ -2955,7 +2955,7 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge) > > > return true; > > > > > > /* Even the oldest 2010 Thunderbolt controller supports D3. */ > > > - if (bridge->is_thunderbolt) > > > + if (dev_is_removable(>dev)) > > > > For this, I'm not entirely sure this is what we want. The purpose of > > this check is to enable port power management of Apple systems with > > Intel Thunderbolt controller and therefore checking for "removable" here > > is kind of misleading IMHO. > [...] > > and then make a quirk in quirks.c that adds the software node property > > for the Apple systems? Or something along those lines. > > Honestly, that feels wrong to me. > > There are non-Apple products with Thunderbolt controllers, > e.g. Supermicro X10SAT was a Xeon board with Redwood Ridge > which was introduced in 2013. This was way before Microsoft > came up with the HotPlugSupportInD3 property. It was also way > before the 2015 BIOS cut-off date that we use to disable > power management on older boards. > > Still, we currently whitelist the Thunderbolt ports on that > board for D3 because we know it works. What if products like > this one use their own power management scheme and we'd cause > a power regression if we needlessly disable D3 for them now? All the non-Apple Thunderbolt products before "HotPlugSupportInD3" use ACPI "assisted" hotplug which means all the PM is done in the BIOS. Essentially it means the controller is only present if there is anything connected and in that case it is always in D0. Unplugging the device makes the controller to be hot-removed (ACPI hotplug) too and that's the only way early Thunderbolt used to save energy.
Re: [Nouveau] [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk
On Sun, Feb 13, 2022 at 10:19:20AM +0100, Lukas Wunner wrote: > Apple had been using its own scheme to put Thunderbolt controllers > into D3cold when nothing is plugged in, about a decade before Microsoft > defined the ACPI property. I meant to say "half a decade", sorry.
Re: [Nouveau] [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk
On Fri, Feb 11, 2022 at 01:32:41PM -0600, Mario Limonciello wrote: > `pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt > controller to indicate that D3 is possible. As this is used solely > for older Apple systems, move it into a quirk that enumerates across > all Intel TBT controllers. I'm not so sure if it is only needed on Apple systems. > @@ -2954,10 +2960,6 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge) > if (pci_bridge_d3_force) > return true; > > - /* Even the oldest 2010 Thunderbolt controller supports D3. */ > - if (bridge->is_thunderbolt) > - return true; > - > /* Platform might know better if the bridge supports D3 */ > if (platform_pci_bridge_d3(bridge)) > return true; The fact that Thunderbolt PCIe ports support D3 is a property of those devices. It's not a property of the platform or a quirk of a particular vendor. Hence in my view the current location of the check (pci_bridge_d3_possible()) makes sense wheras the location you're moving it to does not. > +/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers, but > don't specify > + * it in the ACPI tables > + */ Apple started shipping Thunderbolt in 2011. Intel brought the first chips to market in 2010. The date is meaningful at the code's current location in pci_bridge_d3_possible() because a few lines further down there's a 2015 BIOS cut-off date. Microsoft came up with an ACPI property that BIOS vendors may set so that Windows knows it may put a Thunderbolt controller into D3cold. I'm not even sure if that property was ever officially adopted by the ACPI spec or if it's just a Microsoft-defined "standard". Apple had been using its own scheme to put Thunderbolt controllers into D3cold when nothing is plugged in, about a decade before Microsoft defined the ACPI property. I'm not sure if other vendors came up with their own schemes to power-manage Thunderbolt. We may regress those with the present patch. Thanks, Lukas
Re: [Nouveau] [PATCH v2 3/9] PCI: drop `is_thunderbolt` attribute
On Fri, Feb 11, 2022 at 12:23:51PM +0200, Mika Westerberg wrote: > On Thu, Feb 10, 2022 at 04:43:23PM -0600, Mario Limonciello wrote: > > @@ -2955,7 +2955,7 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge) > > return true; > > > > /* Even the oldest 2010 Thunderbolt controller supports D3. */ > > - if (bridge->is_thunderbolt) > > + if (dev_is_removable(>dev)) > > For this, I'm not entirely sure this is what we want. The purpose of > this check is to enable port power management of Apple systems with > Intel Thunderbolt controller and therefore checking for "removable" here > is kind of misleading IMHO. [...] > and then make a quirk in quirks.c that adds the software node property > for the Apple systems? Or something along those lines. Honestly, that feels wrong to me. There are non-Apple products with Thunderbolt controllers, e.g. Supermicro X10SAT was a Xeon board with Redwood Ridge which was introduced in 2013. This was way before Microsoft came up with the HotPlugSupportInD3 property. It was also way before the 2015 BIOS cut-off date that we use to disable power management on older boards. Still, we currently whitelist the Thunderbolt ports on that board for D3 because we know it works. What if products like this one use their own power management scheme and we'd cause a power regression if we needlessly disable D3 for them now? Thanks, Lukas
Re: [Nouveau] [PATCH v3 04/12] PCI: Drop the `is_thunderbolt` attribute from PCI core
On Fri, Feb 11, 2022 at 01:32:42PM -0600, Mario Limonciello wrote:
> The `is_thunderbolt` attribute is currently a dumping ground for a
> variety of things.
It's not as arbitrary as it may seem. Quite a bit of thought went into
the current design.
> Instead use the driver core removable attribute to indicate the
> detail a device is attached to a thunderbolt or USB4 chain.
You're missing the point that "is_thunderbolt" is set on the *controller*
(i.e. its upstream and downstream ports).
The controller itself is *not* removable if it's the host controller.
However a device can be assumed to be removable if it has an ancestor
which has the "is_thunderbolt" flag set.
> static void pci_set_removable(struct pci_dev *dev)
> {
> struct pci_dev *parent = pci_upstream_bridge(dev);
> + u16 vsec;
> +
> + /* Is the device a Thunderbolt controller? */
> + vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL,
> PCI_VSEC_ID_INTEL_TBT);
This doesn't make any sense because the host controller is not
removable.
> @@ -1860,7 +1855,6 @@ int pci_setup_device(struct pci_dev *dev)
> dev->cfg_size = pci_cfg_space_size(dev);
>
> /* Need to have dev->cfg_size ready */
> - set_pcie_thunderbolt(dev);
>
> set_pcie_untrusted(dev);
Either drop the blank line or drop the code comment if set_pcie_untrusted()
doesn't need dev->cfg_size.
> diff --git a/drivers/platform/x86/apple-gmux.c
> b/drivers/platform/x86/apple-gmux.c
> index 57553f9b4d1d..04232fbc7d56 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -596,7 +596,7 @@ static int gmux_resume(struct device *dev)
>
> static int is_thunderbolt(struct device *dev, void *data)
> {
> - return to_pci_dev(dev)->is_thunderbolt;
> + return pci_is_thunderbolt_attached(to_pci_dev(dev));
> }
No, the gmux driver changes its behavior if a Thunderbolt host
controller is present. Not if there's a Thunderbolt-attached
device present.
Thanks,
Lukas
