From: Bjorn Andersson <andersson@kernel.org>
To: Guru Das Srinagesh <quic_gurus@quicinc.com>
Cc: Andy Gross <agross@kernel.org>,
Philipp Zabel <p.zabel@pengutronix.de>,
linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
David Heidelberg <david@ixit.cz>,
Robert Marko <robimarko@gmail.com>,
Rajendra Nayak <quic_rjendra@quicinc.com>,
Elliot Berman <quic_eberman@quicinc.com>
Subject: Re: [RESEND PATCH v2 5/5] firmware: qcom: scm: Add wait-queue handling logic
Date: Wed, 19 Oct 2022 18:23:00 -0500 [thread overview]
Message-ID: <20221019232300.pw4cpbzlvputury2@builder.lan> (raw)
In-Reply-To: <1661898311-30126-6-git-send-email-quic_gurus@quicinc.com>
On Tue, Aug 30, 2022 at 03:25:11PM -0700, 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 │
> │ │
> │ │
> │ 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 | 76 +++++++++++++++++++++++++++++++++++++----
> 1 file changed, 69 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
> index 4150da1..09cca48 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,77 @@ 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,
There was a reasoning behind using the somewhat cryptic name
__scm_smc_do_quirk(), but wrapping it in a function with the same name
without the underscores is probably just going to confuse the reader
even more.
How about __scm_smc_do_quirk_handle_waitq() ?
> + 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",
You have dev here, use dev_err()
> + 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;
This is the last statement before the condition is evaluated again, so
this continue should be a nop.
> + } else {
The use of IS_WAITQ_SLEEP_OR_WAKE() hides the fact that a0 can only be
QCOM_SCM_WAITQ_WAKE here. I think you should make it explicit with:
} else if (res->a0 == QCOM_SCM_WAITQ_WAKE) {
> + fill_wq_wake_ack_args(smc, smc_call_ctx);
> + continue;
> + }
> + } else if (!res->a0 || (long)res->a0 < 0) {
> + /*
> + * Success, or error.
> + * 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;
The next thing that will happen is that the condition in the loop will
be evaluated and the loop will be broken. So you don't need this break.
It's also not entirely obvious that the first half of this loop will
acquire the wq and always loop, so that at some point the second part
will run to wake up the wq - right before leaving the loop.
I think it would be easier to reason about if you move this chunk below
the loop.
do {
__scm_smc_do_quirk();
if (IS_WAITQ_SLEEP_OR_WAKE()) {
...
}
while (IS_WAITQ_SLEEP_OR_WAKE());
if (wq)
scm_waitq_flag_handler(wq, flags);
return 0;
If you also make sure to only set "wq" in the QCOM_SCM_WAITQ_WAKE case
you will make it clearer that the if (!res->a0) is there to capture the
case where we acked the wakeup and the ack was successful.
One question though, if the wakeup ack fails, should we still wake up
the requested wq? Perhaps it doesn't matter? As the woken up wq would
just face a QCOM_SCM_WAITQ_SLEEP again?
> + }
> + } 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 +190,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 +200,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 +252,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;
This is not necessary, right?
Regards,
Bjorn
>
> + return ret;
> }
> --
> 2.7.4
>
prev parent reply other threads:[~2022-10-19 23:23 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-30 22:25 [RESEND PATCH v2 0/5] SCM: Add support for wait-queue aware firmware Guru Das Srinagesh
2022-08-30 22:25 ` [RESEND PATCH v2 1/5] dt-bindings: firmware: qcom-scm: Add "allow-multi-call" property Guru Das Srinagesh
2022-08-31 8:00 ` Krzysztof Kozlowski
2022-10-18 5:56 ` Sibi Sankar
2022-08-30 22:25 ` [RESEND PATCH v2 2/5] firmware: qcom: scm: Optionally remove SCM call serialization Guru Das Srinagesh
2022-08-31 8:06 ` Krzysztof Kozlowski
2022-08-30 22:25 ` [RESEND PATCH v2 3/5] dt-bindings: firmware: qcom-scm: Add optional interrupt Guru Das Srinagesh
2022-08-31 8:02 ` Krzysztof Kozlowski
2022-10-18 5:49 ` Sibi Sankar
2022-10-18 13:11 ` Krzysztof Kozlowski
2022-08-30 22:25 ` [RESEND PATCH v2 4/5] firmware: qcom: scm: Add wait-queue helper functions Guru Das Srinagesh
2022-08-31 0:32 ` kernel test robot
2022-08-31 6:30 ` kernel test robot
2022-10-19 22:52 ` Bjorn Andersson
2022-08-30 22:25 ` [RESEND PATCH v2 5/5] firmware: qcom: scm: Add wait-queue handling logic Guru Das Srinagesh
2022-10-19 23:23 ` Bjorn Andersson [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221019232300.pw4cpbzlvputury2@builder.lan \
--to=andersson@kernel.org \
--cc=agross@kernel.org \
--cc=david@ixit.cz \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=p.zabel@pengutronix.de \
--cc=quic_eberman@quicinc.com \
--cc=quic_gurus@quicinc.com \
--cc=quic_rjendra@quicinc.com \
--cc=robimarko@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).