Re: [PATCH v4 1/8] PCI: Recognize Thunderbolt devices

[Lukas Wunner]

On Sun, Jan 08, 2017 at 10:23:03AM +0000, Winkler, Tomas wrote:
> > We're about to allow runtime PM on Thunderbolt ports in
> > pci_bridge_d3_possible() and unblock runtime PM for Thunderbolt host
> > hotplug ports in pci_dev_check_d3cold().  In both cases we need to uniquely
> > identify if a PCI device belongs to a Thunderbolt controller.
> > 
> > We also have the need to detect presence of a Thunderbolt controller in
> > drivers/platform/x86/apple-gmux.c because dual GPU MacBook Pros cannot
> > switch external DP/HDMI ports between GPUs if they have Thunderbolt.
> > 
> > Furthermore, in multiple places in the DRM subsystem we need to detect
> > whether a GPU is on-board or attached with Thunderbolt.  As an example,
> > Thunderbolt-attached GPUs shall not be registered with vga_switcheroo.
> > 
> > Intel uses a Vendor-Specific Extended Capability (VSEC) with ID 0x1234 on
> > devices belonging to a Thunderbolt controller which allows us to recognize
> > them.
> > 
> > Detect presence of this VSEC on device probe and cache it in a newly added
> > is_thunderbolt bit in struct pci_dev which can then be queried by
> > pci_bridge_d3_possible(), pci_dev_check_d3cold(), apple-gmux and others.
> > 
> > Cc: Andreas Noever <andreas.noever@xxxxxxxxx>
> > Cc: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> > Cc: Amir Levy <amir.jer.levy@xxxxxxxxx>
> > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> > ---
> >  drivers/pci/pci.h   |  2 ++
> >  drivers/pci/probe.c | 34 ++++++++++++++++++++++++++++++++++
> >  include/linux/pci.h |  1 +
> >  3 files changed, 37 insertions(+)
> > 
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index cb17db2..45c2b81
> > 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -3,6 +3,8 @@
> > 
> >  #define PCI_FIND_CAP_TTL	48
> > 
> > +#define PCI_VSEC_ID_INTEL_TBT	0x1234	/* Thunderbolt */
> 
> Shouldn't bet this defined in pci_ids.h ?

That file is solely intended for IDs used in multiple places, as
explained in the comment at its top.  This particular ID is only used
in the PCI core, hence it's in the PCI core's private drivers/pci/pci.h.

However the line is formatted such that it can just be moved to the
global include/linux/pci_ids.h should it be needed someplace else in
the future.


