| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b7470966-55f6-fee7-0014-2aae3048612e@linaro.org>
Date: Tue, 10 Jan 2023 12:14:54 +0000
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Sibi Sankar <quic_sibis@...cinc.com>, andersson@...nel.org
Cc: agross@...nel.org, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
krzysztof.kozlowski+dt@...aro.org, robh+dt@...nel.org,
konrad.dybcio@...ainline.org, robimarko@...il.com,
quic_gurus@...cinc.com
Subject: Re: [PATCH V7 2/2] firmware: qcom: scm: Add wait-queue handling logic
Hi Sibi,
Few minor comments below,
On 10/01/2023 06:37, Sibi Sankar wrote:
> From: Guru Das Srinagesh <quic_gurus@...cinc.com>
>
> 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.
>
...
> 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
...
> +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;
why are we checking dev pointer in the middle of the call?
A comment here would really help readers.
> +
> + 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;
> +}
> +
...
> 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>
include <linux/completion.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)
> +{
> + /* assert wq_ctx is zero */ > + if (wq_ctx != 0) {
Is this correct? looks like zero is the only valid one.
I thought wq_ctx was a unique number (UID).
> + dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return &scm->waitq_comp;
> +}
> +
> +int qcom_scm_lookup_completion(u32 wq_ctx)
> +{
> + struct completion *wq = NULL;
> +
> + wq = qcom_scm_lookup_wq(__scm, wq_ctx);
> + if (IS_ERR(wq))
> + return PTR_ERR(wq);
> +
> + wait_for_completion(wq);
We can potentially block here forever without a timeout.
As you are reusing completion, I have not seen any reinitialization of
completion, this could potentially return above line without waiting at all.
> +
> + 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_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();
>
> /*
--srini
Powered by blists - more mailing lists