Re: [PATCH v15 16/35] firmware: arm_scmi: Add Telemetry TDCF parsing for UUID lines

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

On Mon,  9 Nov 2026 09:12:02 +0000
Cristian Marussi <cristian.marussi@arm.com> wrote:

> Add support to parse, track and link TDCF UUID lines to the related
> discovered DataEvents.
> 
> Maintain also a central list of discovered UUID lines.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
Trivial stuff + a follow on about how you are handling the
devm_ cleanup for the de and it making your code more complex
than it needs to be.  I think a simple helper will get
you to a better state (and my preferred approach of fine grained
management is probably overkill!)

Jonathan

> ---
> v14 --> v15
>  - use proper casting in tlm_collect
>  - use snapshots to collect UUIDs during full scans
>  - stop tracking UUIDs lines singularly..it's useless
>  - use UUIDs line hashing for lookup
>  - clarified UUID/DE binding are permanent
>  - fixed WARN_ON invocation on missing active_uuid
> v13 --> v14
>  - split out of previous single patch implementing parse SHMTI routines
> ---
>  drivers/firmware/arm_scmi/telemetry.c | 277 +++++++++++++++++++++++++-
>  include/linux/scmi_protocol.h         |   2 +
>  2 files changed, 277 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/telemetry.c b/drivers/firmware/arm_scmi/telemetry.c
> index e3fbcb1e6ea7..38a94d66116f 100644
> --- a/drivers/firmware/arm_scmi/telemetry.c
> +++ b/drivers/firmware/arm_scmi/telemetry.c

>  
> +static int scmi_telemetry_uuids_update(struct telemetry_info *ti,
> +				       struct telemetry_uuid *uuid)
> +{
> +	uuid_t **cur_uuids;
> +
> +	/* Resize array if needed ... */
> +	if (ti->info.num_uuids + SCMI_UUID_DB_THRESH >= ti->uuids_len) {
> +		uuid_t **new_uuids, **old_uuids;
> +		unsigned int new_len = ti->uuids_len * 2;
> +
> +		new_uuids = kzalloc_objs(*new_uuids, new_len);
> +		if (!new_uuids)
> +			return -ENOMEM;
> +
> +		old_uuids = rcu_dereference_protected(ti->info.uuids,
> +						      lockdep_is_held(&ti->uuids_mtx));
> +		/* Copy/move old allocated UUIDs */
> +		for (int i = 0; i < ti->info.num_uuids; i++)
> +			new_uuids[i] = old_uuids[i];

memcpy?

> +
> +		/* Expose new UUID pointer */
> +		rcu_assign_pointer(ti->info.uuids, new_uuids);
> +		ti->uuids_len = new_len;
> +
> +		kfree_rcu_mightsleep(old_uuids);
> +	}
> +
> +	/* Store new UUID reference into the globally accessible UUIDS */
> +	cur_uuids = rcu_dereference_protected(ti->info.uuids,
> +					      lockdep_is_held(&ti->uuids_mtx));
> +	cur_uuids[ti->info.num_uuids] = &uuid->uuid;
> +	rcu_assign_pointer(ti->info.uuids, cur_uuids);
> +	/* Update UUIDs count */
> +	smp_store_release(&ti->info.num_uuids, ti->info.num_uuids + 1);
> +
> +	return 0;
> +}



> @@ -2362,6 +2554,7 @@ static int scmi_telemetry_resources_alloc(struct telemetry_info *ti)
>  	}
>  
>  	INIT_LIST_HEAD(&ti->fcs_des);
> +	hash_init(ti->uuids_ht);

Why init the hash here but the mutex at the caller? I guess
maybe you wanted it to be obvious the mutex was valid, but to me
just doing mutex_init up here is easier.

>  
>  	ti->shmti = no_free_ptr(shmtis);
>  	ti->info.shmtis = no_free_ptr(shmtis_info);
> @@ -2427,6 +2620,37 @@ static void scmi_telemetry_lines_cleanup(struct telemetry_info *ti)
>  	}
>  }

>  static void scmi_telemetry_tdes_cleanup(struct telemetry_info *ti)
>  {
>  	struct scmi_telemetry_de *de;
> @@ -2471,6 +2695,7 @@ static void scmi_telemetry_instance_cleanup(void *arg)
>  
>  	scmi_telemetry_groups_free(ti->rinfo);
>  	scmi_telemetry_tdes_cleanup(ti);
> +	scmi_telemetry_uuids_cleanup(ti);

See below.  The monolithic approach to devm cleanup is making
your setup code harder to read.  Suggestions down there on how
to avoid that.

>  	scmi_telemetry_lines_cleanup(ti);
>  	scmi_telemetry_resources_free(ti);
>  }

>  /**
>   * scmi_telemetry_instance_init  - Instance initializer
>   * @ti: A reference to the telemetry info descriptor for this instance
> @@ -2563,6 +2805,8 @@ scmi_telemetry_resources_enumerate(struct telemetry_info *ti)
>   */
>  static int scmi_telemetry_instance_init(struct telemetry_info *ti)
>  {
> +	unsigned int uuids_len;
> +	uuid_t **cur_uuids;
>  	int ret;
>  
>  	ret = scmi_telemetry_resources_alloc(ti);
> @@ -2571,6 +2815,35 @@ static int scmi_telemetry_instance_init(struct telemetry_info *ti)
>  
>  	xa_init(&ti->xa_des);
>  
> +	/*
> +	 * Always allocate at least one slot for the primary and anyway at
> +	 * least enough to avoid immediate resizing, assuring uuids_len
> +	 * always greater or equal to one.
> +	 */
> +	uuids_len = max(ti->num_shmti * 2, SCMI_UUID_DB_THRESH + 1);
> +	cur_uuids = kzalloc_objs(*cur_uuids, uuids_len);
> +	if (!cur_uuids) {
> +		xa_destroy(&ti->xa_des);
> +		scmi_telemetry_resources_free(ti);
This sort of dance of not using a goto block because we
have unified cleanup at the end always suggests to me one of two things.
Finer grained devm_ callbacks, or a helper that can have a clean return
to this outer function with no side effects, those having all been
cleaned up via goto based unified error handling in the helper.

I personally prefer fine grained devm with each element clearly
being paired with the undo, but the helper is perhaps easier to
introduce here.

> +		return -ENOMEM;
> +	}
> +
> +	mutex_init(&ti->uuids_mtx);
> +	/* Expose new UUID pointer */
> +	scoped_guard(mutex, &ti->uuids_mtx) {
> +		rcu_assign_pointer(ti->info.uuids, cur_uuids);
> +		ti->uuids_len = uuids_len;
> +	}
> +
> +	/* Initialize the first UUIDs in the global list with the primary */
> +	ret = scmi_telemetry_primary_uuid_init(ti);
> +	if (ret) {
> +		scmi_telemetry_uuids_cleanup(ti);
> +		xa_destroy(&ti->xa_des);
> +		scmi_telemetry_resources_free(ti);
> +		return ret;
> +	}
> +
>  	atomic_set(&ti->des_enabled[ENA_STATE], 0);
>  	atomic_set(&ti->des_enabled[ENA_TSTAMP], 0);
>  	mutex_init(&ti->des_enabled_mtx);
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index b7b8bd77b37d..3973779c0aba 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -981,6 +981,8 @@ struct scmi_telemetry_info {
>  	unsigned int active_update_interval;
>  	struct scmi_telemetry_intervals *intervals;
>  	struct scmi_telemetry_shmti_info **shmtis;
> +	unsigned int num_uuids;
> +	uuid_t * __rcu *uuids __counted_by_ptr(num_uuids);
>  };
>  
>  /**