Re: [PATCH v15 00/35] Introduce SCMI Telemetry support

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

> The upcoming SCMI v4.0 specification [0] introduces a new SCMI protocol
> dedicated to System Telemetry.
> 

Hi Cristian,

So, the main issue here is pretty much summarized by:

 28 files changed, 12801 insertions(+), 99 deletions(-)
(obviously there are tests etc, but it's still > 10k lines)

There are relatively few reviewers who would even consider taking that on.
I foolishly thought it wouldn't be too bad (and I review a lot of code).
By about 3/4 of the way though, probably much earlier, I know the quality
of review went down considerably. Honestly it was probably a waste of
time continuing but I'm stubborn like that.

So, unless you have a path to bribing or sweet talking various folk to
deal with reviewing this with each having to set aside multiple days
to do it, likely repeating for a couple of versions yet, I think you've
got to figure out out how to make the first patch set more manageable.
Probably don't split it up as much as the mpam or CCA folk have had to,
but you have a lot of colleagues who are experiencing the same problem
at the moment. They may be able to advise.

So it would be nice if what was in that first patch set was 'functional'
but I don't think it needs to function on every systems or support more
than an absolutely minimal feature set.  Just fail to probe when you see
something that isn't supported by that minimal set if that means
the thing won't be functional.

Once that's in place, over a couple of additional patch sets you can
get to everything you have here.

So what if you choose one data path (shmti perhaps), no notifications,
events, maybe even simplify to one of the readback paths.
Maybe even drop event groups initially. 
Possibly even drop timestamps.  Definitely drop things like boot-on
support and debugfs (unless you are using that as a cheeky 'functional'
test ahead of the main userspace interface.

How much would that slim things down?  It doesn't even have to be
useful, just testable.  Realistically I think the hope of getting
enough review doubles if you can halve the size of what you are asking
people to look at.

Anyhow, gut feeling only but the code felt like it was in a much
better state than some of the versions I've looked at in the past,
so I think the game is now how to get it over the line!

Thanks,

Jonathan


> In a nutshell, the SCMI Telemetry protocol allows an agent to discover at
> runtime the set of Telemetry Data Events (DEs) available on a specific
> platform and provides the means to configure the set of DEs that a user is
> interested into, while reading them back using the collection method that
> is deeemed more suitable for the usecase at hand. (...amongst the various
> possible collection methods allowed by SCMI specification)
> 
> Without delving into the gory details of the whole SCMI Telemetry protocol
> let's just say that the SCMI platform/server firmware advertises a number
> of Telemetry Data Events, each one identified by a 32bit unique ID, and an
> SCMI agent/client, like Linux, can discover them and read back at will the
> associated data value in a number of ways.
> 
> Data collection is mainly intended to happen on demand via shared memory
> areas exposed by the platform firmware, discovered dynamically via SCMI
> Telemetry and accessed by Linux on-demand, but some DE can also be reported
> via SCMI Notifications asynchronous messages or via direct dedicated
> FastChannels (another kind of SCMI memory based access): all of this
> underlying mechanism is anyway hidden to the user since it is mediated by
> the kernel driver which will return the proper data value when queried.
> 
> Anyway, the set of well-known architected DE IDs defined by the spec is
> limited to a dozen IDs, which means that the vast majority of DE IDs are
> customizable per-platform: as a consequence, though, the same ID, say
> '0x1234', could represent completely different things on different systems.
> 
> Precise definitions and semantic of such custom Data Event IDs are out of
> the scope of the SCMI Telemetry specification and of this implementation:
> they are supposed to be provided using some kind of JSON-like description
> file that will have to be consumed by a userspace tool which would be
> finally in charge of making sense of the set of available DEs.
> 
> IOW, in turn, this means that even though the DEs enumerated via SCMI come
> with some sort of topological and qualitative description provided by the
> protocol (like unit of measurements, name, topology info etc), kernel-wise
> we CANNOT be completely sure of "what is what" without being fed-back some
> sort of information about the DEs by the afore mentioned userspace tool.
> 
> For these reasons, currently this series does NOT attempt to register any
> of these DEs with any of the usual in-kernel subsystems (like HWMON, IIO,
> PERF etc), simply because we cannot be sure which DE is suitable, or even
> desirable, for a given subsystem. This also means there are NO in-kernel
> users of these Telemetry data events as of now.
> 
> So, while we do not exclude, for the future, to feed/register some of the
> discovered DEs to/with some of the above mentioned Kernel subsystems, as
> of now we have ONLY modeled a custom userspace API to make SCMI Telemetry
> available to userspace tools.
> 
> With V5 we adopted a pure chardev/IOCTL ABI approach, refining the IOCTL
> based interface already present with the previous FS-based ABI.
> 
> INTERFACES
> 
> For each discovered SCMI Instance a character device named tlm_<N> is
> created under /dev/scmi/ subtree.
> 
> The IOCTL interface described at 'include/uapi/linux/scmi.h' is made
> available to enumerate and configure Telemetry resources: Telemetry data
> can be collected using a few different IOCTls, depending on the required
> granularity.
> 
> Alternatively it is possible to obtain a list of the file descriptors
> referencing directly the underlying SCMI Telemetry SHMTI memory areas and
> implement in user space an SCMI Telemetry parser accessing directly the
> SHMTI, while staying in compliance with the SCMI TDCF format.
> 
> NOTE THAT from v3 onwards the firmware interface level NOW supports ONLY
> the latest SCMI v4.0 specification [0].
> 
> Based on V7.4-rc2, tested on an emulated setup and AGI HW.
> 
> This series is available also at [1].
> 
> If you still reading...any feedback welcome :P
> 
> Thanks,
> Cristian