Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic
From: Rajendra Nayak
Date: Fri Jul 01 2022 - 07:21:17 EST
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@xxxxxxxxxxx>
---
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;
}