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

From: Peter Hilber
Date: Thu Jul 22 2021 - 04:30:36 EST

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,


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

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

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,