Messages in this thread | | | Date | Mon, 8 Apr 2024 15:15:21 +0800 | Subject | Re: [PATCH 1/1] iommu/vt-d: Fix WARN_ON in iommu probe path | From | Baolu Lu <> |
| |
On 2024/4/8 11:52, Tian, Kevin wrote: >> From: Lu Baolu<baolu.lu@linux.intel.com> >> Sent: Sunday, April 7, 2024 9:14 AM >> >> Commit 1a75cc710b95 ("iommu/vt-d: Use rbtree to track iommu probed >> devices") adds all devices probed by the iommu driver in a rbtree >> indexed by the source ID of each device. It assumes that each device >> has a unique source ID. This assumption is incorrect and the VT-d >> spec doesn't state this requirement either. >> >> The reason for using a rbtree to track devices is to look up the device >> with PCI bus and devfunc in the paths of handling ATS invalidation time >> out error and the PRI I/O page faults. Both are PCI ATS feature related. >> >> Only track the devices that have PCI ATS capabilities in the rbtree to >> avoid unnecessary WARN_ON in the iommu probe path. Otherwise, on some >> platforms below kernel splat will be displayed and the iommu probe results >> in failure. > Just be curious. What about two ATS capable devices putting behind > a PCI-to-PCIe bridge?
I don't think ATS capable device putting behind a PCI-to-PCIe bridge is a right configuration for the ATS service. The PCIe spec requires this in section 10.1.1, that
" ATS requires the following: - ATS capable components must interoperate with [PCIe-1.1] compliant components. .. "
My understanding is that PCI-to-PCIe bridge is not a PCIe compliant device.
> >> WARNING: CPU: 3 PID: 166 at drivers/iommu/intel/iommu.c:158 >> intel_iommu_probe_device+0x319/0xd90 >> Call Trace: >> <TASK> >> ? __warn+0x7e/0x180 >> ? intel_iommu_probe_device+0x319/0xd90 >> ? report_bug+0x1f8/0x200 >> ? handle_bug+0x3c/0x70 >> ? exc_invalid_op+0x18/0x70 >> ? asm_exc_invalid_op+0x1a/0x20 >> ? intel_iommu_probe_device+0x319/0xd90 >> ? debug_mutex_init+0x37/0x50 >> __iommu_probe_device+0xf2/0x4f0 >> iommu_probe_device+0x22/0x70 >> iommu_bus_notifier+0x1e/0x40 >> notifier_call_chain+0x46/0x150 >> blocking_notifier_call_chain+0x42/0x60 >> bus_notify+0x2f/0x50 >> device_add+0x5ed/0x7e0 >> platform_device_add+0xf5/0x240 >> mfd_add_devices+0x3f9/0x500 >> ? preempt_count_add+0x4c/0xa0 >> ? up_write+0xa2/0x1b0 >> ? __debugfs_create_file+0xe3/0x150 >> intel_lpss_probe+0x49f/0x5b0 >> ? pci_conf1_write+0xa3/0xf0 >> intel_lpss_pci_probe+0xcf/0x110 [intel_lpss_pci] >> pci_device_probe+0x95/0x120 >> really_probe+0xd9/0x370 >> ? __pfx___driver_attach+0x10/0x10 >> __driver_probe_device+0x73/0x150 >> driver_probe_device+0x19/0xa0 >> __driver_attach+0xb6/0x180 >> ? __pfx___driver_attach+0x10/0x10 >> bus_for_each_dev+0x77/0xd0 >> bus_add_driver+0x114/0x210 >> driver_register+0x5b/0x110 >> ? __pfx_intel_lpss_pci_driver_init+0x10/0x10 [intel_lpss_pci] >> do_one_initcall+0x57/0x2b0 >> ? kmalloc_trace+0x21e/0x280 >> ? do_init_module+0x1e/0x210 >> do_init_module+0x5f/0x210 >> load_module+0x1d37/0x1fc0 >> ? init_module_from_file+0x86/0xd0 >> init_module_from_file+0x86/0xd0 >> idempotent_init_module+0x17c/0x230 >> __x64_sys_finit_module+0x56/0xb0 >> do_syscall_64+0x6e/0x140 >> entry_SYSCALL_64_after_hwframe+0x71/0x79 >> >> Fixes: 1a75cc710b95 ("iommu/vt-d: Use rbtree to track iommu probed >> devices") >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com> >> --- >> drivers/iommu/intel/iommu.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c >> index 50eb9aed47cc..a7ecd90303dc 100644 >> --- a/drivers/iommu/intel/iommu.c >> +++ b/drivers/iommu/intel/iommu.c >> @@ -4299,9 +4299,11 @@ static struct iommu_device >> *intel_iommu_probe_device(struct device *dev) >> } >> >> dev_iommu_priv_set(dev, info); >> - ret = device_rbtree_insert(iommu, info); >> - if (ret) >> - goto free; >> + if (pdev && pci_ats_supported(pdev)) { >> + ret = device_rbtree_insert(iommu, info); >> + if (ret) >> + goto free; >> + } > probably replace device_rbtree with ats_rbtree?
It makes sense. Probably I need a follow-up patch to do such cleanup.
Best regards, baolu
| |