Re: [PATCH v7 12/16] firmware: arm_scmi: Add atomic mode support to smc transport

[Sudeep Holla]

On Mon, Nov 29, 2021 at 07:11:52PM +0000, Cristian Marussi wrote:
> Add a Kernel configuration option to enable SCMI SMC transport atomic
> mode operation for selected SCMI transactions and leave it as default
> disabled.
> 
> Substitute mutex usages with busy-waiting and declare smc transport as
> .atomic_enabled if such Kernel configuration option is enabled.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> ---
> v5 --> v6
> - remove usage of atomic_capable
> - removed needless union
> - reviewed Kconfig help
> v4 --> v5
> - removed RFC tag
> - add CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE option
> - add .atomic_enable support
> - make atomic_capable dependent on
>   CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE
> - make also usage of mutexes vs busy-waiting dependent on
>   CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE
> ---
>  drivers/firmware/arm_scmi/Kconfig | 14 +++++++
>  drivers/firmware/arm_scmi/smc.c   | 66 ++++++++++++++++++++++++++++---
>  2 files changed, 74 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index 638ecec89ff1..d429326433d1 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -78,6 +78,20 @@ config ARM_SCMI_TRANSPORT_SMC
>  	  If you want the ARM SCMI PROTOCOL stack to include support for a
>  	  transport based on SMC, answer Y.
>  
> +config ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE
> +	bool "Enable atomic mode support for SCMI SMC transport"
> +	depends on ARM_SCMI_TRANSPORT_SMC
> +	help
> +	  Enable support of atomic operation for SCMI SMC based transport.
> +
> +	  If you want the SCMI SMC 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.
> +
>  config ARM_SCMI_TRANSPORT_VIRTIO
>  	bool "SCMI transport based on VirtIO"
>  	depends on VIRTIO=y || VIRTIO=ARM_SCMI_PROTOCOL
> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> index b2f31d3feb10..0fc49cb49185 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/arm-smccc.h>
> +#include <linux/atomic.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
> @@ -14,6 +15,9 @@
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> +#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE
> +#include <linux/processor.h>
> +#endif
>  #include <linux/slab.h>
>  
>  #include "common.h"
> @@ -23,14 +27,23 @@
>   *
>   * @cinfo: SCMI channel info
>   * @shmem: Transmit/Receive shared memory area
> - * @shmem_lock: Lock to protect access to Tx/Rx shared memory area
> + * @shmem_lock: Lock to protect access to Tx/Rx shared memory area.
> + *		Used when NOT operating in atomic mode.
> + * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
> + *	      Used when operating in atomic mode.
>   * @func_id: smc/hvc call function id
>   */
>  
>  struct scmi_smc {
>  	struct scmi_chan_info *cinfo;
>  	struct scmi_shared_mem __iomem *shmem;
> +#ifndef CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE
> +	/* Protect access to shmem area */
>  	struct mutex shmem_lock;

Ditto here, do we really need to do this saving ? I would wait until someone
really complains about space. It unnecessarily makes it hard to read.

> +#else
> +#define INFLIGHT_NONE	MSG_TOKEN_MAX
> +	atomic_t inflight;
> +#endif
>  	u32 func_id;
>  };
>  
> @@ -54,6 +67,46 @@ static bool smc_chan_available(struct device *dev, int idx)
>  	return true;
>  }
>  
> +static inline void smc_channel_lock_init(struct scmi_smc *scmi_info)
> +{
> +#ifndef CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE
> +	mutex_init(&scmi_info->shmem_lock);
> +#else
> +	atomic_set(&scmi_info->inflight, INFLIGHT_NONE);

You can do both if you remove conditional definition of struct.

> +#endif
> +}
> +
> +#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE
> +static bool smc_xfer_inflight(struct scmi_xfer *xfer, atomic_t *inflight)
> +{
> +	int ret;
> +
> +	ret = atomic_cmpxchg(inflight, INFLIGHT_NONE, xfer->hdr.seq);
> +
> +	return ret == INFLIGHT_NONE;
> +}
> +#endif
> +
> +static inline void
> +smc_channel_lock_acquire(struct scmi_smc *scmi_info,
> +			 struct scmi_xfer *xfer __maybe_unused)
> +{
> +#ifndef CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE

If possible make it based some local variable or you can always do

	if (IS_ENABLED(CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE))

-- 
Regards,
Sudeep