[PATCH v3] iommu: Fix iommu selftest running test mock domain fail

[PATCH v3] iommu: Fix iommu selftest running test mock domain fail

[Guixin Liu]

When intel_iommu=on is enabled and the iommu selftests are executed,
we observed multiple test case failures. Our investigation revealed
that both the Intel iommu driver and the mock iommu driver registered
iommu_device instances with fwnode=NULL, and both were added to the
iommu_device_list. Later, when the mock iommu driver attempted to
create a device, iommu_init_device attempted to retrieve the iommu_ops
by searching the iommu_device_list using the provided fwnode
(which was NULL).
Since the iommu_device registered by the Intel iommu driver appeared
earlier in the list, it was matched first instead of the mock iommu
driver's iommu_device. This ultimately caused probe_device to fail.

Fix this by alloc a software fwnode for mock iommu driver's iommu_device,
and set it to the device which mock iommu driver created.

Test by "make -C toosl/testing/selftests TARGETS=iommu run_tests":
PASSED: 229 / 229 tests passed.

In addition, this issue is also can be found on amd platform, and
also tested on a amd machine.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
Tested-by: Qinyun Tan <qinyuntan@linux.alibaba.com>
---
Changes from v2 to v3:
- Add export symbol iommu_mock_device_init() and iommu_mock_device_abort()
in iommu.c instead of export iommu_probe_device_lock and iommu_fwspec_free().

Changes from v1 to v2:
- Remove print when iommu_fwspec_init() fail.
 drivers/iommu/iommu-priv.h       |  3 +++
 drivers/iommu/iommu.c            | 28 ++++++++++++++++++++++++++++
 drivers/iommu/iommufd/selftest.c |  5 +++++
 3 files changed, 36 insertions(+)

diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index e236b932e766..40c477db3cd9 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -37,6 +37,9 @@ void iommu_device_unregister_bus(struct iommu_device *iommu,
 				 const struct bus_type *bus,
 				 struct notifier_block *nb);
 
+int iommu_mock_device_init(struct device *dev, struct iommu_device *iommu);
+void iommu_mock_device_abort(struct device *dev);
+
 struct iommu_attach_handle *iommu_attach_handle_get(struct iommu_group *group,
 						    ioasid_t pasid,
 						    unsigned int type);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 060ebe330ee1..6e51cfbebd64 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -304,6 +304,7 @@ void iommu_device_unregister_bus(struct iommu_device *iommu,
 				 struct notifier_block *nb)
 {
 	bus_unregister_notifier(bus, nb);
+	fwnode_remove_software_node(iommu->fwnode);
 	iommu_device_unregister(iommu);
 }
 EXPORT_SYMBOL_GPL(iommu_device_unregister_bus);
@@ -326,6 +327,14 @@ int iommu_device_register_bus(struct iommu_device *iommu,
 	if (err)
 		return err;
 
+	iommu->fwnode = fwnode_create_software_node(NULL, NULL);
+	if (IS_ERR(iommu->fwnode)) {
+		iommu->fwnode = NULL;
+		dev_err(iommu->dev, "add property failed:%d\n", err);
+		bus_unregister_notifier(bus, nb);
+		return err;
+	}
+
 	spin_lock(&iommu_device_lock);
 	list_add_tail(&iommu->list, &iommu_device_list);
 	spin_unlock(&iommu_device_lock);
@@ -335,9 +344,28 @@ int iommu_device_register_bus(struct iommu_device *iommu,
 		iommu_device_unregister_bus(iommu, bus, nb);
 		return err;
 	}
+	WRITE_ONCE(iommu->ready, true);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iommu_device_register_bus);
+
+int iommu_mock_device_init(struct device *dev, struct iommu_device *iommu)
+{
+	int rc;
+
+	mutex_lock(&iommu_probe_device_lock);
+	rc = iommu_fwspec_init(dev, iommu->fwnode);
+	mutex_unlock(&iommu_probe_device_lock);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(iommu_mock_device_init);
+
+void iommu_mock_device_abort(struct device *dev)
+{
+	iommu_fwspec_free(dev);
+}
+EXPORT_SYMBOL_GPL(iommu_mock_device_abort);
 #endif
 
 static struct dev_iommu *dev_iommu_get(struct device *dev)
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 61686603c769..38aaa2b02af4 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -1077,6 +1077,7 @@ static void mock_dev_release(struct device *dev)
 {
 	struct mock_dev *mdev = to_mock_dev(dev);
 
+	iommu_mock_device_abort(&mdev->dev);
 	ida_free(&mock_dev_ida, mdev->id);
 	kfree(mdev);
 }
@@ -1126,6 +1127,10 @@ static struct mock_dev *mock_dev_create(unsigned long dev_flags)
 		goto err_put;
 	}
 
