Re: [PATCH net-next 2/4] dt-bindings: net: qcom,ipq9574-ppe: Tighten ethernet port node constraints

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This patch only closes off the `ethernet-ports` spelling of the
  port container.
- [Low] The commit fixes schema-validation defects already present in the
  binding, but it has no `Fixes:` tag.

--

dt-bindings: net: qcom,ipq9574-ppe: Tighten ethernet port node constraints

This patch drops the stray '+' from the ethernet-port node pattern in the
qcom,ipq9574-ppe binding, so only single-digit unit addresses match. It
also gives the ethernet-ports container type: object and
additionalProperties: false, plus explicit #address-cells and #size-cells
constraints.

> Also restrict the ethernet-ports object to its declared properties.
> Without this, nodes outside the port pattern are accepted through the
> generic ethernet-switch schema.
>
> Signed-off-by: Til Kaiser <mail@xxxxxxxx>

[Severity: Low]
This fixes validation problems that have been in the binding since it was
added. Should it carry a Fixes: tag?

Both the unclosed ethernet-ports block and the "^ethernet-port@[1-6]+$"
pattern come from this commit:

Fixes: 1898fc572118 ("dt-bindings: net: Add PPE for Qualcomm IPQ9574 SoC")

> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq9574-ppe.yaml b/Documentation/devicetree/bindings/net/qcom,ipq9574-ppe.yaml
> index 6d0b21a10732a..1a78e2c82b90c 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ipq9574-ppe.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ipq9574-ppe.yaml
> @@ -174,8 +174,17 @@ properties:
>        - interrupt-names
>  
>    ethernet-ports:
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +      '#size-cells':
> +        const: 0
> +
>      patternProperties:
> -      "^ethernet-port@[1-6]+$":
> +      "^ethernet-port@[1-6]$":

[Severity: High]
This closes off the ethernet-ports container. Can the same nodes still get
through if the container is named ports instead?

The binding pulls in the generic schema:

    allOf:
      - $ref: ethernet-switch.yaml

ethernet-switch.yaml accepts either container name, and accepts any
port@ or ethernet-port@ child with a hex unit address:

    '^(ethernet-)?ports$':
      type: object
      unevaluatedProperties: false
      ...
      patternProperties:
        '^(ethernet-)?port@[0-9a-f]+$':
          type: object
          description: Ethernet switch ports
    ...
    oneOf:
      - required:
          - ports
      - required:
          - ethernet-ports

qcom,ipq9574-ppe.yaml has no ports entry, and it does not list
ethernet-ports under its top-level required: list. Because ports is
evaluated through the allOf $ref, the top-level unevaluatedProperties:
false does not reject it either.

So it looks like a PPE node like this would still validate:

    ports {
        #address-cells = <1>;
        #size-cells = <0>;
        port@7 { };
        ethernet-port@11 { };
    };

None of the PPE per-port constraints apply to that node. That covers the
reg 1..6 range, the required clocks/clock-names/resets/reset-names, the
unevaluatedProperties: false on the port node, and the new single-digit
pattern.

That means the case described in the commit message, "nodes outside the
port pattern are accepted through the generic ethernet-switch schema", can
still happen.

The later patch in this series, "dt-bindings: net: qcom,ipq9574-ppe: Add
IPQ5332 support", also constrains only ethernet-ports in its IPQ5332
if/then block. That includes the "^ethernet-port@[3-6]$": false rule, so
ports { port@3 { }; } would also get past the IPQ5332 port-count limit.

Other bindings with per-port constraints match '^(ethernet-)?ports$', so
both spellings are covered. Examples are dsa/qca8k.yaml,
dsa/mediatek,mt7530.yaml, dsa/microchip,ksz.yaml and dsa/nxp,sja1105.yaml.

Could this binding do the same? Another option would be to forbid the
other spelling with ports: false, and add ethernet-ports to the top-level
required: list.

>          type: object
>          unevaluatedProperties: false
>          $ref: ethernet-switch-port.yaml#

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005152915.101780-1-mail%40tk154.de