Re: [PATCH 05/10] firmware: arm_scmi: Add Telemetry protocol support

On Fri, Oct 17, 2025 at 04:37:25PM +0100, Jonathan Cameron wrote:

On Thu, 25 Sep 2025 21:35:49 +0100
Cristian Marussi [off-list ref] wrote:

quoted

Add basic support for SCMI V4.0-alpha_0 Telemetry protocol including SHMTI,
FastChannels, Notifications and Single Sample Reads collection methods.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

Hi,

Hi thanks for having a look

This is very much in the superficial drive by category as reviews
go.  A few things noted but I've not looked at the code in enough
detail.

...this is still very early days as a series since I moved across a few
different implementations in the previous RFCs, so as noted in the
cover-letter there are in general lots of open-issues...

...BUT I am sure there will be more after this review :P

Thanks for the feedback in the meantime.

Jonathan

quoted

diff --git a/drivers/firmware/arm_scmi/telemetry.c b/drivers/firmware/arm_scmi/telemetry.c
new file mode 100644
index 000000000000..f03000c173c2
--- /dev/null
+++ b/drivers/firmware/arm_scmi/telemetry.c

@@ -0,0 +1,2117 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Telemetry Protocol
+ *
+ * Copyright (C) 2025 ARM Ltd.
+ *

My favorite trivial comment applies.  What does this blank line add
to readability? I'd drop it.

... I would say..at the moment felt right :P ... but there is no need
and I wil drop it.

quoted

+ */

quoted

