Re: [PATCH v14 08/32] firmware: arm_scmi: Add Telemetry resources enumeration

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Fri, 16 Oct 2026 19:55:29 +0100
Cristian Marussi <cristian.marussi@arm.com> wrote:

> Add core Telemetry initialization routines in charge of basic allocations
> and DataEvents enumerations.

Needs more about why you have made key design decisions in here.

> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v13 --> v14
>  - splitted out of the old Basic Telemetry patch, contains the basic
>    instance allocations and the DEs enumeration process
>  - renamed some fields to more strictly adhere to spec nomenclature
Even split out this remains challenging to review.

I've asked for a few comments inline for why certainly structures are used
and stored in arrays when they look like they could just be directly embedded
in each other.

They tend to surface much later when you add the userspace ABI.

> ---
>  drivers/firmware/arm_scmi/telemetry.c | 633 ++++++++++++++++++++++++++
>  include/linux/scmi_protocol.h         |  38 ++
>  2 files changed, 671 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/telemetry.c b/drivers/firmware/arm_scmi/telemetry.c
> index 32f6f8d1106d..87ff75805935 100644
> --- a/drivers/firmware/arm_scmi/telemetry.c
> +++ b/drivers/firmware/arm_scmi/telemetry.c
> @@ -8,15 +8,19 @@
>  #include <linux/atomic.h>
>  #include <linux/bitfield.h>
>  #include <linux/compiler_types.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/limits.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/sprintf.h>
>  #include <linux/string.h>
>  #include <linux/uuid.h>
> +#include <linux/xarray.h>
>  
>  #include "protocols.h"
>  #include "notify.h"
> @@ -51,13 +55,189 @@ struct scmi_msg_resp_telemetry_protocol_attributes {
>  	__le32 default_blk_ts_rate;
>  };
>  
> +struct de_desc_fastchannel {
> +	__le32 addr_low;
> +	__le32 addr_high;
> +	__le32 size;
> +};
> +
> +struct scmi_de_desc {
> +	__le32 de_id;
> +	__le32 group_id;
> +	__le32 de_data_sz;
> +	__le32 de_attributes_1;

Maybe de_attrs_1 is enough?  Would shorted a few lines!


> +#define	IS_NAME_SUPPORTED(d)	(le32_get_bits((d)->de_attributes_1, BIT(31)))
> +#define	IS_FC_SUPPORTED(d)	(le32_get_bits((d)->de_attributes_1, BIT(30)))
> +#define	GET_DE_TYPE(d)		(le32_get_bits((d)->de_attributes_1, GENMASK(29, 22)))
> +#define	IS_PERSISTENT(d)	(le32_get_bits((d)->de_attributes_1, BIT(21)))
> +#define GET_DE_UNIT_EXP(d)							\
> +	({									\
> +		__u32 __signed_exp =						\
> +			le32_get_bits((d)->de_attributes_1, GENMASK(20, 13));	\
> +										\
> +		sign_extend32(__signed_exp, 7);					\
> +	})
> +#define	GET_DE_UNIT(d)		(le32_get_bits((d)->de_attributes_1, GENMASK(12, 5)))
> +#define	TSTAMP_SUPPORT(d)	(le32_get_bits((d)->de_attributes_1, GENMASK(1, 0)))
> +	__le32 de_attributes_2;
> +#define	GET_DE_INSTA_ID(d)	(le32_get_bits((d)->de_attributes_2, GENMASK(31, 24)))
> +#define	GET_COMPO_INSTA_ID(d)	(le32_get_bits((d)->de_attributes_2, GENMASK(23, 8)))
> +#define	GET_COMPO_TYPE(d)	(le32_get_bits((d)->de_attributes_2, GENMASK(7, 0)))
> +	__le32 de_attributes_3;
> +};
> +
> +struct scmi_msg_resp_telemetry_de_description {
> +	__le32 num_desc;

I'd add a comment about this being a few values packed
together to just avoid someone saying __counted_by_le()
is appropriate - the name rather suggests it is!

> +	struct scmi_de_desc desc[]; 
> +};

