Re: [PATCH v5 00/23] Introduce SCMI Telemetry support

[Fayssal Benmlih <Fayssal.Benmlih@arm.com>]

Overall, v5 is a good direction: dropping the filesystem ABI for a chardev/ioctl ABI is simpler, easier to version, and more appropriate for high-rate telemetry. The split between SCMI protocol support, UAPI, driver, docs, and test tool is also much cleaner. The main concern is that there are several correctness and usability issues that should be fixed before merge.

Must-Fix Issues

-scmi_tlm_to_uapi_base_info() uses out->flags |= ... on an uninitialized local struct scmi_tlm_base_info base; this can leak garbage flags. Use out->flags = ... or zero-initialize base.
-scmi_tlm_update_interval.exp is documented and used as signed, but the UAPI declares it as __u32. It should be __s32, otherwise examples like exp = -3 are ABI-broken.
-Group bounds checks use grp_id > num_groups; they should use grp_id >= num_groups to avoid out-of-bounds access.
-Avoid casting UAPI integer fields to bool *, especially __u32 enable/t_enable in scmi_tlm_de_config. Use local bool enable = !!tcfg.enable; variables and copy normalized values back explicitly if needed.
-Array sizes from userspace need overflow and allocation hardening: use array_size(), kvmalloc_array(), and clear max limits for num_des, num_samples, num_grps, etc.
-SCMI_TLM_GET_SHMTI_LIST loops over the user-provided count after only checking it is not too small; if it is too large it can read beyond in->shmtis[]. Loop over the actual firmware count.
-The mmap path needs tighter validation: reject oversized mappings, handle mmap offsets deliberately, set appropriate VMA flags, and document the lifetime/security model for exposed SHMTI memory.

Design Feedback

-No ABI version/capability ioctl. GET_INFO gives protocol/platform info, but userspace also needs driver ABI version and supported ioctl feature bits.
-Enumeration is awkward. List ioctls should support a clear two-call pattern: call with count 0 to get required count, allocate, call again. Returning EINVAL for “buffer too small” is not very ergonomic.
-No generation counter yet. Userspace needs to detect config/resource changes between discovery and read. This should probably land with the first ABI.
-Global config is dangerous. SET_CFG, SET_DE_CFG, and SET_ALL_CFG mutate shared firmware state. Multiple users can race or disrupt each other. Consider documenting ownership semantics, adding locking/session model, or exposing read-only access separately.
-SET_ALL_CFG is not atomic. If configuring all DEs fails midway, userspace gets partial state. Either make that explicit, add rollback/best-effort reporting, or prefer batched config with per-item status.
-No event/poll model. For telemetry, userspace may want poll()/blocking reads/notification-driven collection rather than repeated synchronous ioctls.
-Timestamp usability is underspecified. Userspace needs clear timestamp domain, rate conversion, wrap behavior, and correlation to Linux clocks.
-Raw SHMTI mmap is powerful but has risks. It needs very clear lifetime, permissions, cacheability, offset/length, stale-data, and security documentation.
-Batch APIs lack per-sample status. In BATCH_READ, one bad DE currently appears to fail the whole request. For tools, per-entry status is much better.
-Text read() plus binary ioctls feels split-brain. Either document read() as debug/convenience only, or remove it from the stable interface and keep the ABI purely binary; no need for two different user interfaces with different semantics.
-Rename the generic #define SCMI 0xF1 to something scoped like SCMI_TLM_IOCTL_MAGIC; exporting a broad SCMImacro in UAPI is collision-prone.
-Add .compat_ioctl = compat_ptr_ioctl or equivalent, and document that all structs are 32/64-bit compatible. The __u64user pointers help, but compat behavior should be explicit.
-Clarify concurrency semantics: what happens when two processes enable/disable DEs, change intervals, or read while config changes? A generation counter/reset ioctl, already listed as TODO, should probably be part of the first ABI version.
-Define permissions. Configuration ioctls mutate global firmware telemetry state, so access control/default device mode/udev expectations should be documented.

Suggested additions before ABI freeze

-SCMI_TLM_GET_ABI_INFO
-SCMI_TLM_GET_GENERATION
-SCMI_TLM_RESET
-batched GET/SET_DE_CFG with per-entry status
-pollable notification/event support, if telemetry notifications are meaningful to userspace
-optional read-only open mode or permission model for non-mutating tools

Usability / Docs

-The docs need more precise examples for two-call enumeration, error handling, batch reads, and mmap cleanup/close semantics.
-Fix typos before reposting: “Systemn”, “dwscriptors”, “istance”, “udpated”, “andc”, “A IOCTL”.
-The sample tool should either be promoted into a real selftest-style utility or clearly kept out of the upstreamable set until it has robust argument parsing and error reporting.

Bottom Line

The architecture is promising, but the UAPI needs one more hardening pass before it becomes ABI: The ioctl set covers the basic workflow, but some parts are still hard for user space tools to consume robustly, especially around discovery, concurrency, partial failure, and long-running collection. Those should be fixed before treating the UAPI as stable. I would also focus v6 on fixing the concrete correctness bugs, tightening mmap/security semantics, adding compat support, and making enumeration/configuration behavior predictable for production userspace.

-Fayçal