Re: [PATCH] uacce: fix concurrency of fops_open and uacce_remove
On 2022/6/16 下午4:14, Jean-Philippe Brucker wrote:
On Thu, Jun 16, 2022 at 12:10:18PM +0800, Zhangfei Gao wrote:
diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index 281c54003edc..b6219c6bfb48 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -136,9 +136,16 @@ static int uacce_fops_open(struct inode *inode, struct
file *filep)
if (!q)
return -ENOMEM;
+ mutex_lock(&uacce->queues_lock);
+
+ if (!uacce->parent->driver) {
I don't think this is useful, because the core clears parent->driver after
having run uacce_remove():
rmmod hisi_zip open()
... uacce_fops_open()
__device_release_driver()...
pci_device_remove()
hisi_zip_remove()
hisi_qm_uninit()
uacce_remove()
... ...
mutex_lock(uacce->queues_lock)
... if (!uacce->parent->driver)
device_unbind_cleanup() /* driver still valid, proceed */
dev->driver = NULL
The check if (!uacce->parent->driver) is required, otherwise NULL pointer
may happen.
I agree we need something, what I mean is that this check is not
sufficient.
iommu_sva_bind_device
const struct iommu_ops *ops = dev_iommu_ops(dev); ->
dev->iommu->iommu_dev->ops
rmmod has no issue, but remove parent pci device has the issue.
Ah right, relying on the return value of bind() wouldn't be enough even if
we mandated SVA.
[...]
I think we need the global uacce_mutex to serialize uacce_remove() and
uacce_fops_open(). uacce_remove() would do everything, including
xa_erase(), while holding that mutex. And uacce_fops_open() would try to
obtain the uacce object from the xarray while holding the mutex, which
fails if the uacce object is being removed.
Since fops_open get char device refcount, uacce_release will not happen
until open returns.
The refcount only ensures that the uacce_device object is not freed as
long as there are open fds. But uacce_remove() can run while there are
open fds, or fds in the process of being opened. And atfer uacce_remove()
runs, the uacce_device object still exists but is mostly unusable. For
example once the module is freed, uacce->ops is not valid anymore. But
currently uacce_fops_open() may dereference the ops in this case:
uacce_fops_open()
if (!uacce->parent->driver)
/* Still valid, keep going */
...rmmod
uacce_remove()
... free_module()
uacce->ops->get_queue() /* BUG */
uacce_remove should wait for uacce->queues_lock, until fops_open release
the lock.
If open happen just after the uacce_remove: unlock, uacce_bind_queue in
open should fail.
Accessing uacce->ops after free_module() is a use-after-free. We need all
you men parent release the resources.
the fops to synchronize with uacce_remove() to ensure they don't use any
resource of the parent after it's been freed.
After fops_open, currently we are counting on parent driver stop all dma
first, then call uacce_remove, which is assumption.
Like drivers/crypto/hisilicon/zip/zip_main.c: hisi_qm_wait_task_finish,
which will wait uacce_release.
If comments this , there may other issue,
Unable to handle kernel paging request at virtual address 8b700204
pc : hisi_qm_cache_wb.part.0+0x2c/0xa0
I see uacce_fops_poll() may have the same problem, and should be inside
uacce_mutex.
Do we need consider this, uacce_remove can happen anytime but not
waiting dma stop?
Not sure uacce_mutex can do this.
Currently the sequence is
mutex_lock(&uacce->queues_lock);
mutex_lock(&uacce_mutex);
Or we set all the callbacks of uacce_ops to NULL?
Module_get/put only works for module, but not for removing device.
Thanks
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu: Remove usage of the deprecated ida_simple_xxx API
Use ida_alloc_xxx()/ida_free() instead of
ida_simple_get()/ida_simple_remove().
The latter is deprecated and more verbose.
Signed-off-by: Bo Liu
---
drivers/iommu/iommu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 847ad47a2dfd..cdc86c39954e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -600,7 +600,7 @@ static void iommu_group_release(struct kobject *kobj)
if (group->iommu_data_release)
group->iommu_data_release(group->iommu_data);
- ida_simple_remove(&iommu_group_ida, group->id);
+ ida_free(&iommu_group_ida, group->id);
if (group->default_domain)
iommu_domain_free(group->default_domain);
@@ -641,7 +641,7 @@ struct iommu_group *iommu_group_alloc(void)
INIT_LIST_HEAD(&group->devices);
INIT_LIST_HEAD(&group->entry);
- ret = ida_simple_get(&iommu_group_ida, 0, 0, GFP_KERNEL);
+ ret = ida_alloc(&iommu_group_ida, GFP_KERNEL);
if (ret < 0) {
kfree(group);
return ERR_PTR(ret);
@@ -651,7 +651,7 @@ struct iommu_group *iommu_group_alloc(void)
ret = kobject_init_and_add(&group->kobj, &iommu_group_ktype,
NULL, "%d", group->id);
if (ret) {
- ida_simple_remove(&iommu_group_ida, group->id);
+ ida_free(&iommu_group_ida, group->id);
kobject_put(&group->kobj);
return ERR_PTR(ret);
}
--
2.27.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment
> From: Nicolin Chen
> Sent: Friday, June 17, 2022 6:41 AM
>
> > ...
> > > - if (resv_msi) {
> > > + if (resv_msi && !domain->msi_cookie) {
> > > ret = iommu_get_msi_cookie(domain->domain,
> > > resv_msi_base);
> > > if (ret && ret != -ENODEV)
> > > goto out_detach;
> > > + domain->msi_cookie = true;
> > > }
> >
> > why not moving to alloc_attach_domain() then no need for the new
> > domain field? It's required only when a new domain is allocated.
>
> When reusing an existing domain that doesn't have an msi_cookie,
> we can do iommu_get_msi_cookie() if resv_msi is found. So it is
> not limited to a new domain.
Looks msi_cookie requirement is per platform (currently only
for smmu. see arm_smmu_get_resv_regions()). If there is
no mixed case then above check is not required.
But let's hear whether Robin has a different thought here.
>
> > ...
> > > - if (list_empty(&domain->group_list)) {
> > > - if (list_is_singular(&iommu->domain_list)) {
> > > - if (list_empty(&iommu-
> > > >emulated_iommu_groups)) {
> > > - WARN_ON(iommu->notifier.head);
> > > -
> > > vfio_iommu_unmap_unpin_all(iommu);
> > > - } else {
> > > -
> > > vfio_iommu_unmap_unpin_reaccount(iommu);
> > > - }
> > > - }
> > > - iommu_domain_free(domain->domain);
> > > - list_del(&domain->next);
> > > - kfree(domain);
> > > - vfio_iommu_aper_expand(iommu, &iova_copy);
> >
> > Previously the aperture is adjusted when a domain is freed...
> >
> > > - vfio_update_pgsize_bitmap(iommu);
> > > - }
> > > - /*
> > > - * Removal of a group without dirty tracking may allow
> > > - * the iommu scope to be promoted.
> > > - */
> > > - if (!group->pinned_page_dirty_scope) {
> > > - iommu->num_non_pinned_groups--;
> > > - if (iommu->dirty_page_tracking)
> > > - vfio_iommu_populate_bitmap_full(iommu);
> > > - }
> > > + vfio_iommu_detach_destroy_domain(domain, iommu,
> > > group);
> > > kfree(group);
> > > break;
> > > }
> > >
> > > + vfio_iommu_aper_expand(iommu, &iova_copy);
> >
> > but now it's done for every group detach. The aperture is decided
> > by domain geometry which is not affected by attached groups.
>
> Yea, I've noticed this part. Actually Jason did this change for
> simplicity, and I think it'd be safe to do so?
Perhaps detach_destroy() can return a Boolean to indicate whether
a domain is destroyed.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/ipmmu-vmsa: Fix compatible for rcar-gen4
Fix compatible string for R-Car Gen4.
Fixes: ae684caf465b ("iommu/ipmmu-vmsa: Add support for R-Car Gen4")
Signed-off-by: Yoshihiro Shimoda
---
drivers/iommu/ipmmu-vmsa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 8fdb84b3642b..1d42084d0276 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -987,7 +987,7 @@ static const struct of_device_id ipmmu_of_ids[] = {
.compatible = "renesas,ipmmu-r8a779a0",
.data = &ipmmu_features_rcar_gen4,
}, {
- .compatible = "renesas,rcar-gen4-ipmmu",
+ .compatible = "renesas,rcar-gen4-ipmmu-vmsa",
.data = &ipmmu_features_rcar_gen4,
}, {
/* Terminator */
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment
On Thu, Jun 16, 2022 at 07:08:10AM +, Tian, Kevin wrote:
> ...
> > +static struct vfio_domain *
> > +vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu
> > *iommu,
> > +struct vfio_iommu_group *group)
> > +{
> > + struct iommu_domain *new_domain;
> > + struct vfio_domain *domain;
> > + int ret = 0;
> > +
> > + /* Try to match an existing compatible domain */
> > + list_for_each_entry (domain, &iommu->domain_list, next) {
> > + ret = iommu_attach_group(domain->domain, group-
> > >iommu_group);
> > + if (ret == -EMEDIUMTYPE)
> > + continue;
>
> Probably good to add one line comment here for what EMEDIUMTYPE
> represents. It's not a widely-used retry type like EAGAIN. A comment
> can save the time of digging out the fact by jumping to iommu file.
Sure. I can add that.
> ...
> > - if (resv_msi) {
> > + if (resv_msi && !domain->msi_cookie) {
> > ret = iommu_get_msi_cookie(domain->domain,
> > resv_msi_base);
> > if (ret && ret != -ENODEV)
> > goto out_detach;
> > + domain->msi_cookie = true;
> > }
>
> why not moving to alloc_attach_domain() then no need for the new
> domain field? It's required only when a new domain is allocated.
When reusing an existing domain that doesn't have an msi_cookie,
we can do iommu_get_msi_cookie() if resv_msi is found. So it is
not limited to a new domain.
> ...
> > - if (list_empty(&domain->group_list)) {
> > - if (list_is_singular(&iommu->domain_list)) {
> > - if (list_empty(&iommu-
> > >emulated_iommu_groups)) {
> > - WARN_ON(iommu->notifier.head);
> > -
> > vfio_iommu_unmap_unpin_all(iommu);
> > - } else {
> > -
> > vfio_iommu_unmap_unpin_reaccount(iommu);
> > - }
> > - }
> > - iommu_domain_free(domain->domain);
> > - list_del(&domain->next);
> > - kfree(domain);
> > - vfio_iommu_aper_expand(iommu, &iova_copy);
>
> Previously the aperture is adjusted when a domain is freed...
>
> > - vfio_update_pgsize_bitmap(iommu);
> > - }
> > - /*
> > - * Removal of a group without dirty tracking may allow
> > - * the iommu scope to be promoted.
> > - */
> > - if (!group->pinned_page_dirty_scope) {
> > - iommu->num_non_pinned_groups--;
> > - if (iommu->dirty_page_tracking)
> > - vfio_iommu_populate_bitmap_full(iommu);
> > - }
> > + vfio_iommu_detach_destroy_domain(domain, iommu,
> > group);
> > kfree(group);
> > break;
> > }
> >
> > + vfio_iommu_aper_expand(iommu, &iova_copy);
>
> but now it's done for every group detach. The aperture is decided
> by domain geometry which is not affected by attached groups.
Yea, I've noticed this part. Actually Jason did this change for
simplicity, and I think it'd be safe to do so?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()
On Thu, Jun 16, 2022 at 06:45:09AM +, Tian, Kevin wrote: > > +out_unlock: > > mutex_unlock(&iommu->lock); > > } > > > > I'd just replace the goto with a direct unlock and then return there. > the readability is slightly better. OK. Will do that. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/5] vfio/iommu_type1: Remove the domain->ops comparison
On Thu, Jun 16, 2022 at 06:40:14AM +, Tian, Kevin wrote: > > The domain->ops validation was added, as a precaution, for mixed-driver > > systems. However, at this moment only one iommu driver is possible. So > > remove it. > > It's true on a physical platform. But I'm not sure whether a virtual platform > is allowed to include multiple e.g. one virtio-iommu alongside a virtual VT-d > or a virtual smmu. It might be clearer to claim that (as Robin pointed out) > there is plenty more significant problems than this to solve instead of simply > saying that only one iommu driver is possible if we don't have explicit code > to reject such configuration. 😊 Will edit this part. Thanks! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/4] dt-bindings: qcom-iommu: Add Qualcomm MSM8953 compatible
On Sun, 12 Jun 2022 11:22:13 +0200, Luca Weiss wrote: > Document the compatible used for IOMMU on the msm8953 SoC. > > Signed-off-by: Luca Weiss > --- > Changes from v1: > - new patch > > Documentation/devicetree/bindings/iommu/qcom,iommu.txt | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/5] iommu/mediatek: Remove a unused "mapping" which is only for v1
On 16/06/2022 07:42, Yong Wu wrote:
Just remove a unused variable that only is for mtk_iommu_v1.
Fixes: 9485a04a5bb9 ("iommu/mediatek: Separate mtk_iommu_data for v1 and v2")
It does not fix a bug, so no fixes tag here needed.
With that:
Reviewed-by: Matthias Brugger
Signed-off-by: Yong Wu
---
drivers/iommu/mtk_iommu.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 5e86fd48928a..e65e705d9fc1 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -221,10 +221,7 @@ struct mtk_iommu_data {
struct device *smicomm_dev;
struct mtk_iommu_bank_data *bank;
-
- struct dma_iommu_mapping*mapping; /* For mtk_iommu_v1.c */
struct regmap *pericfg;
-
struct mutexmutex; /* Protect m4u_group/m4u_dom
above */
/*
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse
On 16/06/2022 07:42, Yong Wu wrote:
The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if the i+1
larb is parsed fail(return -EINVAL), we should of_node_put for the 0..i
larbs. In the fail path, one of_node_put matches with of_parse_phandle in
it.
Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
Signed-off-by: Yong Wu
---
drivers/iommu/mtk_iommu.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 3b2489e8a6dd..ab24078938bf 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1071,12 +1071,12 @@ static int mtk_iommu_mm_dts_parse(struct device *dev,
struct component_match **m
Don't we need to call the goto also on error case of:
larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
Regards,
Matthias
plarbdev = of_find_device_by_node(larbnode);
if (!plarbdev) {
- of_node_put(larbnode);
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_larbnode_put;
}
if (!plarbdev->dev.driver) {
- of_node_put(larbnode);
- return -EPROBE_DEFER;
+ ret = -EPROBE_DEFER;
+ goto err_larbnode_put;
}
data->larb_imu[id].dev = &plarbdev->dev;
@@ -1107,9 +1107,20 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
if (!link) {
dev_err(dev, "Unable to link %s.\n",
dev_name(data->smicomm_dev));
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_larbnode_put;
}
return 0;
+
+err_larbnode_put:
+ while (i--) {
+ larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
+ if (larbnode && of_device_is_available(larbnode)) {
+ of_node_put(larbnode);
+ of_node_put(larbnode);
+ }
+ }
+ return ret;
}
static int mtk_iommu_probe(struct platform_device *pdev)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/5] iommu/mediatek: Use dev_err_probe to mute probe_defer err log
On 16/06/2022 07:41, Yong Wu wrote:
Mute the probe defer log:
[2.654806] mtk-iommu 14018000.iommu: mm dts parse fail(-517).
[2.656168] mtk-iommu 1c01f000.iommu: mm dts parse fail(-517).
Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
Signed-off-by: Yong Wu
Reviewed-by: AngeloGioacchino Del Regno
Reviewed-by: Guenter Roeck
Reviewed-by: Matthias Brugger
---
drivers/iommu/mtk_iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index bb9dd92c9898..3b2489e8a6dd 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1204,7 +1204,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
ret = mtk_iommu_mm_dts_parse(dev, &match, data);
if (ret) {
- dev_err(dev, "mm dts parse fail(%d).", ret);
+ dev_err_probe(dev, ret, "mm dts parse fail.");
goto out_runtime_disable;
}
} else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA) &&
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 5/5] iommu/mediatek: Cleanup pericfg lookup flow
On 16/06/2022 13:08, AngeloGioacchino Del Regno wrote:
Since only the INFRA type IOMMU needs to modify register(s) in the
pericfg iospace, it's safe to drop the pericfg_comp_str NULL check;
also, directly assign the regmap handle to data->pericfg instead of
to the infracfg variable to improve code readability.
Signed-off-by: AngeloGioacchino Del Regno
Reviewed-by: Matthias Brugger
---
drivers/iommu/mtk_iommu.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 90685946fcbe..b2ae84046249 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1217,15 +1217,13 @@ static int mtk_iommu_probe(struct platform_device *pdev)
dev_err(dev, "mm dts parse fail(%d).", ret);
goto out_runtime_disable;
}
- } else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA) &&
- data->plat_data->pericfg_comp_str) {
- infracfg =
syscon_regmap_lookup_by_compatible(data->plat_data->pericfg_comp_str);
- if (IS_ERR(infracfg)) {
- ret = PTR_ERR(infracfg);
+ } else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA)) {
+ p = data->plat_data->pericfg_comp_str;
+ data->pericfg = syscon_regmap_lookup_by_compatible(p);
+ if (IS_ERR(data->pericfg)) {
+ ret = PTR_ERR(data->pericfg);
goto out_runtime_disable;
}
-
- data->pericfg = infracfg;
}
platform_set_drvdata(pdev, data);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/5] dt-bindings: iommu: mediatek: Add mediatek,infracfg phandle
On 16/06/2022 13:08, AngeloGioacchino Del Regno wrote: Add property "mediatek,infracfg" to let the mtk_iommu driver retrieve a phandle to the infracfg syscon instead of performing a per-soc compatible lookup in the entire devicetree and set it as a required property for MT2712 and MT8173. Signed-off-by: AngeloGioacchino Del Regno Reviewed-by: Matthias Brugger --- .../bindings/iommu/mediatek,iommu.yaml | 17 + 1 file changed, 17 insertions(+) diff --git a/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml b/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml index 2ae3bbad7f1a..fee0241b5098 100644 --- a/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml +++ b/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml @@ -101,6 +101,10 @@ properties: items: - const: bclk + mediatek,infracfg: +$ref: /schemas/types.yaml#/definitions/phandle +description: The phandle to the mediatek infracfg syscon + mediatek,larbs: $ref: /schemas/types.yaml#/definitions/phandle-array minItems: 1 @@ -167,6 +171,18 @@ allOf: required: - power-domains + - if: + properties: +compatible: + contains: +enum: + - mediatek,mt2712-m4u + - mediatek,mt8173-m4u + +then: + required: +- mediatek,infracfg + - if: # The IOMMUs don't have larbs. not: properties: @@ -191,6 +207,7 @@ examples: interrupts = ; clocks = <&infracfg CLK_INFRA_M4U>; clock-names = "bclk"; +mediatek,infracfg = <&infracfg>; mediatek,larbs = <&larb0>, <&larb1>, <&larb2>, <&larb3>, <&larb4>, <&larb5>; #iommu-cells = <1>; ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v10 1/2] iommu/io-pgtable-arm-v7s: Add a quirk to allow pgtable PA up to 35bit
From: Yunfei Wang
Single memory zone feature will remove ZONE_DMA32 and ZONE_DMA and
cause pgtable PA size larger than 32bit.
Since Mediatek IOMMU hardware support at most 35bit PA in pgtable,
so add a quirk to allow the PA of pgtables support up to bit35.
Signed-off-by: Ning Li
Signed-off-by: Yunfei Wang
---
drivers/iommu/io-pgtable-arm-v7s.c | 67 +++---
include/linux/io-pgtable.h | 15 ---
2 files changed, 63 insertions(+), 19 deletions(-)
diff --git a/drivers/iommu/io-pgtable-arm-v7s.c
b/drivers/iommu/io-pgtable-arm-v7s.c
index be066c1503d3..47b7d7726437 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -182,14 +182,8 @@ static bool arm_v7s_is_mtk_enabled(struct io_pgtable_cfg
*cfg)
(cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT);
}
-static arm_v7s_iopte paddr_to_iopte(phys_addr_t paddr, int lvl,
- struct io_pgtable_cfg *cfg)
+static arm_v7s_iopte to_mtk_iopte(phys_addr_t paddr, arm_v7s_iopte pte)
{
- arm_v7s_iopte pte = paddr & ARM_V7S_LVL_MASK(lvl);
-
- if (!arm_v7s_is_mtk_enabled(cfg))
- return pte;
-
if (paddr & BIT_ULL(32))
pte |= ARM_V7S_ATTR_MTK_PA_BIT32;
if (paddr & BIT_ULL(33))
@@ -199,6 +193,17 @@ static arm_v7s_iopte paddr_to_iopte(phys_addr_t paddr, int
lvl,
return pte;
}
+static arm_v7s_iopte paddr_to_iopte(phys_addr_t paddr, int lvl,
+ struct io_pgtable_cfg *cfg)
+{
+ arm_v7s_iopte pte = paddr & ARM_V7S_LVL_MASK(lvl);
+
+ if (arm_v7s_is_mtk_enabled(cfg))
+ return to_mtk_iopte(paddr, pte);
+
+ return pte;
+}
+
static phys_addr_t iopte_to_paddr(arm_v7s_iopte pte, int lvl,
struct io_pgtable_cfg *cfg)
{
@@ -240,10 +245,17 @@ static void *__arm_v7s_alloc_table(int lvl, gfp_t gfp,
dma_addr_t dma;
size_t size = ARM_V7S_TABLE_SIZE(lvl, cfg);
void *table = NULL;
+ gfp_t gfp_l1;
+
+ /*
+* ARM_MTK_TTBR_EXT extend the translation table base support larger
+* memory address.
+*/
+ gfp_l1 = cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT ?
+GFP_KERNEL : ARM_V7S_TABLE_GFP_DMA;
if (lvl == 1)
- table = (void *)__get_free_pages(
- __GFP_ZERO | ARM_V7S_TABLE_GFP_DMA, get_order(size));
+ table = (void *)__get_free_pages(gfp_l1 | __GFP_ZERO,
get_order(size));
else if (lvl == 2)
table = kmem_cache_zalloc(data->l2_tables, gfp);
@@ -251,7 +263,8 @@ static void *__arm_v7s_alloc_table(int lvl, gfp_t gfp,
return NULL;
phys = virt_to_phys(table);
- if (phys != (arm_v7s_iopte)phys) {
+ if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT ?
+ phys >= (1ULL << cfg->oas) : phys != (arm_v7s_iopte)phys) {
/* Doesn't fit in PTE */
dev_err(dev, "Page table does not fit in PTE: %pa", &phys);
goto out_free;
@@ -457,9 +470,14 @@ static arm_v7s_iopte arm_v7s_install_table(arm_v7s_iopte
*table,
arm_v7s_iopte curr,
struct io_pgtable_cfg *cfg)
{
+ phys_addr_t phys = virt_to_phys(table);
arm_v7s_iopte old, new;
- new = virt_to_phys(table) | ARM_V7S_PTE_TYPE_TABLE;
+ new = phys | ARM_V7S_PTE_TYPE_TABLE;
+
+ if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT)
+ new = to_mtk_iopte(phys, new);
+
if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS)
new |= ARM_V7S_ATTR_NS_TABLE;
@@ -779,6 +797,8 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct
io_pgtable_cfg *cfg,
void *cookie)
{
struct arm_v7s_io_pgtable *data;
+ slab_flags_t slab_flag;
+ phys_addr_t paddr;
if (cfg->ias > (arm_v7s_is_mtk_enabled(cfg) ? 34 : ARM_V7S_ADDR_BITS))
return NULL;
@@ -788,7 +808,8 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct
io_pgtable_cfg *cfg,
if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |
IO_PGTABLE_QUIRK_NO_PERMS |
- IO_PGTABLE_QUIRK_ARM_MTK_EXT))
+ IO_PGTABLE_QUIRK_ARM_MTK_EXT |
+ IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT))
return NULL;
/* If ARM_MTK_4GB is enabled, the NO_PERMS is also expected. */
@@ -796,15 +817,27 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct
io_pgtable_cfg *cfg,
!(cfg->quirks & IO_PGTABLE_QUIRK_NO_PERMS))
return NULL;
+ if ((cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT) &&
+ !arm_v7s_is_mtk_enabled(cfg))
+ return NULL;
+
data = kmalloc(sizeof(*data), GFP_KERNEL);
if (!data)
[PATCH v10 2/2] iommu/mediatek: Allow page table PA up to 35bit
From: Yunfei Wang
Single memory zone feature will remove ZONE_DMA32 and ZONE_DMA. So add
the quirk IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT to let level 1 and level 2
pgtable support at most 35bit PA.
Signed-off-by: Ning Li
Signed-off-by: Yunfei Wang
---
drivers/iommu/mtk_iommu.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index bb9dd92c9898..372a15990a65 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -138,6 +138,7 @@
/* PM and clock always on. e.g. infra iommu */
#define PM_CLK_AO BIT(15)
#define IFA_IOMMU_PCIE_SUPPORT BIT(16)
+#define PGTABLE_PA_35_EN BIT(17)
#define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask) \
pdata)->flags) & (mask)) == (_x))
@@ -240,6 +241,7 @@ struct mtk_iommu_data {
struct mtk_iommu_domain {
struct io_pgtable_cfg cfg;
struct io_pgtable_ops *iop;
+ u32 ttbr;
struct mtk_iommu_bank_data *bank;
struct iommu_domain domain;
@@ -583,6 +585,7 @@ static int mtk_iommu_domain_finalise(struct
mtk_iommu_domain *dom,
if (m4u_dom) {
dom->iop = m4u_dom->iop;
dom->cfg = m4u_dom->cfg;
+ dom->ttbr = m4u_dom->ttbr;
dom->domain.pgsize_bitmap = m4u_dom->cfg.pgsize_bitmap;
goto update_iova_region;
}
@@ -596,6 +599,9 @@ static int mtk_iommu_domain_finalise(struct
mtk_iommu_domain *dom,
.iommu_dev = data->dev,
};
+ if (MTK_IOMMU_HAS_FLAG(data->plat_data, PGTABLE_PA_35_EN))
+ dom->cfg.quirks |= IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT;
+
if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_4GB_MODE))
dom->cfg.oas = data->enable_4GB ? 33 : 32;
else
@@ -606,6 +612,9 @@ static int mtk_iommu_domain_finalise(struct
mtk_iommu_domain *dom,
dev_err(data->dev, "Failed to alloc io pgtable\n");
return -EINVAL;
}
+ dom->ttbr = dom->cfg.quirks & IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT ?
+ dom->cfg.arm_v7s_cfg.ttbr :
+ dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK;
/* Update our support page sizes bitmap */
dom->domain.pgsize_bitmap = dom->cfg.pgsize_bitmap;
@@ -684,8 +693,7 @@ static int mtk_iommu_attach_device(struct iommu_domain
*domain,
goto err_unlock;
}
bank->m4u_dom = dom;
- writel(dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK,
- bank->base + REG_MMU_PT_BASE_ADDR);
+ writel(bank->m4u_dom->ttbr, bank->base + REG_MMU_PT_BASE_ADDR);
pm_runtime_put(m4udev);
}
@@ -1366,8 +1374,7 @@ static int __maybe_unused mtk_iommu_runtime_resume(struct
device *dev)
writel_relaxed(reg->int_control[i], base +
REG_MMU_INT_CONTROL0);
writel_relaxed(reg->int_main_control[i], base +
REG_MMU_INT_MAIN_CONTROL);
writel_relaxed(reg->ivrp_paddr[i], base + REG_MMU_IVRP_PADDR);
- writel(m4u_dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK,
- base + REG_MMU_PT_BASE_ADDR);
+ writel(m4u_dom->ttbr, base + REG_MMU_PT_BASE_ADDR);
} while (++i < data->plat_data->banks_num);
/*
@@ -1401,7 +1408,7 @@ static const struct mtk_iommu_plat_data mt2712_data = {
static const struct mtk_iommu_plat_data mt6779_data = {
.m4u_plat = M4U_MT6779,
.flags = HAS_SUB_COMM_2BITS | OUT_ORDER_WR_EN | WR_THROT_EN |
-MTK_IOMMU_TYPE_MM,
+MTK_IOMMU_TYPE_MM | PGTABLE_PA_35_EN,
.inv_sel_reg = REG_MMU_INV_SEL_GEN2,
.banks_num= 1,
.banks_enable = {true},
--
2.18.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/5] iommu/mediatek: Remove a unused "mapping" which is only for v1
Il 16/06/22 07:42, Yong Wu ha scritto:
Just remove a unused variable that only is for mtk_iommu_v1.
Fixes: 9485a04a5bb9 ("iommu/mediatek: Separate mtk_iommu_data for v1 and v2")
Signed-off-by: Yong Wu
The title isn't immediately clear, looks like you're removing some mapping, not
a struct member...
Perhaps... iommu/mediatek: Remove unused "mapping" member from mtk_iommu_data ?
After clarifying the commit title:
Reviewed-by: AngeloGioacchino Del Regno
Cheers,
Angelo
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 5/5] iommu/mediatek: Cleanup pericfg lookup flow
Since only the INFRA type IOMMU needs to modify register(s) in the
pericfg iospace, it's safe to drop the pericfg_comp_str NULL check;
also, directly assign the regmap handle to data->pericfg instead of
to the infracfg variable to improve code readability.
Signed-off-by: AngeloGioacchino Del Regno
---
drivers/iommu/mtk_iommu.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 90685946fcbe..b2ae84046249 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1217,15 +1217,13 @@ static int mtk_iommu_probe(struct platform_device *pdev)
dev_err(dev, "mm dts parse fail(%d).", ret);
goto out_runtime_disable;
}
- } else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA) &&
- data->plat_data->pericfg_comp_str) {
- infracfg =
syscon_regmap_lookup_by_compatible(data->plat_data->pericfg_comp_str);
- if (IS_ERR(infracfg)) {
- ret = PTR_ERR(infracfg);
+ } else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA)) {
+ p = data->plat_data->pericfg_comp_str;
+ data->pericfg = syscon_regmap_lookup_by_compatible(p);
+ if (IS_ERR(data->pericfg)) {
+ ret = PTR_ERR(data->pericfg);
goto out_runtime_disable;
}
-
- data->pericfg = infracfg;
}
platform_set_drvdata(pdev, data);
--
2.35.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 2/5] iommu/mediatek: Lookup phandle to retrieve syscon to infracfg
This driver will get support for more SoCs and the list of infracfg
compatibles is expected to grow: in order to prevent getting this
situation out of control and see a long list of compatible strings,
add support to retrieve a handle to infracfg's regmap through a
new "mediatek,infracfg" phandle.
In order to keep retrocompatibility with older devicetrees, the old
way is kept in place.
Signed-off-by: AngeloGioacchino Del Regno
Reviewed-by: Miles Chen
Reviewed-by: Yong Wu
Reviewed-by: Matthias Brugger
---
drivers/iommu/mtk_iommu.c | 38 --
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index bb9dd92c9898..90685946fcbe 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1140,22 +1140,32 @@ static int mtk_iommu_probe(struct platform_device *pdev)
data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_4GB_MODE)) {
- switch (data->plat_data->m4u_plat) {
- case M4U_MT2712:
- p = "mediatek,mt2712-infracfg";
- break;
- case M4U_MT8173:
- p = "mediatek,mt8173-infracfg";
- break;
- default:
- p = NULL;
+ infracfg = syscon_regmap_lookup_by_phandle(dev->of_node,
"mediatek,infracfg");
+ if (IS_ERR(infracfg)) {
+ /*
+* Legacy devicetrees will not specify a phandle to
+* mediatek,infracfg: in that case, we use the older
+* way to retrieve a syscon to infra.
+*
+* This is for retrocompatibility purposes only, hence
+* no more compatibles shall be added to this.
+*/
+ switch (data->plat_data->m4u_plat) {
+ case M4U_MT2712:
+ p = "mediatek,mt2712-infracfg";
+ break;
+ case M4U_MT8173:
+ p = "mediatek,mt8173-infracfg";
+ break;
+ default:
+ p = NULL;
+ }
+
+ infracfg = syscon_regmap_lookup_by_compatible(p);
+ if (IS_ERR(infracfg))
+ return PTR_ERR(infracfg);
}
- infracfg = syscon_regmap_lookup_by_compatible(p);
-
- if (IS_ERR(infracfg))
- return PTR_ERR(infracfg);
-
ret = regmap_read(infracfg, REG_INFRA_MISC, &val);
if (ret)
return ret;
--
2.35.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 4/5] arm64: dts: mediatek: mt2712e: Add mediatek, infracfg phandle for IOMMU
The IOMMU driver now looks for the "mediatek,infracfg" phandle as a
new way to retrieve a syscon to that:
even though the old way is retained, it has been deprecated and the
driver will write a message in kmsg advertising to use the phandle
way instead.
For this reason, assign the right phandle to mediatek,infracfg in
the iommu node.
Signed-off-by: AngeloGioacchino Del Regno
Reviewed-by: Miles Chen
---
arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
index 623eb3beabf2..4797537cb368 100644
--- a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
@@ -329,6 +329,7 @@ iommu0: iommu@10205000 {
interrupts = ;
clocks = <&infracfg CLK_INFRA_M4U>;
clock-names = "bclk";
+ mediatek,infracfg = <&infracfg>;
mediatek,larbs = <&larb0>, <&larb1>, <&larb2>,
<&larb3>, <&larb6>;
#iommu-cells = <1>;
@@ -346,6 +347,7 @@ iommu1: iommu@1020a000 {
interrupts = ;
clocks = <&infracfg CLK_INFRA_M4U>;
clock-names = "bclk";
+ mediatek,infracfg = <&infracfg>;
mediatek,larbs = <&larb4>, <&larb5>, <&larb7>;
#iommu-cells = <1>;
};
--
2.35.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 3/5] arm64: dts: mediatek: mt8173: Add mediatek, infracfg phandle for IOMMU
The IOMMU driver now looks for the "mediatek,infracfg" phandle as a
new way to retrieve a syscon to that:
even though the old way is retained, it has been deprecated and the
driver will write a message in kmsg advertising to use the phandle
way instead.
For this reason, assign the right phandle to mediatek,infracfg in
the iommu node.
Signed-off-by: AngeloGioacchino Del Regno
Reviewed-by: Miles Chen
---
arch/arm64/boot/dts/mediatek/mt8173.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index e14b6e68c4df..b6f65c688f02 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -581,6 +581,7 @@ iommu: iommu@10205000 {
interrupts = ;
clocks = <&infracfg CLK_INFRA_M4U>;
clock-names = "bclk";
+ mediatek,infracfg = <&infracfg>;
mediatek,larbs = <&larb0>, <&larb1>, <&larb2>,
<&larb3>, <&larb4>, <&larb5>;
#iommu-cells = <1>;
--
2.35.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 1/5] dt-bindings: iommu: mediatek: Add mediatek, infracfg phandle
Add property "mediatek,infracfg" to let the mtk_iommu driver retrieve a phandle to the infracfg syscon instead of performing a per-soc compatible lookup in the entire devicetree and set it as a required property for MT2712 and MT8173. Signed-off-by: AngeloGioacchino Del Regno --- .../bindings/iommu/mediatek,iommu.yaml | 17 + 1 file changed, 17 insertions(+) diff --git a/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml b/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml index 2ae3bbad7f1a..fee0241b5098 100644 --- a/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml +++ b/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml @@ -101,6 +101,10 @@ properties: items: - const: bclk + mediatek,infracfg: +$ref: /schemas/types.yaml#/definitions/phandle +description: The phandle to the mediatek infracfg syscon + mediatek,larbs: $ref: /schemas/types.yaml#/definitions/phandle-array minItems: 1 @@ -167,6 +171,18 @@ allOf: required: - power-domains + - if: + properties: +compatible: + contains: +enum: + - mediatek,mt2712-m4u + - mediatek,mt8173-m4u + +then: + required: +- mediatek,infracfg + - if: # The IOMMUs don't have larbs. not: properties: @@ -191,6 +207,7 @@ examples: interrupts = ; clocks = <&infracfg CLK_INFRA_M4U>; clock-names = "bclk"; +mediatek,infracfg = <&infracfg>; mediatek,larbs = <&larb0>, <&larb1>, <&larb2>, <&larb3>, <&larb4>, <&larb5>; #iommu-cells = <1>; -- 2.35.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 0/5] mtk_iommu: Specify phandles to infracfg and pericfg
The IOMMU has registers in the infracfg and/or pericfg iospaces: as for the currently supported SoCs, MT2712 and MT8173 need a phandle to infracfg, while MT8195 needs one to pericfg. Before this change, the driver was checking for a SoC-specific infra/peri compatible but, sooner or later, these lists are going to grow a lot... ...and this is why it was chosen to add phandles (as it was done with some other drivers already - look at mtk-pm-domains, mt8192-afe Please note that, while it was necessary to update the devicetrees for MT8173 and MT2712e, there was no update for MT8195 because there is no IOMMU node in there yet. Changes in v4: - Dropped changes introducing mediatek,pericfg handle - Fixed required property in IOMMU example in patch [1/5] - Added a pericfg lookup flow cleanup commit Changes in v3: - Different squashing of dt-bindings patches (sorry for misunderstanding!) - Removed legacy devicetree print Changes in v2: - Squashed dt-bindings patches as suggested by Matthias - Removed quotes from infra/peri phandle refs - Changed dev_warn to dev_info in patches [2/7], [3/7] AngeloGioacchino Del Regno (5): dt-bindings: iommu: mediatek: Add mediatek,infracfg phandle iommu/mediatek: Lookup phandle to retrieve syscon to infracfg arm64: dts: mediatek: mt8173: Add mediatek,infracfg phandle for IOMMU arm64: dts: mediatek: mt2712e: Add mediatek,infracfg phandle for IOMMU iommu/mediatek: Cleanup pericfg lookup flow .../bindings/iommu/mediatek,iommu.yaml| 17 +++ arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 2 + arch/arm64/boot/dts/mediatek/mt8173.dtsi | 1 + drivers/iommu/mtk_iommu.c | 50 +++ 4 files changed, 49 insertions(+), 21 deletions(-) -- 2.35.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse
On 2022-06-16 11:08, Yong Wu wrote:
On Thu, 2022-06-16 at 09:59 +0100, Robin Murphy wrote:
On 2022-06-16 06:42, Yong Wu wrote:
The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if the
i+1
larb is parsed fail(return -EINVAL), we should of_node_put for the
0..i
larbs. In the fail path, one of_node_put matches with
of_parse_phandle in
it.
Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with
the MM TYPE")
Signed-off-by: Yong Wu
---
drivers/iommu/mtk_iommu.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 3b2489e8a6dd..ab24078938bf 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1071,12 +1071,12 @@ static int mtk_iommu_mm_dts_parse(struct
device *dev, struct component_match **m
plarbdev = of_find_device_by_node(larbnode);
if (!plarbdev) {
- of_node_put(larbnode);
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_larbnode_put;
}
if (!plarbdev->dev.driver) {
- of_node_put(larbnode);
- return -EPROBE_DEFER;
+ ret = -EPROBE_DEFER;
+ goto err_larbnode_put;
}
data->larb_imu[id].dev = &plarbdev->dev;
@@ -1107,9 +1107,20 @@ static int mtk_iommu_mm_dts_parse(struct
device *dev, struct component_match **m
DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
if (!link) {
dev_err(dev, "Unable to link %s.\n", dev_name(data-
smicomm_dev));
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_larbnode_put;
}
return 0;
+
+err_larbnode_put:
+ while (i--) {
+ larbnode = of_parse_phandle(dev->of_node,
"mediatek,larbs", i);
+ if (larbnode && of_device_is_available(larbnode)) {
+ of_node_put(larbnode);
+ of_node_put(larbnode);
+ }
This looks a bit awkward - could we not just iterate through
data->larb_imu and put dev->of_node for each valid dev?
It should work. Thanks very much.
Also, of_find_device_by_node() takes a reference on the struct
device
itself, so strictly we should be doing put_device() on those as well
if we're bailing out.
Thanks for this hint. A new reference for me. I will add it.
In fact, thinking about it some more we may as well do the of_node_put()
unconditionally immediately after the of_find_device_by_node() call, so
then it's *only* the device references we'd need to worry about cleaning
up in the failure path.
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse
On Thu, 2022-06-16 at 09:59 +0100, Robin Murphy wrote:
> On 2022-06-16 06:42, Yong Wu wrote:
> > The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if the
> > i+1
> > larb is parsed fail(return -EINVAL), we should of_node_put for the
> > 0..i
> > larbs. In the fail path, one of_node_put matches with
> > of_parse_phandle in
> > it.
> >
> > Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with
> > the MM TYPE")
> > Signed-off-by: Yong Wu
> > ---
> > drivers/iommu/mtk_iommu.c | 21 -
> > 1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 3b2489e8a6dd..ab24078938bf 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -1071,12 +1071,12 @@ static int mtk_iommu_mm_dts_parse(struct
> > device *dev, struct component_match **m
> >
> > plarbdev = of_find_device_by_node(larbnode);
> > if (!plarbdev) {
> > - of_node_put(larbnode);
> > - return -ENODEV;
> > + ret = -ENODEV;
> > + goto err_larbnode_put;
> > }
> > if (!plarbdev->dev.driver) {
> > - of_node_put(larbnode);
> > - return -EPROBE_DEFER;
> > + ret = -EPROBE_DEFER;
> > + goto err_larbnode_put;
> > }
> > data->larb_imu[id].dev = &plarbdev->dev;
> >
> > @@ -1107,9 +1107,20 @@ static int mtk_iommu_mm_dts_parse(struct
> > device *dev, struct component_match **m
> >DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
> > if (!link) {
> > dev_err(dev, "Unable to link %s.\n", dev_name(data-
> > >smicomm_dev));
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto err_larbnode_put;
> > }
> > return 0;
> > +
> > +err_larbnode_put:
> > + while (i--) {
> > + larbnode = of_parse_phandle(dev->of_node,
> > "mediatek,larbs", i);
> > + if (larbnode && of_device_is_available(larbnode)) {
> > + of_node_put(larbnode);
> > + of_node_put(larbnode);
> > + }
>
> This looks a bit awkward - could we not just iterate through
> data->larb_imu and put dev->of_node for each valid dev?
It should work. Thanks very much.
>
> Also, of_find_device_by_node() takes a reference on the struct
> device
> itself, so strictly we should be doing put_device() on those as well
> if we're bailing out.
Thanks for this hint. A new reference for me. I will add it.
>
> Robin.
>
> > + }
> > + return ret;
> > }
> >
> > static int mtk_iommu_probe(struct platform_device *pdev)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse
On 2022-06-16 06:42, Yong Wu wrote:
The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if the i+1
larb is parsed fail(return -EINVAL), we should of_node_put for the 0..i
larbs. In the fail path, one of_node_put matches with of_parse_phandle in
it.
Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
Signed-off-by: Yong Wu
---
drivers/iommu/mtk_iommu.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 3b2489e8a6dd..ab24078938bf 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1071,12 +1071,12 @@ static int mtk_iommu_mm_dts_parse(struct device *dev,
struct component_match **m
plarbdev = of_find_device_by_node(larbnode);
if (!plarbdev) {
- of_node_put(larbnode);
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_larbnode_put;
}
if (!plarbdev->dev.driver) {
- of_node_put(larbnode);
- return -EPROBE_DEFER;
+ ret = -EPROBE_DEFER;
+ goto err_larbnode_put;
}
data->larb_imu[id].dev = &plarbdev->dev;
@@ -1107,9 +1107,20 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
if (!link) {
dev_err(dev, "Unable to link %s.\n",
dev_name(data->smicomm_dev));
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_larbnode_put;
}
return 0;
+
+err_larbnode_put:
+ while (i--) {
+ larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
+ if (larbnode && of_device_is_available(larbnode)) {
+ of_node_put(larbnode);
+ of_node_put(larbnode);
+ }
This looks a bit awkward - could we not just iterate through
data->larb_imu and put dev->of_node for each valid dev?
Also, of_find_device_by_node() takes a reference on the struct device
itself, so strictly we should be doing put_device() on those as well if
we're bailing out.
Robin.
+ }
+ return ret;
}
static int mtk_iommu_probe(struct platform_device *pdev)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 6/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to pericfg
Il 16/06/22 08:30, Yong Wu ha scritto: On Mon, 2022-06-13 at 10:13 +0200, AngeloGioacchino Del Regno wrote: Il 13/06/22 07:32, Yong Wu ha scritto: On Thu, 2022-06-09 at 12:08 +0200, AngeloGioacchino Del Regno wrote: On some SoCs (of which only MT8195 is supported at the time of writing), the "R" and "W" (I/O) enable bits for the IOMMUs are in the pericfg_ao register space and not in the IOMMU space: as it happened already with infracfg, it is expected that this list will grow. Currently I don't see the list will grow. As commented before, In the lastest SoC, The IOMMU enable bits for IOMMU will be in ATF, rather than in this pericfg register region. In this case, Is this patch unnecessary? or we could add this patch when there are 2 SoCs use this setting at least? what's your opinion? Perhaps I've misunderstood... besides, can you please check if there's any other SoC (not just chromebooks, also smartphone SoCs) that need this logic? As far as I know, SmartPhone SoCs don't enable the infra iommu until now. they don't have this logic. I don't object this patch, I think we could add it when at least 2 SoCs need this. Thanks very much for help improving here. Many thanks for checking that! Now that everything is clear, I can safely go on with pushing a v4 of this series. Thanks again! Angelo ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] uacce: fix concurrency of fops_open and uacce_remove
On Thu, Jun 16, 2022 at 12:10:18PM +0800, Zhangfei Gao wrote:
> > > diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> > > index 281c54003edc..b6219c6bfb48 100644
> > > --- a/drivers/misc/uacce/uacce.c
> > > +++ b/drivers/misc/uacce/uacce.c
> > > @@ -136,9 +136,16 @@ static int uacce_fops_open(struct inode *inode,
> > > struct file *filep)
> > > if (!q)
> > > return -ENOMEM;
> > > + mutex_lock(&uacce->queues_lock);
> > > +
> > > + if (!uacce->parent->driver) {
> > I don't think this is useful, because the core clears parent->driver after
> > having run uacce_remove():
> >
> >rmmod hisi_zip open()
> > ... uacce_fops_open()
> > __device_release_driver() ...
> > pci_device_remove()
> > hisi_zip_remove()
> >hisi_qm_uninit()
> > uacce_remove()
> > ... ...
> > mutex_lock(uacce->queues_lock)
> > ... if (!uacce->parent->driver)
> > device_unbind_cleanup() /* driver still valid, proceed */
> > dev->driver = NULL
>
> The check if (!uacce->parent->driver) is required, otherwise NULL pointer
> may happen.
I agree we need something, what I mean is that this check is not
sufficient.
> iommu_sva_bind_device
> const struct iommu_ops *ops = dev_iommu_ops(dev); ->
> dev->iommu->iommu_dev->ops
>
> rmmod has no issue, but remove parent pci device has the issue.
Ah right, relying on the return value of bind() wouldn't be enough even if
we mandated SVA.
[...]
> >
> > I think we need the global uacce_mutex to serialize uacce_remove() and
> > uacce_fops_open(). uacce_remove() would do everything, including
> > xa_erase(), while holding that mutex. And uacce_fops_open() would try to
> > obtain the uacce object from the xarray while holding the mutex, which
> > fails if the uacce object is being removed.
>
> Since fops_open get char device refcount, uacce_release will not happen
> until open returns.
The refcount only ensures that the uacce_device object is not freed as
long as there are open fds. But uacce_remove() can run while there are
open fds, or fds in the process of being opened. And atfer uacce_remove()
runs, the uacce_device object still exists but is mostly unusable. For
example once the module is freed, uacce->ops is not valid anymore. But
currently uacce_fops_open() may dereference the ops in this case:
uacce_fops_open()
if (!uacce->parent->driver)
/* Still valid, keep going */
...rmmod
uacce_remove()
... free_module()
uacce->ops->get_queue() /* BUG */
Accessing uacce->ops after free_module() is a use-after-free. We need all
the fops to synchronize with uacce_remove() to ensure they don't use any
resource of the parent after it's been freed.
I see uacce_fops_poll() may have the same problem, and should be inside
uacce_mutex.
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 3/3] iommu/mediatek: Allow page table PA up to 35bit
On Wed, 2022-06-15 at 18:25 +0100, Robin Murphy wrote:
> On 2022-06-15 17:12, yf.wang--- via iommu wrote:
> > From: Yunfei Wang
> >
> > Single memory zone feature will remove ZONE_DMA32 and ZONE_DMA. So
> > add
> > the quirk IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT to let level 1 and
> > level 2
> > pgtable support at most 35bit PA.
>
> I'm not sure how this works in practice, given that you don't seem to
> be
> setting the IOMMU's own DMA masks to more than 32 bits, so the DMA
> mapping in io-pgtable is going to fail if you ever do actually
> allocate
> a pagetable page above 4GB :/
>
Hi Robin,
About DMA masks, the master device has set dma mask, when iommu do dma map,
it will check the dam_mask of the master dev to determine which range the
address of the iova alloc is in.
Add the quirk ARM_MTK_TTBR_EXT to let pgtable support larger phys memory.
Do actually test work fine, example:
pgtable phys address:0x12054006a, encoded to 32 bits ttbr:0x20540001
> > Signed-off-by: Ning Li
> > Signed-off-by: Yunfei Wang
> > ---
> > drivers/iommu/mtk_iommu.c | 14 +-
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 3d62399e8865..4dbc33758711 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -138,6 +138,7 @@
> > /* PM and clock always on. e.g. infra iommu */
> > #define PM_CLK_AO BIT(15)
> > #define IFA_IOMMU_PCIE_SUPPORTBIT(16)
> > +#define PGTABLE_PA_35_EN BIT(17)
> >
> > #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask) \
> > pdata)->flags) & (mask)) == (_x))
> > @@ -240,6 +241,7 @@ struct mtk_iommu_data {
> > struct mtk_iommu_domain {
> > struct io_pgtable_cfg cfg;
> > struct io_pgtable_ops *iop;
> > + u32 ttbr;
> >
> > struct mtk_iommu_bank_data *bank;
> > struct iommu_domain domain;
> > @@ -596,6 +598,9 @@ static int mtk_iommu_domain_finalise(struct
> > mtk_iommu_domain *dom,
> > .iommu_dev = data->dev,
> > };
> >
> > + if (MTK_IOMMU_HAS_FLAG(data->plat_data, PGTABLE_PA_35_EN))
> > + dom->cfg.quirks |= IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT;
> > +
> > if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_4GB_MODE))
> > dom->cfg.oas = data->enable_4GB ? 33 : 32;
> > else
> > @@ -684,8 +689,8 @@ static int mtk_iommu_attach_device(struct
> > iommu_domain *domain,
> > goto err_unlock;
> > }
> > bank->m4u_dom = dom;
> > - writel(dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK,
> > - bank->base + REG_MMU_PT_BASE_ADDR);
> > + bank->m4u_dom->ttbr = MTK_IOMMU_ADDR(dom-
> > >cfg.arm_v7s_cfg.ttbr);
> > + writel(bank->m4u_dom->ttbr, data->base +
> > REG_MMU_PT_BASE_ADDR);
>
> To add to my comment on patch #1, having to make this change here
> further indicates that you're using it the wrong way.
>
> Thanks,
> Robin.
>
Hi Robin,
According to your Path#1's suggestion, next version will keep ttbr to encoded
32 bits,
then will don't need to modify it.
Thanks,
Yunfei.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment
> From: Nicolin Chen
> Sent: Thursday, June 16, 2022 8:03 AM
>
> Un-inline the domain specific logic from the attach/detach_group ops into
> two paired functions vfio_iommu_alloc_attach_domain() and
> vfio_iommu_detach_destroy_domain() that strictly deal with creating and
> destroying struct vfio_domains.
>
> Add the logic to check for EMEDIUMTYPE return code of
> iommu_attach_group()
> and avoid the extra domain allocations and attach/detach sequences of the
> old code. This allows properly detecting an actual attach error, like
> -ENOMEM, vs treating all attach errors as an incompatible domain.
>
> Co-developed-by: Jason Gunthorpe
> Signed-off-by: Jason Gunthorpe
> Signed-off-by: Nicolin Chen
> ---
> drivers/vfio/vfio_iommu_type1.c | 298 +---
> 1 file changed, 163 insertions(+), 135 deletions(-)
>
...
> +static struct vfio_domain *
> +vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu
> *iommu,
> +struct vfio_iommu_group *group)
> +{
> + struct iommu_domain *new_domain;
> + struct vfio_domain *domain;
> + int ret = 0;
> +
> + /* Try to match an existing compatible domain */
> + list_for_each_entry (domain, &iommu->domain_list, next) {
> + ret = iommu_attach_group(domain->domain, group-
> >iommu_group);
> + if (ret == -EMEDIUMTYPE)
> + continue;
Probably good to add one line comment here for what EMEDIUMTYPE
represents. It's not a widely-used retry type like EAGAIN. A comment
can save the time of digging out the fact by jumping to iommu file.
...
> - if (resv_msi) {
> + if (resv_msi && !domain->msi_cookie) {
> ret = iommu_get_msi_cookie(domain->domain,
> resv_msi_base);
> if (ret && ret != -ENODEV)
> goto out_detach;
> + domain->msi_cookie = true;
> }
why not moving to alloc_attach_domain() then no need for the new
domain field? It's required only when a new domain is allocated.
...
> - if (list_empty(&domain->group_list)) {
> - if (list_is_singular(&iommu->domain_list)) {
> - if (list_empty(&iommu-
> >emulated_iommu_groups)) {
> - WARN_ON(iommu->notifier.head);
> -
> vfio_iommu_unmap_unpin_all(iommu);
> - } else {
> -
> vfio_iommu_unmap_unpin_reaccount(iommu);
> - }
> - }
> - iommu_domain_free(domain->domain);
> - list_del(&domain->next);
> - kfree(domain);
> - vfio_iommu_aper_expand(iommu, &iova_copy);
Previously the aperture is adjusted when a domain is freed...
> - vfio_update_pgsize_bitmap(iommu);
> - }
> - /*
> - * Removal of a group without dirty tracking may allow
> - * the iommu scope to be promoted.
> - */
> - if (!group->pinned_page_dirty_scope) {
> - iommu->num_non_pinned_groups--;
> - if (iommu->dirty_page_tracking)
> - vfio_iommu_populate_bitmap_full(iommu);
> - }
> + vfio_iommu_detach_destroy_domain(domain, iommu,
> group);
> kfree(group);
> break;
> }
>
> + vfio_iommu_aper_expand(iommu, &iova_copy);
but now it's done for every group detach. The aperture is decided
by domain geometry which is not affected by attached groups.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