> +
> +struct telemetry_de {
> +	enum timestamps ts_type;
> +	u32 ts_rate;

I think some of this is not yet used and it would be better to bring
elements in only when we can see how they are used.
The ts_rate nested some layers below is the one initialized in this
patch I think.

> +	bool enumerated;
> +	void __iomem *base;
> +	u32 sid;
> +	u32 offset;
> +	/* NOTE THAT DE data_sz is registered in scmi_telemetry_de */
> +	u32 fc_size;
> +	struct list_head item;
> +	struct scmi_telemetry_de de;
> +	/* Protect last_val/ts/magic accesses - MUST BE KEPT LAST */
> +	struct mutex mtx;
> +};
> +
> +#define to_tde(d)	container_of(d, struct telemetry_de, de)
> +
>  struct telemetry_info {
>  	bool streaming_mode;
>  	unsigned int num_shmti;
>  	unsigned int num_des_tstamp;
>  	unsigned int default_blk_ts_rate;
>  	const struct scmi_protocol_handle *ph;
> +	struct telemetry_de *tdes;
> +	struct xarray xa_des;
> +	/* Mutex to protect access to @free_des */
> +	struct mutex free_mtx;
> +	struct list_head free_des;
> +	struct list_head fcs_des;
>  	struct scmi_telemetry_info info;
> +	atomic_t rinfo_initializing;
> +	struct completion rinfo_initdone;
> +	struct scmi_telemetry_res_info *rinfo;
> +	struct scmi_telemetry_res_info *(*res_get)(struct telemetry_info *ti);
> +};

> +static struct telemetry_de *scmi_telemetry_tde_get(struct telemetry_info *ti,
> +						   unsigned int de_id)
> +{
> +	struct telemetry_de *tde;
> +
> +	/* Pick a new tde */

Not sure the comment adds anything as function is well enough named.

> +	tde = scmi_telemetry_free_tde_get(ti);
> +	if (!tde) {
> +		dev_err(ti->ph->dev, "Cannot get TDE for ID:0x%08X\n", de_id);
> +		return ERR_PTR(-ENOSPC);
> +	}
> +
> +	tde->sid = SHMTI_ID_INVALID;
> +
> +	return tde;
> +}
>
> +
> +struct scmi_tlm_de_priv {
> +	struct telemetry_info *ti;
> +	void *next;
> +	size_t rx_len;
>  };
>  
>  static int
> @@ -99,6 +279,221 @@ scmi_telemetry_protocol_attributes_get(struct telemetry_info *ti)
>  	return ret;
>  }
>  
> +static void iter_tlm_prepare_message(void *message,
> +				     unsigned int desc_index, const void *priv)
> +{
> +	put_unaligned_le32(desc_index, message);
> +}
> +
> +static int iter_de_descr_update_state(struct scmi_iterator_state *st,
> +				      const void *response, void *priv)
> +{
> +	const struct scmi_msg_resp_telemetry_de_description *r = response;
> +	struct scmi_tlm_de_priv *p = priv;
> +
> +	st->num_returned = le32_get_bits(r->num_desc, GENMASK(15, 0));
> +	st->num_remaining = le32_get_bits(r->num_desc, GENMASK(31, 16));
> +
> +	if (st->rx_len < (sizeof(*r) + sizeof(r->desc[0]) * st->num_returned))
> +		return -EINVAL;
> +
> +	/* Initialized to first descriptor and initial payload size */
> +	p->next = (void *)r->desc;

Can p->next just be const?  Would avoid need for casting here.

> +	p->rx_len = st->rx_len - sizeof(*r);
> +
> +	return 0;
> +}

