Date:   Wed, 19 Oct 2022 17:52:49 -0500
From:   Bjorn Andersson <andersson@...nel.org>
To:     Guru Das Srinagesh <quic_gurus@...cinc.com>
Cc:     Andy Gross <agross@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        David Heidelberg <david@...t.cz>,
        Robert Marko <robimarko@...il.com>,
        Rajendra Nayak <quic_rjendra@...cinc.com>,
        Elliot Berman <quic_eberman@...cinc.com>
Subject: Re: [RESEND PATCH v2 4/5] firmware: qcom: scm: Add wait-queue helper
 functions

On Tue, Aug 30, 2022 at 03:25:10PM -0700, Guru Das Srinagesh wrote:
> When the firmware (FW) supports multiple requests per VM, and the VM
> also supports it via the `allow-multi-call` device tree flag, the
> floodgates are thrown open for them to all 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 two
> new return values in addition to success or error: SCM_WAITQ_SLEEP and
> SCM_WAITQ_WAKE.
> 
>   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. (The
> 	mechanism to wake it back up will be described in detail in the
> 	next patch for the sake of simplicity.)
> 
> 	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".
> 
>   2) SCM_WAITQ_WAKE:
> 
>   	When an SCM call receives this return value instead of success
>   	or error, FW wishes to signal HLOS to wake up a (different)
>   	previously sleeping call.
> 
>   	FW tells HLOS which call to wake up via the additional return
>   	values `wq_ctx`, `smc_call_ctx` and `flags`. The first two have
>   	already been explained above.
> 
>   	`flags` can be either WAKE_ONE or WAKE_ALL. Meaning, wake either
>   	one, or all, of the SCM calls that HLOS is associating with the
>   	given `wq_ctx`.
> 
> A sleeping SCM call can be woken up by either an interrupt that FW
> raises, or via a SCM_WAITQ_WAKE return value for a new SCM call.
> 
> The handshake mechanism that HLOS uses to talk to FW about wait-queue
> operations involves three new SMC calls. These are:
> 
>   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.
> 
>   3) wq_wake_ack(smc_call_ctx):
> 
>   	Arguments:	smc_call_ctx
> 
>   	HLOS needs to issue this in response to receiving an
>   	SCM_WAITQ_WAKE, passing to FW the same smc_call_ctx that FW
>   	passed to HLOS via the SMC_WAITQ_WAKE call.
> 
> (Reminder that the full handshake mechanism will be detailed in the
> subsequent patch.)
> 
> Also add the interrupt handler that wakes up a sleeping SCM call.
> 
> Signed-off-by: Guru Das Srinagesh <quic_gurus@...cinc.com>
> ---
>  drivers/firmware/qcom_scm-smc.c |  56 +++++++++++++++++++
>  drivers/firmware/qcom_scm.c     | 119 +++++++++++++++++++++++++++++++++++++++-
>  drivers/firmware/qcom_scm.h     |  12 ++++
>  3 files changed, 186 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
> index 66193c2..4150da1 100644
> --- a/drivers/firmware/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom_scm-smc.c
> @@ -53,6 +53,62 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
>  	} while (res->a0 == QCOM_SCM_INTERRUPTED);
>  }
>  
> +static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)

These two functions are used in the next patch only, so please move them
there.

> +{
> +	memset(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;
> +}
> +
> +static void fill_wq_wake_ack_args(struct arm_smccc_args *wake_ack, u32 smc_call_ctx)
> +{
> +	memset(wake_ack->args, 0, ARRAY_SIZE(wake_ack->args));
> +
> +	wake_ack->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_ACK));
> +
> +	wake_ack->args[1] = QCOM_SCM_ARGS(1);
> +
> +	wake_ack->args[2] = smc_call_ctx;
> +}
> +
> +static void fill_get_wq_ctx_args(struct arm_smccc_args *get_wq_ctx)

Afaict, this is only called once, right below. So please inline this
snippet in scm_get_wq_ctx()

