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:   Wed, 10 Aug 2022 20:00:24 -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 v2 4/5] firmware: qcom: scm: Add wait-queue helper
 functions
Message-ID: <20220811030022.GA18104@quicinc.com>
References: <1658529438-9234-1-git-send-email-quic_gurus@quicinc.com>
 <1658529438-9234-5-git-send-email-quic_gurus@quicinc.com>
 <1f284b9c-257b-a127-55c0-e6cc8c07a9eb@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <1f284b9c-257b-a127-55c0-e6cc8c07a9eb@quicinc.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

Hey Rajendra,

Sorry for the delay in response. Needed to clarify with internal team members
on these questions before responding.

On Aug 02 2022 17:07, Rajendra Nayak wrote:
> 
> On 7/23/2022 4:07 AM, Guru Das Srinagesh wrote:
> >When the firmware (FW) supports multiple requests per VM, and the VM
> >also supports it via the `allow-multi-call` device tree flag, the
> >floodgates are thrown open for them to all reach the firmware at the
> >same time.

[...]

> >   2) SCM_WAITQ_WAKE:
> >
> >   	When an SCM call receives this return value instead of success
> >   	or error, FW wishes to signal HLOS to wake up a (different)
> >   	previously sleeping call.
> >
> >   	FW tells HLOS which call to wake up via the additional return
> >   	values `wq_ctx`, `smc_call_ctx` and `flags`. The first two have
> >   	already been explained above.
> >
> >   	`flags` can be either WAKE_ONE or WAKE_ALL. Meaning, wake either
> >   	one, or all, of the SCM calls that HLOS is associating with the
> >   	given `wq_ctx`.
> >
> >A sleeping SCM call can be woken up by either an interrupt that FW
> >raises, or via a SCM_WAITQ_WAKE return value for a new SCM call.
> 
> Do you know why the FW was not designed to always use an interrupt?
> That would have made the handling of this in kernel a lot less complicated.

Because:

1. Our firmware in TrustZone cannot raise interrupts on its own - it needs the
hypervisor to do that.

2. Thus, in platforms where there is no hypervisor, there is no interrupt
possible - only SMC_WAITQ_WAKE.

Therefore, relying only on an interrupt would render the driver unable to
support platforms without a hypervisor, which we didn't want to do.

> >The handshake mechanism that HLOS uses to talk to FW about wait-queue
> >operations involves three new SMC calls. These are:
> >

[...]

> >+static void scm_irq_work(struct work_struct *work)
> >+{
> >+	int ret;
> >+	u32 wq_ctx, flags, more_pending = 0;
> >+	struct completion *wq_to_wake;
> >+	struct qcom_scm_waitq *w = container_of(work, struct qcom_scm_waitq, scm_irq_work);
> >+	struct qcom_scm *scm = container_of(w, struct qcom_scm, waitq);
> >+
> >+	do {
> >+		ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
> >+		if (ret) {
> >+			pr_err("GET_WQ_CTX SMC call failed: %d\n", ret);
> >+			return;
> >+		}
> >+
> >+		wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
> >+		if (IS_ERR_OR_NULL(wq_to_wake)) {
> >+			pr_err("No waitqueue found for wq_ctx %d: %ld\n",
> >+					wq_ctx, PTR_ERR(wq_to_wake));
> >+			return;
> 
> What happens if at this point 'more_pending' was true? will the FW raise
> another interrupt?

Hmm. At this point, the interrupt handler is early-exiting without waking up a
sleeping call via the flag_handler() because firmware has goofed up and given
it an invalid wq_ctx. We have bigger problems than `more_pending` being true.

> 
> >+		}
> >+
> >+		scm_waitq_flag_handler(wq_to_wake, flags);
> >+	} while (more_pending);
> >+}

Thank you.

Guru Das.