Re: [PATCH v10 21/24] firmware: arm_scmi: Add System Telemetry driver

[Fayssal Benmlih]

Hi Cristian,

Thanks for the V10 fd-installation and instance-lifetime rework. Delaying
fd_install() until after all copyouts and pinning the instance from device,
anonymous and VMA references address two of the main V7 blockers.

I still see the following implementation issues.

In scmi_tlm_abi_info_get_ioctl(), usize has no upper bound and the actual
copy_struct_from_user() error is still converted to -EFAULT:

        if (copy_struct_from_user(&base, sizeof(base), uptr, usize))
                return -EFAULT;

Please reject unreasonable values, for example above PAGE_SIZE, and return
the helper's actual error so that -E2BIG is preserved.

scmi_tlm_config_set_ioctl() still packs secs and exp without checking that
they fit the SCMI interval encoding or represent an advertised interval.
Please validate both fields before SCMI_TLM_BUILD_UPDATE_INTERVAL().

For a group, state_set() can succeed before collection_configure() fails,
leaving partially applied state. The outer dead_mtx now serializes ioctl
callers, which addresses the interleaving part of my comment, but rollback
or an explicitly reported partial-failure result is still needed.

scmi_tlm_intervals_get_ioctl() can dereference a NULL per-group intervals
pointer when interval enumeration failed or the group does not support
per-group interval configuration. Please return the saved enumeration error
or -EOPNOTSUPP first.

The capacity test also still appears reversed:

        if (ivs.num_intervals > tlm_ivs->num_intervals)
                return -ENOSPC;

A smaller output capacity returns a successful prefix, while a larger
capacity is rejected. The output count is not changed to report the required
firmware count. Please use normal capacity semantics and allocate/copy only
the actual number of intervals.

scmi_tlm_to_uapi_intervals() still uses |= for the output-only DISCRETE bit.
If userspace supplies that bit and firmware reports segmented intervals, it
remains set. Please clear the output bit first and derive it solely from the
firmware descriptor.

scmi_tlm_batch_initialize() still copies the initial contents of the status
array from userspace. Successful entries are never assigned afterward, so
they can retain arbitrary userspace values. Since states is an output array,
please retain the kvzalloc-initialized zeros and only write negative errors
for failed entries.

SET_ALL_CFG remains partially applied if one or more state_set() calls fail.
V10 now continues through every DE and returns the last error, but userspace
still cannot identify which DEs changed. Please provide rollback or a
per-entry completion result, or define the partial-completion contract
explicitly.

The generic list handling rejects num_items greater than the available
count while accepting a smaller count and returning a prefix. This preserves
the reversed UUID-list capacity behavior reported on V7. Please implement
and document consistent capacity semantics for all four list ioctls.

The mmap validation still accepts a mapping whose VMA length covers len but
not PAGE_ALIGN(offset + len). It rejects only a page count greater than the
expected value, not a page count below it. The addition of len and offset is
also unchecked.

Please require the complete page-rounded mapping, use checked addition, and
either reject nonzero vm_pgoff or document partial mappings. Executable and
private mappings should also be rejected unless they are intentionally part
of the ABI.

Finally, event subscriptions remain instance-wide rather than per open
file. Closing the telemetry fd intentionally leaves the subscription active,
and another opener that knows the cookie can cancel it.

If this lifetime model is intentional, it needs to be documented and
cancellation must be restricted to the owner. Otherwise, please associate
subscriptions with a per-file context and clean them up from release.

Thanks,
Fayçal