Re: [PATCH v4] PCI: Relabel JHL6540 on Lenovo X1 Carbon 7,8

[Mika Westerberg]

On Mon, Jan 22, 2024 at 05:50:24PM -0600, Mario Limonciello wrote:
> On 1/19/2024 04:22, Mika Westerberg wrote:
> > On Fri, Jan 19, 2024 at 09:48:29AM +0200, Mika Westerberg wrote:
> > > On Fri, Jan 19, 2024 at 07:37:56AM +0200, Mika Westerberg wrote:
> > > > On Thu, Jan 18, 2024 at 08:12:56AM -0800, Dmitry Torokhov wrote:
> > > > > On Thu, Jan 18, 2024 at 09:47:07AM -0600, Mario Limonciello wrote:
> > > > > > On 1/18/2024 00:00, Mika Westerberg wrote:
> > > > > > > > Before my patch, you see that the JHL6540 controller is inaccurately
> > > > > > > > labeled “removable”:
> > > > > > > > $ udevadm info -a -p /sys/bus/pci/devices/0000:05:00.0 | grep -e
> > > > > > > > {removable} -e {device} -e {vendor} -e looking
> > > > > > > >     looking at device '/devices/pci0000:00/0000:00:1d.4/0000:05:00.0':
> > > > > > > >       ATTR{device}=="0x15d3"
> > > > > > > >       ATTR{removable}=="removable"
> > > > > > > >       ATTR{vendor}=="0x8086"
> > > > > > > 
> > > > > > > This is actually accurate. The Thunderbolt controller is itself
> > > > > > > hot-removable and that BTW happens to be hot-removed when fwupd applies
> > > > > > > firmware upgrades to the device.
> > > > > 
> > > > > This is quite interesting take. Does fwupd rip the controller out of the
> > > > > box to update it? By that account your touchpad is also removable as it
> > > > > may stop functioning when its firmware gets updated.
> > > > 
> > > > The Thunderbolt controller is connected to a hotpluggable PCIe root port
> > > > so it will be dissappear from the userspace so that "removable" in that
> > > > sense is accurate.
> > > 
> > > There are systems as well where the Thunderbolt (and/or xHCI) controller
> > > only appears if there is anything plugged to the physical Type-C ports
> > > and it gets removed pretty soon after the physical device gets
> > > unplugged. These are also the same Alpine Ridge and Titan Ridge
> > > controllers that this patch is dealing with.
> > > 
> > > I tried to think about some sort of more generic heuristic how to figure
> > > out that the controller is actually inside the physical system but there
> > > is a problem that the same controller can appear on the bus as well, eg.
> > > you plug in Thunderbolt dock and that one has xHCI controller too. That
> > > device should definitely be "removable". With the "software CM" systems
> > > we have a couple of additional hints in the ACPI tables that can be used
> > > to identify the "tunneled" ports but this does not apply to the older
> > > systems I'm afraid.
> > 
> > The below "might" work:
> > 
> > 1. A device that is directly behind a PCIe root or downstream port that
> >     has ->external_facing == 1.
> > 
> > 2. It is a PCIe endpoint.
> > 
> > 3. It is a sibling to or has any of the below PCI IDs (see
> >     drivers/thunderbolt/nhi.h for the definitions):
> > 
> >     PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI
> >     PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI
> >     PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_USBONLY_NHI
> >     PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_USBONLY_NHI
> >     PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI
> >     PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI
> >     PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI
> > 
> >     And for all USB4 we can use the PCI class:
> > 
> >     PCI_CLASS_SERIAL_USB_USB4
> > 
> > (4. With USB4 systems we could also add the check that the device is not
> > below the tunneled PCIe ports but that's kind of redundant).
> > 
> > I think this covers the existing devices as well as future discrete USB4
> > controllers and marks both the NHI and the xHCI as "fixed" which we
> > could also use to lift the bounce buffering restriction from them.
> > 
> > @Mario, did I miss anything?
> 
> The bounce buffering restriction is only for unaligned DMA isn't it? Does
> that tend to happen a lot?

AFAICT no but this would allow to use IOMMU identity mappings instead of
full mappings with these devices.

> But otherwise I think this does a good job.  It will cover external
> enclosure cases too because of having to check it's directly behind a root
> port.
> 
> But it should also include comments about why it's not needed on newer
> systems (IE the ACPI hints for someone with no prior knowledge looking at
> this to find).

Agree.