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

[Fayssal Benmlih]

Hi Cristian,

A few inline comments below.

> +struct scmi_tlm_des_list {
> +	__u32 num_des;
> +	__u32 pad;
> +	__u64 des;
> +};

The input/output semantics of num_des need to be explicit. The
implementation treats it as input buffer capacity and later replaces it
with the returned count, while the kerneldoc only describes the returned
count.

Please define the same contract consistently for all variable-length
ioctls. Ideally, an undersized or zero-length request should copy the
required count back before returning -ENOSPC. Otherwise userspace has to
obtain counts through several separate, ioctl-specific mechanisms.

> +struct scmi_tlm_de_sample {
> +	__u32 id;
> +	__u32 pad;
> +	__u64 tstamp;
> +	__u64 val;
> +};

BATCH_READ still has no per-entry status. One invalid or temporarily
unavailable DE fails the entire request, and userspace cannot determine
which preceding entries were read successfully.

Could BATCH_READ use a batch-specific entry containing the sample plus an
__s32 status? This would also leave the single-DE sample structure simple.

A maximum permitted batch count should also be defined so that userspace
and the kernel agree on resource limits.

> + * Used by:
> + *	RW - SCMI_TLM_GET_DE_VALUE

SCMI_TLM_GET_DE_VALUE was renamed to SCMI_TLM_DE_READ.

> + * Used by:
> + *	RW - SCMI_TLM_SINGLE_SAMPLE
> + *	RW - SCMI_TLM_BULK_READ
> + *	RW - SCMI_TLM_BATCH_READ

SCMI_TLM_SINGLE_SAMPLE was renamed to SCMI_TLM_SINGLE_READ.

> +#define SCMI_TLM_BASE_SUPPORT_UPDATE_NOTIFICATION	(1 << 3)

The ABI advertises notification support but currently provides no way for
userspace to wait for or consume such notifications. Similarly, the
generation counter added at the protocol layer is not exposed by this
UAPI.

Since these are already listed as ABI TODOs, should the first version
include the generation/event interface, or should this feature bit remain
unexposed until the functionality is consumable from userspace?

> +#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_de_config)
> [...]
> +#define SCMI_TLM_SET_ALL_CFG	_IOWR(SCMI_TLM_IOCTL_MAGIC, 0x0A, struct scmi_tlm_de_config)

These implementations only consume their arguments and do not copy
updated structures back. Unless returning normalized/current state is
planned, should these be _IOW rather than _IOWR? This should be settled
before the ioctl numbers become ABI.

Thanks,
Fayçal