[V7,2/2] firmware: qcom: scm: Add wait-queue handling logic

  From: Guru Das Srinagesh <quic_gurus@quicinc.com>

When the firmware (FW) supports multiple requests per VM, multiple requests
from the same/different VM can 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 a new return
value in addition to success or error: SCM_WAITQ_SLEEP. A sleeping SCM call
can be woken up by an interrupt that the FW raises.

  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.

	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".

The handshake mechanism that HLOS uses to talk to FW about wait-queue
operations involves two new SMC calls.

  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.

(The mechanism to wake a SMC call back up is described in detail below)

 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         │
   │                        │◄────────────────────────────────┤
   │                        │                                 │
   │                        │                                 │

With the exception of get_wq_ctx(), the other SMC call wq_resume() can
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)

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

Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---

v7:
- Move lookup + wait_for_completion into a single function in qcom_scm [Bjorn]
- Simplify completion retrieval [Bjorn]

v6:
- Fix misc. nits in the scm driver [Krzysztof]

v5:
- Handle the wake_one/wake_all flags [Guru]
- Rename flag handler to qcom_scm_waitq_wakeup [Bjorn]
- Resume scm call can return ebusy as well handle that scenario by retrying
  the original smc call and not the resume call

v4:
- platform_set_drvdata will be used by __scm_smc_do_quirk_handle_waitq to
  get access to scm struct from device so retain it
- Use a single completion as it satisfies all of the current usecases [Bjorn]
- Inline scm_get_wq_ctx [Bjorn]
- Convert all pr_err to dev_err [Bjorn]
- Handle idr_destroy in a thread safe manner [Bjorn]
- Misc. Style fixes [Bjorn]

v3:
- Fixup irq handling so as not to affect SoCs without the interrupt.
- Fix warnings reported by kernel test-bot.

 drivers/firmware/qcom_scm-smc.c | 90 ++++++++++++++++++++++++++++++---
 drivers/firmware/qcom_scm.c     | 89 +++++++++++++++++++++++++++++++-
 drivers/firmware/qcom_scm.h     |  8 +++
 3 files changed, 179 insertions(+), 8 deletions(-)
  

On Jan 10 2023 12:07, Sibi Sankar wrote:

...

> +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq,
> +					   struct arm_smccc_res *res)
> +{
> +	int ret;
> +	struct arm_smccc_args resume;
> +	u32 wq_ctx, smc_call_ctx, flags;
> +	struct arm_smccc_args *smc = waitq;
> +
> +	do {
> +		__scm_smc_do_quirk(smc, res);
> +
> +		if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
> +			wq_ctx = res->a1;
> +			smc_call_ctx = res->a2;
> +			flags = res->a3;
> +
> +			if (!dev)
> +				return -EPROBE_DEFER;
> +
> +			ret = qcom_scm_lookup_completion(wq_ctx);

I see that this function has been created in response to Bjorn's comment [1]
about avoiding the dev_get_drvdata() call, but I would prefer to not use this
function as it hides the fact that the wait_for_completion() is occurring here.

Knowing where the waiting is happening is useful not just for understanding
code flow but also for debugging issues in the future.

...

> +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)
> +{

This function is called qcom_scm_lookup_wq() but there is no looking up
occurring here. Could this comment be added for context?

/* 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.
 */

> +	/* assert wq_ctx is zero */
> +	if (wq_ctx != 0) {
> +		dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return &scm->waitq_comp;
> +}
> +
...

[1] https://lore.kernel.org/lkml/20221208221125.bflo7unhcrgfsgbr@builder.lan/
  

Hi Sibi,

Few minor comments below,

On 10/01/2023 06:37, Sibi Sankar wrote:
> From: Guru Das Srinagesh <quic_gurus@quicinc.com>
> 
> When the firmware (FW) supports multiple requests per VM, multiple requests
> from the same/different VM can 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 a new return
> value in addition to success or error: SCM_WAITQ_SLEEP. A sleeping SCM call
> can be woken up by an interrupt that the FW raises.
> 
...

>   drivers/firmware/qcom_scm-smc.c | 90 ++++++++++++++++++++++++++++++---
>   drivers/firmware/qcom_scm.c     | 89 +++++++++++++++++++++++++++++++-
>   drivers/firmware/qcom_scm.h     |  8 +++
>   3 files changed, 179 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
> index d111833364ba..30999f04749c 100644
> --- a/drivers/firmware/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom_scm-smc.c
...
> +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq,
> +					   struct arm_smccc_res *res)
> +{
> +	int ret;
> +	struct arm_smccc_args resume;
> +	u32 wq_ctx, smc_call_ctx, flags;
> +	struct arm_smccc_args *smc = waitq;
> +
> +	do {
> +		__scm_smc_do_quirk(smc, res);
> +
> +		if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
> +			wq_ctx = res->a1;
> +			smc_call_ctx = res->a2;
> +			flags = res->a3;
> +
> +			if (!dev)
> +				return -EPROBE_DEFER;

why are we checking dev pointer in the middle of the call?
A comment here would really help readers.

> +
> +			ret = qcom_scm_lookup_completion(wq_ctx);
> +			if (ret)
> +				return ret;
> +
> +			fill_wq_resume_args(&resume, smc_call_ctx);
> +			smc = &resume;
> +		}
> +	} while (res->a0 == QCOM_SCM_WAITQ_SLEEP);
> +
> +	return 0;
> +}
> +
...
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index cdbfe54c8146..19ac506a9b1f 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -4,6 +4,7 @@
>    */
>   #include <linux/platform_device.h>
>   #include <linux/init.h>
> +#include <linux/interrupt.h>
>   #include <linux/cpumask.h>
>   #include <linux/export.h>
>   #include <linux/dma-mapping.h>
> @@ -13,6 +14,7 @@
>   #include <linux/qcom_scm.h>
>   #include <linux/of.h>
>   #include <linux/of_address.h>
> +#include <linux/of_irq.h>
>   #include <linux/of_platform.h>
>   #include <linux/clk.h>
>   #include <linux/reset-controller.h>

