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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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