| Message ID | 20220210224329.2793-4-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Overhaul is_thunderbolt | expand |
Hi Mario, On Thu, Feb 10, 2022 at 04:43:23PM -0600, Mario Limonciello wrote: > The `is_thunderbolt` attribute is currently a dumping ground for a > variety of things. > > Instead use the driver core removable attribute to indicate the > detail a device is attached to a thunderbolt or USB4 chain. > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/pci/pci.c | 2 +- > drivers/pci/probe.c | 20 +++++++------------- > drivers/platform/x86/apple-gmux.c | 2 +- > include/linux/pci.h | 5 ++--- > 4 files changed, 11 insertions(+), 18 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 9ecce435fb3f..1264984d5e6d 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -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(&bridge->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. I wonder if we could instead remove the check completely here and rely on the below: if (platform_pci_bridge_d3(bridge)) return true; and that would then look like: static inline bool platform_pci_bridge_d3(struct pci_dev *dev) { if (pci_use_mid_pm()) return false; if (acpi_pci_bridge_d3(dev)) return true; if (device_property_read_bool(&dev->dev, "HotPlugSupportInD3")) return true; return false; } and then make a quirk in quirks.c that adds the software node property for the Apple systems? Or something along those lines. @Lukas, what do you think?
[Public] > -----Original Message----- > From: Mika Westerberg <mika.westerberg@linux.intel.com> > Sent: Friday, February 11, 2022 04:24 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: Bjorn Helgaas <bhelgaas@google.com>; Andreas Noever > <andreas.noever@gmail.com>; open list:PCI SUBSYSTEM <linux- > pci@vger.kernel.org>; open list:THUNDERBOLT DRIVER <linux- > usb@vger.kernel.org>; open list:RADEON and AMDGPU DRM DRIVERS <amd- > gfx@lists.freedesktop.org>; open list:DRM DRIVERS <dri- > devel@lists.freedesktop.org>; open list:DRM DRIVER FOR NVIDIA > GEFORCE/QUADRO GPUS <nouveau@lists.freedesktop.org>; open list:X86 > PLATFORM DRIVERS <platform-driver-x86@vger.kernel.org>; Michael Jamet > <michael.jamet@intel.com>; Yehezkel Bernat <YehezkelShB@gmail.com>; > Lukas Wunner <lukas@wunner.de>; Deucher, Alexander > <Alexander.Deucher@amd.com> > Subject: Re: [PATCH v2 3/9] PCI: drop `is_thunderbolt` attribute > > Hi Mario, > > On Thu, Feb 10, 2022 at 04:43:23PM -0600, Mario Limonciello wrote: > > The `is_thunderbolt` attribute is currently a dumping ground for a > > variety of things. > > > > Instead use the driver core removable attribute to indicate the > > detail a device is attached to a thunderbolt or USB4 chain. > > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > --- > > drivers/pci/pci.c | 2 +- > > drivers/pci/probe.c | 20 +++++++------------- > > drivers/platform/x86/apple-gmux.c | 2 +- > > include/linux/pci.h | 5 ++--- > > 4 files changed, 11 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index 9ecce435fb3f..1264984d5e6d 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -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(&bridge->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. > > I wonder if we could instead remove the check completely here and rely > on the below: > > if (platform_pci_bridge_d3(bridge)) > return true; > > and that would then look like: > > static inline bool platform_pci_bridge_d3(struct pci_dev *dev) > { > if (pci_use_mid_pm()) > return false; > > if (acpi_pci_bridge_d3(dev)) > return true; > > if (device_property_read_bool(&dev->dev, "HotPlugSupportInD3")) > return true; > > return false; > } > > and then make a quirk in quirks.c that adds the software node property > for the Apple systems? Or something along those lines. > > @Lukas, what do you think? I took a stab at doing this for V3, but I'm unsure whether ALL of the TBT controllers in pci_ids.h have been used in Apple laptops, so it might be a bit wasteful of a quirk list. If there is a known list somewhere that is shorter than that, it may be possible to pare down. Lukas, if you can please look closely at patch 3 of v3.
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(&bridge->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
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(&bridge->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.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 9ecce435fb3f..1264984d5e6d 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -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(&bridge->dev)) return true; /* Platform might know better if the bridge supports D3 */ diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 17a969942d37..e41656cdd8f0 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1577,16 +1577,6 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev) pdev->is_hotplug_bridge = 1; } -static void set_pcie_thunderbolt(struct pci_dev *dev) -{ - u16 vsec; - - /* Is the device part of a Thunderbolt controller? */ - vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT); - if (vsec) - dev->is_thunderbolt = 1; -} - static void set_pcie_untrusted(struct pci_dev *dev) { struct pci_dev *parent; @@ -1603,6 +1593,10 @@ static void set_pcie_untrusted(struct pci_dev *dev) 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); /* * We (only) consider everything downstream from an external_facing @@ -1615,8 +1609,9 @@ static void pci_set_removable(struct pci_dev *dev) * accessible to user / may not be removed by end user, and thus not * exposed as "removable" to userspace. */ - if (parent && - (parent->external_facing || dev_is_removable(&parent->dev))) + if (vsec || + (parent && + (parent->external_facing || dev_is_removable(&parent->dev)))) dev_set_removable(&dev->dev, DEVICE_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); 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)); } static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) diff --git a/include/linux/pci.h b/include/linux/pci.h index 1e5b769e42fc..d9719eb14654 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -442,7 +442,6 @@ struct pci_dev { unsigned int is_virtfn:1; unsigned int is_hotplug_bridge:1; unsigned int shpc_managed:1; /* SHPC owned by shpchp */ - unsigned int is_thunderbolt:1; /* Thunderbolt controller */ unsigned int no_cmd_complete:1; /* Lies about command completed events */ /* @@ -2447,11 +2446,11 @@ static inline bool pci_is_thunderbolt_attached(struct pci_dev *pdev) { struct pci_dev *parent = pdev; - if (pdev->is_thunderbolt) + if (dev_is_removable(&pdev->dev)) return true; while ((parent = pci_upstream_bridge(parent))) - if (parent->is_thunderbolt) + if (dev_is_removable(&parent->dev)) return true; return false;
The `is_thunderbolt` attribute is currently a dumping ground for a variety of things. Instead use the driver core removable attribute to indicate the detail a device is attached to a thunderbolt or USB4 chain. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/pci/pci.c | 2 +- drivers/pci/probe.c | 20 +++++++------------- drivers/platform/x86/apple-gmux.c | 2 +- include/linux/pci.h | 5 ++--- 4 files changed, 11 insertions(+), 18 deletions(-)