Re: [PATCH v15 18/35] firmware: arm_scmi: Add Telemetry DataEvent read capabilities

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

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

> Add support for Telemetry operations needed to read DataEvent values and
> timestamps as single entities or all together in a single bulk buffer.
> The returned values are effectiely retrieved from the platform only
> when strictly needed, i.e. when no fresh recent cached value was already
> available.
> The DataEvent values are fetched transparently from the platform origins
> using the proper synchronization and consistency primitives, directly from
> the SHMTIs areas or the FastChannels memory depending on the configuration.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

As far as I can spot there is nothing stopping you introducing the
struct scmi_telemetry_de_sample much earlier in the set and reducing
the churn here.   Does looks like it would be too bad to do so
I'd suggest you do so.

> ---
>  drivers/firmware/arm_scmi/telemetry.c | 678 ++++++++++++++++++++++++--
>  include/linux/scmi_protocol.h         |  24 +
>  2 files changed, 666 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/telemetry.c b/drivers/firmware/arm_scmi/telemetry.c
> index 1f6505fff199..bf37faf99865 100644
> --- a/drivers/firmware/arm_scmi/telemetry.c
> +++ b/drivers/firmware/arm_scmi/telemetry.c


> @@ -695,8 +707,9 @@ scmi_telemetry_tde_cache_stale(struct telemetry_de *tde, u32 magic)
>  }
>  
>  static void
> -scmi_telemetry_tde_cache_update(struct telemetry_de *tde, u64 val,
> -				u64 *tstamp, u32 *magic)
> +scmi_telemetry_tde_cache_update(struct telemetry_de *tde,
> +				struct scmi_telemetry_de_sample *sample,

Better to push this back to the patch that introduced this function.  I don't
think it does any harm to always use the structure for passing in the
val and tstamp.  Do this earlier will reduce churn here and doesn't
look that painful to do.

> +				u32 *magic)
>  {
>  	lockdep_assert_held(&tde->mtx);
>  
> @@ -708,10 +721,37 @@ scmi_telemetry_tde_cache_update(struct telemetry_de *tde, u64 val,
>  		return;
>  
>  	tde->last_magic = magic ? *magic : TDCF_BAD_END_SEQ;
> -	tde->last_val = val;
> -	tde->last_ts = tstamp && scmi_tde_has_tstamp(tde) ? *tstamp : 0;
> -	if (tstamp)
> -		*tstamp = tde->last_ts;
> +	tde->last_val = sample->val;
> +	tde->last_ts = scmi_tde_has_tstamp(tde) ? sample->tstamp : 0;
> +	/* Update returned value too */
> +	sample->tstamp = tde->last_ts;

This is currently rather circular.  I guess the aim is to squash
any stale timestamps.  Probably want a comment about that.

> +
> +	trace_scmi_tlm_collect(tde->last_ts, tde->de.info->id,
> +			       tde->last_val, "CACHED_DE_UPDATED");
> +}
> +
> +static int
> +scmi_telemetry_tde_cache_lookup(struct telemetry_de *tde,
> +				struct scmi_telemetry_de_sample *sample,
> +				u32 *magic)
> +{
> +	lockdep_assert_held(&tde->mtx);
> +
> +	/*
> +	 * Bailout without providing any sample when the cache is
> +	 * stale, i.e. last_magic < *magic

Short line wrap.

> +	 */
> +	if (magic && scmi_telemetry_tde_cache_stale(tde, *magic))
> +		return -EAGAIN;
> +
> +	sample->id = tde->de.info->id;
> +	sample->val = tde->last_val;
> +	sample->tstamp = tde->last_ts;
	sample = (struct scmi_telemetry_de_sample) {
		.id = tde.info->id,
		.val = tde->last_val,
		.tstamp = tde->last_ts,
	};

perhaps so it is clear we are setting it all (clearing the padding
seems fine anyway...

> +
> +	trace_scmi_tlm_collect(sample->tstamp, sample->id,
> +			       sample->val, "CACHED_DE_READ");
> +
> +	return 0;
>  }
>  
>  struct scmi_tlm_de_priv {
> @@ -2068,45 +2108,45 @@ scmi_telemetry_tde_allocate(struct telemetry_info *ti, u32 de_id,
>  
>  static inline int
>  scmi_telemetry_line_data_parse(struct telemetry_de *tde, struct shmti_snap *snap,
> -			       u64 *val, u64 *tstamp, struct payload *payld,
> -			       struct payload *bts_payld, u32 magic)
> +			       struct scmi_telemetry_de_sample *sample,

Similar to above. Maybe makes sense to push the existence of the structure earlier
in the patch set to avoid churn at this point.

> +			       struct payload *payld, struct payload *bts_payld,
> +			       u32 magic)
>  {
>  	u32 meta = PAYLD_META_RAW(payld);
>  
>  	lockdep_assert_held(&tde->mtx);
>  
>  	/* Clear returned values in case of errors */
> -	*val = 0;
> -
> -	if (tstamp) {
> -		*tstamp = 0;
> -		if (USE_BLK_TS_RAW(meta)) {
> -			/* Read out the actual BLK_TS */
> -			*tstamp = scmi_telemetry_blkts_read(magic, tde,
> -							    bts_payld, snap);
> -		} else if (LINE_TS_VALID_RAW(meta)) {
> -			/*
> -			 * Note that LINE_TS_VALID implies HAS_LINE_EXT and that
> -			 * the per DE line_ts_rate is advertised in the DE
> -			 * descriptor, BUT we check anyway against buggy FW.
> -			 */
> -			if (!HAS_LINE_EXT_RAW(meta)) {
> -				trace_scmi_tlm_access(tde->de.info->id,
> -						      "SHMTI_BAD_TS_LINE", 0, 0);
> -				return -EPROTO;
> -			}
> -
> -			*tstamp = LINE_TSTAMP_GET_RAW(&payld->tsl);
> +	sample->tstamp = 0;
> +	sample->val = 0;

If we are clearing for errors, then maybe do the whole thing?
Generally I'd hope we never use values in error paths and the
clearing is unnecessary but if doing it seems like doing it general
makes most sense.

> +
> +	if (USE_BLK_TS_RAW(meta)) {
> +		/* Read out the actual BLK_TS */
> +		sample->tstamp = scmi_telemetry_blkts_read(magic, tde,
> +							   bts_payld, snap);
> +	} else if (LINE_TS_VALID_RAW(meta)) {
> +		/*
> +		 * Note that LINE_TS_VALID implies HAS_LINE_EXT and that
> +		 * the per DE line_ts_rate is advertised in the DE
> +		 * descriptor, BUT we check anyway against buggy FW.
> +		 */
> +		if (!HAS_LINE_EXT_RAW(meta)) {
> +			trace_scmi_tlm_access(tde->de.info->id,
> +					      "SHMTI_BAD_TS_LINE", 0, 0);
> +			return -EPROTO;
>  		}
> +
> +		sample->tstamp = LINE_TSTAMP_GET_RAW(&payld->tsl);
>  	}
>  
>  	/* Data is always valid since we are NOT handling BLK TS lines here */
> -	*val = LINE_DATA_GET_RAW(&payld->l);
> +	sample->val = LINE_DATA_GET_RAW(&payld->l);
>  
> -	trace_scmi_tlm_collect(tstamp ? *tstamp : 0, tde->de.info->id,
> -			       *val, "SHMTI_DE_READ");
> +	/* Trace originally read tstamp */
> +	trace_scmi_tlm_collect(sample->tstamp, tde->de.info->id,
> +			       sample->val, "SHMTI_DE_READ");
>  
> -	scmi_telemetry_tde_cache_update(tde, *val, tstamp, &magic);
> +	scmi_telemetry_tde_cache_update(tde, sample, &magic);
>  
>  	return 0;
>  }

  
>

> +
> +static int scmi_telemetry_des_sample_get(const struct scmi_protocol_handle *ph,
> +					 int grp_id, unsigned int *num_samples,
> +					 struct scmi_telemetry_de_sample *samples)
> +{
> +	struct telemetry_info *ti = ph->get_priv(ph);
> +	struct scmi_msg_telemetry_config_set *msg;
> +	struct scmi_xfer *t;
> +	bool is_group;
> +	u32 control;
> +	int ret;

I guess it is prevented at a higher level, but perhaps some indication
of this one not needing to take the config_mtx and why would be helpful.
It's not locally obvious why other config messages can race with each
other to need the defense on those config_set calls but not this one.

> +
> +	if (!ti->info.enabled || !num_samples || !samples ||
> +	    !ti->info.single_read_support)
> +		return -EINVAL;
> +
> +	is_group = grp_id != SCMI_TLM_GRP_INVALID;
> +	if (is_group && grp_id >= ti->info.base.num_groups)
> +		return -EINVAL;
> +
> +	ret = ph->xops->xfer_get_init(ph, TELEMETRY_CONFIG_SET,
> +				      sizeof(*msg), 0, &t);
> +	if (ret)
> +		return ret;
> +
> +	msg = t->tx.buf;
> +	msg->group_identifier = cpu_to_le32(grp_id);
> +	control = TELEMETRY_ENABLE;
> +	control |= !is_group ? TELEMETRY_SET_SELECTOR_ALL :
> +		TELEMETRY_SET_SELECTOR_GROUP;
> +	control |= TELEMETRY_MODE_SINGLE;

Similar to earlier. I think this would be a lot more readable as appropriate
FIELD_PREP()

> +	msg->control = cpu_to_le32(control);
> +	msg->sampling_rate = 0;
> +
> +	ret = ph->xops->do_xfer_with_response(ph, t);
> +	/*
> +	 * PARTIAL_ERRORs are reported as -EBADMSG and related lines,
> +	 * marked as DATA_INVALID, will be properly skipped by
> +	 * scmi_telemetry_msg_payld_process.
> +	 */
> +	if (!ret || ret == -EBADMSG) {
> +		struct scmi_msg_resp_telemetry_reading_complete *r = t->rx.buf;
> +		u32 num_dwords;
> +
> +		if (ret == -EBADMSG)
> +			trace_scmi_tlm_access(0, "BAD_SAMPLE_PARTIAL_ERROR", 0, 0);
> +
> +		if (t->rx.len < sizeof(*r))
> +			return -EPROTO;
> +
> +		num_dwords = le32_to_cpu(r->num_dwords);
> +		/* Update cached DEs values from payload */
> +		if (num_dwords) {
> +			size_t payld_sz = 0, dwords_sz;
> +
> +			if (check_mul_overflow(num_dwords, sizeof(r->array[0]), &dwords_sz) ||
> +			    check_add_overflow(sizeof(*r), dwords_sz, &payld_sz) ||
> +			    payld_sz > t->rx.len) {
> +				dev_err(ph->dev,
> +					"Malformed Telemetry delayed response - num_dwords:%u  payld_sz:%zd  rx_len:%zd\n",
> +					num_dwords, payld_sz, t->rx.len);
> +				ret = -EPROTO;
> +				goto out;
> +			}
> +
> +			scmi_telemetry_msg_payld_process(ti, num_dwords, r->array,
> +							 payld_sz - sizeof(*r), false);
> +		}
> +
> +		/* Scan and update SMHTIs and FCs */
> +		scmi_telemetry_scan_update(ti);
> +
> +		/* Collect all last cached values, override any PARTIAL_ERROR */
> +		ret = scmi_telemetry_samples_collect(ti, grp_id, num_samples,
> +						     samples);
> +	}
> +
> +out:
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
;
>  
>  /**
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index ce35734d2249..7177f739f062 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -994,6 +994,13 @@ struct scmi_telemetry_info {
>  	enum scmi_telemetry_collection current_mode;
>  };
>  
> +struct scmi_telemetry_de_sample {
> +	unsigned int id;
> +	unsigned int pad;
> +	unsigned long long tstamp;
> +	unsigned long long val;

Why not the fixed size kernel types?

> +};