Re: [PATCH v2 23/33] iommu/mediatek-v1 Convert to probe/release_device() call-backs
Hi,
On Mon, May 18, 2020 at 02:51:20PM +0800, Yong Wu wrote:
> 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(>mutex);
>
> if (ret)
> break;
> +
> + iommu_group_probe_finalize(group);
> }
>
> return ret;
> --
Yes, I think moving the probe_finalize call out of the group->mutex
section is the right fix for this issue.
Thanks for reporting it and working on a fix.
Regards,
Joerg
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 23/33] iommu/mediatek-v1 Convert to probe/release_device() call-backs
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(>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...
> >
> > [] (iommu_attach_device) from []
> > (__arm_iommu_attach_device+0x34/0x90)
> > [] (__arm_iommu_attach_device) from []
> > (arm_iommu_attach_device+0xc/0x20)
> > [] (arm_iommu_attach_device) from []
> > (mtk_iommu_probe_finalize+0x34/0x50)
> > [] (mtk_iommu_probe_finalize) from []
> > (bus_iommu_probe+0x2a8/0x2c4)
> > [] (bus_iommu_probe) from [] (bus_set_iommu
> > +0x88/0xd4)
> > [] (bus_set_iommu) from [] (mtk_iommu_probe
> > +0x2f8/0x364)
>
> Thanks for the report, is
>
>
> https://lore.kernel.org/lkml/1589530123-30240-1-git-send-email-yong...@mediatek.com/
>
> 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(>mutex);
if (ret)
break;
+
+ iommu_group_probe_finalize(group);
}
return ret;
--
>
>
> Thanks,
>
> Joerg
>
> ___
> Linux-mediatek mailing list
> linux-media...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 23/33] iommu/mediatek-v1 Convert to probe/release_device() call-backs
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(>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... > > [] (iommu_attach_device) from [] > (__arm_iommu_attach_device+0x34/0x90) > [] (__arm_iommu_attach_device) from [] > (arm_iommu_attach_device+0xc/0x20) > [] (arm_iommu_attach_device) from [] > (mtk_iommu_probe_finalize+0x34/0x50) > [] (mtk_iommu_probe_finalize) from [] > (bus_iommu_probe+0x2a8/0x2c4) > [] (bus_iommu_probe) from [] (bus_set_iommu > +0x88/0xd4) > [] (bus_set_iommu) from [] (mtk_iommu_probe > +0x2f8/0x364) Thanks for the report, is https://lore.kernel.org/lkml/1589530123-30240-1-git-send-email-yong...@mediatek.com/ The fix for this issue or is something else required? Thanks, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 23/33] iommu/mediatek-v1 Convert to probe/release_device() call-backs
On Tue, 2020-04-14 at 15:15 +0200, Joerg Roedel wrote:
> From: Joerg Roedel
>
> Convert the Mediatek-v1 IOMMU driver to use the probe_device() and
> release_device() call-backs of iommu_ops, so that the iommu core code
> does the group and sysfs setup.
>
> Signed-off-by: Joerg Roedel
> ---
> drivers/iommu/mtk_iommu_v1.c | 50 +++-
> 1 file changed, 20 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
> index a31be05601c9..7bdd74c7cb9f 100644
> --- a/drivers/iommu/mtk_iommu_v1.c
> +++ b/drivers/iommu/mtk_iommu_v1.c
> @@ -416,14 +416,12 @@ static int mtk_iommu_create_mapping(struct device *dev,
> return 0;
> }
>
> -static int mtk_iommu_add_device(struct device *dev)
> +static struct iommu_device *mtk_iommu_probe_device(struct device *dev)
> {
> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> - struct dma_iommu_mapping *mtk_mapping;
> struct of_phandle_args iommu_spec;
> struct of_phandle_iterator it;
> struct mtk_iommu_data *data;
> - struct iommu_group *group;
> int err;
>
> of_for_each_phandle(, err, dev->of_node, "iommus",
> @@ -442,35 +440,28 @@ static int mtk_iommu_add_device(struct device *dev)
> }
>
> if (!fwspec || fwspec->ops != _iommu_ops)
> - return -ENODEV; /* Not a iommu client device */
> + return ERR_PTR(-ENODEV); /* Not a iommu client device */
>
> - /*
> - * This is a short-term bodge because the ARM DMA code doesn't
> - * understand multi-device groups, but we have to call into it
> - * successfully (and not just rely on a normal IOMMU API attach
> - * here) in order to set the correct DMA API ops on @dev.
> - */
> - group = iommu_group_alloc();
> - if (IS_ERR(group))
> - return PTR_ERR(group);
> + data = dev_iommu_priv_get(dev);
>
> - err = iommu_group_add_device(group, dev);
> - iommu_group_put(group);
> - if (err)
> - return err;
> + return >iommu;
> +}
>
> - data = dev_iommu_priv_get(dev);
> +static void mtk_iommu_probe_finalize(struct device *dev)
> +{
> + struct dma_iommu_mapping *mtk_mapping;
> + struct mtk_iommu_data *data;
> + int err;
> +
> + data= dev_iommu_priv_get(dev);
> mtk_mapping = data->dev->archdata.iommu;
> - err = arm_iommu_attach_device(dev, mtk_mapping);
> - if (err) {
> - iommu_group_remove_device(dev);
> - return err;
> - }
>
> - return iommu_device_link(>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...
[] (iommu_attach_device) from []
(__arm_iommu_attach_device+0x34/0x90)
[] (__arm_iommu_attach_device) from []
(arm_iommu_attach_device+0xc/0x20)
[] (arm_iommu_attach_device) from []
(mtk_iommu_probe_finalize+0x34/0x50)
[] (mtk_iommu_probe_finalize) from []
(bus_iommu_probe+0x2a8/0x2c4)
[] (bus_iommu_probe) from [] (bus_set_iommu
+0x88/0xd4)
[] (bus_set_iommu) from [] (mtk_iommu_probe
+0x2f8/0x364)
> }
>
> -static void mtk_iommu_remove_device(struct device *dev)
> +static void mtk_iommu_release_device(struct device *dev)
> {
> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> struct mtk_iommu_data *data;
> @@ -479,9 +470,6 @@ static void mtk_iommu_remove_device(struct device *dev)
> return;
>
> data = dev_iommu_priv_get(dev);
> - iommu_device_unlink(>iommu, dev);
> -
> - iommu_group_remove_device(dev);
> iommu_fwspec_free(dev);
> }
>
> @@ -534,8 +522,10 @@ static const struct iommu_ops mtk_iommu_ops = {
> .map= mtk_iommu_map,
> .unmap = mtk_iommu_unmap,
> .iova_to_phys = mtk_iommu_iova_to_phys,
> - .add_device = mtk_iommu_add_device,
> - .remove_device = mtk_iommu_remove_device,
> + .probe_device = mtk_iommu_probe_device,
> + .probe_finalize = mtk_iommu_probe_finalize,
> + .release_device = mtk_iommu_release_device,
> + .device_group = generic_device_group,
> .pgsize_bitmap = ~0UL << MT2701_IOMMU_PAGE_SHIFT,
> };
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
