[RESEND PATCH v2 0/5] SCM: Add support for wait-queue aware firmware

[RESEND PATCH v2 0/5] SCM: Add support for wait-queue aware firmware

[Guru Das Srinagesh]

This patch series enables the QCOM SCM driver to support firmware (FW) versions
that expect the high-level OS (HLOS) to be tolerant of SCM call requests not
being processed right away and, instead, being placed on a wait-queue in FW and
processed accordingly.

The problem this feature is fixing is as follows. In a scenario where there is
a VM in addition to HLOS (and an underlying hypervisor):

1. HLOS makes an SMC call on core 5
2. The hypervisor scheduling interrupt interrupts this SMC call.
3. The hypervisor schedules the VM on core 5.
4. The VM makes an SMC call on core 5.
5. The SMC call is non-interruptibly stuck on FW spinlock on core 5.
6. HLOS cannot reschedule since core 5 is not responding to Reschedule IPIs.
7. Watchdog timer expires waiting for core 5.

This problem is solved by FW returning a new return code SCM_WAITQ_SLEEP to
HLOS right away when it is overwhelmed by the VM's SMC call. HLOS then places
the call on a wait-queue and wakes it up when it receives an interrupt that
signifies "all-clear".

Additionally, this new design also supports scenarios involving only HLOS (and
no other VMs).  Such scenarios make use of a second new return code
SCM_WAITQ_WAKE.

There are three new SMC calls also being defined in this design that, together
with the two new return codes, form the handshake protocol between Linux and
FW.

This design is also backwards-compatible with existing firmware versions that
do not support this feature.

---
v2:
- Changes made to patches 4 and 5 are listed therein.
- Rebased dt-bindings on top of the YAML conversion patch [1].

[1] https://lore.kernel.org/lkml/20220708090431.30437-1-david@ixit.cz/
---

Guru Das Srinagesh (5):
  dt-bindings: firmware: qcom-scm: Add "allow-multi-call" property
  firmware: qcom: scm: Optionally remove SCM call serialization
  dt-bindings: firmware: qcom-scm: Add optional interrupt
  firmware: qcom: scm: Add wait-queue helper functions
  firmware: qcom: scm: Add wait-queue handling logic

 .../devicetree/bindings/firmware/qcom,scm.yaml     |  10 ++
 drivers/firmware/qcom_scm-smc.c                    | 140 +++++++++++++++++++--
 drivers/firmware/qcom_scm.c                        | 125 +++++++++++++++++-
 drivers/firmware/qcom_scm.h                        |  14 +++
 4 files changed, 279 insertions(+), 10 deletions(-)

-- 
2.7.4

[RESEND PATCH v2 1/5] dt-bindings: firmware: qcom-scm: Add "allow-multi-call" property

[Guru Das Srinagesh]

For firmware that supports it, allow multiple SCM calls to be passed
down to it by removing the serialization lock in the SCM driver.

This patch is based on this YAML conversion patch [1] that is in-flight
currently.

[1] https://lore.kernel.org/lkml/20220708090431.30437-1-david@ixit.cz/

Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
---
 Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
index 9fdeee0..e279fd2 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
@@ -70,6 +70,11 @@ properties:
   '#reset-cells':
     const: 1
 
+  allow-multi-call:
+    description:
+      Specify this flag to remove SCM call serialization. Need to ensure that
+      the firmware being used supports this feature first.
+
   qcom,dload-mode:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     items:
-- 
2.7.4

Re: [RESEND PATCH v2 1/5] dt-bindings: firmware: qcom-scm: Add "allow-multi-call" property

[Krzysztof Kozlowski]

On 31/08/2022 01:25, Guru Das Srinagesh wrote:
> For firmware that supports it, allow multiple SCM calls to be passed
> down to it by removing the serialization lock in the SCM driver.
> 
> This patch is based on this YAML conversion patch [1] that is in-flight
> currently.
> 
> [1] https://lore.kernel.org/lkml/20220708090431.30437-1-david@ixit.cz/
> 
> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>

Thank you for your patch. There is something to discuss/improve.

If you make a resend, at least be sure you Cc proper people. Use
scripts/get_maintainers.pl to CC all maintainers and relevant mailing lists.

This was not Cced to device tree maintainers, therefore it has to be NAK-ed.


> ---
>  Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> index 9fdeee0..e279fd2 100644
> --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> @@ -70,6 +70,11 @@ properties:
>    '#reset-cells':
>      const: 1
>  
> +  allow-multi-call:
> +    description:
> +      Specify this flag to remove SCM call serialization. Need to ensure that
> +      the firmware being used supports this feature first.

Missing vendor prefix, missing type.

Isn't support for this obvious from compatible?\

Best regards,
Krzysztof

Re: [RESEND PATCH v2 1/5] dt-bindings: firmware: qcom-scm: Add "allow-multi-call" property

[Sibi Sankar]

Hey Krzysztof,
Thanks for taking time to review the series.


On 8/31/22 1:30 PM, Krzysztof Kozlowski wrote:
> On 31/08/2022 01:25, Guru Das Srinagesh wrote:
>> For firmware that supports it, allow multiple SCM calls to be passed
>> down to it by removing the serialization lock in the SCM driver.
>>
>> This patch is based on this YAML conversion patch [1] that is in-flight
>> currently.
>>
>> [1] https://lore.kernel.org/lkml/20220708090431.30437-1-david@ixit.cz/
>>
>> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> If you make a resend, at least be sure you Cc proper people. Use
> scripts/get_maintainers.pl to CC all maintainers and relevant mailing lists.
> 
> This was not Cced to device tree maintainers, therefore it has to be NAK-ed.
> 
> 
>> ---
>>   Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
>> index 9fdeee0..e279fd2 100644
>> --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
>> @@ -70,6 +70,11 @@ properties:
>>     '#reset-cells':
>>       const: 1
>>   
>> +  allow-multi-call:
>> +    description:
>> +      Specify this flag to remove SCM call serialization. Need to ensure that
>> +      the firmware being used supports this feature first.
> 
> Missing vendor prefix, missing type.

Ack

> 
> Isn't support for this obvious from compatible?\

On further testing it looks like the property can't be truly enabled on
sm8450 yet so I'll drop the patch in the next re-spin.

> 
> Best regards,
> Krzysztof
> 

[RESEND PATCH v2 2/5] firmware: qcom: scm: Optionally remove SCM call serialization

[Guru Das Srinagesh]

Some firmware versions support the handling of multiple SCM calls at the
same time. Add a device tree boolean property which, when specified,
allows this to happen.

Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
---
 drivers/firmware/qcom_scm-smc.c | 8 ++++++--
 drivers/firmware/qcom_scm.c     | 6 ++++++
 drivers/firmware/qcom_scm.h     | 2 ++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
