RE: [EXT] Re: [PATCH 1/4] dt-bindings: firmware: secvio: Add device tree bindings

[Vabhav Sharma]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Sent: Thursday, May 9, 2024 11:24 AM
> To: Vabhav Sharma <vabhav.sharma@xxxxxxx>; Rob Herring
> <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Franck Lenormand <franck.lenormand@xxxxxxx>;
> Aisheng Dong <aisheng.dong@xxxxxxx>; Shawn Guo
> <shawnguo@xxxxxxxxxx>; Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>;
> Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>; Fabio Estevam
> <festevam@xxxxxxxxx>; Peng Fan <peng.fan@xxxxxxx>
> Cc: devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> imx@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Varun Sethi
> <V.Sethi@xxxxxxx>; Silvano Di Ninno <silvano.dininno@xxxxxxx>; Pankaj
> Gupta <pankaj.gupta@xxxxxxx>; Frank Li <frank.li@xxxxxxx>; Daniel Baluta
> <daniel.baluta@xxxxxxx>
> Subject: [EXT] Re: [PATCH 1/4] dt-bindings: firmware: secvio: Add device tree
> bindings
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On 09/05/2024 02:45, Vabhav Sharma wrote:
> > Document the secvio device tree bindings.
> >
> > The tampers are security feature available on i.MX products and
> > managed by SNVS block.The tamper goal is to detect the variation of
> > hardware or physical parameters, which can indicate an attack.
> >
> > The SNVS, which provides secure non-volatile storage, allows to detect
> > some hardware attacks against the SoC.They are connected to the
> > security-violation ports, which send an alert when an out-of-range
> > value is detected.
> >
> > The "imx-secvio-sc" module is designed to report security violations
> > and tamper triggering via SCU firmware to the user.
> >
> > Add the imx-scu secvio sub node and secvio sub node description.
> >
> > Signed-off-by: Franck LENORMAND <franck.lenormand@xxxxxxx>
> > Signed-off-by: Vabhav Sharma <vabhav.sharma@xxxxxxx>
> > ---
> 
> That's not v1, right? What changed? Why do we have to guess this?
Yes, correct this is v2, I will provide changelog details in v3 for v2 and v1
> 
> This is thoroughly documented in kernel process so read the documentation
> before posting.
> 
> 
> >  .../bindings/arm/freescale/fsl,scu-secvio.yaml     | 35
> ++++++++++++++++++++++
> >  .../devicetree/bindings/firmware/fsl,scu.yaml      | 10 +++++++
> >  2 files changed, 45 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
> > new file mode 100644
> > index 000000000000..30dc1e21f903
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.y
> > +++ aml
> > @@ -0,0 +1,35 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fschemas%2Farm%2Ffreescale%2Ffsl%2Cscu-
> secvio.yaml%23&dat
> >
> +a=05%7C02%7Cvabhav.sharma%40nxp.com%7Cdea3ecc999444d8c3f7108dc
> 6fec67e
> >
> +b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63850830837113
> 8503%7CU
> >
> +nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTi
> I6Ik1h
> >
> +aWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Nl9F3A9%2BTraZboxg3KXT
> 35pIPAyYZ
> > +51URq1wff7XCmo%3D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C02%7Cvabhav.sharma
> >
> +%40nxp.com%7Cdea3ecc999444d8c3f7108dc6fec67eb%7C686ea1d3bc2b4c
> 6fa92cd
> >
> +99c5c301635%7C0%7C0%7C638508308371152796%7CUnknown%7CTWFpb
> GZsb3d8eyJW
> >
> +IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 0%7C
> >
> +%7C%7C&sdata=dwOG7uVGtO8rccW7vcRwvc2%2B9gyDroIsWnFlXyFxbOM%
> 3D&reserve
> > +d=0
> > +
> > +title: NXP i.MX Security Violation driver
> 
> Bindings are for hardware, not drivers. Describe hardware.
Yes, I will use "security violation detection hardware exported through SCU firmware"
> 
> > +
> > +maintainers:
> > +  - Franck LENORMAND <franck.lenormand@xxxxxxx>
> > +
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.
Ok
> 
> > +  Receive security violation from the SNVS via the SCU firmware.
> > + Allow to  register notifier for additional processing
> 
> Notifier? That's a Linux thing, how does it relate to the hardware?
Violation detected by HW will call driver API to signify the violation.
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - fsl,imx-sc-secvio
> 
> Missing SoC compatibles.
Ok, I will use fsl,imx8dxl-sc-secvio
> 
> So no, that's just abuse of DT to instantiate driver.
> 
> NAK. Drop the binding.
I will detail the dt binding to describe the real hardware
> 
> Best regards,
> Krzysztof