Re: [PATCH v7 22/23] [RFC] Documentation: Add SCMI System Telemetry documentation

[Fayssal Benmlih <Fayssal.Benmlih@arm.com>]

Hi Cristian,

A few documentation issues remain inline.

>  - SCMI_TLM_GET_ABI_INFO: Gather ABI versioning detail and basic SCMI
>    Telemetry features like number of resources and supported features.

Please explain the distinction between abi_features and the SCMI instance
features. In particular, reset is usable only when both the ABI and the
firmware instance advertise support.

>  - SCMI_TLM_GET_CFG / SCMI_TLM_SET_CFG: Get or set the whole instance,
>    or a specific group, configuration.
>
>  - SCMI_TLM_GET_DE_CFG / SCMI_TLM_SET_DE_CFG: Get or set the
>    configuration of a specific DataEvent.
>
>  - SCMI_TLM_GET_ALL_CFG / SCMI_TLM_SET_ALL_CFG: Get or set the
>    cumulative configuration of ALL the DataEvents defined on the
>    platform.

Please state that this configuration is global to the SCMI instance, not
per-open-file state.

The documentation should also define concurrent-caller behavior, whether
multi-command operations can interleave, partial failure of SET_ALL_CFG
and batch operations, and which commands require write access or
privilege.

>  - SCMI_TLM_EVENT_REGISTER: Subscribe/unsubscribe to a Telemetry event.

This ioctl name does not exist in the V7 UAPI. The defined command is
SCMI_TLM_EVENT_SUBSCRIBE.

Please also document subscription ownership and lifetime, including what
happens when the telemetry fd or eventfd is closed without an explicit
unsubscribe.

>  - SCMI_TLM_BATCH_READ: Report the last samples for the DataEvents IDs
>    specified within the samples input params.

Please document the per-entry status representation, whether the status
array is optional, what happens to successful entries when a later item
fails without statuses, and the maximum permitted batch count.

>  - SCMI_TLM_GET_SHMTI_LIST: Gather a list of open file descriptors, one
>    for each SHMTI memory area defined for this instance, that can be used
>    to memory-map such areas in the calling process address space [...]

The mmap ABI still needs to specify:

- the exact PAGE_ALIGN(offset + len) mapping length;
- permitted vm_pgoff, protection and sharing flags;
- that returned fds are read-only and close-on-exec;
- cacheability and coherency expectations;
- fd and VMA lifetime across removal, reset and reconfiguration;
- cleanup using munmap() and close();
- security requirements for bytes surrounding an unaligned SHMTI.

> 		/* Enable Telemetry as a whole, set a 400ms update interval */
> 		cfg.enable = 1;
> 		cfg.active.secs = 400;
> 		cfg.active.exp = -3;

Please document the permitted secs/exp ranges and how a requested value is
matched against discrete or segmented advertised intervals.

> 		struct scmi_tlm_de_sample samples[3] = {};
> 		struct scmi_tlm_data_read data = {};
> [...]
> 		data.num_samples = 3;
> 		data.samples = (unsigned long)samples;
>
> 		ret = ioctl(fd, SCMI_TLM_BATCH_READ, &data);

V7's batch handler expects struct scmi_tlm_batch, including item_sz,
items, and optional states. This example still passes struct
scmi_tlm_data_read, so it fails item_sz validation and is smaller than the
structure copied by the handler.

Please update the example to use the V7 batch ABI.

> 		for (int i = 0; i < 3; i++)
> 			fprintf(stdout, "%llu: 0x%08X -> %llu\n",
> 					samples[i].tstamp, samples[i].id,
> 					samples[i].val);

Please document the timestamp domain, clock-rate conversion, wrap
behavior, and whether it can be correlated with a Linux clock.

> 		if (ret)
> 			return ret;
> [...]
> 		return 0;

The example still leaks fd on all successful and most error exits. Please
use one cleanup path and close fd before returning. Examples involving
SHMTIs or batches should similarly demonstrate freeing arrays, closing
returned anonymous fds and unmapping mappings.

Thanks,
Fayçal