diff mbox series

[v9,2/7] iommu/mediatek: Fix two IOMMU share pagetable issue

Message ID 20230317085541.20447-3-yong.wu@mediatek.com
State New
Headers
Series MT8188 IOMMU SUPPORT |

Commit Message

Yong Wu March 17, 2023, 8:55 a.m. UTC 
  From: "Chengci.Xu" <chengci.xu@mediatek.com>

Prepare for mt8188 to fix a two IOMMU HWs share pagetable issue.

We have two MM IOMMU HWs in mt8188, one is VPP-IOMMU, another is VDO-IOMMU.
The 2 MM IOMMU HWs share pagetable don't work in this case:
 a) VPP-IOMMU probe firstly.
 b) VDO-IOMMU probe.
 c) The master for VDO-IOMMU probe (means frstdata is vpp-iommu).
 d) The master in another domain probe. No matter it is vdo or vpp.
Then it still create a new pagetable in step d). The problem is
"frstdata->bank[0]->m4u_dom" was not initialized. Then when d) enter, it
still create a new one.

In this patch, we create a new variable "share_dom" for this share
pgtable case, it should be helpful for readable. and put all the share
pgtable logic in the mtk_iommu_domain_finalise.

In mt8195, the master of VPP-IOMMU probes before than VDO-IOMMU
from its dtsi node sequence, we don't see this issue in it. Prepare for
mt8188.

Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/iommu/mtk_iommu.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

Comments

AngeloGioacchino Del Regno March 17, 2023, 9:34 a.m. UTC | #1 
Il 17/03/23 09:55, Yong Wu ha scritto:
> From: "Chengci.Xu" <chengci.xu@mediatek.com>
> 
> Prepare for mt8188 to fix a two IOMMU HWs share pagetable issue.
> 
> We have two MM IOMMU HWs in mt8188, one is VPP-IOMMU, another is VDO-IOMMU.
> The 2 MM IOMMU HWs share pagetable don't work in this case:
>   a) VPP-IOMMU probe firstly.
>   b) VDO-IOMMU probe.
>   c) The master for VDO-IOMMU probe (means frstdata is vpp-iommu).
>   d) The master in another domain probe. No matter it is vdo or vpp.
> Then it still create a new pagetable in step d). The problem is
> "frstdata->bank[0]->m4u_dom" was not initialized. Then when d) enter, it
> still create a new one.
> 
> In this patch, we create a new variable "share_dom" for this share
> pgtable case, it should be helpful for readable. and put all the share
> pgtable logic in the mtk_iommu_domain_finalise.
> 
> In mt8195, the master of VPP-IOMMU probes before than VDO-IOMMU
> from its dtsi node sequence, we don't see this issue in it. Prepare for
> mt8188.
> 
> Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>

I'm not sure whether this is *not* a fix... if a specific platform wasn't
affected, this may still be a logic mistake... to be cautious, I would
still add a Fixes tag to this one.

Otherwise, I would reword the commit title and remove the "Fix" word, so
that it would look something like...

iommu/mediatek: Prepare for pagetables domain sharing

I'm sorry for noticing that only at v9, but the previous versions had a bit
more stuff to address and couldn't really focus on analyzing this one.

If there's anything I've misunderstood about this patch, please feel free
to explain.

Regards,
Angelo
Yong Wu March 21, 2023, 6:50 a.m. UTC | #2 
On Fri, 2023-03-17 at 10:34 +0100, AngeloGioacchino Del Regno wrote:
> Il 17/03/23 09:55, Yong Wu ha scritto:
> > From: "Chengci.Xu" <chengci.xu@mediatek.com>
> > 
> > Prepare for mt8188 to fix a two IOMMU HWs share pagetable issue.
> > 
> > We have two MM IOMMU HWs in mt8188, one is VPP-IOMMU, another is
> > VDO-IOMMU.
> > The 2 MM IOMMU HWs share pagetable don't work in this case:
> >   a) VPP-IOMMU probe firstly.
> >   b) VDO-IOMMU probe.
> >   c) The master for VDO-IOMMU probe (means frstdata is vpp-iommu).
> >   d) The master in another domain probe. No matter it is vdo or
> > vpp.
> > Then it still create a new pagetable in step d). The problem is
> > "frstdata->bank[0]->m4u_dom" was not initialized. Then when d)
> > enter, it
> > still create a new one.
> > 
> > In this patch, we create a new variable "share_dom" for this share
> > pgtable case, it should be helpful for readable. and put all the
> > share
> > pgtable logic in the mtk_iommu_domain_finalise.
> > 
> > In mt8195, the master of VPP-IOMMU probes before than VDO-IOMMU
> > from its dtsi node sequence, we don't see this issue in it. Prepare
> > for
> > mt8188.
> > 
> > Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> 
> I'm not sure whether this is *not* a fix... if a specific platform
> wasn't
> affected, this may still be a logic mistake... to be cautious, I
> would
> still add a Fixes tag to this one.

I think you are right. If we need add the Fixes tag, it should fix this
one: 645b87c190c9 ("iommu/mediatek: Fix 2 HW sharing pgtable issue").

Before I thought the code flow was changed a lot. I added the bank
structure and removed the mtk_iommu.h, I'm a bit afraid that this fix
patch can not be applied clean, then it will introduce confuse when
applying to the previous version for the maintainers.

Meanwhile, After mt8195, mt8186/mt6795/m8365/6795 were merged in
upstream. All of them don't have this sharing case, thus I thought this
fix it is not so necessary.

What's your opinion? and should I send this one separately if I add the
fixes tag?


