[PATCH 0/3] SCM: Support latest version of waitq-aware firmware

[PATCH 0/3] SCM: Support latest version of waitq-aware firmware

[Unnathi Chalicheemala]

This series adds support for the latest improvements made in SCM
firmware (FW) that allow for multiple wait-queues in FW.

This series has been tested on SA8775P SoC.

P.S. While at Qualcomm, Guru Das Srinagesh authored these patches.
Thanks Guru!

Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
---
Unnathi Chalicheemala (3):
      firmware: qcom-scm: Initialize waitq before setting global __scm
      firmware: qcom-scm: Support multiple waitq contexts
      firmware: qcom-scm: Remove QCOM_SMC_WAITQ_FLAG_WAKE_ALL

 drivers/firmware/qcom/qcom_scm-smc.c |  7 ++-
 drivers/firmware/qcom/qcom_scm.c     | 85 +++++++++++++++++++++++++-----------
 drivers/firmware/qcom/qcom_scm.h     |  3 +-
 3 files changed, 67 insertions(+), 28 deletions(-)
---
base-commit: 39133352cbed6626956d38ed72012f49b0421e7b
change-id: 20240223-multi_waitq-4942468dc498

Best regards,
-- 
Unnathi Chalicheemala <quic_uchalich@quicinc.com>

[PATCH 1/3] firmware: qcom-scm: Initialize waitq before setting global __scm

[Unnathi Chalicheemala]

Suppose there's another device probing at the same time as SCM driver.
It calls into SCM and triggers WAITQ sleep but waitq completion
structure may not be initialized yet.
Fix this by moving initialization of waitq before setting global __scm.

Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic")
Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
---
 drivers/firmware/qcom/qcom_scm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 520de9b5633a..c1be8270ead1 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1854,11 +1854,11 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	init_completion(&scm->waitq_comp);
+
 	__scm = scm;
 	__scm->dev = &pdev->dev;
 
