On 3/4/2025 4:49 AM, Bartosz Golaszewski wrote:
> On Fri, Feb 28, 2025 at 6:40 AM Unnathi Chalicheemala
> <unnathi.chalicheemala@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
>> ---
>> 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
>>
>>
