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.