Re: [PATCH V7 2/2] firmware: qcom: scm: Add wait-queue handling logic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sibi,

Few minor comments below,

On 10/01/2023 06:37, Sibi Sankar wrote:
From: Guru Das Srinagesh <quic_gurus@xxxxxxxxxxx>

When the firmware (FW) supports multiple requests per VM, multiple requests
from the same/different VM can reach the firmware at the same time. Since
the firmware currently being used has limited resources, it guards them
with a resource lock and puts requests on a wait-queue internally and
signals to HLOS that it is doing so. It does this by returning a new return
value in addition to success or error: SCM_WAITQ_SLEEP. A sleeping SCM call
can be woken up by an interrupt that the FW raises.

...

  drivers/firmware/qcom_scm-smc.c | 90 ++++++++++++++++++++++++++++++---
  drivers/firmware/qcom_scm.c     | 89 +++++++++++++++++++++++++++++++-
  drivers/firmware/qcom_scm.h     |  8 +++
  3 files changed, 179 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
index d111833364ba..30999f04749c 100644
--- a/drivers/firmware/qcom_scm-smc.c
+++ b/drivers/firmware/qcom_scm-smc.c
...
+static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq,
+					   struct arm_smccc_res *res)
+{
+	int ret;
+	struct arm_smccc_args resume;
+	u32 wq_ctx, smc_call_ctx, flags;
+	struct arm_smccc_args *smc = waitq;
+
+	do {
+		__scm_smc_do_quirk(smc, res);
+
+		if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
+			wq_ctx = res->a1;
+			smc_call_ctx = res->a2;
+			flags = res->a3;
+
+			if (!dev)
+				return -EPROBE_DEFER;

why are we checking dev pointer in the middle of the call?
A comment here would really help readers.

+
+			ret = qcom_scm_lookup_completion(wq_ctx);
+			if (ret)
+				return ret;
+
+			fill_wq_resume_args(&resume, smc_call_ctx);
+			smc = &resume;
+		}
+	} while (res->a0 == QCOM_SCM_WAITQ_SLEEP);
+
+	return 0;
+}
+
...
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index cdbfe54c8146..19ac506a9b1f 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -4,6 +4,7 @@
   */
  #include <linux/platform_device.h>
  #include <linux/init.h>
+#include <linux/interrupt.h>
  #include <linux/cpumask.h>
  #include <linux/export.h>
  #include <linux/dma-mapping.h>
@@ -13,6 +14,7 @@
  #include <linux/qcom_scm.h>
  #include <linux/of.h>
  #include <linux/of_address.h>
+#include <linux/of_irq.h>
  #include <linux/of_platform.h>
  #include <linux/clk.h>
  #include <linux/reset-controller.h>

include <linux/completion.h> ??


@@ -33,6 +35,7 @@ struct qcom_scm {
  	struct clk *iface_clk;
  	struct clk *bus_clk;
  	struct icc_path *path;
+	struct completion waitq_comp;
  	struct reset_controller_dev reset;
/* control access to the interconnect path */
@@ -63,6 +66,9 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
  	BIT(2), BIT(1), BIT(4), BIT(6)
  };
+#define QCOM_SMC_WAITQ_FLAG_WAKE_ONE BIT(0)
+#define QCOM_SMC_WAITQ_FLAG_WAKE_ALL	BIT(1)
+
  static const char * const qcom_scm_convention_names[] = {
  	[SMC_CONVENTION_UNKNOWN] = "unknown",
  	[SMC_CONVENTION_ARM_32] = "smc arm 32",
@@ -1325,11 +1331,79 @@ bool qcom_scm_is_available(void)
  }
  EXPORT_SYMBOL(qcom_scm_is_available);
+static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)
+{
+	/* assert wq_ctx is zero */ > +	if (wq_ctx != 0) {

Is this correct? looks like zero is the only valid one.

I thought wq_ctx was a unique number (UID).

+		dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return &scm->waitq_comp;
+}
+
+int qcom_scm_lookup_completion(u32 wq_ctx)
+{
+	struct completion *wq = NULL;
+
+	wq = qcom_scm_lookup_wq(__scm, wq_ctx);
+	if (IS_ERR(wq))
+		return PTR_ERR(wq);
+
+	wait_for_completion(wq);

We can potentially block here forever without a timeout.

As you are reusing completion, I have not seen any reinitialization of completion, this could potentially return above line without waiting at all.

+
+	return 0;
+}
+
+static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx, bool wake_all)
+{
+	struct completion *wq_to_wake;
+
+	wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
+	if (IS_ERR(wq_to_wake))
+		return PTR_ERR(wq_to_wake);
+
+	if (wake_all)
+		complete_all(wq_to_wake);
+	else
+		complete(wq_to_wake);

+
+	return 0;
+}
+
+static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
+{
+	int ret;
+	struct qcom_scm *scm = data;
+	u32 wq_ctx, flags, more_pending = 0;
+
+	do {
+		ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
+		if (ret) {
+			dev_err(scm->dev, "GET_WQ_CTX SMC call failed: %d\n", ret);
+			goto out;
+		}
+
+		if (flags != QCOM_SMC_WAITQ_FLAG_WAKE_ONE &&
+		    flags != QCOM_SMC_WAITQ_FLAG_WAKE_ALL) {
+			dev_err(scm->dev, "Invalid flags found for wq_ctx: %u\n", flags);
+			goto out;
+		}
+
+		ret = qcom_scm_waitq_wakeup(scm, wq_ctx, !!(flags & QCOM_SMC_WAITQ_FLAG_WAKE_ALL));
+		if (ret)
+			goto out;
+	} while (more_pending);
+
+out:
+	return IRQ_HANDLED;
+}
+
  static int qcom_scm_probe(struct platform_device *pdev)
  {
  	struct qcom_scm *scm;
  	unsigned long clks;
-	int ret;
+	int irq, ret;
scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
  	if (!scm)
@@ -1402,6 +1476,19 @@ static int qcom_scm_probe(struct platform_device *pdev)
  	__scm = scm;
  	__scm->dev = &pdev->dev;
+ init_completion(&__scm->waitq_comp);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		if (irq != -ENXIO)
+			return irq;
+	} else {
+		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
+						IRQF_ONESHOT, "qcom-scm", __scm);
+		if (ret < 0)
+			return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
+	}
+
  	__get_convention();
/*

--srini



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux