Re: [PATCH v7 19/23] uapi: Add ARM SCMI Telemetry definitions

Hi Cristian,

A few comments on the updated UAPI inline.

struct scmi_tlm_batch {
	__u32 num_items;
	__u32 item_sz;
	__u64 reserved;
	__u64 states;
	__u64 items;
};

Please define an upper bound for num_items. Without a common ABI limit,
callers can request effectively unbounded allocation and iteration, and
userspace does not know which sizes the kernel is expected to support.

 * @states: A reference to an arrays of u32 items representing the
 *	    outcome of the requests for each single item in @items: these are
 *	    ordered in the same order as the @items. - OUT

These statuses contain zero or a negative Linux errno, but the array is
described as u32 while the implementation uses int *. Could this be
specified as an array of __s32 so the signed error-value ABI is explicit?

Is states == 0 explicitly supported? The driver treats it as optional and
stops at the first error when it is absent, potentially after preceding
configuration changes have succeeded. Please document both the optional
pointer and the resulting partial-completion semantics.

#define SCMI_TLM_SET_CFG	_IOWR(SCMI_TLM_IOCTL_MAGIC, 0x02, struct scmi_tlm_config)
[...]
#define SCMI_TLM_SET_DE_CFG	_IOWR(SCMI_TLM_IOCTL_MAGIC, 0x05, struct scmi_tlm_batch)
[...]
#define SCMI_TLM_SET_ALL_CFG	_IOWR(SCMI_TLM_IOCTL_MAGIC, 0x0A, struct scmi_tlm_de_config)

SET_DE_CFG now returns per-DE tracking information, so _IOWR makes sense
for that command.

SET_CFG and SET_ALL_CFG, however, still only consume their arguments and
do not copy a result back. Unless output is planned, should these two
commands be _IOW before the ioctl numbers become ABI?

#define SCMI_TLM_BATCH_READ	_IOWR(SCMI_TLM_IOCTL_MAGIC, 0x10, struct scmi_tlm_data_read)

SCMI_TLM_BATCH_READ is encoded with struct scmi_tlm_data_read, but
scmi_tlm_des_batch_read_ioctl() copies and interprets struct
scmi_tlm_batch. These structures are different sizes, so _IOC_SIZE(cmd)
does not describe what the handler accesses.

Please use struct scmi_tlm_batch here and update the documentation example
accordingly.

A few kerneldoc nits:

- scmi_tlm_abi_info documents @de_impl_version, but the member is named
  primary_de_impl_version.
- scmi_tlm_batch documents @items_sz, but the member is item_sz.
- scmi_tlm_grp_info documents a reserved member that is not present in the
  structure.

Thanks,
Fayçal