index d111833..66193c2 100644
--- a/drivers/firmware/qcom_scm-smc.c
+++ b/drivers/firmware/qcom_scm-smc.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright (c) 2015,2019 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #include <linux/io.h>
@@ -63,11 +64,14 @@ static void __scm_smc_do(const struct arm_smccc_args *smc,
 	}
 
 	do {
-		mutex_lock(&qcom_scm_lock);
+		if (!qcom_scm_allow_multicall)
+			mutex_lock(&qcom_scm_lock);
 
 		__scm_smc_do_quirk(smc, res);
 
-		mutex_unlock(&qcom_scm_lock);
+		if (!qcom_scm_allow_multicall)
+			mutex_unlock(&qcom_scm_lock);
+
 
 		if (res->a0 == QCOM_SCM_V2_EBUSY) {
 			if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index cdbfe54..978706a 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright (c) 2010,2015,2019 The Linux Foundation. All rights reserved.
  * Copyright (C) 2015 Linaro Ltd.
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 #include <linux/platform_device.h>
 #include <linux/init.h>
@@ -23,6 +24,8 @@
 static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
 module_param(download_mode, bool, 0);
 
+bool qcom_scm_allow_multicall = false;
+
 #define SCM_HAS_CORE_CLK	BIT(0)
 #define SCM_HAS_IFACE_CLK	BIT(1)
 #define SCM_HAS_BUS_CLK		BIT(2)
@@ -1402,6 +1405,9 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	__scm = scm;
 	__scm->dev = &pdev->dev;
 
+	qcom_scm_allow_multicall = of_property_read_bool(__scm->dev->of_node,
+							"allow-multi-call");
+
 	__get_convention();
 
 	/*
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 0d51eef..c0a4d6b 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /* Copyright (c) 2010-2015,2019 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 #ifndef __QCOM_SCM_INT_H
 #define __QCOM_SCM_INT_H
@@ -12,6 +13,7 @@ enum qcom_scm_convention {
 };
 
 extern enum qcom_scm_convention qcom_scm_convention;
+extern bool qcom_scm_allow_multicall;
 
 #define MAX_QCOM_SCM_ARGS 10
 #define MAX_QCOM_SCM_RETS 3
-- 
2.7.4

Re: [RESEND PATCH v2 2/5] firmware: qcom: scm: Optionally remove SCM call serialization

[Krzysztof Kozlowski]

On 31/08/2022 01:25, Guru Das Srinagesh wrote:
> Some firmware versions support the handling of multiple SCM calls at the
> same time. Add a device tree boolean property which, when specified,
> allows this to happen.
> 
> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
> ---
>  drivers/firmware/qcom_scm-smc.c | 8 ++++++--
>  drivers/firmware/qcom_scm.c     | 6 ++++++
>  drivers/firmware/qcom_scm.h     | 2 ++
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
> index d111833..66193c2 100644
> --- a/drivers/firmware/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom_scm-smc.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright (c) 2015,2019 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
>  #include <linux/io.h>
> @@ -63,11 +64,14 @@ static void __scm_smc_do(const struct arm_smccc_args *smc,
>  	}
>  
>  	do {
> -		mutex_lock(&qcom_scm_lock);
> +		if (!qcom_scm_allow_multicall)
> +			mutex_lock(&qcom_scm_lock);
>  
>  		__scm_smc_do_quirk(smc, res);
>  
> -		mutex_unlock(&qcom_scm_lock);
> +		if (!qcom_scm_allow_multicall)
> +			mutex_unlock(&qcom_scm_lock);
> +
>  
>  		if (res->a0 == QCOM_SCM_V2_EBUSY) {
>  			if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index cdbfe54..978706a 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright (c) 2010,2015,2019 The Linux Foundation. All rights reserved.
>   * Copyright (C) 2015 Linaro Ltd.
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  #include <linux/platform_device.h>
>  #include <linux/init.h>
> @@ -23,6 +24,8 @@
>  static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
>  module_param(download_mode, bool, 0);
>  
> +bool qcom_scm_allow_multicall = false;

No global variables. This should be part of context/state container.

> +
>  #define SCM_HAS_CORE_CLK	BIT(0)
>  #define SCM_HAS_IFACE_CLK	BIT(1)
>  #define SCM_HAS_BUS_CLK		BIT(2)
> @@ -1402,6 +1405,9 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	__scm = scm;
>  	__scm->dev = &pdev->dev;
>  
> +	qcom_scm_allow_multicall = of_property_read_bool(__scm->dev->of_node,
> +							"allow-multi-call");
> +
>  	__get_convention();
>  
>  	/*
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index 0d51eef..c0a4d6b 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -1,5 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  /* Copyright (c) 2010-2015,2019 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  #ifndef __QCOM_SCM_INT_H
>  #define __QCOM_SCM_INT_H
> @@ -12,6 +13,7 @@ enum qcom_scm_convention {
>  };
>  
>  extern enum qcom_scm_convention qcom_scm_convention;
> +extern bool qcom_scm_allow_multicall;

No externs for variables. You break encapsulation.

>  
>  #define MAX_QCOM_SCM_ARGS 10
>  #define MAX_QCOM_SCM_RETS 3


Best regards,
Krzysztof

[RESEND PATCH v2 3/5] dt-bindings: firmware: qcom-scm: Add optional interrupt

[Guru Das Srinagesh]

Add an interrupt specification to the bindings to support the wait-queue
feature.

Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
---
 Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
index e279fd2..4d6c89f 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
@@ -75,6 +75,11 @@ properties:
       Specify this flag to remove SCM call serialization. Need to ensure that
       the firmware being used supports this feature first.
 
+  interrupts:
+    description:
+      The wait-queue interrupt that firmware raises as part of handshake
+      protocol to handle sleeping SCM calls.
+
   qcom,dload-mode:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     items:
-- 
2.7.4

Re: [RESEND PATCH v2 3/5] dt-bindings: firmware: qcom-scm: Add optional interrupt

[Krzysztof Kozlowski]

On 31/08/2022 01:25, Guru Das Srinagesh wrote:
> Add an interrupt specification to the bindings to support the wait-queue
> feature.
> 
> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>

Also not CC-ed to proper people and lists.

> ---
>  Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> index e279fd2..4d6c89f 100644
> --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> @@ -75,6 +75,11 @@ properties:
>        Specify this flag to remove SCM call serialization. Need to ensure that
>        the firmware being used supports this feature first.
>  
> +  interrupts:
> +    description:
> +      The wait-queue interrupt that firmware raises as part of handshake
> +      protocol to handle sleeping SCM calls.

Missing constraints.

Which firmwares support it?

> +
>    qcom,dload-mode:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      items:


Best regards,
Krzysztof

Re: [RESEND PATCH v2 3/5] dt-bindings: firmware: qcom-scm: Add optional interrupt

[Sibi Sankar]

Hey Krzysztof,
Thanks for taking time to review the series.

On 8/31/22 1:32 PM, Krzysztof Kozlowski wrote:
> On 31/08/2022 01:25, Guru Das Srinagesh wrote:
>> Add an interrupt specification to the bindings to support the wait-queue
>> feature.
>>
>> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
> 
> Also not CC-ed to proper people and lists.
> 
>> ---
>>   Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
>> index e279fd2..4d6c89f 100644
>> --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
>> @@ -75,6 +75,11 @@ properties:
>>         Specify this flag to remove SCM call serialization. Need to ensure that
>>         the firmware being used supports this feature first.
>>   
>> +  interrupts:
>> +    description:
>> +      The wait-queue interrupt that firmware raises as part of handshake
>> +      protocol to handle sleeping SCM calls.
> 
> Missing constraints.
> 
> Which firmwares support it?
> 

The interrupt property for scm firmware from a binding perspective
is completely optional i.e. not all tz fw running in the wild on sm8450
devices support this feature. The bootloader does the interrupt property
addition on sm8450 devices with support.

-Sibi

>> +
>>     qcom,dload-mode:
>>       $ref: /schemas/types.yaml#/definitions/phandle-array
>>       items:
> 
> 
> Best regards,
> Krzysztof
> 

Re: [RESEND PATCH v2 3/5] dt-bindings: firmware: qcom-scm: Add optional interrupt

[Krzysztof Kozlowski]

On 18/10/2022 01:49, Sibi Sankar wrote:
>>>   Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
>>> index e279fd2..4d6c89f 100644
>>> --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
>>> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
>>> @@ -75,6 +75,11 @@ properties:
>>>         Specify this flag to remove SCM call serialization. Need to ensure that
>>>         the firmware being used supports this feature first.
>>>   
>>> +  interrupts:
>>> +    description:
>>> +      The wait-queue interrupt that firmware raises as part of handshake
>>> +      protocol to handle sleeping SCM calls.
>>
>> Missing constraints.
>>
>> Which firmwares support it?
>>
> 
> The interrupt property for scm firmware from a binding perspective
> is completely optional i.e. not all tz fw running in the wild on sm8450
> devices support this feature. The bootloader does the interrupt property
> addition on sm8450 devices with support.

OK.

Best regards,
Krzysztof

[RESEND PATCH v2 4/5] firmware: qcom: scm: Add wait-queue helper functions

[Guru Das Srinagesh]

When the firmware (FW) supports multiple requests per VM, and the VM
also supports it via the `allow-multi-call` device tree flag, the
floodgates are thrown open for them to all reach the firmware at the
same time.

Since the firmware currently being used has limited resources, it guards
them with a resource lock and puts requests on a wait-queue internally
and signals to HLOS that it is doing so. It does this by returning two
new return values in addition to success or error: SCM_WAITQ_SLEEP and
SCM_WAITQ_WAKE.

  1) SCM_WAITQ_SLEEP:

  	When an SCM call receives this return value instead of success
  	or error, FW has placed this call on a wait-queue and
  	has signalled HLOS to put it to non-interruptible sleep. (The
	mechanism to wake it back up will be described in detail in the
	next patch for the sake of simplicity.)

	Along with this return value, FW also passes to HLOS `wq_ctx` -
	a unique number (UID) identifying the wait-queue that it has put
	the call on, internally. This is to help HLOS with its own
	bookkeeping to wake this sleeping call later.

	Additionally, FW also passes to HLOS `smc_call_ctx` - a UID
	identifying the SCM call thus being put to sleep. This is also
	for HLOS' bookkeeping to wake this call up later.

	These two additional values are passed via the a1 and a2
	registers.

	N.B.: The "ctx" in the above UID names = "context".

  2) SCM_WAITQ_WAKE:

  	When an SCM call receives this return value instead of success
  	or error, FW wishes to signal HLOS to wake up a (different)
  	previously sleeping call.

  	FW tells HLOS which call to wake up via the additional return
  	values `wq_ctx`, `smc_call_ctx` and `flags`. The first two have
  	already been explained above.

  	`flags` can be either WAKE_ONE or WAKE_ALL. Meaning, wake either
  	one, or all, of the SCM calls that HLOS is associating with the
  	given `wq_ctx`.

A sleeping SCM call can be woken up by either an interrupt that FW
raises, or via a SCM_WAITQ_WAKE return value for a new SCM call.

The handshake mechanism that HLOS uses to talk to FW about wait-queue
operations involves three new SMC calls. These are:

  1) get_wq_ctx():

    	Arguments: 	None
    	Returns:	wq_ctx, flags, more_pending

    	Get the wait-queue context, and wake up either one or all of the
    	sleeping SCM calls associated with that wait-queue.

    	Additionally, repeat this if there are more wait-queues that are
    	ready to have their requests woken up (`more_pending`).

  2) wq_resume(smc_call_ctx):

  	Arguments:	smc_call_ctx

  	HLOS needs to issue this in response to receiving an
  	IRQ, passing to FW the same smc_call_ctx that FW
  	receives from HLOS via the get_wq_ctx() call.

  3) wq_wake_ack(smc_call_ctx):

  	Arguments:	smc_call_ctx

  	HLOS needs to issue this in response to receiving an
  	SCM_WAITQ_WAKE, passing to FW the same smc_call_ctx that FW
  	passed to HLOS via the SMC_WAITQ_WAKE call.

(Reminder that the full handshake mechanism will be detailed in the
subsequent patch.)

Also add the interrupt handler that wakes up a sleeping SCM call.

Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
---
 drivers/firmware/qcom_scm-smc.c |  56 +++++++++++++++++++
 drivers/firmware/qcom_scm.c     | 119 +++++++++++++++++++++++++++++++++++++++-
 drivers/firmware/qcom_scm.h     |  12 ++++
 3 files changed, 186 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
index 66193c2..4150da1 100644
--- a/drivers/firmware/qcom_scm-smc.c
+++ b/drivers/firmware/qcom_scm-smc.c
@@ -53,6 +53,62 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
 	} while (res->a0 == QCOM_SCM_INTERRUPTED);
 }
 
+static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
+{
+	memset(resume->args, 0, ARRAY_SIZE(resume->args));
+
+	resume->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
+			 ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
+			 SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_RESUME));
+
+	resume->args[1] = QCOM_SCM_ARGS(1);
+
+	resume->args[2] = smc_call_ctx;
+}
+
+static void fill_wq_wake_ack_args(struct arm_smccc_args *wake_ack, u32 smc_call_ctx)
+{
+	memset(wake_ack->args, 0, ARRAY_SIZE(wake_ack->args));
+
+	wake_ack->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
+			 ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
+			 SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_ACK));
+
+	wake_ack->args[1] = QCOM_SCM_ARGS(1);
+
+	wake_ack->args[2] = smc_call_ctx;
+}
+
+static void fill_get_wq_ctx_args(struct arm_smccc_args *get_wq_ctx)
+{
+	memset(get_wq_ctx->args, 0, ARRAY_SIZE(get_wq_ctx->args));
+
+	get_wq_ctx->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
+			 ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
+			 SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_WQ_CTX));
+}
+
+int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending)
+{
+	int ret;
+	struct arm_smccc_args get_wq_ctx = {0};
+	struct arm_smccc_res get_wq_res;
+
+	fill_get_wq_ctx_args(&get_wq_ctx);
+
+	__scm_smc_do_quirk(&get_wq_ctx, &get_wq_res);
+	/* Guaranteed to return only success or error, no WAITQ_* */
+	ret = get_wq_res.a0;
+	if (ret)
+		return ret;
+
+	*wq_ctx = get_wq_res.a1;
+	*flags  = get_wq_res.a2;
+	*more_pending = get_wq_res.a3;
+
+	return 0;
+}
+
 static void __scm_smc_do(const struct arm_smccc_args *smc,
 			 struct arm_smccc_res *res, bool atomic)
 {
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 978706a..9ae3fcf 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -3,8 +3,12 @@
  * Copyright (C) 2015 Linaro Ltd.
  * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
  */
+#define pr_fmt(fmt)     "qcom-scm: %s: " fmt, __func__
+
 #include <linux/platform_device.h>
+#include <linux/idr.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/cpumask.h>
 #include <linux/export.h>
 #include <linux/dma-mapping.h>
@@ -13,10 +17,13 @@
 #include <linux/types.h>
 #include <linux/qcom_scm.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/clk.h>
 #include <linux/reset-controller.h>
+#include <linux/spinlock.h>
+#include <linux/ktime.h>
 #include <linux/arm-smccc.h>
 
 #include "qcom_scm.h"
@@ -30,6 +37,12 @@ bool qcom_scm_allow_multicall = false;
 #define SCM_HAS_IFACE_CLK	BIT(1)
 #define SCM_HAS_BUS_CLK		BIT(2)
 
+struct qcom_scm_waitq {
+	struct idr idr;
+	spinlock_t idr_lock;
+	struct work_struct scm_irq_work;
+};
+
 struct qcom_scm {
 	struct device *dev;
 	struct clk *core_clk;
@@ -37,6 +50,7 @@ struct qcom_scm {
 	struct clk *bus_clk;
 	struct icc_path *path;
 	struct reset_controller_dev reset;
+	struct qcom_scm_waitq waitq;
 
 	/* control access to the interconnect path */
 	struct mutex scm_bw_lock;
@@ -62,10 +76,14 @@ struct qcom_scm_mem_map_info {
 static const u8 qcom_scm_cpu_cold_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
 	0, BIT(0), BIT(3), BIT(5)
 };
+
 static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
 	BIT(2), BIT(1), BIT(4), BIT(6)
 };
 
+#define QCOM_SMC_WAITQ_FLAG_WAKE_ONE	BIT(0)
+#define QCOM_SMC_WAITQ_FLAG_WAKE_ALL	BIT(1)
+
 static const char * const qcom_scm_convention_names[] = {
 	[SMC_CONVENTION_UNKNOWN] = "unknown",
 	[SMC_CONVENTION_ARM_32] = "smc arm 32",
@@ -1328,11 +1346,92 @@ bool qcom_scm_is_available(void)
 }
 EXPORT_SYMBOL(qcom_scm_is_available);
 
+struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)
+{
+	struct completion *wq = NULL;
+	u32 wq_ctx_idr = wq_ctx;
+	unsigned long flags;
+	int err;
+
+	spin_lock_irqsave(&scm->waitq.idr_lock, flags);
+	wq = idr_find(&scm->waitq.idr, wq_ctx);
+	if (wq)
+		goto out;
+
+	wq = devm_kzalloc(scm->dev, sizeof(*wq), GFP_ATOMIC);
+	if (!wq) {
+		wq = ERR_PTR(-ENOMEM);
+		goto out;
+	}
+
+	init_completion(wq);
+
+	err = idr_alloc_u32(&scm->waitq.idr, wq, &wq_ctx_idr,
+			    U32_MAX, GFP_ATOMIC);
+	if (err < 0) {
+		devm_kfree(scm->dev, wq);
+		wq = ERR_PTR(err);
+	}
+
+out:
+	spin_unlock_irqrestore(&scm->waitq.idr_lock, flags);
+	return wq;
+}
+
+void scm_waitq_flag_handler(struct completion *wq, u32 flags)
+{
+	switch (flags) {
+	case QCOM_SMC_WAITQ_FLAG_WAKE_ONE:
+		complete(wq);
+		break;
+	case QCOM_SMC_WAITQ_FLAG_WAKE_ALL:
+		complete_all(wq);
+		break;
+	default:
+		pr_err("invalid flags: %u\n", flags);
+	}
+}
+
+static void scm_irq_work(struct work_struct *work)
+{
+	int ret;
+	u32 wq_ctx, flags, more_pending = 0;
+	struct completion *wq_to_wake;
+	struct qcom_scm_waitq *w = container_of(work, struct qcom_scm_waitq, scm_irq_work);
+	struct qcom_scm *scm = container_of(w, struct qcom_scm, waitq);
+
+	do {
+		ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
+		if (ret) {
+			pr_err("GET_WQ_CTX SMC call failed: %d\n", ret);
+			return;
+		}
+
+		wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
+		if (IS_ERR_OR_NULL(wq_to_wake)) {
+			pr_err("No waitqueue found for wq_ctx %d: %ld\n",
+					wq_ctx, PTR_ERR(wq_to_wake));
+			return;
+		}
+
+		scm_waitq_flag_handler(wq_to_wake, flags);
+	} while (more_pending);
+}
+
+static irqreturn_t qcom_scm_irq_handler(int irq, void *p)
+{
+	struct qcom_scm *scm = p;
+
+	schedule_work(&scm->waitq.scm_irq_work);
+
+	return IRQ_HANDLED;
+}
+
 static int qcom_scm_probe(struct platform_device *pdev)
 {
 	struct qcom_scm *scm;
 	unsigned long clks;
-	int ret;
+	int irq, ret;
 
 	scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
 	if (!scm)
@@ -1402,12 +1501,29 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	platform_set_drvdata(pdev, scm);
+
 	__scm = scm;
 	__scm->dev = &pdev->dev;
 
+	spin_lock_init(&__scm->waitq.idr_lock);
+	idr_init(&__scm->waitq.idr);
 	qcom_scm_allow_multicall = of_property_read_bool(__scm->dev->of_node,
 							"allow-multi-call");
 
+	INIT_WORK(&__scm->waitq.scm_irq_work, scm_irq_work);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq) {
+		ret = devm_request_threaded_irq(__scm->dev, irq, NULL,
+			qcom_scm_irq_handler, IRQF_ONESHOT, "qcom-scm", __scm);
+		if (ret < 0) {
+			pr_err("Failed to request qcom-scm irq: %d\n", ret);
+			idr_destroy(&__scm->waitq.idr);
+			return ret;
+		}
+	}
+
 	__get_convention();
 
 	/*
@@ -1423,6 +1539,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
 
 static void qcom_scm_shutdown(struct platform_device *pdev)
 {
+	idr_destroy(&__scm->waitq.idr);
 	/* Clean shutdown, disable download mode to allow normal restart */
 	if (download_mode)
 		qcom_scm_set_download_mode(false);
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index c0a4d6b..ae3a331 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -62,6 +62,11 @@ struct qcom_scm_res {
 	u64 result[MAX_QCOM_SCM_RETS];
 };
 
+struct qcom_scm;
+extern struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx);
+extern void scm_waitq_flag_handler(struct completion *wq, u32 flags);
+extern int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending);
+
 #define SCM_SMC_FNID(s, c)	((((s) & 0xFF) << 8) | ((c) & 0xFF))
 extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 			  enum qcom_scm_convention qcom_convention,
