Re: [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: [PATCH v3 04/12] PCI: Drop the `is_thunderbolt` attribute from PCI core
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.
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.
Present in the system? Like not necessarily part of the topology?
Thanks,
Lukas
Re: [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
[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
---
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(>dev)))
+ if (vsec ||
+ (parent &&
+ (parent->external_facing || dev_is_removable(>dev
dev_set_removable(>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 intis_virtfn:1;
unsigned intis_hotplug_bridge:1;
unsigned intshpc_managed:1; /* SHPC owned by shpchp */
- unsigned intis_thunderbolt:1; /* Thunderbolt controller */
unsigned intno_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(>dev))
return true;
while ((parent = pci_upstream_bridge(parent)))
- if (parent->is_thunderbolt)
+ if (dev_is_removable(>dev))
return true;
return false;
--
2.34.1
