[PATCH 1/1] iommu/vt-d: Fix WARN_ON in iommu probe path

[PATCH 1/1] iommu/vt-d: Fix WARN_ON in iommu probe path

[Lu Baolu]

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.

 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;
+	}
 
 	if (sm_supported(iommu) && !dev_is_real_dma_subdevice(dev)) {
 		ret = intel_pasid_alloc_table(dev);
@@ -4336,7 +4338,8 @@ static void intel_iommu_release_device(struct device *dev)
 	struct intel_iommu *iommu = info->iommu;
 
 	mutex_lock(&iommu->iopf_lock);
-	device_rbtree_remove(info);
+	if (dev_is_pci(dev) && pci_ats_supported(to_pci_dev(dev)))
+		device_rbtree_remove(info);
 	mutex_unlock(&iommu->iopf_lock);
 
 	if (sm_supported(iommu) && !dev_is_real_dma_subdevice(dev) &&
-- 
2.34.1

RE: [PATCH 1/1] iommu/vt-d: Fix WARN_ON in iommu probe path

[Tian, Kevin]

> 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?

> 
>  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?

> 
>  	if (sm_supported(iommu) && !dev_is_real_dma_subdevice(dev)) {
>  		ret = intel_pasid_alloc_table(dev);
> @@ -4336,7 +4338,8 @@ static void intel_iommu_release_device(struct
> device *dev)
>  	struct intel_iommu *iommu = info->iommu;
> 
>  	mutex_lock(&iommu->iopf_lock);
> -	device_rbtree_remove(info);
> +	if (dev_is_pci(dev) && pci_ats_supported(to_pci_dev(dev)))
> +		device_rbtree_remove(info);
>  	mutex_unlock(&iommu->iopf_lock);
> 
>  	if (sm_supported(iommu) && !dev_is_real_dma_subdevice(dev) &&
> --
> 2.34.1

Re: [PATCH 1/1] iommu/vt-d: Fix WARN_ON in iommu probe path

[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

RE: [PATCH 1/1] iommu/vt-d: Fix WARN_ON in iommu probe path

[Tian, Kevin]

+Bjorn

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Monday, April 8, 2024 3:15 PM
> 
> 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.
> 

what is the pci core policy on such device? pci_enable_ats() doesn't
prevent such device and I saw various places in pci core do handle
the PCI_EXP_TYPE_PCIE_BRIDGE type...

Re: [PATCH 1/1] iommu/vt-d: Fix WARN_ON in iommu probe path

[Baolu Lu]

On 4/7/24 9:14 AM, Lu Baolu wrote:
> 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.
> 
>   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(-)

This patch closes a boot bug described here.

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10689

I will queue it for v6.9-rc if no objection.

Best regards,
baolu

RE: [PATCH 1/1] iommu/vt-d: Fix WARN_ON in iommu probe path

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, April 11, 2024 10:48 AM
> 
> On 4/7/24 9:14 AM, Lu Baolu wrote:
> > 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.
> >
> >   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(-)
> 
> This patch closes a boot bug described here.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10689
> 
> I will queue it for v6.9-rc if no objection.
> 

let's do it. the issue which I raised is rare and can be tackled when
it arises.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH 1/1] iommu/vt-d: Fix WARN_ON in iommu probe path

[Ethan Zhao]

On 4/8/2024 11:52 AM, 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?

No PCI-to-PCIe bridges such device AFAIK, ATS TLP couldn't be
routed via PCIe-to-PCI bridge unmodified. ATS is based on PCIe.

But the device-rbtree is possible holding mutiple device nodes behind
PCIe-to-PCI bridge indexed by the same requester-ID.

Thanks,
Ethan

>>   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?
>
>>   	if (sm_supported(iommu) && !dev_is_real_dma_subdevice(dev)) {
>>   		ret = intel_pasid_alloc_table(dev);
>> @@ -4336,7 +4338,8 @@ static void intel_iommu_release_device(struct
>> device *dev)
>>   	struct intel_iommu *iommu = info->iommu;
>>
>>   	mutex_lock(&iommu->iopf_lock);
>> -	device_rbtree_remove(info);
>> +	if (dev_is_pci(dev) && pci_ats_supported(to_pci_dev(dev)))
>> +		device_rbtree_remove(info);
>>   	mutex_unlock(&iommu->iopf_lock);
>>
>>   	if (sm_supported(iommu) && !dev_is_real_dma_subdevice(dev) &&
>> --
>> 2.34.1
>