linux-kernel - Re: [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional

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: <YVOR+/xfdRUwGt5D@builder.lan>
Date:   Tue, 28 Sep 2021 17:06:51 -0500
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Rob Herring <robh+dt@...nel.org>
Cc:     Stephan Gerhold <stephan@...hold.net>,
        Andy Gross <agross@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        devicetree@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional

On Tue 28 Sep 14:51 CDT 2021, Rob Herring wrote:

> On Tue, Sep 28, 2021 at 12:49 PM Bjorn Andersson
> <bjorn.andersson@...aro.org> wrote:
> >
> > On Tue 28 Sep 12:34 CDT 2021, Rob Herring wrote:
> >
> > > On Tue, Sep 28, 2021 at 5:22 AM Stephan Gerhold <stephan@...hold.net> wrote:
> > > >
> > > > On Mon, Sep 27, 2021 at 09:45:44PM -0700, Bjorn Andersson wrote:
> > > > > In the olden days the Qualcomm shared memory (SMEM) region consisted of
> > > > > multiple chunks of memory, so SMEM was described as a standalone node
> > > > > with references to its various memory regions.
> > > > >
> > > > > But practically all modern Qualcomm platforms has a single reserved memory
> > > > > region used for SMEM. So rather than having to use two nodes to describe
> > > > > the one SMEM region, update the binding to allow the reserved-memory
> > > > > region alone to describe SMEM.
> > > > >
> > > > > The olden format is preserved as valid, as this is widely used already.
> > > > >
> > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> > > > > ---
> > > > >  .../bindings/soc/qcom/qcom,smem.yaml          | 34 ++++++++++++++++---
> > > > >  1 file changed, 30 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml
> > > > > index f7e17713b3d8..4149cf2b66be 100644
> > > > > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml
> > > > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml
> > > > > [...]
> > > > > @@ -43,6 +55,20 @@ examples:
> > > > >          #size-cells = <1>;
> > > > >          ranges;
> > > > >
> > > > > +        smem@...0000 {
> > > >
> > > > I think this is a good opportunity to make a decision which node name
> > > > should be used here. :)
> > >
> > > reserved-memory node names are kind of a mess, so I haven't tried for
> > > any standard... It needs to be solved globally.
> > >
> >
> > I'd be happy to paint the shed any color you decide :)
> 
> I didn't ask for it to be painted. Unless it is for everyone, I don't
> care unless there's some clear pattern used already.
> 

As Stephan indicated, I feel that I'll set precedence when I change
"memory" -> "smem" in the last patch.

> > That said, the binding itself doesn't mandate any node name, so it's
> > just the example here that would be "wrong" - and just as wrong as it
> > currently is.
> 
> The example is right. The dts is wrong.
> 

But I can't both not paint the node and resolve the fact that the dts is
wrong. So which one should I go with?

Should we leave the node name as is until we've decided what to do with
the reserved-memory children? Or should I start accepting patches that
changes "memory" to a list of non-generic names?

> Perhaps we need a schema for 'any node name that doesn't match already
> defined ones'.
> 
> > > > You use smem@ here but mentioned before that you think using the generic
> > > > memory@ would be better [1]. And you use memory@ in PATCH 3/3:
> > > >
> > > > -               smem_mem: memory@...00000 {
> > > > +               memory@...00000 {
> > > > +                       compatible = "qcom,smem";
> > > >                         reg = <0x0 0x86000000 0 0x200000>;
> > > >                         no-map;
> > > > +                       hwlocks = <&tcsr_mutex 3>;
> > > >                 };
> > > >
> > > > However, if you would use memory@ as example in this DT schema,
> > > > Rob's bot would complain with the same error that I mentioned earlier [2]:
> > > >
> > > > soc/qcom/qcom,smem.example.dt.yaml: memory@...0000: 'device_type' is a required property
> > > >         From schema: dtschema/schemas/memory.yaml
> > > >
> > > > We should either fix the error when using memory@ or start using some
> > > > different node name (Stephen Boyd suggested shared-memory@ for example).
> > > > Otherwise we'll just keep introducing more and more dtbs_check errors
> > > > for the Qualcomm device trees.
> > >
> > > A different node name. A node name should only have 1 meaning and
> > > 'memory' is already defined.
> > >
> > > The main issue here is what to name nodes with only a size and no address.
> > >
> >
> > This particular node has both address and size (as does all of the other
> > reserved-memory regions we use upstream today)...
> 
> I'm not talking about *just* QCom. If we define something here, it's
> got to cover everyone.
> 
> In summary, you can't use 'memory' or anything other established,
> standard node name.
> 

I know that "memory" is wrong, but I'm not sure about what you're asking
me to do.

Regards,
Bjorn