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

From: Bjorn Andersson
Date: Tue Jan 10 2023 - 13:11:09 EST

Next message: Dan Williams: "RE: [PATCH 1/2] x86/pci: Simplify is_mmconf_reserved() messages"
Previous message: Greg Kroah-Hartman: "[PATCH 6.0 000/148] 6.0.19-rc1 review"
In reply to: Sibi Sankar: "Re: [PATCH V7 2/2] firmware: qcom: scm: Add wait-queue handling logic"
Next in thread: Sibi Sankar: "Re: [PATCH V7 2/2] firmware: qcom: scm: Add wait-queue handling logic"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Jan 10, 2023 at 12:07:45PM +0530, Sibi Sankar wrote:
> From: Guru Das Srinagesh <quic_gurus@xxxxxxxxxxx>
>
> When the firmware (FW) supports multiple requests per VM, multiple requests
> from the same/different VM can reach the firmware at the same time. Since
> the firmware currently being used has limited resources, it guards them
> with a resource lock and puts requests on a wait-queue internally and
> signals to HLOS that it is doing so. It does this by returning a new return
> value in addition to success or error: SCM_WAITQ_SLEEP. A sleeping SCM call
> can be woken up by an interrupt that the FW raises.
>
> 1) SCM_WAITQ_SLEEP:
>
> When an SCM call receives this return value instead of success
> or error, FW has placed this call on a wait-queue and has signalled
> HLOS to put it to non-interruptible sleep.
>
> Along with this return value, FW also passes to HLOS `wq_ctx` -
> a unique number (UID) identifying the wait-queue that it has put
> the call on, internally. This is to help HLOS with its own
> bookkeeping to wake this sleeping call later.
>
> Additionally, FW also passes to HLOS `smc_call_ctx` - a UID
> identifying the SCM call thus being put to sleep. This is also
> for HLOS' bookkeeping to wake this call up later.
>
> These two additional values are passed via the a1 and a2
> registers.
>
> N.B.: The "ctx" in the above UID names = "context".
>
> The handshake mechanism that HLOS uses to talk to FW about wait-queue
> operations involves two new SMC calls.
>
> 1) get_wq_ctx():
>
> Arguments: None
> Returns: wq_ctx, flags, more_pending
>
> Get the wait-queue context, and wake up either one or all of the
> sleeping SCM calls associated with that wait-queue.
>
> Additionally, repeat this if there are more wait-queues that are
> ready to have their requests woken up (`more_pending`).
>
> 2) wq_resume(smc_call_ctx):
>
> Arguments: smc_call_ctx
>
> HLOS needs to issue this in response to receiving an
> IRQ, passing to FW the same smc_call_ctx that FW
> receives from HLOS via the get_wq_ctx() call.
>
> (The mechanism to wake a SMC call back up is described in detail below)
>
> 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 │
> │ │◄────────────────────────────────┤
> │ │ │
> │ │ │
>
> With the exception of get_wq_ctx(), the other SMC call wq_resume() can
> 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)
>
> Also add the interrupt handler that wakes up a sleeping SCM call.
>
> Signed-off-by: Guru Das Srinagesh <quic_gurus@xxxxxxxxxxx>
> Co-developed-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
> Signed-off-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
> ---
>
> v7:
> - Move lookup + wait_for_completion into a single function in qcom_scm [Bjorn]
> - Simplify completion retrieval [Bjorn]
>
> v6:
> - Fix misc. nits in the scm driver [Krzysztof]
>
> v5:
> - Handle the wake_one/wake_all flags [Guru]
> - Rename flag handler to qcom_scm_waitq_wakeup [Bjorn]
> - Resume scm call can return ebusy as well handle that scenario by retrying
> the original smc call and not the resume call
>
> v4:
> - platform_set_drvdata will be used by __scm_smc_do_quirk_handle_waitq to
> get access to scm struct from device so retain it
> - Use a single completion as it satisfies all of the current usecases [Bjorn]
> - Inline scm_get_wq_ctx [Bjorn]
> - Convert all pr_err to dev_err [Bjorn]
> - Handle idr_destroy in a thread safe manner [Bjorn]
> - Misc. Style fixes [Bjorn]
>
> v3:
> - Fixup irq handling so as not to affect SoCs without the interrupt.
> - Fix warnings reported by kernel test-bot.
>
> drivers/firmware/qcom_scm-smc.c | 90 ++++++++++++++++++++++++++++++---
> drivers/firmware/qcom_scm.c | 89 +++++++++++++++++++++++++++++++-
> drivers/firmware/qcom_scm.h | 8 +++
> 3 files changed, 179 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
> index d111833364ba..30999f04749c 100644
> --- a/drivers/firmware/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom_scm-smc.c
> @@ -52,29 +52,101 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
> } while (res->a0 == QCOM_SCM_INTERRUPTED);
> }
>
> -static void __scm_smc_do(const struct arm_smccc_args *smc,
> - struct arm_smccc_res *res, bool atomic)
> +static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
> {
> - int retry_count = 0;
> + memset(resume->args, 0, sizeof(resume->args[0]) * ARRAY_SIZE(resume->args));
> +
> + resume->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
> + ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
> + SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_RESUME));
> +
> + resume->args[1] = QCOM_SCM_ARGS(1);
> +
> + resume->args[2] = smc_call_ctx;
> +}
> +
> +int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending)
> +{
> + int ret;
> + struct arm_smccc_args get_wq_ctx = {0};
> + struct arm_smccc_res get_wq_res;
> +
> + get_wq_ctx.args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
> + ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
> + SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_WQ_CTX));
> +
> + /* Guaranteed to return only success or error, no WAITQ_* */
> + __scm_smc_do_quirk(&get_wq_ctx, &get_wq_res);
> + ret = get_wq_res.a0;
> + if (ret)
> + return ret;
> +
> + *wq_ctx = get_wq_res.a1;
> + *flags = get_wq_res.a2;
> + *more_pending = get_wq_res.a3;
> +
> + return 0;
> +}
> +
> +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq,
> + struct arm_smccc_res *res)
> +{
> + int ret;
> + struct arm_smccc_args resume;
> + u32 wq_ctx, smc_call_ctx, flags;
> + struct arm_smccc_args *smc = waitq;
> +
> + do {
> + __scm_smc_do_quirk(smc, res);
> +
> + if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
> + wq_ctx = res->a1;
> + smc_call_ctx = res->a2;
> + flags = res->a3;
> +
> + if (!dev)
> + return -EPROBE_DEFER;
> +
> + ret = qcom_scm_lookup_completion(wq_ctx);
> + if (ret)
> + return ret;
> +
> + fill_wq_resume_args(&resume, smc_call_ctx);
> + smc = &resume;
> + }
> + } while (res->a0 == QCOM_SCM_WAITQ_SLEEP);
> +
> + return 0;
> +}
> +
> +static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
> + struct arm_smccc_res *res, bool atomic)
> +{
> + int ret, retry_count = 0;
>
> if (atomic) {
> __scm_smc_do_quirk(smc, res);
> - return;
> + return 0;
> }
>
> do {
> mutex_lock(&qcom_scm_lock);
>
> - __scm_smc_do_quirk(smc, res);
> + ret = __scm_smc_do_quirk_handle_waitq(dev, smc, res);
>
> mutex_unlock(&qcom_scm_lock);
>
> + if (ret)
> + return ret;
> +
> if (res->a0 == QCOM_SCM_V2_EBUSY) {
> if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
> break;
> msleep(QCOM_SCM_EBUSY_WAIT_MS);
> }
> } while (res->a0 == QCOM_SCM_V2_EBUSY);
> +
> + return 0;
> }
>
>
> @@ -83,7 +155,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;
> @@ -135,13 +207,17 @@ 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 error check follows after args_virt cleanup*/
> + ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
>
> 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;
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index cdbfe54c8146..19ac506a9b1f 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -4,6 +4,7 @@
> */
> #include <linux/platform_device.h>
> #include <linux/init.h>
> +#include <linux/interrupt.h>
> #include <linux/cpumask.h>
> #include <linux/export.h>
> #include <linux/dma-mapping.h>
> @@ -13,6 +14,7 @@
> #include <linux/qcom_scm.h>
> #include <linux/of.h>
> #include <linux/of_address.h>
> +#include <linux/of_irq.h>
> #include <linux/of_platform.h>
> #include <linux/clk.h>
> #include <linux/reset-controller.h>
> @@ -33,6 +35,7 @@ struct qcom_scm {
> struct clk *iface_clk;
> struct clk *bus_clk;
> struct icc_path *path;
> + struct completion waitq_comp;
> struct reset_controller_dev reset;
>
> /* control access to the interconnect path */
> @@ -63,6 +66,9 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
> BIT(2), BIT(1), BIT(4), BIT(6)
> };
>
> +#define QCOM_SMC_WAITQ_FLAG_WAKE_ONE BIT(0)
> +#define QCOM_SMC_WAITQ_FLAG_WAKE_ALL BIT(1)
> +
> static const char * const qcom_scm_convention_names[] = {
> [SMC_CONVENTION_UNKNOWN] = "unknown",
> [SMC_CONVENTION_ARM_32] = "smc arm 32",
> @@ -1325,11 +1331,79 @@ bool qcom_scm_is_available(void)
> }
> EXPORT_SYMBOL(qcom_scm_is_available);
>
> +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)

