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

On Mon, Aug 03, 2026 at 11:25:20PM +0100, Fayssal Benmlih wrote:
> Hi Cristian,
> 

Hi,

> 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.

This structure is meant to be a generic container of different kind of
objects, so the sensible upper bound depends on the contained object...
....num_des mostly, check which I forgot to add, anyway, in the IOCTl
handling...I'll do that and document this...

> 
> >  * @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?

Sure.

> 
> 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.

I'll do.

> 
> > #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?
> 

I'll double check all of this.

> > #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.

Indeed..some kind of leftover...the puzzling thing is that it worked fine
when tested even with such mismatched IOC_SIZE()...I'll fix

> 
> 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.
> 

...ok...so, beside all of the above bugs (and a few more from Sashiko) to be
certainly fixed in the upcoming series, at this point I assume, from the lack
of further comments, that from your userspace perspective the UAPI/ABI is now
sufficiently complete, feature-wise, and the usage model is acceptable.

Thanks,
Cristian