Re: [PATCH net-next v5 01/12] dt-bindings: net: airoha: Add EN7581 ethernet-ports properties

Re: [PATCH net-next v5 01/12] dt-bindings: net: airoha: Add EN7581 ethernet-ports properties

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3782 bytes --]

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] The `reg` property check in the conditional `if` block uses an invalid JSON schema sequence that will always evaluate to false, causing valid device trees to fail validation.
> - [High] The `ethernet-port@[0-5]` nodes model a `net_device` but strictly forbid standard network configuration properties like `mac-address` and `phy-handle`.
> - [Low] Missing `#address-cells` and `#size-cells` in the `required` list for conditional child nodes.
> --
> 
> commit 270a641148bef0e32520efd84b62bf6b97aa5a63
> Author: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> dt-bindings: net: airoha: Add EN7581 ethernet-ports properties
> 
> This commit introduces the ethernet-port property for EN7581 and AN7583
> SoCs to model multiple net_devices connected via an external arbiter to
> GDM3 or GDM4 ports.
> 
> > diff --git a/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > --- a/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > +++ b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> [ ... ]
> > @@ -130,6 +130,38 @@ patternProperties:
> >          maximum: 4
> >          description: GMAC port identifier
> >  
> > +      '#address-cells':
> > +        const: 1
> > +      '#size-cells':
> > +        const: 0
> > +
> > +    allOf:
> > +      - if:
> > +          properties:
> > +            reg:
> > +              items:
> > +                - enum:
> > +                    - 3
> > +                    - 4
> 
> Will this schema condition evaluate as expected?
> 
> In dt-schema, reg properties are parsed into a matrix format, which is an
> array of arrays of integers like [[3]]. This schema checks if the first
> element of the outer array matches the scalar values 3 or 4, but since
> the first element is an inner array, it won't match a scalar.
> 
> Could this cause the if block to always evaluate to false, causing valid
> ethernet-port nodes to be incorrectly rejected because the parent node
> sets unevaluatedProperties: false?

ack, I will fix it in v6.

> 
> > +        then:
> 
> Since #address-cells and #size-cells are needed for the ethernet@[3-4]
> variants to act as buses for the new ethernet-port children, should they
> be included in a required list within this conditional block?
> 
> Without them, an incomplete device tree could omit these properties and
> silently pass schema validation.

ack, I will fix it in v6.

> 
> > +          patternProperties:
> > +            "^ethernet-port@[0-5]$":
> > +              type: object
> > +              unevaluatedProperties: false
> 
> Does setting unevaluatedProperties: false here without referencing
> ethernet-controller.yaml# forbid standard network properties?
> 
> Since this models a net_device, driver code often expects to parse standard
> properties like mac-address or phy-handle. Device tree authors might not
> be able to configure them without triggering schema validation errors.

ack, I will fix it in v6.

Regards,
Lorenzo

> 
> > +              description: External ethernet port ID available on the GDM port
> > +
> > +              properties:
> > +                compatible:
> > +                  const: airoha,eth-port
> > +
> > +                reg:
> > +                  maxItems: 1
> > +                  description: External ethernet port identifier
> > +
> > +              required:
> > +                - compatible
> > +                - reg
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260509-airoha-eth-multi-serdes-v5-0-805e38edc2aa@kernel.org?part=1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]