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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 4

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).

- [Medium] The conversion drops the two leading legacy 'reg' regions
  (sub-control and SerDes) that the old binding documented as the…
- [Medium] The top-level 'required' list in hisilicon,hns-dsaf-v1.yaml
  leaves out 'desc-num' and 'buf-size', although the deleted text…
- [Low] The patch deletes
  Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt, but…
- [Low] The deleted text binding had the buf-size and desc-num ranges
  swapped ('buf-size: ... should be 16-1024', 'desc-num: ... should be…
- [Low] The concern says that 'mc-mac-mask' with only 'maxItems: 6'
  accepts 1–5 bytes, while hns_mac_get_info() needs exactly ETH_ALEN (6)…
- [Low] reg and reg-names use 'minItems: 1' for every 'mode'.

--

dt-bindings: net: Convert HiSilicon hns-dsaf to DT schema

This commit converts the HiSilicon hns-dsaf text binding to a YAML DT
schema. It drops the unused top-level phy-handle property and documents
the previously undocumented media-type port property.

> 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

[ ... ]

> +  reg:
> +    minItems: 1
> +    items:
> +      - description:
> +          PPE register base and size
> +      - description:
> +          DSA Fabric base register and size (not required for single-port mode)
> +
> +  reg-names:
> +    minItems: 1
> +    items:
> +      - const: ppe-base
> +      - const: dsaf-base

[Severity: Low]
minItems: 1 applies to every mode here. Would the schema accept a node
with mode = "6port-16rss" and only reg-names = "ppe-base"?

The description says dsaf-base may only be left out in single-port mode.
hns_dsaf_get_cfg() skips it only when HNS_DSAF_IS_DEBUG() is true, which
means dsaf_mode == DSAF_MODE_DISABLE_SP:

	if (!HNS_DSAF_IS_DEBUG(dsaf_dev)) {
		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
						   "dsaf-base");
		if (!res) {
			res = platform_get_resource(pdev, IORESOURCE_MEM,
						    res_idx);
			...
		}
		dsaf_dev->io_base = devm_ioremap_resource(&pdev->dev, res);

When subctrl-syscon is used, res_idx is still 0 at this point. The
fallback therefore picks the ppe-base region again, and
devm_ioremap_resource() fails with -EBUSY.

Could an if/then on mode require two reg and reg-names entries for
2port-64vf, 6port-16rss and 6port-16vf?

[ ... ]

> +  buf-size:
> +    description: RX buffer size (bytes)
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [512, 1024, 2048, 4096]
> +
> +  desc-num:
> +    description: number of descriptors in TX and RX queue
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 16
> +    maximum: 1024

[Severity: Low]
This isn't a bug, but these ranges are the reverse of what the deleted
text binding said:

- buf-size: rx buffer size, should be 16-1024.
- desc-num: number of description in TX and RX queue, should be 512, 1024, 2048 or 4096.

The new constraints match the driver. hns_dsaf_get_cfg() checks desc-num
against HNS_DSAF_MIN_DESC_CNT and HNS_DSAF_MAX_DESC_CNT, and it checks
buf-size through hns_rcb_buf_size2type().

The only related note is "Add constraints for buf-size and desc-num" in
the v2 changelog, and that text is dropped when the patch is applied.
Could the commit message say that the old text had these ranges swapped?

[ ... ]

> +      mc-mac-mask:
> +        description: multicast MAC address mask
> +        $ref: /schemas/types.yaml#/definitions/uint8-array
> +        maxItems: 6

[Severity: Low]
hns_mac_get_info() needs exactly ETH_ALEN bytes for this property. With
any other length it falls back to the broadcast mask:

	if (fwnode_property_read_u8_array(mac_cb->fw_port, "mc-mac-mask",
					  mac_cb->mc_mask, ETH_ALEN)) {
		dev_warn(mac_cb->dev,
			 "no mc-mac-mask property, set to default value.\n");
		eth_broadcast_addr(mac_cb->mc_mask);
	}

Does maxItems: 6 on its own reject a mask shorter than 6 bytes? If the
dtschema fixups set minItems equal to maxItems here, this is already
fine. If they don't, is an explicit minItems: 6 needed?

[ ... ]

> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - mode

[Severity: Medium]
Should subctrl-syscon be in this list now?

The old binding allowed two leading reg regions, for sub-control and
SerDes, as an alternative to subctrl-syscon. This conversion drops those
regions, so reg can now only describe ppe-base and dsaf-base.

The OF path in hns_dsaf_get_cfg() still uses the old layout when the
phandle is missing:

	np_temp = of_parse_phandle(np, "subctrl-syscon", 0);
	syscon = syscon_node_to_regmap(np_temp);
	of_node_put(np_temp);
	if (IS_ERR_OR_NULL(syscon)) {
		res = platform_get_resource(pdev, IORESOURCE_MEM,
					    res_idx++);
		...
		dsaf_dev->sc_base = devm_ioremap_resource(&pdev->dev,
							  res);
		...
		res = platform_get_resource(pdev, IORESOURCE_MEM,
					    res_idx++);
		...
		dsaf_dev->sds_base = devm_ioremap_resource(&pdev->dev,
							   res);

Under the new schema reg[0] is ppe-base, so the lookup by name that
follows requests the same region a second time:

	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ppe-base");
	...
	dsaf_dev->ppe_base = devm_ioremap_resource(&pdev->dev, res);

That call fails with -EBUSY. In single-port mode, where only ppe-base is
present, probe fails even earlier with "serdes-ctrl info is needed!".

So a node without subctrl-syscon passes dt_binding_check but can never
probe. The in-tree hip06 and hip07 dtsi files already set
subctrl-syscon, so requiring it would not break them.

Also, the narrowing of reg appears only in the v2 changelog ("Drop
erroneous 1st 2 'reg' entries") and not in the commit body. Could the
body mention it, as it does for the phy-handle and media-type changes?

[Severity: Medium]
The deleted text binding listed buf-size and desc-num under "Required
properties". hns_dsaf_get_cfg() fails probe if either one is missing:

	ret = device_property_read_u32(dsaf_dev->dev, "desc-num", &desc_num);
	if (ret < 0 || desc_num < HNS_DSAF_MIN_DESC_CNT ||
	    desc_num > HNS_DSAF_MAX_DESC_CNT) {
		...
		return -EINVAL;
	}
	...
	ret = device_property_read_u32(dsaf_dev->dev, "buf-size", &buf_size);
	if (ret < 0) {
		...
		return ret;
	}

Should desc-num and buf-size be added to this list as well?

[ ... ]

> diff --git a/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt b/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt
> deleted file mode 100644
> index 8ee4b1cedae80..0000000000000
> --- a/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt
> +++ /dev/null

[Severity: Low]
The ae-handle description in
Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt still points
to this file:

- ae-handle: accelerator engine handle for hns,
  specifies a reference to the associating hardware driver node.
  see Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt

Should it point to hisilicon,hns-dsaf-v1.yaml instead?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930201919.76465-2-robh%40kernel.org