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;
From:   Bjorn Andersson <quic_bjorande@quicinc.com>
To:     Bjorn Andersson <andersson@kernel.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Chris Lew <quic_clew@quicinc.com>
CC:     <linux-arm-msm@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: [PATCH 2/2] rpmsg: glink: Wait for intent, not just request ack
Date:   Mon, 27 Mar 2023 07:46:17 -0700
Message-ID: <20230327144617.3134175-3-quic_bjorande@quicinc.com>
In-Reply-To: <20230327144617.3134175-1-quic_bjorande@quicinc.com>
References: <20230327144617.3134175-1-quic_bjorande@quicinc.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
Precedence: bulk

In some implementations of the remote firmware, an intent request
acknowledgement is sent when it's determined if the intent allocation
will be fulfilled, but then the intent is queued after the
acknowledgement.

The result is that upon receving a granted allocation request, the
search for the newly allocated intent will fail and an additional
request will be made. This will at best waste memory, but if the second
request is rejected the transaction will be incorrectly rejected.

Take the incoming intent into account in the wait to mitigate this
problem.

The above scenario can still happen, in the case that, on that same
channel, an unrelated intent is delivered prior to the request
acknowledgement and a separate process enters the send path and picks up
the intent. Given that there's no relationship between the
acknowledgement and the delivered (or to be delivered intent), there
doesn't seem to be a solution to this problem.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/rpmsg/qcom_glink_native.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index e3494de7dce8..3055e0a473e8 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -146,6 +146,7 @@ enum {
  * @open_req:	completed once open-request has been received
  * @intent_req_lock: Synchronises multiple intent requests
  * @intent_req_result: Result of intent request
+ * @intent_received: flag indicating that an intent has been received
  * @intent_req_wq: wait queue for intent_req signalling
  */
 struct glink_channel {
@@ -177,6 +178,7 @@ struct glink_channel {
 
 	struct mutex intent_req_lock;
 	int intent_req_result;
+	bool intent_received;
 	wait_queue_head_t intent_req_wq;
 };
 
@@ -420,13 +422,13 @@ static void qcom_glink_handle_intent_req_ack(struct qcom_glink *glink,
 		return;
 	}
 
-	channel->intent_req_result = granted;
+	WRITE_ONCE(channel->intent_req_result, granted);
 	wake_up_all(&channel->intent_req_wq);
 }
 
 static void qcom_glink_intent_req_abort(struct glink_channel *channel)
 {
-	channel->intent_req_result = 0;
+	WRITE_ONCE(channel->intent_req_result, 0);
 	wake_up_all(&channel->intent_req_wq);
 }
 
@@ -757,6 +759,11 @@ static void qcom_glink_handle_rx_done(struct qcom_glink *glink,
 		kfree(intent);
 	}
 	spin_unlock_irqrestore(&channel->intent_lock, flags);
+
+	if (reuse) {
+		WRITE_ONCE(channel->intent_received, true);
+		wake_up_all(&channel->intent_req_wq);
+	}
 }
 
 /**
@@ -994,6 +1001,9 @@ static void qcom_glink_handle_intent(struct qcom_glink *glink,
 			dev_err(glink->dev, "failed to store remote intent\n");
 	}
 
+	WRITE_ONCE(channel->intent_received, true);
+	wake_up_all(&channel->intent_req_wq);
+
 	kfree(msg);
 	qcom_glink_rx_advance(glink, ALIGN(msglen, 8));
 }
@@ -1273,6 +1283,7 @@ static int qcom_glink_request_intent(struct qcom_glink *glink,
 	mutex_lock(&channel->intent_req_lock);
 
 	WRITE_ONCE(channel->intent_req_result, -1);
+	WRITE_ONCE(channel->intent_received, false);
 
 	cmd.id = GLINK_CMD_RX_INTENT_REQ;
 	cmd.cid = channel->lcid;
@@ -1283,7 +1294,8 @@ static int qcom_glink_request_intent(struct qcom_glink *glink,
 		goto unlock;
 
 	ret = wait_event_timeout(channel->intent_req_wq,
-				 READ_ONCE(channel->intent_req_result) >= 0,
+				 READ_ONCE(channel->intent_req_result) >= 0 &&
+				 READ_ONCE(channel->intent_received),
 				 10 * HZ);
 	if (!ret) {
 		dev_err(glink->dev, "intent request timed out\n");
-- 
2.25.1