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

Sudeep Holla
2021-12-13T11:42:41Z

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@arm.com>
> ---
> 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