> +
> +static int scmi_telemetry_de_descriptor_parse(struct telemetry_info *ti,
> +					      struct telemetry_de *tde,
> +					      void **next, size_t *rx_len)
> +{
> +	const struct scmi_de_desc *desc = *next;
> +	size_t payld_sz = sizeof(*desc);
> +
> +	lockdep_assert_held(&tde->mtx);
> +
> +	if (*rx_len < payld_sz)
> +		return -ENOSPC;
> +
> +	tde->de.info->id = le32_to_cpu(desc->de_id);
> +	tde->de.info->grp_id = le32_to_cpu(desc->group_id);
> +	tde->de.info->data_sz = le32_to_cpu(desc->de_data_sz);
> +	tde->de.info->type = GET_DE_TYPE(desc);
> +	tde->de.info->unit = GET_DE_UNIT(desc);
> +	tde->de.info->unit_exp = GET_DE_UNIT_EXP(desc);
> +	tde->de.info->instance_id = GET_DE_INSTA_ID(desc);
> +	tde->de.info->compo_instance_id = GET_COMPO_INSTA_ID(desc);
> +	tde->de.info->compo_type = GET_COMPO_TYPE(desc);
> +	tde->de.info->persistent = IS_PERSISTENT(desc);
> +	tde->ts_type = TSTAMP_SUPPORT(desc);
> +	tde->de.tstamp_support = !!tde->ts_type;
> +	tde->de.fc_support = IS_FC_SUPPORTED(desc);
> +	tde->de.name_support = IS_NAME_SUPPORTED(desc);
> +	/* Update DE_DESCRIPTOR size for the next iteration */
> +	*next += sizeof(*desc);
> +
> +	payld_sz += scmi_telemetry_tde_var_size_calc(tde);
> +	if (*rx_len < payld_sz)
> +		return -ENOSPC;
> +
> +	if (tde->ts_type == TSTAMP_LINE) {
> +		__le32 *line_ts_rate = *next;
> +
> +		tde->de.info->ts_rate = le32_to_cpu((*line_ts_rate));
> +
> +		/* Variably sized depending on TS support */
> +		*next += sizeof(*line_ts_rate);
> +	} else if (tde->ts_type == TSTAMP_BLK) {
> +		/* Setup default BLK TS value at first */
> +		tde->de.info->ts_rate = ti->default_blk_ts_rate;
> +	}
> +
> +	if (tde->de.fc_support) {
> +		u32 size;
> +		u64 phys_addr;
> +		void __iomem *addr;
> +		struct de_desc_fastchannel *dfc;
> +
> +		dfc = *next;
> +		phys_addr = le32_to_cpu(dfc->addr_low);
> +		phys_addr |= (u64)le32_to_cpu(dfc->addr_high) << 32;

Does
		phys_addr = get_unaligned_le64(&dfc->addr_low);
work for this?  For me that would provide simple documentation of
the maths.

> +
> +		size = le32_to_cpu(dfc->size);
> +		addr = devm_ioremap(ti->ph->dev, phys_addr, size);
> +		if (!addr)
> +			return -EADDRNOTAVAIL;
> +
> +		tde->base = addr;
> +		tde->offset = 0;
> +		tde->fc_size = size;
> +
> +		/* Variably sized depending on FC support */
> +		*next += sizeof(*dfc);
> +	}
> +
> +	if (tde->de.name_support) {
> +		const char *de_name = *next;
> +
> +		strscpy(tde->de.info->name, de_name, SCMI_SHORT_NAME_MAX_SIZE);
> +		/* Variably sized depending on name support */
> +		*next += SCMI_SHORT_NAME_MAX_SIZE;
> +	}
> +
> +	/* Count timestamped DEs */
> +	ti->num_des_tstamp += !!tde->de.tstamp_support;
> +
> +	*rx_len -= payld_sz;
> +
> +	return 0;
> +}
> +
> +static int iter_de_descr_process_response(const struct scmi_protocol_handle *ph,
> +					  const void *response,
> +					  struct scmi_iterator_state *st,
> +					  void *priv)
> +{
> +	struct scmi_tlm_de_priv *p = priv;
> +	struct telemetry_info *ti = p->ti;
> +	const struct scmi_de_desc *desc = p->next;
> +	struct telemetry_de *tde;
> +	bool discovered = false;
> +	unsigned int de_id;
> +	int ret;
> +
> +	de_id = le32_to_cpu(desc->de_id);
> +	/* Check if this DE has already been discovered by other means... */

Maybe mention what 'other means?' as not locally obvious.

> +	tde = scmi_telemetry_tde_lookup(ti, de_id);
> +	if (!tde) {
> +		/* Create a new one */
> +		tde = scmi_telemetry_tde_get(ti, de_id);
> +		if (IS_ERR(tde))
> +			return PTR_ERR(tde);
> +
> +		discovered = true;
> +	}
> +
> +	guard(mutex)(&tde->mtx);
> +	if (tde->enumerated) {
> +		/* Cannot be a duplicate of a DE already created by enumeration */
> +		dev_err(ph->dev,
> +			"Discovered INVALID DE with DUPLICATED ID:0x%08X\n",
> +			de_id);
> +		return -EINVAL;
> +	}
> +
> +	ret = scmi_telemetry_de_descriptor_parse(ti, tde, &p->next, &p->rx_len);
> +	if (ret) {
> +		dev_err(ph->dev, "Malformed DE Descriptor - ret:%d\n", ret);
> +		/* DE not enumerated at this point were created in this call */
> +		if (discovered)
> +			scmi_telemetry_free_tde_put(ti, tde);
> +		return ret;
> +	}
> +
> +	/* Now fully enumerated */
> +	tde->enumerated = true;
> +
> +	if (discovered) {
> +		/* Register if it was not already ... */
I guess this comment is really about the whole block including
if (discovered) (for the if not already part.

If so move the comment outside the if.

> +		ret = scmi_telemetry_tde_register(ti, tde);
> +		if (ret) {
> +			/* DE not enumerated at this point were created in this call */
> +			scmi_telemetry_free_tde_put(ti, tde);
> +			return ret;
> +		}
> +	}
> +
> +	/* Add to FastChannels list */
> +	if (tde->de.fc_support)
> +		list_add(&tde->item, &ti->fcs_des);
> +
> +	return 0;
> +}

>  
> +/**
> + * scmi_telemetry_resources_alloc  - Resources allocation
> + * @ti: A reference to the telemetry info descriptor for this instance
> + *
> + * This allocates and initializes dedicated resources for the maximum possible
> + * number of needed telemetry resources, based on information gathered from
> + * the initial enumeration: these allocations represent an upper bound on
> + * the number of discoverable telemetry resources and they will be later
> + * populated during late deferred further discovery phases.

This could do with some comments to give us a hint why it needs
to be allocated in so many different allocations.

> + *
> + * Return: 0 on Success, errno otherwise
> + */
> +static int scmi_telemetry_resources_alloc(struct telemetry_info *ti)
> +{
> +	/* Array to hold pointers to discovered DEs */
> +	struct scmi_telemetry_de **des __free(kfree) =
> +		kzalloc_objs(*des, ti->info.base.num_des);
> +	if (!des)
> +		return -ENOMEM;
> +
> +	/* The allocated DE descriptors */
> +	struct telemetry_de *tdes __free(kfree) =
> +		kzalloc_objs(*tdes, ti->info.base.num_des);
> +	if (!tdes)
> +		return -ENOMEM;
> +
> +	/* Allocate a set of contiguous DE info descriptors. */

Why do we care that they are contiguous?  That might be the case
but if it isn't important then don't bother mentioning it.
If it does matter, then say why. I think it's not needed
until patch 28 and that's a long way for a reviewer to look!

> +	struct scmi_telemetry_de_info *dei_store __free(kfree) =
> +		kzalloc_objs(*dei_store, ti->info.base.num_des);
> +	if (!dei_store)
> +		return -ENOMEM;
> +
> +	struct scmi_telemetry_res_info *rinfo __free(kfree) =
> +		kzalloc_obj(*rinfo);
> +	if (!rinfo)
> +		return -ENOMEM;
> +
> +	mutex_init(&ti->free_mtx);
> +	INIT_LIST_HEAD(&ti->free_des);
> +	for (int i = 0; i < ti->info.base.num_des; i++) {
> +		mutex_init(&tdes[i].mtx);
> +		guard(mutex)(&tdes[i].mtx);
> +
> +		/* Bind contiguous DE info structures */
> +		tdes[i].de.info = &dei_store[i];
> +		scmi_telemetry_free_tde_put(ti, &tdes[i]);

Does this call actually do anything other than take
some locks to zero stuff that is already zero?

> +	}
> +
> +	INIT_LIST_HEAD(&ti->fcs_des);
> +
> +	ti->tdes = no_free_ptr(tdes);
> +
> +	rinfo->des = no_free_ptr(des);
> +	rinfo->dei_store = no_free_ptr(dei_store);
> +
> +	/* Ensure all of the above assignments are visible */
> +	smp_store_release(&ti->rinfo, no_free_ptr(rinfo));
> +
> +	return 0;
> +}

> +static void scmi_telemetry_instance_cleanup(void *arg)
> +{
> +	struct telemetry_info *ti = arg;
> +
> +	scmi_telemetry_tdes_cleanup(ti);
> +	scmi_telemetry_resources_free(ti);
> +}
> +
> +/**
> + * scmi_telemetry_resources_enumerate  - Enumeration helper
> + * @ti: A reference to the telemetry info descriptor for this instance
> + *
> + * This helper is configured to be called once on the first enumeration
> + * attempt, when triggered by invoking ti->res_get() from somewhere else.
> + *
> + * Once run it substitues itself in ti->res_get() with the simple accessor
> + * __scmi_telemetry_resources_get, which returns a descriptor to the resources
> + * that were possibly discovered.
> + *
> + * Note that, while it attempts to fully enumerate Data Events and Groups, it
> + * does NOT fail when such enumerations fail, instead it simply gives up with
> + * the end result that only a partially populated, but consistent, resources
> + * descriptor will be returned; in such a case the incomplete descriptor will
> + * be marked as NOT fully_enumerated: this design enables the kernel to deal
> + * with badly implemented out-of-spec firmware support while keep on providing
> + * a minimal sane, albeit possibly incomplete, set of telemetry respources.
> + *
> + * Return: A reference to a fully or partially populated resources descriptor
> + */
> +static struct scmi_telemetry_res_info *
> +scmi_telemetry_resources_enumerate(struct telemetry_info *ti)
> +{
> +	struct device *dev = ti->ph->dev;
> +	int ret;
> +
> +	/*
> +	 * Ensure the following initialization can be called only once
> +	 * from one thread of execution.
> +	 */
> +	if (atomic_cmpxchg(&ti->rinfo_initializing, 0, 1)) {
> +		/*
> +		 * When initialization is already ongoing in another thread,
> +		 * just wait for its completion and return the fully or partially
> +		 * populated rinfo.
> +		 */
> +		if (!completion_done(&ti->rinfo_initdone))
> +			wait_for_completion(&ti->rinfo_initdone);
> +
> +		/* Ensure rinfo descriptor is visible */
> +		return smp_load_acquire(&ti->rinfo);
> +	}
> +
> +	/* Note that this code below can be run only once by one thread */
> +	ret = scmi_telemetry_de_descriptors_get(ti);
> +	if (ret) {
> +		dev_err(dev, FW_BUG "Cannot fully enumerate DEs resources. Degraded system.\n");
> +		goto done;
> +	}
> +
> +	ti->rinfo->fully_enumerated = true;
> +done:
> +	/* Disable initialization permanently */
> +	smp_store_release(&ti->res_get, __scmi_telemetry_resources_get);
> +
> +	/* Unblock concurrent threads that have been stalled */
> +	complete_all(&ti->rinfo_initdone);
> +
> +	/* Ensure local rinfo is visible before returning it */
> +	smp_mb();
> +	return ti->rinfo;
> +}
> +
> +/**
> + * scmi_telemetry_instance_init  - Instance initializer
> + * @ti: A reference to the telemetry info descriptor for this instance
> + *
> + * Note that this allocates and initialize all the resources possibly needed
> + * and then setups the @scmi_telemetry_resources_enumerate helper as the
> + * default method for the first call to ti->res_get(): this mechanism enables
> + * the possibility of optionally implementing deferred enumeration policies
> + * which optionally delay the discovery phase and related SCMI message exchanges
> + * to a later point in time.
> + *
> + * Return: 0 on Success, errno otherwise
> + */
> +static int scmi_telemetry_instance_init(struct telemetry_info *ti)
> +{
> +	int ret;
> +
> +	/* Allocate and Initialize on first call... */

That comment made me think that this was a singleton in that
if you called it a second time it would fail. Seems that it just
allocates whenever it is called (which happens to be once) so
I'd drop the comment as adding confusion rather than clarity.

> +	ret = scmi_telemetry_resources_alloc(ti);
> +	if (ret)
> +		return ret;
> +
> +	xa_init(&ti->xa_des);
> +
> +	/* Setup resources lazy initialization */
> +	atomic_set(&ti->rinfo_initializing, 0);
> +	init_completion(&ti->rinfo_initdone);
> +	/* Ensure the new res_get() operation is visible after this point */
> +	smp_store_mb(ti->res_get, scmi_telemetry_resources_enumerate);
> +
> +	return devm_add_action_or_reset(ti->ph->dev,
> +					scmi_telemetry_instance_cleanup, ti);

If you can make it more obvious what this is matching against in the above function
that would be good. Normally that is done by putting the setup and tear down
next to each other.

Might be better to split it into two callbacks so that you can couple
resources_free with resource_alloc above then the other bit with whatever
that is unwinding.


> +}

> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 121c83e1c6d8..018427daf55f 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -13,6 +13,7 @@
>  #include <linux/device.h>
>  #include <linux/notifier.h>
>  #include <linux/types.h>
> +#include <linux/uuid.h>
>  
>  #define SCMI_MAX_STR_SIZE		64
>  #define SCMI_SHORT_NAME_MAX_SIZE	16
> @@ -825,6 +826,37 @@ struct scmi_pinctrl_proto_ops {
>  	int (*pin_free)(const struct scmi_protocol_handle *ph, u32 pin);
>  };
>  
> +struct scmi_telemetry_de_info {
> +	unsigned int id;
> +	unsigned int grp_id;
> +	unsigned int data_sz;
> +	unsigned int type;
> +	unsigned int unit;
> +	int unit_exp;
> +	unsigned int ts_rate;
> +	unsigned int instance_id;
> +	unsigned int compo_instance_id;
> +	unsigned int compo_type;
> +	bool persistent;
> +	char name[16];
> +};
> +
> +struct scmi_telemetry_de {
> +	bool tstamp_support;
> +	bool fc_support;
> +	bool name_support;
> +	struct scmi_telemetry_de_info *info;

I'd be tempted to put the pointer first though perhaps
it is acting as a separator for the bools?  If so I'd
use a comment instead.

> +	bool enabled;
> +	bool tstamp_enabled;
> +};
> +
> +struct scmi_telemetry_res_info {
> +	bool fully_enumerated;
> +	unsigned int num_des;
> +	struct scmi_telemetry_de **des;
> +	struct scmi_telemetry_de_info *dei_store;
> +};
> +
>  struct scmi_telemetry_base_info {
>  	unsigned int version;
>  	uuid_t primary_revision;
> @@ -848,10 +880,16 @@ struct scmi_telemetry_info {
>   *	by SCMI Telemetry Protocol
>   *
>   * @info_get: get the general Telemetry information.
> + * @de_lookup: get a specific DE descriptor from the DE id.
> + * @res_get: get a reference to the Telemetry resources descriptor.
>   */
>  struct scmi_telemetry_proto_ops {
>  	const struct scmi_telemetry_info __must_check *(*info_get)
>  		(const struct scmi_protocol_handle *ph);
> +	const struct scmi_telemetry_de __must_check *(*de_lookup)
> +		(const struct scmi_protocol_handle *ph, u32 id);
> +	const struct scmi_telemetry_res_info __must_check *(*res_get)
> +		(const struct scmi_protocol_handle *ph);

Similar to previous. I'm not seeing the value in the __must_check
markings.  They make sense when the return value isn't the data
just asked for which one would assume would always be used.

>  };
>  
>  /**