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: <aa754d43-ec58-97c7-e50b-0459a850e302@quicinc.com>
Date:   Fri, 1 Jul 2022 16:29:38 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH 4/5] firmware: qcom: scm: Add wait-queue helper functions
Content-Language: en-US
To:     Guru Das Srinagesh <quic_gurus@quicinc.com>,
        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>
CC:     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-5-git-send-email-quic_gurus@quicinc.com>
From:   Rajendra Nayak <quic_rjendra@quicinc.com>
In-Reply-To: <1656359076-13018-5-git-send-email-quic_gurus@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 6/28/2022 1:14 AM, Guru Das Srinagesh wrote:
> When the firmware (FW) supports multiple requests per VM, and the VM also
> supports it via the `enable-multi-call` device tree flag, the floodgates
> are thrown open for them to all reach the firmware at the same time.
> 
> Since the firmware currently being used has limited resources, it guards
> them with a resource lock and puts requests on a wait-queue internally
> and signals to HLOS that it is doing so. It does this by returning two
> new return values in addition to success or error: SCM_WAITQ_SLEEP and
> SCM_WAITQ_WAKE.
> 
>    1) SCM_WAITQ_SLEEP:
> 
>    	When an SCM call receives this return value instead of success
>    	or error, FW has placed this call on a wait-queue and
>    	has signalled HLOS to put it to non-interruptible sleep. (The
> 	mechanism to wake it back up will be described in detail in the
> 	next patch for the sake of simplicity.)
> 
> 	Along with this return value, FW also passes to HLOS `wq_ctx` -
> 	a unique number (UID) identifying the wait-queue that it has put
> 	the call on, internally. This is to help HLOS with its own
> 	bookkeeping to wake this sleeping call later.
> 
> 	Additionally, FW also passes to HLOS `smc_call_ctx` - a UID
> 	identifying the SCM call thus being put to sleep. This is also
> 	for HLOS' bookkeeping to wake this call up later.
> 
> 	These two additional values are passed via the a1 and a2
> 	registers.
> 
> 	N.B.: The "ctx" in the above UID names = "context".
> 
>    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.

What happens to this SCM call itself (The one which gets an SCM_WAITQ_WAKE returned
instead of a success or failure)?
is it processed? how does the firmware in that case return a success or error?

> 
>    	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.
> 
> The handshake mechanism that HLOS uses to talk to FW about wait-queue
> operations involves three new SMC calls. These are:
> 
>    1) get_wq_ctx():
> 
>      	Arguments: 	None
>      	Returns:	wq_ctx, flags, more_pending
> 
>      	Get the wait-queue context, and wake up either one or all of the
>      	sleeping SCM calls associated with that wait-queue.
> 
>      	Additionally, repeat this if there are more wait-queues that are
>      	ready to have their requests woken up (`more_pending`).
> 
>    2) wq_resume(smc_call_ctx):
> 
>    	Arguments:	smc_call_ctx
> 
>    	HLOS needs to issue this in response to receiving an
>    	IRQ, passing to FW the same smc_call_ctx that FW
>    	receives from HLOS via the get_wq_ctx() call.
> 
>    3) wq_wake_ack(smc_call_ctx):
> 
>    	Arguments:	smc_call_ctx
> 
>    	HLOS needs to issue this in response to receiving an
>    	SCM_WAITQ_WAKE, passing to FW the same smc_call_ctx that FW
>    	passed to HLOS via the SMC_WAITQ_WAKE call.
> 
> (Reminder that the full handshake mechanism will be detailed in the
> subsequent patch.)
> 
> Also add the interrupt handler that wakes up a sleeping SCM call.
> 
> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>

[]...

> +
>   static int qcom_scm_probe(struct platform_device *pdev)
>   {
>   	struct qcom_scm *scm;
>   	unsigned long clks;
> -	int ret;
> +	int irq, ret;
>   
>   	scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
>   	if (!scm)
> @@ -1333,12 +1432,28 @@ static int qcom_scm_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> +	platform_set_drvdata(pdev, scm);
> +
>   	__scm = scm;
>   	__scm->dev = &pdev->dev;
>   
> +	spin_lock_init(&__scm->waitq.idr_lock);
> +	idr_init(&__scm->waitq.idr);
>   	qcom_scm_allow_multicall = of_property_read_bool(__scm->dev->of_node,
>   							"allow-multi-call");
>   
> +	INIT_WORK(&__scm->waitq.scm_irq_work, scm_irq_work);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq) {
> +		ret = devm_request_threaded_irq(__scm->dev, irq, NULL,
> +			qcom_scm_irq_handler, IRQF_ONESHOT, "qcom-scm", __scm);
> +		if (ret < 0) {
> +			pr_err("Failed to request qcom-scm irq: %d\n", ret);

idr_destroy()?

> +			return ret;
> +		}
> +	}
> +
>   	__get_convention();
>   
>   	/*
> @@ -1354,6 +1469,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>   
>   static void qcom_scm_shutdown(struct platform_device *pdev)
>   {
> +	idr_destroy(&__scm->waitq.idr);
>   	/* Clean shutdown, disable download mode to allow normal restart */
>   	if (download_mode)
>   		qcom_scm_set_download_mode(false);