Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] New provisioning commands (`fabric-new`, `endpoint-set`, `port-set`, `port-peer-new`, `port-peer-del`) do not reply with a full message; most only ACK on success. - [Medium] The `fabric-new-params` nested attribute introduces an unnecessary extra wrapping nest for request parameters. --
--- Patch [8]: [RFC PATCH 08/12] drm/fabric: add provisioning netlink uAPI --- commit 99dd7c47ab1a93cd9f0234f8280e8cd261d71d5a Author: Ilia Levi <[email protected]> Subject: drm/fabric: add provisioning netlink uAPI This commit extends the DRM fabric YNL specification with six privileged provisioning commands for managing fabrics, endpoints, and ports. > diff --git a/Documentation/netlink/specs/drm_fabric.yaml > b/Documentation/netlink/specs/drm_fabric.yaml > --- a/Documentation/netlink/specs/drm_fabric.yaml > +++ b/Documentation/netlink/specs/drm_fabric.yaml > @@ -108,6 +125,17 @@ attribute-sets: > or DUMP reply carries its snapshot's generation; a notification > carries the generation of its change. Statistics reads do not > advance it. > + # Provisioning request parameters > + - > + name: admin-state > + type: u32 > + enum: admin-state > + doc: Requested administrative state. > + - > + name: fabric-new-params > + type: nest > + nested-attributes: fabric-new-params > + doc: Creation parameters for ``fabric-new`` (full nest). [Severity: Medium] Does this introduce an extra wrapping nest for the request parameters? The generic netlink design rules for new families specify avoiding extra wrapping nests. Wrapping the request parameters inside the fabric-new-params nested attribute rather than placing them directly in the main drm-fabric request attributes creates unnecessary API complexity. [ ... ] > @@ -421,6 +483,97 @@ operations: > reply shape (full fabric nest). > notify: fabric-get > > + - > + name: endpoint-change-ntf > + doc: | > + Endpoint state change notification. > + Reuses the endpoint-get reply shape (full endpoint nest). > + notify: endpoint-get > + > + - > + name: fabric-new > + doc: Create a new empty fabric > + attribute-set: drm-fabric > + flags: [admin-perm] > + do: > + pre: drm-fabric-nl-pre-doit > + post: drm-fabric-nl-post-doit > + request: > + attributes: > + - fabric-new-params > + reply: > + attributes: > + - fabric-id [Severity: Medium] Should this operation reply with a full message representing the object rather than just the fabric-id? The generic netlink design rules require that all operations, especially NEW/ADD, reply with a full message carrying identifying information about the new object. Replying only with the ID forces userspace to issue a separate GET command to retrieve the full state. [ ... ] > + - > + name: endpoint-set > + doc: Attach/detach an endpoint to a fabric and/or set its admin state > + attribute-set: drm-fabric > + flags: [admin-perm] > + do: > + pre: drm-fabric-nl-endpoint-pre-doit > + post: drm-fabric-nl-endpoint-post-doit > + request: > + attributes: > + - endpoint-id > + - dev-name > + - bus-name > + - fabric-id > + - admin-state [Severity: Medium] Does this command intentionally omit a reply block? Without a reply block, this state-mutating operation will only ACK on success. Since once a command only ACKs, that behavior becomes unchangeable uAPI, should this operation be updated to return a full message reflecting the updated state? [ ... ] > + - > + name: port-peer-new > + doc: Set a port's neighbor > + attribute-set: drm-fabric > + flags: [admin-perm] > + do: > + pre: drm-fabric-nl-port-pre-doit > + post: drm-fabric-nl-port-post-doit > + request: > + attributes: > + - endpoint-id > + - port-index > + - peer [Severity: Medium] Like the endpoint-set command above, should this ADD operation define a reply payload rather than defaulting to an ACK-only response? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