include <linux/completion.h> ??


> @@ -33,6 +35,7 @@ struct qcom_scm {
>   	struct clk *iface_clk;
>   	struct clk *bus_clk;
>   	struct icc_path *path;
> +	struct completion waitq_comp;
>   	struct reset_controller_dev reset;
>   
>   	/* control access to the interconnect path */
> @@ -63,6 +66,9 @@ 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",
> @@ -1325,11 +1331,79 @@ bool qcom_scm_is_available(void)
>   }
>   EXPORT_SYMBOL(qcom_scm_is_available);
>   
> +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)
> +{
> +	/* assert wq_ctx is zero */ > +	if (wq_ctx != 0) {

Is this correct? looks like zero is the only valid one.

I thought wq_ctx was a unique number (UID).

> +		dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return &scm->waitq_comp;
> +}
> +
> +int qcom_scm_lookup_completion(u32 wq_ctx)
> +{
> +	struct completion *wq = NULL;
> +
> +	wq = qcom_scm_lookup_wq(__scm, wq_ctx);
> +	if (IS_ERR(wq))
> +		return PTR_ERR(wq);
> +
> +	wait_for_completion(wq);

We can potentially block here forever without a timeout.

As you are reusing completion, I have not seen any reinitialization of 
completion, this could potentially return above line without waiting at all.

> +
> +	return 0;
> +}
> +
> +static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx, bool wake_all)
> +{
> +	struct completion *wq_to_wake;
> +
> +	wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
> +	if (IS_ERR(wq_to_wake))
> +		return PTR_ERR(wq_to_wake);
> +
> +	if (wake_all)
> +		complete_all(wq_to_wake);
> +	else
> +		complete(wq_to_wake);

> +
> +	return 0;
> +}
> +
> +static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
> +{
> +	int ret;
> +	struct qcom_scm *scm = data;
> +	u32 wq_ctx, flags, more_pending = 0;
> +
> +	do {
> +		ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
> +		if (ret) {
> +			dev_err(scm->dev, "GET_WQ_CTX SMC call failed: %d\n", ret);
> +			goto out;
> +		}
> +
> +		if (flags != QCOM_SMC_WAITQ_FLAG_WAKE_ONE &&
> +		    flags != QCOM_SMC_WAITQ_FLAG_WAKE_ALL) {
> +			dev_err(scm->dev, "Invalid flags found for wq_ctx: %u\n", flags);
> +			goto out;
> +		}
> +
> +		ret = qcom_scm_waitq_wakeup(scm, wq_ctx, !!(flags & QCOM_SMC_WAITQ_FLAG_WAKE_ALL));
> +		if (ret)
> +			goto out;
> +	} while (more_pending);
> +
> +out:
> +	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,6 +1476,19 @@ static int qcom_scm_probe(struct platform_device *pdev)
>   	__scm = scm;
>   	__scm->dev = &pdev->dev;
>   
> +	init_completion(&__scm->waitq_comp);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		if (irq != -ENXIO)
> +			return irq;
> +	} else {
> +		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
> +						IRQF_ONESHOT, "qcom-scm", __scm);
> +		if (ret < 0)
> +			return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
> +	}
> +
>   	__get_convention();
>   
>   	/*

--srini
  

On Tue, Jan 10, 2023 at 12:14:11AM -0800, Guru Das Srinagesh wrote:
> On Jan 10 2023 12:07, Sibi Sankar wrote:
> 
> ...
> 
> > +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq,
> > +					   struct arm_smccc_res *res)
> > +{
> > +	int ret;
> > +	struct arm_smccc_args resume;
> > +	u32 wq_ctx, smc_call_ctx, flags;
> > +	struct arm_smccc_args *smc = waitq;
> > +
> > +	do {
> > +		__scm_smc_do_quirk(smc, res);
> > +
> > +		if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
> > +			wq_ctx = res->a1;
> > +			smc_call_ctx = res->a2;
> > +			flags = res->a3;
> > +
> > +			if (!dev)
> > +				return -EPROBE_DEFER;
> > +
> > +			ret = qcom_scm_lookup_completion(wq_ctx);
> 
> I see that this function has been created in response to Bjorn's comment [1]
> about avoiding the dev_get_drvdata() call, but I would prefer to not use this
> function as it hides the fact that the wait_for_completion() is occurring here.
> 

My reasoning here is that I don't want the waiting for the completion
that happen in one part of the driver and the completion happening in a
completely different one.

> Knowing where the waiting is happening is useful not just for understanding
> code flow but also for debugging issues in the future.
> 

Absolutely agree, this should be named to make that obvious to the
reader.

> ...
> 
> > +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)
> > +{
> 
> This function is called qcom_scm_lookup_wq() but there is no looking up
> occurring here. Could this comment be added for context?
> 
> /* 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.
>  */
> 

