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

["David Hildenbrand (Arm)" <david@kernel.org>]

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. :)

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.


> +
> +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.

[...]

> +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.

> +	__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"

> +#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.

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

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.

[...]
> +
> +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?

> +	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.


It's a lot of code, so I only managed to skim briefly over most of it.

-- 
Cheers,

David