+
+struct scmi_de_desc {
+	__le32 id;
+	__le32 grp_id;
+	__le32 data_sz;
+	__le32 attr_1;
+#define	IS_NAME_SUPPORTED(d)	((d)->attr_1 & BIT(31))
+#define	IS_FC_SUPPORTED(d)	((d)->attr_1 & BIT(30))
+#define	GET_DE_TYPE(d)		(le32_get_bits((d)->attr_1, GENMASK(29, 22)))
+#define	IS_PERSISTENT(d)	((d)->attr_1 & BIT(21))
+#define GET_DE_UNIT_EXP(d)						\
+	({								\
+		int __signed_exp =					\
+			le32_get_bits((d)->attr_1, GENMASK(20, 13));	\
+									\
+		if (__signed_exp & BIT(7))				\
+			__signed_exp |= GENMASK(31, 8);			\
+		__signed_exp;						\
+	})
+#define	GET_DE_UNIT(d)		(le32_get_bits((d)->attr_1, GENMASK(12, 5)))
+
+#define GET_DE_TSTAMP_EXP(d)						\
+	({								\
+		int __signed_exp =					\
+			FIELD_GET(GENMASK(4, 1), (d)->attr_1);		\
+									\
+		if (__signed_exp & BIT(3))				\
+			__signed_exp |= GENMASK(31, 4);			\
+		__signed_exp;						\

See below for sign_extend32() using code to replace these.

Sadly enough, in the past I am sure I have also searched for something similar
and did not find it despite being merged since 2010 apparently... :<

quoted

+
+struct scmi_msg_resp_telemetry_reading_complete {
+	__le32 num_dwords;
+	__le32 dwords[];

__counted_by(num_word);

Mmmm, this is really used to cast to a variable sized received message
payload..is the __counted_by gonna work as intended in this case ?
..because this means a bad sizing could be triggered by a bad FW...

(I suppose I will find my answer looking better at __counted_by inner workings...)

I will check anyway the processing of this var size message...
 

quoted

+};
+
+/* TDCF */
+
+#define TO_CPU_64(h, l)	(((u64)le32_to_cpu((h)) << 32) | le32_to_cpu((l)))

Some of this stuff sounds very generic and isn't at all.

Personally I think I'd just drop this one as it may be better to see
the implementation wherever it is used.

Ok ...it was to avoid a bit of duplication in some macros down below
and made the intent more clear from the name..

quoted

+static int scmi_telemetry_tdcf_line_parse(struct telemetry_info *ti,
+					  struct payload __iomem *payld,
+					  struct telemetry_shmti *shmti,
+					  bool update)
+{
+	int used_qwords;
+
+	used_qwords = (USE_LINE_TS(payld) && TS_VALID(payld)) ?
+		QWORDS_TS_LINE_DATA_PAYLD : QWORDS_LINE_DATA_PAYLD;
+
+	/*Invalid lines are not an error, could simply be disabled DEs */

Check for inconsistent comment syntax etc.

Ok.

quoted

+	if (DATA_INVALID(payld))
+		return used_qwords;

quoted

+
+static int scmi_telemetry_shmti_scan(struct telemetry_info *ti,
+				     unsigned int shmti_id, u64 ts,
+				     bool update)
+{
+	struct telemetry_shmti *shmti = &ti->shmti[shmti_id];
+	struct tdcf __iomem *tdcf = shmti->base;
+	int retries = SCMI_TLM_TDCF_MAX_RETRIES;
+	u64 startm = 0, endm = 0xffffffffffffffff;

No one likes counting fs.  Use a GENMASK probably.

Yes definitely better, even though is not really a mask but just a fixed
invalid value to use at start...

quoted

+	void *eplg = SHMTI_EPLG(shmti);

quoted

+static void
+scmi_telemetry_msg_payld_process(struct telemetry_info *ti,
+				 unsigned int num_dwords, unsigned int *dwords,

I'd kind of expect something called dwords to have a fixed size. u32, u64 or
whatever.

Yes I agree.

quoted

+				 ktime_t timestamp)
+{
+	u32 next = 0;
+
+	while (next < num_dwords) {
+		struct payload *payld = (struct payload *)&dwords[next];
+		struct scmi_telemetry_de *de;
+		struct telemetry_de *tde;
+		u32 de_id;
+
+		next += USE_LINE_TS(payld) ?
+			TS_LINE_DATA_PAYLD_WORDS : LINE_DATA_PAYLD_WORDS;
+
+		if (DATA_INVALID(payld)) {
+			dev_err(ti->dev, "MSG - Received INVALID DATA line\n");
+			continue;
+		}
+
+		de_id = le32_to_cpu(payld->id);
+		de = xa_load(&ti->xa_des, de_id);
+		if (!de || !de->enabled) {
+			dev_err(ti->dev,
+				"MSG - Received INVALID DE - ID:%u  enabled:%d\n",
+				de_id, de ? (de->enabled ? 'Y' : 'N') : 'X');
+			continue;
+		}
+
+		tde = to_tde(de);
+		guard(mutex)(&tde->mtx);
+		tde->cached = true;
+		tde->last_val = LINE_DATA_GET(&payld->tsl);
+		//TODO BLK_TS in notification payloads
+		if (USE_LINE_TS(payld) && TS_VALID(payld))
+			tde->last_ts = LINE_TSTAMP_GET(&payld->tsl);
+		else
+			tde->last_ts = 0;
+	}
+}

quoted

diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 59527193d6dd..6c6db95d0089 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h

...

quoted

+#define	SCMI_TLM_GET_UPDATE_INTERVAL_SECS(x)				\
+	(le32_get_bits((x), GENMASK(20, 5)))

Why is this one little endian specific and the next just uses assumption of
CPU Endian?

Mmmm is not becasue the next one extract a 5 bit value that being < 8bit
has no endianity issue ?

....haviong said that...I think this version has a lot of endian issue to be
fixed as pointed out also by the kernel bots...

quoted

+#define SCMI_TLM_GET_UPDATE_INTERVAL_EXP(x)				\
+	({								\
+		int __signed_exp = FIELD_GET(GENMASK(4, 0), (x));	\
+									\
+		if (__signed_exp & BIT(4))				\
+			__signed_exp |= GENMASK(31, 5);			\

sign_extend32() from bitops.h should work here and is much more self explanatory.
That would then make this something like

#define SCMI_TLM_GET_UPDATE_INTERVAL_EXP(x) \
 	sign_extend32(x, 4);
or you can mask it first if you like but I don't think it makes any difference
in practice.

Yes using well known helpers is much better...I will rework and
test...just in case :D

quoted

+		__signed_exp;						\
+	})
+
+#define SCMI_TLM_BUILD_UPDATE_INTERVAL(s, e)				    \
+	(FIELD_PREP(GENMASK(20, 5), (s)) | FIELD_PREP(GENMASK(4, 0), (e)))

quoted

+
+struct scmi_telemetry_update_report {
+	ktime_t		timestamp;
+	unsigned int	agent_id;
+	int		status;
+	unsigned int	num_dwords;
+	unsigned int	dwords[];

More places where __counted_by is appropriate. I'll not comment on any others and
just assume you'll add them wherever appropriate.

Sure, but, as said above, I will reason a bit on the places where the struct is used
to cast a received FW payload (which is NOT the case in this latter example...)

Thanks,
Crisian