| Message ID | 1656359076-13018-6-git-send-email-quic_gurus@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SCM: Add support for wait-queue aware firmware | expand |
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; > }
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; >> }
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.
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.
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.
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.
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; }
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(-)