From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C0710C433EF for ; Mon, 13 Dec 2021 11:44:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=xXIueCgu4m9gbPcrNDT/AnAPNtYAPzCgBtLw9dWKi3c=; b=Le7HS1J+hOYBcS SBR+qQB80Jem9wQg9rFk+51oKYq7mBmpSk+hjyzSPEGbx8ekYEUC2De8vSUF1nHY0lnoUEEznKJFT JjtBXo2DDiiDPDfxAQU6zSltVzQIy57Ee0blNNIQeqrDvRbqhwWnZTnvVp4pN++a++2eMP+Rfmptq puYwm0wyz8ovr2m8yLfoT/gKmNeh5SSs0jM4pr8hOWPh9+7LOeP3Y3EopjdpEIaqueCVwtMxEjP61 PfjLZWY6jD8cbo++c5b0ETAwCMvXvyHT1bn1Hd2AkWq8gaPaZwQjfXFp0vwTLKWfHW0E8YoZiviHY LLEMBRuIvUZkcQYlaYEw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mwjjC-009LG7-4G; Mon, 13 Dec 2021 11:42:46 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mwjj5-009LES-1p for linux-arm-kernel@lists.infradead.org; Mon, 13 Dec 2021 11:42:40 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 02C8C6D; Mon, 13 Dec 2021 03:42:38 -0800 (PST) Received: from bogus (unknown [10.57.33.218]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7F7573F793; Mon, 13 Dec 2021 03:42:36 -0800 (PST) Date: Mon, 13 Dec 2021 11:42:33 +0000 From: Sudeep Holla To: Cristian Marussi Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, james.quinlan@broadcom.com, Jonathan.Cameron@Huawei.com, f.fainelli@gmail.com, etienne.carriere@linaro.org, Sudeep Holla , vincent.guittot@linaro.org, souvik.chakravarty@arm.com Subject: Re: [PATCH v7 12/16] firmware: arm_scmi: Add atomic mode support to smc transport Message-ID: <20211213114233.iwxllcmggkwhglvm@bogus> References: <20211129191156.29322-1-cristian.marussi@arm.com> <20211129191156.29322-13-cristian.marussi@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20211129191156.29322-13-cristian.marussi@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211213_034239_218481_2E9D0D79 X-CRM114-Status: GOOD ( 26.62 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > --- > 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 > +#include > #include > #include > #include > @@ -14,6 +15,9 @@ > #include > #include > #include > +#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE > +#include > +#endif > #include > > #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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel