Re: [PATCH v2 23/33] iommu/mediatek-v1 Convert to probe/release_device() call-backs

[Yong Wu]

On Fri, 2020-05-15 at 12:07 +0200, Joerg Roedel wrote:
> Hi,
> 
> On Fri, May 15, 2020 at 03:44:59PM +0800, Yong Wu wrote:
> > On Tue, 2020-04-14 at 15:15 +0200, Joerg Roedel wrote:
> > > -	return iommu_device_link(&data->iommu, dev);
> > > +	err = arm_iommu_attach_device(dev, mtk_mapping);
> > > +	if (err)
> > > +		dev_err(dev, "Can't create IOMMU mapping - DMA-OPS will not work\n");
> > 
> > 
> > Hi Joerg,
> > 
> >      Thanks very much for this patch.
> > 
> >      This arm_iommu_attach_device is called just as we expected.
> > 
> >      But it will fail in this callstack as the group->mutex was tried to
> > be re-locked...
> > 
> > [<c0938e8c>] (iommu_attach_device) from [<c0317590>]
> > (__arm_iommu_attach_device+0x34/0x90)
> > [<c0317590>] (__arm_iommu_attach_device) from [<c03175f8>]
> > (arm_iommu_attach_device+0xc/0x20)
> > [<c03175f8>] (arm_iommu_attach_device) from [<c09432cc>]
> > (mtk_iommu_probe_finalize+0x34/0x50)
> > [<c09432cc>] (mtk_iommu_probe_finalize) from [<c093a8ac>]
> > (bus_iommu_probe+0x2a8/0x2c4)
> > [<c093a8ac>] (bus_iommu_probe) from [<c093a950>] (bus_set_iommu
> > +0x88/0xd4)
> > [<c093a950>] (bus_set_iommu) from [<c0943c74>] (mtk_iommu_probe
> > +0x2f8/0x364)
> 
> Thanks for the report, is
> 
> 	https://lore.kernel.org/lkml/1589530123-30240-1-git-send-email-yong.wu@xxxxxxxxxxxx/
> 
> The fix for this issue or is something else required?

No. That patch only adjust the internal flow to satisfy the latest
framework, it's not for fixing this mutex issue. 

Here I only reported this issue.

below is my local patch. split "dma_attach" to attach_device and
probe_finalize. About attach_device, Use the existed
__iommu_attach_group instead. Then rename from the "dma_attach" to
"probe_finalize" to do the probe_finalize job. And move it outside of
the mutex_unlock.

I'm not sure if it is right. and of course I will test if you have any
other solution. Thanks.


--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1665,26 +1665,20 @@ static void probe_alloc_default_domain(struct
bus_type *bus,
 
 }
 
-static int iommu_group_do_dma_attach(struct device *dev, void *data)
+static int iommu_group_do_probe_finalize(struct device *dev, void
*data)
 {
 	struct iommu_domain *domain = data;
-	const struct iommu_ops *ops;
-	int ret;
-
-	ret = __iommu_attach_device(domain, dev);
-
-	ops = domain->ops;
+	const struct iommu_ops *ops = domain->ops;
 
-	if (ret == 0 && ops->probe_finalize)
+	if (ops->probe_finalize)
 		ops->probe_finalize(dev);
-
-	return ret;
+	return 0;
 }
 
-static int __iommu_group_dma_attach(struct iommu_group *group)
+static int iommu_group_probe_finalize(struct iommu_group *group)
 {
 	return __iommu_group_for_each_dev(group, group->default_domain,
-					  iommu_group_do_dma_attach);
+					  iommu_group_do_probe_finalize);
 }
 
 static int iommu_do_create_direct_mappings(struct device *dev, void
*data)
@@ -1731,12 +1725,14 @@ int bus_iommu_probe(struct bus_type *bus)
 
 		iommu_group_create_direct_mappings(group);
 
-		ret = __iommu_group_dma_attach(group);
+		ret = __iommu_attach_group(group->default_domain, group);
 
 		mutex_unlock(&group->mutex);
 
 		if (ret)
 			break;
+
+		iommu_group_probe_finalize(group);
 	}
 
 	return ret;
-- 

> 
> 
> Thanks,
> 
> 	Joerg
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-mediatek