| Message ID | 20200414131542.25608-24-joro@8bytes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: Move iommu_group setup to IOMMU core code | expand |
On Tue, 2020-04-14 at 15:15 +0200, Joerg Roedel wrote: > From: Joerg Roedel <jroedel@suse.de> > > 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 <jroedel@suse.de> > --- > 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(&it, err, dev->of_node, "iommus", > @@ -442,35 +440,28 @@ static int mtk_iommu_add_device(struct device *dev) > } > > if (!fwspec || fwspec->ops != &mtk_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 &data->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(&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) > } > > -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(&data->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, > }; >
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@mediatek.com/ The fix for this issue or is something else required? Thanks, Joerg
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@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(&group->mutex); if (ret) break; + + iommu_group_probe_finalize(group); } return ret;
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(&group->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
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(&it, err, dev->of_node, "iommus", @@ -442,35 +440,28 @@ static int mtk_iommu_add_device(struct device *dev) } if (!fwspec || fwspec->ops != &mtk_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 &data->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(&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"); } -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(&data->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, };