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

From: Peter Hilber <peter.hilber@opensynergy.com>
To: Cristian Marussi <cristian.marussi@arm.com>
Cc: linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	virtualization@lists.linux-foundation.org,
	virtio-dev@lists.oasis-open.org, sudeep.holla@arm.com,
	james.quinlan@broadcom.com, Jonathan.Cameron@Huawei.com,
	f.fainelli@gmail.com, etienne.carriere@linaro.org,
	vincent.guittot@linaro.org, souvik.chakravarty@arm.com,
	igor.skalkin@opensynergy.com, alex.bennee@linaro.org,
	jean-philippe@linaro.org, mikhail.golubev@opensynergy.com,
	anton.yakovlev@opensynergy.com, Vasyl.Vavrychuk@opensynergy.com,
	Andriy.Tryshnivskyy@opensynergy.com
Subject: Re: [PATCH v6 00/17] Introduce SCMI transport based on VirtIO
Date: Thu, 22 Jul 2021 10:30:27 +0200	[thread overview]
Message-ID: <a2b796ef-3ce4-65a7-c9e6-4d9a97738c10@opensynergy.com> (raw)
In-Reply-To: <20210719113657.GI49078@e120937-lin>

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