Agree.

Regards,
Bjorn

> > +	/* assert wq_ctx is zero */
> > +	if (wq_ctx != 0) {
> > +		dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	return &scm->waitq_comp;
> > +}
> > +
> ...
> 
> [1] https://lore.kernel.org/lkml/20221208221125.bflo7unhcrgfsgbr@builder.lan/
  

On Tue, Jan 10, 2023 at 12:07:45PM +0530, Sibi Sankar wrote:
> From: Guru Das Srinagesh <quic_gurus@quicinc.com>
> 
> When the firmware (FW) supports multiple requests per VM, multiple requests
> from the same/different VM can 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 a new return
> value in addition to success or error: SCM_WAITQ_SLEEP. A sleeping SCM call
> can be woken up by an interrupt that the FW raises.
> 
>   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.
> 
> 	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".
> 
> The handshake mechanism that HLOS uses to talk to FW about wait-queue
> operations involves two new SMC calls.
> 
>   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.
> 
> (The mechanism to wake a SMC call back up is described in detail below)
> 
>  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         │
>    │                        │◄────────────────────────────────┤
>    │                        │                                 │
>    │                        │                                 │
> 
> With the exception of get_wq_ctx(), the other SMC call wq_resume() can
> 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)
> 
> Also add the interrupt handler that wakes up a sleeping SCM call.
> 
> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
> Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com>
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
> 
> v7:
> - Move lookup + wait_for_completion into a single function in qcom_scm [Bjorn]
> - Simplify completion retrieval [Bjorn]
> 
> v6:
> - Fix misc. nits in the scm driver [Krzysztof]
> 
> v5:
> - Handle the wake_one/wake_all flags [Guru]
> - Rename flag handler to qcom_scm_waitq_wakeup [Bjorn]
> - Resume scm call can return ebusy as well handle that scenario by retrying
>   the original smc call and not the resume call
> 
> v4:
> - platform_set_drvdata will be used by __scm_smc_do_quirk_handle_waitq to
>   get access to scm struct from device so retain it
> - Use a single completion as it satisfies all of the current usecases [Bjorn]
> - Inline scm_get_wq_ctx [Bjorn]
> - Convert all pr_err to dev_err [Bjorn]
> - Handle idr_destroy in a thread safe manner [Bjorn]
> - Misc. Style fixes [Bjorn]
> 
> v3:
> - Fixup irq handling so as not to affect SoCs without the interrupt.
> - Fix warnings reported by kernel test-bot.
> 
>  drivers/firmware/qcom_scm-smc.c | 90 ++++++++++++++++++++++++++++++---
>  drivers/firmware/qcom_scm.c     | 89 +++++++++++++++++++++++++++++++-
>  drivers/firmware/qcom_scm.h     |  8 +++
>  3 files changed, 179 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
> index d111833364ba..30999f04749c 100644
> --- a/drivers/firmware/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom_scm-smc.c
> @@ -52,29 +52,101 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
>  	} while (res->a0 == QCOM_SCM_INTERRUPTED);
>  }
>  
> -static void __scm_smc_do(const struct arm_smccc_args *smc,
> -			 struct arm_smccc_res *res, bool atomic)
> +static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
>  {
> -	int retry_count = 0;
> +	memset(resume->args, 0, sizeof(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;
> +}
> +
> +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;
> +
> +	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));
> +
> +	/* Guaranteed to return only success or error, no WAITQ_* */
> +	__scm_smc_do_quirk(&get_wq_ctx, &get_wq_res);
> +	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 int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq,
> +					   struct arm_smccc_res *res)
> +{
> +	int ret;
> +	struct arm_smccc_args resume;
> +	u32 wq_ctx, smc_call_ctx, flags;
> +	struct arm_smccc_args *smc = waitq;
> +
> +	do {
> +		__scm_smc_do_quirk(smc, res);
> +
> +		if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
> +			wq_ctx = res->a1;
> +			smc_call_ctx = res->a2;
> +			flags = res->a3;
> +
> +			if (!dev)
> +				return -EPROBE_DEFER;
> +
> +			ret = qcom_scm_lookup_completion(wq_ctx);
> +			if (ret)
> +				return ret;
> +
> +			fill_wq_resume_args(&resume, smc_call_ctx);
> +			smc = &resume;
> +		}
> +	} while (res->a0 == QCOM_SCM_WAITQ_SLEEP);
> +
> +	return 0;
> +}
> +
> +static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
> +			struct arm_smccc_res *res, bool atomic)
> +{
> +	int ret, retry_count = 0;
>  
>  	if (atomic) {
>  		__scm_smc_do_quirk(smc, res);
> -		return;
> +		return 0;
>  	}
>  
>  	do {
>  		mutex_lock(&qcom_scm_lock);
>  
> -		__scm_smc_do_quirk(smc, res);
> +		ret = __scm_smc_do_quirk_handle_waitq(dev, smc, res);
>  
>  		mutex_unlock(&qcom_scm_lock);
>  
> +		if (ret)
> +			return ret;
> +
>  		if (res->a0 == QCOM_SCM_V2_EBUSY) {
>  			if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
>  				break;
>  			msleep(QCOM_SCM_EBUSY_WAIT_MS);
>  		}
>  	}  while (res->a0 == QCOM_SCM_V2_EBUSY);
> +
> +	return 0;
>  }
>  
>  
> @@ -83,7 +155,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;
> @@ -135,13 +207,17 @@ 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 error check follows after args_virt cleanup*/
> +	ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
>  
>  	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;
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index cdbfe54c8146..19ac506a9b1f 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -4,6 +4,7 @@
>   */
>  #include <linux/platform_device.h>
>  #include <linux/init.h>
> +#include <linux/interrupt.h>
>  #include <linux/cpumask.h>
>  #include <linux/export.h>
>  #include <linux/dma-mapping.h>
> @@ -13,6 +14,7 @@
>  #include <linux/qcom_scm.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/clk.h>
>  #include <linux/reset-controller.h>
> @@ -33,6 +35,7 @@ struct qcom_scm {
>  	struct clk *iface_clk;
>  	struct clk *bus_clk;
>  	struct icc_path *path;
> +	struct completion waitq_comp;
>  	struct reset_controller_dev reset;
>  
>  	/* control access to the interconnect path */
> @@ -63,6 +66,9 @@ 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",
> @@ -1325,11 +1331,79 @@ bool qcom_scm_is_available(void)
>  }
>  EXPORT_SYMBOL(qcom_scm_is_available);
>  
> +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)

