* [PATCH 0/5] SCM: Add support for wait-queue aware firmware
@ 2022-06-27 19:44 Guru Das Srinagesh
2022-06-27 19:44 ` [PATCH 1/5] dt-bindings: firmware: qcom-scm: Add "allow-multi-call" property Guru Das Srinagesh
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Guru Das Srinagesh @ 2022-06-27 19:44 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Philipp Zabel, linux-arm-msm, linux-kernel
Cc: David Heidelberg, Robert Marko, Rajendra Nayak, Elliot Berman,
Guru Das Srinagesh
This patch series enables the QCOM SCM driver to support firmware (FW) versions
that expect the high-level OS (HLOS) to be tolerant of SCM call requests not
being processed right away and, instead, being placed on a wait-queue in FW and
processed accordingly.
The problem this feature is fixing is as follows. In a scenario where there is
a VM in addition to HLOS (and an underlying hypervisor):
1. HLOS makes an SMC call on core 5
2. The hypervisor scheduling interrupt interrupts this SMC call.
3. The hypervisor schedules the VM on core 5.
4. The VM makes an SMC call on core 5.
5. The SMC call is non-interruptibly stuck on FW spinlock on core 5.
6. HLOS cannot reschedule since core 5 is not responding to Reschedule IPIs.
7. Watchdog timer expires waiting for core 5.
This problem is solved by FW returning a new return code SCM_WAITQ_SLEEP to
HLOS right away when it is overwhelmed by the VM's SMC call. HLOS then places
the call on a wait-queue and wakes it up when it receives an interrupt that
signifies "all-clear".
This new design also supports scenarios involving only HLOS (and no other VMs).
Such scenarios make use of a second new return code SCM_WAITQ_WAKE.
There are three new SMC calls also being defined in this design that, together
with the two new return codes, form the handshake protocol between Linux and
FW.
This design is also backwards-compatible with existing firmware versions that
do not support this feature.
Guru Das Srinagesh (5):
dt-bindings: firmware: qcom-scm: Add "allow-multi-call" property
firmware: qcom: scm: Optionally remove SCM call serialization
dt-bindings: firmware: qcom-scm: Add optional interrupt
firmware: qcom: scm: Add wait-queue helper functions
firmware: qcom: scm: Add wait-queue handling logic
.../devicetree/bindings/firmware/qcom,scm.txt | 4 +
drivers/firmware/qcom_scm-smc.c | 143 +++++++++++++++++++--
drivers/firmware/qcom_scm.c | 124 +++++++++++++++++-
drivers/firmware/qcom_scm.h | 14 ++
4 files changed, 275 insertions(+), 10 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/5] dt-bindings: firmware: qcom-scm: Add "allow-multi-call" property
2022-06-27 19:44 [PATCH 0/5] SCM: Add support for wait-queue aware firmware Guru Das Srinagesh
@ 2022-06-27 19:44 ` Guru Das Srinagesh
2022-06-27 19:44 ` [PATCH 2/5] firmware: qcom: scm: Optionally remove SCM call serialization Guru Das Srinagesh
` (3 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Guru Das Srinagesh @ 2022-06-27 19:44 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Philipp Zabel, linux-arm-msm, linux-kernel
Cc: David Heidelberg, Robert Marko, Rajendra Nayak, Elliot Berman,
Guru Das Srinagesh
For firmware that supports it, allow multiple SCM calls to be passed
down to it by removing the serialization lock in the SCM driver.
There is a patch already [1] to convert the bindings to YAML. I will
make a patch against it when it gets accepted.
[1] https://lore.kernel.org/linux-arm-msm/20220626183247.142776-3-david@ixit.cz/
Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
---
Documentation/devicetree/bindings/firmware/qcom,scm.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
index 0f4e5ab..8a68018 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.txt
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
@@ -43,6 +43,8 @@ Required properties:
clock and "bus" for the bus clock per the requirements of the compatible.
- qcom,dload-mode: phandle to the TCSR hardware block and offset of the
download mode control register (optional)
+- allow-multi-call: Specify this to remove SCM call serialization. Need to
+ ensure that the firmware being used supports this.
Example for MSM8916:
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/5] firmware: qcom: scm: Optionally remove SCM call serialization
2022-06-27 19:44 [PATCH 0/5] SCM: Add support for wait-queue aware firmware Guru Das Srinagesh
2022-06-27 19:44 ` [PATCH 1/5] dt-bindings: firmware: qcom-scm: Add "allow-multi-call" property Guru Das Srinagesh
@ 2022-06-27 19:44 ` Guru Das Srinagesh
2022-06-27 19:44 ` [PATCH 3/5] dt-bindings: firmware: qcom-scm: Add optional interrupt Guru Das Srinagesh
` (2 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Guru Das Srinagesh @ 2022-06-27 19:44 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Philipp Zabel, linux-arm-msm, linux-kernel
Cc: David Heidelberg, Robert Marko, Rajendra Nayak, Elliot Berman,
Guru Das Srinagesh
Some firmware versions support the handling of multiple SCM calls at the
same time. Add a device tree boolean property which, when specified,
allows this to happen.
Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
---
drivers/firmware/qcom_scm-smc.c | 8 ++++++--
drivers/firmware/qcom_scm.c | 6 ++++++
drivers/firmware/qcom_scm.h | 2 ++
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
index d111833..66193c2 100644
--- a/drivers/firmware/qcom_scm-smc.c
+++ b/drivers/firmware/qcom_scm-smc.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/* Copyright (c) 2015,2019 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
*/
#include <linux/io.h>
@@ -63,11 +64,14 @@ static void __scm_smc_do(const struct arm_smccc_args *smc,
}
do {
- mutex_lock(&qcom_scm_lock);
+ if (!qcom_scm_allow_multicall)
+ mutex_lock(&qcom_scm_lock);
__scm_smc_do_quirk(smc, res);
- mutex_unlock(&qcom_scm_lock);
+ if (!qcom_scm_allow_multicall)
+ mutex_unlock(&qcom_scm_lock);
+
if (res->a0 == QCOM_SCM_V2_EBUSY) {
if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 3163660..4046073 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
/* Copyright (c) 2010,2015,2019 The Linux Foundation. All rights reserved.
* Copyright (C) 2015 Linaro Ltd.
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
*/
#include <linux/platform_device.h>
#include <linux/init.h>
@@ -22,6 +23,8 @@
static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
module_param(download_mode, bool, 0);
+bool qcom_scm_allow_multicall = false;
+
#define SCM_HAS_CORE_CLK BIT(0)
#define SCM_HAS_IFACE_CLK BIT(1)
#define SCM_HAS_BUS_CLK BIT(2)
@@ -1333,6 +1336,9 @@ static int qcom_scm_probe(struct platform_device *pdev)
__scm = scm;
__scm->dev = &pdev->dev;
+ qcom_scm_allow_multicall = of_property_read_bool(__scm->dev->of_node,
+ "allow-multi-call");
+
__get_convention();
/*
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 0d51eef..c0a4d6b 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/* Copyright (c) 2010-2015,2019 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
*/
#ifndef __QCOM_SCM_INT_H
#define __QCOM_SCM_INT_H
@@ -12,6 +13,7 @@ enum qcom_scm_convention {
};
extern enum qcom_scm_convention qcom_scm_convention;
+extern bool qcom_scm_allow_multicall;
#define MAX_QCOM_SCM_ARGS 10
#define MAX_QCOM_SCM_RETS 3
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/5] dt-bindings: firmware: qcom-scm: Add optional interrupt
2022-06-27 19:44 [PATCH 0/5] SCM: Add support for wait-queue aware firmware Guru Das Srinagesh
2022-06-27 19:44 ` [PATCH 1/5] dt-bindings: firmware: qcom-scm: Add "allow-multi-call" property Guru Das Srinagesh
2022-06-27 19:44 ` [PATCH 2/5] firmware: qcom: scm: Optionally remove SCM call serialization Guru Das Srinagesh
@ 2022-06-27 19:44 ` Guru Das Srinagesh
2022-06-27 19:44 ` [PATCH 4/5] firmware: qcom: scm: Add wait-queue helper functions Guru Das Srinagesh
2022-06-27 19:44 ` [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic Guru Das Srinagesh
4 siblings, 0 replies; 17+ messages in thread
From: Guru Das Srinagesh @ 2022-06-27 19:44 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Philipp Zabel, linux-arm-msm, linux-kernel
Cc: David Heidelberg, Robert Marko, Rajendra Nayak, Elliot Berman,
Guru Das Srinagesh
Add an interrupt specification to the bindings to support the wait-queue
feature.
There's a patch already in flight [1] to convert the bindings to YAML. I
will make a patch against it when it gets accepted.
[1] https://lore.kernel.org/linux-arm-msm/20220626183247.142776-3-david@ixit.cz/
Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
---
Documentation/devicetree/bindings/firmware/qcom,scm.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
index 8a68018..e1d746e 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.txt
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
@@ -45,6 +45,8 @@ Required properties:
download mode control register (optional)
- allow-multi-call: Specify this to remove SCM call serialization. Need to
ensure that the firmware being used supports this.
+- interrupts: Wait-queue interrupt that firmware raises as part of handshake
+ procedure to handle sleeping SCM calls.
Example for MSM8916:
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/5] firmware: qcom: scm: Add wait-queue helper functions
2022-06-27 19:44 [PATCH 0/5] SCM: Add support for wait-queue aware firmware Guru Das Srinagesh
` (2 preceding siblings ...)
2022-06-27 19:44 ` [PATCH 3/5] dt-bindings: firmware: qcom-scm: Add optional interrupt Guru Das Srinagesh
@ 2022-06-27 19:44 ` Guru Das Srinagesh
2022-06-28 13:33 ` Rajendra Nayak
2022-07-01 10:59 ` Rajendra Nayak
2022-06-27 19:44 ` [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic Guru Das Srinagesh
4 siblings, 2 replies; 17+ messages in thread
From: Guru Das Srinagesh @ 2022-06-27 19:44 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Philipp Zabel, linux-arm-msm, linux-kernel
Cc: David Heidelberg, Robert Marko, Rajendra Nayak, Elliot Berman,
Guru Das Srinagesh
When the firmware (FW) supports multiple requests per VM, and the VM also
supports it via the `enable-multi-call` device tree flag, the floodgates
are thrown open for them to all 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 two
new return values in addition to success or error: SCM_WAITQ_SLEEP and
SCM_WAITQ_WAKE.
1) SCM_WAITQ_SLEEP:
When an SCM call receives this return value instead of success
or error, FW has placed this call on a wait-queue and
has signalled HLOS to put it to non-interruptible sleep. (The
mechanism to wake it back up will be described in detail in the
next patch for the sake of simplicity.)
Along with this return value, FW also passes to HLOS `wq_ctx` -
a unique number (UID) identifying the wait-queue that it has put
the call on, internally. This is to help HLOS with its own
bookkeeping to wake this sleeping call later.
Additionally, FW also passes to HLOS `smc_call_ctx` - a UID
identifying the SCM call thus being put to sleep. This is also
for HLOS' bookkeeping to wake this call up later.
These two additional values are passed via the a1 and a2
registers.
N.B.: The "ctx" in the above UID names = "context".
2) SCM_WAITQ_WAKE:
When an SCM call receives this return value instead of success
or error, FW wishes to signal HLOS to wake up a (different)
previously sleeping call.
FW tells HLOS which call to wake up via the additional return
values `wq_ctx`, `smc_call_ctx` and `flags`. The first two have
already been explained above.
`flags` can be either WAKE_ONE or WAKE_ALL. Meaning, wake either
one, or all, of the SCM calls that HLOS is associating with the
given `wq_ctx`.
A sleeping SCM call can be woken up by either an interrupt that FW
raises, or via a SCM_WAITQ_WAKE return value for a new SCM call.
The handshake mechanism that HLOS uses to talk to FW about wait-queue
operations involves three new SMC calls. These are:
1) get_wq_ctx():
Arguments: None
Returns: wq_ctx, flags, more_pending
Get the wait-queue context, and wake up either one or all of the
sleeping SCM calls associated with that wait-queue.
Additionally, repeat this if there are more wait-queues that are
ready to have their requests woken up (`more_pending`).
2) wq_resume(smc_call_ctx):
Arguments: smc_call_ctx
HLOS needs to issue this in response to receiving an
IRQ, passing to FW the same smc_call_ctx that FW
receives from HLOS via the get_wq_ctx() call.
3) wq_wake_ack(smc_call_ctx):
Arguments: smc_call_ctx
HLOS needs to issue this in response to receiving an
SCM_WAITQ_WAKE, passing to FW the same smc_call_ctx that FW
passed to HLOS via the SMC_WAITQ_WAKE call.
(Reminder that the full handshake mechanism will be detailed in the
subsequent patch.)
Also add the interrupt handler that wakes up a sleeping SCM call.
Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
---
drivers/firmware/qcom_scm-smc.c | 56 +++++++++++++++++++
drivers/firmware/qcom_scm.c | 118 +++++++++++++++++++++++++++++++++++++++-
drivers/firmware/qcom_scm.h | 12 ++++
3 files changed, 185 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
index 66193c2..4150da1 100644
--- a/drivers/firmware/qcom_scm-smc.c
+++ b/drivers/firmware/qcom_scm-smc.c
@@ -53,6 +53,62 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
} while (res->a0 == QCOM_SCM_INTERRUPTED);
}
+static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
+{
+ memset(resume->args, 0, ARRAY_SIZE(resume->args));
+
+ resume->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
+ ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
+ SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_RESUME));
+
+ resume->args[1] = QCOM_SCM_ARGS(1);
+
+ resume->args[2] = smc_call_ctx;
+}
+
+static void fill_wq_wake_ack_args(struct arm_smccc_args *wake_ack, u32 smc_call_ctx)
+{
+ memset(wake_ack->args, 0, ARRAY_SIZE(wake_ack->args));
+
+ wake_ack->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
+ ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
+ SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_ACK));
+
+ wake_ack->args[1] = QCOM_SCM_ARGS(1);
+
+ wake_ack->args[2] = smc_call_ctx;
+}
+
+static void fill_get_wq_ctx_args(struct arm_smccc_args *get_wq_ctx)
+{
+ memset(get_wq_ctx->args, 0, ARRAY_SIZE(get_wq_ctx->args));
+
+ get_wq_ctx->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
+ ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
+ SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_WQ_CTX));
+}
+
+int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending)
+{
+ int ret;
+ struct arm_smccc_args get_wq_ctx = {0};
+ struct arm_smccc_res get_wq_res;
+
+ fill_get_wq_ctx_args(&get_wq_ctx);
+
+ __scm_smc_do_quirk(&get_wq_ctx, &get_wq_res);
+ /* Guaranteed to return only success or error, no WAITQ_* */
+ ret = get_wq_res.a0;
+ if (ret)
+ return ret;
+
+ *wq_ctx = get_wq_res.a1;
+ *flags = get_wq_res.a2;
+ *more_pending = get_wq_res.a3;
+
+ return 0;
+}
+
static void __scm_smc_do(const struct arm_smccc_args *smc,
struct arm_smccc_res *res, bool atomic)
{
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 4046073..248126c 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -3,8 +3,12 @@
* Copyright (C) 2015 Linaro Ltd.
* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
*/
+#define pr_fmt(fmt) "qcom-scm: %s: " fmt, __func__
+
#include <linux/platform_device.h>
+#include <linux/idr.h>
#include <linux/init.h>
+#include <linux/interrupt.h>
#include <linux/cpumask.h>
#include <linux/export.h>
#include <linux/dma-mapping.h>
@@ -12,10 +16,13 @@
#include <linux/types.h>
#include <linux/qcom_scm.h>
#include <linux/of.h>
+#include <linux/of_irq.h>
#include <linux/of_address.h>
#include <linux/of_platform.h>
#include <linux/clk.h>
#include <linux/reset-controller.h>
+#include <linux/spinlock.h>
+#include <linux/ktime.h>
#include <linux/arm-smccc.h>
#include "qcom_scm.h"
@@ -29,12 +36,19 @@ bool qcom_scm_allow_multicall = false;
#define SCM_HAS_IFACE_CLK BIT(1)
#define SCM_HAS_BUS_CLK BIT(2)
+struct qcom_scm_waitq {
+ struct idr idr;
+ spinlock_t idr_lock;
+ struct work_struct scm_irq_work;
+};
+
struct qcom_scm {
struct device *dev;
struct clk *core_clk;
struct clk *iface_clk;
struct clk *bus_clk;
struct reset_controller_dev reset;
+ struct qcom_scm_waitq waitq;
u64 dload_mode_addr;
};
@@ -56,10 +70,14 @@ struct qcom_scm_mem_map_info {
static const u8 qcom_scm_cpu_cold_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
0, BIT(0), BIT(3), BIT(5)
};
+
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",
@@ -1266,11 +1284,92 @@ bool qcom_scm_is_available(void)
}
EXPORT_SYMBOL(qcom_scm_is_available);
+struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)
+{
+ struct completion *wq = NULL;
+ u32 wq_ctx_idr = wq_ctx;
+ unsigned long flags;
+ int err;
+
+ spin_lock_irqsave(&scm->waitq.idr_lock, flags);
+ wq = idr_find(&scm->waitq.idr, wq_ctx);
+ if (wq)
+ goto out;
+
+ wq = devm_kzalloc(scm->dev, sizeof(*wq), GFP_ATOMIC);
+ if (!wq) {
+ wq = ERR_PTR(-ENOMEM);
+ goto out;
+ }
+
+ init_completion(wq);
+
+ err = idr_alloc_u32(&scm->waitq.idr, wq, &wq_ctx_idr,
+ U32_MAX, GFP_ATOMIC);
+ if (err < 0) {
+ devm_kfree(scm->dev, wq);
+ wq = ERR_PTR(err);
+ }
+
+out:
+ spin_unlock_irqrestore(&scm->waitq.idr_lock, flags);
+ return wq;
+}
+
+void scm_waitq_flag_handler(struct completion *wq, u32 flags)
+{
+ switch (flags) {
+ case QCOM_SMC_WAITQ_FLAG_WAKE_ONE:
+ complete(wq);
+ break;
+ case QCOM_SMC_WAITQ_FLAG_WAKE_ALL:
+ complete_all(wq);
+ break;
+ default:
+ pr_err("invalid flags: %u\n", flags);
+ }
+}
+
+static void scm_irq_work(struct work_struct *work)
+{
+ int ret;
+ u32 wq_ctx, flags, more_pending = 0;
+ struct completion *wq_to_wake;
+ struct qcom_scm_waitq *w = container_of(work, struct qcom_scm_waitq, scm_irq_work);
+ struct qcom_scm *scm = container_of(w, struct qcom_scm, waitq);
+
+ do {
+ ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
+ if (ret) {
+ pr_err("GET_WQ_CTX SMC call failed: %d\n", ret);
+ return;
+ }
+
+ wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
+ if (IS_ERR_OR_NULL(wq_to_wake)) {
+ pr_err("No waitqueue found for wq_ctx %d: %ld\n",
+ wq_ctx, PTR_ERR(wq_to_wake));
+ return;
+ }
+
+ scm_waitq_flag_handler(wq_to_wake, flags);
+ } while (more_pending);
+}
+
+static irqreturn_t qcom_scm_irq_handler(int irq, void *p)
+{
+ struct qcom_scm *scm = p;
+
+ schedule_work(&scm->waitq.scm_irq_work);
+
+ 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)
@@ -1333,12 +1432,28 @@ static int qcom_scm_probe(struct platform_device *pdev)
if (ret)
return ret;
+ platform_set_drvdata(pdev, scm);
+
__scm = scm;
__scm->dev = &pdev->dev;
+ spin_lock_init(&__scm->waitq.idr_lock);
+ idr_init(&__scm->waitq.idr);
qcom_scm_allow_multicall = of_property_read_bool(__scm->dev->of_node,
"allow-multi-call");
+ INIT_WORK(&__scm->waitq.scm_irq_work, scm_irq_work);
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq) {
+ ret = devm_request_threaded_irq(__scm->dev, irq, NULL,
+ qcom_scm_irq_handler, IRQF_ONESHOT, "qcom-scm", __scm);
+ if (ret < 0) {
+ pr_err("Failed to request qcom-scm irq: %d\n", ret);
+ return ret;
+ }
+ }
+
__get_convention();
/*
@@ -1354,6 +1469,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
static void qcom_scm_shutdown(struct platform_device *pdev)
{
+ idr_destroy(&__scm->waitq.idr);
/* Clean shutdown, disable download mode to allow normal restart */
if (download_mode)
qcom_scm_set_download_mode(false);
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index c0a4d6b..ae3a331 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -62,6 +62,11 @@ struct qcom_scm_res {
u64 result[MAX_QCOM_SCM_RETS];
};
+struct qcom_scm;
+extern struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx);
+extern void scm_waitq_flag_handler(struct completion *wq, u32 flags);
+extern int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending);
+
#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,
enum qcom_scm_convention qcom_convention,
@@ -131,6 +136,11 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
#define QCOM_SCM_SMMU_CONFIG_ERRATA1 0x03
#define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL 0x02
+#define QCOM_SCM_SVC_WAITQ 0x24
+#define QCOM_SCM_WAITQ_ACK 0x01
+#define QCOM_SCM_WAITQ_RESUME 0x02
+#define QCOM_SCM_WAITQ_GET_WQ_CTX 0x03
+
extern void __qcom_scm_init(void);
/* common error codes */
@@ -141,6 +151,8 @@ extern void __qcom_scm_init(void);
#define QCOM_SCM_EINVAL_ARG -2
#define QCOM_SCM_ERROR -1
#define QCOM_SCM_INTERRUPTED 1
+#define QCOM_SCM_WAITQ_SLEEP 2
+#define QCOM_SCM_WAITQ_WAKE 3
static inline int qcom_scm_remap_error(int err)
{
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic
2022-06-27 19:44 [PATCH 0/5] SCM: Add support for wait-queue aware firmware Guru Das Srinagesh
` (3 preceding siblings ...)
2022-06-27 19:44 ` [PATCH 4/5] firmware: qcom: scm: Add wait-queue helper functions Guru Das Srinagesh
@ 2022-06-27 19:44 ` Guru Das Srinagesh
2022-07-01 11:02 ` Rajendra Nayak
4 siblings, 1 reply; 17+ messages in thread
From: Guru Das Srinagesh @ 2022-06-27 19:44 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Philipp Zabel, linux-arm-msm, linux-kernel
Cc: David Heidelberg, Robert Marko, Rajendra Nayak, Elliot Berman,
Guru Das Srinagesh
Add logic to handle QCOM_SCM_WAITQ_SLEEP or QCOM_SCM_WAITQ_WAKE return
codes.
Scenario 1: Requests made by 2 different VMs:
VM_1 VM_2 Firmware
│ │ │
│ │ │
│ │ │
│ │ │
│ REQUEST_1 │ │
├────────────────────────┼─────────────────────────────────┤
│ │ │
│ │ ┌──┼──┐
│ │ │ │ │
│ │ REQUEST_2 │ │ │
│ ├──────────────────────────────┼──┤ │
│ │ │ │ │Resource
│ │ │ │ │is busy
│ │ {WQ_SLEEP} │ │ │
│ │◄─────────────────────────────┼──┤ │
│ │ wq_ctx, smc_call_ctx │ │ │
│ │ └──┼──┘
│ REQUEST_1 COMPLETE │ │
│◄───────────────────────┼─────────────────────────────────┤
│ │ │
│ │ IRQ │
│ │◄─-------------------------------│
│ │ │
│ │ get_wq_ctx() │
│ ├────────────────────────────────►│
│ │ │
│ │ │
│ │◄────────────────────────────────┤
│ │ wq_ctx, flags, and │
│ │ more_pending │
│ │ │
│ │ │
│ │ wq_resume(smc_call_ctx) │
│ ├────────────────────────────────►│
│ │ │
│ │ │
│ │ REQUEST_2 COMPLETE │
│ │◄────────────────────────────────┤
│ │ │
│ │ │
Scenario 2: Two Requests coming in from same VM:
VM_1 Firmware
│ │
│ │
│ │
│ │
│ REQUEST_1 │
├──────────────────────────────────────────────────────────┤
│ │
│ ┌────┼───┐
│ │ │ │
│ │ │ │
│ │ │ │
│ REQUEST_2 │ │ │
├─────────────────────────────────────────────────────┼───►│ │
│ │ │ │Resource
│ │ │ │is busy
│ {WQ_SLEEP} │ │ │
│◄────────────────────────────────────────────────────┼────┤ │
│ wq_ctx, req2_smc_call_ctx │ │ │
│ │ │ │
│ └────┼───┘
│ │
│ {WQ_WAKE} │
│◄─────────────────────────────────────────────────────────┤
│ wq_ctx, req1_smc_call_ctx, flags │
│ │
│ │
│ wq_wake_ack(req1_smc_call_ctx) │
├─────────────────────────────────────────────────────────►│
│ │
│ REQUEST_1 COMPLETE │
│◄─────────────────────────────────────────────────────────┤
│ │
│ │
│ wq_resume(req_2_smc_call_ctx) │
├─────────────────────────────────────────────────────────►│
│ │
│ REQUEST_2 COMPLETE │
│◄─────────────────────────────────────────────────────────┤
│ │
With the exception of get_wq_ctx(), the other two newly-introduced SMC
calls, wq_ack() and wq_resume() can themselves return WQ_SLEEP (these
nested rounds of WQ_SLEEP are not shown in the above diagram for the
sake of simplicity). Therefore, introduce a new do-while loop to handle
multiple WQ_SLEEP return values for the same parent SCM call.
Request Completion in the above diagram refers to either a success
return value (zero) or error (and not SMC_WAITQ_SLEEP or
SMC_WAITQ_WAKE).
Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
---
drivers/firmware/qcom_scm-smc.c | 79 +++++++++++++++++++++++++++++++++++++----
1 file changed, 72 insertions(+), 7 deletions(-)
diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
index 4150da1..fe95cc3 100644
--- a/drivers/firmware/qcom_scm-smc.c
+++ b/drivers/firmware/qcom_scm-smc.c
@@ -53,6 +53,9 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
} while (res->a0 == QCOM_SCM_INTERRUPTED);
}
+#define IS_WAITQ_SLEEP_OR_WAKE(res) \
+ (res->a0 == QCOM_SCM_WAITQ_SLEEP || res->a0 == QCOM_SCM_WAITQ_WAKE)
+
static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
{
memset(resume->args, 0, ARRAY_SIZE(resume->args));
@@ -109,25 +112,80 @@ int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending)
return 0;
}
-static void __scm_smc_do(const struct arm_smccc_args *smc,
+static int scm_smc_do_quirk(struct device *dev, struct arm_smccc_args *smc,
+ struct arm_smccc_res *res)
+{
+ struct completion *wq = NULL;
+ struct qcom_scm *qscm;
+ u32 wq_ctx, smc_call_ctx, flags;
+
+ do {
+ __scm_smc_do_quirk(smc, res);
+
+ if (IS_WAITQ_SLEEP_OR_WAKE(res)) {
+ wq_ctx = res->a1;
+ smc_call_ctx = res->a2;
+ flags = res->a3;
+
+ if (!dev)
+ return -EPROBE_DEFER;
+
+ qscm = dev_get_drvdata(dev);
+ wq = qcom_scm_lookup_wq(qscm, wq_ctx);
+ if (IS_ERR_OR_NULL(wq)) {
+ pr_err("No waitqueue found for wq_ctx %d: %ld\n",
+ wq_ctx, PTR_ERR(wq));
+ return PTR_ERR(wq);
+ }
+
+ if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
+ wait_for_completion(wq);
+ fill_wq_resume_args(smc, smc_call_ctx);
+ wq = NULL;
+ continue;
+ } else {
+ fill_wq_wake_ack_args(smc, smc_call_ctx);
+ continue;
+ }
+ } else if ((long)res->a0 < 0) {
+ /* Error, simply return to caller */
+ break;
+ } else {
+ /*
+ * Success.
+ * wq will be set only if a prior WAKE happened.
+ * Its value will be the one from the prior WAKE.
+ */
+ if (wq)
+ scm_waitq_flag_handler(wq, flags);
+ break;
+ }
+ } while (IS_WAITQ_SLEEP_OR_WAKE(res));
+
+ return 0;
+}
+
+static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
struct arm_smccc_res *res, bool atomic)
{
- int retry_count = 0;
+ int ret, retry_count = 0;
if (atomic) {
__scm_smc_do_quirk(smc, res);
- return;
+ return 0;
}
do {
if (!qcom_scm_allow_multicall)
mutex_lock(&qcom_scm_lock);
- __scm_smc_do_quirk(smc, res);
+ ret = scm_smc_do_quirk(dev, smc, res);
if (!qcom_scm_allow_multicall)
mutex_unlock(&qcom_scm_lock);
+ if (ret)
+ return ret;
if (res->a0 == QCOM_SCM_V2_EBUSY) {
if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
@@ -135,6 +193,8 @@ static void __scm_smc_do(const struct arm_smccc_args *smc,
msleep(QCOM_SCM_EBUSY_WAIT_MS);
}
} while (res->a0 == QCOM_SCM_V2_EBUSY);
+
+ return 0;
}
@@ -143,7 +203,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
struct qcom_scm_res *res, bool atomic)
{
int arglen = desc->arginfo & 0xf;
- int i;
+ int i, ret;
dma_addr_t args_phys = 0;
void *args_virt = NULL;
size_t alloc_len;
@@ -195,19 +255,24 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
smc.args[SCM_SMC_LAST_REG_IDX] = args_phys;
}
- __scm_smc_do(&smc, &smc_res, atomic);
+ ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
+ /* ret error check follows after args_virt cleanup*/
if (args_virt) {
dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE);
kfree(args_virt);
}
+ if (ret)
+ return ret;
+
if (res) {
res->result[0] = smc_res.a1;
res->result[1] = smc_res.a2;
res->result[2] = smc_res.a3;
}
- return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
+ ret = (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
+ return ret;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] firmware: qcom: scm: Add wait-queue helper functions
2022-06-27 19:44 ` [PATCH 4/5] firmware: qcom: scm: Add wait-queue helper functions Guru Das Srinagesh
@ 2022-06-28 13:33 ` Rajendra Nayak
2022-06-28 17:43 ` Guru Das Srinagesh
2022-07-14 1:04 ` Guru Das Srinagesh
2022-07-01 10:59 ` Rajendra Nayak
1 sibling, 2 replies; 17+ messages in thread
From: Rajendra Nayak @ 2022-06-28 13:33 UTC (permalink / raw)
To: Guru Das Srinagesh, Andy Gross, Bjorn Andersson, Philipp Zabel,
linux-arm-msm, linux-kernel
Cc: David Heidelberg, Robert Marko, Elliot Berman
On 6/28/2022 1:14 AM, Guru Das Srinagesh wrote:
> When the firmware (FW) supports multiple requests per VM, and the VM also
> supports it via the `enable-multi-call` device tree flag, the floodgates
the bindings actually define 'allow-multi-call'
> are thrown open for them to all 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 two
> new return values in addition to success or error: SCM_WAITQ_SLEEP and
> SCM_WAITQ_WAKE.
>
> 1) SCM_WAITQ_SLEEP:
>
> When an SCM call receives this return value instead of success
> or error, FW has placed this call on a wait-queue and
> has signalled HLOS to put it to non-interruptible sleep. (The
> mechanism to wake it back up will be described in detail in the
> next patch for the sake of simplicity.)
>
> Along with this return value, FW also passes to HLOS `wq_ctx` -
> a unique number (UID) identifying the wait-queue that it has put
> the call on, internally. This is to help HLOS with its own
> bookkeeping to wake this sleeping call later.
>
> Additionally, FW also passes to HLOS `smc_call_ctx` - a UID
> identifying the SCM call thus being put to sleep. This is also
> for HLOS' bookkeeping to wake this call up later.
>
> These two additional values are passed via the a1 and a2
> registers.
>
> N.B.: The "ctx" in the above UID names = "context".
>
> 2) SCM_WAITQ_WAKE:
>
> When an SCM call receives this return value instead of success
> or error, FW wishes to signal HLOS to wake up a (different)
> previously sleeping call.
>
> FW tells HLOS which call to wake up via the additional return
> values `wq_ctx`, `smc_call_ctx` and `flags`. The first two have
> already been explained above.
>
> `flags` can be either WAKE_ONE or WAKE_ALL. Meaning, wake either
> one, or all, of the SCM calls that HLOS is associating with the
> given `wq_ctx`.
>
> A sleeping SCM call can be woken up by either an interrupt that FW
> raises, or via a SCM_WAITQ_WAKE return value for a new SCM call.
>
> The handshake mechanism that HLOS uses to talk to FW about wait-queue
> operations involves three new SMC calls. These are:
>
> 1) get_wq_ctx():
>
> Arguments: None
> Returns: wq_ctx, flags, more_pending
>
> Get the wait-queue context, and wake up either one or all of the
> sleeping SCM calls associated with that wait-queue.
>
> Additionally, repeat this if there are more wait-queues that are
> ready to have their requests woken up (`more_pending`).
>
> 2) wq_resume(smc_call_ctx):
>
> Arguments: smc_call_ctx
>
> HLOS needs to issue this in response to receiving an
> IRQ, passing to FW the same smc_call_ctx that FW
> receives from HLOS via the get_wq_ctx() call.
>
> 3) wq_wake_ack(smc_call_ctx):
>
> Arguments: smc_call_ctx
>
> HLOS needs to issue this in response to receiving an
> SCM_WAITQ_WAKE, passing to FW the same smc_call_ctx that FW
> passed to HLOS via the SMC_WAITQ_WAKE call.
>
> (Reminder that the full handshake mechanism will be detailed in the
> subsequent patch.)
>
> Also add the interrupt handler that wakes up a sleeping SCM call.
>
> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
> ---
> drivers/firmware/qcom_scm-smc.c | 56 +++++++++++++++++++
> drivers/firmware/qcom_scm.c | 118 +++++++++++++++++++++++++++++++++++++++-
> drivers/firmware/qcom_scm.h | 12 ++++
> 3 files changed, 185 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
> index 66193c2..4150da1 100644
> --- a/drivers/firmware/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom_scm-smc.c
> @@ -53,6 +53,62 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
> } while (res->a0 == QCOM_SCM_INTERRUPTED);
> }
>
> +static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
> +{
> + memset(resume->args, 0, ARRAY_SIZE(resume->args));
> +
> + resume->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
> + ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
> + SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_RESUME));
> +
> + resume->args[1] = QCOM_SCM_ARGS(1);
> +
> + resume->args[2] = smc_call_ctx;
> +}
> +
> +static void fill_wq_wake_ack_args(struct arm_smccc_args *wake_ack, u32 smc_call_ctx)
> +{
> + memset(wake_ack->args, 0, ARRAY_SIZE(wake_ack->args));
> +
> + wake_ack->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
> + ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
> + SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_ACK));
> +
> + wake_ack->args[1] = QCOM_SCM_ARGS(1);
> +
> + wake_ack->args[2] = smc_call_ctx;
> +}
> +
> +static void fill_get_wq_ctx_args(struct arm_smccc_args *get_wq_ctx)
> +{
> + memset(get_wq_ctx->args, 0, ARRAY_SIZE(get_wq_ctx->args));
> +
> + get_wq_ctx->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
> + ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
> + SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_WQ_CTX));
> +}
> +
> +int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending)
> +{
> + int ret;
> + struct arm_smccc_args get_wq_ctx = {0};
> + struct arm_smccc_res get_wq_res;
> +
> + fill_get_wq_ctx_args(&get_wq_ctx);
> +
> + __scm_smc_do_quirk(&get_wq_ctx, &get_wq_res);
> + /* Guaranteed to return only success or error, no WAITQ_* */
> + ret = get_wq_res.a0;
> + if (ret)
> + return ret;
> +
> + *wq_ctx = get_wq_res.a1;
> + *flags = get_wq_res.a2;
> + *more_pending = get_wq_res.a3;
> +
> + return 0;
> +}
> +
> static void __scm_smc_do(const struct arm_smccc_args *smc,
> struct arm_smccc_res *res, bool atomic)
> {
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 4046073..248126c 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -3,8 +3,12 @@
> * Copyright (C) 2015 Linaro Ltd.
> * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> */
> +#define pr_fmt(fmt) "qcom-scm: %s: " fmt, __func__
> +
> #include <linux/platform_device.h>
> +#include <linux/idr.h>
> #include <linux/init.h>
> +#include <linux/interrupt.h>
> #include <linux/cpumask.h>
> #include <linux/export.h>
> #include <linux/dma-mapping.h>
> @@ -12,10 +16,13 @@
> #include <linux/types.h>
> #include <linux/qcom_scm.h>
> #include <linux/of.h>
> +#include <linux/of_irq.h>
> #include <linux/of_address.h>
> #include <linux/of_platform.h>
> #include <linux/clk.h>
> #include <linux/reset-controller.h>
> +#include <linux/spinlock.h>
> +#include <linux/ktime.h>
> #include <linux/arm-smccc.h>
>
> #include "qcom_scm.h"
> @@ -29,12 +36,19 @@ bool qcom_scm_allow_multicall = false;
> #define SCM_HAS_IFACE_CLK BIT(1)
> #define SCM_HAS_BUS_CLK BIT(2)
>
> +struct qcom_scm_waitq {
> + struct idr idr;
> + spinlock_t idr_lock;
> + struct work_struct scm_irq_work;
> +};
> +
> struct qcom_scm {
> struct device *dev;
> struct clk *core_clk;
> struct clk *iface_clk;
> struct clk *bus_clk;
> struct reset_controller_dev reset;
> + struct qcom_scm_waitq waitq;
>
> u64 dload_mode_addr;
> };
> @@ -56,10 +70,14 @@ struct qcom_scm_mem_map_info {
> static const u8 qcom_scm_cpu_cold_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
> 0, BIT(0), BIT(3), BIT(5)
> };
> +
> 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",
> @@ -1266,11 +1284,92 @@ bool qcom_scm_is_available(void)
> }
> EXPORT_SYMBOL(qcom_scm_is_available);
>
> +struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)
> +{
> + struct completion *wq = NULL;
> + u32 wq_ctx_idr = wq_ctx;
> + unsigned long flags;
> + int err;
> +
> + spin_lock_irqsave(&scm->waitq.idr_lock, flags);
> + wq = idr_find(&scm->waitq.idr, wq_ctx);
> + if (wq)
> + goto out;
> +
> + wq = devm_kzalloc(scm->dev, sizeof(*wq), GFP_ATOMIC);
> + if (!wq) {
> + wq = ERR_PTR(-ENOMEM);
> + goto out;
> + }
> +
> + init_completion(wq);
> +
> + err = idr_alloc_u32(&scm->waitq.idr, wq, &wq_ctx_idr,
> + U32_MAX, GFP_ATOMIC);
> + if (err < 0) {
> + devm_kfree(scm->dev, wq);
> + wq = ERR_PTR(err);
> + }
> +
> +out:
> + spin_unlock_irqrestore(&scm->waitq.idr_lock, flags);
> + return wq;
> +}
> +
> +void scm_waitq_flag_handler(struct completion *wq, u32 flags)
> +{
> + switch (flags) {
> + case QCOM_SMC_WAITQ_FLAG_WAKE_ONE:
> + complete(wq);
> + break;
> + case QCOM_SMC_WAITQ_FLAG_WAKE_ALL:
> + complete_all(wq);
> + break;
> + default:
> + pr_err("invalid flags: %u\n", flags);
> + }
> +}
> +
> +static void scm_irq_work(struct work_struct *work)
> +{
> + int ret;
> + u32 wq_ctx, flags, more_pending = 0;
> + struct completion *wq_to_wake;
> + struct qcom_scm_waitq *w = container_of(work, struct qcom_scm_waitq, scm_irq_work);
> + struct qcom_scm *scm = container_of(w, struct qcom_scm, waitq);
> +
> + do {
> + ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
> + if (ret) {
> + pr_err("GET_WQ_CTX SMC call failed: %d\n", ret);
> + return;
> + }
> +
> + wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
> + if (IS_ERR_OR_NULL(wq_to_wake)) {
> + pr_err("No waitqueue found for wq_ctx %d: %ld\n",
> + wq_ctx, PTR_ERR(wq_to_wake));
> + return;
> + }
> +
> + scm_waitq_flag_handler(wq_to_wake, flags);
> + } while (more_pending);
> +}
> +
> +static irqreturn_t qcom_scm_irq_handler(int irq, void *p)
> +{
> + struct qcom_scm *scm = p;
> +
> + schedule_work(&scm->waitq.scm_irq_work);
> +
> + 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)
> @@ -1333,12 +1432,28 @@ static int qcom_scm_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + platform_set_drvdata(pdev, scm);
> +
> __scm = scm;
> __scm->dev = &pdev->dev;
>
> + spin_lock_init(&__scm->waitq.idr_lock);
> + idr_init(&__scm->waitq.idr);
> qcom_scm_allow_multicall = of_property_read_bool(__scm->dev->of_node,
> "allow-multi-call");
>
> + INIT_WORK(&__scm->waitq.scm_irq_work, scm_irq_work);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq) {
> + ret = devm_request_threaded_irq(__scm->dev, irq, NULL,
> + qcom_scm_irq_handler, IRQF_ONESHOT, "qcom-scm", __scm);
> + if (ret < 0) {
> + pr_err("Failed to request qcom-scm irq: %d\n", ret);
> + return ret;
> + }
> + }
> +
> __get_convention();
>
> /*
> @@ -1354,6 +1469,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>
> static void qcom_scm_shutdown(struct platform_device *pdev)
> {
> + idr_destroy(&__scm->waitq.idr);
> /* Clean shutdown, disable download mode to allow normal restart */
> if (download_mode)
> qcom_scm_set_download_mode(false);
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index c0a4d6b..ae3a331 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -62,6 +62,11 @@ struct qcom_scm_res {
> u64 result[MAX_QCOM_SCM_RETS];
> };
>
> +struct qcom_scm;
> +extern struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx);
> +extern void scm_waitq_flag_handler(struct completion *wq, u32 flags);
> +extern int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending);
> +
> #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,
> enum qcom_scm_convention qcom_convention,
> @@ -131,6 +136,11 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
> #define QCOM_SCM_SMMU_CONFIG_ERRATA1 0x03
> #define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL 0x02
>
> +#define QCOM_SCM_SVC_WAITQ 0x24
> +#define QCOM_SCM_WAITQ_ACK 0x01
> +#define QCOM_SCM_WAITQ_RESUME 0x02
> +#define QCOM_SCM_WAITQ_GET_WQ_CTX 0x03
> +
> extern void __qcom_scm_init(void);
>
> /* common error codes */
> @@ -141,6 +151,8 @@ extern void __qcom_scm_init(void);
> #define QCOM_SCM_EINVAL_ARG -2
> #define QCOM_SCM_ERROR -1
> #define QCOM_SCM_INTERRUPTED 1
> +#define QCOM_SCM_WAITQ_SLEEP 2
> +#define QCOM_SCM_WAITQ_WAKE 3
>
> static inline int qcom_scm_remap_error(int err)
> {
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] firmware: qcom: scm: Add wait-queue helper functions
2022-06-28 13:33 ` Rajendra Nayak
@ 2022-06-28 17:43 ` Guru Das Srinagesh
2022-07-14 1:04 ` Guru Das Srinagesh
1 sibling, 0 replies; 17+ messages in thread
From: Guru Das Srinagesh @ 2022-06-28 17:43 UTC (permalink / raw)
To: Rajendra Nayak
Cc: Andy Gross, Bjorn Andersson, Philipp Zabel, linux-arm-msm,
linux-kernel, David Heidelberg, Robert Marko, Elliot Berman
On Jun 28 2022 19:03, Rajendra Nayak wrote:
>
>
> On 6/28/2022 1:14 AM, Guru Das Srinagesh wrote:
> >When the firmware (FW) supports multiple requests per VM, and the VM also
> >supports it via the `enable-multi-call` device tree flag, the floodgates
>
> the bindings actually define 'allow-multi-call'
Good catch, will fix this typo in the next patchset!
Thank you.
Guru Das.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] firmware: qcom: scm: Add wait-queue helper functions
2022-06-27 19:44 ` [PATCH 4/5] firmware: qcom: scm: Add wait-queue helper functions Guru Das Srinagesh
2022-06-28 13:33 ` Rajendra Nayak
@ 2022-07-01 10:59 ` Rajendra Nayak
2022-07-14 1:04 ` Guru Das Srinagesh
1 sibling, 1 reply; 17+ messages in thread
From: Rajendra Nayak @ 2022-07-01 10:59 UTC (permalink / raw)
To: Guru Das Srinagesh, Andy Gross, Bjorn Andersson, Philipp Zabel,
linux-arm-msm, linux-kernel
Cc: David Heidelberg, Robert Marko, Elliot Berman
On 6/28/2022 1:14 AM, Guru Das Srinagesh wrote:
> When the firmware (FW) supports multiple requests per VM, and the VM also
> supports it via the `enable-multi-call` device tree flag, the floodgates
> are thrown open for them to all 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 two
> new return values in addition to success or error: SCM_WAITQ_SLEEP and
> SCM_WAITQ_WAKE.
>
> 1) SCM_WAITQ_SLEEP:
>
> When an SCM call receives this return value instead of success
> or error, FW has placed this call on a wait-queue and
> has signalled HLOS to put it to non-interruptible sleep. (The
> mechanism to wake it back up will be described in detail in the
> next patch for the sake of simplicity.)
>
> Along with this return value, FW also passes to HLOS `wq_ctx` -
> a unique number (UID) identifying the wait-queue that it has put
> the call on, internally. This is to help HLOS with its own
> bookkeeping to wake this sleeping call later.
>
> Additionally, FW also passes to HLOS `smc_call_ctx` - a UID
> identifying the SCM call thus being put to sleep. This is also
> for HLOS' bookkeeping to wake this call up later.
>
> These two additional values are passed via the a1 and a2
> registers.
>
> N.B.: The "ctx" in the above UID names = "context".
>
> 2) SCM_WAITQ_WAKE:
>
> When an SCM call receives this return value instead of success
> or error, FW wishes to signal HLOS to wake up a (different)
> previously sleeping call.
What happens to this SCM call itself (The one which gets an SCM_WAITQ_WAKE returned
instead of a success or failure)?
is it processed? how does the firmware in that case return a success or error?
>
> FW tells HLOS which call to wake up via the additional return
> values `wq_ctx`, `smc_call_ctx` and `flags`. The first two have
> already been explained above.
>
> `flags` can be either WAKE_ONE or WAKE_ALL. Meaning, wake either
> one, or all, of the SCM calls that HLOS is associating with the
> given `wq_ctx`.
>
> A sleeping SCM call can be woken up by either an interrupt that FW
> raises, or via a SCM_WAITQ_WAKE return value for a new SCM call.
>
> The handshake mechanism that HLOS uses to talk to FW about wait-queue
> operations involves three new SMC calls. These are:
>
> 1) get_wq_ctx():
>
> Arguments: None
> Returns: wq_ctx, flags, more_pending
>
> Get the wait-queue context, and wake up either one or all of the
> sleeping SCM calls associated with that wait-queue.
>
> Additionally, repeat this if there are more wait-queues that are
> ready to have their requests woken up (`more_pending`).
>
> 2) wq_resume(smc_call_ctx):
>
> Arguments: smc_call_ctx
>
> HLOS needs to issue this in response to receiving an
> IRQ, passing to FW the same smc_call_ctx that FW
> receives from HLOS via the get_wq_ctx() call.
>
> 3) wq_wake_ack(smc_call_ctx):
>
> Arguments: smc_call_ctx
>
> HLOS needs to issue this in response to receiving an
> SCM_WAITQ_WAKE, passing to FW the same smc_call_ctx that FW
> passed to HLOS via the SMC_WAITQ_WAKE call.
>
> (Reminder that the full handshake mechanism will be detailed in the
> subsequent patch.)
>
> Also add the interrupt handler that wakes up a sleeping SCM call.
>
> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
[]...
> +
> 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)
> @@ -1333,12 +1432,28 @@ static int qcom_scm_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + platform_set_drvdata(pdev, scm);
> +
> __scm = scm;
> __scm->dev = &pdev->dev;
>
> + spin_lock_init(&__scm->waitq.idr_lock);
> + idr_init(&__scm->waitq.idr);
> qcom_scm_allow_multicall = of_property_read_bool(__scm->dev->of_node,
> "allow-multi-call");
>
> + INIT_WORK(&__scm->waitq.scm_irq_work, scm_irq_work);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq) {
> + ret = devm_request_threaded_irq(__scm->dev, irq, NULL,
> + qcom_scm_irq_handler, IRQF_ONESHOT, "qcom-scm", __scm);
> + if (ret < 0) {
> + pr_err("Failed to request qcom-scm irq: %d\n", ret);
idr_destroy()?
> + return ret;
> + }
> + }
> +
> __get_convention();
>
> /*
> @@ -1354,6 +1469,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>
> static void qcom_scm_shutdown(struct platform_device *pdev)
> {
> + idr_destroy(&__scm->waitq.idr);
> /* Clean shutdown, disable download mode to allow normal restart */
> if (download_mode)
> qcom_scm_set_download_mode(false);
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic
2022-06-27 19:44 ` [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic Guru Das Srinagesh
@ 2022-07-01 11:02 ` Rajendra Nayak
2022-07-01 11:21 ` Rajendra Nayak
0 siblings, 1 reply; 17+ messages in thread
From: Rajendra Nayak @ 2022-07-01 11:02 UTC (permalink / raw)
To: Guru Das Srinagesh, Andy Gross, Bjorn Andersson, Philipp Zabel,
linux-arm-msm, linux-kernel
Cc: David Heidelberg, Robert Marko, Elliot Berman
On 6/28/2022 1:14 AM, Guru Das Srinagesh wrote:
> Add logic to handle QCOM_SCM_WAITQ_SLEEP or QCOM_SCM_WAITQ_WAKE return
> codes.
>
> Scenario 1: Requests made by 2 different VMs:
>
> VM_1 VM_2 Firmware
> │ │ │
> │ │ │
> │ │ │
> │ │ │
> │ REQUEST_1 │ │
> ├────────────────────────┼─────────────────────────────────┤
> │ │ │
> │ │ ┌──┼──┐
> │ │ │ │ │
> │ │ REQUEST_2 │ │ │
> │ ├──────────────────────────────┼──┤ │
> │ │ │ │ │Resource
> │ │ │ │ │is busy
> │ │ {WQ_SLEEP} │ │ │
> │ │◄─────────────────────────────┼──┤ │
> │ │ wq_ctx, smc_call_ctx │ │ │
> │ │ └──┼──┘
> │ REQUEST_1 COMPLETE │ │
> │◄───────────────────────┼─────────────────────────────────┤
> │ │ │
> │ │ IRQ │
> │ │◄─-------------------------------│
> │ │ │
> │ │ get_wq_ctx() │
> │ ├────────────────────────────────►│
> │ │ │
> │ │ │
> │ │◄────────────────────────────────┤
> │ │ wq_ctx, flags, and │
> │ │ more_pending │
> │ │ │
> │ │ │
> │ │ wq_resume(smc_call_ctx) │
> │ ├────────────────────────────────►│
> │ │ │
> │ │ │
> │ │ REQUEST_2 COMPLETE │
> │ │◄────────────────────────────────┤
> │ │ │
> │ │ │
>
> Scenario 2: Two Requests coming in from same VM:
>
> VM_1 Firmware
> │ │
> │ │
> │ │
> │ │
> │ REQUEST_1 │
> ├──────────────────────────────────────────────────────────┤
> │ │
> │ ┌────┼───┐
> │ │ │ │
> │ │ │ │
> │ │ │ │
> │ REQUEST_2 │ │ │
> ├─────────────────────────────────────────────────────┼───►│ │
> │ │ │ │Resource
> │ │ │ │is busy
> │ {WQ_SLEEP} │ │ │
> │◄────────────────────────────────────────────────────┼────┤ │
> │ wq_ctx, req2_smc_call_ctx │ │ │
> │ │ │ │
> │ └────┼───┘
> │ │
> │ {WQ_WAKE} │
> │◄─────────────────────────────────────────────────────────┤
> │ wq_ctx, req1_smc_call_ctx, flags │
This is perhaps the same thing I asked on the previous patch,
I am guessing {WQ_WAKE} is returned in respone to REQUEST_1?
How do you know in this case if REQUEST_1 was a success or failure?
> │ │
> │ │
> │ wq_wake_ack(req1_smc_call_ctx) │
> ├─────────────────────────────────────────────────────────►│
> │ │
> │ REQUEST_1 COMPLETE │
> │◄─────────────────────────────────────────────────────────┤
> │ │
> │ │
> │ wq_resume(req_2_smc_call_ctx) │
> ├─────────────────────────────────────────────────────────►│
> │ │
> │ REQUEST_2 COMPLETE │
> │◄─────────────────────────────────────────────────────────┤
> │ │
>
> With the exception of get_wq_ctx(), the other two newly-introduced SMC
> calls, wq_ack() and wq_resume() can themselves return WQ_SLEEP (these
> nested rounds of WQ_SLEEP are not shown in the above diagram for the
> sake of simplicity). Therefore, introduce a new do-while loop to handle
> multiple WQ_SLEEP return values for the same parent SCM call.
>
> Request Completion in the above diagram refers to either a success
> return value (zero) or error (and not SMC_WAITQ_SLEEP or
> SMC_WAITQ_WAKE).
>
> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
> ---
> drivers/firmware/qcom_scm-smc.c | 79 +++++++++++++++++++++++++++++++++++++----
> 1 file changed, 72 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
> index 4150da1..fe95cc3 100644
> --- a/drivers/firmware/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom_scm-smc.c
> @@ -53,6 +53,9 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
> } while (res->a0 == QCOM_SCM_INTERRUPTED);
> }
>
> +#define IS_WAITQ_SLEEP_OR_WAKE(res) \
> + (res->a0 == QCOM_SCM_WAITQ_SLEEP || res->a0 == QCOM_SCM_WAITQ_WAKE)
> +
> static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
> {
> memset(resume->args, 0, ARRAY_SIZE(resume->args));
> @@ -109,25 +112,80 @@ int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending)
> return 0;
> }
>
> -static void __scm_smc_do(const struct arm_smccc_args *smc,
> +static int scm_smc_do_quirk(struct device *dev, struct arm_smccc_args *smc,
> + struct arm_smccc_res *res)
> +{
> + struct completion *wq = NULL;
> + struct qcom_scm *qscm;
> + u32 wq_ctx, smc_call_ctx, flags;
> +
> + do {
> + __scm_smc_do_quirk(smc, res);
> +
> + if (IS_WAITQ_SLEEP_OR_WAKE(res)) {
> + wq_ctx = res->a1;
> + smc_call_ctx = res->a2;
> + flags = res->a3;
> +
> + if (!dev)
> + return -EPROBE_DEFER;
> +
> + qscm = dev_get_drvdata(dev);
> + wq = qcom_scm_lookup_wq(qscm, wq_ctx);
> + if (IS_ERR_OR_NULL(wq)) {
> + pr_err("No waitqueue found for wq_ctx %d: %ld\n",
> + wq_ctx, PTR_ERR(wq));
> + return PTR_ERR(wq);
> + }
> +
> + if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
> + wait_for_completion(wq);
> + fill_wq_resume_args(smc, smc_call_ctx);
> + wq = NULL;
> + continue;
> + } else {
> + fill_wq_wake_ack_args(smc, smc_call_ctx);
> + continue;
> + }
> + } else if ((long)res->a0 < 0) {
> + /* Error, simply return to caller */
> + break;
> + } else {
> + /*
> + * Success.
> + * wq will be set only if a prior WAKE happened.
> + * Its value will be the one from the prior WAKE.
> + */
> + if (wq)
> + scm_waitq_flag_handler(wq, flags);
> + break;
> + }
> + } while (IS_WAITQ_SLEEP_OR_WAKE(res));
> +
> + return 0;
> +}
> +
> +static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
> struct arm_smccc_res *res, bool atomic)
> {
> - int retry_count = 0;
> + int ret, retry_count = 0;
>
> if (atomic) {
> __scm_smc_do_quirk(smc, res);
> - return;
> + return 0;
> }
>
> do {
> if (!qcom_scm_allow_multicall)
> mutex_lock(&qcom_scm_lock);
>
> - __scm_smc_do_quirk(smc, res);
> + ret = scm_smc_do_quirk(dev, smc, res);
>
> if (!qcom_scm_allow_multicall)
> mutex_unlock(&qcom_scm_lock);
>
> + if (ret)
> + return ret;
>
> if (res->a0 == QCOM_SCM_V2_EBUSY) {
> if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
> @@ -135,6 +193,8 @@ static void __scm_smc_do(const struct arm_smccc_args *smc,
> msleep(QCOM_SCM_EBUSY_WAIT_MS);
> }
> } while (res->a0 == QCOM_SCM_V2_EBUSY);
> +
> + return 0;
> }
>
>
> @@ -143,7 +203,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> struct qcom_scm_res *res, bool atomic)
> {
> int arglen = desc->arginfo & 0xf;
> - int i;
> + int i, ret;
> dma_addr_t args_phys = 0;
> void *args_virt = NULL;
> size_t alloc_len;
> @@ -195,19 +255,24 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> smc.args[SCM_SMC_LAST_REG_IDX] = args_phys;
> }
>
> - __scm_smc_do(&smc, &smc_res, atomic);
> + ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
> + /* ret error check follows after args_virt cleanup*/
>
> if (args_virt) {
> dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE);
> kfree(args_virt);
> }
>
> + if (ret)
> + return ret;
> +
> if (res) {
> res->result[0] = smc_res.a1;
> res->result[1] = smc_res.a2;
> res->result[2] = smc_res.a3;
> }
>
> - return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
> + ret = (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
>
> + return ret;
> }
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic
2022-07-01 11:02 ` Rajendra Nayak
@ 2022-07-01 11:21 ` Rajendra Nayak
2022-07-14 0:57 ` Guru Das Srinagesh
2022-07-21 16:48 ` Guru Das Srinagesh
0 siblings, 2 replies; 17+ messages in thread
From: Rajendra Nayak @ 2022-07-01 11:21 UTC (permalink / raw)
To: Guru Das Srinagesh, Andy Gross, Bjorn Andersson, Philipp Zabel,
linux-arm-msm, linux-kernel
Cc: David Heidelberg, Robert Marko, Elliot Berman
On 7/1/2022 4:32 PM, Rajendra Nayak wrote:
>
>
> On 6/28/2022 1:14 AM, Guru Das Srinagesh wrote:
>> Add logic to handle QCOM_SCM_WAITQ_SLEEP or QCOM_SCM_WAITQ_WAKE return
>> codes.
>>
>> Scenario 1: Requests made by 2 different VMs:
>>
>> VM_1 VM_2 Firmware
>> │ │ │
>> │ │ │
>> │ │ │
>> │ │ │
>> │ REQUEST_1 │ │
>> ├────────────────────────┼─────────────────────────────────┤
>> │ │ │
>> │ │ ┌──┼──┐
>> │ │ │ │ │
>> │ │ REQUEST_2 │ │ │
>> │ ├──────────────────────────────┼──┤ │
>> │ │ │ │ │Resource
>> │ │ │ │ │is busy
>> │ │ {WQ_SLEEP} │ │ │
>> │ │◄─────────────────────────────┼──┤ │
>> │ │ wq_ctx, smc_call_ctx │ │ │
>> │ │ └──┼──┘
>> │ REQUEST_1 COMPLETE │ │
>> │◄───────────────────────┼─────────────────────────────────┤
>> │ │ │
>> │ │ IRQ │
>> │ │◄─-------------------------------│
>> │ │ │
>> │ │ get_wq_ctx() │
>> │ ├────────────────────────────────►│
>> │ │ │
>> │ │ │
>> │ │◄────────────────────────────────┤
>> │ │ wq_ctx, flags, and │
>> │ │ more_pending │
>> │ │ │
>> │ │ │
>> │ │ wq_resume(smc_call_ctx) │
>> │ ├────────────────────────────────►│
>> │ │ │
>> │ │ │
>> │ │ REQUEST_2 COMPLETE │
>> │ │◄────────────────────────────────┤
>> │ │ │
>> │ │ │
>>
>> Scenario 2: Two Requests coming in from same VM:
>>
>> VM_1 Firmware
>> │ │
>> │ │
>> │ │
>> │ │
>> │ REQUEST_1 │
>> ├──────────────────────────────────────────────────────────┤
>> │ │
>> │ ┌────┼───┐
>> │ │ │ │
>> │ │ │ │
>> │ │ │ │
>> │ REQUEST_2 │ │ │
>> ├─────────────────────────────────────────────────────┼───►│ │
>> │ │ │ │Resource
>> │ │ │ │is busy
>> │ {WQ_SLEEP} │ │ │
>> │◄────────────────────────────────────────────────────┼────┤ │
>> │ wq_ctx, req2_smc_call_ctx │ │ │
>> │ │ │ │
>> │ └────┼───┘
>> │ │
>> │ {WQ_WAKE} │
>> │◄─────────────────────────────────────────────────────────┤
>> │ wq_ctx, req1_smc_call_ctx, flags │
>
>
> This is perhaps the same thing I asked on the previous patch,
> I am guessing {WQ_WAKE} is returned in respone to REQUEST_1?
> How do you know in this case if REQUEST_1 was a success or failure?
>
Ok looking at this some more, I think what we are saying is that the FW returns
{WQ_WAKE} to REQUEST_1, we then call wq_wake_ack and the return of
*that* will tell if REQUEST_1 was success or failure?
Did I get it right?
>
>> │ │
>> │ │
>> │ wq_wake_ack(req1_smc_call_ctx) │
>> ├─────────────────────────────────────────────────────────►│
>> │ │
>> │ REQUEST_1 COMPLETE │
>> │◄─────────────────────────────────────────────────────────┤
>> │ │
>> │ │
>> │ wq_resume(req_2_smc_call_ctx) │
>> ├─────────────────────────────────────────────────────────►│
>> │ │
>> │ REQUEST_2 COMPLETE │
>> │◄─────────────────────────────────────────────────────────┤
>> │ │
>>
>> With the exception of get_wq_ctx(), the other two newly-introduced SMC
>> calls, wq_ack() and wq_resume() can themselves return WQ_SLEEP (these
>> nested rounds of WQ_SLEEP are not shown in the above diagram for the
>> sake of simplicity). Therefore, introduce a new do-while loop to handle
>> multiple WQ_SLEEP return values for the same parent SCM call.
>>
>> Request Completion in the above diagram refers to either a success
>> return value (zero) or error (and not SMC_WAITQ_SLEEP or
>> SMC_WAITQ_WAKE).
>>
>> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
>> ---
>> drivers/firmware/qcom_scm-smc.c | 79 +++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 72 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
>> index 4150da1..fe95cc3 100644
>> --- a/drivers/firmware/qcom_scm-smc.c
>> +++ b/drivers/firmware/qcom_scm-smc.c
>> @@ -53,6 +53,9 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
>> } while (res->a0 == QCOM_SCM_INTERRUPTED);
>> }
>> +#define IS_WAITQ_SLEEP_OR_WAKE(res) \
>> + (res->a0 == QCOM_SCM_WAITQ_SLEEP || res->a0 == QCOM_SCM_WAITQ_WAKE)
>> +
>> static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
>> {
>> memset(resume->args, 0, ARRAY_SIZE(resume->args));
>> @@ -109,25 +112,80 @@ int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending)
>> return 0;
>> }
>> -static void __scm_smc_do(const struct arm_smccc_args *smc,
>> +static int scm_smc_do_quirk(struct device *dev, struct arm_smccc_args *smc,
>> + struct arm_smccc_res *res)
>> +{
>> + struct completion *wq = NULL;
>> + struct qcom_scm *qscm;
>> + u32 wq_ctx, smc_call_ctx, flags;
>> +
>> + do {
>> + __scm_smc_do_quirk(smc, res);
>> +
>> + if (IS_WAITQ_SLEEP_OR_WAKE(res)) {
>> + wq_ctx = res->a1;
>> + smc_call_ctx = res->a2;
>> + flags = res->a3;
>> +
>> + if (!dev)
>> + return -EPROBE_DEFER;
>> +
>> + qscm = dev_get_drvdata(dev);
>> + wq = qcom_scm_lookup_wq(qscm, wq_ctx);
>> + if (IS_ERR_OR_NULL(wq)) {
>> + pr_err("No waitqueue found for wq_ctx %d: %ld\n",
>> + wq_ctx, PTR_ERR(wq));
>> + return PTR_ERR(wq);
>> + }
>> +
>> + if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
>> + wait_for_completion(wq);
>> + fill_wq_resume_args(smc, smc_call_ctx);
>> + wq = NULL;
>> + continue;
>> + } else {
>> + fill_wq_wake_ack_args(smc, smc_call_ctx);
>> + continue;
>> + }
>> + } else if ((long)res->a0 < 0) {
>> + /* Error, simply return to caller */
>> + break;
if my understanding above is correct, shouldn't we do a
>> + if (wq)
>> + scm_waitq_flag_handler(wq, flags);
in the error case also?
Also why no just scm_waitq_flag_handler(wq, flags); before fill_wq_wake_ack_args(smc, smc_call_ctx);?
>> + } else {
>> + /*
>> + * Success.
>> + * wq will be set only if a prior WAKE happened.
>> + * Its value will be the one from the prior WAKE.
>> + */
>> + if (wq)
>> + scm_waitq_flag_handler(wq, flags);
>> + break;
>> + }
>> + } while (IS_WAITQ_SLEEP_OR_WAKE(res));
>> +
>> + return 0;
>> +}
>> +
>> +static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
>> struct arm_smccc_res *res, bool atomic)
>> {
>> - int retry_count = 0;
>> + int ret, retry_count = 0;
>> if (atomic) {
>> __scm_smc_do_quirk(smc, res);
>> - return;
>> + return 0;
>> }
>> do {
>> if (!qcom_scm_allow_multicall)
>> mutex_lock(&qcom_scm_lock);
>> - __scm_smc_do_quirk(smc, res);
>> + ret = scm_smc_do_quirk(dev, smc, res);
>> if (!qcom_scm_allow_multicall)
>> mutex_unlock(&qcom_scm_lock);
>> + if (ret)
>> + return ret;
>> if (res->a0 == QCOM_SCM_V2_EBUSY) {
>> if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
>> @@ -135,6 +193,8 @@ static void __scm_smc_do(const struct arm_smccc_args *smc,
>> msleep(QCOM_SCM_EBUSY_WAIT_MS);
>> }
>> } while (res->a0 == QCOM_SCM_V2_EBUSY);
>> +
>> + return 0;
>> }
>> @@ -143,7 +203,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>> struct qcom_scm_res *res, bool atomic)
>> {
>> int arglen = desc->arginfo & 0xf;
>> - int i;
>> + int i, ret;
>> dma_addr_t args_phys = 0;
>> void *args_virt = NULL;
>> size_t alloc_len;
>> @@ -195,19 +255,24 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>> smc.args[SCM_SMC_LAST_REG_IDX] = args_phys;
>> }
>> - __scm_smc_do(&smc, &smc_res, atomic);
>> + ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
>> + /* ret error check follows after args_virt cleanup*/
>> if (args_virt) {
>> dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE);
>> kfree(args_virt);
>> }
>> + if (ret)
>> + return ret;
>> +
>> if (res) {
>> res->result[0] = smc_res.a1;
>> res->result[1] = smc_res.a2;
>> res->result[2] = smc_res.a3;
>> }
>> - return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
>> + ret = (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
>> + return ret;
>> }
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic
2022-07-01 11:21 ` Rajendra Nayak
@ 2022-07-14 0:57 ` Guru Das Srinagesh
2022-07-19 10:33 ` Rajendra Nayak
2022-07-21 16:48 ` Guru Das Srinagesh
1 sibling, 1 reply; 17+ messages in thread
From: Guru Das Srinagesh @ 2022-07-14 0:57 UTC (permalink / raw)
To: Rajendra Nayak
Cc: Andy Gross, Bjorn Andersson, Philipp Zabel, linux-arm-msm,
linux-kernel, David Heidelberg, Robert Marko, Elliot Berman
On Jul 01 2022 16:51, Rajendra Nayak wrote:
>
>
> On 7/1/2022 4:32 PM, Rajendra Nayak wrote:
> >
> >
> >On 6/28/2022 1:14 AM, Guru Das Srinagesh wrote:
> >>Add logic to handle QCOM_SCM_WAITQ_SLEEP or QCOM_SCM_WAITQ_WAKE return
> >>codes.
> >>
> >>Scenario 1: Requests made by 2 different VMs:
> >>
> >> VM_1 VM_2 Firmware
> >> │ │ │
> >> │ │ │
> >> │ │ │
> >> │ │ │
> >> │ REQUEST_1 │ │
> >> ├────────────────────────┼─────────────────────────────────┤
> >> │ │ │
> >> │ │ ┌──┼──┐
> >> │ │ │ │ │
> >> │ │ REQUEST_2 │ │ │
> >> │ ├──────────────────────────────┼──┤ │
> >> │ │ │ │ │Resource
> >> │ │ │ │ │is busy
> >> │ │ {WQ_SLEEP} │ │ │
> >> │ │◄─────────────────────────────┼──┤ │
> >> │ │ wq_ctx, smc_call_ctx │ │ │
> >> │ │ └──┼──┘
> >> │ REQUEST_1 COMPLETE │ │
> >> │◄───────────────────────┼─────────────────────────────────┤
> >> │ │ │
> >> │ │ IRQ │
> >> │ │◄─-------------------------------│
> >> │ │ │
> >> │ │ get_wq_ctx() │
> >> │ ├────────────────────────────────►│
> >> │ │ │
> >> │ │ │
> >> │ │◄────────────────────────────────┤
> >> │ │ wq_ctx, flags, and │
> >> │ │ more_pending │
> >> │ │ │
> >> │ │ │
> >> │ │ wq_resume(smc_call_ctx) │
> >> │ ├────────────────────────────────►│
> >> │ │ │
> >> │ │ │
> >> │ │ REQUEST_2 COMPLETE │
> >> │ │◄────────────────────────────────┤
> >> │ │ │
> >> │ │ │
> >>
> >>Scenario 2: Two Requests coming in from same VM:
> >>
> >> VM_1 Firmware
> >> │ │
> >> │ │
> >> │ │
> >> │ │
> >> │ REQUEST_1 │
> >> ├──────────────────────────────────────────────────────────┤
> >> │ │
> >> │ ┌────┼───┐
> >> │ │ │ │
> >> │ │ │ │
> >> │ │ │ │
> >> │ REQUEST_2 │ │ │
> >> ├─────────────────────────────────────────────────────┼───►│ │
> >> │ │ │ │Resource
> >> │ │ │ │is busy
> >> │ {WQ_SLEEP} │ │ │
> >> │◄────────────────────────────────────────────────────┼────┤ │
> >> │ wq_ctx, req2_smc_call_ctx │ │ │
> >> │ │ │ │
> >> │ └────┼───┘
> >> │ │
> >> │ {WQ_WAKE} │
> >> │◄─────────────────────────────────────────────────────────┤
> >> │ wq_ctx, req1_smc_call_ctx, flags │
> >
> >
> >This is perhaps the same thing I asked on the previous patch,
> >I am guessing {WQ_WAKE} is returned in respone to REQUEST_1?
> >How do you know in this case if REQUEST_1 was a success or failure?
> >
>
> Ok looking at this some more, I think what we are saying is that the FW returns
> {WQ_WAKE} to REQUEST_1, we then call wq_wake_ack and the return of
> *that* will tell if REQUEST_1 was success or failure?
> Did I get it right?
Yes, that is correct. I should have added an explanatory note in the commit
message to this effect:
│ {WQ_WAKE} <-- Return value │
│◄─────────────────────────────────────────────────────────┤
│ wq_ctx, req1_smc_call_ctx, flags <-- Its payload │
What this means is that the WQ_WAKE is sent by the FW to VM1 (direction of
arrow is from right to left) and that the additional data packed as payload
indicate that it is meant for REQUEST_1 (`req1_smc_call_ctx`).
Hopefully this will help understand the diagram better.
Thank you.
Guru Das.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] firmware: qcom: scm: Add wait-queue helper functions
2022-07-01 10:59 ` Rajendra Nayak
@ 2022-07-14 1:04 ` Guru Das Srinagesh
0 siblings, 0 replies; 17+ messages in thread
From: Guru Das Srinagesh @ 2022-07-14 1:04 UTC (permalink / raw)
To: Rajendra Nayak
Cc: Andy Gross, Bjorn Andersson, Philipp Zabel, linux-arm-msm,
linux-kernel, David Heidelberg, Robert Marko, Elliot Berman
On Jul 01 2022 16:29, Rajendra Nayak wrote:
>
> On 6/28/2022 1:14 AM, Guru Das Srinagesh wrote:
> >When the firmware (FW) supports multiple requests per VM, and the VM also
> >supports it via the `enable-multi-call` device tree flag, the floodgates
> >are thrown open for them to all 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 two
> >new return values in addition to success or error: SCM_WAITQ_SLEEP and
> >SCM_WAITQ_WAKE.
> >
> > 1) SCM_WAITQ_SLEEP:
> >
> > When an SCM call receives this return value instead of success
> > or error, FW has placed this call on a wait-queue and
> > has signalled HLOS to put it to non-interruptible sleep. (The
> > mechanism to wake it back up will be described in detail in the
> > next patch for the sake of simplicity.)
> >
> > Along with this return value, FW also passes to HLOS `wq_ctx` -
> > a unique number (UID) identifying the wait-queue that it has put
> > the call on, internally. This is to help HLOS with its own
> > bookkeeping to wake this sleeping call later.
> >
> > Additionally, FW also passes to HLOS `smc_call_ctx` - a UID
> > identifying the SCM call thus being put to sleep. This is also
> > for HLOS' bookkeeping to wake this call up later.
> >
> > These two additional values are passed via the a1 and a2
> > registers.
> >
> > N.B.: The "ctx" in the above UID names = "context".
> >
> > 2) SCM_WAITQ_WAKE:
> >
> > When an SCM call receives this return value instead of success
> > or error, FW wishes to signal HLOS to wake up a (different)
> > previously sleeping call.
>
> What happens to this SCM call itself (The one which gets an SCM_WAITQ_WAKE returned
> instead of a success or failure)?
> is it processed? how does the firmware in that case return a success or error?
Hopefully, with the clarificatory note posted in response to your query on the
other patch, this is clear. To answer your question:
Let's refer to the SCM call that received an SCM_WAITQ_WAKE as the parent call.
The parent call's success or failure depends on the result of the wq_wake_ack()
call defined below.
>
...
> > 3) wq_wake_ack(smc_call_ctx):
> >
> > Arguments: smc_call_ctx
> >
> > HLOS needs to issue this in response to receiving an
> > SCM_WAITQ_WAKE, passing to FW the same smc_call_ctx that FW
> > passed to HLOS via the SMC_WAITQ_WAKE call.
...
> >+
> > 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)
> >@@ -1333,12 +1432,28 @@ static int qcom_scm_probe(struct platform_device *pdev)
> > if (ret)
> > return ret;
> >+ platform_set_drvdata(pdev, scm);
> >+
> > __scm = scm;
> > __scm->dev = &pdev->dev;
> >+ spin_lock_init(&__scm->waitq.idr_lock);
> >+ idr_init(&__scm->waitq.idr);
> > qcom_scm_allow_multicall = of_property_read_bool(__scm->dev->of_node,
> > "allow-multi-call");
> >+ INIT_WORK(&__scm->waitq.scm_irq_work, scm_irq_work);
> >+
> >+ irq = platform_get_irq(pdev, 0);
> >+ if (irq) {
> >+ ret = devm_request_threaded_irq(__scm->dev, irq, NULL,
> >+ qcom_scm_irq_handler, IRQF_ONESHOT, "qcom-scm", __scm);
> >+ if (ret < 0) {
> >+ pr_err("Failed to request qcom-scm irq: %d\n", ret);
>
> idr_destroy()?
Yes, will add in next patchset.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] firmware: qcom: scm: Add wait-queue helper functions
2022-06-28 13:33 ` Rajendra Nayak
2022-06-28 17:43 ` Guru Das Srinagesh
@ 2022-07-14 1:04 ` Guru Das Srinagesh
1 sibling, 0 replies; 17+ messages in thread
From: Guru Das Srinagesh @ 2022-07-14 1:04 UTC (permalink / raw)
To: Rajendra Nayak
Cc: Andy Gross, Bjorn Andersson, Philipp Zabel, linux-arm-msm,
linux-kernel, David Heidelberg, Robert Marko, Elliot Berman
On Jun 28 2022 19:03, Rajendra Nayak wrote:
>
>
> On 6/28/2022 1:14 AM, Guru Das Srinagesh wrote:
> >When the firmware (FW) supports multiple requests per VM, and the VM also
> >supports it via the `enable-multi-call` device tree flag, the floodgates
>
> the bindings actually define 'allow-multi-call'
Whoops, will fix in next patchset.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic
2022-07-14 0:57 ` Guru Das Srinagesh
@ 2022-07-19 10:33 ` Rajendra Nayak
2022-07-21 16:50 ` Guru Das Srinagesh
0 siblings, 1 reply; 17+ messages in thread
From: Rajendra Nayak @ 2022-07-19 10:33 UTC (permalink / raw)
To: Guru Das Srinagesh
Cc: Andy Gross, Bjorn Andersson, Philipp Zabel, linux-arm-msm,
linux-kernel, David Heidelberg, Robert Marko, Elliot Berman
On 7/14/2022 6:27 AM, Guru Das Srinagesh wrote:
> On Jul 01 2022 16:51, Rajendra Nayak wrote:
>>
>>
>> On 7/1/2022 4:32 PM, Rajendra Nayak wrote:
>>>
>>>
>>> On 6/28/2022 1:14 AM, Guru Das Srinagesh wrote:
>>>> Add logic to handle QCOM_SCM_WAITQ_SLEEP or QCOM_SCM_WAITQ_WAKE return
>>>> codes.
>>>>
>>>> Scenario 1: Requests made by 2 different VMs:
>>>>
>>>> VM_1 VM_2 Firmware
>>>> │ │ │
>>>> │ │ │
>>>> │ │ │
>>>> │ │ │
>>>> │ REQUEST_1 │ │
>>>> ├────────────────────────┼─────────────────────────────────┤
>>>> │ │ │
>>>> │ │ ┌──┼──┐
>>>> │ │ │ │ │
>>>> │ │ REQUEST_2 │ │ │
>>>> │ ├──────────────────────────────┼──┤ │
>>>> │ │ │ │ │Resource
>>>> │ │ │ │ │is busy
>>>> │ │ {WQ_SLEEP} │ │ │
>>>> │ │◄─────────────────────────────┼──┤ │
>>>> │ │ wq_ctx, smc_call_ctx │ │ │
>>>> │ │ └──┼──┘
>>>> │ REQUEST_1 COMPLETE │ │
>>>> │◄───────────────────────┼─────────────────────────────────┤
>>>> │ │ │
>>>> │ │ IRQ │
>>>> │ │◄─-------------------------------│
>>>> │ │ │
>>>> │ │ get_wq_ctx() │
>>>> │ ├────────────────────────────────►│
>>>> │ │ │
>>>> │ │ │
>>>> │ │◄────────────────────────────────┤
>>>> │ │ wq_ctx, flags, and │
>>>> │ │ more_pending │
>>>> │ │ │
>>>> │ │ │
>>>> │ │ wq_resume(smc_call_ctx) │
>>>> │ ├────────────────────────────────►│
>>>> │ │ │
>>>> │ │ │
>>>> │ │ REQUEST_2 COMPLETE │
>>>> │ │◄────────────────────────────────┤
>>>> │ │ │
>>>> │ │ │
>>>>
>>>> Scenario 2: Two Requests coming in from same VM:
>>>>
>>>> VM_1 Firmware
>>>> │ │
>>>> │ │
>>>> │ │
>>>> │ │
>>>> │ REQUEST_1 │
>>>> ├──────────────────────────────────────────────────────────┤
>>>> │ │
>>>> │ ┌────┼───┐
>>>> │ │ │ │
>>>> │ │ │ │
>>>> │ │ │ │
>>>> │ REQUEST_2 │ │ │
>>>> ├─────────────────────────────────────────────────────┼───►│ │
>>>> │ │ │ │Resource
>>>> │ │ │ │is busy
>>>> │ {WQ_SLEEP} │ │ │
>>>> │◄────────────────────────────────────────────────────┼────┤ │
>>>> │ wq_ctx, req2_smc_call_ctx │ │ │
>>>> │ │ │ │
>>>> │ └────┼───┘
>>>> │ │
>>>> │ {WQ_WAKE} │
>>>> │◄─────────────────────────────────────────────────────────┤
>>>> │ wq_ctx, req1_smc_call_ctx, flags │
>>>
>>>
>>> This is perhaps the same thing I asked on the previous patch,
>>> I am guessing {WQ_WAKE} is returned in respone to REQUEST_1?
>>> How do you know in this case if REQUEST_1 was a success or failure?
>>>
>>
>> Ok looking at this some more, I think what we are saying is that the FW returns
>> {WQ_WAKE} to REQUEST_1, we then call wq_wake_ack and the return of
>> *that* will tell if REQUEST_1 was success or failure?
>> Did I get it right?
>
> Yes, that is correct. I should have added an explanatory note in the commit
> message to this effect:
>
>
> │ {WQ_WAKE} <-- Return value │
> │◄─────────────────────────────────────────────────────────┤
> │ wq_ctx, req1_smc_call_ctx, flags <-- Its payload │
>
> What this means is that the WQ_WAKE is sent by the FW to VM1 (direction of
> arrow is from right to left) and that the additional data packed as payload
> indicate that it is meant for REQUEST_1 (`req1_smc_call_ctx`).
>
> Hopefully this will help understand the diagram better.
Ok thanks for the explanation, I actually had a few more comments down in that
patch which you did not answer, can you clarify them too?
>> + } else if ((long)res->a0 < 0) {
>> + /* Error, simply return to caller */
>> + break;
if my understanding above is correct, shouldn't we do a
>> + if (wq)
>> + scm_waitq_flag_handler(wq, flags);
in the error case also?
Also why no just scm_waitq_flag_handler(wq, flags); before fill_wq_wake_ack_args(smc, smc_call_ctx);?
>
> Thank you.
>
> Guru Das.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic
2022-07-01 11:21 ` Rajendra Nayak
2022-07-14 0:57 ` Guru Das Srinagesh
@ 2022-07-21 16:48 ` Guru Das Srinagesh
1 sibling, 0 replies; 17+ messages in thread
From: Guru Das Srinagesh @ 2022-07-21 16:48 UTC (permalink / raw)
To: Rajendra Nayak
Cc: Andy Gross, Bjorn Andersson, Philipp Zabel, linux-arm-msm,
linux-kernel, David Heidelberg, Robert Marko, Elliot Berman
On Jul 01 2022 16:51, Rajendra Nayak wrote:
> >>+
> >>+ if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
> >>+ wait_for_completion(wq);
> >>+ fill_wq_resume_args(smc, smc_call_ctx);
> >>+ wq = NULL;
> >>+ continue;
> >>+ } else {
> >>+ fill_wq_wake_ack_args(smc, smc_call_ctx);
> >>+ continue;
> >>+ }
> >>+ } else if ((long)res->a0 < 0) {
> >>+ /* Error, simply return to caller */
> >>+ break;
>
> if my understanding above is correct, shouldn't we do a
> >>+ if (wq)
> >>+ scm_waitq_flag_handler(wq, flags);
> in the error case also?
Yes, you're right, since both error or success means that a request is
complete. We should act the same way for error as for success. Thanks for
catching this.
> Also why no just scm_waitq_flag_handler(wq, flags); before fill_wq_wake_ack_args(smc, smc_call_ctx);?
Because that is not part of the protocol - calling scm_waitq_flag_handler(wq, flags)
would result in a completion being freed, meaning a sleeping call would be
woken up, which is not what we want. When a WAITQ_WAKE is received, the
action to be taken is to immediately respond with a wq_wake_ack() and nothing
else.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic
2022-07-19 10:33 ` Rajendra Nayak
@ 2022-07-21 16:50 ` Guru Das Srinagesh
0 siblings, 0 replies; 17+ messages in thread
From: Guru Das Srinagesh @ 2022-07-21 16:50 UTC (permalink / raw)
To: Rajendra Nayak
Cc: Andy Gross, Bjorn Andersson, Philipp Zabel, linux-arm-msm,
linux-kernel, David Heidelberg, Robert Marko, Elliot Berman
On Jul 19 2022 16:03, Rajendra Nayak wrote:
> Ok thanks for the explanation, I actually had a few more comments down in that
> patch which you did not answer, can you clarify them too?
Just replied. Sorry, missed them amidst the wall of lines of context. Could you
please remove all lines of context that are not relevant while replying? That
will help draw attention to the comments that are being made.
Thank you.
Guru Das.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-07-21 16:50 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-27 19:44 [PATCH 0/5] SCM: Add support for wait-queue aware firmware Guru Das Srinagesh
2022-06-27 19:44 ` [PATCH 1/5] dt-bindings: firmware: qcom-scm: Add "allow-multi-call" property Guru Das Srinagesh
2022-06-27 19:44 ` [PATCH 2/5] firmware: qcom: scm: Optionally remove SCM call serialization Guru Das Srinagesh
2022-06-27 19:44 ` [PATCH 3/5] dt-bindings: firmware: qcom-scm: Add optional interrupt Guru Das Srinagesh
2022-06-27 19:44 ` [PATCH 4/5] firmware: qcom: scm: Add wait-queue helper functions Guru Das Srinagesh
2022-06-28 13:33 ` Rajendra Nayak
2022-06-28 17:43 ` Guru Das Srinagesh
2022-07-14 1:04 ` Guru Das Srinagesh
2022-07-01 10:59 ` Rajendra Nayak
2022-07-14 1:04 ` Guru Das Srinagesh
2022-06-27 19:44 ` [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic Guru Das Srinagesh
2022-07-01 11:02 ` Rajendra Nayak
2022-07-01 11:21 ` Rajendra Nayak
2022-07-14 0:57 ` Guru Das Srinagesh
2022-07-19 10:33 ` Rajendra Nayak
2022-07-21 16:50 ` Guru Das Srinagesh
2022-07-21 16:48 ` Guru Das Srinagesh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).