Re: [PATCH net-next v2] dt-bindings: net: Convert HiSilicon hns-dsaf to DT schema

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: additionalProperties false ignores properties from $ref
- [Medium] dt-bindings: missing required properties desc-num, buf-size and subctrl-syscon
- [Low] dt-bindings: driver still uses removed top-level phy-handle property
--

commit d25b601aef3405df25a4432415cfd3e767a90222
Author: Rob Herring (Arm) <robh@kernel.org>
Subject: dt-bindings: net: Convert HiSilicon hns-dsaf to DT schema

This commit converts the HiSilicon hns-dsaf device tree binding from
the old text format to the new YAML schema format. It drops the
top-level phy-handle property and adds the media-type property.

> Convert the HiSilicon hns-dsaf binding to DT schema format.
> 
> Drop 'phy-handle' at top level as there are no users. Add undocumented
> 'media-type' property.

[Severity: Low]
Is it entirely accurate that there are no users of the top-level
phy-handle property?

Looking at hns_mac_init_phy_handle() in
drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c:

    np = of_parse_phandle(mac_cb->dev->of_node, "phy-handle",
                          mac_cb->mac_id);

The driver still explicitly parses this property from the top-level DSAF
node as a fallback if port subnodes are not present. Removing this from
the schema without removing the fallback logic in the driver will cause
out-of-tree or legacy device trees relying on this feature to needlessly
fail schema validation.

Should it remain in the schema (possibly marked as deprecated) until the
C driver is patched to drop support entirely?

[ ... ]
> diff --git a/Documentation/devicetree/bindings/net/hisilicon,hns-dsaf-v1.yaml b/Documentation/devicetree/bindings/net/hisilicon,hns-dsaf-v1.yaml
> new file mode 100644
> index 0000000000000..36251b45abeb9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/hisilicon,hns-dsaf-v1.yaml
[ ... ]
> +patternProperties:
> +  "^port@[0-5]$":
> +    description: DSA Fabric port node
> +    $ref: ethernet-switch-port.yaml#
> +    additionalProperties: false

[Severity: Medium]
Does additionalProperties: false correctly allow properties inherited
from the $ref?

Under JSON Schema semantics, additionalProperties only restricts the
properties explicitly listed in the local object, completely ignoring
any properties evaluated via a $ref.

Would it be better to use unevaluatedProperties: false here so standard
Ethernet port properties aren't incorrectly rejected by dt_binding_check
when present in the device tree?

> +
> +    properties:
> +      reg:
> +        maximum: 5
[ ... ]
> +    required:
> +      - reg
> +      - serdes-syscon
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - mode

[Severity: Medium]
Should desc-num, buf-size, and subctrl-syscon be included in this
required list?

Looking at hns_dsaf_get_cfg() in
drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c, the driver strictly
fails the probe if desc-num or buf-size are omitted.

Furthermore, if subctrl-syscon is missing, the driver's fallback logic
consumes reg index 0 (ppe-base) as the syscon base. Later, when it
attempts to map ppe-base again by name, it fails with an -EBUSY error
and aborts the probe.

Since omitting any of these three properties causes a guaranteed driver
probe failure, shouldn't they be marked as required to ensure invalid
device trees are properly flagged?

> +
> +additionalProperties: false
> +
> +examples:
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930201919.76465-2-robh@kernel.org?part=1