| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 19 Jul 2022 16:03:20 +0530
From: Rajendra Nayak <quic_rjendra@...cinc.com>
To: Guru Das Srinagesh <quic_gurus@...cinc.com>
CC: Andy Gross <agross@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
<linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
"David Heidelberg" <david@...t.cz>,
Robert Marko <robimarko@...il.com>,
"Elliot Berman" <quic_eberman@...cinc.com>
Subject: Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic
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.
Powered by blists - more mailing lists