linux-kernel - RE: [PATCH 2/3] dt-bindings: firmware: xilinx: Add conditional pinctrl schema

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:
 <LV8PR12MB9206B51A38A82F5A1730DA3486C42@LV8PR12MB9206.namprd12.prod.outlook.com>
Date: Thu, 20 Feb 2025 12:18:19 +0000
From: "Jain, Ronak" <ronak.jain@....com>
To: Rob Herring <robh@...nel.org>
CC: "krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "Simek, Michal" <michal.simek@....com>, "Manne, Nava
 kishore" <nava.kishore.manne@....com>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 2/3] dt-bindings: firmware: xilinx: Add conditional
 pinctrl schema

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Rob,

> -----Original Message-----
> From: Jain, Ronak
> Sent: Thursday, February 13, 2025 4:46 PM
> To: Rob Herring <robh@...nel.org>
> Cc: krzk+dt@...nel.org; conor+dt@...nel.org; Simek, Michal
> <michal.simek@....com>; Manne, Nava kishore
> <nava.kishore.manne@....com>; devicetree@...r.kernel.org; linux-arm-
> kernel@...ts.infradead.org; linux-kernel@...r.kernel.org
> Subject: RE: [PATCH 2/3] dt-bindings: firmware: xilinx: Add conditional pinctrl
> schema
>
> Hi Rob,
>
> > -----Original Message-----
> > From: Rob Herring <robh@...nel.org>
> > Sent: Wednesday, February 12, 2025 3:24 AM
> > To: Jain, Ronak <ronak.jain@....com>
> > Cc: krzk+dt@...nel.org; conor+dt@...nel.org; Simek, Michal
> > <michal.simek@....com>; Manne, Nava kishore
> > <nava.kishore.manne@....com>; devicetree@...r.kernel.org; linux-arm-
> > kernel@...ts.infradead.org; linux-kernel@...r.kernel.org
> > Subject: Re: [PATCH 2/3] dt-bindings: firmware: xilinx: Add conditional pinctrl
> > schema
> >
> > On Thu, Feb 06, 2025 at 06:22:43AM -0800, Ronak Jain wrote:
> > > Updates the Device Tree bindings for Xilinx firmware by introducing
> > > conditional schema references for the pinctrl node.
> > >
> > > Previously, the pinctrl node directly referenced
> > > xlnx,zynqmp-pinctrl.yaml. However, this patch modifies the schema to
> > > conditionally apply the correct pinctrl schema based on the compatible
> > > property. Specifically:
> > > - If compatible contains "xlnx,zynqmp-pinctrl", reference
> > >   xlnx,zynqmp-pinctrl.yaml.
> > > - If compatible contains "xlnx,versal-pinctrl", reference
> > >   xlnx,versal-pinctrl.yaml.
> > >
> > > Additionally, an example entry for "xlnx,versal-pinctrl" has been
> > > added under the examples section.
> > >
> > > Signed-off-by: Ronak Jain <ronak.jain@....com>
> > > ---
> > >  .../firmware/xilinx/xlnx,zynqmp-firmware.yaml | 20 ++++++++++++++++++-
> > >  1 file changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-
> > firmware.yaml b/Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-
> > firmware.yaml
> > > index 2b72fb9d3c22..d50438b0fca8 100644
> > > --- a/Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-
> > firmware.yaml
> > > +++ b/Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-
> > firmware.yaml
> > > @@ -76,7 +76,6 @@ properties:
> > >      type: object
> > >
> > >    pinctrl:
> > > -    $ref: /schemas/pinctrl/xlnx,zynqmp-pinctrl.yaml#
> > >      description: The pinctrl node provides access to pinconfig and pincontrol
> > >        functionality available in firmware.
> > >      type: object
> > > @@ -106,6 +105,21 @@ properties:
> > >      type: object
> > >      deprecated: true
> > >
> > > +allOf:
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: xlnx,zynqmp-firmware
> > > +    then:
> > > +      properties:
> > > +        pinctrl:
> > > +          $ref: /schemas/pinctrl/xlnx,zynqmp-pinctrl.yaml#
> > > +    else:
> > > +      properties:
> > > +        pinctrl:
> > > +          $ref: /schemas/pinctrl/xlnx,versal-pinctrl.yaml#
> >
> > The somewhat preferred way to do this would be to do this in the top
> > level:
> >
> > pinctrl:
> >   type: object
> >   additionalProperties: true
> >   properties:
> >     compatible:
> >       contains:
> >         enum:
> >           - xlnx,zynqmp-pinctrl
> >           - xlnx,versal-pinctrl
> >   required:
> >     - compatible
> >
> > Otherwise, the pinctrl schema ends up being applied twice.
>
> Thanks for the patch review and inputs. I'll update and send the next version.
>

In your suggested code, the schema allows either xlnx,zynqmp-pinctrl or xlnx,versal-pinctrl on any platform, which is incorrect. This means that if a user mistakenly assigns xlnx,versal-pinctrl to a ZynqMP platform or xlnx,zynqmp-pinctrl to a Versal platform, the wrong reference will be used, but no error is reported. The dt-binding check still passes instead of flagging this as an issue.

By using a conditional schema, we can enforce platform-specific compatibility, ensuring that the correct compatible string is used for the corresponding platform. This would also generate an error if an incorrect compatible string is provided, preventing misconfigurations.

Please let me know your thoughts.

Thanks,
Ronak

> Thanks,
> Ronak
> >
> > > +
> > >  required:
> > >    - compatible
> > >
> > > @@ -164,6 +178,10 @@ examples:
> > >          compatible = "xlnx,versal-fpga";
> > >        };
> > >
> > > +      pinctrl {
> > > +        compatible = "xlnx,versal-pinctrl";
> > > +      };
> > > +
> > >        xlnx_aes: zynqmp-aes {
> > >          compatible = "xlnx,zynqmp-aes";
> > >        };
> > > --
> > > 2.34.1
> > >