+	rc = iommu_mock_device_init(&mdev->dev, &mock_iommu.iommu_dev);
+	if (rc)
+		goto err_put;
+
 	rc = device_add(&mdev->dev);
 	if (rc)
 		goto err_put;
-- 
2.43.0

Re: [PATCH v3] iommu: Fix iommu selftest running test mock domain fail

[Jason Gunthorpe]

On Wed, Sep 24, 2025 at 03:43:59PM +0800, Guixin Liu wrote:
> @@ -1077,6 +1077,7 @@ static void mock_dev_release(struct device *dev)
>  {
>  	struct mock_dev *mdev = to_mock_dev(dev);
>  
> +	iommu_mock_device_abort(&mdev->dev);
>  	ida_free(&mock_dev_ida, mdev->id);
>  	kfree(mdev);
>  }

I don't think this call is necessary? Once device_add() completes the
iommu_bus_notifier() should be active for the device.

BUS_NOTIFY_REMOVED_DEVICE triggers iommu_release_device() which does
dev_iommu_free() and frees the fwspec automatically.

The only unnatural condition is if device_add() fails then the
notifier can't run and the caller has to clean it up.

Yes?

> @@ -1126,6 +1127,10 @@ static struct mock_dev *mock_dev_create(unsigned long dev_flags)
>  		goto err_put;
>  	}
>  
> +	rc = iommu_mock_device_init(&mdev->dev, &mock_iommu.iommu_dev);
> +	if (rc)
> +		goto err_put;
> +
>  	rc = device_add(&mdev->dev);
>  	if (rc)
>  		goto err_put;

So I will suggest one further refinement to pull the device_add() into
iommu_mock_device_add() and then it can internally free the fwspec on
error and don't bother with iommu_mock_device_abort().

But other than that little note this looks fine to me.

Jason

Re: [PATCH v3] iommu: Fix iommu selftest running test mock domain fail

[Guixin Liu]

在 2025/9/24 20:53, Jason Gunthorpe 写道:
> On Wed, Sep 24, 2025 at 03:43:59PM +0800, Guixin Liu wrote:
>> @@ -1077,6 +1077,7 @@ static void mock_dev_release(struct device *dev)
>>   {
>>   	struct mock_dev *mdev = to_mock_dev(dev);
>>   
>> +	iommu_mock_device_abort(&mdev->dev);
>>   	ida_free(&mock_dev_ida, mdev->id);
>>   	kfree(mdev);
>>   }
> I don't think this call is necessary? Once device_add() completes the
> iommu_bus_notifier() should be active for the device.
>
> BUS_NOTIFY_REMOVED_DEVICE triggers iommu_release_device() which does
> dev_iommu_free() and frees the fwspec automatically.
>
> The only unnatural condition is if device_add() fails then the
> notifier can't run and the caller has to clean it up.
>
> Yes?
You are right, I didn't consider it comprehensively.
>
>> @@ -1126,6 +1127,10 @@ static struct mock_dev *mock_dev_create(unsigned long dev_flags)
>>   		goto err_put;
>>   	}
>>   
>> +	rc = iommu_mock_device_init(&mdev->dev, &mock_iommu.iommu_dev);
>> +	if (rc)
>> +		goto err_put;
>> +
>>   	rc = device_add(&mdev->dev);
>>   	if (rc)
>>   		goto err_put;
> So I will suggest one further refinement to pull the device_add() into
> iommu_mock_device_add() and then it can internally free the fwspec on
> error and don't bother with iommu_mock_device_abort().
Nice idea, I will change it in v4, thanks.

Best Regards,
Guixin Liu
> But other than that little note this looks fine to me.
>
> Jason