> >  extern const unsigned char pcie_link_speed[];
> > 
> >  bool pcie_cap_has_lnkctl(const struct pci_dev *dev); diff --git
> > a/drivers/pci/probe.c b/drivers/pci/probe.c index e164b5c..891a8fa 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1206,6 +1206,37 @@ void set_pcie_hotplug_bridge(struct pci_dev
> > *pdev)
> >  		pdev->is_hotplug_bridge = 1;
> >  }
> > 
> > +static void set_pcie_vendor_specific(struct pci_dev *dev) {
> 
> 
> Why don't u implement this  in quirk.c ?

The is_thunderbolt bit signifies whether a given PCI device is part
of a Thunderbolt daisy chain.  (As explained in the comment in
include/linux/pci.h, see below.)  So it is set on all the PCI devices
that comprise a Thunderbolt controller, but also on all endpoint devices
that are attached via Thunderbolt.

Consequently this function needs to be executed for all PCI devices,
not just for a subset that could be identified by a vendor, device or
class ID.

In the DRM drivers nouveau, radeon and amdgpu I need to prevent calls
to vga_switcheroo_register_client() and vga_switcheroo_init_domain_pm_ops()
if the device in question is attached via Thunderbolt.  That's because
external GPUs can't drive the panel of a laptop or be powered down like an
on-board Optimus/PowerXpress GPU.  We've got a bug there right now that
manifests itself once someone attaches an eGPU to a dual GPU laptop.
With this patch I'll be able to just skip registration with vga_switcheroo
if is_thunderbolt is set on a GPU's pci_dev, thereby fixing the bug.


BTW do you have information on the contents/meaning of the VSEC that you
would be able/allowed to share?  What I've seen so far is that its length
is 0x1c bytes on older controllers (e.g. Light Ridge, Port Ridge) and its
contents are always the same, regardless if the controller is used in
host mode or endpoint mode:

500: 0b 00 01 00 34 12 c1 01|50 09 00 00 39 00 00 00
510: 00 00 00 00 00 00 00 00 00 00 00 00

However on Alpine Ridge the size of the VSEC has increased significantly
to 0xd8 bytes, even though the version stayed at 1 as before:

500: 0b 00 01 60 34 12 81 0d 50 09 b0 0c b9 06 18 08
510: 00 38 00 01 00 00 00 00 00 00 00 00 32 02 10 00
520: ef d3 00 40 00 00 00 00 f0 f0 30 c1 00 02 30 00
530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
540: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
550: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
560: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
570: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
590: 00 00 00 08 00 00 00 00 00 00 00 00 00 00 00 00
5a0: 00 00 00 00 00 00 00 00 00 00 00 00 38 24 01 00
5b0: 08 40 00 1c 00 00 01 18 04 03 00 f0 06 00 00 00
5c0: 00 00 08 40 06 00 00 01 90 00 08 1b 00 08 80 08
5d0: 80 7f 08 00 00 00 00 00

Thanks,

Lukas

> 
> > +	int vsec = 0;
> > +	u32 header;
> > +
> > +	while ((vsec = pci_find_next_ext_capability(dev, vsec,
> > +						    PCI_EXT_CAP_ID_VNDR))) {
> > +		pci_read_config_dword(dev, vsec + PCI_VNDR_HEADER,
> > &header);
> > +
> > +		/* Is the device part of a Thunderbolt controller? */
> > +		if (dev->vendor == PCI_VENDOR_ID_INTEL &&
> > +		    PCI_VNDR_HEADER_ID(header) == PCI_VSEC_ID_INTEL_TBT)
> > +			dev->is_thunderbolt = 1;
> > +	}
> > +
> > +	/*
> > +	 * Is the device attached with Thunderbolt?  Walk upwards and check
> > for
> > +	 * each encountered bridge if it's part of a Thunderbolt controller.
> > +	 * Reaching the host bridge means dev is soldered to the mainboard.
> > +	 */
> > +	if (!dev->is_thunderbolt) {
> > +		struct pci_dev *parent = dev;
> > +
> > +		while ((parent = pci_upstream_bridge(parent)))
> > +			if (parent->is_thunderbolt) {
> > +				dev->is_thunderbolt = 1;
> > +				break;
> > +			}
> > +	}
> > +}
> > +
> >  /**
> >   * pci_ext_cfg_is_aliased - is ext config space just an alias of std config?
> >   * @dev: PCI device
> > @@ -1358,6 +1389,9 @@ int pci_setup_device(struct pci_dev *dev)
> >  	/* need to have dev->class ready */
> >  	dev->cfg_size = pci_cfg_space_size(dev);
> > 
> > +	/* need to have dev->cfg_size ready */
> > +	set_pcie_vendor_specific(dev);
> > +
> >  	/* "Unknown power state" */
> >  	dev->current_state = PCI_UNKNOWN;
> > 
> > diff --git a/include/linux/pci.h b/include/linux/pci.h index e2d1a12..3c775e8
> > 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -358,6 +358,7 @@ struct pci_dev {
> >  	unsigned int	is_virtfn:1;
> >  	unsigned int	reset_fn:1;
> >  	unsigned int    is_hotplug_bridge:1;
> > +	unsigned int	is_thunderbolt:1; /* part of Thunderbolt daisy chain */
> >  	unsigned int    __aer_firmware_first_valid:1;
> >  	unsigned int	__aer_firmware_first:1;
> >  	unsigned int	broken_intx_masking:1;
> > --
> > 2.11.0