-	init_completion(&__scm->waitq_comp);
-
 	irq = platform_get_irq_optional(pdev, 0);
 	if (irq < 0) {
 		if (irq != -ENXIO)

-- 
2.25.1

Re: [PATCH 1/3] firmware: qcom-scm: Initialize waitq before setting global __scm

[Bartosz Golaszewski]

On Wed, Feb 28, 2024 at 7:50 PM Unnathi Chalicheemala
<quic_uchalich@quicinc.com> wrote:
>
> Suppose there's another device probing at the same time as SCM driver.
> It calls into SCM and triggers WAITQ sleep but waitq completion
> structure may not be initialized yet.
> Fix this by moving initialization of waitq before setting global __scm.
>
> Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic")
> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 520de9b5633a..c1be8270ead1 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -1854,11 +1854,11 @@ static int qcom_scm_probe(struct platform_device *pdev)
>         if (ret)
>                 return ret;
>
> +       init_completion(&scm->waitq_comp);
> +
>         __scm = scm;
>         __scm->dev = &pdev->dev;

If another driver can actually probe at the same time then this needs
to be moved to the end of this function, the `__scm->dev` assignment
must be done before the `__scm = scm` one and it must use
WRITE_ONCE().

Bart

>
> -       init_completion(&__scm->waitq_comp);
> -
>         irq = platform_get_irq_optional(pdev, 0);
>         if (irq < 0) {
>                 if (irq != -ENXIO)
>
> --
> 2.25.1
>
>

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

[Unnathi Chalicheemala]

Currently, only a single waitqueue context is supported, with waitqueue
id zero. SM8650 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.
Unique waitqueue contexts are supported using xarray to create a
hash table for associating a unique wq_ctx with a struct completion
variable for easy lookup.
The waitqueue ids can be >=0 as now we have more than one waitqueue
context.

Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
---
 drivers/firmware/qcom/qcom_scm-smc.c |  7 +++-
 drivers/firmware/qcom/qcom_scm.c     | 77 ++++++++++++++++++++++++++----------
 drivers/firmware/qcom/qcom_scm.h     |  3 +-
 3 files changed, 64 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 c1be8270ead1..4606c49ef155 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -21,6 +21,7 @@
 #include <linux/platform_device.h>
 #include <linux/reset-controller.h>
 #include <linux/types.h>
+#include <linux/xarray.h>
 
 #include "qcom_scm.h"
 
@@ -33,7 +34,7 @@ struct qcom_scm {
 	struct clk *iface_clk;
 	struct clk *bus_clk;
 	struct icc_path *path;
-	struct completion waitq_comp;
+	struct xarray waitq;
 	struct reset_controller_dev reset;
 
 	/* control access to the interconnect path */
@@ -1742,42 +1743,74 @@ 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;
+	struct completion *old;
+	int err;
+
+	wq = xa_load(&scm->waitq, 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 = kzalloc(sizeof(*wq), GFP_ATOMIC);
+	if (!wq) {
+		wq = ERR_PTR(-ENOMEM);
+		goto out;
 	}
 
-	return 0;
+	init_completion(wq);
+
+	old = xa_store(&scm->waitq, wq_ctx, wq, GFP_ATOMIC);
+	err = xa_err(old);
+	if (err) {
+		kfree(wq);
+		wq = ERR_PTR(err);
+	}
+
+out:
+	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,7 +1887,9 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	init_completion(&scm->waitq_comp);
+	platform_set_drvdata(pdev, scm);
+
+	xa_init(&scm->waitq);
 
 	__scm = scm;
 	__scm->dev = &pdev->dev;
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))

-- 
2.25.1

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

[Chris Lew]

On 2/28/2024 10:50 AM, Unnathi Chalicheemala wrote:
> Currently, only a single waitqueue context is supported, with waitqueue
> id zero. SM8650 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.
> Unique waitqueue contexts are supported using xarray to create a
> hash table for associating a unique wq_ctx with a struct completion
> variable for easy lookup.
> The waitqueue ids can be >=0 as now we have more than one waitqueue
> context.
> 
> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm-smc.c |  7 +++-
>  drivers/firmware/qcom/qcom_scm.c     | 77 ++++++++++++++++++++++++++----------
>  drivers/firmware/qcom/qcom_scm.h     |  3 +-
>  3 files changed, 64 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;
> +			}
> +

Do we need to do this !dev check within the do/while loop? Seems like it
could be done once at the start.

> +			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 c1be8270ead1..4606c49ef155 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -21,6 +21,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/reset-controller.h>
>  #include <linux/types.h>
> +#include <linux/xarray.h>
>  
>  #include "qcom_scm.h"
>  
> @@ -33,7 +34,7 @@ struct qcom_scm {
>  	struct clk *iface_clk;
>  	struct clk *bus_clk;
>  	struct icc_path *path;
> -	struct completion waitq_comp;
> +	struct xarray waitq;
>  	struct reset_controller_dev reset;
>  
>  	/* control access to the interconnect path */
> @@ -1742,42 +1743,74 @@ 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;
> +	struct completion *old;
> +	int err;
> +
> +	wq = xa_load(&scm->waitq, 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 = kzalloc(sizeof(*wq), GFP_ATOMIC);
> +	if (!wq) {
> +		wq = ERR_PTR(-ENOMEM);
> +		goto out;
>  	}
>  
> -	return 0;
> +	init_completion(wq);
> +
> +	old = xa_store(&scm->waitq, wq_ctx, wq, GFP_ATOMIC);
> +	err = xa_err(old);
> +	if (err) {
> +		kfree(wq);
> +		wq = ERR_PTR(err);
> +	}
> +

Any chance for this function to be called concurrently before there is a
valid wq stored in the xarray? If that were to happen we could have two
valid xa_stores happen on the same wq_ctx. One of the entries would be
returned as old and might be leaked depending on timing.

> +out:
> +	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,7 +1887,9 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	init_completion(&scm->waitq_comp);
> +	platform_set_drvdata(pdev, scm);
> +
> +	xa_init(&scm->waitq);
>  
>  	__scm = scm;
>  	__scm->dev = &pdev->dev;
> 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);

Is there a benefit to having qcom_scm passed in? I see we're adding scm
as drvdata in this patch, but we still have a single global __scm
pointer in qcom_scm.c. Are there going to be multiple instances of the
qcom_scm device?

Thanks,
Chris

>  int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending);
>  
>  #define SCM_SMC_FNID(s, c)	((((s) & 0xFF) << 8) | ((c) & 0xFF))
> 

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

[Unnathi Chalicheemala]

On 2/28/2024 3:20 PM, Chris Lew wrote:
> 
> 
> On 2/28/2024 10:50 AM, Unnathi Chalicheemala wrote:
>> Currently, only a single waitqueue context is supported, with waitqueue
>> id zero. SM8650 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.
>> Unique waitqueue contexts are supported using xarray to create a
>> hash table for associating a unique wq_ctx with a struct completion
>> variable for easy lookup.
>> The waitqueue ids can be >=0 as now we have more than one waitqueue
>> context.
>>
>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
>> ---
>>  drivers/firmware/qcom/qcom_scm-smc.c |  7 +++-
>>  drivers/firmware/qcom/qcom_scm.c     | 77 ++++++++++++++++++++++++++----------
>>  drivers/firmware/qcom/qcom_scm.h     |  3 +-
>>  3 files changed, 64 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;
>> +			}
>> +
> 
> Do we need to do this !dev check within the do/while loop? Seems like it
> could be done once at the start.
> 
Apologies for the late reply Chris.
Yes, will move this check out side the do {} while() loop.

