| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aPdbkHixRsWjsZ3d@pluto>
Date: Tue, 21 Oct 2025 11:08:16 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: Cristian Marussi <cristian.marussi@....com>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
arm-scmi@...r.kernel.org, sudeep.holla@....com,
james.quinlan@...adcom.com, f.fainelli@...il.com,
vincent.guittot@...aro.org, etienne.carriere@...com,
peng.fan@....nxp.com, michal.simek@....com, quic_sibis@...cinc.com,
dan.carpenter@...aro.org, d-gole@...com, souvik.chakravarty@....com
Subject: 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 <cristian.marussi@....com> wrote:
>
> > 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@....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
>
>
> > 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.
> > + */
>
> > +
> > +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... :<
>
> > +
> > +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...
> > +};
> > +
> > +/* 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..
>
> > +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.
>
> > + if (DATA_INVALID(payld))
> > + return used_qwords;
>
> > +
> > +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...
>
> > + void *eplg = SHMTI_EPLG(shmti);
>
>
> > +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.
>
> > + 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;
> > + }
> > +}
>
>
> > 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
>
> ...
>
> > +#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...
>
> > +#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
>
> > + __signed_exp; \
> > + })
> > +
> > +#define SCMI_TLM_BUILD_UPDATE_INTERVAL(s, e) \
> > + (FIELD_PREP(GENMASK(20, 5), (s)) | FIELD_PREP(GENMASK(4, 0), (e)))
>
> > +
> > +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
Powered by blists - more mailing lists