Re: [PATCH net-next 6/8] netlink: specs: devlink: populate multi-attr attrs for region read and line card

[netdev-bot+sashiko@kernel.org]

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

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

- [High] The commit message enumerates three repeated attributes that
  need both `multi-attr: true` and an entry in the operation attribute…

--

netlink: specs: devlink: populate multi-attr attrs for region read and line card

This adds multi-attr: true to the region-snapshot, region-chunk and
linecard-type attributes, and adds region-chunks to the region-read reply
plus the linecard state/type attributes to the linecard-get reply.  The
stated goal is that all three repeated attributes get both the multi-attr
property and an entry in the attribute list used by C code generation.

> diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml
> index f23466fb27f97..9d816a0b7da41 100644
> --- a/Documentation/netlink/specs/devlink.yaml
> +++ b/Documentation/netlink/specs/devlink.yaml
> @@ -1264,6 +1264,7 @@ attribute-sets:
>      attributes:
>        -
>          name: region-snapshot
> +        multi-attr: true

[Severity: High]
The commit message says that for all three attributes "the attributes
themselves are missing from the attr list used by C code gen".  For
region-chunks and the linecard attributes the patch adds them to the
respective replies, but is the region-snapshots half missing?

Grepping the spec, region-snapshots only appears in its own definition in
the devlink attribute set and as the dl-region-snapshots subset name; no
operation lists it.  The region-get reply is still:

        reply: &region-get-reply
          value: 42
          attributes: *region-id-attrs

where region-id-attrs is {bus-name, dev-name, index, port-index,
region-name}.

Since ynl-gen-c only generates parsing code for attribute sets reachable
from an operation's attribute list, does the new multi-attr: true on
region-snapshot have any effect?  The generated region-get reply struct
would still have no snapshot list.

There is also a related gap in what the spec describes: for every
REGION_GET do and dump message, devlink_nl_region_fill() in
net/devlink/region.c emits attributes the reply does not declare:

	err = devlink_nl_put_u64(msg, DEVLINK_ATTR_REGION_SIZE, region->size);
	...
	err = nla_put_u32(msg, DEVLINK_ATTR_REGION_MAX_SNAPSHOTS, region->max_snapshots);
	...
	err = devlink_nl_region_snapshots_id_put(msg, devlink, region);

and devlink_nl_region_snapshots_id_put() nests one
DEVLINK_ATTR_REGION_SNAPSHOT per entry of region->snapshot_list:

	snapshots_attr = nla_nest_start_noflag(msg, DEVLINK_ATTR_REGION_SNAPSHOTS);
	...
	list_for_each_entry(snapshot, &region->snapshot_list, list) {
		err = devlink_nl_region_snapshot_id_put(msg, devlink, snapshot);

Would it make sense to add region-snapshots (and region-size /
region-max-snapshots) to the region-get reply, or otherwise to narrow the
commit message to the two operations the patch actually completes?

Checking the tip of the series (59624e85e9fb), the region-get reply is
still attributes: *region-id-attrs, so no later patch in the series
changes this.

> @@ -2045,6 +2048,7 @@ operations:
>              - index
>              - port-index
>              - region-name
> +            - region-chunks
>  

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910200312.2665792-1-kuba%40kernel.org