Re: [PATCH v6 18/22] uapi: Add ARM SCMI Telemetry definitions

Hi Cristian,=0A=
=0A=
A few inline comments below.=0A=
=0A=
> +struct scmi_tlm_des_list {=0A=
> +	__u32 num_des;=0A=
> +	__u32 pad;=0A=
> +	__u64 des;=0A=
> +};=0A=
=0A=
The input/output semantics of num_des need to be explicit. The=0A=
implementation treats it as input buffer capacity and later replaces it=0A=
with the returned count, while the kerneldoc only describes the returned=0A=
count.=0A=
=0A=
Please define the same contract consistently for all variable-length=0A=
ioctls. Ideally, an undersized or zero-length request should copy the=0A=
required count back before returning -ENOSPC. Otherwise userspace has to=0A=
obtain counts through several separate, ioctl-specific mechanisms.=0A=
=0A=
> +struct scmi_tlm_de_sample {=0A=
> +	__u32 id;=0A=
> +	__u32 pad;=0A=
> +	__u64 tstamp;=0A=
> +	__u64 val;=0A=
> +};=0A=
=0A=
BATCH_READ still has no per-entry status. One invalid or temporarily=0A=
unavailable DE fails the entire request, and userspace cannot determine=0A=
which preceding entries were read successfully.=0A=
=0A=
Could BATCH_READ use a batch-specific entry containing the sample plus an=
=0A=
__s32 status? This would also leave the single-DE sample structure simple.=
=0A=
=0A=
A maximum permitted batch count should also be defined so that userspace=0A=
and the kernel agree on resource limits.=0A=
=0A=
> + * Used by:=0A=
> + *	RW - SCMI_TLM_GET_DE_VALUE=0A=
=0A=
SCMI_TLM_GET_DE_VALUE was renamed to SCMI_TLM_DE_READ.=0A=
=0A=
> + * Used by:=0A=
> + *	RW - SCMI_TLM_SINGLE_SAMPLE=0A=
> + *	RW - SCMI_TLM_BULK_READ=0A=
> + *	RW - SCMI_TLM_BATCH_READ=0A=
=0A=
SCMI_TLM_SINGLE_SAMPLE was renamed to SCMI_TLM_SINGLE_READ.=0A=
=0A=
> +#define SCMI_TLM_BASE_SUPPORT_UPDATE_NOTIFICATION	(1 << 3)=0A=
=0A=
The ABI advertises notification support but currently provides no way for=
=0A=
userspace to wait for or consume such notifications. Similarly, the=0A=
generation counter added at the protocol layer is not exposed by this=0A=
UAPI.=0A=
=0A=
Since these are already listed as ABI TODOs, should the first version=0A=
include the generation/event interface, or should this feature bit remain=
=0A=
unexposed until the functionality is consumable from userspace?=0A=
=0A=
> +#define SCMI_TLM_SET_CFG	_IOWR(SCMI_TLM_IOCTL_MAGIC, 0x02, struct scmi_t=
lm_config)=0A=
> [...]=0A=
> +#define SCMI_TLM_SET_DE_CFG	_IOWR(SCMI_TLM_IOCTL_MAGIC, 0x05, struct scm=
i_tlm_de_config)=0A=
> [...]=0A=
> +#define SCMI_TLM_SET_ALL_CFG	_IOWR(SCMI_TLM_IOCTL_MAGIC, 0x0A, struct sc=
mi_tlm_de_config)=0A=
=0A=
These implementations only consume their arguments and do not copy=0A=
updated structures back. Unless returning normalized/current state is=0A=
planned, should these be _IOW rather than _IOWR? This should be settled=0A=
before the ioctl numbers become ABI.=0A=
=0A=
Thanks,=0A=
Fay=E7al=