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

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

[Unnathi Chalicheemala]

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

To support multi VM synchronization when VMs make SMC calls on same CPU,
waitqueue mechanism is added in firmware which runs at EL2 & EL3 exception
levels.

The macros defined in the first patch are only used in SCM driver and will
be dropped once Gunyah hypervisor driver patches are merged as fill_irq_fwspec_params()
is reused from here:
https://lore.kernel.org/all/20240405081735319-0700.eberman@hu-eberman-lv.qualcomm.com/

P.S. While at Qualcomm, Guru Das Srinagesh authored the initial version of these patches.
Thanks Guru!

---
Changes in v2:
- Dropped "Initialize waitq before setting global __scm" as it was merged here:
https://lore.kernel.org/r/1711034642-22860-4-git-send-email-quic_mojha@quicinc.com
- Decoupled "Remove QCOM_SMC_WAITQ_FLAG_WAKE_ALL" from series
- Converted xarray to a statically sized array
- Initialize waitq array in probe function
- Remove reinit of waitq completion struct in scm_get_completion()
- Introduced new APIs to get no. of waitqueue contexts and waitqueue IRQ no.
directly from firmware.

- Link to v1: https://lore.kernel.org/all/20240228-multi_waitq-v1-0-ccb096419af0@quicinc.com/

Unnathi Chalicheemala (2):
  firmware: qcom_scm: Add API to get waitqueue IRQ info
  firmware: qcom_scm: Support multiple waitq contexts

 drivers/firmware/qcom/qcom_scm.c | 135 ++++++++++++++++++++++++++-----
 drivers/firmware/qcom/qcom_scm.h |   1 +
 2 files changed, 116 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] firmware: qcom_scm: Add API to get waitqueue IRQ info

[Unnathi Chalicheemala]

Bootloader and firmware for SM8650 and older chipsets expect node
name as "qcom_scm". However, DeviceTree uses node name "scm" and this
mismatch prevents firmware from correctly identifying waitqueue IRQ
information. Waitqueue IRQ is used for signaling between secure and
non-secure worlds.

To resolve this, introduce qcom_scm_get_waitq_irq() that'll get the
hardware irq number to be used from firmware instead of relying on data
provided by devicetree, thereby bypassing the DeviceTree node name
mismatch.

This hardware irq number is converted to a linux irq number using newly
defined fill_irq_fwspec_params(). This linux irq number is then supplied to
the threaded_irq call.

Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
---
 drivers/firmware/qcom/qcom_scm.c | 59 +++++++++++++++++++++++++++++++-
 drivers/firmware/qcom/qcom_scm.h |  1 +
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 00c379a3cceb..ed51fbb1c065 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -32,6 +32,14 @@
 #include "qcom_scm.h"
 #include "qcom_tzmem.h"
 
+#define GIC_SPI_BASE            32
+#define GIC_MAX_SPI             987  // 1019 - 32
+#define GIC_ESPI_BASE           4096
+#define GIC_MAX_ESPI            1024 // 5120 - 4096
+
+#define GIC_IRQ_TYPE_SPI        0
+#define GIC_IRQ_TYPE_ESPI       2
+
 static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
 module_param(download_mode, bool, 0);
 
@@ -1819,6 +1827,55 @@ bool qcom_scm_is_available(void)
 }
 EXPORT_SYMBOL_GPL(qcom_scm_is_available);
 