This could either be seen as a remnant of the older versions of this
patch, or a signalling of "place implementation here". I dislike both...

Please rename it qcom_scm_assert_valid_wq_ctx() and just reference
&scm->waitq_comp directly in the two places below.

> +{
> + /* assert wq_ctx is zero */
> + if (wq_ctx != 0) {
> + dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx);

I think you should be more specific here, perhaps:

"Firmware unexpectedly passed non-zero wq_ctx\n"

And then as suggested a comment stating what needs to be done when that
happens.

> + return ERR_PTR(-EINVAL);
> + }
> +
> + return &scm->waitq_comp;
> +}
> +
> +int qcom_scm_lookup_completion(u32 wq_ctx)
> +{
> + struct completion *wq = NULL;

You assign it on the very next line, no need to do so here.

> +
> + wq = qcom_scm_lookup_wq(__scm, wq_ctx);
> + if (IS_ERR(wq))
> + return PTR_ERR(wq);
> +
> + wait_for_completion(wq);
> +
> + return 0;
> +}
> +
> +static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx, bool wake_all)
> +{
> + struct completion *wq_to_wake;

"wq" would be sufficient.

Thanks,
Bjorn

> +
> + wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
> + if (IS_ERR(wq_to_wake))
> + return PTR_ERR(wq_to_wake);
> +
> + if (wake_all)
> + complete_all(wq_to_wake);
> + else
> + complete(wq_to_wake);
> +
> + return 0;
> +}
> +
> +static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
> +{
> + int ret;
> + struct qcom_scm *scm = data;
> + u32 wq_ctx, flags, more_pending = 0;
> +
> + do {
> + ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
> + if (ret) {
> + dev_err(scm->dev, "GET_WQ_CTX SMC call failed: %d\n", ret);
> + goto out;
> + }
> +
> + if (flags != QCOM_SMC_WAITQ_FLAG_WAKE_ONE &&
> + flags != QCOM_SMC_WAITQ_FLAG_WAKE_ALL) {
> + dev_err(scm->dev, "Invalid flags found for wq_ctx: %u\n", flags);
> + goto out;
> + }
> +
> + ret = qcom_scm_waitq_wakeup(scm, wq_ctx, !!(flags & QCOM_SMC_WAITQ_FLAG_WAKE_ALL));
> + if (ret)
> + goto out;
> + } while (more_pending);
> +
> +out:
> + return IRQ_HANDLED;
> +}
> +
> static int qcom_scm_probe(struct platform_device *pdev)
> {
> struct qcom_scm *scm;
> unsigned long clks;
> - int ret;
> + int irq, ret;
>
> scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
> if (!scm)
> @@ -1402,6 +1476,19 @@ static int qcom_scm_probe(struct platform_device *pdev)
> __scm = scm;
> __scm->dev = &pdev->dev;
>
> + init_completion(&__scm->waitq_comp);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + if (irq != -ENXIO)
> + return irq;
> + } else {
> + ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
> + IRQF_ONESHOT, "qcom-scm", __scm);
> + if (ret < 0)
> + return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
> + }
> +
> __get_convention();
>
> /*
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index db3d08a01209..018e9867d55a 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -60,6 +60,9 @@ struct qcom_scm_res {
> u64 result[MAX_QCOM_SCM_RETS];
> };
>
> +int qcom_scm_lookup_completion(u32 wq_ctx);
> +int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending);
> +
> #define SCM_SMC_FNID(s, c) ((((s) & 0xFF) << 8) | ((c) & 0xFF))
> extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> enum qcom_scm_convention qcom_convention,
> @@ -129,6 +132,10 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
> #define QCOM_SCM_SMMU_CONFIG_ERRATA1 0x03
> #define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL 0x02
>
> +#define QCOM_SCM_SVC_WAITQ 0x24
> +#define QCOM_SCM_WAITQ_RESUME 0x02
> +#define QCOM_SCM_WAITQ_GET_WQ_CTX 0x03
> +
> /* common error codes */
> #define QCOM_SCM_V2_EBUSY -12
> #define QCOM_SCM_ENOMEM -5
> @@ -137,6 +144,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
> #define QCOM_SCM_EINVAL_ARG -2
> #define QCOM_SCM_ERROR -1
> #define QCOM_SCM_INTERRUPTED 1
> +#define QCOM_SCM_WAITQ_SLEEP 2
>
> static inline int qcom_scm_remap_error(int err)
> {
> --
> 2.17.1
>

Next message: Dan Williams: "RE: [PATCH 1/2] x86/pci: Simplify is_mmconf_reserved() messages"
Previous message: Greg Kroah-Hartman: "[PATCH 6.0 000/148] 6.0.19-rc1 review"
In reply to: Sibi Sankar: "Re: [PATCH V7 2/2] firmware: qcom: scm: Add wait-queue handling logic"
Next in thread: Sibi Sankar: "Re: [PATCH V7 2/2] firmware: qcom: scm: Add wait-queue handling logic"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]