@@ -131,6 +136,11 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
 #define QCOM_SCM_SMMU_CONFIG_ERRATA1		0x03
 #define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL	0x02
 
+#define QCOM_SCM_SVC_WAITQ			0x24
+#define QCOM_SCM_WAITQ_ACK			0x01
+#define QCOM_SCM_WAITQ_RESUME			0x02
+#define QCOM_SCM_WAITQ_GET_WQ_CTX		0x03
+
 extern void __qcom_scm_init(void);
 
 /* common error codes */
@@ -141,6 +151,8 @@ extern void __qcom_scm_init(void);
 #define QCOM_SCM_EINVAL_ARG	-2
 #define QCOM_SCM_ERROR		-1
 #define QCOM_SCM_INTERRUPTED	1
+#define QCOM_SCM_WAITQ_SLEEP	2
+#define QCOM_SCM_WAITQ_WAKE	3
 
 static inline int qcom_scm_remap_error(int err)
 {
-- 
2.7.4

Re: [RESEND PATCH v2 4/5] firmware: qcom: scm: Add wait-queue helper functions

[kernel test robot]

Hi Guru,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20220830]
[cannot apply to robh/for-next linus/master v6.0-rc3 v6.0-rc2 v6.0-rc1 v6.0-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Guru-Das-Srinagesh/SCM-Add-support-for-wait-queue-aware-firmware/20220831-063013
base:    282342f2dc97ccf54254c5de51bcc1101229615f
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220831/202208310849.pEettWuy-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/ad41ee028d07c3e3e41b15e6bd8e2985f30df508
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Guru-Das-Srinagesh/SCM-Add-support-for-wait-queue-aware-firmware/20220831-063013
        git checkout ad41ee028d07c3e3e41b15e6bd8e2985f30df508
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/firmware/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/string.h:20,
                    from include/linux/bitmap.h:11,
                    from include/linux/cpumask.h:12,
                    from include/linux/smp.h:13,
                    from include/linux/lockdep.h:14,
                    from include/linux/spinlock.h:62,
                    from include/linux/mmzone.h:8,
                    from arch/m68k/include/asm/virtconvert.h:12,
                    from arch/m68k/include/asm/io_mm.h:26,
                    from arch/m68k/include/asm/io.h:8,
                    from include/linux/io.h:13,
                    from drivers/firmware/qcom_scm-smc.c:6:
   drivers/firmware/qcom_scm-smc.c: In function 'fill_wq_resume_args':
>> arch/m68k/include/asm/string.h:68:25: warning: 'memset' used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size]
      68 | #define memset(d, c, n) __builtin_memset(d, c, n)
         |                         ^~~~~~~~~~~~~~~~
   drivers/firmware/qcom_scm-smc.c:58:9: note: in expansion of macro 'memset'
      58 |         memset(resume->args, 0, ARRAY_SIZE(resume->args));
         |         ^~~~~~
   drivers/firmware/qcom_scm-smc.c: In function 'fill_wq_wake_ack_args':
