linux-kernel - Re: [PATCH V8 2/2] firmware: qcom: scm: Add wait-queue handling logic

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <991d6680-480c-b9be-e09b-3b129dbb40a6@quicinc.com>
Date:   Fri, 13 Jan 2023 00:12:13 +0530
From:   Sibi Sankar <quic_sibis@...cinc.com>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        <andersson@...nel.org>
CC:     <agross@...nel.org>, <linux-arm-msm@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <robh+dt@...nel.org>,
        <konrad.dybcio@...ainline.org>, <robimarko@...il.com>,
        <quic_gurus@...cinc.com>
Subject: Re: [PATCH V8 2/2] firmware: qcom: scm: Add wait-queue handling logic

Hey Srini,
Thanks for taking time to review the series.

On 1/11/23 21:31, Srinivas Kandagatla wrote:
> 
> 
> On 11/01/2023 10:17, Sibi Sankar wrote:
>> +static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int 
>> wq_ctx, bool wake_all)
>> +{
>> +    int ret;
>> +
>> +    ret = qcom_scm_assert_valid_wq_ctx(wq_ctx);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (wake_all)
>> +        complete_all(&__scm->waitq_comp);
> 
> As you explained in v7 that there will be only one caller at any point 
> in time and that will be synchronous, so complete_all here is a dead 
> code, isn't it?
> 
> Adding complete_all here is missleading and will require reinit 
> completion in case you want to reuse the same completion.
> 
> AFAIU, you should remove support to wake_all in this patchset and add it 
> when we really can do multiple scm calls simultaneously.
> 

ACK, IIRC v4 of the series had it dropped and was added
later on for feature parity but like you pointed out it's
dead code and should be added back later when there is support
for it. I'll fix this in the next re-spin.


> --srini
> 
> --srini
>> +    else
>> +        complete(&__scm->waitq_comp);
>> +