[PATCH] firmware: qcom-scm: Support multiple waitq contexts

[PATCH] firmware: qcom-scm: Support multiple waitq contexts

[Guru Das Srinagesh]

Currently, only a single waitqueue context is supported (zero). 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
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,
-- 
Guru Das Srinagesh <quic_gurus@quicinc.com>

Re: [PATCH] firmware: qcom-scm: Support multiple waitq contexts

[Jeff Johnson]

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,

Re: [PATCH] firmware: qcom-scm: Support multiple waitq contexts

[Guru Das Srinagesh]

On Oct 27 2023 16:44, Jeff Johnson wrote:
> On 10/27/2023 3:37 PM, Guru Das Srinagesh wrote:
> > Currently, only a single waitqueue context is supported (zero). Firmware
> 
> which firmware?

It's the same combination of hypervisor + ARM Trustzone software mentioned in
the earlier commit 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling
logic").
> 
> > 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."

Will replace idr with xarray in next patchset.

Thank you.

Guru Das.