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

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

 





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;
  }



[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