> +{
> +	memset(get_wq_ctx->args, 0, ARRAY_SIZE(get_wq_ctx->args));
> +
> +	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));
> +}
> +
> +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;
> +
> +	fill_get_wq_ctx_args(&get_wq_ctx);
> +
> +	__scm_smc_do_quirk(&get_wq_ctx, &get_wq_res);
> +	/* Guaranteed to return only success or error, no WAITQ_* */
> +	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 void __scm_smc_do(const struct arm_smccc_args *smc,
>  			 struct arm_smccc_res *res, bool atomic)
>  {
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 978706a..9ae3fcf 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -3,8 +3,12 @@
>   * Copyright (C) 2015 Linaro Ltd.
>   * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
> +#define pr_fmt(fmt)     "qcom-scm: %s: " fmt, __func__

No thanks, please use dev_err().

> +
>  #include <linux/platform_device.h>
> +#include <linux/idr.h>
>  #include <linux/init.h>
> +#include <linux/interrupt.h>
>  #include <linux/cpumask.h>
>  #include <linux/export.h>
>  #include <linux/dma-mapping.h>
> @@ -13,10 +17,13 @@
>  #include <linux/types.h>
>  #include <linux/qcom_scm.h>
>  #include <linux/of.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_address.h>
>  #include <linux/of_platform.h>
>  #include <linux/clk.h>
>  #include <linux/reset-controller.h>
> +#include <linux/spinlock.h>
> +#include <linux/ktime.h>
>  #include <linux/arm-smccc.h>
>  
>  #include "qcom_scm.h"
> @@ -30,6 +37,12 @@ bool qcom_scm_allow_multicall = false;
>  #define SCM_HAS_IFACE_CLK	BIT(1)
>  #define SCM_HAS_BUS_CLK		BIT(2)
>  
> +struct qcom_scm_waitq {
> +	struct idr idr;
> +	spinlock_t idr_lock;
> +	struct work_struct scm_irq_work;
> +};
> +
>  struct qcom_scm {
>  	struct device *dev;
>  	struct clk *core_clk;
> @@ -37,6 +50,7 @@ struct qcom_scm {
>  	struct clk *bus_clk;
>  	struct icc_path *path;
>  	struct reset_controller_dev reset;
> +	struct qcom_scm_waitq waitq;
>  
>  	/* control access to the interconnect path */
>  	struct mutex scm_bw_lock;
> @@ -62,10 +76,14 @@ struct qcom_scm_mem_map_info {
>  static const u8 qcom_scm_cpu_cold_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
>  	0, BIT(0), BIT(3), BIT(5)
>  };
> +

This seems unrelated.

>  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",
> @@ -1328,11 +1346,92 @@ bool qcom_scm_is_available(void)
>  }
>  EXPORT_SYMBOL(qcom_scm_is_available);
>  
> +struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)
> +{
> +	struct completion *wq = NULL;
> +	u32 wq_ctx_idr = wq_ctx;
> +	unsigned long flags;
> +	int err;
> +
> +	spin_lock_irqsave(&scm->waitq.idr_lock, flags);
> +	wq = idr_find(&scm->waitq.idr, wq_ctx);
> +	if (wq)
> +		goto out;
> +
> +	wq = devm_kzalloc(scm->dev, sizeof(*wq), GFP_ATOMIC);

I don't know how the wq_ctx are picked by the secure size, but as
written here there's no guarantee that this won't consume all system
memory after some time.


Is the worst-case number of requests waiting really bad enough that we
need an advanced data structure here?

How about just having a linked list with all currently waiting processes
and then loop over that in scm_waitq_flag_handler() to wake them up
selectively?

> +	if (!wq) {
> +		wq = ERR_PTR(-ENOMEM);
> +		goto out;
> +	}
> +
> +	init_completion(wq);
> +
> +	err = idr_alloc_u32(&scm->waitq.idr, wq, &wq_ctx_idr,
> +			    U32_MAX, GFP_ATOMIC);
> +	if (err < 0) {
> +		devm_kfree(scm->dev, wq);
> +		wq = ERR_PTR(err);
> +	}
> +
> +out:
> +	spin_unlock_irqrestore(&scm->waitq.idr_lock, flags);
> +	return wq;
> +}
> +
> +void scm_waitq_flag_handler(struct completion *wq, u32 flags)

This is indeed handling the waitq_flag, but I find that it would be
cleaner if this was:

void qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx, bool wake_all)