>> arch/m68k/include/asm/string.h:68:25: warning: 'memset' used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size]
      68 | #define memset(d, c, n) __builtin_memset(d, c, n)
         |                         ^~~~~~~~~~~~~~~~
   drivers/firmware/qcom_scm-smc.c:71:9: note: in expansion of macro 'memset'
      71 |         memset(wake_ack->args, 0, ARRAY_SIZE(wake_ack->args));
         |         ^~~~~~
   drivers/firmware/qcom_scm-smc.c: In function 'fill_get_wq_ctx_args':
>> arch/m68k/include/asm/string.h:68:25: warning: 'memset' used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size]
      68 | #define memset(d, c, n) __builtin_memset(d, c, n)
         |                         ^~~~~~~~~~~~~~~~
   drivers/firmware/qcom_scm-smc.c:84:9: note: in expansion of macro 'memset'
      84 |         memset(get_wq_ctx->args, 0, ARRAY_SIZE(get_wq_ctx->args));
         |         ^~~~~~
   drivers/firmware/qcom_scm-smc.c: At top level:
   drivers/firmware/qcom_scm-smc.c:69:13: warning: 'fill_wq_wake_ack_args' defined but not used [-Wunused-function]
      69 | static void fill_wq_wake_ack_args(struct arm_smccc_args *wake_ack, u32 smc_call_ctx)
         |             ^~~~~~~~~~~~~~~~~~~~~
   drivers/firmware/qcom_scm-smc.c:56:13: warning: 'fill_wq_resume_args' defined but not used [-Wunused-function]
      56 | static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
         |             ^~~~~~~~~~~~~~~~~~~


vim +/memset +68 arch/m68k/include/asm/string.h

ea61bc461d09e8 Greg Ungerer 2010-09-07  65  
ea61bc461d09e8 Greg Ungerer 2010-09-07  66  #define __HAVE_ARCH_MEMSET
ea61bc461d09e8 Greg Ungerer 2010-09-07  67  extern void *memset(void *, int, __kernel_size_t);
ea61bc461d09e8 Greg Ungerer 2010-09-07 @68  #define memset(d, c, n) __builtin_memset(d, c, n)
ea61bc461d09e8 Greg Ungerer 2010-09-07  69  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [RESEND PATCH v2 4/5] firmware: qcom: scm: Add wait-queue helper functions

[kernel test robot]

Hi Guru,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20220830]
[cannot apply to robh/for-next linus/master v6.0-rc3 v6.0-rc2 v6.0-rc1 v6.0-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Guru-Das-Srinagesh/SCM-Add-support-for-wait-queue-aware-firmware/20220831-063013
base:    282342f2dc97ccf54254c5de51bcc1101229615f
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20220831/202208311447.pd8ZLIWT-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/ad41ee028d07c3e3e41b15e6bd8e2985f30df508
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Guru-Das-Srinagesh/SCM-Add-support-for-wait-queue-aware-firmware/20220831-063013
        git checkout ad41ee028d07c3e3e41b15e6bd8e2985f30df508
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/firmware/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/string.h:20,
                    from include/linux/bitmap.h:11,
                    from include/linux/cpumask.h:12,
                    from include/linux/smp.h:13,
                    from include/linux/lockdep.h:14,
                    from include/linux/rcupdate.h:29,
                    from include/linux/rculist.h:11,
                    from include/linux/pid.h:5,
                    from include/linux/sched.h:14,
                    from include/linux/delay.h:23,
                    from drivers/firmware/qcom_scm-smc.c:8:
   drivers/firmware/qcom_scm-smc.c: In function 'fill_wq_resume_args':
>> arch/sparc/include/asm/string.h:18:29: warning: 'memset' used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size]
      18 | #define memset(s, c, count) __builtin_memset(s, c, count)
         |                             ^~~~~~~~~~~~~~~~
   drivers/firmware/qcom_scm-smc.c:58:9: note: in expansion of macro 'memset'
      58 |         memset(resume->args, 0, ARRAY_SIZE(resume->args));
         |         ^~~~~~
   drivers/firmware/qcom_scm-smc.c: In function 'fill_wq_wake_ack_args':
>> arch/sparc/include/asm/string.h:18:29: warning: 'memset' used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size]
      18 | #define memset(s, c, count) __builtin_memset(s, c, count)
         |                             ^~~~~~~~~~~~~~~~
   drivers/firmware/qcom_scm-smc.c:71:9: note: in expansion of macro 'memset'
      71 |         memset(wake_ack->args, 0, ARRAY_SIZE(wake_ack->args));
         |         ^~~~~~
   drivers/firmware/qcom_scm-smc.c: In function 'fill_get_wq_ctx_args':