>> +			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 c1be8270ead1..4606c49ef155 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -21,6 +21,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/reset-controller.h>
>>  #include <linux/types.h>
>> +#include <linux/xarray.h>
>>  
>>  #include "qcom_scm.h"
>>  
>> @@ -33,7 +34,7 @@ struct qcom_scm {
>>  	struct clk *iface_clk;
>>  	struct clk *bus_clk;
>>  	struct icc_path *path;
>> -	struct completion waitq_comp;
>> +	struct xarray waitq;
>>  	struct reset_controller_dev reset;
>>  
>>  	/* control access to the interconnect path */
>> @@ -1742,42 +1743,74 @@ 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;
>> +	struct completion *old;
>> +	int err;
>> +
>> +	wq = xa_load(&scm->waitq, 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 = kzalloc(sizeof(*wq), GFP_ATOMIC);
>> +	if (!wq) {
>> +		wq = ERR_PTR(-ENOMEM);
>> +		goto out;
>>  	}
>>  
>> -	return 0;
>> +	init_completion(wq);
>> +
>> +	old = xa_store(&scm->waitq, wq_ctx, wq, GFP_ATOMIC);
>> +	err = xa_err(old);
>> +	if (err) {
>> +		kfree(wq);
>> +		wq = ERR_PTR(err);
>> +	}
>> +
> 
> Any chance for this function to be called concurrently before there is a
> valid wq stored in the xarray? If that were to happen we could have two
> valid xa_stores happen on the same wq_ctx. One of the entries would be
> returned as old and might be leaked depending on timing.
> 

Thanks for pointing this out. Yes, this function is called every time a
qcom_smc_call() is made. But xarray documentation says that xa_store() uses
an internal xa_lock to synchronize accesses - I will look more into this.

To prevent rewriting entries at same wq_ctx, I think xa_store can be replaced
with xa_insert() - it'll prevent rewrite on existing entry.

>> +out:
>> +	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,7 +1887,9 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return ret;
>>  
>> -	init_completion(&scm->waitq_comp);
>> +	platform_set_drvdata(pdev, scm);
>> +
>> +	xa_init(&scm->waitq);
>>  
>>  	__scm = scm;
>>  	__scm->dev = &pdev->dev;
>> 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);
> 
> Is there a benefit to having qcom_scm passed in? I see we're adding scm
> as drvdata in this patch, but we still have a single global __scm
> pointer in qcom_scm.c. Are there going to be multiple instances of the
> qcom_scm device?
> 

I'll check and remove if possible.
Thanks a lot for the review Chris!

> Thanks,
> Chris
> 
>>  int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending);
>>  
>>  #define SCM_SMC_FNID(s, c)	((((s) & 0xFF) << 8) | ((c) & 0xFF))
>>

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

[Bjorn Andersson]

