Re: [PATCH v15 17/35] firmware: arm_scmi: Add Telemetry configuration operations

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

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

> Add support for basic Telemetry configuration operations to selectively
> enable or disable DataEvents monitoring.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
Various comments inline.  Includes a suggestion that some
__iomem pointer specific overflow checkers might make for
neater code here and in earlier patches.

> +
> +struct scmi_msg_telemetry_config_set {
> +	__le32 group_identifier;
> +	__le32 control;
> +#define TELEMETRY_ENABLE		(BIT(0))
> +#define TELEMETRY_MODE			GENMASK(4, 1)
> +#define TELEMETRY_MODE_SET(x)		(FIELD_PREP(TELEMETRY_MODE, (x)))
> +#define	TLM_ONDEMAND			(0)
> +#define	TLM_NOTIFS			(1)
> +#define	TLM_SINGLE			(2)
> +#define TELEMETRY_MODE_ONDEMAND		TELEMETRY_MODE_SET(TLM_ONDEMAND)
> +#define TELEMETRY_MODE_NOTIFS		TELEMETRY_MODE_SET(TLM_NOTIFS)
> +#define TELEMETRY_MODE_SINGLE		TELEMETRY_MODE_SET(TLM_SINGLE)

I'd just put the FIELD_PREP inline. It is self explanatory on only a char or
two longer..

> +
> +#define TLM_ORPHANS			(0)
> +#define TLM_GROUP			(1)
> +#define TLM_ALL				(2)
> +#define TELEMETRY_SET_SELECTOR(x)	(FIELD_PREP(GENMASK(8, 5), (x)))
> +#define	TELEMETRY_SET_SELECTOR_ORPHANS	TELEMETRY_SET_SELECTOR(TLM_ORPHANS)
> +#define	TELEMETRY_SET_SELECTOR_GROUP	TELEMETRY_SET_SELECTOR(TLM_GROUP)
> +#define	TELEMETRY_SET_SELECTOR_ALL	TELEMETRY_SET_SELECTOR(TLM_ALL)
> +	__le32 sampling_rate;
> +};



> +
> +static struct payload __iomem *
> +scmi_telemetry_blkts_payld_locate(struct device *dev,
> +				  struct telemetry_shmti *shmti, u32 *bts_offs,
> +				  struct payload __iomem *payld, u32 de_id)
> +{
> +	struct payload __iomem *bts_payld = NULL;
> +
> +	if (!bts_offs)
> +		return IOMEM_ERR_PTR(-EINVAL);
> +
> +	if (*bts_offs &&
> +	    check_add_overflow((__force unsigned long)shmti->base, *bts_offs,
> +			       (__force unsigned long *)&bts_payld))

This code has quit a lot of checks on pointer overflow and many need
some ugly casts. Maybe we can have some local helpers for this?

bool check_add_overflow_iomemptr(void __iomem* start, size_t off, void __iomem **out)
{
	unsigned long _start, _out;
	bool overflow;

	_start = (__force unsigned long)start;
	overflow = check_add_overflow(_start, off, &_out);
	*out = (__force void __iomem *)_out;

	return overflow;
}

or something like that?

> +		return IOMEM_ERR_PTR(-EPROTO);
> +
> +	/* Check SHMTI overflow on returned offset */
> +	if (bts_payld) {
> +		if (scmi_telemetry_shmti_overflow(shmti, bts_payld)) {
> +			trace_scmi_tlm_access(de_id, "SHMTI_OVERFLOW", 0, 0);
> +			return IOMEM_ERR_PTR(-EINVAL);
> +		}
> +	} else {
> +		dev_warn_once(dev, "Received BLK_TS with zero offset.\n");
> +		trace_scmi_tlm_access(de_id, "BLKTS_WITH_ZERO_OFFSET", 0, 0);
> +
> +		/*
> +		 * Find the BLK_TS immediately preceding this DE payld by
> +		 * scanning.
> +		 */
> +		bts_payld = scmi_telemetry_nearest_line_by_type(shmti, payld,
> +								TDCF_BLK_TS_LINE);
> +		if (WARN_ON(!bts_payld))
> +			return IOMEM_ERR_PTR(-EPROTO);
> +
> +		/* Calculate offset of found blkts */
> +		*bts_offs = (void __iomem *)bts_payld - shmti->base;
> +	}
> +
> +	return bts_payld;
> +}

