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

[Unnathi Chalicheemala <unnathi.chalicheemala@oss.qualcomm.com>]

On 3/4/2025 4:49 AM, Bartosz Golaszewski wrote:
> On Fri, Feb 28, 2025 at 6:40 AM Unnathi Chalicheemala
> <unnathi.chalicheemala@oss.qualcomm.com> wrote:
>>
>> Currently, only a single waitqueue context exists, with waitqueue id zero.
>> Multi-waitqueue mechanism is added in firmware to support the case when
>> multiple VMs make SMC calls or single VM making multiple calls on same CPU.
>>
>> When VMs make SMC call, firmware will allocate waitqueue context assuming
>> the SMC call to be a blocking call. SMC calls that cannot acquire resources
>> are returned to sleep in the calling VM. When resource is available, VM
>> will be notified to wake sleeping thread and resume SMC call.
>> SM8650 firmware can allocate two such waitq contexts so create these two
>> waitqueue contexts.
>>
>> Unique waitqueue contexts are supported by a dynamically sized array where
>> each unique wq_ctx is associated with a struct completion variable for easy
>> lookup. To get the number of waitqueue contexts directly from firmware,
>> qcom_scm_query_waitq_cnt() is introduced. On older targets which support
> 
> Seems like it's actually called qcom_scm_query_waitq_count
> 

Yes my bad. Will correct this in next series.

>> only a single waitqueue, wq_cnt is set to 1 as SCM call for
>> query_waitq_cnt() is not implemented for single waitqueue case.
>>
>> Signed-off-by: Unnathi Chalicheemala <unnathi.chalicheemala@oss.qualcomm.com>
>> ---
>>  drivers/firmware/qcom/qcom_scm.c | 75 ++++++++++++++++++++++++++++------------
>>  1 file changed, 53 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 1aa42685640da8a14191557896fbb49423697a10..ec139380ce5ba6d11f1023258e1d36edcf3d9d45 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -47,7 +47,7 @@ struct qcom_scm {
>>         struct clk *iface_clk;
>>         struct clk *bus_clk;
>>         struct icc_path *path;
>> -       struct completion waitq_comp;
>> +       struct completion *waitq;
>>         struct reset_controller_dev reset;
>>
>>         /* control access to the interconnect path */
>> @@ -57,6 +57,7 @@ struct qcom_scm {
>>         u64 dload_mode_addr;
>>
>>         struct qcom_tzmem_pool *mempool;
>> +       unsigned int wq_cnt;
>>  };
>>
>>  struct qcom_scm_current_perm_info {
>> @@ -2118,6 +2119,25 @@ static int qcom_scm_fill_irq_fwspec_params(struct irq_fwspec *fwspec, u32 virq)
>>         return 0;
>>  }
>>
>> +static int qcom_scm_query_waitq_count(struct qcom_scm *scm)
>> +{
>> +       int ret;
>> +       struct qcom_scm_desc desc = {
>> +               .svc = QCOM_SCM_SVC_WAITQ,
>> +               .cmd = QCOM_SCM_WAITQ_GET_INFO,
>> +               .owner = ARM_SMCCC_OWNER_SIP
>> +       };
>> +       struct qcom_scm_res res;
>> +
>> +       ret = qcom_scm_call_atomic(scm->dev, &desc, &res);
> 
> This can fail for a multitude of reasons - some of which we may want
> to propagate to the caller, how about being more fine-grained and
> using __qcom_scm_is_call_available() to check if
> QCOM_SCM_WAITQ_GET_INFO is available first?
> 

I agree, will return 1 in the case call is unavailable.

Thanks for your review Bartosz!

>> +       if (ret) {
>> +               dev_err(scm->dev, "Multi-waitqueue support unavailable\n");
> 
> Is this an error though? From the commit message it seems it's normal
> operation on older platforms?
> 
> Bartosz
> 
> 
>> +               return 1;
>> +       }
>> +
>> +       return res.result[0] & GENMASK(7, 0);
>> +}
>> +
>>  static int qcom_scm_get_waitq_irq(void)
>>  {
>>         int ret;
>> @@ -2149,42 +2169,40 @@ static int qcom_scm_get_waitq_irq(void)
>>         return ret;
>>  }
>>
>> -static int qcom_scm_assert_valid_wq_ctx(u32 wq_ctx)
>> +static struct completion *qcom_scm_get_completion(u32 wq_ctx)
>>  {
>> -       /* FW currently only supports a single wq_ctx (zero).
>> -        * TODO: Update this logic to include dynamic allocation and lookup of
>> -        * completion structs when FW supports more wq_ctx values.
>> -        */
>> -       if (wq_ctx != 0) {
>> -               dev_err(__scm->dev, "Firmware unexpectedly passed non-zero wq_ctx\n");
>> -               return -EINVAL;
>> -       }
>> +       struct completion *wq;
>>
>> -       return 0;
>> +       if (WARN_ON_ONCE(wq_ctx >= __scm->wq_cnt))
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       wq = &__scm->waitq[wq_ctx];
>> +
>> +       return wq;
>>  }
>>
>>  int qcom_scm_wait_for_wq_completion(u32 wq_ctx)
>>  {
>> -       int ret;
>> +       struct completion *wq;
>>
>> -       ret = qcom_scm_assert_valid_wq_ctx(wq_ctx);
>> -       if (ret)
>> -               return ret;
>> +       wq = qcom_scm_get_completion(wq_ctx);
>> +       if (IS_ERR(wq))
>> +               return PTR_ERR(wq);
>>
>> -       wait_for_completion(&__scm->waitq_comp);
>> +       wait_for_completion(wq);
>>
>>         return 0;
>>  }
>>
>>  static int qcom_scm_waitq_wakeup(unsigned int wq_ctx)
>>  {
>> -       int ret;
>> +       struct completion *wq;
>>
>> -       ret = qcom_scm_assert_valid_wq_ctx(wq_ctx);
>> -       if (ret)
>> -               return ret;
>> +       wq = qcom_scm_get_completion(wq_ctx);
>> +       if (IS_ERR(wq))
>> +               return PTR_ERR(wq);
>>
>> -       complete(&__scm->waitq_comp);
>> +       complete(wq);
>>
>>         return 0;
>>  }
>> @@ -2260,6 +2278,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>         struct qcom_tzmem_pool_config pool_config;
>>         struct qcom_scm *scm;
>>         int irq, ret;
>> +       int i;
>>
>>         scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
>>         if (!scm)
>> @@ -2270,7 +2289,19 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>         if (ret < 0)
>>                 return ret;
>>
>> -       init_completion(&scm->waitq_comp);
>> +       ret = qcom_scm_query_waitq_count(scm);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       scm->wq_cnt = ret;
>> +
>> +       scm->waitq = devm_kcalloc(&pdev->dev, scm->wq_cnt, sizeof(*scm->waitq), GFP_KERNEL);
>> +       if (!scm->waitq)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < scm->wq_cnt; i++)
>> +               init_completion(&scm->waitq[i]);
>> +
>>         mutex_init(&scm->scm_bw_lock);
>>
>>         scm->path = devm_of_icc_get(&pdev->dev, NULL);
>>
>> --
>> 2.34.1
>>
>>