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: <7d1106b0-178c-7167-34d5-330bb2763c3e@quicinc.com>
Date:   Tue, 19 Jul 2022 16:03:20 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic
Content-Language: en-US
To:     Guru Das Srinagesh <quic_gurus@quicinc.com>
CC:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        "David Heidelberg" <david@ixit.cz>,
        Robert Marko <robimarko@gmail.com>,
        "Elliot Berman" <quic_eberman@quicinc.com>
References: <1656359076-13018-1-git-send-email-quic_gurus@quicinc.com>
 <1656359076-13018-6-git-send-email-quic_gurus@quicinc.com>
 <ec04538b-93e2-fa93-4043-c489a0f228e8@quicinc.com>
 <ad5e875d-e5d7-3b6f-4915-98ba08df8c5d@quicinc.com>
 <20220714005735.GA22183@quicinc.com>
From:   Rajendra Nayak <quic_rjendra@quicinc.com>
In-Reply-To: <20220714005735.GA22183@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 7/14/2022 6:27 AM, Guru Das Srinagesh wrote:
> On Jul 01 2022 16:51, Rajendra Nayak wrote:
>>
>>
>> On 7/1/2022 4:32 PM, Rajendra Nayak wrote:
>>>
>>>
>>> On 6/28/2022 1:14 AM, 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                    │
>>>
>>>
>>> This is perhaps the same thing I asked on the previous patch,
>>> I am guessing {WQ_WAKE} is returned in respone to REQUEST_1?
>>> How do you know in this case if REQUEST_1 was a success or failure?
>>>
>>
>> Ok looking at this some more, I think what we are saying is that the FW returns
>> {WQ_WAKE} to REQUEST_1, we then call wq_wake_ack and the return of
>> *that* will tell if REQUEST_1 was success or failure?
>> Did I get it right?
> 
> Yes, that is correct. I should have added an explanatory note in the commit
> message to this effect:
> 
> 
>       │      {WQ_WAKE}                         <-- Return value  │
>       │◄─────────────────────────────────────────────────────────┤
>       │      wq_ctx, req1_smc_call_ctx, flags  <-- Its payload   │
> 
> What this means is that the WQ_WAKE is sent by the FW to VM1 (direction of
> arrow is from right to left) and that the additional data packed as payload
> indicate that it is meant for REQUEST_1 (`req1_smc_call_ctx`).
> 
> Hopefully this will help understand the diagram better.

Ok thanks for the explanation, I actually had a few more comments down in that
patch which you did not answer, can you clarify them too?

>> +        } else if ((long)res->a0 < 0) {
>> +            /* Error, simply return to caller */
>> +            break;

if my understanding above is correct, shouldn't we do a
>> +            if (wq)
>> +                scm_waitq_flag_handler(wq, flags);
in the error case also?

Also why no just scm_waitq_flag_handler(wq, flags); before fill_wq_wake_ack_args(smc, smc_call_ctx);?

> 
> Thank you.
> 
> Guru Das.