Re: [PATCH v12 06/25] firmware: arm_scmi: Add basic Telemetry support

[Cristian Marussi <cristian.marussi@arm.com>]

On Tue, Sep 22, 2026 at 04:42:59PM +0200, David Hildenbrand (Arm) wrote:
> On 9/20/26 11:19, Cristian Marussi wrote:
> > Add SCMIv4.0 Telemetry basic support to enable initialization and resources
> > enumeration: add all the telemetry messages definitions and parsing logic
> > but only a few simple state gathering protocol operations.
> 
> As someone unfamiliar with the spec, it would be nice to summarize which parts
> of the spec this patch implements, so it's easier to cross-reference.
> 
> Same for the other patches. :)

Sure...I will add more descriptive comments.

> 
> It's a lot to review. So having a better description to guide the reviewer
> through the patch would really help. I assume we could split this up further, like
> 
> a) Add basic definition (mechanical from the spec)
> 
> b) Add telemetry stub and hook it up
> 
> c) query attributes
> 
> d) intialize X
> 
> ...
> 
> Will result in quite some patches .... so I won't suggest that just yet. Maybe
> people familiar with the spec have it easier revieweing this.
> 

I'll see if I can split this basic support patch a bit further...

> 
> > +
> > +enum scmi_telemetry_protocol_cmd {
> > +	TELEMETRY_LIST_SHMTI = 0x3,
> > +	TELEMETRY_DE_DESCRIPTION = 0x4,
> > +	TELEMETRY_LIST_UPDATE_INTERVALS = 0x5,
> > +	TELEMETRY_DE_CONFIGURE = 0x6,
> > +	TELEMETRY_DE_ENABLED_LIST = 0x7,
> > +	TELEMETRY_CONFIG_SET = 0x8,
> > +	TELEMETRY_READING_COMPLETE = TELEMETRY_CONFIG_SET,
> 
> TELEMETRY_READING_COMPLETE is not really a command but listed as a "delated
> response". Should this be something separate (and not mangled into protocol_cmd
> ?). Or is "_cmd" not the right term for this collection?
> 
> I have no SCMI experience, so it might be a rather supid queestion.
> 

So in SCMI there are 2 types of commands synchronous and asynchronous:
support for the latter ones is optional AND it is command specific: i.e.
ONLY some commands has an alternative async flavour.

In a nutshell a sync command transaction is made of 2 messages cmd/reply
across the same Agent_2_Platform bidirectional channel...
Instead if a command can be particularly 'heavy/slow' platform side to
satisfy, usually async support is present in the spec: in such case you
have a 3 message transaction:

1. A2P: cmd/reply ... where the reply ONLY carries the status OK

2. P2A: delayed_response ... a later asyncronous further reply that
carries the real content (e.g. a sensor reading that needed a lot of
time for the sensor itself to settle..)

The end result is that the A2P sync channel is NOT kept busy waiting for
the delayed reply...

Having said that, in the spec, while the sync commands are defined
across protocols with a monotonically increasing MSG_ID starting from 0
(with 0,1,2 IDs commonly defined across all protocols), the DELAYED_RESPONSE
are defined to have the same MSG_ID as the sync command for which they
constitues the async part ...

That is the reason we define them in such a way across all of the
existing 10 standard protocols...

