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;
Date:   Thu, 21 Jul 2022 09:48:07 -0700
From:   Guru Das Srinagesh <quic_gurus@quicinc.com>
To:     Rajendra Nayak <quic_rjendra@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>
Subject: Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic
Message-ID: <20220721164807.GA14440@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>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ad5e875d-e5d7-3b6f-4915-98ba08df8c5d@quicinc.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

On Jul 01 2022 16:51, Rajendra Nayak wrote:
> >>+
> >>+            if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
> >>+                wait_for_completion(wq);
> >>+                fill_wq_resume_args(smc, smc_call_ctx);
> >>+                wq = NULL;
> >>+                continue;
> >>+            } else {
> >>+                fill_wq_wake_ack_args(smc, smc_call_ctx);
> >>+                continue;
> >>+            }
> >>+        } 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?

Yes, you're right, since both error or success means that a request is
complete. We should act the same way for error as for success. Thanks for
catching this.

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

Because that is not part of the protocol - calling scm_waitq_flag_handler(wq, flags)
would result in a completion being freed, meaning a sleeping call would be
woken up, which is not what we want. When a WAITQ_WAKE is received, the
action to be taken is to immediately respond with a wq_wake_ack() and nothing
else.