>> arch/sparc/include/asm/string.h:18:29: warning: 'memset' used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size]
      18 | #define memset(s, c, count) __builtin_memset(s, c, count)
         |                             ^~~~~~~~~~~~~~~~
   drivers/firmware/qcom_scm-smc.c:84:9: note: in expansion of macro 'memset'
      84 |         memset(get_wq_ctx->args, 0, ARRAY_SIZE(get_wq_ctx->args));
         |         ^~~~~~
   drivers/firmware/qcom_scm-smc.c: At top level:
   drivers/firmware/qcom_scm-smc.c:69:13: warning: 'fill_wq_wake_ack_args' defined but not used [-Wunused-function]
      69 | static void fill_wq_wake_ack_args(struct arm_smccc_args *wake_ack, u32 smc_call_ctx)
         |             ^~~~~~~~~~~~~~~~~~~~~
   drivers/firmware/qcom_scm-smc.c:56:13: warning: 'fill_wq_resume_args' defined but not used [-Wunused-function]
      56 | static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
         |             ^~~~~~~~~~~~~~~~~~~


vim +/memset +18 arch/sparc/include/asm/string.h

70a6fcf3283a0a Al Viro 2016-01-17  16  
70a6fcf3283a0a Al Viro 2016-01-17  17  #define __HAVE_ARCH_MEMSET
70a6fcf3283a0a Al Viro 2016-01-17 @18  #define memset(s, c, count) __builtin_memset(s, c, count)
70a6fcf3283a0a Al Viro 2016-01-17  19  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [RESEND PATCH v2 4/5] firmware: qcom: scm: Add wait-queue helper functions

[Bjorn Andersson]

On Tue, Aug 30, 2022 at 03:25:10PM -0700, Guru Das Srinagesh wrote:
> When the firmware (FW) supports multiple requests per VM, and the VM
> also supports it via the `allow-multi-call` device tree flag, the
> floodgates are thrown open for them to all reach the firmware at the
> same time.
> 
> Since the firmware currently being used has limited resources, it guards
> them with a resource lock and puts requests on a wait-queue internally
> and signals to HLOS that it is doing so. It does this by returning two
> new return values in addition to success or error: SCM_WAITQ_SLEEP and
> SCM_WAITQ_WAKE.
> 
>   1) SCM_WAITQ_SLEEP:
> 
>   	When an SCM call receives this return value instead of success
>   	or error, FW has placed this call on a wait-queue and
>   	has signalled HLOS to put it to non-interruptible sleep. (The
> 	mechanism to wake it back up will be described in detail in the
> 	next patch for the sake of simplicity.)
> 
> 	Along with this return value, FW also passes to HLOS `wq_ctx` -
> 	a unique number (UID) identifying the wait-queue that it has put
> 	the call on, internally. This is to help HLOS with its own
> 	bookkeeping to wake this sleeping call later.
> 
> 	Additionally, FW also passes to HLOS `smc_call_ctx` - a UID
> 	identifying the SCM call thus being put to sleep. This is also
> 	for HLOS' bookkeeping to wake this call up later.
> 
> 	These two additional values are passed via the a1 and a2
> 	registers.
> 
> 	N.B.: The "ctx" in the above UID names = "context".
> 
>   2) SCM_WAITQ_WAKE:
> 
>   	When an SCM call receives this return value instead of success
>   	or error, FW wishes to signal HLOS to wake up a (different)
>   	previously sleeping call.
> 
>   	FW tells HLOS which call to wake up via the additional return
>   	values `wq_ctx`, `smc_call_ctx` and `flags`. The first two have
>   	already been explained above.
> 
>   	`flags` can be either WAKE_ONE or WAKE_ALL. Meaning, wake either
>   	one, or all, of the SCM calls that HLOS is associating with the
>   	given `wq_ctx`.
> 
> A sleeping SCM call can be woken up by either an interrupt that FW
> raises, or via a SCM_WAITQ_WAKE return value for a new SCM call.
> 
> The handshake mechanism that HLOS uses to talk to FW about wait-queue
> operations involves three new SMC calls. These are:
> 
>   1) get_wq_ctx():
> 
>     	Arguments: 	None
>     	Returns:	wq_ctx, flags, more_pending
> 
>     	Get the wait-queue context, and wake up either one or all of the
>     	sleeping SCM calls associated with that wait-queue.
> 
>     	Additionally, repeat this if there are more wait-queues that are
>     	ready to have their requests woken up (`more_pending`).
> 
>   2) wq_resume(smc_call_ctx):
> 
>   	Arguments:	smc_call_ctx
> 
>   	HLOS needs to issue this in response to receiving an
>   	IRQ, passing to FW the same smc_call_ctx that FW
>   	receives from HLOS via the get_wq_ctx() call.
> 
>   3) wq_wake_ack(smc_call_ctx):
> 
>   	Arguments:	smc_call_ctx
> 
>   	HLOS needs to issue this in response to receiving an
>   	SCM_WAITQ_WAKE, passing to FW the same smc_call_ctx that FW
>   	passed to HLOS via the SMC_WAITQ_WAKE call.
> 
> (Reminder that the full handshake mechanism will be detailed in the
> subsequent patch.)
> 
> Also add the interrupt handler that wakes up a sleeping SCM call.
> 
> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
> ---
>  drivers/firmware/qcom_scm-smc.c |  56 +++++++++++++++++++
>  drivers/firmware/qcom_scm.c     | 119 +++++++++++++++++++++++++++++++++++++++-
>  drivers/firmware/qcom_scm.h     |  12 ++++
>  3 files changed, 186 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
> index 66193c2..4150da1 100644
> --- a/drivers/firmware/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom_scm-smc.c
> @@ -53,6 +53,62 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
>  	} while (res->a0 == QCOM_SCM_INTERRUPTED);
>  }
>  
> +static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)

These two functions are used in the next patch only, so please move them
there.

> +{
> +	memset(resume->args, 0, ARRAY_SIZE(resume->args));
> +
> +	resume->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
> +			 ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
> +			 SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_RESUME));
> +
> +	resume->args[1] = QCOM_SCM_ARGS(1);
> +
> +	resume->args[2] = smc_call_ctx;
> +}
> +
> +static void fill_wq_wake_ack_args(struct arm_smccc_args *wake_ack, u32 smc_call_ctx)
> +{
> +	memset(wake_ack->args, 0, ARRAY_SIZE(wake_ack->args));
> +
> +	wake_ack->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
> +			 ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
> +			 SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_ACK));
> +
> +	wake_ack->args[1] = QCOM_SCM_ARGS(1);
> +
> +	wake_ack->args[2] = smc_call_ctx;
> +}
> +
> +static void fill_get_wq_ctx_args(struct arm_smccc_args *get_wq_ctx)

Afaict, this is only called once, right below. So please inline this
snippet in scm_get_wq_ctx()