+static int qcom_scm_fill_irq_fwspec_params(struct irq_fwspec *fwspec, u32 virq)
+{
+	if (WARN(virq < GIC_SPI_BASE, "Unexpected virq: %d\n", virq)) {
+		return -ENXIO;
+	} else if (virq <= (GIC_SPI_BASE + GIC_MAX_SPI)) {
+		fwspec->param_count = 3;
+		fwspec->param[0] = GIC_IRQ_TYPE_SPI;
+		fwspec->param[1] = virq - GIC_SPI_BASE;
+		fwspec->param[2] = IRQ_TYPE_EDGE_RISING;
+	} else if (WARN(virq < GIC_ESPI_BASE, "Unexpected virq: %d\n", virq)) {
+		return -ENXIO;
+	} else if (virq < (GIC_ESPI_BASE + GIC_MAX_ESPI)) {
+		fwspec->param_count = 3;
+		fwspec->param[0] = GIC_IRQ_TYPE_ESPI;
+		fwspec->param[1] = virq - GIC_ESPI_BASE;
+		fwspec->param[2] = IRQ_TYPE_EDGE_RISING;
+	} else {
+		WARN(1, "Unexpected virq: %d\n", virq);
+		return -ENXIO;
+	}
+	return 0;
+}
+
+static int qcom_scm_get_waitq_irq(void)
+{
+	int ret;
+	u32 hwirq;
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_WAITQ,
+		.cmd = QCOM_SCM_WAITQ_GET_INFO,
+		.owner = ARM_SMCCC_OWNER_SIP
+	};
+	struct qcom_scm_res res;
+	struct irq_fwspec fwspec;
+
+	ret = qcom_scm_call_atomic(__scm->dev, &desc, &res);
+	if (ret)
+		return ret;
+
+	fwspec.fwnode = of_node_to_fwnode(__scm->dev->of_node);
+	hwirq = res.result[1] & 0xffff;
+	ret = qcom_scm_fill_irq_fwspec_params(&fwspec, hwirq);
+	if (ret)
+		return ret;
+	ret = irq_create_fwspec_mapping(&fwspec);
+
+	return ret;
+}
+
 static int qcom_scm_assert_valid_wq_ctx(u32 wq_ctx)
 {
 	/* FW currently only supports a single wq_ctx (zero).
@@ -1936,7 +1993,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	/* Let all above stores be available after this */
 	smp_store_release(&__scm, scm);
 
-	irq = platform_get_irq_optional(pdev, 0);
+	irq = qcom_scm_get_waitq_irq();
 	if (irq < 0) {
 		if (irq != -ENXIO)
 			return irq;
diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
index 685b8f59e7a6..ab0f88f5f777 100644
--- a/drivers/firmware/qcom/qcom_scm.h
+++ b/drivers/firmware/qcom/qcom_scm.h
@@ -143,6 +143,7 @@ struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void);
 #define QCOM_SCM_SVC_WAITQ			0x24
 #define QCOM_SCM_WAITQ_RESUME			0x02
 #define QCOM_SCM_WAITQ_GET_WQ_CTX		0x03
+#define QCOM_SCM_WAITQ_GET_INFO		0x04
 
 #define QCOM_SCM_SVC_GPU			0x28
 #define QCOM_SCM_SVC_GPU_INIT_REGS		0x01
-- 
2.34.1

[PATCH v2 2/2] firmware: qcom_scm: Support multiple waitq contexts

[Unnathi Chalicheemala]

Currently, only a single waitqueue context exists, with waitqueue id zero.
Multi-waitqueue mechanism is added in firmware to support the case when
multiple VMs make SMC calls or single VM making multiple calls on same CPU.

When VMs make SMC call, firmware will allocate waitqueue context assuming
the SMC call to be a blocking call. SMC calls that cannot acquire resources
are returned to sleep in the calling VM. When resource is available, VM
will be notified to wake sleeping thread and resume SMC call.
SM8650 firmware can allocate two such waitq contexts so create these two
waitqueue contexts.

Unique waitqueue contexts are supported by a dynamically sized array where
each unique wq_ctx is associated with a struct completion variable for easy
lookup. To get the number of waitqueue contexts directly from firmware,
qcom_scm_query_waitq_cnt() is introduced. On older targets which support
only a single waitqueue, wq_cnt is set to 1 as SCM call for
query_waitq_cnt() is not implemented for single waitqueue case.

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

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index ed51fbb1c065..b2c5505de681 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -44,12 +44,13 @@ static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
 module_param(download_mode, bool, 0);
 
 struct qcom_scm {
+	int wq_cnt;
 	struct device *dev;
 	struct clk *core_clk;
 	struct clk *iface_clk;
 	struct clk *bus_clk;
 	struct icc_path *path;
-	struct completion waitq_comp;
+	struct completion *waitq;
 	struct reset_controller_dev reset;
 
 	/* control access to the interconnect path */
@@ -1850,6 +1851,31 @@ static int qcom_scm_fill_irq_fwspec_params(struct irq_fwspec *fwspec, u32 virq)
 	return 0;
 }
 
+static int qcom_scm_query_waitq_count(void)
+{
+	bool avail;
+	int count;
+	int ret;
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_WAITQ,
+		.cmd = QCOM_SCM_WAITQ_GET_INFO,
+		.owner = ARM_SMCCC_OWNER_SIP
+	};
+	struct qcom_scm_res res;
+
+	avail = __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_INFO);
+	if (!avail) {
+		count = 1;
+		return count;
+	}
+
+	ret = qcom_scm_call_atomic(__scm->dev, &desc, &res);
+	if (ret)
+		return ret;
+	count = res.result[0] & 0xff;
+	return count;
+}
+
 static int qcom_scm_get_waitq_irq(void)
 {
 	int ret;
@@ -1876,42 +1902,40 @@ static int qcom_scm_get_waitq_irq(void)
 	return ret;
 }
 
-static int qcom_scm_assert_valid_wq_ctx(u32 wq_ctx)
+static struct completion *qcom_scm_get_completion(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.
-	 */
-	if (wq_ctx != 0) {
-		dev_err(__scm->dev, "Firmware unexpectedly passed non-zero wq_ctx\n");
-		return -EINVAL;
-	}
+	struct completion *wq;
 
-	return 0;
+	if (wq_ctx >= __scm->wq_cnt)
+		return ERR_PTR(-EINVAL);
+
+	wq = &__scm->waitq[wq_ctx];
+
+	return wq;
 }
 
 int qcom_scm_wait_for_wq_completion(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(wq_ctx);
+	if (IS_ERR(wq))
+		return PTR_ERR(wq);
 
-	wait_for_completion(&__scm->waitq_comp);
+	wait_for_completion(wq);
 
 	return 0;
 }
 
 static int qcom_scm_waitq_wakeup(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(wq_ctx);
+	if (IS_ERR(wq))
+		return PTR_ERR(wq);
 
-	complete(&__scm->waitq_comp);
+	complete(wq);
 
 	return 0;
 }
@@ -1948,6 +1972,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	struct qcom_tzmem_pool_config pool_config;
 	struct qcom_scm *scm;
 	int irq, ret;
+	int i;
 
 	scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
 	if (!scm)
@@ -1958,7 +1983,6 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	init_completion(&scm->waitq_comp);
 	mutex_init(&scm->scm_bw_lock);
 
 	scm->path = devm_of_icc_get(&pdev->dev, NULL);
@@ -1993,6 +2017,20 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	/* Let all above stores be available after this */
 	smp_store_release(&__scm, scm);
 