On Wed, Feb 28, 2024 at 10:50:00AM -0800, Unnathi Chalicheemala wrote:
> Currently, only a single waitqueue context is supported, with waitqueue
> id zero. SM8650 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.
> Unique waitqueue contexts are supported using xarray to create a
> hash table for associating a unique wq_ctx with a struct completion
> variable for easy lookup.
> The waitqueue ids can be >=0 as now we have more than one waitqueue
> context.
> 
> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm-smc.c |  7 +++-
>  drivers/firmware/qcom/qcom_scm.c     | 77 ++++++++++++++++++++++++++----------
>  drivers/firmware/qcom/qcom_scm.h     |  3 +-
>  3 files changed, 64 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;

No client driver will be prepared to handle an EPROBE_DEFER from here.

> +			}
> +
> +			ret = qcom_scm_wait_for_wq_completion(dev_get_drvdata(dev), wq_ctx);

I think it's fine to just continue to rely on __scm in the callee. If
you don't agree, please consider this a separate cleanup patch.

>  			if (ret)
>  				return ret;
>  
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index c1be8270ead1..4606c49ef155 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -21,6 +21,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/reset-controller.h>
>  #include <linux/types.h>
> +#include <linux/xarray.h>
>  
>  #include "qcom_scm.h"
>  
> @@ -33,7 +34,7 @@ struct qcom_scm {
>  	struct clk *iface_clk;
>  	struct clk *bus_clk;
>  	struct icc_path *path;
> -	struct completion waitq_comp;
> +	struct xarray waitq;
>  	struct reset_controller_dev reset;
>  
>  	/* control access to the interconnect path */
> @@ -1742,42 +1743,74 @@ 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;
> +	struct completion *old;
> +	int err;
> +
> +	wq = xa_load(&scm->waitq, wq_ctx);

What happens if qcom_scm_waitq_wakeup() is invoked concurrently with
qcom_scm_waitq_wakeup(), such that both find wq = NULL here?

> +	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

I'm confused, when I reviewed this previously I expressed that unless "a
few" is more than 73, or that the value space is sparse, the use of an
xarray only complicates the implementation.

Either describe why this wouldn't work, or use a statically sized array.

> +	 * 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 = kzalloc(sizeof(*wq), GFP_ATOMIC);
> +	if (!wq) {
> +		wq = ERR_PTR(-ENOMEM);
> +		goto out;
>  	}
>  
> -	return 0;
> +	init_completion(wq);
> +
> +	old = xa_store(&scm->waitq, wq_ctx, wq, GFP_ATOMIC);
> +	err = xa_err(old);
> +	if (err) {
> +		kfree(wq);
> +		wq = ERR_PTR(err);
> +	}
> +
> +out:
> +	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",

You don't attempt to wait until a few lines further down, so this error
message doesn't seem to reflect the actual problem that occurred. Seems
like this will only ever be ENOMEM in practice, in which case we
shouldn't print an error, right?

dev_err() seems suitable if this remains though.

> +						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",

As above.

Regards,
Bjorn

> +						wq_ctx, PTR_ERR(wq));
> +		return PTR_ERR(wq);
> +	}
>  
> -	complete(&__scm->waitq_comp);
> +	complete(wq);
>  
>  	return 0;
>  }
> @@ -1854,7 +1887,9 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	init_completion(&scm->waitq_comp);
> +	platform_set_drvdata(pdev, scm);
> +
> +	xa_init(&scm->waitq);
>  
>  	__scm = scm;
>  	__scm->dev = &pdev->dev;
> 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))
> 
> -- 
> 2.25.1
> 

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

[Unnathi Chalicheemala]

On 3/5/2024 2:28 PM, Bjorn Andersson wrote:
> On Wed, Feb 28, 2024 at 10:50:00AM -0800, Unnathi Chalicheemala wrote:
>> Currently, only a single waitqueue context is supported, with waitqueue
>> id zero. SM8650 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.
>> Unique waitqueue contexts are supported using xarray to create a
>> hash table for associating a unique wq_ctx with a struct completion
>> variable for easy lookup.
>> The waitqueue ids can be >=0 as now we have more than one waitqueue
>> context.
>>
>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
>> ---
>>  drivers/firmware/qcom/qcom_scm-smc.c |  7 +++-
>>  drivers/firmware/qcom/qcom_scm.c     | 77 ++++++++++++++++++++++++++----------
>>  drivers/firmware/qcom/qcom_scm.h     |  3 +-
>>  3 files changed, 64 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;
> 
> No client driver will be prepared to handle an EPROBE_DEFER from here.
> 
>> +			}
>> +
>> +			ret = qcom_scm_wait_for_wq_completion(dev_get_drvdata(dev), wq_ctx);
> 
> I think it's fine to just continue to rely on __scm in the callee. If
> you don't agree, please consider this a separate cleanup patch.
> 