> [...]
> 
> > +struct scmi_de_desc {
> > +	__le32 id;
> 
> I'll make a couple of generic comments, that should apply to most definitions in
> here.
> 
> In the spec some of these things are prefixed by "de" e.g., "de_id".
> 
> Was this deliberate? Having the spec match the implementation allows for easier
> grepping of stuff.
> 
> > +	__le32 grp_id;
> 
> Spec calls this "group_id"
> 
> > +	__le32 data_sz;
> 
> Spec calls this "de_data_size" etc.
> 
> If there is good reason to use slightly different names, best to spell that out
> in the patch description.

In general of course I agree with this and try to keep the naming
exactly the same of the spec: in this case there are 2 reasons for the
deviation:

1. EVOLUTION - spec has evolved a lot and names have changed slightly between
   spec revisions AND I have NOT kept up with that...MY BAD...I will fix these
   occurencies

2. READABILITY - some naming is, IMHO, a bit silly/ill chosen in the
   spec because as an example if the implementation follows the spec
   naming pedantically, you would end up dealing with stuff like:

	de->de_id
	de->de_data_size

...so I killed some of this prefixes where from the context in the code and in
the spec SHOULD BE apparent what they represent...

Regarding some other cases like "grp_id" instead of "group_id"...I have NO excuses :D
... just me trying to keep statements short :P

> 
> > +	__le32 attr_1;
> 
> E.g., grepping the spec for "attr_1" I get no hits. So I have to remember that
> the spec might call this "de_attributes_1"

Here is a mix of OLD spec, ill shortening of mine and silly naming I
would probably move this to:

	de->attributes_1
> 
> > +#define	IS_NAME_SUPPORTED(d)	(le32_get_bits((d)->attr_1, BIT(31)))
> 
> DE_ATTRIBUTES_1_NAME_SPECIFIED would be clearer. in general, spelling out to
> which field a define belongs *might* make it harder to get stuff wrong.

Ok...I was trying to avoid cumbersome NAMING while conveying the meaning
BUT I can certainly move to a more pedantic spec-compliant naming...

> 
> (spec calls it "Named specified" which sounds like a bug)

I suppose it is to meant that the optional DE name exist, while avoiding
to use a nomenclature that is too similar to Names and Extended Names as
they are used by all other protocols :D

> 
> Same for the other definitions. Again, maybe diverging from the spec is fine.
> 
> Personally, I would try to stay as close as possible to the naming in the spec.
> 

Yes that would be my aim ...I will fix the needless shortening and align
the name to the last releases spec fiels while still deviating from the
spec ease readability by avoding stuff like de->de_id

> [...]
> > +
> > +static void scmi_telemetry_free_tde_put(struct telemetry_info *ti,
> > +					struct telemetry_de *tde)
> > +{
> > +	struct scmi_telemetry_de_info *info;
> > +
> > +	guard(mutex)(&ti->free_mtx);
> > +	/* Save clear and restore */
> > +	info = READ_ONCE(tde->de.info);
> 
> Where is the matching WRITE_ONCE? IOW, who is expected to modify this concurrently?

Mmmm....right..I'll check...this was added to clear freed strcuture
while preserving some fields ...

> 
> > +	memset(info, 0, sizeof(*info));
> > +	memset(tde, 0, offsetof(struct telemetry_de, mtx));
> > +	tde->de.info = info;
> > +	list_add_tail(&tde->item, &ti->free_des);
> > +}
> > +
> 
> [...]
> > +
> > +static int scmi_telemetry_protocol_init(const struct scmi_protocol_handle *ph)
> > +{
> > +	struct device *dev = ph->dev;
> > +	struct telemetry_info *ti;
> > +	int ret;
> > +
> > +	dev_dbg(dev, "Telemetry Version %d.%d\n",
> > +		PROTOCOL_REV_MAJOR(ph->version), PROTOCOL_REV_MINOR(ph->version));
> > +
> > +	ti = devm_kzalloc(dev, sizeof(*ti), GFP_KERNEL);
> > +	if (!ti)
> > +		return -ENOMEM;
> > +
> > +	ti->ph = ph;
> > +
> > +	ret = scmi_telemetry_protocol_attributes_get(ti);
> > +	if (ret) {
> > +		dev_err(dev, FW_BUG "Cannot retrieve protocol attributes. Abort.\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = scmi_telemetry_instance_init(ti);
> > +	if (ret) {
> > +		dev_err(dev, "Cannot initialize instance. Abort.\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = scmi_telemetry_enumerate_common_intervals(ti);
> > +	if (ret) {
> > +		dev_err(dev, FW_BUG "Cannot enumerate update intervals. Abort.\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = scmi_telemetry_enumerate_shmti(ti);
> > +	if (ret) {
> > +		dev_err(dev, FW_BUG "Cannot enumerate SHMTIs. Abort.\n");
> > +		return ret;
> > +	}
> > +
> > +	ti->info.base.version = ph->version;
> 
> I'm surprised that there is nothing to cleanup on the error paths, but at least
> the devm_ stuff is being taken care of I guess.
> 

This protocol_init function runs within an opened devm_group so that on
failure or when protocol is deinitialized by an unbound driver all is
cleaned up by the core SCMI stack through the devres group...a mechanism that
raised controversy already in the past :D but when all the core SCMI stack
and this mechanism was written there were NO cleanup.h magic available to
ease the development of SCMI protocols....

Anyway this initialization is triggered when one of the SCMI driver
using this protocol is bound and probes on its own...so the devres
machinery is anyway attached, albeit indirectly, to a probe path.

Having said that, in Telemetry I do use cleanup.h during intermediate
allocation and, once the allocation are finalized (no_free_ptr) I attach
a cleanup routine to the devres hooks...(routine which I now needs some
refinement on some err path...) that is the reason of the apparent lack
of freeing path

Thanks,
Cristian