All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Boyd <swboyd@chromium.org>
To: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	Elliot Berman <eberman@codeaurora.org>,
	Brian Masney <masneyb@onstation.org>,
	Stephan Gerhold <stephan@gerhold.net>,
	Jeffrey Hugo <jhugo@codeaurora.org>,
	Douglas Anderson <dianders@chromium.org>
Subject: [PATCH 2/6] firmware: qcom_scm: Reduce locking section for __get_convention()
Date: Tue, 23 Feb 2021 13:45:35 -0800	[thread overview]
Message-ID: <20210223214539.1336155-3-swboyd@chromium.org> (raw)
In-Reply-To: <20210223214539.1336155-1-swboyd@chromium.org>

We shouldn't need to hold this spinlock here around the entire SCM call
into the firmware and back. Instead, we should be able to query the
firmware, potentially in parallel with other CPUs making the same
convention detection firmware call, and then grab the lock to update the
calling convention detected. The convention doesn't change at runtime so
calling into firmware more than once is possibly wasteful but simpler.
Besides, this is the slow path, not the fast path where we've already
detected the convention used.

More importantly, this allows us to add more logic here to workaround
the case where the firmware call to check for availability isn't
implemented in the firmware at all. In that case we can check the
firmware node compatible string and force a calling convention.

Note that we remove the 'has_queried' logic that is repeated twice. That
could lead to the calling convention being printed multiple times to the
kernel logs if the bool is true but __query_convention() is running on
multiple CPUs. We also shorten the time where the lock is held, but we
keep the lock held around the printk because it doesn't seem hugely
important to drop it for that.

Cc: Elliot Berman <eberman@codeaurora.org>
Cc: Brian Masney <masneyb@onstation.org>
Cc: Stephan Gerhold <stephan@gerhold.net>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Douglas Anderson <dianders@chromium.org>
Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/qcom_scm-smc.c | 12 ++++---
 drivers/firmware/qcom_scm.c     | 55 ++++++++++++++++-----------------
 drivers/firmware/qcom_scm.h     |  7 +++--
 3 files changed, 39 insertions(+), 35 deletions(-)

diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
index 497c13ba98d6..d111833364ba 100644
--- a/drivers/firmware/qcom_scm-smc.c
+++ b/drivers/firmware/qcom_scm-smc.c
@@ -77,8 +77,10 @@ static void __scm_smc_do(const struct arm_smccc_args *smc,
 	}  while (res->a0 == QCOM_SCM_V2_EBUSY);
 }
 
-int scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
-		 struct qcom_scm_res *res, bool atomic)
+
+int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
+		   enum qcom_scm_convention qcom_convention,
+		   struct qcom_scm_res *res, bool atomic)
 {
 	int arglen = desc->arginfo & 0xf;
 	int i;
@@ -87,9 +89,8 @@ int scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 	size_t alloc_len;
 	gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
 	u32 smccc_call_type = atomic ? ARM_SMCCC_FAST_CALL : ARM_SMCCC_STD_CALL;
-	u32 qcom_smccc_convention =
-			(qcom_scm_convention == SMC_CONVENTION_ARM_32) ?
-			ARM_SMCCC_SMC_32 : ARM_SMCCC_SMC_64;
+	u32 qcom_smccc_convention = (qcom_convention == SMC_CONVENTION_ARM_32) ?
+				    ARM_SMCCC_SMC_32 : ARM_SMCCC_SMC_64;
 	struct arm_smccc_res smc_res;
 	struct arm_smccc_args smc = {0};
 
@@ -148,4 +149,5 @@ int scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 	}
 
 	return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
+
 }
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 2be5573dce53..21e07a464bd9 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -113,11 +113,10 @@ static void qcom_scm_clk_disable(void)
 	clk_disable_unprepare(__scm->bus_clk);
 }
 
-enum qcom_scm_convention qcom_scm_convention;
-static bool has_queried __read_mostly;
-static DEFINE_SPINLOCK(query_lock);
+enum qcom_scm_convention qcom_scm_convention = SMC_CONVENTION_UNKNOWN;
+static DEFINE_SPINLOCK(scm_query_lock);
 