> Otherwise, I would reword the commit title and remove the "Fix" word,
> so
> that it would look something like...
> 
> iommu/mediatek: Prepare for pagetables domain sharing
> 
> I'm sorry for noticing that only at v9, but the previous versions had
> a bit
> more stuff to address and couldn't really focus on analyzing this
> one.
> 
> If there's anything I've misunderstood about this patch, please feel
> free
> to explain.
> 
> Regards,
> Angelo
>
AngeloGioacchino Del Regno March 21, 2023, 8:52 a.m. UTC | #3 
Il 21/03/23 07:50, Yong Wu (吴勇) ha scritto:
> On Fri, 2023-03-17 at 10:34 +0100, AngeloGioacchino Del Regno wrote:
>> Il 17/03/23 09:55, Yong Wu ha scritto:
>>> From: "Chengci.Xu" <chengci.xu@mediatek.com>
>>>
>>> Prepare for mt8188 to fix a two IOMMU HWs share pagetable issue.
>>>
>>> We have two MM IOMMU HWs in mt8188, one is VPP-IOMMU, another is
>>> VDO-IOMMU.
>>> The 2 MM IOMMU HWs share pagetable don't work in this case:
>>>    a) VPP-IOMMU probe firstly.
>>>    b) VDO-IOMMU probe.
>>>    c) The master for VDO-IOMMU probe (means frstdata is vpp-iommu).
>>>    d) The master in another domain probe. No matter it is vdo or
>>> vpp.
>>> Then it still create a new pagetable in step d). The problem is
>>> "frstdata->bank[0]->m4u_dom" was not initialized. Then when d)
>>> enter, it
>>> still create a new one.
>>>
>>> In this patch, we create a new variable "share_dom" for this share
>>> pgtable case, it should be helpful for readable. and put all the
>>> share
>>> pgtable logic in the mtk_iommu_domain_finalise.
>>>
>>> In mt8195, the master of VPP-IOMMU probes before than VDO-IOMMU
>>> from its dtsi node sequence, we don't see this issue in it. Prepare
>>> for
>>> mt8188.
>>>
>>> Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>
>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>
>> I'm not sure whether this is *not* a fix... if a specific platform
>> wasn't
>> affected, this may still be a logic mistake... to be cautious, I
>> would
>> still add a Fixes tag to this one.
> 
> I think you are right. If we need add the Fixes tag, it should fix this
> one: 645b87c190c9 ("iommu/mediatek: Fix 2 HW sharing pgtable issue").
> 
> Before I thought the code flow was changed a lot. I added the bank
> structure and removed the mtk_iommu.h, I'm a bit afraid that this fix
> patch can not be applied clean, then it will introduce confuse when
> applying to the previous version for the maintainers.
> 
> Meanwhile, After mt8195, mt8186/mt6795/m8365/6795 were merged in
> upstream. All of them don't have this sharing case, thus I thought this
> fix it is not so necessary.
> 
> What's your opinion? and should I send this one separately if I add the
> fixes tag?
> 

Well, it would be nicer to send it separately but, realistically, the
described issue does *not* happen on the previous kernel releases for
the supported SoCs... so it's not necessary to split this.

Add the Fixes tag and send this again inside of this series, that's
going to be fine.

Thanks!
Angelo
diff mbox series

Patch

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index e0264d5f1c9a..9802e5c1a625 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -258,6 +258,8 @@  struct mtk_iommu_data {
 	struct device			*smicomm_dev;
 
 	struct mtk_iommu_bank_data	*bank;
+	struct mtk_iommu_domain		*share_dom; /* For 2 HWs share pgtable */
+
 	struct regmap			*pericfg;
 	struct mutex			mutex; /* Protect m4u_group/m4u_dom above */
 
@@ -624,15 +626,14 @@  static int mtk_iommu_domain_finalise(struct mtk_iommu_domain *dom,
 				     struct mtk_iommu_data *data,
 				     unsigned int region_id)
 {
+	struct mtk_iommu_domain	*share_dom = data->share_dom;
 	const struct mtk_iommu_iova_region *region;
-	struct mtk_iommu_domain	*m4u_dom;
-
-	/* Always use bank0 in sharing pgtable case */
-	m4u_dom = data->bank[0].m4u_dom;
-	if (m4u_dom) {
-		dom->iop = m4u_dom->iop;
-		dom->cfg = m4u_dom->cfg;
-		dom->domain.pgsize_bitmap = m4u_dom->cfg.pgsize_bitmap;
+
+	/* Always use share domain in sharing pgtable case */
+	if (MTK_IOMMU_HAS_FLAG(data->plat_data, SHARE_PGTABLE) && share_dom) {
+		dom->iop = share_dom->iop;
+		dom->cfg = share_dom->cfg;
+		dom->domain.pgsize_bitmap = share_dom->cfg.pgsize_bitmap;
 		goto update_iova_region;
 	}
 
@@ -662,6 +663,9 @@  static int mtk_iommu_domain_finalise(struct mtk_iommu_domain *dom,
 	/* Update our support page sizes bitmap */
 	dom->domain.pgsize_bitmap = dom->cfg.pgsize_bitmap;
 
+	if (MTK_IOMMU_HAS_FLAG(data->plat_data, SHARE_PGTABLE))
+		data->share_dom = dom;
+
 update_iova_region:
 	/* Update the iova region for this domain */
 	region = data->plat_data->iova_region + region_id;
@@ -712,7 +716,9 @@  static int mtk_iommu_attach_device(struct iommu_domain *domain,
 		/* Data is in the frstdata in sharing pgtable case. */
 		frstdata = mtk_iommu_get_frst_data(hw_list);
 
+		mutex_lock(&frstdata->mutex);
 		ret = mtk_iommu_domain_finalise(dom, frstdata, region_id);
+		mutex_unlock(&frstdata->mutex);
 		if (ret) {
 			mutex_unlock(&dom->mutex);
 			return ret;