Re: [PATCH v6 00/17] Introduce SCMI transport based on VirtIO

[Peter Hilber <peter.hilber@opensynergy.com>]

On 19.07.21 13:36, Cristian Marussi wrote:
> On Thu, Jul 15, 2021 at 06:35:38PM +0200, Peter Hilber wrote:
> > On 12.07.21 16:18, Cristian Marussi wrote:
> > > Hi all,
> >
> > Hi Cristian,
> >
> > thanks for your update. Please find some additional comments in this reply
> > and the following.
> >
> > Best regards,
> >
> > Peter
>
> Hi Peter,
>
> thanks for the feedback.
>
> > > While reworking this series starting from the work done up to V3 by
> > > OpenSynergy, I am keeping the original autorship and list distribution
> > > unchanged.
> > >
> > > The main aim of this rework, as said, is to simplify where possible the
> > > SCMI VirtIO support added in V3 by adding at first some new general
> > > mechanisms in the SCMI Transport layer.
> > >
> > > Indeed, after some initial small fixes, patches 05/06/07/08 add such new
> > > additional mechanisms to the SCMI core to ease implementation of more
> > > complex transports like virtio, while also addressing a few general issues
> > > already potentially affecting existing transports.
> > >
> > > In terms of rework I dropped original V3 patches 05/06/07/08/12 as no more
> > > needed, and modified where needed the remaining original patches to take
> > > advantage of the above mentioned new SCMI transport features.
> > >
> > > DT bindings patch has been ported on top of freshly YAML converted arm,scmi
> > > bindings.
> > >
> > > Moreover, since V5 I dropped support for polling mode from the virtio-scmi
> > > transport, since it is an optional general mechanism provided by the core
> > > to allow transports lacking a completion IRQ to work and it seemed a
> > > needless addition/complication in the context of virtio transport.
> >
> > Just for correctness, in my understanding polling is not completely optional
> > ATM. Polling would be required by scmi_cpufreq_fast_switch(). But that
> > requirement might be irrelevant for now.
>
> Cpufreq core can use .fast_switch (scmi_cpufreq_fast_switch) op only if
> policy->fast_switch_enabled is true which in turn reported as true by
> the SCMI cpufreq driver iff SCMI FastChannels are supported by Perf
> implementation server side, but the SCMI Device VirtIO spec (5.17)
> explicitly does NOT support SCMI FastChannels as of now.
>
> Anyway, even though we should support in the future SCMI FastChannels on
> VirtIO SCMI transport, fastchannels are by defintion per-protocol/per-command/
> per-domain-id specific, based on sharedMem or MMIO, unidirectional and do not
> even allow for a response from the platform (SCMIV3.0 4.1.1 5.3) so polling
> won't be a thing anyway unless I'm missing something.
>
> BUT you made a good point in fact anyway, because the generic perf->freq_set/get
> API CAN be indeed invoked in polling mode, and, even though we do not use them
> in polling as of now (if not in the FastChannel scenario above) this could be a
> potential problem in general if when the underlying transport do not support poll
> the core just drop any poll_completion=true messages.
>
> So, while I still think it is not sensible to enable poll mode in SCMI Virtio,
> because would be a sort of faked polling and increases complexity, I'm now
> considering the fact that maybe the right behaviour of the SCMI core in such a
> scenario would be to warn the user as it does now AND then fallback to use
> non-polling, probably better if such a behavior is made condtional on some
> transport config desc flag that allow such fallback behavior.
>
> Any thought ?

Maybe the SCMI protocols should request "atomic" instead of "polling"? 
That semantics are the actual intent in my understanding. So the 
"Introduce atomic support for SCMI transports" patch series [1] could 
potentially address this?

Best regards,

Peter


[1] https://lkml.org/lkml/2021/7/12/3089