Re: [PATCH v2 00/10] Refine the locking for dev->iommu_group

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Aug 08, 2023 at 06:31:47PM +0800, Chen-Yu Tsai wrote:
> INFO: task kworker/u18:1:67 blocked for more than 122 seconds.
>       Not tainted 6.5.0-rc5-next-20230808-08004-g396bbe23dbf4 #859
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> task:kworker/u18:1   state:D stack:0     pid:67    ppid:2      flags:0x00000008
> Workqueue: events_unbound deferred_probe_work_func
> Call trace:
>  __switch_to+0x138/0x1e8
>  __schedule+0x728/0x1388
>  schedule+0xa8/0x170
>  schedule_preempt_disabled+0x44/0x80
>  __mutex_lock+0x3fc/0x598
>  mutex_lock_nested+0x2c/0x40
>  __iommu_probe_device+0xb8/0x6e0
>  probe_iommu_group+0x18/0x38
>  bus_for_each_dev+0xe4/0x168
>  bus_iommu_probe+0x8c/0x240
>  iommu_device_register+0x120/0x1b0
>  mtk_iommu_probe+0x494/0x7a0
>  platform_probe+0x94/0x100
>  really_probe+0x1e4/0x3e8
>  __driver_probe_device+0xc0/0x1a0
>  driver_probe_device+0x110/0x1f0
>  __device_attach_driver+0xf0/0x1b0
>  bus_for_each_drv+0xf0/0x170
>  __device_attach+0x120/0x240
>  device_initial_probe+0x1c/0x30
>  bus_probe_device+0xdc/0xe8
>  deferred_probe_work_func+0xf0/0x140
>  process_one_work+0x3b0/0x910
>  worker_thread+0x33c/0x610
>  kthread+0x1dc/0x1f0
>  ret_from_fork+0x10/0x20

Oh weird, I wonder why this didn't show up on my testing? It is trying
to probe the iommu device itself against the iommu and deadlocks on
the device_lock (which is held during probe):

> process_one_work+0x2c4/0x910
>  #2: ffffff80c14090f8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x8c/0x240
>  #3: ffffff80c14090f8 (&dev->mutex){....}-{3:3}, at:

I suppose the easy fix is to just exclude the iommu driver from
probing, it doesn't have an iommu usually anyhow.

Does this work for you?

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7dbbcffac21930..faa0e3520f66b0 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -273,7 +273,7 @@ int iommu_device_register(struct iommu_device *iommu,
 
 	for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++) {
 		iommu_buses[i]->iommu_ops = ops;
-		err = bus_iommu_probe(iommu_buses[i]);
+		err = bus_iommu_probe(iommu_buses[i], iommu);
 	}
 	if (err)
 		iommu_device_unregister(iommu);
@@ -1784,12 +1784,21 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
 	return group->default_domain;
 }
 
+struct probe_iommu_args {
+	struct list_head *group_list;
+	struct iommu_device *iommu;
+};
+
 static int probe_iommu_group(struct device *dev, void *data)
 {
-	struct list_head *group_list = data;
+	struct probe_iommu_args *args = data;
 	int ret;
 
-	ret = __iommu_probe_device(dev, group_list);
+	/* We never probe the iommu device itself */
+	if (args->iommu && args->iommu->dev == dev)
+		return 0;
+
+	ret = __iommu_probe_device(dev, args->group_list);
 	if (ret == -ENODEV)
 		ret = 0;
 
@@ -1858,13 +1867,16 @@ static void iommu_group_do_probe_finalize(struct device *dev)
 		ops->probe_finalize(dev);
 }
 
-int bus_iommu_probe(const struct bus_type *bus)
+int bus_iommu_probe(const struct bus_type *bus, struct iommu_device *iommu)
 {
 	struct iommu_group *group, *next;
+	struct probe_iommu_args args = {};
 	LIST_HEAD(group_list);
 	int ret;
 
-	ret = bus_for_each_dev(bus, NULL, &group_list, probe_iommu_group);
+	args.group_list = &group_list;
+	args.iommu = iommu;
+	ret = bus_for_each_dev(bus, NULL, &args, probe_iommu_group);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 97c45f50bf4332..e8ab7cb832ba37 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1234,6 +1234,9 @@ static int omap_iommu_probe(struct platform_device *pdev)
 		if (err)
 			goto out_sysfs;
 		obj->has_iommu_driver = true;
+	} else {
+		/* Re-probe bus to probe device attached to this IOMMU */
+		bus_iommu_probe(&platform_bus_type, NULL);
 	}
 
 	pm_runtime_enable(obj->dev);
@@ -1242,9 +1245,6 @@ static int omap_iommu_probe(struct platform_device *pdev)
 
 	dev_info(&pdev->dev, "%s registered\n", obj->name);
 
-	/* Re-probe bus to probe device attached to this IOMMU */
-	bus_iommu_probe(&platform_bus_type);
-
 	return 0;
 
 out_sysfs:
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f1e18e81fca78b..8e10225e0d611a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -465,7 +465,8 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 	return dev->iommu->iommu_dev->ops;
 }
 
-extern int bus_iommu_probe(const struct bus_type *bus);
+extern int bus_iommu_probe(const struct bus_type *bus,
+			   struct iommu_device *iommu);
 extern bool iommu_present(const struct bus_type *bus);
 extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
 extern bool iommu_group_has_isolated_msi(struct iommu_group *group);