> +{
> +	memset(get_wq_ctx->args, 0, ARRAY_SIZE(get_wq_ctx->args));
> +
> +	get_wq_ctx->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
> +			 ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
> +			 SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_WQ_CTX));
> +}
> +
> +int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending)
> +{
> +	int ret;
> +	struct arm_smccc_args get_wq_ctx = {0};
> +	struct arm_smccc_res get_wq_res;
> +
> +	fill_get_wq_ctx_args(&get_wq_ctx);
> +
> +	__scm_smc_do_quirk(&get_wq_ctx, &get_wq_res);
> +	/* Guaranteed to return only success or error, no WAITQ_* */
> +	ret = get_wq_res.a0;
> +	if (ret)
> +		return ret;
> +
> +	*wq_ctx = get_wq_res.a1;
> +	*flags  = get_wq_res.a2;
> +	*more_pending = get_wq_res.a3;
> +
> +	return 0;
> +}
> +
>  static void __scm_smc_do(const struct arm_smccc_args *smc,
>  			 struct arm_smccc_res *res, bool atomic)
>  {
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 978706a..9ae3fcf 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -3,8 +3,12 @@
>   * Copyright (C) 2015 Linaro Ltd.
>   * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
> +#define pr_fmt(fmt)     "qcom-scm: %s: " fmt, __func__

No thanks, please use dev_err().

> +
>  #include <linux/platform_device.h>
> +#include <linux/idr.h>
>  #include <linux/init.h>
> +#include <linux/interrupt.h>
>  #include <linux/cpumask.h>
>  #include <linux/export.h>
>  #include <linux/dma-mapping.h>
> @@ -13,10 +17,13 @@
>  #include <linux/types.h>
>  #include <linux/qcom_scm.h>
>  #include <linux/of.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_address.h>
>  #include <linux/of_platform.h>
>  #include <linux/clk.h>
>  #include <linux/reset-controller.h>
> +#include <linux/spinlock.h>
> +#include <linux/ktime.h>
>  #include <linux/arm-smccc.h>
>  
>  #include "qcom_scm.h"
> @@ -30,6 +37,12 @@ bool qcom_scm_allow_multicall = false;
>  #define SCM_HAS_IFACE_CLK	BIT(1)
>  #define SCM_HAS_BUS_CLK		BIT(2)
>  
> +struct qcom_scm_waitq {
> +	struct idr idr;
> +	spinlock_t idr_lock;
> +	struct work_struct scm_irq_work;
> +};
> +
>  struct qcom_scm {
>  	struct device *dev;
>  	struct clk *core_clk;
> @@ -37,6 +50,7 @@ struct qcom_scm {
>  	struct clk *bus_clk;
>  	struct icc_path *path;
>  	struct reset_controller_dev reset;
> +	struct qcom_scm_waitq waitq;
>  
>  	/* control access to the interconnect path */
>  	struct mutex scm_bw_lock;
> @@ -62,10 +76,14 @@ struct qcom_scm_mem_map_info {
>  static const u8 qcom_scm_cpu_cold_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
>  	0, BIT(0), BIT(3), BIT(5)
>  };
> +

This seems unrelated.

>  static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
>  	BIT(2), BIT(1), BIT(4), BIT(6)
>  };
>  
> +#define QCOM_SMC_WAITQ_FLAG_WAKE_ONE	BIT(0)
> +#define QCOM_SMC_WAITQ_FLAG_WAKE_ALL	BIT(1)
> +
>  static const char * const qcom_scm_convention_names[] = {
>  	[SMC_CONVENTION_UNKNOWN] = "unknown",
>  	[SMC_CONVENTION_ARM_32] = "smc arm 32",
> @@ -1328,11 +1346,92 @@ bool qcom_scm_is_available(void)
>  }
>  EXPORT_SYMBOL(qcom_scm_is_available);
>  
> +struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)
> +{
> +	struct completion *wq = NULL;
> +	u32 wq_ctx_idr = wq_ctx;
> +	unsigned long flags;
> +	int err;
> +
> +	spin_lock_irqsave(&scm->waitq.idr_lock, flags);
> +	wq = idr_find(&scm->waitq.idr, wq_ctx);
> +	if (wq)
> +		goto out;
> +
> +	wq = devm_kzalloc(scm->dev, sizeof(*wq), GFP_ATOMIC);

I don't know how the wq_ctx are picked by the secure size, but as
written here there's no guarantee that this won't consume all system
memory after some time.


Is the worst-case number of requests waiting really bad enough that we
need an advanced data structure here?

How about just having a linked list with all currently waiting processes
and then loop over that in scm_waitq_flag_handler() to wake them up
selectively?

> +	if (!wq) {
> +		wq = ERR_PTR(-ENOMEM);
> +		goto out;
> +	}
> +
> +	init_completion(wq);
> +
> +	err = idr_alloc_u32(&scm->waitq.idr, wq, &wq_ctx_idr,
> +			    U32_MAX, GFP_ATOMIC);
> +	if (err < 0) {
> +		devm_kfree(scm->dev, wq);
> +		wq = ERR_PTR(err);
> +	}
> +
> +out:
> +	spin_unlock_irqrestore(&scm->waitq.idr_lock, flags);
> +	return wq;
> +}
> +
> +void scm_waitq_flag_handler(struct completion *wq, u32 flags)

This is indeed handling the waitq_flag, but I find that it would be
cleaner if this was:

void qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx, bool wake_all)

> +{
> +	switch (flags) {
> +	case QCOM_SMC_WAITQ_FLAG_WAKE_ONE:
> +		complete(wq);
> +		break;
> +	case QCOM_SMC_WAITQ_FLAG_WAKE_ALL:
> +		complete_all(wq);
> +		break;
> +	default:
> +		pr_err("invalid flags: %u\n", flags);
> +	}
> +}
> +
> +static void scm_irq_work(struct work_struct *work)
> +{
> +	int ret;
> +	u32 wq_ctx, flags, more_pending = 0;
> +	struct completion *wq_to_wake;
> +	struct qcom_scm_waitq *w = container_of(work, struct qcom_scm_waitq, scm_irq_work);
> +	struct qcom_scm *scm = container_of(w, struct qcom_scm, waitq);
> +
> +	do {
> +		ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
> +		if (ret) {
> +			pr_err("GET_WQ_CTX SMC call failed: %d\n", ret);

You have scm->dev, so use dev_err().

> +			return;
> +		}
> +
> +		wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
> +		if (IS_ERR_OR_NULL(wq_to_wake)) {
> +			pr_err("No waitqueue found for wq_ctx %d: %ld\n",
> +					wq_ctx, PTR_ERR(wq_to_wake));
> +			return;
> +		}
> +
> +		scm_waitq_flag_handler(wq_to_wake, flags);
> +	} while (more_pending);
> +}
> +
> +static irqreturn_t qcom_scm_irq_handler(int irq, void *p)
> +{
> +	struct qcom_scm *scm = p;
> +
> +	schedule_work(&scm->waitq.scm_irq_work);

All the worker does is to query and wake up sleeping processes, why
can't this be done here in the threaded irq handler directly?

> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int qcom_scm_probe(struct platform_device *pdev)
>  {
>  	struct qcom_scm *scm;
>  	unsigned long clks;
> -	int ret;
> +	int irq, ret;
>  
>  	scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
>  	if (!scm)
> @@ -1402,12 +1501,29 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	platform_set_drvdata(pdev, scm);

I'm not able to figure out why this is needed, please drop if possible.

> +
>  	__scm = scm;
>  	__scm->dev = &pdev->dev;
>  
> +	spin_lock_init(&__scm->waitq.idr_lock);
> +	idr_init(&__scm->waitq.idr);
>  	qcom_scm_allow_multicall = of_property_read_bool(__scm->dev->of_node,
>  							"allow-multi-call");
>  
> +	INIT_WORK(&__scm->waitq.scm_irq_work, scm_irq_work);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq) {
> +		ret = devm_request_threaded_irq(__scm->dev, irq, NULL,
> +			qcom_scm_irq_handler, IRQF_ONESHOT, "qcom-scm", __scm);
> +		if (ret < 0) {
> +			pr_err("Failed to request qcom-scm irq: %d\n", ret);

You have scm->dev, please use it in dev_err().

> +			idr_destroy(&__scm->waitq.idr);
> +			return ret;
> +		}
> +	}
> +
>  	__get_convention();
>  
>  	/*
> @@ -1423,6 +1539,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  
>  static void qcom_scm_shutdown(struct platform_device *pdev)
>  {
> +	idr_destroy(&__scm->waitq.idr);

You can still have interrupts coming in and work scheduled, that will
use the idr. You need to ensure that can't happen.

Regards,
Bjorn

>  	/* Clean shutdown, disable download mode to allow normal restart */
>  	if (download_mode)
>  		qcom_scm_set_download_mode(false);
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index c0a4d6b..ae3a331 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -62,6 +62,11 @@ struct qcom_scm_res {
>  	u64 result[MAX_QCOM_SCM_RETS];
>  };
>  
> +struct qcom_scm;
> +extern struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx);
> +extern void scm_waitq_flag_handler(struct completion *wq, u32 flags);
> +extern int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending);
> +
>  #define SCM_SMC_FNID(s, c)	((((s) & 0xFF) << 8) | ((c) & 0xFF))
>  extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  			  enum qcom_scm_convention qcom_convention,
> @@ -131,6 +136,11 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
>  #define QCOM_SCM_SMMU_CONFIG_ERRATA1		0x03
>  #define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL	0x02
>  
> +#define QCOM_SCM_SVC_WAITQ			0x24
> +#define QCOM_SCM_WAITQ_ACK			0x01
> +#define QCOM_SCM_WAITQ_RESUME			0x02
> +#define QCOM_SCM_WAITQ_GET_WQ_CTX		0x03
> +
>  extern void __qcom_scm_init(void);
>  
>  /* common error codes */
> @@ -141,6 +151,8 @@ extern void __qcom_scm_init(void);
>  #define QCOM_SCM_EINVAL_ARG	-2
>  #define QCOM_SCM_ERROR		-1
>  #define QCOM_SCM_INTERRUPTED	1
> +#define QCOM_SCM_WAITQ_SLEEP	2
> +#define QCOM_SCM_WAITQ_WAKE	3
>  
>  static inline int qcom_scm_remap_error(int err)
>  {
> -- 
> 2.7.4
> 

[RESEND PATCH v2 5/5] firmware: qcom: scm: Add wait-queue handling logic

[Guru Das Srinagesh]

Add logic to handle QCOM_SCM_WAITQ_SLEEP or QCOM_SCM_WAITQ_WAKE return
codes.

Scenario 1: Requests made by 2 different VMs:

  VM_1                     VM_2                            Firmware
    │                        │                                 │
    │                        │                                 │
    │                        │                                 │
    │                        │                                 │
    │      REQUEST_1         │                                 │
    ├────────────────────────┼─────────────────────────────────┤
    │                        │                                 │
    │                        │                              ┌──┼──┐
    │                        │                              │  │  │
    │                        │     REQUEST_2                │  │  │
    │                        ├──────────────────────────────┼──┤  │
    │                        │                              │  │  │Resource
    │                        │                              │  │  │is busy
    │                        │       {WQ_SLEEP}             │  │  │
    │                        │◄─────────────────────────────┼──┤  │
    │                        │  wq_ctx, smc_call_ctx        │  │  │
    │                        │                              └──┼──┘
    │   REQUEST_1 COMPLETE   │                                 │
    │◄───────────────────────┼─────────────────────────────────┤
    │                        │                                 │
    │                        │         IRQ                     │
    │                        │◄─-------------------------------│
    │                        │                                 │
    │                        │      get_wq_ctx()               │
    │                        ├────────────────────────────────►│
    │                        │                                 │
    │                        │                                 │
    │                        │◄────────────────────────────────┤
    │                        │   wq_ctx, flags, and            │
    │                        │        more_pending             │
    │                        │                                 │
    │                        │                                 │
    │                        │ wq_resume(smc_call_ctx)         │
    │                        ├────────────────────────────────►│
    │                        │                                 │
    │                        │                                 │
    │                        │      REQUEST_2 COMPLETE         │
    │                        │◄────────────────────────────────┤
    │                        │                                 │
    │                        │                                 │

Scenario 2: Two Requests coming in from same VM:

  VM_1                                                     Firmware
    │                                                          │
    │                                                          │
    │                                                          │
    │                                                          │
    │      REQUEST_1                                           │
    ├──────────────────────────────────────────────────────────┤
    │                                                          │
    │                                                     ┌────┼───┐
    │                                                     │    │   │
    │                                                     │    │   │
    │                                                     │    │   │
    │      REQUEST_2                                      │    │   │
    ├─────────────────────────────────────────────────────┼───►│   │
    │                                                     │    │   │Resource
    │                                                     │    │   │is busy
    │      {WQ_SLEEP}                                     │    │   │
    │◄────────────────────────────────────────────────────┼────┤   │
    │      wq_ctx, req2_smc_call_ctx                      │    │   │
    │                                                     │    │   │
    │                                                     └────┼───┘
    │                                                          │
    │      {WQ_WAKE}                                           │
    │◄─────────────────────────────────────────────────────────┤
    │      wq_ctx, req1_smc_call_ctx, flags                    │
    │                                                          │
    │                                                          │
    │      wq_wake_ack(req1_smc_call_ctx)                      │
    ├─────────────────────────────────────────────────────────►│
    │                                                          │
    │      REQUEST_1 COMPLETE                                  │
    │◄─────────────────────────────────────────────────────────┤
    │                                                          │
    │                                                          │
    │      wq_resume(req_2_smc_call_ctx)                       │
    ├─────────────────────────────────────────────────────────►│
    │                                                          │
    │      REQUEST_2 COMPLETE                                  │
    │◄─────────────────────────────────────────────────────────┤
    │                                                          │

With the exception of get_wq_ctx(), the other two newly-introduced SMC
calls, wq_ack() and wq_resume() can themselves return WQ_SLEEP (these
nested rounds of WQ_SLEEP are not shown in the above diagram for the
sake of simplicity). Therefore, introduce a new do-while loop to handle
multiple WQ_SLEEP return values for the same parent SCM call.

Request Completion in the above diagram refers to either a success
return value (zero) or error (and not SMC_WAITQ_SLEEP or
SMC_WAITQ_WAKE).

Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
---
 drivers/firmware/qcom_scm-smc.c | 76 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 69 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
index 4150da1..09cca48 100644
--- a/drivers/firmware/qcom_scm-smc.c
+++ b/drivers/firmware/qcom_scm-smc.c
@@ -53,6 +53,9 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
 	} while (res->a0 == QCOM_SCM_INTERRUPTED);
 }
 
