Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8ede3a51-179d-986b-ea02-d698c8bda284@quicinc.com>
Date:   Wed, 11 Jan 2023 15:04:05 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH V7 2/2] firmware: qcom: scm: Add wait-queue handling logic
Content-Language: en-US
To:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        <andersson@kernel.org>
CC:     <agross@kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <robh+dt@kernel.org>,
        <konrad.dybcio@somainline.org>, <robimarko@gmail.com>,
        <quic_gurus@quicinc.com>
References: <20230110063745.16739-1-quic_sibis@quicinc.com>
 <20230110063745.16739-3-quic_sibis@quicinc.com>
 <b7470966-55f6-fee7-0014-2aae3048612e@linaro.org>
From:   Sibi Sankar <quic_sibis@quicinc.com>
In-Reply-To: <b7470966-55f6-fee7-0014-2aae3048612e@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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