Re: [PATCH v6 14/16] firmware: arm_scmi: Add atomic mode support to virtio transport

[Cristian Marussi]

On Mon, Nov 22, 2021 at 11:06:38PM +0000, Cristian Marussi wrote:
> Add support for .mark_txdone and .poll_done transport operations to SCMI
> VirtIO transport as pre-requisites to enable atomic operations.
> 
> Add a Kernel configuration option to enable SCMI VirtIO transport polling
> and atomic mode for selected SCMI transactions while leaving it default
> disabled.
> 
> Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
> Cc: Igor Skalkin <igor.skalkin@xxxxxxxxxxxxxxx>
> Cc: Peter Hilber <peter.hilber@xxxxxxxxxxxxxxx>
> Cc: virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>


Hi,

> ---
>  drivers/firmware/arm_scmi/Kconfig  |  15 ++
>  drivers/firmware/arm_scmi/virtio.c | 212 +++++++++++++++++++++++++++--
>  2 files changed, 214 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index d429326433d1..7794bd41eaa0 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -118,6 +118,21 @@ config ARM_SCMI_TRANSPORT_VIRTIO_VERSION1_COMPLIANCE
>  	  the ones implemented by kvmtool) and let the core Kernel VirtIO layer
>  	  take care of the needed conversions, say N.
>  
> +config ARM_SCMI_TRANSPORT_VIRTIO_ATOMIC_ENABLE
> +	bool "Enable atomic mode for SCMI VirtIO transport"
> +	depends on ARM_SCMI_TRANSPORT_VIRTIO
> +	help
> +	  Enable support of atomic operation for SCMI VirtIO based transport.
> +
> +	  If you want the SCMI VirtIO based transport to operate in atomic
> +	  mode, avoiding any kind of sleeping behaviour for selected
> +	  transactions on the TX path, answer Y.
> +
> +	  Enabling atomic mode operations allows any SCMI driver using this
> +	  transport to optionally ask for atomic SCMI transactions and operate
> +	  in atomic context too, at the price of using a number of busy-waiting
> +	  primitives all over instead. If unsure say N.
> +
>  endif #ARM_SCMI_PROTOCOL
>  

[snip]

>  static void scmi_vio_complete_cb(struct virtqueue *vqueue)
> @@ -138,12 +157,29 @@ static void scmi_vio_complete_cb(struct virtqueue *vqueue)
>  			goto unlock_ready_out;
>  		}
>  
> +#ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO_ATOMIC_ENABLE
> +		/* At first loop on pending_cmd_list of buffered msg if any */
> +		if (!vioch->is_rx) {
> +			struct scmi_vio_msg *tmp;
> +
> +			list_for_each_entry_safe(msg, tmp,
> +						 &vioch->pending_cmds_list,
> +						 list) {
> +				scmi_rx_callback(vioch->cinfo,
> +						 msg_read_header(msg->input),
> +						 msg);
> +				scmi_vio_feed_vq_tx(vioch, msg);
> +			}
> +		}
> +#endif
> +

doing further testing I spotted a trivial bug on the limit condition
where polling and non-polling requests are interleaved on the cmd
vqueue. (hard to reproduce ... but it's there and it's broken)

This one-liner fixes the issue with the above snippet:

diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c
index 3101024751b0..cf247093ef48 100644
--- a/drivers/firmware/arm_scmi/virtio.c
+++ b/drivers/firmware/arm_scmi/virtio.c
@@ -168,6 +168,7 @@ static void scmi_vio_complete_cb(struct virtqueue *vqueue)
                                scmi_rx_callback(vioch->cinfo,
                                                 msg_read_header(msg->input),
                                                 msg);
+                               list_del(&msg->list);
                                scmi_vio_feed_vq_tx(vioch, msg);
                        }
                }

It seems there will be a V7 ...

Thanks,
Cristian