+#define IS_WAITQ_SLEEP_OR_WAKE(res) \
+	(res->a0 == QCOM_SCM_WAITQ_SLEEP || res->a0 == QCOM_SCM_WAITQ_WAKE)
+
 static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
 {
 	memset(resume->args, 0, ARRAY_SIZE(resume->args));
@@ -109,25 +112,77 @@ int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending)
 	return 0;
 }
 
-static void __scm_smc_do(const struct arm_smccc_args *smc,
+static int scm_smc_do_quirk(struct device *dev, struct arm_smccc_args *smc,
+			    struct arm_smccc_res *res)
+{
+	struct completion *wq = NULL;
+	struct qcom_scm *qscm;
+	u32 wq_ctx, smc_call_ctx, flags;
+
+	do {
+		__scm_smc_do_quirk(smc, res);
+
+		if (IS_WAITQ_SLEEP_OR_WAKE(res)) {
+			wq_ctx = res->a1;
+			smc_call_ctx = res->a2;
+			flags = res->a3;
+
+			if (!dev)
+				return -EPROBE_DEFER;
+
+			qscm = dev_get_drvdata(dev);
+			wq = qcom_scm_lookup_wq(qscm, wq_ctx);
+			if (IS_ERR_OR_NULL(wq)) {
+				pr_err("No waitqueue found for wq_ctx %d: %ld\n",
+						wq_ctx, PTR_ERR(wq));
+				return PTR_ERR(wq);
+			}
+
+			if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
+				wait_for_completion(wq);
+				fill_wq_resume_args(smc, smc_call_ctx);
+				wq = NULL;
+				continue;
+			} else {
+				fill_wq_wake_ack_args(smc, smc_call_ctx);
+				continue;
+			}
+		} else if (!res->a0 || (long)res->a0 < 0) {
+			/*
+			 * Success, or error.
+			 * wq will be set only if a prior WAKE happened.
+			 * Its value will be the one from the prior WAKE.
+			 */
+			if (wq)
+				scm_waitq_flag_handler(wq, flags);
+			break;
+		}
+	} while (IS_WAITQ_SLEEP_OR_WAKE(res));
+
+	return 0;
+}
+
+static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
 			 struct arm_smccc_res *res, bool atomic)
 {
-	int retry_count = 0;
+	int ret, retry_count = 0;
 
 	if (atomic) {
 		__scm_smc_do_quirk(smc, res);
-		return;
+		return 0;
 	}
 
 	do {
 		if (!qcom_scm_allow_multicall)
 			mutex_lock(&qcom_scm_lock);
 
-		__scm_smc_do_quirk(smc, res);
+		ret = scm_smc_do_quirk(dev, smc, res);
 
 		if (!qcom_scm_allow_multicall)
 			mutex_unlock(&qcom_scm_lock);
 
+		if (ret)
+			return ret;
 
 		if (res->a0 == QCOM_SCM_V2_EBUSY) {
 			if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
@@ -135,6 +190,8 @@ static void __scm_smc_do(const struct arm_smccc_args *smc,
 			msleep(QCOM_SCM_EBUSY_WAIT_MS);
 		}
 	}  while (res->a0 == QCOM_SCM_V2_EBUSY);
+
+	return 0;
 }
 
 
@@ -143,7 +200,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 		   struct qcom_scm_res *res, bool atomic)
 {
 	int arglen = desc->arginfo & 0xf;
-	int i;
+	int i, ret;
 	dma_addr_t args_phys = 0;
 	void *args_virt = NULL;
 	size_t alloc_len;
@@ -195,19 +252,24 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 		smc.args[SCM_SMC_LAST_REG_IDX] = args_phys;
 	}
 
-	__scm_smc_do(&smc, &smc_res, atomic);
+	ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
+	/* ret error check follows after args_virt cleanup*/
 
 	if (args_virt) {
 		dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE);
 		kfree(args_virt);
 	}
 
+	if (ret)
+		return ret;
+
 	if (res) {
 		res->result[0] = smc_res.a1;
 		res->result[1] = smc_res.a2;
 		res->result[2] = smc_res.a3;
 	}
 
-	return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
+	ret = (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
 
+	return ret;
 }
-- 
2.7.4

Re: [RESEND PATCH v2 5/5] firmware: qcom: scm: Add wait-queue handling logic

[Bjorn Andersson]

