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

[Jonathan Cameron]

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

> Add support to parse, track and link TDCF BLK_TS lines to the related
> discovered DataEvents.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>

A few minor things inline.  + I think some of previous patch belongs
in here. Getting patch break up perfect is sometimes a pain, but it
really helps keep things logically together for reviewers and on
a massive set like this, anything that keeps the reviewers happy
is the way to go!

Jonathan

> ---
>  drivers/firmware/arm_scmi/telemetry.c | 317 +++++++++++++++++++++++++-
>  1 file changed, 313 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/telemetry.c b/drivers/firmware/arm_scmi/telemetry.c
> index 810195889484..e3fbcb1e6ea7 100644
> --- a/drivers/firmware/arm_scmi/telemetry.c
> +++ b/drivers/firmware/arm_scmi/telemetry.c


> +
> +static inline void scmi_telemetry_line_free(struct telemetry_line *line)
> +{
> +	void *blob;
> +
> +	switch (line->type) {
> +	case TDCF_BLK_TS_LINE:
> +		blob = to_blkts(line);
> +		kfree(blob);
The local variable isn't adding much over

		kfree(to_blkts(line));

> +		break;
> +	default:
> +		WARN_ON(1);
> +		break;
> +	}
> +}


> +
> +static struct telemetry_block_ts *
> +scmi_telemetry_blkts_create(struct telemetry_shmti *shmti, unsigned long offset)
> +{
> +	struct telemetry_block_ts *bts;
> +	int ret;
> +
> +	bts = kzalloc_obj(*bts);

Use a __free() here and then no_free_ptr() on the return.

> +	if (!bts)
> +		return NULL;
> +
> +	ret = scmi_telemetry_line_init(shmti, &bts->line, offset, TDCF_BLK_TS_LINE);
> +	if (ret) {
> +		kfree(bts);
> +		return NULL;
> +	}
> +
> +	trace_scmi_tlm_collect(0, shmti->info.sid, offset, "SHMTI_NEW_BLKTS");
> +
> +	return bts;
> +}

> +
> +/**
> + * scmi_telemetry_tdcf_blkts_parse  - A BLK_TS line parser
> + *
> + * @ti: A reference to the telemetry_info descriptor
> + * @bts_payld: TDCF payld line to process
> + * @snap: A reference to the working SHMTI snapshot that will be processed
> + * @active_bts: Input/output reference to keep track of the last blk_ts found
> + *
> + * Process a valid TDCF BLK_TS line and, after having looked up or created a
> + * blk_ts descriptor, update the related data and return it as the currently
> + * active blk_ts, given that it is effectively the last found during this
> + * scan.
> + */
> +static void scmi_telemetry_tdcf_blkts_parse(struct telemetry_info *ti,
> +					    struct payload *bts_payld,
> +					    struct shmti_snap *snap,
> +					    struct telemetry_block_ts **active_bts)
> +{
> +	struct telemetry_block_ts *bts, *previous = *active_bts;
> +	struct telemetry_shmti *shmti = snap->shmti;
> +	u32 meta = PAYLD_META_RAW(bts_payld);
> +	unsigned long offset;
> +
> +	/* Check for spec compliance */
> +	if (BLK_TS_INVALID_RAW(meta)) {
> +		trace_scmi_tlm_access(0, "BLK_TS_INVALID", 0, meta);
> +		return;
> +	}
> +
> +	offset = TDCF_OFFS(snap->base, bts_payld);
> +	/* A BLK_TS descriptor MUST be returned: it is found or it is created */
> +	bts = scmi_telemetry_blkts_get_or_create(shmti, offset);
> +	/*
> +	 * On error processing the new BLK_TS bailout taking care to clear
> +	 * the previous last active BLK_TS to avoid mistakenly binding to the
> +	 * next data lines.
> +	 */
> +	if (WARN_ON(!bts))
> +		goto out;
> +
> +	/* Update the descriptor with the lastest TS */
> +	scmi_telemetry_blkts_update_snap(ti, bts, bts_payld, snap->magic);
> +
> +out:
> +	/* Drop one refcount on the previous active since we found a new one */

I'd have this mention where the reference was taken as then it's clear why
it makes sense to drop one here. Something like
	/*
	 * Drop one refcount on previous active to match with the one take
	 * in creation as no longer need to keep it around for potential new
	 * users.
	 */

> +	if (previous)
> +		scmi_telemetry_line_put(&previous->line);
> +
> +	*active_bts = bts;
>  }
>  
>  static inline struct telemetry_de *
> @@ -1561,7 +1801,8 @@ scmi_telemetry_line_data_parse(struct telemetry_de *tde, struct shmti_snap *snap
>  		*tstamp = 0;
>  		if (USE_BLK_TS_RAW(meta)) {
>  			/* Read out the actual BLK_TS */
> -			*tstamp = scmi_telemetry_blkts_read(magic, tde);
> +			*tstamp = scmi_telemetry_blkts_read(magic, tde,
> +							    bts_payld, snap);

Noted in review of previous patch that I'd just have all the parameters
back there rather than adding these two here.  It isn't consistent in that
some parameters were used and others not, as in that patch magic wasn't used
either!

>  		} else if (LINE_TS_VALID_RAW(meta)) {
>  			/*
>  			 * Note that LINE_TS_VALID implies HAS_LINE_EXT and that

> @@ -1906,6 +2187,15 @@ scmi_telemetry_shmti_refresh(struct telemetry_shmti *shmti, int *idx)
>  	return work;
>  }
>  
> +static inline void
> +scmi_telemetry_reset_active_lines(struct telemetry_block_ts **active_bts)
> +{
> +	if (*active_bts) {
> +		scmi_telemetry_line_put(&(*active_bts)->line);
> +		*active_bts = NULL;
> +	}
> +}
> +
>  /**
>   * scmi_telemetry_shmti_scan  - Full SHMTI scan
>   * @ti: A reference to the telemetry info descriptor
> @@ -1976,6 +2266,7 @@ static int scmi_telemetry_shmti_scan(struct telemetry_info *ti,
>  		qwords -= used_qwords;
>  	}
>  
> +	scmi_telemetry_reset_active_lines(&active_bts);

It's only resetting one.  Maybe reset_active_line?

>  	srcu_read_unlock(&shmti->srcu, idx);
>  
>  	if (ret)
> @@ -2120,6 +2411,22 @@ static void scmi_telemetry_resources_free(struct telemetry_info *ti)
>  	dev_dbg(ti->ph->dev, "SCMI Telemetry resources freed for instance\n");
>  }
>  
> +static void scmi_telemetry_lines_cleanup(struct telemetry_info *ti)
> +{
> +	for (int id = 0; id < ti->num_shmti; id++) {
> +		struct telemetry_shmti *shmti = &ti->shmti[id];
> +		struct telemetry_line *line;
> +		unsigned long idx;
> +
> +		guard(mutex)(&shmti->lines_mtx);
> +		/* Drop all remaining currently unbound lines and their containers */

I'd avoid the comment.  It's a _put so should be clear there may be references
held that keep it alive for now. The comment kind of invites questions about what
it means to drop unbound ones here, and what the lifetimes of others look like.

> +		xa_for_each(&shmti->xa_lines, idx, line)
> +			__scmi_telemetry_line_put(line);
> +
> +		xa_destroy(&shmti->xa_lines);
> +	}
> +}