| Message ID | 20230124222023.316089-1-robh@kernel.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | dt-bindings: firmware: arm,scmi: Restrict protocol child node properties | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Tue, Jan 24, 2023 at 04:20:23PM -0600, Rob Herring wrote: > The SCMI protocol child nodes are missing any constraints on unknown > properties. Specifically, either 'unevaluatedProperties' or > 'additionalProperties' is needed. The current structure with a regex > match for all child nodes doesn't work for this purpose, so let's move > the common properties '$defs' entry which each specific protocol node > can reference and set 'unevaluatedProperties: false'. > Makes sense to me. Also thanks for $defs example, wasn't aware of how to do that. Can you please take it though your tree ? Assuming that, Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> -- Regards, Sudeep
On Tue, Jan 24, 2023 at 04:20:23PM -0600, Rob Herring wrote: > The SCMI protocol child nodes are missing any constraints on unknown > properties. Specifically, either 'unevaluatedProperties' or > 'additionalProperties' is needed. The current structure with a regex > match for all child nodes doesn't work for this purpose, so let's move > the common properties '$defs' entry which each specific protocol node > can reference and set 'unevaluatedProperties: false'. > Hi Rob, > Signed-off-by: Rob Herring <robh@kernel.org> > --- > .../bindings/firmware/arm,scmi.yaml | 43 ++++++++++++++----- > 1 file changed, 33 insertions(+), 10 deletions(-) > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > index 176796931a22..2f7c51c75e85 100644 > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml > @@ -100,7 +100,9 @@ properties: > Channel specifier required when using OP-TEE transport. > > protocol@11: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x11 > @@ -112,7 +114,9 @@ properties: > - '#power-domain-cells' > > protocol@13: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x13 > @@ -124,7 +128,9 @@ properties: > - '#clock-cells' > > protocol@14: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x14 > @@ -136,7 +142,9 @@ properties: > - '#clock-cells' > > protocol@15: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x15 > @@ -148,7 +156,9 @@ properties: > - '#thermal-sensor-cells' > > protocol@16: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x16 > @@ -160,20 +170,31 @@ properties: > - '#reset-cells' > > protocol@17: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x17 > > regulators: > type: object > + additionalProperties: false > description: > The list of all regulators provided by this SCMI controller. > > + properties: > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > patternProperties: > - '^regulators@[0-9a-f]+$': > + '^regulator@[0-9a-f]+$': > type: object > $ref: "../regulator/regulator.yaml#" > + unevaluatedProperties: false > > properties: > reg: > @@ -184,15 +205,17 @@ properties: > - reg > > protocol@18: > - type: object > + $ref: '#/$defs/protocol-node' > + unevaluatedProperties: false > + > properties: > reg: > const: 0x18 > > additionalProperties: false > > -patternProperties: > - '^protocol@[0-9a-f]+$': > +$defs: > + protocol-node: > type: object > description: so now that the catch-all protocol@ patternProperty is gone in favour of the 'protocol-node' definition and $refs, does that mean that any current and future SCMI officially published protocol <N> has to be added to the above explicit protocol list, even though it does not have any special additional required property beside reg ? (like protocol@18 above...) As an example SystemPower protocol@12 is not listed above too and it has nothing more than a reg=0x12 prop (liek 0x18), but before this patch was 'covered' by the patternProperty (so Krzysztof shot down, rightly, my recent attempt to add a distinct protocol@12 def), but now it does not seem anymore the case...so will we need to add an explicit protocol node for any future protocol addition ? (SCMI is extensible up to 255 protos..) Thanks, Cristian
On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > so now that the catch-all protocol@ patternProperty is gone in favour > of the 'protocol-node' definition and $refs, does that mean that any > current and future SCMI officially published protocol <N> has to be > added to the above explicit protocol list, even though it does not > have any special additional required property beside reg ? > (like protocol@18 above...) > If there are no consumers, should we just not add and deal with it entirely within the kernel. I know we rely today on presence of node before we initialise, but hey we have exception for system power protocol for other reasons, why not add this one too. In short we shouldn't have to add a node if there are no consumers. It was one of the topic of discussion initially when SCMI binding was added and they exist only for the consumers otherwise we don't need it as everything is discoverable from the interface. -- Regards, Sudeep
On Wed, Jan 25, 2023 at 02:11:13PM +0000, Sudeep Holla wrote: > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > so now that the catch-all protocol@ patternProperty is gone in favour > > of the 'protocol-node' definition and $refs, does that mean that any > > current and future SCMI officially published protocol <N> has to be > > added to the above explicit protocol list, even though it does not > > have any special additional required property beside reg ? > > (like protocol@18 above...) > > > > If there are no consumers, should we just not add and deal with it > entirely within the kernel. I know we rely today on presence of node > before we initialise, but hey we have exception for system power protocol > for other reasons, why not add this one too. > > In short we shouldn't have to add a node if there are no consumers. It > was one of the topic of discussion initially when SCMI binding was added > and they exist only for the consumers otherwise we don't need it as > everything is discoverable from the interface. > I'm fine with that, just wanted to understand/clarify the rule here. Thanks, Cristian
On Wed, Jan 25, 2023 at 8:11 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > so now that the catch-all protocol@ patternProperty is gone in favour > > of the 'protocol-node' definition and $refs, does that mean that any > > current and future SCMI officially published protocol <N> has to be > > added to the above explicit protocol list, even though it does not > > have any special additional required property beside reg ? > > (like protocol@18 above...) > > > > If there are no consumers, should we just not add and deal with it > entirely within the kernel. I know we rely today on presence of node > before we initialise, but hey we have exception for system power protocol > for other reasons, why not add this one too. > > In short we shouldn't have to add a node if there are no consumers. It > was one of the topic of discussion initially when SCMI binding was added > and they exist only for the consumers otherwise we don't need it as > everything is discoverable from the interface. As you might guess, I agree. We need to keep 0x18 I suppose, right? I assume it is already in use. Are there any others that didn't get documented? We'd need to keep them because old kernels would still need them. Rob
On Wed, Jan 25, 2023 at 02:11:13PM +0000, Sudeep Holla wrote: > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > so now that the catch-all protocol@ patternProperty is gone in favour > > of the 'protocol-node' definition and $refs, does that mean that any > > current and future SCMI officially published protocol <N> has to be > > added to the above explicit protocol list, even though it does not > > have any special additional required property beside reg ? > > (like protocol@18 above...) > > > > If there are no consumers, should we just not add and deal with it > entirely within the kernel. I know we rely today on presence of node > before we initialise, but hey we have exception for system power protocol > for other reasons, why not add this one too. > > In short we shouldn't have to add a node if there are no consumers. It > was one of the topic of discussion initially when SCMI binding was added > and they exist only for the consumers otherwise we don't need it as > everything is discoverable from the interface. It is fine for me the no-consumers/no-node argument (which anyway would require a few changes in the core init logic anyway to work this way...), BUT is it not that ANY protocol (even future-ones) does have, potentially, consumers indeed, since each protocol-node can potentially have a dedicated channel and related DT channel-descriptor ? (when multiple channels are allowed by the transport) I mean, as an example, you dont strictly need protos 0x18/0x12 nodes for anything (if we patch the core init as said) UNLESS you want to dedicate a channel to those protocols; so I'm just checking here if these kind of scenarios will still be allowed with this binding change, or if I am missing something. Thanks, Cristian
On Thu, Jan 26, 2023 at 09:43:44AM +0000, Cristian Marussi wrote: > On Wed, Jan 25, 2023 at 02:11:13PM +0000, Sudeep Holla wrote: > > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > > so now that the catch-all protocol@ patternProperty is gone in favour > > > of the 'protocol-node' definition and $refs, does that mean that any > > > current and future SCMI officially published protocol <N> has to be > > > added to the above explicit protocol list, even though it does not > > > have any special additional required property beside reg ? > > > (like protocol@18 above...) > > > > > > > If there are no consumers, should we just not add and deal with it > > entirely within the kernel. I know we rely today on presence of node > > before we initialise, but hey we have exception for system power protocol > > for other reasons, why not add this one too. > > > > In short we shouldn't have to add a node if there are no consumers. It > > was one of the topic of discussion initially when SCMI binding was added > > and they exist only for the consumers otherwise we don't need it as > > everything is discoverable from the interface. > > It is fine for me the no-consumers/no-node argument (which anyway would > require a few changes in the core init logic anyway to work this way...), > BUT is it not that ANY protocol (even future-ones) does have, potentially, > consumers indeed, since each protocol-node can potentially have a dedicated > channel and related DT channel-descriptor ? (when multiple channels are > allowed by the transport) > > I mean, as an example, you dont strictly need protos 0x18/0x12 nodes for > anything (if we patch the core init as said) UNLESS you want to dedicate > a channel to those protocols; so I'm just checking here if these kind of > scenarios will still be allowed with this binding change, or if I am > missing something. Ah, good point on the transport information. Yes we will need a node if a protocol has a dedicated transport. No one has used so far other than Juno perf, but we never know. We can always extended the bindings if needed. Sorry for missing the dedicated transport part.
On Thu, Jan 26, 2023 at 8:46 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Thu, Jan 26, 2023 at 09:43:44AM +0000, Cristian Marussi wrote: > > On Wed, Jan 25, 2023 at 02:11:13PM +0000, Sudeep Holla wrote: > > > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > > > so now that the catch-all protocol@ patternProperty is gone in favour > > > > of the 'protocol-node' definition and $refs, does that mean that any > > > > current and future SCMI officially published protocol <N> has to be > > > > added to the above explicit protocol list, even though it does not > > > > have any special additional required property beside reg ? > > > > (like protocol@18 above...) > > > > > > > > > > If there are no consumers, should we just not add and deal with it > > > entirely within the kernel. I know we rely today on presence of node > > > before we initialise, but hey we have exception for system power protocol > > > for other reasons, why not add this one too. > > > > > > In short we shouldn't have to add a node if there are no consumers. It > > > was one of the topic of discussion initially when SCMI binding was added > > > and they exist only for the consumers otherwise we don't need it as > > > everything is discoverable from the interface. > > > > It is fine for me the no-consumers/no-node argument (which anyway would > > require a few changes in the core init logic anyway to work this way...), > > BUT is it not that ANY protocol (even future-ones) does have, potentially, > > consumers indeed, since each protocol-node can potentially have a dedicated > > channel and related DT channel-descriptor ? (when multiple channels are > > allowed by the transport) > > > > I mean, as an example, you dont strictly need protos 0x18/0x12 nodes for > > anything (if we patch the core init as said) UNLESS you want to dedicate > > a channel to those protocols; so I'm just checking here if these kind of > > scenarios will still be allowed with this binding change, or if I am > > missing something. > > Ah, good point on the transport information. Yes we will need a node if > a protocol has a dedicated transport. No one has used so far other than > Juno perf, but we never know. We can always extended the bindings if > needed. > > Sorry for missing the dedicated transport part. So I need to add back 'protocol@.*' or not? Rob
On Thu, Jan 26, 2023 at 09:25:12AM -0600, Rob Herring wrote: > On Thu, Jan 26, 2023 at 8:46 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Thu, Jan 26, 2023 at 09:43:44AM +0000, Cristian Marussi wrote: > > > On Wed, Jan 25, 2023 at 02:11:13PM +0000, Sudeep Holla wrote: > > > > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > > > > so now that the catch-all protocol@ patternProperty is gone in favour > > > > > of the 'protocol-node' definition and $refs, does that mean that any > > > > > current and future SCMI officially published protocol <N> has to be > > > > > added to the above explicit protocol list, even though it does not > > > > > have any special additional required property beside reg ? > > > > > (like protocol@18 above...) > > > > > > > > > > > > > If there are no consumers, should we just not add and deal with it > > > > entirely within the kernel. I know we rely today on presence of node > > > > before we initialise, but hey we have exception for system power protocol > > > > for other reasons, why not add this one too. > > > > > > > > In short we shouldn't have to add a node if there are no consumers. It > > > > was one of the topic of discussion initially when SCMI binding was added > > > > and they exist only for the consumers otherwise we don't need it as > > > > everything is discoverable from the interface. > > > > > > It is fine for me the no-consumers/no-node argument (which anyway would > > > require a few changes in the core init logic anyway to work this way...), > > > BUT is it not that ANY protocol (even future-ones) does have, potentially, > > > consumers indeed, since each protocol-node can potentially have a dedicated > > > channel and related DT channel-descriptor ? (when multiple channels are > > > allowed by the transport) > > > > > > I mean, as an example, you dont strictly need protos 0x18/0x12 nodes for > > > anything (if we patch the core init as said) UNLESS you want to dedicate > > > a channel to those protocols; so I'm just checking here if these kind of > > > scenarios will still be allowed with this binding change, or if I am > > > missing something. > > > > Ah, good point on the transport information. Yes we will need a node if > > a protocol has a dedicated transport. No one has used so far other than > > Juno perf, but we never know. We can always extended the bindings if > > needed. > > > > Sorry for missing the dedicated transport part. > > So I need to add back 'protocol@.*' or not? IMO it is better to know what exactly gets added under each of these protocol sub-nodes and so better to have entry specific to each known protocols. I liked that fact with this change as I have seen some crazy vendor extensions adding all sorts of non-sense defining some vendor protocol. For example [1], in which case we can catch those better than existing schema which matches all. So let's not add protocol@.* if possible or until that becomes the only cleaner way to maintain this. -- Regards, Sudeep [1] https://lore.kernel.org/all/1667451512-9655-2-git-send-email-quic_sibis@quicinc.com/
On Thu, Jan 26, 2023 at 11:04 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Thu, Jan 26, 2023 at 09:25:12AM -0600, Rob Herring wrote: > > On Thu, Jan 26, 2023 at 8:46 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > On Thu, Jan 26, 2023 at 09:43:44AM +0000, Cristian Marussi wrote: > > > > On Wed, Jan 25, 2023 at 02:11:13PM +0000, Sudeep Holla wrote: > > > > > On Wed, Jan 25, 2023 at 01:43:48PM +0000, Cristian Marussi wrote: > > > > > > so now that the catch-all protocol@ patternProperty is gone in favour > > > > > > of the 'protocol-node' definition and $refs, does that mean that any > > > > > > current and future SCMI officially published protocol <N> has to be > > > > > > added to the above explicit protocol list, even though it does not > > > > > > have any special additional required property beside reg ? > > > > > > (like protocol@18 above...) > > > > > > > > > > > > > > > > If there are no consumers, should we just not add and deal with it > > > > > entirely within the kernel. I know we rely today on presence of node > > > > > before we initialise, but hey we have exception for system power protocol > > > > > for other reasons, why not add this one too. > > > > > > > > > > In short we shouldn't have to add a node if there are no consumers. It > > > > > was one of the topic of discussion initially when SCMI binding was added > > > > > and they exist only for the consumers otherwise we don't need it as > > > > > everything is discoverable from the interface. > > > > > > > > It is fine for me the no-consumers/no-node argument (which anyway would > > > > require a few changes in the core init logic anyway to work this way...), > > > > BUT is it not that ANY protocol (even future-ones) does have, potentially, > > > > consumers indeed, since each protocol-node can potentially have a dedicated > > > > channel and related DT channel-descriptor ? (when multiple channels are > > > > allowed by the transport) > > > > > > > > I mean, as an example, you dont strictly need protos 0x18/0x12 nodes for > > > > anything (if we patch the core init as said) UNLESS you want to dedicate > > > > a channel to those protocols; so I'm just checking here if these kind of > > > > scenarios will still be allowed with this binding change, or if I am > > > > missing something. > > > > > > Ah, good point on the transport information. Yes we will need a node if > > > a protocol has a dedicated transport. No one has used so far other than > > > Juno perf, but we never know. We can always extended the bindings if > > > needed. > > > > > > Sorry for missing the dedicated transport part. > > > > So I need to add back 'protocol@.*' or not? > > IMO it is better to know what exactly gets added under each of these protocol > sub-nodes and so better to have entry specific to each known protocols. I > liked that fact with this change as I have seen some crazy vendor extensions > adding all sorts of non-sense defining some vendor protocol. For example [1], > in which case we can catch those better than existing schema which matches > all. So let's not add protocol@.* if possible or until that becomes the only > cleaner way to maintain this. TBC, 'protocol@.*' would not allow anything but the properties defined in the /$defs/protocol-node. So [1] would throw errors without a schema addition. We should either do that along with dropping 'protocol@18' or we keep protocol 0x18 node and add all other providerless protocols. I don't think we need the latter to just check unit-address vs. reg. I want to come up with a better way to do that (dtc does some, but only for defined bus types). Rob
Hi Rob, On Fri, Jan 27, 2023 at 12:52:33PM -0600, Rob Herring wrote: > > TBC, 'protocol@.*' would not allow anything but the properties defined > in the /$defs/protocol-node. So [1] would throw errors without a > schema addition. Right I clearly missed that, somehow I assumed it would allow. > We should either do that along with dropping 'protocol@18' or we keep > protocol 0x18 node and add all other providerless protocols. I don't > think we need the latter to just check unit-address vs. reg. I only argument today it to allow protocol specific transport. So we could delay addition of it until someone needs that way. So far we haven't seen anyone using it other than performance(even that is not needed with the introduction of fast channels that are auto discoverable in relatively newer versions of the spec). -- Regards, Sudeep
On Mon, Feb 6, 2023 at 4:47 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > Hi Rob, > > On Fri, Jan 27, 2023 at 12:52:33PM -0600, Rob Herring wrote: > > > > TBC, 'protocol@.*' would not allow anything but the properties defined > > in the /$defs/protocol-node. So [1] would throw errors without a > > schema addition. > > Right I clearly missed that, somehow I assumed it would allow. > > > We should either do that along with dropping 'protocol@18' or we keep > > protocol 0x18 node and add all other providerless protocols. I don't > > think we need the latter to just check unit-address vs. reg. > > I only argument today it to allow protocol specific transport. So we could > delay addition of it until someone needs that way. So far we haven't seen > anyone using it other than performance(even that is not needed with the > introduction of fast channels that are auto discoverable in relatively > newer versions of the spec). I failed to think about 'protocol@.*' would match on every protocol, so we have to list them explicitly: '^protocol@(18|xx|yy|zz)$' Anyways, I think the conclusion is the patch should stay as-is and so I've applied it. Rob
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml index 176796931a22..2f7c51c75e85 100644 --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml @@ -100,7 +100,9 @@ properties: Channel specifier required when using OP-TEE transport. protocol@11: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x11 @@ -112,7 +114,9 @@ properties: - '#power-domain-cells' protocol@13: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x13 @@ -124,7 +128,9 @@ properties: - '#clock-cells' protocol@14: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x14 @@ -136,7 +142,9 @@ properties: - '#clock-cells' protocol@15: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x15 @@ -148,7 +156,9 @@ properties: - '#thermal-sensor-cells' protocol@16: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x16 @@ -160,20 +170,31 @@ properties: - '#reset-cells' protocol@17: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x17 regulators: type: object + additionalProperties: false description: The list of all regulators provided by this SCMI controller. + properties: + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + patternProperties: - '^regulators@[0-9a-f]+$': + '^regulator@[0-9a-f]+$': type: object $ref: "../regulator/regulator.yaml#" + unevaluatedProperties: false properties: reg: @@ -184,15 +205,17 @@ properties: - reg protocol@18: - type: object + $ref: '#/$defs/protocol-node' + unevaluatedProperties: false + properties: reg: const: 0x18 additionalProperties: false -patternProperties: - '^protocol@[0-9a-f]+$': +$defs: + protocol-node: type: object description: Each sub-node represents a protocol supported. If the platform
The SCMI protocol child nodes are missing any constraints on unknown properties. Specifically, either 'unevaluatedProperties' or 'additionalProperties' is needed. The current structure with a regex match for all child nodes doesn't work for this purpose, so let's move the common properties '$defs' entry which each specific protocol node can reference and set 'unevaluatedProperties: false'. Signed-off-by: Rob Herring <robh@kernel.org> --- .../bindings/firmware/arm,scmi.yaml | 43 ++++++++++++++----- 1 file changed, 33 insertions(+), 10 deletions(-)