This could either be seen as a remnant of the older versions of this
patch, or a signalling of "place implementation here". I dislike both...

Please rename it qcom_scm_assert_valid_wq_ctx() and just reference
&scm->waitq_comp directly in the two places below.

> +{
> +	/* assert wq_ctx is zero */
> +	if (wq_ctx != 0) {
> +		dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx);

I think you should be more specific here, perhaps:

	"Firmware unexpectedly passed non-zero wq_ctx\n"

And then as suggested a comment stating what needs to be done when that
happens.

> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return &scm->waitq_comp;
> +}
> +
> +int qcom_scm_lookup_completion(u32 wq_ctx)
> +{
> +	struct completion *wq = NULL;

You assign it on the very next line, no need to do so here.

> +
> +	wq = qcom_scm_lookup_wq(__scm, wq_ctx);
> +	if (IS_ERR(wq))
> +		return PTR_ERR(wq);
> +
> +	wait_for_completion(wq);
> +
> +	return 0;
> +}
> +
> +static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx, bool wake_all)
> +{
> +	struct completion *wq_to_wake;

"wq" would be sufficient.

Thanks,
Bjorn

> +
> +	wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
> +	if (IS_ERR(wq_to_wake))
> +		return PTR_ERR(wq_to_wake);
> +
> +	if (wake_all)
> +		complete_all(wq_to_wake);
> +	else
> +		complete(wq_to_wake);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
> +{
> +	int ret;
> +	struct qcom_scm *scm = data;
> +	u32 wq_ctx, flags, more_pending = 0;
> +
> +	do {
> +		ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
> +		if (ret) {
> +			dev_err(scm->dev, "GET_WQ_CTX SMC call failed: %d\n", ret);
> +			goto out;
> +		}
> +
> +		if (flags != QCOM_SMC_WAITQ_FLAG_WAKE_ONE &&
> +		    flags != QCOM_SMC_WAITQ_FLAG_WAKE_ALL) {
> +			dev_err(scm->dev, "Invalid flags found for wq_ctx: %u\n", flags);
> +			goto out;
> +		}
> +
> +		ret = qcom_scm_waitq_wakeup(scm, wq_ctx, !!(flags & QCOM_SMC_WAITQ_FLAG_WAKE_ALL));
> +		if (ret)
> +			goto out;
> +	} while (more_pending);
> +
> +out:
> +	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,6 +1476,19 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	__scm = scm;
>  	__scm->dev = &pdev->dev;
>  
> +	init_completion(&__scm->waitq_comp);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		if (irq != -ENXIO)
> +			return irq;
> +	} else {
> +		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
> +						IRQF_ONESHOT, "qcom-scm", __scm);
> +		if (ret < 0)
> +			return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
> +	}
> +
>  	__get_convention();
>  
>  	/*
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index db3d08a01209..018e9867d55a 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -60,6 +60,9 @@ struct qcom_scm_res {
>  	u64 result[MAX_QCOM_SCM_RETS];
>  };
>  
> +int qcom_scm_lookup_completion(u32 wq_ctx);
> +int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending);
> +
>  #define SCM_SMC_FNID(s, c)	((((s) & 0xFF) << 8) | ((c) & 0xFF))
>  extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  			  enum qcom_scm_convention qcom_convention,
> @@ -129,6 +132,10 @@ 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_RESUME			0x02
> +#define QCOM_SCM_WAITQ_GET_WQ_CTX		0x03
> +
>  /* common error codes */
>  #define QCOM_SCM_V2_EBUSY	-12
>  #define QCOM_SCM_ENOMEM		-5
> @@ -137,6 +144,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
>  #define QCOM_SCM_EINVAL_ARG	-2
>  #define QCOM_SCM_ERROR		-1
>  #define QCOM_SCM_INTERRUPTED	1
> +#define QCOM_SCM_WAITQ_SLEEP	2
>  
>  static inline int qcom_scm_remap_error(int err)
>  {
> -- 
> 2.17.1
>
  

On Jan 10 2023 11:54, Bjorn Andersson wrote:
> On Tue, Jan 10, 2023 at 12:14:11AM -0800, Guru Das Srinagesh wrote:
> > On Jan 10 2023 12:07, Sibi Sankar wrote:
> > 
> > ...
> > 
> > > +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq,
> > > +					   struct arm_smccc_res *res)
> > > +{
> > > +	int ret;
> > > +	struct arm_smccc_args resume;
> > > +	u32 wq_ctx, smc_call_ctx, flags;
> > > +	struct arm_smccc_args *smc = waitq;
> > > +
> > > +	do {
> > > +		__scm_smc_do_quirk(smc, res);
> > > +
> > > +		if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
> > > +			wq_ctx = res->a1;
> > > +			smc_call_ctx = res->a2;
> > > +			flags = res->a3;
> > > +
> > > +			if (!dev)
> > > +				return -EPROBE_DEFER;
> > > +
> > > +			ret = qcom_scm_lookup_completion(wq_ctx);
> > 
> > I see that this function has been created in response to Bjorn's comment [1]
> > about avoiding the dev_get_drvdata() call, but I would prefer to not use this
> > function as it hides the fact that the wait_for_completion() is occurring here.
> > 
> 
> My reasoning here is that I don't want the waiting for the completion
> that happen in one part of the driver and the completion happening in a
> completely different one.

ACK.

Thank you.

Guru Das.
  

Hey Srini,

Thanks for taking time to review the series.

On 1/10/23 17:44, Srinivas Kandagatla wrote:
> Hi Sibi,
> 
> Few minor comments below,
> 
> On 10/01/2023 06:37, Sibi Sankar wrote:
>> From: Guru Das Srinagesh <quic_gurus@quicinc.com>
>>
>> When the firmware (FW) supports multiple requests per VM, multiple 
>> requests
>> from the same/different VM can 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 a new 
>> return
>> value in addition to success or error: SCM_WAITQ_SLEEP. A sleeping SCM 
>> call
>> can be woken up by an interrupt that the FW raises.
>>
> ...
> 
>>   drivers/firmware/qcom_scm-smc.c | 90 ++++++++++++++++++++++++++++++---
>>   drivers/firmware/qcom_scm.c     | 89 +++++++++++++++++++++++++++++++-
>>   drivers/firmware/qcom_scm.h     |  8 +++
>>   3 files changed, 179 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom_scm-smc.c 
>> b/drivers/firmware/qcom_scm-smc.c
>> index d111833364ba..30999f04749c 100644
>> --- a/drivers/firmware/qcom_scm-smc.c
>> +++ b/drivers/firmware/qcom_scm-smc.c
> ...
>> +static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct 
>> arm_smccc_args *waitq,
>> +                       struct arm_smccc_res *res)
>> +{
>> +    int ret;
>> +    struct arm_smccc_args resume;
>> +    u32 wq_ctx, smc_call_ctx, flags;
>> +    struct arm_smccc_args *smc = waitq;
>> +
>> +    do {
>> +        __scm_smc_do_quirk(smc, res);
>> +
>> +        if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
>> +            wq_ctx = res->a1;
>> +            smc_call_ctx = res->a2;
>> +            flags = res->a3;
>> +
>> +            if (!dev)
>> +                return -EPROBE_DEFER;
> 
> why are we checking dev pointer in the middle of the call?
> A comment here would really help readers.

Given that we no longer use drv_data to pass around scm struct,
the check is no longer required. I'll drop it in the next re-spin.

> 
>> +
>> +            ret = qcom_scm_lookup_completion(wq_ctx);
>> +            if (ret)
>> +                return ret;
>> +
>> +            fill_wq_resume_args(&resume, smc_call_ctx);
>> +            smc = &resume;
>> +        }
>> +    } while (res->a0 == QCOM_SCM_WAITQ_SLEEP);
>> +
>> +    return 0;
>> +}
>> +
> ...
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index cdbfe54c8146..19ac506a9b1f 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -4,6 +4,7 @@
>>    */
>>   #include <linux/platform_device.h>
>>   #include <linux/init.h>
>> +#include <linux/interrupt.h>
>>   #include <linux/cpumask.h>
>>   #include <linux/export.h>
>>   #include <linux/dma-mapping.h>
>> @@ -13,6 +14,7 @@
>>   #include <linux/qcom_scm.h>
>>   #include <linux/of.h>
>>   #include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>>   #include <linux/of_platform.h>
>>   #include <linux/clk.h>
>>   #include <linux/reset-controller.h>
> 
> include <linux/completion.h> ??
> 

