linux-kernel - Re: [PATCH v2 03/10] iommu/mediatek: Get regionid from larb/port id

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <538959e38bbb694d4e0bae7078fa34cd3d23c65b.camel@mediatek.com>
Date:   Fri, 10 Feb 2023 06:13:17 +0000
From:   Yong Wu (吴勇) <Yong.Wu@...iatek.com>
To:     "joro@...tes.org" <joro@...tes.org>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>
CC:     "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Youlin Pei (裴友林) 
        <youlin.pei@...iatek.com>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Anan Sun (孙安安) <Anan.Sun@...iatek.com>,
        Libo Kang (康利波) <Libo.Kang@...iatek.com>,
        "kyrie.wu@...iatek.corp-partner.google.com" 
        <kyrie.wu@...iatek.corp-partner.google.com>,
        "robin.murphy@....com" <robin.murphy@....com>,
        "hverkuil@...all.nl" <hverkuil@...all.nl>,
        Chengci Xu (许承赐) 
        <Chengci.Xu@...iatek.com>,
        Yunfei Dong (董云飞) 
        <Yunfei.Dong@...iatek.com>,
        YF Wang (王云飞) <YF.Wang@...iatek.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
        Mingyuan Ma (马鸣远) 
        <Mingyuan.Ma@...iatek.com>, "will@...nel.org" <will@...nel.org>,
        "nfraprado@...labora.com" <nfraprado@...labora.com>
Subject: Re: [PATCH v2 03/10] iommu/mediatek: Get regionid from larb/port id

On Thu, 2023-02-09 at 14:39 +0100, AngeloGioacchino Del Regno wrote:
> Il 08/02/23 06:36, Yong Wu ha scritto:
> > After commit f1ad5338a4d5 ("of: Fix "dma-ranges" handling for bus
> > controllers"), the dma-ranges is not allowed for dts leaf node.
> > but we still would like to separate to different masters
> > into different iova regions.
> > 
> > Thus we have to separate it by the HW larbid and portid. For
> > example,
> > larb1/2 are in region2 and larb3 is in region3. The problem is that
> > some ports inside a larb are in region4 while some ports inside
> > this
> > larb are in region5. Therefore I define a "larb_region_msk" to help
> > record the information for each a port. Take a example for a larb:
> >   [1] = ~0: means all ports in this larb are in region1;
> >   [2] = BIT(3) | BIT(4): means port3/4 in this larb are region2;
> >   [3] = ~(BIT(3) | BIT(4)): means all the other ports except
> > port3/4
> >                             in this larb are region3.
> > 
> > This method also avoids the users forget/abuse the iova regions.
> > 
> > Signed-off-by: Yong Wu <yong.wu@...iatek.com>
> > ---
> >   drivers/iommu/mtk_iommu.c | 43 +++++++++++++++++++++-------------
> > -----
> >   1 file changed, 23 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index d5a4955910ff..fc3d9be120a0 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -8,7 +8,6 @@
> >   #include <linux/clk.h>
> >   #include <linux/component.h>
> >   #include <linux/device.h>
> > -#include <linux/dma-direct.h>
> >   #include <linux/err.h>
> >   #include <linux/interrupt.h>
> >   #include <linux/io.h>
> > @@ -194,6 +193,7 @@ struct mtk_iommu_plat_data {
> >   	enum mtk_iommu_plat	m4u_plat;
> >   	u32			flags;
> >   	u32			inv_sel_reg;
> > +	const u32		(*larb_region_msk)[32];
> 
> Can you please document this larb region mask in code, other than the
> commit
> description?
> 
> I can see this being essential for the next person reading this
> driver's code
> without digging through the commit history. At least some comment on
> top of
> the pointer, or on top of the struct declaration... and perhaps also
> describe
> briefly that the array is "indexed by region" (so 1 = region 1; 2 =
> region 2)
> and that the region index corresponds to the same index as
> `mtk_iommu_iova_region`.

Thanks for this suggestion. I will comment these in the code in next
version.

> 
> Before doing that, I'd like to check if anyone else has a better
> solution for
> that... because when looking at data for one of the SoCs in here, it
> looks a bit intimidating!
> 
> Copy-paste from patch [04/10] of this series for the reader's
> commodity:
> 
> static const unsigned int mt8195_larb_region_msk[][32] = {
> 	[0] = {~0, ~0, ~0, ~0},               /* Region0: all ports for
> larb0/1/2/3 */
> 	[1] = {0, 0, 0, 0, 0, 0, 0, 0,
> 	       0, 0, 0, 0, 0, 0, 0, 0,
> 	       0, 0, 0, ~0, ~0, ~0, ~0, ~0,   /* Region1:
> larb19/20/21/22/23/24 */
> 	       ~0},
> 	[2] = {0, 0, 0, 0, ~0, ~0, ~0, ~0,    /* Region2: the other
> larbs. */
> 	       ~0, ~0, ~0, ~0, ~0, ~0, ~0, ~0,
> 	       ~0, ~0, 0, 0, 0, 0, 0, 0,
> 	       0, ~0, ~0, ~0, ~0},
> 	[3] = {0},
> 	[4] = {[18] = BIT(0) | BIT(1)},       /* Only larb18 port0/1 */
> 	[5] = {[18] = BIT(2) | BIT(3)},       /* Only larb18 port2/3 */
> };
> 
> ^^^^ That's what I actually mean by "intimidating"... :-P
> 
> It's just looks though, there's nothing much complicated here.

Thanks.

> 
> Regards,
> Angelo
> 
>