Nouveau - Feb 2022 - [PATCH v3 04/12] PCI: Drop the `is_thunderbolt` attribute from PCI core

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 at amd.com>
---
 drivers/pci/probe.c               | 20 +++++++-------------
 drivers/platform/x86/apple-gmux.c |  2 +-
 include/linux/pci.h               |  5 ++---
 3 files changed, 10 insertions(+), 17 deletions(-)

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;
-- 
2.34.1

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