ack

> 
>> @@ -33,6 +35,7 @@ struct qcom_scm {
>>       struct clk *iface_clk;
>>       struct clk *bus_clk;
>>       struct icc_path *path;
>> +    struct completion waitq_comp;
>>       struct reset_controller_dev reset;
>>       /* control access to the interconnect path */
>> @@ -63,6 +66,9 @@ 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",
>> @@ -1325,11 +1331,79 @@ bool qcom_scm_is_available(void)
>>   }
>>   EXPORT_SYMBOL(qcom_scm_is_available);
>> +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, 
>> u32 wq_ctx)
>> +{
>> +    /* assert wq_ctx is zero */ > +    if (wq_ctx != 0) {
> 
> Is this correct? looks like zero is the only valid one.
> 
> I thought wq_ctx was a unique number (UID).

Currently the SMC calls from the kernel scm driver are still serialized
and firmware only supports a single wq_ctx. This is expected to change
in the future, will document it the comments.


> 
>> +        dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx);
>> +        return ERR_PTR(-EINVAL);
>> +    }
>> +
>> +    return &scm->waitq_comp;
>> +}
>> +
>> +int qcom_scm_lookup_completion(u32 wq_ctx)
>> +{
>> +    struct completion *wq = NULL;
>> +
>> +    wq = qcom_scm_lookup_wq(__scm, wq_ctx);
>> +    if (IS_ERR(wq))
>> +        return PTR_ERR(wq);
>> +
>> +    wait_for_completion(wq);
> 
> We can potentially block here forever without a timeout.
> 

yeah potentially until a hung task timeout. This is what
we want since we can't make additional scm calls anyway.

> As you are reusing completion, I have not seen any reinitialization of 
> completion, this could potentially return above line without waiting at 
> all.

A complete would paired with a single waiter, so additional
completes would be neccessary for it to go through without
waiting.

> 
>> +
>> +    return 0;
>> +}
>> +
>> +static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int 
>> wq_ctx, bool wake_all)
>> +{
>> +    struct completion *wq_to_wake;
>> +
>> +    wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
>> +    if (IS_ERR(wq_to_wake))
>> +        return PTR_ERR(wq_to_wake);
>> +
>> +    if (wake_all)
>> +        complete_all(wq_to_wake);
>> +    else
>> +        complete(wq_to_wake);
> 
>> +
>> +    return 0;
>> +}
>> +
>> +static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
>> +{
>> +    int ret;
>> +    struct qcom_scm *scm = data;
>> +    u32 wq_ctx, flags, more_pending = 0;
>> +
>> +    do {
>> +        ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
>> +        if (ret) {
>> +            dev_err(scm->dev, "GET_WQ_CTX SMC call failed: %d\n", ret);
>> +            goto out;
>> +        }
>> +
>> +        if (flags != QCOM_SMC_WAITQ_FLAG_WAKE_ONE &&
>> +            flags != QCOM_SMC_WAITQ_FLAG_WAKE_ALL) {
>> +            dev_err(scm->dev, "Invalid flags found for wq_ctx: %u\n", 
>> flags);
>> +            goto out;
>> +        }
>> +
>> +        ret = qcom_scm_waitq_wakeup(scm, wq_ctx, !!(flags & 
>> QCOM_SMC_WAITQ_FLAG_WAKE_ALL));
>> +        if (ret)
>> +            goto out;
>> +    } while (more_pending);
>> +
>> +out:
>> +    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,6 +1476,19 @@ static int qcom_scm_probe(struct 
>> platform_device *pdev)
>>       __scm = scm;
>>       __scm->dev = &pdev->dev;
>> +    init_completion(&__scm->waitq_comp);
>> +
>> +    irq = platform_get_irq(pdev, 0);
>> +    if (irq < 0) {
>> +        if (irq != -ENXIO)
>> +            return irq;
>> +    } else {
>> +        ret = devm_request_threaded_irq(__scm->dev, irq, NULL, 
>> qcom_scm_irq_handler,
>> +                        IRQF_ONESHOT, "qcom-scm", __scm);
>> +        if (ret < 0)
>> +            return dev_err_probe(scm->dev, ret, "Failed to request 
>> qcom-scm irq\n");
>> +    }
>> +
>>       __get_convention();
>>       /*
> 
> --srini
  

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

On 1/10/23 23:37, Bjorn Andersson wrote:
> On Tue, Jan 10, 2023 at 12:07:45PM +0530, Sibi Sankar wrote:
>> From: Guru Das Srinagesh <quic_gurus@quicinc.com>
>>
>> When the firmware (FW) supports multiple requests per VM, multiple requests
>> from the same/different VM can 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 a new return
>> value in addition to success or error: SCM_WAITQ_SLEEP. A sleeping SCM call
>> can be woken up by an interrupt that the FW raises.
>>
>>    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.
>>
>> 	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".
>>
>> The handshake mechanism that HLOS uses to talk to FW about wait-queue
>> operations involves two new SMC calls.
>>
>>    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.
>>
>> (The mechanism to wake a SMC call back up is described in detail below)
>>
>>   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         │
>>     │                        │◄────────────────────────────────┤
>>     │                        │                                 │
>>     │                        │                                 │
>>
>> With the exception of get_wq_ctx(), the other SMC call wq_resume() can
>> 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)
>>
>> Also add the interrupt handler that wakes up a sleeping SCM call.
>>
>> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
>> Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>
>> v7:
>> - Move lookup + wait_for_completion into a single function in qcom_scm [Bjorn]
>> - Simplify completion retrieval [Bjorn]
>>
>> v6:
>> - Fix misc. nits in the scm driver [Krzysztof]
>>
>> v5:
>> - Handle the wake_one/wake_all flags [Guru]
>> - Rename flag handler to qcom_scm_waitq_wakeup [Bjorn]
>> - Resume scm call can return ebusy as well handle that scenario by retrying
>>    the original smc call and not the resume call
>>
>> v4:
>> - platform_set_drvdata will be used by __scm_smc_do_quirk_handle_waitq to
>>    get access to scm struct from device so retain it
>> - Use a single completion as it satisfies all of the current usecases [Bjorn]
>> - Inline scm_get_wq_ctx [Bjorn]
>> - Convert all pr_err to dev_err [Bjorn]
>> - Handle idr_destroy in a thread safe manner [Bjorn]
>> - Misc. Style fixes [Bjorn]
>>
>> v3:
>> - Fixup irq handling so as not to affect SoCs without the interrupt.
>> - Fix warnings reported by kernel test-bot.
>>
>>   drivers/firmware/qcom_scm-smc.c | 90 ++++++++++++++++++++++++++++++---
>>   drivers/firmware/qcom_scm.c     | 89 +++++++++++++++++++++++++++++++-
>>   drivers/firmware/qcom_scm.h     |  8 +++
>>   3 files changed, 179 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
>> index d111833364ba..30999f04749c 100644
>> --- a/drivers/firmware/qcom_scm-smc.c
>> +++ b/drivers/firmware/qcom_scm-smc.c
>> @@ -52,29 +52,101 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
>>   	} while (res->a0 == QCOM_SCM_INTERRUPTED);
>>   }
>>   
>> -static void __scm_smc_do(const struct arm_smccc_args *smc,
>> -			 struct arm_smccc_res *res, bool atomic)
>> +static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
>>   {
>> -	int retry_count = 0;
>> +	memset(resume->args, 0, sizeof(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;
>> +}
>> +
>> +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;
>> +
>> +	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));
>> +
>> +	/* Guaranteed to return only success or error, no WAITQ_* */
>> +	__scm_smc_do_quirk(&get_wq_ctx, &get_wq_res);
>> +	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 int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq,
>> +					   struct arm_smccc_res *res)
>> +{
>> +	int ret;
>> +	struct arm_smccc_args resume;
>> +	u32 wq_ctx, smc_call_ctx, flags;
>> +	struct arm_smccc_args *smc = waitq;
>> +
>> +	do {
>> +		__scm_smc_do_quirk(smc, res);
>> +
>> +		if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
>> +			wq_ctx = res->a1;
>> +			smc_call_ctx = res->a2;
>> +			flags = res->a3;
>> +
>> +			if (!dev)
>> +				return -EPROBE_DEFER;
>> +
>> +			ret = qcom_scm_lookup_completion(wq_ctx);
>> +			if (ret)
>> +				return ret;
>> +
>> +			fill_wq_resume_args(&resume, smc_call_ctx);
>> +			smc = &resume;
>> +		}
>> +	} while (res->a0 == QCOM_SCM_WAITQ_SLEEP);
>> +
>> +	return 0;
>> +}
>> +
>> +static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
>> +			struct arm_smccc_res *res, bool atomic)
>> +{
>> +	int ret, retry_count = 0;
>>   
>>   	if (atomic) {
>>   		__scm_smc_do_quirk(smc, res);
>> -		return;
>> +		return 0;
>>   	}
>>   
>>   	do {
>>   		mutex_lock(&qcom_scm_lock);
>>   
>> -		__scm_smc_do_quirk(smc, res);
>> +		ret = __scm_smc_do_quirk_handle_waitq(dev, smc, res);
>>   
>>   		mutex_unlock(&qcom_scm_lock);
>>   
>> +		if (ret)
>> +			return ret;
>> +
>>   		if (res->a0 == QCOM_SCM_V2_EBUSY) {
>>   			if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
>>   				break;
>>   			msleep(QCOM_SCM_EBUSY_WAIT_MS);
>>   		}
>>   	}  while (res->a0 == QCOM_SCM_V2_EBUSY);
>> +
>> +	return 0;
>>   }
>>   
>>   
>> @@ -83,7 +155,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;
>> @@ -135,13 +207,17 @@ 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 error check follows after args_virt cleanup*/
>> +	ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
>>   
>>   	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;
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index cdbfe54c8146..19ac506a9b1f 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -4,6 +4,7 @@
>>    */
>>   #include <linux/platform_device.h>
>>   #include <linux/init.h>
>> +#include <linux/interrupt.h>
>>   #include <linux/cpumask.h>
>>   #include <linux/export.h>
>>   #include <linux/dma-mapping.h>
>> @@ -13,6 +14,7 @@
>>   #include <linux/qcom_scm.h>
>>   #include <linux/of.h>
>>   #include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>>   #include <linux/of_platform.h>
>>   #include <linux/clk.h>
>>   #include <linux/reset-controller.h>
>> @@ -33,6 +35,7 @@ struct qcom_scm {
>>   	struct clk *iface_clk;
>>   	struct clk *bus_clk;
>>   	struct icc_path *path;
>> +	struct completion waitq_comp;
>>   	struct reset_controller_dev reset;
>>   
>>   	/* control access to the interconnect path */
>> @@ -63,6 +66,9 @@ 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",
>> @@ -1325,11 +1331,79 @@ bool qcom_scm_is_available(void)
>>   }
>>   EXPORT_SYMBOL(qcom_scm_is_available);
>>   
>> +static struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)
> 
> This could either be seen as a remnant of the older versions of this
> patch, or a signalling of "place implementation here". I dislike both...
> 
> Please rename it qcom_scm_assert_valid_wq_ctx() and just reference
> &scm->waitq_comp directly in the two places below.

