diff mbox series

[v2] PCI: Relabel JHL6540 on Lenovo X1 Carbon 7,8

Message ID 20231219-thunderbolt-pci-patch-4-v2-1-ec2d7af45a9b@chromium.org
State New
Headers show
Series [v2] PCI: Relabel JHL6540 on Lenovo X1 Carbon 7,8 | expand

Commit Message

Esther Shimanovich Dec. 19, 2023, 9:34 p.m. UTC 
On Lenovo X1 Carbon Gen 7/8 devices, when a platform enables a policy to
distrust removable PCI devices, the build-in USB-C ports stop working at
all.
This happens because these X1 Carbon models have a unique feature; a
Thunderbolt controller that is discrete from the SoC. The software sees
this controller, and incorrectly assumes it is a removable PCI device,
even though it is fixed to the computer and is wired to the computer's
own USB-C ports.

Relabel all the components of the JHL6540 controller as DEVICE_FIXED,
and where applicable, external_facing.

Ensure that the security policy to distrust external PCI devices works
as intended, and that the device's USB-C ports are able to enumerate
even when the policy is enabled.

Signed-off-by: Esther Shimanovich <eshimanovich@chromium.org>
---
Changes in v2:
- nothing new, v1 was just a test run to see if the ASCII diagram would
  be rendered properly in mutt and k-9
- for folks using gmail, make sure to select "show original" on the top
  right, as otherwise the diagram will be garbled by the standard
  non-monospace font
- Link to v1: https://lore.kernel.org/r/20231219-thunderbolt-pci-patch-4-v1-1-4e8e3773f0a9@chromium.org
---
 drivers/pci/quirks.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)


---
base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
change-id: 20231219-thunderbolt-pci-patch-4-ede71cb833c4

Best regards,

Comments

Dmitry Torokhov Dec. 19, 2023, 10:15 p.m. UTC | #1 
Hi Esther,