>
> +
> +static int scmi_telemetry_group_state_set(const struct scmi_protocol_handle *ph,
> +					  const bool *enable, const bool *tstamp,
> +					  struct scmi_telemetry_group *grp)
> +{
> +	struct scmi_msg_resp_telemetry_de_configure *resp;
> +	struct telemetry_info *ti = ph->get_priv(ph);
> +	struct scmi_msg_telemetry_de_configure *msg;
> +	struct scmi_xfer *t;
> +	int ret;
> +
> +	/* Stall concurrent configuration request */
> +	guard(mutex)(&ti->config_mtx);
> +
> +	if (!scmi_telemetry_remote_state_update_needed(ti, enable, &grp->enabled,
> +						       tstamp, &grp->tstamp_enabled,
> +						       true, grp))
> +		return 0;
> +
> +	ret = ph->xops->xfer_get_init(ph, TELEMETRY_DE_CONFIGURE,
> +				      sizeof(*msg), sizeof(*resp), &t);
> +	if (ret)
> +		return ret;
> +
> +	msg = t->tx.buf;
> +	scmi_telemetry_state_message_prepare(msg, grp->info->grp_id, true,
> +					     enable, &grp->enabled,
> +					     tstamp, &grp->tstamp_enabled);
> +	resp = t->rx.buf;
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {

So this is doing the out of line good path just to keep the ability to use
guard. I'd be tempted to not use guard() or use a wrapper function
just for that.  guard() magic is lovely but only until it makes us
write harder to read code.  The challenge here is we can't easily
do __free magic for the xfer.

> +		/*
> +		 * Update cached state on command success anyway, despite any
> +		 * possible later response validation issue: once the command
> +		 * has succedeed keep our state in sync with HW.
> +		 */
> +		scmi_telemetry_state_cache_update(ti, enable, &grp->enabled,
> +						  tstamp, &grp->tstamp_enabled,
> +						  grp, NULL);
> +		ret = scmi_telemetry_group_state_set_resp_process(ti, grp, resp);
> +		if (ret)
> +			dev_err(ph->dev,
> +				"Could NOT process GROUP %u state response - ret:%d\n",
> +				grp->info->grp_id, ret);
> +	}
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static int scmi_telemetry_state_get(const struct scmi_protocol_handle *ph,
> +				    const u32 *id, bool *enabled, bool *tstamp_enabled,
> +				    u32 *sid, u32 *offset, uuid_t  *uuid)

bonus space before *uuid.

> +{
> +	struct telemetry_info *ti = ph->get_priv(ph);
> +	struct scmi_telemetry_de *de;
> +	struct telemetry_de *tde;
> +
> +	if (!enabled || !tstamp_enabled)
> +		return -EINVAL;
> +
> +	ti->res_get(ti);
> +	if (!id) {
> +		guard(mutex)(&ti->des_enabled_mtx);
> +		/* Returning the all_des_* state */
> +		*enabled =
> +			(atomic_read(&ti->des_enabled[ENA_STATE]) == ti->info.base.num_des);
> +		*tstamp_enabled =
> +			(atomic_read(&ti->des_enabled[ENA_TSTAMP]) == ti->num_des_tstamp);

Ah.  These are reference counters. I had misunderstood that earlier.

I'd add some docs for this function though as isn't immediately obvious what
should be returned if these don't match.

> +
> +		return 0;
> +	}
> +
> +	de = xa_load(&ti->xa_des, *id);
> +	if (!de)
> +		return -ENODEV;
> +
> +	tde = to_tde(de);
> +	guard(mutex)(&tde->mtx);
> +
> +	*enabled = de->enabled;
> +	*tstamp_enabled = de->tstamp_enabled;
> +
> +	scmi_telemetry_tde_location_update(tde, sid, offset, uuid);
> +
> +	return 0;
> +}
> +
> +static int scmi_telemetry_state_set(const struct scmi_protocol_handle *ph,
> +				    bool is_group, u32 id, const bool *enable,
> +				    const bool *tstamp, u32 *sid, u32 *offset,
> +				    uuid_t *uuid)
> +{
> +	struct telemetry_info *ti = ph->get_priv(ph);
> +	struct scmi_telemetry_res_info *rinfo;
> +
> +	rinfo = ti->res_get(ti);
> +	if (!is_group) {
> +		struct scmi_telemetry_de *de;
> +
> +		de = xa_load(&ti->xa_des, id);
> +		if (!de)
> +			return -ENODEV;
> +
> +		/* Filter out unplausible timestamp requests */

Rather than error out if we get one? That sounds fragile.

> +		return scmi_telemetry_tde_state_set(ph, enable,
> +						    de->tstamp_support ? tstamp : NULL,
> +						    de, sid, offset, uuid);
> +	} else {
> +		struct scmi_telemetry_group *grp;
> +
> +		if (id >= rinfo->num_groups)
> +			return -EINVAL;
> +
> +		grp = &rinfo->grps[id];
> +		return scmi_telemetry_group_state_set(ph, enable, tstamp, grp);
> +	}
> +}
> +
> +static int
> +scmi_telemetry_collection_configure(const struct scmi_protocol_handle *ph,
> +				    unsigned int res_id, const bool *enable,
> +				    const unsigned int *update_interval_ms,
> +				    const enum scmi_telemetry_collection *mode)
> +{
> +	enum scmi_telemetry_collection *current_mode, next_mode;
> +	struct telemetry_info *ti = ph->get_priv(ph);
> +	struct scmi_msg_telemetry_config_set *msg;
> +	bool tlm_enable, *current_state, is_group;
> +	struct scmi_telemetry_res_info *rinfo;
> +	unsigned int *active_update_interval;
> +	struct scmi_xfer *t;
> +	__le32 interval;
> +	u32 control;
> +	int ret;
> +
> +	/* Stall concurrent configuration request */
> +	guard(mutex)(&ti->config_mtx);
> +
> +	if (mode && (*mode <= SCMI_TLM_SINGLE_READ) &&

This first line checks if mode is in range of allowed values. 
When would it not be?  Feels like a confusing thing to check here
as either of the next two conditions required it to be specific
values that meet that requirement. Was that meant to be an out
of range check?

> +	    ((*mode == SCMI_TLM_NOTIFICATION && !ti->info.continuous_update_support) ||
> +	     (*mode == SCMI_TLM_SINGLE_READ && !ti->info.single_read_support)))
> +		return -EINVAL;
> +
> +	is_group = res_id != SCMI_TLM_GRP_INVALID;
> +	if (is_group) {
> +		rinfo = ti->res_get(ti);
> +
> +		if (res_id >= rinfo->num_groups ||
> +		    !ti->info.per_group_config_support)
> +			return -EINVAL;
> +	}
> +
> +	if (!is_group) {

Maybe this makes sense in later patches, but for now, it looks like
you can just flip this condition and combine the else below iwth the
if (is_group) just above. That is...

	if (is_group) {
		rinfo = ti->res_get(ti);

		if (res_id >= rinfo->num_groups ||
		    !ti->info.per_group_config_support)
			return -EINVAL;

		active_update_interval = &rinfo->grps[res_id].active_update_interval;
		current_mode = &rinfo->grps[res_id].current_mode;
		current_state = &rinfo->grps[res_id].enabled;
	} else {
		active_update_interval = &ti->info.active_update_interval;
		current_mode = &ti->info.current_mode;
		current_state = &ti->info.enabled;
	}

> +		active_update_interval = &ti->info.active_update_interval;
> +		current_mode = &ti->info.current_mode;
> +		current_state = &ti->info.enabled;
> +	} else {
> +		active_update_interval =
> +			&rinfo->grps[res_id].active_update_interval;
> +		current_mode = &rinfo->grps[res_id].current_mode;
> +		current_state = &rinfo->grps[res_id].enabled;
> +	}
> +
> +	if (!enable && !update_interval_ms && (!mode || *mode == *current_mode))
> +		return 0;
> +
> +	ret = ph->xops->xfer_get_init(ph, TELEMETRY_CONFIG_SET,
> +				      sizeof(*msg), 0, &t);
> +	if (ret)
> +		return ret;
> +
> +	if (!update_interval_ms)

Flip logic then it's visually obvious that if you have it you
use the passed in active_interval_ms

> +		interval = cpu_to_le32(*active_update_interval);
> +	else
> +		interval = cpu_to_le32(*update_interval_ms);
> +
> +	tlm_enable = enable ? *enable : *current_state;
> +	next_mode = mode ? *mode : *current_mode;
> +
> +	msg = t->tx.buf;
> +	msg->group_identifier = cpu_to_le32(res_id);
> +	control = tlm_enable ? TELEMETRY_ENABLE : 0;
> +	control |= !is_group ? TELEMETRY_SET_SELECTOR_ALL : TELEMETRY_SET_SELECTOR_GROUP;
> +	control |= TELEMETRY_MODE_SET(next_mode);

	control = FIELD_PREP(TELEMETRY_ENABLE, tlm_emable ? 1 : 0) |
		  FIELD_PREP(TELEMETRY_SET_SELECTOR, TLM_SET_SELECTOR_ALL) |
		  FIELD_PREP(TELMETRY_MODE_SET, next_mode);

With adjusted defines is clearer to read than via the defines at the top of
file.  I'm never sure why people feel like wrappers around FIELD_PREP make
things more readable - I want to see that this is building a register value
and what goes in what named field.  The macros like TELEMETRY_SET_SELECTOR()
obscure that.

> +	msg->control = cpu_to_le32(control);
> +	msg->sampling_rate = interval;
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {
I'd do
	if (ret) {
		ph->xops->xfer_put(ph, t);
		return ret;
	}

Even though it duplicates that unwind, I think it is worth it
to avoid out of line good paths.  Or just don't use guard and use
a goto.



> +		*current_mode = next_mode;
> +		*current_state = tlm_enable;
> +		/*
> +		 * Enable streaming mode when notifications were explicitly
> +		 * requested as a global config, not on a group.
> +		 */
> +		if (!is_group)
> +			ti->streaming_mode = *current_mode == SCMI_TLM_NOTIFICATION;
> +		if (update_interval_ms)
> +			*active_update_interval = le32_to_cpu(interval);
> +	}
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}

> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 3973779c0aba..ce35734d2249 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -888,6 +888,12 @@ enum scmi_telemetry_compo_type {
>  #define SCMI_TLM_BUILD_UPDATE_INTERVAL(s, e)				    \
>  	(FIELD_PREP(GENMASK(20, 5), (s)) | FIELD_PREP(GENMASK(4, 0), (e)))
>  
> +enum scmi_telemetry_collection {
> +	SCMI_TLM_ONDEMAND,
> +	SCMI_TLM_NOTIFICATION,
> +	SCMI_TLM_SINGLE_READ,

Given these are called out a 0 1 2 in the spec I'd assign the
values to them just to make that association clear. 

> +};