+	platform_set_drvdata(pdev, scm);
+	ret = qcom_scm_query_waitq_count();
+	if (ret < 0)
+		return ret;
+
+	scm->wq_cnt = ret;
+
+	scm->waitq = devm_kcalloc(&pdev->dev, scm->wq_cnt, sizeof(*scm->waitq), GFP_KERNEL);
+	if (!scm->waitq)
+		return -ENOMEM;
+
+	for (i = 0; i < scm->wq_cnt; i++)
+		init_completion(&scm->waitq[i]);
+
 	irq = qcom_scm_get_waitq_irq();
 	if (irq < 0) {
 		if (irq != -ENXIO)
-- 
2.34.1

Re: [PATCH v2 1/2] firmware: qcom_scm: Add API to get waitqueue IRQ info

[Konrad Dybcio]

On 30.08.2024 12:15 AM, Unnathi Chalicheemala wrote:
> Bootloader and firmware for SM8650 and older chipsets expect node
> name as "qcom_scm". However, DeviceTree uses node name "scm" and this
> mismatch prevents firmware from correctly identifying waitqueue IRQ
> information. Waitqueue IRQ is used for signaling between secure and
> non-secure worlds.
> 
> To resolve this, introduce qcom_scm_get_waitq_irq() that'll get the
> hardware irq number to be used from firmware instead of relying on data
> provided by devicetree, thereby bypassing the DeviceTree node name
> mismatch.
> 
> This hardware irq number is converted to a linux irq number using newly
> defined fill_irq_fwspec_params(). This linux irq number is then supplied to
> the threaded_irq call.
> 
> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 59 +++++++++++++++++++++++++++++++-
>  drivers/firmware/qcom/qcom_scm.h |  1 +
>  2 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 00c379a3cceb..ed51fbb1c065 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -32,6 +32,14 @@
>  #include "qcom_scm.h"
>  #include "qcom_tzmem.h"
>  
> +#define GIC_SPI_BASE            32
> +#define GIC_MAX_SPI             987  // 1019 - 32
> +#define GIC_ESPI_BASE           4096
> +#define GIC_MAX_ESPI            1024 // 5120 - 4096

Are these going to remain constant on different implementations of the
interrupt controller across different SoCs that use this? Are these
mandated anywhere in the arm spec and/or present across the tree with
parts touching gicv3?

Also, the subtraction comments take some guesswork.. perhaps something like
0..31 etc. would be easier.

The MAX_(E)SPI macros could also just have the hwirq number to make the
if-conditions below simpler

> +
> +#define GIC_IRQ_TYPE_SPI        0
> +#define GIC_IRQ_TYPE_ESPI       2

We can definitely use dt-bindings for this

> +
>  static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
>  module_param(download_mode, bool, 0);
>  
> @@ -1819,6 +1827,55 @@ bool qcom_scm_is_available(void)
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_is_available);
>  
> +static int qcom_scm_fill_irq_fwspec_params(struct irq_fwspec *fwspec, u32 virq)
> +{
> +	if (WARN(virq < GIC_SPI_BASE, "Unexpected virq: %d\n", virq)) {
> +		return -ENXIO;
> +	} else if (virq <= (GIC_SPI_BASE + GIC_MAX_SPI)) {
> +		fwspec->param_count = 3;
> +		fwspec->param[0] = GIC_IRQ_TYPE_SPI;
> +		fwspec->param[1] = virq - GIC_SPI_BASE;
> +		fwspec->param[2] = IRQ_TYPE_EDGE_RISING;
> +	} else if (WARN(virq < GIC_ESPI_BASE, "Unexpected virq: %d\n", virq)) {
> +		return -ENXIO;
> +	} else if (virq < (GIC_ESPI_BASE + GIC_MAX_ESPI)) {
> +		fwspec->param_count = 3;
> +		fwspec->param[0] = GIC_IRQ_TYPE_ESPI;
> +		fwspec->param[1] = virq - GIC_ESPI_BASE;
> +		fwspec->param[2] = IRQ_TYPE_EDGE_RISING;
> +	} else {
> +		WARN(1, "Unexpected virq: %d\n", virq);
> +		return -ENXIO;
> +	}
> +	return 0;

This could use some prettifying (incl the previous comment):

if (GIC_SPI_BASE <= virq && virq <= GIC_SPI_MAX) {
	fwspec->param[0] = GIC_IRQ_TYPE_SPI;
	fwspec->param[1] = virq - GIC_SPI_BASE;
} else if (GIC_ESPI_BASE <= virq && virq <= GIC_ESPI_MAX) {
	fwspec->param[0] = GIC_IRQ_TYPE_ESPI;
	fwspec->param[1] = virq - GIC_ESPI_BASE;
} else {
	WARN(1, "Unexpected virq"...
	return -ENXIO;
}

fwspec->param[2] = IRQ_TYPE_EDGE_RISING;
fwspec->param_count = 3;

is much easier to follow along in my opinion

> +}
> +
> +static int qcom_scm_get_waitq_irq(void)
> +{
> +	int ret;
> +	u32 hwirq;
> +	struct qcom_scm_desc desc = {
> +		.svc = QCOM_SCM_SVC_WAITQ,
> +		.cmd = QCOM_SCM_WAITQ_GET_INFO,
> +		.owner = ARM_SMCCC_OWNER_SIP
> +	};
> +	struct qcom_scm_res res;
> +	struct irq_fwspec fwspec;
> +
> +	ret = qcom_scm_call_atomic(__scm->dev, &desc, &res);
> +	if (ret)
> +		return ret;
> +
> +	fwspec.fwnode = of_node_to_fwnode(__scm->dev->of_node);
> +	hwirq = res.result[1] & 0xffff;

GENMASK(15, 0)

> +	ret = qcom_scm_fill_irq_fwspec_params(&fwspec, hwirq);
> +	if (ret)
> +		return ret;
> +	ret = irq_create_fwspec_mapping(&fwspec);
> +
> +	return ret;
> +}
> +
>  static int qcom_scm_assert_valid_wq_ctx(u32 wq_ctx)
>  {
>  	/* FW currently only supports a single wq_ctx (zero).
> @@ -1936,7 +1993,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	/* Let all above stores be available after this */
>  	smp_store_release(&__scm, scm);
>  
> -	irq = platform_get_irq_optional(pdev, 0);
> +	irq = qcom_scm_get_waitq_irq();
>  	if (irq < 0) {
>  		if (irq != -ENXIO)

Is this smc call left unimplemented on !auto platforms? If it's not
(or it spits out bogus data), we're going to get a WARN splat in the
log..

Additionally, this mechanism ties the trustzone and hypervisor together..
Why isn't this done in gunyah which abstracts these resources? A hypercall
sounds much saner than tying in a third party into the mix

Konrad

Re: [PATCH v2 2/2] firmware: qcom_scm: Support multiple waitq contexts

[Bjorn Andersson]

On Thu, Aug 29, 2024 at 03:15:55PM GMT, Unnathi Chalicheemala wrote:
> Currently, only a single waitqueue context exists, with waitqueue id zero.
> Multi-waitqueue mechanism is added in firmware to support the case when
> multiple VMs make SMC calls or single VM making multiple calls on same CPU.
> 
> When VMs make SMC call, firmware will allocate waitqueue context assuming
> the SMC call to be a blocking call. SMC calls that cannot acquire resources
> are returned to sleep in the calling VM. When resource is available, VM
> will be notified to wake sleeping thread and resume SMC call.
> SM8650 firmware can allocate two such waitq contexts so create these two
> waitqueue contexts.
> 
> Unique waitqueue contexts are supported by a dynamically sized array where
> each unique wq_ctx is associated with a struct completion variable for easy
> lookup. To get the number of waitqueue contexts directly from firmware,
> qcom_scm_query_waitq_cnt() is introduced. On older targets which support
> only a single waitqueue, wq_cnt is set to 1 as SCM call for
> query_waitq_cnt() is not implemented for single waitqueue case.
> 
> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 82 +++++++++++++++++++++++---------
>  1 file changed, 60 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index ed51fbb1c065..b2c5505de681 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -44,12 +44,13 @@ static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
>  module_param(download_mode, bool, 0);
>  
>  struct qcom_scm {
> +	int wq_cnt;

Does it make sense for this to be negative? Please make it unsigned.

Also, might not be the most significant member of this struct, so
perhaps you can move it further down?

>  	struct device *dev;
>  	struct clk *core_clk;
>  	struct clk *iface_clk;
>  	struct clk *bus_clk;
>  	struct icc_path *path;
> -	struct completion waitq_comp;
> +	struct completion *waitq;
>  	struct reset_controller_dev reset;
>  
>  	/* control access to the interconnect path */
> @@ -1850,6 +1851,31 @@ static int qcom_scm_fill_irq_fwspec_params(struct irq_fwspec *fwspec, u32 virq)
>  	return 0;
>  }
>  
> +static int qcom_scm_query_waitq_count(void)
> +{
> +	bool avail;
> +	int count;
> +	int ret;
> +	struct qcom_scm_desc desc = {
> +		.svc = QCOM_SCM_SVC_WAITQ,
> +		.cmd = QCOM_SCM_WAITQ_GET_INFO,
> +		.owner = ARM_SMCCC_OWNER_SIP
> +	};
> +	struct qcom_scm_res res;
> +
> +	avail = __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_INFO);
> +	if (!avail) {
> +		count = 1;
> +		return count;

count is a local variable, so just return count; and drop the {} please.


Perhaps even drop the local boolean variable:

	if (!__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_INFO))
		return 1;

> +	}
> +
> +	ret = qcom_scm_call_atomic(__scm->dev, &desc, &res);
> +	if (ret)

(Keep this local variable, as that's in line with the style...)

> +		return ret;
> +	count = res.result[0] & 0xff;
> +	return count;

Again, return res.result[0] & 0xff; should be sufficient, no need for a
local variable immediately followed by a return statement.

> +}
> +
>  static int qcom_scm_get_waitq_irq(void)
>  {
>  	int ret;
> @@ -1876,42 +1902,40 @@ static int qcom_scm_get_waitq_irq(void)
>  	return ret;
>  }
>  
> -static int qcom_scm_assert_valid_wq_ctx(u32 wq_ctx)
> +static struct completion *qcom_scm_get_completion(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.
> -	 */
> -	if (wq_ctx != 0) {
> -		dev_err(__scm->dev, "Firmware unexpectedly passed non-zero wq_ctx\n");
> -		return -EINVAL;
> -	}
> +	struct completion *wq;
>  
> -	return 0;
> +	if (wq_ctx >= __scm->wq_cnt)

I'm guessing that we're not expecting to ever hit this, but if we do, we
will fail a qcom_scm_call() or qcom_scm_call_atomic() call, giving
someone down the road a bad week of debugging...

How about wrapping the conditional in a WARN_ON_ONCE()?

> +		return ERR_PTR(-EINVAL);
> +
> +	wq = &__scm->waitq[wq_ctx];
> +
> +	return wq;
>  }
>  
>  int qcom_scm_wait_for_wq_completion(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(wq_ctx);
> +	if (IS_ERR(wq))
> +		return PTR_ERR(wq);
>  
> -	wait_for_completion(&__scm->waitq_comp);
> +	wait_for_completion(wq);
>  
>  	return 0;
>  }
>  
>  static int qcom_scm_waitq_wakeup(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(wq_ctx);
> +	if (IS_ERR(wq))
> +		return PTR_ERR(wq);
>  
> -	complete(&__scm->waitq_comp);
> +	complete(wq);
>  
>  	return 0;
>  }
> @@ -1948,6 +1972,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	struct qcom_tzmem_pool_config pool_config;
>  	struct qcom_scm *scm;
>  	int irq, ret;
> +	int i;
>  
>  	scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
>  	if (!scm)
> @@ -1958,7 +1983,6 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> -	init_completion(&scm->waitq_comp);
>  	mutex_init(&scm->scm_bw_lock);
>  
>  	scm->path = devm_of_icc_get(&pdev->dev, NULL);
> @@ -1993,6 +2017,20 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	/* Let all above stores be available after this */
>  	smp_store_release(&__scm, scm);

Should have spotted this earlier... But if any code below this point
takes an error path (i.e. we return non-0 from hereon) devres will free
__scm and anyone calling the qcom_scm API will hit a use-after-free.

Add to that it doesn't seem like a good idea to have
qcom_scm_is_available() return true until we have setup the wait queue
count or setup tzmem at least.

>  
> +	platform_set_drvdata(pdev, scm);

I believe this is a leftover from previous versions of this patch?

Regards,
Bjorn

> +	ret = qcom_scm_query_waitq_count();
> +	if (ret < 0)
> +		return ret;
> +
> +	scm->wq_cnt = ret;
> +
> +	scm->waitq = devm_kcalloc(&pdev->dev, scm->wq_cnt, sizeof(*scm->waitq), GFP_KERNEL);
> +	if (!scm->waitq)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < scm->wq_cnt; i++)
> +		init_completion(&scm->waitq[i]);
> +
>  	irq = qcom_scm_get_waitq_irq();
>  	if (irq < 0) {
>  		if (irq != -ENXIO)
> -- 
> 2.34.1
> 

Re: [PATCH v2 1/2] firmware: qcom_scm: Add API to get waitqueue IRQ info

[Bjorn Andersson]

On Thu, Aug 29, 2024 at 03:15:54PM GMT, Unnathi Chalicheemala wrote:
> Bootloader and firmware for SM8650 and older chipsets expect node
> name as "qcom_scm".

Perhaps we can add the reason why bootloader/firmware is looking for
this node? Perhaps "looks for a node named qcom_scm, in order to patch
the wait queue IRQ information." ?

> However, DeviceTree uses node name "scm" and this
> mismatch prevents firmware from correctly identifying waitqueue IRQ
> information. Waitqueue IRQ is used for signaling between secure and
> non-secure worlds.
> 
> To resolve this, introduce qcom_scm_get_waitq_irq() that'll get the
> hardware irq number to be used from firmware instead of relying on data
> provided by devicetree, thereby bypassing the DeviceTree node name
> mismatch.
> 
> This hardware irq number is converted to a linux irq number using newly

Capitalize IRQ, and Linux throughout the message.

Regards,
Bjorn

> defined fill_irq_fwspec_params(). This linux irq number is then supplied to
> the threaded_irq call.
> 
> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 59 +++++++++++++++++++++++++++++++-
>  drivers/firmware/qcom/qcom_scm.h |  1 +
>  2 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 00c379a3cceb..ed51fbb1c065 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -32,6 +32,14 @@
>  #include "qcom_scm.h"
>  #include "qcom_tzmem.h"
>  
> +#define GIC_SPI_BASE            32
> +#define GIC_MAX_SPI             987  // 1019 - 32
> +#define GIC_ESPI_BASE           4096
> +#define GIC_MAX_ESPI            1024 // 5120 - 4096
> +
> +#define GIC_IRQ_TYPE_SPI        0
> +#define GIC_IRQ_TYPE_ESPI       2
> +
>  static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
>  module_param(download_mode, bool, 0);
>  
> @@ -1819,6 +1827,55 @@ bool qcom_scm_is_available(void)
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_is_available);
>  
> +static int qcom_scm_fill_irq_fwspec_params(struct irq_fwspec *fwspec, u32 virq)
> +{
> +	if (WARN(virq < GIC_SPI_BASE, "Unexpected virq: %d\n", virq)) {
> +		return -ENXIO;
> +	} else if (virq <= (GIC_SPI_BASE + GIC_MAX_SPI)) {
> +		fwspec->param_count = 3;
> +		fwspec->param[0] = GIC_IRQ_TYPE_SPI;
> +		fwspec->param[1] = virq - GIC_SPI_BASE;
> +		fwspec->param[2] = IRQ_TYPE_EDGE_RISING;
> +	} else if (WARN(virq < GIC_ESPI_BASE, "Unexpected virq: %d\n", virq)) {
> +		return -ENXIO;
> +	} else if (virq < (GIC_ESPI_BASE + GIC_MAX_ESPI)) {
> +		fwspec->param_count = 3;
> +		fwspec->param[0] = GIC_IRQ_TYPE_ESPI;
> +		fwspec->param[1] = virq - GIC_ESPI_BASE;
> +		fwspec->param[2] = IRQ_TYPE_EDGE_RISING;
> +	} else {
> +		WARN(1, "Unexpected virq: %d\n", virq);
> +		return -ENXIO;
> +	}
> +	return 0;
> +}
> +
> +static int qcom_scm_get_waitq_irq(void)
> +{
> +	int ret;
> +	u32 hwirq;
> +	struct qcom_scm_desc desc = {
> +		.svc = QCOM_SCM_SVC_WAITQ,
> +		.cmd = QCOM_SCM_WAITQ_GET_INFO,
> +		.owner = ARM_SMCCC_OWNER_SIP
> +	};
> +	struct qcom_scm_res res;
> +	struct irq_fwspec fwspec;
> +
> +	ret = qcom_scm_call_atomic(__scm->dev, &desc, &res);
> +	if (ret)
> +		return ret;
> +
> +	fwspec.fwnode = of_node_to_fwnode(__scm->dev->of_node);
> +	hwirq = res.result[1] & 0xffff;
> +	ret = qcom_scm_fill_irq_fwspec_params(&fwspec, hwirq);
> +	if (ret)
> +		return ret;
> +	ret = irq_create_fwspec_mapping(&fwspec);
> +
> +	return ret;
> +}
> +
>  static int qcom_scm_assert_valid_wq_ctx(u32 wq_ctx)
>  {
>  	/* FW currently only supports a single wq_ctx (zero).
> @@ -1936,7 +1993,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	/* Let all above stores be available after this */
>  	smp_store_release(&__scm, scm);
>  
> -	irq = platform_get_irq_optional(pdev, 0);
> +	irq = qcom_scm_get_waitq_irq();
>  	if (irq < 0) {
>  		if (irq != -ENXIO)
>  			return irq;
> diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
> index 685b8f59e7a6..ab0f88f5f777 100644
> --- a/drivers/firmware/qcom/qcom_scm.h
> +++ b/drivers/firmware/qcom/qcom_scm.h
> @@ -143,6 +143,7 @@ struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void);
>  #define QCOM_SCM_SVC_WAITQ			0x24
>  #define QCOM_SCM_WAITQ_RESUME			0x02
>  #define QCOM_SCM_WAITQ_GET_WQ_CTX		0x03
> +#define QCOM_SCM_WAITQ_GET_INFO		0x04
>  
>  #define QCOM_SCM_SVC_GPU			0x28
>  #define QCOM_SCM_SVC_GPU_INIT_REGS		0x01
> -- 
> 2.34.1
> 

Re: [PATCH v2 1/2] firmware: qcom_scm: Add API to get waitqueue IRQ info

[Unnathi Chalicheemala]

On 8/29/2024 4:45 PM, Konrad Dybcio wrote:
> On 30.08.2024 12:15 AM, Unnathi Chalicheemala wrote:
>> Bootloader and firmware for SM8650 and older chipsets expect node
>> name as "qcom_scm". However, DeviceTree uses node name "scm" and this
>> mismatch prevents firmware from correctly identifying waitqueue IRQ
>> information. Waitqueue IRQ is used for signaling between secure and
>> non-secure worlds.
>>
>> To resolve this, introduce qcom_scm_get_waitq_irq() that'll get the
>> hardware irq number to be used from firmware instead of relying on data
>> provided by devicetree, thereby bypassing the DeviceTree node name
>> mismatch.
>>
>> This hardware irq number is converted to a linux irq number using newly
>> defined fill_irq_fwspec_params(). This linux irq number is then supplied to
>> the threaded_irq call.
>>
>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
>> ---
>>  drivers/firmware/qcom/qcom_scm.c | 59 +++++++++++++++++++++++++++++++-
>>  drivers/firmware/qcom/qcom_scm.h |  1 +
>>  2 files changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 00c379a3cceb..ed51fbb1c065 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -32,6 +32,14 @@
>>  #include "qcom_scm.h"
>>  #include "qcom_tzmem.h"
>>  
>> +#define GIC_SPI_BASE            32
>> +#define GIC_MAX_SPI             987  // 1019 - 32
>> +#define GIC_ESPI_BASE           4096
>> +#define GIC_MAX_ESPI            1024 // 5120 - 4096
> 
> Are these going to remain constant on different implementations of the
> interrupt controller across different SoCs that use this? Are these
> mandated anywhere in the arm spec and/or present across the tree with
> parts touching gicv3?
> 

Yes they're constant across all SoCs that use Gunyah hypervisor.
They're documented in the GIC v3/v4 programming guide - I don't think
they're present in the tree.
INTID				Interrupt Type 
16 – 31				     PPIs
1056 – 1119 (GICv3.1)

32 – 1019			     SPIs
4096 – 5119 (GICv3.1) 

> Also, the subtraction comments take some guesswork.. perhaps something like
> 0..31 etc. would be easier.
> 
Ack.
> The MAX_(E)SPI macros could also just have the hwirq number to make the
> if-conditions below simpler
> 
Ack. I broke it up so the macros could be understandable. I can make them just
hwirq numbers.
>> +
>> +#define GIC_IRQ_TYPE_SPI        0
>> +#define GIC_IRQ_TYPE_ESPI       2
> 
> We can definitely use dt-bindings for this
> 
>> +
>>  static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
>>  module_param(download_mode, bool, 0);
>>  
>> @@ -1819,6 +1827,55 @@ bool qcom_scm_is_available(void)
>>  }
>>  EXPORT_SYMBOL_GPL(qcom_scm_is_available);
>>  
>> +static int qcom_scm_fill_irq_fwspec_params(struct irq_fwspec *fwspec, u32 virq)
>> +{
>> +	if (WARN(virq < GIC_SPI_BASE, "Unexpected virq: %d\n", virq)) {
>> +		return -ENXIO;
>> +	} else if (virq <= (GIC_SPI_BASE + GIC_MAX_SPI)) {
>> +		fwspec->param_count = 3;
>> +		fwspec->param[0] = GIC_IRQ_TYPE_SPI;
>> +		fwspec->param[1] = virq - GIC_SPI_BASE;
>> +		fwspec->param[2] = IRQ_TYPE_EDGE_RISING;
>> +	} else if (WARN(virq < GIC_ESPI_BASE, "Unexpected virq: %d\n", virq)) {
>> +		return -ENXIO;
>> +	} else if (virq < (GIC_ESPI_BASE + GIC_MAX_ESPI)) {
>> +		fwspec->param_count = 3;
>> +		fwspec->param[0] = GIC_IRQ_TYPE_ESPI;
>> +		fwspec->param[1] = virq - GIC_ESPI_BASE;
>> +		fwspec->param[2] = IRQ_TYPE_EDGE_RISING;
>> +	} else {
>> +		WARN(1, "Unexpected virq: %d\n", virq);
>> +		return -ENXIO;
>> +	}
>> +	return 0;
> 
> This could use some prettifying (incl the previous comment):
> 
> if (GIC_SPI_BASE <= virq && virq <= GIC_SPI_MAX) {
> 	fwspec->param[0] = GIC_IRQ_TYPE_SPI;
> 	fwspec->param[1] = virq - GIC_SPI_BASE;
> } else if (GIC_ESPI_BASE <= virq && virq <= GIC_ESPI_MAX) {
> 	fwspec->param[0] = GIC_IRQ_TYPE_ESPI;
> 	fwspec->param[1] = virq - GIC_ESPI_BASE;
> } else {
> 	WARN(1, "Unexpected virq"...
> 	return -ENXIO;
> }
> 
> fwspec->param[2] = IRQ_TYPE_EDGE_RISING;
> fwspec->param_count = 3;
> 
> is much easier to follow along in my opinion
> 
Ack, thanks!
>> +}
>> +
>> +static int qcom_scm_get_waitq_irq(void)
>> +{
>> +	int ret;
>> +	u32 hwirq;
>> +	struct qcom_scm_desc desc = {
>> +		.svc = QCOM_SCM_SVC_WAITQ,
>> +		.cmd = QCOM_SCM_WAITQ_GET_INFO,
>> +		.owner = ARM_SMCCC_OWNER_SIP
>> +	};
>> +	struct qcom_scm_res res;
>> +	struct irq_fwspec fwspec;
>> +
>> +	ret = qcom_scm_call_atomic(__scm->dev, &desc, &res);
>> +	if (ret)
>> +		return ret;
>> +
>> +	fwspec.fwnode = of_node_to_fwnode(__scm->dev->of_node);
>> +	hwirq = res.result[1] & 0xffff;
> 
> GENMASK(15, 0)
> 
Ack.
>> +	ret = qcom_scm_fill_irq_fwspec_params(&fwspec, hwirq);
>> +	if (ret)
>> +		return ret;
>> +	ret = irq_create_fwspec_mapping(&fwspec);
>> +
>> +	return ret;
>> +}
>> +
>>  static int qcom_scm_assert_valid_wq_ctx(u32 wq_ctx)
>>  {
>>  	/* FW currently only supports a single wq_ctx (zero).
>> @@ -1936,7 +1993,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>  	/* Let all above stores be available after this */
>>  	smp_store_release(&__scm, scm);
>>  
>> -	irq = platform_get_irq_optional(pdev, 0);
>> +	irq = qcom_scm_get_waitq_irq();
>>  	if (irq < 0) {
>>  		if (irq != -ENXIO)
> 
> Is this smc call left unimplemented on !auto platforms? If it's not
> (or it spits out bogus data), we're going to get a WARN splat in the
> log..
> 
This call is implemented on all platforms(auto and !auto) from SM8650 onward.
Will double-check on this.
> Additionally, this mechanism ties the trustzone and hypervisor together..
> Why isn't this done in gunyah which abstracts these resources? A hypercall
> sounds much saner than tying in a third party into the mix
> 
fill_irqfwspec_params is actually a function in gunyah's header file but I copied it
here as didn't want multi waitqueue support to be dependent on Gunyah's patches. I'll
check if this can be made a hyper call. 
Thanks for the review Konrad!
> Konrad

Re: [PATCH v2 2/2] firmware: qcom_scm: Support multiple waitq contexts

[Unnathi Chalicheemala]

On 9/4/2024 2:54 PM, Bjorn Andersson wrote:
> On Thu, Aug 29, 2024 at 03:15:55PM GMT, Unnathi Chalicheemala wrote:
>> Currently, only a single waitqueue context exists, with waitqueue id zero.
>> Multi-waitqueue mechanism is added in firmware to support the case when
>> multiple VMs make SMC calls or single VM making multiple calls on same CPU.
>>
>> When VMs make SMC call, firmware will allocate waitqueue context assuming
>> the SMC call to be a blocking call. SMC calls that cannot acquire resources
>> are returned to sleep in the calling VM. When resource is available, VM
>> will be notified to wake sleeping thread and resume SMC call.
>> SM8650 firmware can allocate two such waitq contexts so create these two
>> waitqueue contexts.
>>
>> Unique waitqueue contexts are supported by a dynamically sized array where
>> each unique wq_ctx is associated with a struct completion variable for easy
>> lookup. To get the number of waitqueue contexts directly from firmware,
>> qcom_scm_query_waitq_cnt() is introduced. On older targets which support
>> only a single waitqueue, wq_cnt is set to 1 as SCM call for
>> query_waitq_cnt() is not implemented for single waitqueue case.
>>
>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
>> ---
>>  drivers/firmware/qcom/qcom_scm.c | 82 +++++++++++++++++++++++---------
>>  1 file changed, 60 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index ed51fbb1c065..b2c5505de681 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -44,12 +44,13 @@ static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
>>  module_param(download_mode, bool, 0);
>>  
>>  struct qcom_scm {
>> +	int wq_cnt;
> 
> Does it make sense for this to be negative? Please make it unsigned.
> 
> Also, might not be the most significant member of this struct, so
> perhaps you can move it further down?
> 
Ack.
>>  	struct device *dev;
>>  	struct clk *core_clk;
>>  	struct clk *iface_clk;
>>  	struct clk *bus_clk;
>>  	struct icc_path *path;
>> -	struct completion waitq_comp;
>> +	struct completion *waitq;
>>  	struct reset_controller_dev reset;
>>  
>>  	/* control access to the interconnect path */
>> @@ -1850,6 +1851,31 @@ static int qcom_scm_fill_irq_fwspec_params(struct irq_fwspec *fwspec, u32 virq)
>>  	return 0;
>>  }
>>  
>> +static int qcom_scm_query_waitq_count(void)
>> +{
>> +	bool avail;
>> +	int count;
>> +	int ret;
>> +	struct qcom_scm_desc desc = {
>> +		.svc = QCOM_SCM_SVC_WAITQ,
>> +		.cmd = QCOM_SCM_WAITQ_GET_INFO,
>> +		.owner = ARM_SMCCC_OWNER_SIP
>> +	};
>> +	struct qcom_scm_res res;
>> +
>> +	avail = __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_INFO);
>> +	if (!avail) {
>> +		count = 1;
>> +		return count;
> 
> count is a local variable, so just return count; and drop the {} please.
> 
> 
> Perhaps even drop the local boolean variable:
> 
> 	if (!__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_INFO))
> 		return 1;
> 
Ack.
>> +	}
>> +
>> +	ret = qcom_scm_call_atomic(__scm->dev, &desc, &res);
>> +	if (ret)
> 
> (Keep this local variable, as that's in line with the style...)
> 
>> +		return ret;
>> +	count = res.result[0] & 0xff;
>> +	return count;
> 
> Again, return res.result[0] & 0xff; should be sufficient, no need for a
> local variable immediately followed by a return statement.
> 
Ack.
>> +}
>> +
>>  static int qcom_scm_get_waitq_irq(void)
>>  {
>>  	int ret;
>> @@ -1876,42 +1902,40 @@ static int qcom_scm_get_waitq_irq(void)
>>  	return ret;
>>  }
>>  
>> -static int qcom_scm_assert_valid_wq_ctx(u32 wq_ctx)
>> +static struct completion *qcom_scm_get_completion(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.
>> -	 */
>> -	if (wq_ctx != 0) {
>> -		dev_err(__scm->dev, "Firmware unexpectedly passed non-zero wq_ctx\n");
>> -		return -EINVAL;
>> -	}
>> +	struct completion *wq;
>>  
>> -	return 0;
>> +	if (wq_ctx >= __scm->wq_cnt)
> 
> I'm guessing that we're not expecting to ever hit this, but if we do, we
> will fail a qcom_scm_call() or qcom_scm_call_atomic() call, giving
> someone down the road a bad week of debugging...
> 
> How about wrapping the conditional in a WARN_ON_ONCE()?
> 
Yes understood, ack.
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	wq = &__scm->waitq[wq_ctx];
>> +
>> +	return wq;
>>  }
>>  
>>  int qcom_scm_wait_for_wq_completion(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(wq_ctx);
>> +	if (IS_ERR(wq))
>> +		return PTR_ERR(wq);
>>  
>> -	wait_for_completion(&__scm->waitq_comp);
>> +	wait_for_completion(wq);
>>  
>>  	return 0;
>>  }
>>  
>>  static int qcom_scm_waitq_wakeup(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(wq_ctx);
>> +	if (IS_ERR(wq))
>> +		return PTR_ERR(wq);
>>  
>> -	complete(&__scm->waitq_comp);
>> +	complete(wq);
>>  
>>  	return 0;
>>  }
>> @@ -1948,6 +1972,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>  	struct qcom_tzmem_pool_config pool_config;
>>  	struct qcom_scm *scm;
>>  	int irq, ret;
>> +	int i;
>>  
>>  	scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
>>  	if (!scm)
>> @@ -1958,7 +1983,6 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	init_completion(&scm->waitq_comp);
>>  	mutex_init(&scm->scm_bw_lock);
>>  
>>  	scm->path = devm_of_icc_get(&pdev->dev, NULL);
>> @@ -1993,6 +2017,20 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>  	/* Let all above stores be available after this */
>>  	smp_store_release(&__scm, scm);
> 
> Should have spotted this earlier... But if any code below this point
> takes an error path (i.e. we return non-0 from hereon) devres will free
> __scm and anyone calling the qcom_scm API will hit a use-after-free.
> 
> Add to that it doesn't seem like a good idea to have
> qcom_scm_is_available() return true until we have setup the wait queue
> count or setup tzmem at least.
> 
Would the other calls that go through error path below need to be before the smp_store_release?
Just wondering if that needs to be fixed in a separate patch..

And I think the waitq initialization before the smp_store_release should be okay.
>>  
>> +	platform_set_drvdata(pdev, scm);
> 
> I believe this is a leftover from previous versions of this patch?
> 
Yes, will remove this.
> Regards,
> Bjorn
> 
>> +	ret = qcom_scm_query_waitq_count();
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	scm->wq_cnt = ret;
>> +
>> +	scm->waitq = devm_kcalloc(&pdev->dev, scm->wq_cnt, sizeof(*scm->waitq), GFP_KERNEL);
>> +	if (!scm->waitq)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < scm->wq_cnt; i++)
>> +		init_completion(&scm->waitq[i]);
>> +
>>  	irq = qcom_scm_get_waitq_irq();
>>  	if (irq < 0) {
>>  		if (irq != -ENXIO)
>> -- 
>> 2.34.1
>>
>