On Thu, Dec 21, 2023 at 03:53:42PM -0500, Esther Shimanovich wrote: > 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. Thanks for all your work here. This is going to be a maintenance problem. We typically use quirks to work around hardware defects, e.g., a device that advertises a feature that doesn't work per spec. But I don't see where the defect is here. And I doubt that this is really a unique situation. So it's likely that this will happen on other systems, and we don't want to have to add quirks every time another one shows up. If this is a firmware defect, e.g., if this platform is using "ExternalFacingPort" incorrectly, we can add a quirk to work around that, too. But I'm not sure that's the case. > Signed-off-by: Esther Shimanovich <eshimanovich@xxxxxxxxxxxx> > --- > Changes in v4: > - replaced a dmi check in the rootport quirk with a subsystem vendor and > device check. > - Link to v3: https://lore.kernel.org/r/20231220-thunderbolt-pci-patch-4-v3-1-056fd1717d06@xxxxxxxxxxxx > > Changes in v3: > - removed redundant dmi check, as the subsystem vendor check is > sufficient > - switched to PCI_VENDOR_ID_LENOVO instead of hex code > - Link to v2: https://lore.kernel.org/r/20231219-thunderbolt-pci-patch-4-v2-1-ec2d7af45a9b@xxxxxxxxxxxx > > 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@xxxxxxxxxxxx > --- > drivers/pci/quirks.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 112 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index ea476252280a..34e43323ff14 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -3873,6 +3873,118 @@ 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 Can you include a citation (spec name, revision, section) for this DMAR requirement? > + * 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. Random nits: Capitalize spec terms like "PCI", "USB", "Root Port", etc., consistently in English text. Rewrap to fill 78 columns or so. Add blank lines between paragraphs. This applies to the commit log (which should be wrapped to 75 to allow for "git log" indent) as well as comments. But honestly, I hope we can figure out a solution that doesn't require a big comment like this. Checking for random device IDs to deduce the topology is not the way PCI is supposed to work. PCI is designed to be enumerable, so software can learn what it needs to know by interrogating the hardware directly. For cases where that's impossible, ACPI is supposed to fill the gaps, e.g., with "ExternalFacingPort". If this patch covers over a gap that firmware doesn't handle yet, maybe we need to add some new firmware interface. If that's the case, we can add quirks for platforms that don't have the new interface. But we at least need a plan that doesn't require quirks indefinitely. > + */ > +static void carbon_X1_fixup_relabel_alpine_ridge(struct pci_dev *dev) > +{ > + /* Is this JHL6540 PCI component embedded in a Lenovo device? */ > + if (dev->subsystem_vendor != PCI_VENDOR_ID_LENOVO) > + 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) > +{ > + /* Is this JHL6540 PCI component embedded in a Lenovo device? */ > + if (dev->subsystem_vendor != PCI_VENDOR_ID_LENOVO) > + 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->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 > > --- > base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86 > change-id: 20231219-thunderbolt-pci-patch-4-ede71cb833c4 > > Best regards, > -- > Esther Shimanovich <eshimanovich@xxxxxxxxxxxx> >
