Messages in this thread Patch in this message | | | Date | Fri, 1 Jul 2022 16:51:00 +0530 | Subject | Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic | From | Rajendra Nayak <> |
| |
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; >> }
| |