linux-kernel - Re: [PATCH 2/8] iommu: mtk_iommu: Lookup phandle to retrieve syscon to infracfg

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: <20220518184604.GK3302100-robh@kernel.org>
Date:   Wed, 18 May 2022 13:46:04 -0500
From:   Rob Herring <robh@...nel.org>
To:     Robin Murphy <robin.murphy@....com>
Cc:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>, yong.wu@...iatek.com,
        joro@...tes.org, will@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, matthias.bgg@...il.com,
        iommu@...ts.linux-foundation.org,
        linux-mediatek@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/8] iommu: mtk_iommu: Lookup phandle to retrieve syscon
 to infracfg

On Wed, May 18, 2022 at 12:07:58PM +0100, Robin Murphy wrote:
> On 2022-05-18 09:29, AngeloGioacchino Del Regno wrote:
> > Il 17/05/22 16:12, Robin Murphy ha scritto:
> > > On 2022-05-17 14:21, AngeloGioacchino Del Regno wrote:
> > > > 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, but also a dev_warn() was added to advertise
> > > > this change in hope that the user will see it and eventually update
> > > > the devicetree if this is possible.
> > > > 
> > > > Signed-off-by: AngeloGioacchino Del Regno
> > > > <angelogioacchino.delregno@...labora.com>
> > > > ---
> > > >   drivers/iommu/mtk_iommu.c | 40 +++++++++++++++++++++++++--------------
> > > >   1 file changed, 26 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > > > index 71b2ace74cd6..cfaaa98d2b50 100644
> > > > --- a/drivers/iommu/mtk_iommu.c
> > > > +++ b/drivers/iommu/mtk_iommu.c
> > > > @@ -1134,22 +1134,34 @@ 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)) {
> > > > +            dev_warn(dev, "Cannot find phandle to mediatek,infracfg:"
> > > > +                      " Please update your devicetree.\n");
> > > 
> > > Is this really a dev_warn-level problem? There's no functional
> > > impact, given that we can't stop supporting the original binding any
> > > time soon, if ever, so I suspect this is more likely to just annoy
> > > users and CI systems than effect any significant change.
> > > 
> > 
> > The upstream devicetrees were updated to use the new handle and this is
> > a way to
> > warn about having outdated DTs... besides, I believe that CIs will
> > always get the
> > devicetree from the same tree that the kernel was compiled from (hence
> > no message
> > will be thrown).
> > 
> > In any case, if you think that a dev_info would be more appropriate, I
> > can change
> > that no problem.
> 
> If there's some functional impact from using the old binding vs. the new one
> then it's reasonable to inform the user of that (as we do in arm-smmu, for
> example).
> 
> However if you change an established binding for non-functional reasons,
> then you get to support both bindings, and it's not the end user's problem
> at all. There seems to be zero reason to update an existing DTB for this
> difference alone, so TBH I don't think it deserves a message at all.

It's also not the kernel's job to validate the DT. It's horrible at it 
and we have something else now.

Rob