On Tue, Aug 30, 2022 at 03:25:11PM -0700, Guru Das Srinagesh wrote:
> Add logic to handle QCOM_SCM_WAITQ_SLEEP or QCOM_SCM_WAITQ_WAKE return
> codes.
> 
> Scenario 1: Requests made by 2 different VMs:
> 
>   VM_1                     VM_2                            Firmware
>     │                        │                                 │
>     │                        │                                 │
>     │                        │                                 │
>     │                        │                                 │
>     │      REQUEST_1         │                                 │
>     ├────────────────────────┼─────────────────────────────────┤
>     │                        │                                 │
>     │                        │                              ┌──┼──┐
>     │                        │                              │  │  │
>     │                        │     REQUEST_2                │  │  │
>     │                        ├──────────────────────────────┼──┤  │
>     │                        │                              │  │  │Resource
>     │                        │                              │  │  │is busy
>     │                        │       {WQ_SLEEP}             │  │  │
>     │                        │◄─────────────────────────────┼──┤  │
>     │                        │  wq_ctx, smc_call_ctx        │  │  │
>     │                        │                              └──┼──┘
>     │   REQUEST_1 COMPLETE   │                                 │
>     │◄───────────────────────┼─────────────────────────────────┤
>     │                        │                                 │
>     │                        │         IRQ                     │
>     │                        │◄─-------------------------------│
>     │                        │                                 │
>     │                        │      get_wq_ctx()               │
>     │                        ├────────────────────────────────►│
>     │                        │                                 │
>     │                        │                                 │
>     │                        │◄────────────────────────────────┤
>     │                        │   wq_ctx, flags, and            │
>     │                        │        more_pending             │
>     │                        │                                 │
>     │                        │                                 │
>     │                        │ wq_resume(smc_call_ctx)         │
>     │                        ├────────────────────────────────►│
>     │                        │                                 │
>     │                        │                                 │
>     │                        │      REQUEST_2 COMPLETE         │
>     │                        │◄────────────────────────────────┤
>     │                        │                                 │
>     │                        │                                 │
> 
> Scenario 2: Two Requests coming in from same VM:
> 
>   VM_1                                                     Firmware
>     │                                                          │
>     │                                                          │
>     │                                                          │
>     │                                                          │
>     │      REQUEST_1                                           │
>     ├──────────────────────────────────────────────────────────┤
>     │                                                          │
>     │                                                     ┌────┼───┐
>     │                                                     │    │   │
>     │                                                     │    │   │
>     │                                                     │    │   │
>     │      REQUEST_2                                      │    │   │
>     ├─────────────────────────────────────────────────────┼───►│   │
>     │                                                     │    │   │Resource
>     │                                                     │    │   │is busy
>     │      {WQ_SLEEP}                                     │    │   │
>     │◄────────────────────────────────────────────────────┼────┤   │
>     │      wq_ctx, req2_smc_call_ctx                      │    │   │
>     │                                                     │    │   │
>     │                                                     └────┼───┘
>     │                                                          │
>     │      {WQ_WAKE}                                           │
>     │◄─────────────────────────────────────────────────────────┤
>     │      wq_ctx, req1_smc_call_ctx, flags                    │
>     │                                                          │
>     │                                                          │
>     │      wq_wake_ack(req1_smc_call_ctx)                      │
>     ├─────────────────────────────────────────────────────────►│
>     │                                                          │
>     │      REQUEST_1 COMPLETE                                  │
>     │◄─────────────────────────────────────────────────────────┤
>     │                                                          │
>     │                                                          │
>     │      wq_resume(req_2_smc_call_ctx)                       │
>     ├─────────────────────────────────────────────────────────►│
>     │                                                          │
>     │      REQUEST_2 COMPLETE                                  │
>     │◄─────────────────────────────────────────────────────────┤
>     │                                                          │
> 
> With the exception of get_wq_ctx(), the other two newly-introduced SMC
> calls, wq_ack() and wq_resume() can themselves return WQ_SLEEP (these
> nested rounds of WQ_SLEEP are not shown in the above diagram for the
> sake of simplicity). Therefore, introduce a new do-while loop to handle
> multiple WQ_SLEEP return values for the same parent SCM call.
> 
> Request Completion in the above diagram refers to either a success
> return value (zero) or error (and not SMC_WAITQ_SLEEP or
> SMC_WAITQ_WAKE).
> 
> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
> ---
>  drivers/firmware/qcom_scm-smc.c | 76 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 69 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
> index 4150da1..09cca48 100644
> --- a/drivers/firmware/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom_scm-smc.c
> @@ -53,6 +53,9 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
>  	} while (res->a0 == QCOM_SCM_INTERRUPTED);
>  }
>  
> +#define IS_WAITQ_SLEEP_OR_WAKE(res) \
> +	(res->a0 == QCOM_SCM_WAITQ_SLEEP || res->a0 == QCOM_SCM_WAITQ_WAKE)
> +
>  static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
>  {
>  	memset(resume->args, 0, ARRAY_SIZE(resume->args));
> @@ -109,25 +112,77 @@ int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending)
>  	return 0;
>  }
>  
> -static void __scm_smc_do(const struct arm_smccc_args *smc,
> +static int scm_smc_do_quirk(struct device *dev, struct arm_smccc_args *smc,

There was a reasoning behind using the somewhat cryptic name
__scm_smc_do_quirk(), but wrapping it in a function with the same name
without the underscores is probably just going to confuse the reader
even more.

How about __scm_smc_do_quirk_handle_waitq() ?

> +			    struct arm_smccc_res *res)
> +{
> +	struct completion *wq = NULL;
> +	struct qcom_scm *qscm;
> +	u32 wq_ctx, smc_call_ctx, flags;
> +
> +	do {
> +		__scm_smc_do_quirk(smc, res);
> +
> +		if (IS_WAITQ_SLEEP_OR_WAKE(res)) {
> +			wq_ctx = res->a1;
> +			smc_call_ctx = res->a2;
> +			flags = res->a3;
> +
> +			if (!dev)
> +				return -EPROBE_DEFER;
> +
> +			qscm = dev_get_drvdata(dev);
> +			wq = qcom_scm_lookup_wq(qscm, wq_ctx);
> +			if (IS_ERR_OR_NULL(wq)) {
> +				pr_err("No waitqueue found for wq_ctx %d: %ld\n",

You have dev here, use dev_err()

> +						wq_ctx, PTR_ERR(wq));
> +				return PTR_ERR(wq);
> +			}
> +
> +			if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
> +				wait_for_completion(wq);
> +				fill_wq_resume_args(smc, smc_call_ctx);
> +				wq = NULL;
> +				continue;

This is the last statement before the condition is evaluated again, so
this continue should be a nop.

> +			} else {

The use of IS_WAITQ_SLEEP_OR_WAKE() hides the fact that a0 can only be
QCOM_SCM_WAITQ_WAKE here. I think you should make it explicit with:
			} else if (res->a0 == QCOM_SCM_WAITQ_WAKE) {

> +				fill_wq_wake_ack_args(smc, smc_call_ctx);
> +				continue;
> +			}
> +		} else if (!res->a0 || (long)res->a0 < 0) {
> +			/*
> +			 * Success, or error.
> +			 * wq will be set only if a prior WAKE happened.
> +			 * Its value will be the one from the prior WAKE.
> +			 */
> +			if (wq)
> +				scm_waitq_flag_handler(wq, flags);
> +			break;

The next thing that will happen is that the condition in the loop will
be evaluated and the loop will be broken. So you don't need this break.


It's also not entirely obvious that the first half of this loop will
acquire the wq and always loop, so that at some point the second part
will run to wake up the wq - right before leaving the loop.

I think it would be easier to reason about if you move this chunk below
the loop.

do {
	__scm_smc_do_quirk();
	if (IS_WAITQ_SLEEP_OR_WAKE()) {
		...
	}
while (IS_WAITQ_SLEEP_OR_WAKE());

if (wq)
	scm_waitq_flag_handler(wq, flags);

return 0;



If you also make sure to only set "wq" in the QCOM_SCM_WAITQ_WAKE case
you will make it clearer that the if (!res->a0) is there to capture the
case where we acked the wakeup and the ack was successful.


One question though, if the wakeup ack fails, should we still wake up
the requested wq? Perhaps it doesn't matter? As the woken up wq would
just face a QCOM_SCM_WAITQ_SLEEP again?

> +		}
> +	} while (IS_WAITQ_SLEEP_OR_WAKE(res));
> +
> +	return 0;
> +}
> +
> +static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
>  			 struct arm_smccc_res *res, bool atomic)
>  {
> -	int retry_count = 0;
> +	int ret, retry_count = 0;
>  
>  	if (atomic) {
>  		__scm_smc_do_quirk(smc, res);
> -		return;
> +		return 0;
>  	}
>  
>  	do {
>  		if (!qcom_scm_allow_multicall)
>  			mutex_lock(&qcom_scm_lock);
>  
> -		__scm_smc_do_quirk(smc, res);
> +		ret = scm_smc_do_quirk(dev, smc, res);
>  
>  		if (!qcom_scm_allow_multicall)
>  			mutex_unlock(&qcom_scm_lock);
>  
> +		if (ret)
> +			return ret;
>  
>  		if (res->a0 == QCOM_SCM_V2_EBUSY) {
>  			if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
> @@ -135,6 +190,8 @@ static void __scm_smc_do(const struct arm_smccc_args *smc,
>  			msleep(QCOM_SCM_EBUSY_WAIT_MS);
>  		}
>  	}  while (res->a0 == QCOM_SCM_V2_EBUSY);
> +
> +	return 0;
>  }
>  
>  
> @@ -143,7 +200,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  		   struct qcom_scm_res *res, bool atomic)
>  {
>  	int arglen = desc->arginfo & 0xf;
> -	int i;
> +	int i, ret;
>  	dma_addr_t args_phys = 0;
>  	void *args_virt = NULL;
>  	size_t alloc_len;
> @@ -195,19 +252,24 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  		smc.args[SCM_SMC_LAST_REG_IDX] = args_phys;
>  	}
>  
> -	__scm_smc_do(&smc, &smc_res, atomic);
> +	ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
> +	/* ret error check follows after args_virt cleanup*/
>  
>  	if (args_virt) {
>  		dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE);
>  		kfree(args_virt);
>  	}
>  
> +	if (ret)
> +		return ret;
> +
>  	if (res) {
>  		res->result[0] = smc_res.a1;
>  		res->result[1] = smc_res.a2;
>  		res->result[2] = smc_res.a3;
>  	}
>  
> -	return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
> +	ret = (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;

This is not necessary, right?

Regards,
Bjorn

>  
> +	return ret;
>  }
> -- 
> 2.7.4
>