> +{
> +	switch (flags) {
> +	case QCOM_SMC_WAITQ_FLAG_WAKE_ONE:
> +		complete(wq);
> +		break;
> +	case QCOM_SMC_WAITQ_FLAG_WAKE_ALL:
> +		complete_all(wq);
> +		break;
> +	default:
> +		pr_err("invalid flags: %u\n", flags);
> +	}
> +}
> +
> +static void scm_irq_work(struct work_struct *work)
> +{
> +	int ret;
> +	u32 wq_ctx, flags, more_pending = 0;
> +	struct completion *wq_to_wake;
> +	struct qcom_scm_waitq *w = container_of(work, struct qcom_scm_waitq, scm_irq_work);
> +	struct qcom_scm *scm = container_of(w, struct qcom_scm, waitq);
> +
> +	do {
> +		ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
> +		if (ret) {
> +			pr_err("GET_WQ_CTX SMC call failed: %d\n", ret);

You have scm->dev, so use dev_err().

> +			return;
> +		}
> +
> +		wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
> +		if (IS_ERR_OR_NULL(wq_to_wake)) {
> +			pr_err("No waitqueue found for wq_ctx %d: %ld\n",
> +					wq_ctx, PTR_ERR(wq_to_wake));
> +			return;
> +		}
> +
> +		scm_waitq_flag_handler(wq_to_wake, flags);
> +	} while (more_pending);
> +}
> +
> +static irqreturn_t qcom_scm_irq_handler(int irq, void *p)
> +{
> +	struct qcom_scm *scm = p;
> +
> +	schedule_work(&scm->waitq.scm_irq_work);

All the worker does is to query and wake up sleeping processes, why
can't this be done here in the threaded irq handler directly?

> +
> +	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,12 +1501,29 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	platform_set_drvdata(pdev, scm);

I'm not able to figure out why this is needed, please drop if possible.

> +
>  	__scm = scm;
>  	__scm->dev = &pdev->dev;
>  
> +	spin_lock_init(&__scm->waitq.idr_lock);
> +	idr_init(&__scm->waitq.idr);
>  	qcom_scm_allow_multicall = of_property_read_bool(__scm->dev->of_node,
>  							"allow-multi-call");
>  
> +	INIT_WORK(&__scm->waitq.scm_irq_work, scm_irq_work);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq) {
> +		ret = devm_request_threaded_irq(__scm->dev, irq, NULL,
> +			qcom_scm_irq_handler, IRQF_ONESHOT, "qcom-scm", __scm);
> +		if (ret < 0) {
> +			pr_err("Failed to request qcom-scm irq: %d\n", ret);

You have scm->dev, please use it in dev_err().

> +			idr_destroy(&__scm->waitq.idr);
> +			return ret;
> +		}
> +	}
> +
>  	__get_convention();
>  
>  	/*
> @@ -1423,6 +1539,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  
>  static void qcom_scm_shutdown(struct platform_device *pdev)
>  {
> +	idr_destroy(&__scm->waitq.idr);

You can still have interrupts coming in and work scheduled, that will
use the idr. You need to ensure that can't happen.

Regards,
Bjorn

>  	/* Clean shutdown, disable download mode to allow normal restart */
>  	if (download_mode)
>  		qcom_scm_set_download_mode(false);
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index c0a4d6b..ae3a331 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -62,6 +62,11 @@ struct qcom_scm_res {
>  	u64 result[MAX_QCOM_SCM_RETS];
>  };
>  
> +struct qcom_scm;
> +extern struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx);
> +extern void scm_waitq_flag_handler(struct completion *wq, u32 flags);
> +extern 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,
> @@ -131,6 +136,11 @@ 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_ACK			0x01
> +#define QCOM_SCM_WAITQ_RESUME			0x02
> +#define QCOM_SCM_WAITQ_GET_WQ_CTX		0x03
> +
>  extern void __qcom_scm_init(void);
>  
>  /* common error codes */
> @@ -141,6 +151,8 @@ extern void __qcom_scm_init(void);
>  #define QCOM_SCM_EINVAL_ARG	-2
>  #define QCOM_SCM_ERROR		-1
>  #define QCOM_SCM_INTERRUPTED	1
> +#define QCOM_SCM_WAITQ_SLEEP	2
> +#define QCOM_SCM_WAITQ_WAKE	3
>  
>  static inline int qcom_scm_remap_error(int err)
>  {
> -- 
> 2.7.4
> 

Powered by blists - more mailing lists