Re: [PATCH v5 00/23] Introduce SCMI Telemetry support

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

Hi,

thanks for looking into this again, Subrahmanya,

Let me add some additional points to Cristian's reply.

> 
>>
>>  I agree that asking this series to grow a generic telemetry subsystem now is
>>  too much. With only SCMI in-tree, it would be easy to create a "generic"
>>  interface that is really just SCMI with different names. I also agree that raw
>>  SHMTI is not just a debug path if the expected users are high-rate tooling.
>>
>>  So let me narrow the ask.
>>
>>  I am not asking for `drivers/telemetry/` in v6. I am asking that the SCMI
>>  chardev/UAPI not be tied too closely to the current SCMI core storage layout.
>>  In particular, I think v6 should try to:
>>
>>    - copy descriptor/config/sample data through fixed and extensible UAPI
>>      structs, not structs mirroring SCMI internal objects;
> 
> If you mean by this adding a bit more of pad/reserved space scattered
> here and there, it is not a problem; also, one thing that is still missing
> and planned to be added is some ABI Versioning and related IOCTL,
> should not this be enough to address future possible UAPI expansions ?
> 
> Regarding avoiding mapping internal SCMI object in the UAPI, I think I
> am already NOT blindly sharing internal structures but instead I copy
> explicitly internal representations into the UAPI fields....I think the
> only exception to this is the sample (scmi_tlm_de_sample) that I take
> care to keep aligned with the internals to avoid the overhead of the
> double copy...but it seems one of the few pretty much general concept
> that I felt confident to abstract no ?
> 
> ...but of course this UAPI abstraction into which I copy the results
> is mapped into SCMI inspired concepts/abstractions...for the reasons
> we said....

Getting the UAPI right and future-proof is certainly the most critical thing.
Adding some more padding to keep it extensible is certainly doable.

I remember plans for having an ABI versioning though, would that still be
required (or could it even be deferred) if we add some more padding to the structs?

Or would ABI versioning just avoid that for now?

> 
>>    - treat DE IDs, group IDs and SHMTI IDs as protocol identifiers, not as
>>      implicit indexes into kernel arrays;
> 
> Not sure what you mean here... a shim layer used to acces info data
> indirectly instead of the SCMI data structures ?
> 
> or some sort of layer/abstraction to be able to remap references
> internally ? again it is not easy to imagine an abstraction without any
> other example than SCMI...or you mean naming issue in the UAPI ?

I'm also having a hard time understanding this.

Subrahmanya, can you share an example of before vs. after to clarify? Thanks

> 
>>    - keep the TDCF/SHMTI parsing and SCMI command handling behind small
>>      internal ops used by the chardev side;
> 
> well..if you refer to the telemetry_ops, this is an SCMI driver so it
> has the structure of an SCMI driver, the Telemetry Operations exposed by
> the core SCMI stack are the internal ops themselves that you ask for in
> my opinion...I mean this is pretty much internal stuff that has nothing
> to do with the UAPI itself and I imagine that when some future protocol
> of yours, like the XXMI you mentioned, will appear you will write a new
> driver on your own subsystem that will implement the UAPI itself for
> your protocols (if you want to share that)...any possible future internal
> reworking due to possible common abstractions on our side can be carried
> out in the future...

Any internal refactorings should be had once required, not prematurely.

At this point it's not even clear when/how/if any internal user would want to
reuse some pieces.


> 
>>    - document the config model explicitly: per SCMI instance, last writer wins,
>>      generation changes tell userspace that state changed;
>>    - document the mmap contract: length, offsets, cache attributes, lifetime,
>>      and reset/reconfiguration behaviour.
> 
> Completely agreed on this...Documentation is still lacking, including
> extensive examples...I should have marked docs as RFC too...

+1

-- 
Cheers,

David