Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <3f5f42e5-265f-40f1-af38-7057622270d2@quicinc.com>
Date:   Fri, 27 Oct 2023 16:44:53 -0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] firmware: qcom-scm: Support multiple waitq contexts
Content-Language: en-US
To:     Guru Das Srinagesh <quic_gurus@quicinc.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>
CC:     <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <kernel@quicinc.com>
References: <20231027-multi-wqs-v1-1-d47cd7f3590f@quicinc.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20231027-multi-wqs-v1-1-d47cd7f3590f@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/27/2023 3:37 PM, Guru Das Srinagesh wrote:
> Currently, only a single waitqueue context is supported (zero). Firmware

which firmware?

> now supports multiple waitqueue contexts, so add support to dynamically
> create and support as many unique waitqueue contexts as firmware returns
> to the driver.
> 
> This is done by using the idr framework to create a hash table for

why idr?
Per https://docs.kernel.org/core-api/idr.html
"The IDR interface is deprecated; please use the XArray instead."

> associating a unique wq_ctx with a struct completion variable for easy
> lookup.
> 
> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
> ---
> This series is based on the tip of linux-next because a couple of SCM
> driver-related patches are still in-flight in Bjorn's tree.
> ---
>   drivers/firmware/qcom/qcom_scm-smc.c |  7 ++-
>   drivers/firmware/qcom/qcom_scm.c     | 90 +++++++++++++++++++++++++++---------
>   drivers/firmware/qcom/qcom_scm.h     |  3 +-
>   3 files changed, 77 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> index 16cf88acfa8e..80083e3615b1 100644
> --- a/drivers/firmware/qcom/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> @@ -103,7 +103,12 @@ static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_
>   			wq_ctx = res->a1;
>   			smc_call_ctx = res->a2;
>   
> -			ret = qcom_scm_wait_for_wq_completion(wq_ctx);
> +			if (!dev) {
> +				/* Protect the dev_get_drvdata() call that follows */
> +				return -EPROBE_DEFER;
> +			}
> +
> +			ret = qcom_scm_wait_for_wq_completion(dev_get_drvdata(dev), wq_ctx);
>   			if (ret)
>   				return ret;
>   
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 520de9b5633a..70f0c35beb16 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -10,6 +10,7 @@
>   #include <linux/dma-mapping.h>
>   #include <linux/export.h>
>   #include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/idr.h>
>   #include <linux/init.h>
>   #include <linux/interconnect.h>
>   #include <linux/interrupt.h>
> @@ -27,13 +28,18 @@
>   static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
>   module_param(download_mode, bool, 0);
>   
> +struct qcom_scm_waitq {
> +	struct idr idr;
> +	spinlock_t idr_lock;
> +};
> +
>   struct qcom_scm {
>   	struct device *dev;
>   	struct clk *core_clk;
>   	struct clk *iface_clk;
>   	struct clk *bus_clk;
>   	struct icc_path *path;
> -	struct completion waitq_comp;
> +	struct qcom_scm_waitq waitq;
>   	struct reset_controller_dev reset;
>   
>   	/* control access to the interconnect path */
> @@ -1742,42 +1748,76 @@ bool qcom_scm_is_available(void)
>   }
>   EXPORT_SYMBOL_GPL(qcom_scm_is_available);
>   
> -static int qcom_scm_assert_valid_wq_ctx(u32 wq_ctx)
> +static struct completion *qcom_scm_get_completion(struct qcom_scm *scm, u32 wq_ctx)
>   {
> -	/* FW currently only supports a single wq_ctx (zero).
> -	 * TODO: Update this logic to include dynamic allocation and lookup of
> -	 * completion structs when FW supports more wq_ctx values.
> +	struct completion *wq;
> +	unsigned long flags;
> +	int err;
> +
> +	spin_lock_irqsave(&scm->waitq.idr_lock, flags);
> +	wq = idr_find(&scm->waitq.idr, wq_ctx);
> +	if (wq) {
> +		/*
> +		 * Valid struct completion *wq found corresponding to
> +		 * given wq_ctx. We're done here.
> +		 */
> +		goto out;
> +	}
> +
> +	/*
> +	 * If a struct completion *wq does not exist for wq_ctx, create it. FW
> +	 * only uses a finite number of wq_ctx values, so we will be reaching
> +	 * here only a few times right at the beginning of the device's uptime
> +	 * and then early-exit from idr_find() above subsequently.
>   	 */
> -	if (wq_ctx != 0) {
> -		dev_err(__scm->dev, "Firmware unexpectedly passed non-zero wq_ctx\n");
> -		return -EINVAL;
> +	wq = devm_kzalloc(scm->dev, sizeof(*wq), GFP_ATOMIC);
> +	if (!wq) {
> +		wq = ERR_PTR(-ENOMEM);
> +		goto out;
>   	}
>   
> -	return 0;
> +	init_completion(wq);
> +
> +	err = idr_alloc_u32(&scm->waitq.idr, wq, &wq_ctx, wq_ctx, GFP_ATOMIC);
> +	if (err < 0) {
> +		/* Don't wait for driver to be unloaded to free wq */
> +		devm_kfree(scm->dev, wq);
> +		wq = ERR_PTR(err);
> +	}
> +
> +out:
> +	spin_unlock_irqrestore(&scm->waitq.idr_lock, flags);
> +	return wq;
>   }
>   
> -int qcom_scm_wait_for_wq_completion(u32 wq_ctx)
> +int qcom_scm_wait_for_wq_completion(struct qcom_scm *scm, u32 wq_ctx)
>   {
> -	int ret;
> +	struct completion *wq;
>   
> -	ret = qcom_scm_assert_valid_wq_ctx(wq_ctx);
> -	if (ret)
> -		return ret;
> +	wq = qcom_scm_get_completion(scm, wq_ctx);
> +	if (IS_ERR(wq)) {
> +		pr_err("Unable to wait on invalid waitqueue for wq_ctx %d: %ld\n",
> +				wq_ctx, PTR_ERR(wq));
> +		return PTR_ERR(wq);
> +	}
>   
> -	wait_for_completion(&__scm->waitq_comp);
> +	wait_for_completion(wq);
>   
>   	return 0;
>   }
>   
>   static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx)
>   {
> -	int ret;
> +	struct completion *wq;
>   
> -	ret = qcom_scm_assert_valid_wq_ctx(wq_ctx);
> -	if (ret)
> -		return ret;
> +	wq = qcom_scm_get_completion(scm, wq_ctx);
> +	if (IS_ERR(wq)) {
> +		pr_err("Unable to wake up invalid waitqueue for wq_ctx %d: %ld\n",
> +				wq_ctx, PTR_ERR(wq));
> +		return PTR_ERR(wq);
> +	}
>   
> -	complete(&__scm->waitq_comp);
> +	complete(wq);
>   
>   	return 0;
>   }
> @@ -1854,10 +1894,13 @@ static int qcom_scm_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> +	platform_set_drvdata(pdev, scm);
> +
>   	__scm = scm;
>   	__scm->dev = &pdev->dev;
>   
> -	init_completion(&__scm->waitq_comp);
> +	spin_lock_init(&__scm->waitq.idr_lock);
> +	idr_init(&__scm->waitq.idr);
>   
>   	irq = platform_get_irq_optional(pdev, 0);
>   	if (irq < 0) {
> @@ -1905,8 +1948,13 @@ static int qcom_scm_probe(struct platform_device *pdev)
>   
>   static void qcom_scm_shutdown(struct platform_device *pdev)
>   {
> +	struct qcom_scm *scm;
> +
> +	scm = platform_get_drvdata(pdev);
> +
>   	/* Clean shutdown, disable download mode to allow normal restart */
>   	qcom_scm_set_download_mode(false);
> +	idr_destroy(&scm->waitq.idr);
>   }
>   
>   static const struct of_device_id qcom_scm_dt_match[] = {
> diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
> index 4532907e8489..d54df5a2b690 100644
> --- a/drivers/firmware/qcom/qcom_scm.h
> +++ b/drivers/firmware/qcom/qcom_scm.h
> @@ -62,7 +62,8 @@ struct qcom_scm_res {
>   	u64 result[MAX_QCOM_SCM_RETS];
>   };
>   
> -int qcom_scm_wait_for_wq_completion(u32 wq_ctx);
> +struct qcom_scm;
> +int qcom_scm_wait_for_wq_completion(struct qcom_scm *scm, 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))
> 
> ---
> base-commit: 2ef7141596eed0b4b45ef18b3626f428a6b0a822
> change-id: 20231027-multi-wqs-8b87fc0d3b53
> 
> Best regards,