-static void __query_convention(void)
+static enum qcom_scm_convention __get_convention(void)
 {
 	unsigned long flags;
 	struct qcom_scm_desc desc = {
@@ -130,36 +129,36 @@ static void __query_convention(void)
 		.owner = ARM_SMCCC_OWNER_SIP,
 	};
 	struct qcom_scm_res res;
+	enum qcom_scm_convention probed_convention;
 	int ret;
 
-	spin_lock_irqsave(&query_lock, flags);
-	if (has_queried)
-		goto out;
+	if (likely(qcom_scm_convention != SMC_CONVENTION_UNKNOWN))
+		return qcom_scm_convention;
 
-	qcom_scm_convention = SMC_CONVENTION_ARM_64;
-	// Device isn't required as there is only one argument - no device
-	// needed to dma_map_single to secure world
-	ret = scm_smc_call(NULL, &desc, &res, true);
+	/*
+	 * Device isn't required as there is only one argument - no device
+	 * needed to dma_map_single to secure world
+	 */
+	probed_convention = SMC_CONVENTION_ARM_64;
+	ret = __scm_smc_call(NULL, &desc, probed_convention, &res, true);
 	if (!ret && res.result[0] == 1)
-		goto out;
+		goto found;
 
-	qcom_scm_convention = SMC_CONVENTION_ARM_32;
-	ret = scm_smc_call(NULL, &desc, &res, true);
+	probed_convention = SMC_CONVENTION_ARM_32;
+	ret = __scm_smc_call(NULL, &desc, probed_convention, &res, true);
 	if (!ret && res.result[0] == 1)
-		goto out;
-
-	qcom_scm_convention = SMC_CONVENTION_LEGACY;
-out:
-	has_queried = true;
-	spin_unlock_irqrestore(&query_lock, flags);
-	pr_info("qcom_scm: convention: %s\n",
-		qcom_scm_convention_names[qcom_scm_convention]);
-}
+		goto found;
+
+	probed_convention = SMC_CONVENTION_LEGACY;
+found:
+	spin_lock_irqsave(&scm_query_lock, flags);
+	if (probed_convention != qcom_scm_convention) {
+		qcom_scm_convention = probed_convention;
+		pr_info("qcom_scm: convention: %s\n",
+			qcom_scm_convention_names[qcom_scm_convention]);
+	}
+	spin_unlock_irqrestore(&scm_query_lock, flags);
 
-static inline enum qcom_scm_convention __get_convention(void)
-{
-	if (unlikely(!has_queried))
-		__query_convention();
 	return qcom_scm_convention;
 }
 
@@ -1239,7 +1238,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	__scm = scm;
 	__scm->dev = &pdev->dev;
 
-	__query_convention();
+	__get_convention();
 
 	/*
 	 * If requested enable "download mode", from this point on warmboot
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 95cd1ac30ab0..632fe3142462 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -61,8 +61,11 @@ struct qcom_scm_res {
 };
 
 #define SCM_SMC_FNID(s, c)	((((s) & 0xFF) << 8) | ((c) & 0xFF))
-extern int scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
-			struct qcom_scm_res *res, bool atomic);
+extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
+			  enum qcom_scm_convention qcom_convention,
+			  struct qcom_scm_res *res, bool atomic);
+#define scm_smc_call(dev, desc, res, atomic) \
+	__scm_smc_call((dev), (desc), qcom_scm_convention, (res), (atomic))
 
 #define SCM_LEGACY_FNID(s, c)	(((s) << 10) | ((c) & 0x3ff))
 extern int scm_legacy_call_atomic(struct device *dev,
-- 
https://chromeos.dev


  parent reply	other threads:[~2021-02-23 21:47 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-23 21:45 [PATCH 0/6] firmware: qcom_scm: Fix SMCCC detection on sc7180 Stephen Boyd
2021-02-23 21:45 ` [PATCH 1/6] firmware: qcom_scm: Make __qcom_scm_is_call_available() return bool Stephen Boyd
2021-03-06  0:44   ` Bjorn Andersson
2021-02-23 21:45 ` Stephen Boyd [this message]
2021-02-23 21:45 ` [PATCH 3/6] firmware: qcom_scm: Workaround lack of "is available" call on SC7180 Stephen Boyd
2021-02-23 23:38   ` Jeffrey Hugo
2021-02-23 23:46     ` Stephen Boyd
2021-02-23 21:45 ` [PATCH 4/6] firmware: qcom_scm: Suppress sysfs bind attributes Stephen Boyd
2021-02-23 21:45 ` [PATCH 5/6] firmware: qcom_scm: Fix kernel-doc function names to match Stephen Boyd
2021-02-23 21:45 ` [PATCH 6/6] firmware: qcom_scm: Only compile legacy calls on ARM Stephen Boyd
2021-03-04  3:35   ` Elliot Berman
2021-03-04  6:14     ` Stephen Boyd
2021-03-05 18:18       ` Elliot Berman
2021-03-06  6:18         ` Stephen Boyd
2021-03-07 17:42           ` Bjorn Andersson
2021-03-23  3:36             ` Stephen Boyd
2021-03-23 18:27               ` Elliot Berman
2021-03-23 18:46                 ` Bjorn Andersson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210223214539.1336155-3-swboyd@chromium.org \
    --to=swboyd@chromium.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=dianders@chromium.org \
    --cc=eberman@codeaurora.org \
    --cc=jhugo@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=masneyb@onstation.org \
    --cc=stephan@gerhold.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.