On Tue, Dec 19, 2023 at 04:34:33PM -0500, Esther Shimanovich wrote:
> +static void carbon_X1_fixup_relabel_alpine_ridge(struct pci_dev *dev)
> +{
> +	if (!dmi_match(DMI_PRODUCT_FAMILY, "ThinkPad X1 Carbon 7th") &&
> +	    !dmi_match(DMI_PRODUCT_FAMILY, "ThinkPad X1 Carbon Gen 8"))
> +		return;
> +
> +	/* Is this JHL6540 PCI component embedded in a Lenovo device? */
> +	if (dev->subsystem_vendor != 0x17aa)
> +		return;

Maybe use PCI_VENDOR_ID_LENOVO and move the check first - it is cheaper
than string comparison. In general, symbolic constants are preferred to
magic numbers.

Actually, do we really need to check DMI given the checks below?

> +
> +	/* Is this JHL6540 PCI component embedded in an X1 Carbon Gen 7/8? */
> +	if (dev->subsystem_device != 0x22be && // Gen 8
> +	    dev->subsystem_device != 0x2292) { // Gen 7
> +		return;
> +	}
> +
> +	dev_set_removable(&dev->dev, DEVICE_FIXED);
> +
> +	/* Not all 0x15d3 components are external facing */
> +	if (dev->device == 0x15d3 &&

Again, maybe PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE?

Thanks.
Esther Shimanovich Dec. 19, 2023, 11:19 p.m. UTC | #2 
> Maybe use PCI_VENDOR_ID_LENOVO and move the check first - it is cheaper
> than string comparison. In general, symbolic constants are preferred to
> magic numbers.

That makes sense! Will do.

> Actually, do we really need to check DMI given the checks below?

I was advised by Rajat Jain to check DMI. This is the reasoning he
gave me: "I'm not certain if you can use subsystem vendor alone
because, subsystem vendor & ID are defined by the PCI device vendor I
think (Intel here). What if Intel sold the same bridges to another
company and has the same subsystem vendor / ID."
To me it seems like each company in practice has a different subsystem
ID, but I don't know enough to confirm this 100%. If you are confident
that the subsystem IDs are sufficient, let me know and I'm happy to
switch them.
I'd appreciate some more insight on this before I remove the DMI checks!

>
> > +
> > +     /* Not all 0x15d3 components are external facing */
> > +     if (dev->device == 0x15d3 &&
>
> Again, maybe PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE?

Oh! I missed that. Will use, thanks!
Dmitry Torokhov Dec. 20, 2023, 1:38 a.m. UTC | #3 
On Tue, Dec 19, 2023 at 06:19:39PM -0500, Esther Shimanovich wrote:
> > Maybe use PCI_VENDOR_ID_LENOVO and move the check first - it is cheaper
> > than string comparison. In general, symbolic constants are preferred to
> > magic numbers.
> 
> That makes sense! Will do.
> 
> > Actually, do we really need to check DMI given the checks below?
> 
> I was advised by Rajat Jain to check DMI. This is the reasoning he
> gave me: "I'm not certain if you can use subsystem vendor alone
> because, subsystem vendor & ID are defined by the PCI device vendor I
> think (Intel here). What if Intel sold the same bridges to another
> company and has the same subsystem vendor / ID."

I believe subsystem vendor and product IDs are not baked into the device
but set up by the system firmware, and therefore there should be no
concerns with mixing up IDs, but I am happy to be corrected by people
with more experience with PCI.

Thanks.
Esther Shimanovich Dec. 20, 2023, 7:46 p.m. UTC | #4 
> Again, maybe PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE?

Question--- PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE is defined in
"drivers/thunderbolt/nhi.h" as opposed to "include/linux/pci_ids.h".
It seems like the ids in "drivers/thunderbolt/nhi.h" are specifically
for use within the thunderbolt driver only. Would you want me to move
it into pci_ids.h so that I could use it here? Or could I ignore this
suggestion? My personal inclination is that that would make more sense
to do in a separate refactoring patch.
Dmitry Torokhov Dec. 20, 2023, 10:03 p.m. UTC | #5 
On Wed, Dec 20, 2023 at 02:46:20PM -0500, Esther Shimanovich wrote:
> > Again, maybe PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE?
> 
> Question--- PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE is defined in
> "drivers/thunderbolt/nhi.h" as opposed to "include/linux/pci_ids.h".
> It seems like the ids in "drivers/thunderbolt/nhi.h" are specifically
> for use within the thunderbolt driver only. Would you want me to move
> it into pci_ids.h so that I could use it here? Or could I ignore this
> suggestion? My personal inclination is that that would make more sense
> to do in a separate refactoring patch.

I'll leave that decision to PCI maintainers.

Thanks.
diff mbox series

Patch

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index ea476252280a..58401b099407 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3873,6 +3873,116 @@  DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL,
 			       quirk_apple_poweroff_thunderbolt);
 #endif
 
+/*
+ * On most ThinkPad Carbon 7/8s, JHL6540 Thunderbolt 3 bridges are set
+ * incorrectly as DEVICE_REMOVABLE despite being built into the device.
+ * This is the side effect of a unique hardware configuration.
+ *
+ * Normally, Thunderbolt functionality is integrated to the SoC and
+ * its root ports.
+ *
+ *                          Most devices:
+ *                    root port --> USB-C port
+ *
+ * But X1 Carbon Gen 7/8 uses Whiskey Lake and Comet Lake SoC, which
+ * don't come with Thunderbolt functionality. Therefore, a discrete
+ * Thunderbolt Host PCI controller was added between the root port and
+ * the USB-C port.
+ *
+ *                        Thinkpad Carbon 7/8s
+ *                 (w/ Whiskey Lake and Comet Lake SoC):
+ *                root port -->  JHL6540   --> USB-C port
+ *
+ * Because the root port is labeled by FW as "ExternalFacingPort", as
+ * required by the DMAR ACPI spec, the JHL6540 chip is inaccurately
+ * labeled as DEVICE_REMOVABLE by the kernel pci driver.
+ * Therefore, the built-in USB-C ports do not enumerate when policies
+ * forbidding external pci devices are enforced.
+ *
+ * This fix relabels the pci components in the built-in JHL6540 chip as
+ * DEVICE_FIXED, ensuring that the built-in USB-C ports always enumerate
+ * properly as intended.
+ *
+ * This fix also labels the external facing components of the JHL6540 as
+ * external_facing, so that the pci attach policy works as intended.
+ *
+ * The ASCII diagram below describes the pci layout of the JHL6540 chip.
+ *
+ *                         Root Port
+ *                 [8086:02b4] or [8086:9db4]
+ *                             |
+ *                        JHL6540 Chip
+ *     __________________________________________________
+ *    |                      Bridge                      |
+ *    |        PCI ID ->  [8086:15d3]                    |
+ *    |         DEVFN ->      (00)                       |
+ *    |       _________________|__________________       |
+ *    |      |           |            |           |      |
+ *    |    Bridge     Bridge        Bridge      Bridge   |
+ *    | [8086:15d3] [8086:15d3]  [8086:15d3] [8086:15d3] |
+ *    |    (00)        (08)         (10)        (20)     |
+ *    |      |           |            |           |      |
+ *    |     NHI          |     USB Controller     |      |
+ *    | [8086:15d2]      |       [8086:15d4]      |      |
+ *    |    (00)          |          (00)          |      |
+ *    |      |___________|            |___________|      |
+ *    |____________|________________________|____________|
+ *                 |                        |
+ *             USB-C Port               USB-C Port
+ *
+ *
+ * Based on what a JHL6549 pci component's pci id, subsystem device id
+ * and devfn are, we can infer if it is fixed and if it faces a usb port;
+ * which would mean it is external facing.
+ * This quirk uses these values to identify the pci components and set the
+ * properties accordingly.
+ */
+static void carbon_X1_fixup_relabel_alpine_ridge(struct pci_dev *dev)
+{
+	if (!dmi_match(DMI_PRODUCT_FAMILY, "ThinkPad X1 Carbon 7th") &&
+	    !dmi_match(DMI_PRODUCT_FAMILY, "ThinkPad X1 Carbon Gen 8"))
+		return;
+
+	/* Is this JHL6540 PCI component embedded in a Lenovo device? */
+	if (dev->subsystem_vendor != 0x17aa)
+		return;
+
+	/* Is this JHL6540 PCI component embedded in an X1 Carbon Gen 7/8? */
+	if (dev->subsystem_device != 0x22be && // Gen 8
+	    dev->subsystem_device != 0x2292) { // Gen 7
+		return;
+	}
+
+	dev_set_removable(&dev->dev, DEVICE_FIXED);
+
+	/* Not all 0x15d3 components are external facing */
+	if (dev->device == 0x15d3 &&
+	    dev->devfn != 0x08 &&
+	    dev->devfn != 0x20) {
+		return;
+	}
+
+	dev->external_facing = true;
+}
+
+/*
+ * We also need to relabel the root port as a consequence of changing
+ * the JHL6540's PCIE hierarchy.
+ */
+static void carbon_X1_fixup_rootport_not_removable(struct pci_dev *dev)
+{
+	if (!dmi_match(DMI_PRODUCT_FAMILY, "ThinkPad X1 Carbon 7th") &&
+	    !dmi_match(DMI_PRODUCT_FAMILY, "ThinkPad X1 Carbon Gen 8"))
+		return;
+
+	dev->external_facing = false;
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x15d3, carbon_X1_fixup_relabel_alpine_ridge);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x15d2, carbon_X1_fixup_relabel_alpine_ridge);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x15d4, carbon_X1_fixup_relabel_alpine_ridge);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x02b4, carbon_X1_fixup_rootport_not_removable);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x9db4, carbon_X1_fixup_rootport_not_removable);
+
 /*
  * Following are device-specific reset methods which can be used to
  * reset a single function if other methods (e.g. FLR, PM D0->D3) are