Yes, sure. I will remove the !dev check. 

>>  			if (ret)
>>  				return ret;
>>  
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index c1be8270ead1..4606c49ef155 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>>  
>> -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;
>> +	struct completion *old;
>> +	int err;
>> +
>> +	wq = xa_load(&scm->waitq, wq_ctx);
> 
> What happens if qcom_scm_waitq_wakeup() is invoked concurrently with
> qcom_scm_waitq_wakeup(), such that both find wq = NULL here?
> 

Ack. Thanks, I didn't check this race condition. I will define a mutex to check for
wq=NULL before creating one.

>> +	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
> 
> I'm confused, when I reviewed this previously I expressed that unless "a
> few" is more than 73, or that the value space is sparse, the use of an
> xarray only complicates the implementation.
> 
> Either describe why this wouldn't work, or use a statically sized array.
> 

Sorry I missed this comment. I'll check and change this to a static array
in the next version.

>> +	 * 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 = kzalloc(sizeof(*wq), GFP_ATOMIC);
>> +	if (!wq) {
>> +		wq = ERR_PTR(-ENOMEM);
>> +		goto out;
>>  	}
>>  
>> -	return 0;
>> +	init_completion(wq);
>> +
>> +	old = xa_store(&scm->waitq, wq_ctx, wq, GFP_ATOMIC);
>> +	err = xa_err(old);
>> +	if (err) {
>> +		kfree(wq);
>> +		wq = ERR_PTR(err);
>> +	}
>> +
>> +out:
>> +	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",
> 
> You don't attempt to wait until a few lines further down, so this error
> message doesn't seem to reflect the actual problem that occurred. Seems
> like this will only ever be ENOMEM in practice, in which case we
> shouldn't print an error, right?
> 
> dev_err() seems suitable if this remains though.
> 

Ack. You're right; will use dev_err().

>> +						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",
> 
> As above.
> 

Ack. Thanks a lot for the review Bjorn!

> Regards,
> Bjorn
> 
>> +						wq_ctx, PTR_ERR(wq));
>> +		return PTR_ERR(wq);
>> +	}
>>  
>> -	complete(&__scm->waitq_comp);
>> +	complete(wq);
>>  
>>  	return 0;
>>  }
>> @@ -1854,7 +1887,9 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return ret;
>>  
>> -	init_completion(&scm->waitq_comp);
>> +	platform_set_drvdata(pdev, scm);
>> +
>> +	xa_init(&scm->waitq);
>>  
>>  	__scm = scm;
>>  	__scm->dev = &pdev->dev;
>> 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))
>>
>> -- 
>> 2.25.1
>>

[PATCH 3/3] firmware: qcom-scm: Remove QCOM_SMC_WAITQ_FLAG_WAKE_ALL

[Unnathi Chalicheemala]

This will not be supported by current firmware due to firmware
limitations, so remove it.

Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
---
 drivers/firmware/qcom/qcom_scm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 4606c49ef155..2657372f210f 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -113,7 +113,6 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
 };
 
 #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",
@@ -1828,9 +1827,8 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
 			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);
+		if (flags != QCOM_SMC_WAITQ_FLAG_WAKE_ONE) {
+			dev_err(scm->dev, "Invalid flags received for wq_ctx: %u\n", flags);
 			goto out;
 		}
 

-- 
2.25.1

Re: [PATCH 3/3] firmware: qcom-scm: Remove QCOM_SMC_WAITQ_FLAG_WAKE_ALL

[Konrad Dybcio]

On 2/28/24 19:50, Unnathi Chalicheemala wrote:
> This will not be supported by current firmware due to firmware
> limitations, so remove it.

"will not be" - but is it today? Has it ever been? Will it ever be?

Remember, you're changing code that needs to keep working on all
platforms from APQ8064 to X1E80100.. If that's only a change on
SM8650 or so, this isn't a valid argument.

Konrad