ack

> 
>> +{
>> +	/* assert wq_ctx is zero */
>> +	if (wq_ctx != 0) {
>> +		dev_err(scm->dev, "No waitqueue found for wq_ctx %d\n", wq_ctx);
> 
> I think you should be more specific here, perhaps:
> 
> 	"Firmware unexpectedly passed non-zero wq_ctx\n"
> 
> And then as suggested a comment stating what needs to be done when that
> happens.

ack

> 
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	return &scm->waitq_comp;
>> +}
>> +
>> +int qcom_scm_lookup_completion(u32 wq_ctx)
>> +{
>> +	struct completion *wq = NULL;
> 
> You assign it on the very next line, no need to do so here.

I'll just reference it directly from the scm struct.

> 
>> +
>> +	wq = qcom_scm_lookup_wq(__scm, wq_ctx);
>> +	if (IS_ERR(wq))
>> +		return PTR_ERR(wq);
>> +
>> +	wait_for_completion(wq);
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx, bool wake_all)
>> +{
>> +	struct completion *wq_to_wake;
> 
> "wq" would be sufficient.

I'll just reference it directly from the scm struct.

> 
> Thanks,
> Bjorn
> 
>> +
>> +	wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
>> +	if (IS_ERR(wq_to_wake))
>> +		return PTR_ERR(wq_to_wake);
>> +
>> +	if (wake_all)
>> +		complete_all(wq_to_wake);
>> +	else
>> +		complete(wq_to_wake);
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
>> +{
>> +	int ret;
>> +	struct qcom_scm *scm = data;
>> +	u32 wq_ctx, flags, more_pending = 0;
>> +
>> +	do {
>> +		ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
>> +		if (ret) {
>> +			dev_err(scm->dev, "GET_WQ_CTX SMC call failed: %d\n", ret);
>> +			goto out;
>> +		}
>> +
>> +		if (flags != QCOM_SMC_WAITQ_FLAG_WAKE_ONE &&
>> +		    flags != QCOM_SMC_WAITQ_FLAG_WAKE_ALL) {
>> +			dev_err(scm->dev, "Invalid flags found for wq_ctx: %u\n", flags);
>> +			goto out;
>> +		}
>> +
>> +		ret = qcom_scm_waitq_wakeup(scm, wq_ctx, !!(flags & QCOM_SMC_WAITQ_FLAG_WAKE_ALL));
>> +		if (ret)
>> +			goto out;
>> +	} while (more_pending);
>> +
>> +out:
>> +	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,6 +1476,19 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>   	__scm = scm;
>>   	__scm->dev = &pdev->dev;
>>   
>> +	init_completion(&__scm->waitq_comp);
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0) {
>> +		if (irq != -ENXIO)
>> +			return irq;
>> +	} else {
>> +		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
>> +						IRQF_ONESHOT, "qcom-scm", __scm);
>> +		if (ret < 0)
>> +			return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
>> +	}
>> +
>>   	__get_convention();
>>   
>>   	/*
>> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
>> index db3d08a01209..018e9867d55a 100644
>> --- a/drivers/firmware/qcom_scm.h
>> +++ b/drivers/firmware/qcom_scm.h
>> @@ -60,6 +60,9 @@ struct qcom_scm_res {
>>   	u64 result[MAX_QCOM_SCM_RETS];
>>   };
>>   
>> +int qcom_scm_lookup_completion(u32 wq_ctx);

I'll rename ^^ into qcom_scm_wait_for_wq_completion instead.

>> +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,
>> @@ -129,6 +132,10 @@ 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_RESUME			0x02
>> +#define QCOM_SCM_WAITQ_GET_WQ_CTX		0x03
>> +
>>   /* common error codes */
>>   #define QCOM_SCM_V2_EBUSY	-12
>>   #define QCOM_SCM_ENOMEM		-5
>> @@ -137,6 +144,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
>>   #define QCOM_SCM_EINVAL_ARG	-2
>>   #define QCOM_SCM_ERROR		-1
>>   #define QCOM_SCM_INTERRUPTED	1
>> +#define QCOM_SCM_WAITQ_SLEEP	2
>>   
>>   static inline int qcom_scm_remap_error(int err)
>>   {
>> -- 
>> 2.17.1
>>