Skip to contentSkip to conversation

Glink native fixes upstreaming

PATCH Applied Deepak Kumar Singh 2 participants 13 messages Jul 30, 2020 – Oct 15, 2021

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover Glink native fixes upstreaming

Includes fixes for -
Few race conditions while channel release and close Proper unregistration of rpmsg device to avoid use of stale device Send notify command to remote when glink fifo is full Handling packet size larger that 16K

Arun Kumar Neelakantam (3):
rpmsg: glink: Add TX_DATA_CONT command while sending
rpmsg: glink: Remove the rpmsg dev in close_ack
rpmsg: glink: Send READ_NOTIFY command in FIFO full case

Chris Lew (2):
rpmsg: glink: Deny intent request if reusable intent fits
rpmsg: glink: Remove channel decouple from rpdev release

Konstantin Dorfman (1):
rpmsg: glink: fix destroy channel endpoint logic

 drivers/rpmsg/qcom_glink_native.c | 109 +++++++++++++++++++++++++++++++++++---
 1 file changed, 101 insertions(+), 8 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#1 lore raw

PATCH 1/6 rpmsg: glink: fix destroy channel endpoint logic

From: Konstantin Dorfman <[email protected]>

When rpmsg client driver destroys last channel endpoint, remove rpmsg device is triggered. In both cases (destroy endpoint and remove device) a glink close command sent to the remote peer.

This change, when for removing rpmsg device endpoint already destroyed will avoid sending second glink close command.

---
 drivers/rpmsg/qcom_glink_native.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/rpmsg/qcom_glink_native.c +4 -0
index 1995f5b..2668c66 100644
@@ -1210,6 +1210,10 @@ static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
 	unsigned long flags;
 
 	spin_lock_irqsave(&channel->recv_lock, flags);
+	if (!channel->ept.cb) {
+		spin_unlock_irqrestore(&channel->recv_lock, flags);
+		return;
+	}
 	channel->ept.cb = NULL;
 	spin_unlock_irqrestore(&channel->recv_lock, flags);
 
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#2 lore raw

Re: [PATCH V1 1/6] rpmsg: glink: fix destroy channel endpoint logic

Quoted 9 lines · Deepak Kumar Singh

From: Konstantin Dorfman <[email protected]>

When rpmsg client driver destroys last channel endpoint, remove rpmsg device is triggered. In both cases (destroy endpoint and remove device) a glink close command sent to the remote peer.

This change, when for removing rpmsg device endpoint already destroyed will avoid sending second glink close command.

Should it really be considered valid to rpmsg_destroy_ept() the rpmsg_device's primary endpoint?

Do you have a use case where this makes sense?

Also, I think this has a potential to hide a problems of clients doing a "double free" on the ept.

Regards,
Bjorn

Quoted 25 lines
---
 drivers/rpmsg/qcom_glink_native.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/rpmsg/qcom_glink_native.c +4 -0
index 1995f5b..2668c66 100644
@@ -1210,6 +1210,10 @@ static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
 	unsigned long flags;
 
 	spin_lock_irqsave(&channel->recv_lock, flags);
+	if (!channel->ept.cb) {
+		spin_unlock_irqrestore(&channel->recv_lock, flags);
+		return;
+	}
 	channel->ept.cb = NULL;
 	spin_unlock_irqrestore(&channel->recv_lock, flags);
 
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#3 lore raw

PATCH 2/6 rpmsg: glink: Deny intent request if reusable intent fits

In reply to Deepak Kumar Singh

From: Chris Lew <[email protected]>

In high traffic scenarios a remote may request extra intents to send data faster. If the work thread that handles these intent requests is starved of cpu time, then these requests can build up. Some remote procs may not be able to handle this burst of built up intent requests.

In order to prevent intent build up, deny intent requests that can be fulfilled by default intents that are reusable.

---
 drivers/rpmsg/qcom_glink_native.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)
drivers/rpmsg/qcom_glink_native.c +16 -1
index 2668c66..df3c608 100644
@@ -734,9 +734,11 @@ static void qcom_glink_handle_rx_done(struct qcom_glink *glink,
 static void qcom_glink_handle_intent_req(struct qcom_glink *glink,
 					 u32 cid, size_t size)
 {
-	struct glink_core_rx_intent *intent;
+	struct glink_core_rx_intent *intent = NULL;
+	struct glink_core_rx_intent *tmp;
 	struct glink_channel *channel;
 	unsigned long flags;
+	int iid;
 
 	spin_lock_irqsave(&glink->idr_lock, flags);
 	channel = idr_find(&glink->rcids, cid);
@@ -747,6 +749,19 @@ static void qcom_glink_handle_intent_req(struct qcom_glink *glink,
 		return;
 	}
 
+	spin_lock_irqsave(&channel->intent_lock, flags);
+	idr_for_each_entry(&channel->liids, tmp, iid) {
+		if (tmp->size >= size && tmp->reuse) {
+			intent = tmp;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&channel->intent_lock, flags);
+	if (intent) {
+		qcom_glink_send_intent_req_ack(glink, channel, !!intent);
+		return;
+	}
+
 	intent = qcom_glink_alloc_intent(glink, channel, size, false);
 	if (intent)
 		qcom_glink_advertise_intent(glink, channel, intent);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#4 lore raw

Re: [PATCH V1 2/6] rpmsg: glink: Deny intent request if reusable intent fits

Quoted 37 lines · Deepak Kumar Singh

From: Chris Lew <[email protected]>

In high traffic scenarios a remote may request extra intents to send data faster. If the work thread that handles these intent requests is starved of cpu time, then these requests can build up. Some remote procs may not be able to handle this burst of built up intent requests.

In order to prevent intent build up, deny intent requests that can be fulfilled by default intents that are reusable.

---
 drivers/rpmsg/qcom_glink_native.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)
drivers/rpmsg/qcom_glink_native.c +3 -1
index 2668c66..df3c608 100644
@@ -734,9 +734,11 @@ static void qcom_glink_handle_rx_done(struct qcom_glink *glink,
 static void qcom_glink_handle_intent_req(struct qcom_glink *glink,
 					 u32 cid, size_t size)
 {
-	struct glink_core_rx_intent *intent;
+	struct glink_core_rx_intent *intent = NULL;
+	struct glink_core_rx_intent *tmp;
 	struct glink_channel *channel;
 	unsigned long flags;
+	int iid;
 
 	spin_lock_irqsave(&glink->idr_lock, flags);
 	channel = idr_find(&glink->rcids, cid);
@@ -747,6 +749,19 @@ static void qcom_glink_handle_intent_req(struct qcom_glink *glink,
 		return;
 	}
 

drivers/rpmsg/qcom_glink_native.c static void qcom_glink_handle_intent_req(struct qcom_glink *glink, · line 761

Quoted 10 lines
drivers/rpmsg/qcom_glink_native.c +10 -0
+	spin_lock_irqsave(&channel->intent_lock, flags);
+	idr_for_each_entry(&channel->liids, tmp, iid) {
+		if (tmp->size >= size && tmp->reuse) {
+			intent = tmp;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&channel->intent_lock, flags);
+	if (intent) {
+		qcom_glink_send_intent_req_ack(glink, channel, !!intent);

This looks reasonable, but you know that !!intent is true in this block, and looking more at it I think the end result would look better as:

	/* Try to find a reusable intent to fit the request */
	spin_lock()
	intent = findit();
	spin_unlock()

	/* Allocate a new intent if none was found */
	if (!intent) {
		intent = qcom_glink_alloc_intent(glink, channel, size, false);
		if (intent)
			qcom_glink_advertise_intent(glink, channel, intent);
	}

	qcom_glink_send_intent_req_ack(glink, channel, !!intent);

Regards,
Bjorn

Quoted 10 lines
drivers/rpmsg/qcom_glink_native.c +3 -0
+		return;
+	}
+
 	intent = qcom_glink_alloc_intent(glink, channel, size, false);
 	if (intent)
 		qcom_glink_advertise_intent(glink, channel, intent);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#5 lore raw

PATCH 3/6 rpmsg: glink: Add TX_DATA_CONT command while sending

In reply to Deepak Kumar Singh

From: Arun Kumar Neelakantam <[email protected]>

With current design the transport can send packets of size upto FIFO_SIZE which is 16k and return failure for all packets above 16k.

Add TX_DATA_CONT command to send packets greater than 16k by splitting into 8K chunks.

---
 drivers/rpmsg/qcom_glink_native.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)
drivers/rpmsg/qcom_glink_native.c +34 -4
index df3c608..ac179b1 100644
@@ -1289,6 +1289,8 @@ static int __qcom_glink_send(struct glink_channel *channel,
 	} __packed req;
 	int ret;
 	unsigned long flags;
+	int chunk_size = len;
+	int left_size = 0;
 
 	if (!glink->intentless) {
 		while (!intent) {
@@ -1322,18 +1324,46 @@ static int __qcom_glink_send(struct glink_channel *channel,
 		iid = intent->id;
 	}
 
+	if (wait && (chunk_size > SZ_8K)) {
+		chunk_size = SZ_8K;
+		left_size = len - chunk_size;
+	}
 	req.msg.cmd = cpu_to_le16(RPM_CMD_TX_DATA);
 	req.msg.param1 = cpu_to_le16(channel->lcid);
 	req.msg.param2 = cpu_to_le32(iid);
-	req.chunk_size = cpu_to_le32(len);
-	req.left_size = cpu_to_le32(0);
+	req.chunk_size = cpu_to_le32(chunk_size);
+	req.left_size = cpu_to_le32(left_size);
 
-	ret = qcom_glink_tx(glink, &req, sizeof(req), data, len, wait);
+	ret = qcom_glink_tx(glink, &req, sizeof(req), data, chunk_size, wait);
 
 	/* Mark intent available if we failed */
-	if (ret && intent)
+	if (ret && intent) {
 		intent->in_use = false;
+		return ret;
+	}
 
+	while (left_size > 0) {
+		data = (void *)((char *)data + chunk_size);
+		chunk_size = left_size;
+		if (chunk_size > SZ_8K)
+			chunk_size = SZ_8K;
+		left_size -= chunk_size;
+
+		req.msg.cmd = cpu_to_le16(RPM_CMD_TX_DATA_CONT);
+		req.msg.param1 = cpu_to_le16(channel->lcid);
+		req.msg.param2 = cpu_to_le32(iid);
+		req.chunk_size = cpu_to_le32(chunk_size);
+		req.left_size = cpu_to_le32(left_size);
+
+		ret = qcom_glink_tx(glink, &req, sizeof(req), data,
+				    chunk_size, wait);
+
+		/* Mark intent available if we failed */
+		if (ret && intent) {
+			intent->in_use = false;
+			break;
+		}
+	}
 	return ret;
 }
 
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#6 lore raw

Re: (subset) [PATCH V1 3/6] rpmsg: glink: Add TX_DATA_CONT command while sending

Quoted 9 lines · Deepak Kumar Singh

From: Arun Kumar Neelakantam <[email protected]>

With current design the transport can send packets of size upto FIFO_SIZE which is 16k and return failure for all packets above 16k.

Add TX_DATA_CONT command to send packets greater than 16k by splitting into 8K chunks.

[...]

Applied, thanks!

[3/6] rpmsg: glink: Add TX_DATA_CONT command while sending

      commit: 8956927faed366b60b0355f4a4317a10e281ced7

Best regards,

Signature · 1 line
Bjorn Andersson <[email protected]>
#7 lore raw

PATCH 4/6 rpmsg: glink: Remove the rpmsg dev in close_ack

In reply to Deepak Kumar Singh

From: Arun Kumar Neelakantam <[email protected]>

Un-register and register of rpmsg driver is sending invalid open_ack on closed channel.

To avoid sending invalid open_ack case unregister the rpmsg device after receiving the local_close_ack from remote side.

---
 drivers/rpmsg/qcom_glink_native.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/rpmsg/qcom_glink_native.c +12 -0
index ac179b1..031bc1d 100644
@@ -1526,6 +1526,7 @@ static void qcom_glink_rx_close(struct qcom_glink *glink, unsigned int rcid)
 
 		rpmsg_unregister_device(glink->dev, &chinfo);
 	}
+	channel->rpdev = NULL;
 
 	qcom_glink_send_close_ack(glink, channel->rcid);
 
@@ -1539,6 +1540,7 @@ static void qcom_glink_rx_close(struct qcom_glink *glink, unsigned int rcid)
 
 static void qcom_glink_rx_close_ack(struct qcom_glink *glink, unsigned int lcid)
 {
+	struct rpmsg_channel_info chinfo;
 	struct glink_channel *channel;
 	unsigned long flags;
 
@@ -1553,6 +1555,16 @@ static void qcom_glink_rx_close_ack(struct qcom_glink *glink, unsigned int lcid)
 	channel->lcid = 0;
 	spin_unlock_irqrestore(&glink->idr_lock, flags);
 
+	/* Decouple the potential rpdev from the channel */
+	if (channel->rpdev) {
+		strlcpy(chinfo.name, channel->name, sizeof(chinfo.name));
+		chinfo.src = RPMSG_ADDR_ANY;
+		chinfo.dst = RPMSG_ADDR_ANY;
+
+		rpmsg_unregister_device(glink->dev, &chinfo);
+	}
+	channel->rpdev = NULL;
+
 	kref_put(&channel->refcount, qcom_glink_channel_release);
 }
 
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#8 lore raw

Re: (subset) [PATCH V1 4/6] rpmsg: glink: Remove the rpmsg dev in close_ack

Quoted 9 lines · Deepak Kumar Singh

From: Arun Kumar Neelakantam <[email protected]>

Un-register and register of rpmsg driver is sending invalid open_ack on closed channel.

To avoid sending invalid open_ack case unregister the rpmsg device after receiving the local_close_ack from remote side.

[...]

Applied, thanks!

[4/6] rpmsg: glink: Remove the rpmsg dev in close_ack

      commit: c7c182d4447e172f87e37d6c04879b94b8635b37

Best regards,

Signature · 1 line
Bjorn Andersson <[email protected]>
#9 lore raw

PATCH 5/6 rpmsg: glink: Remove channel decouple from rpdev release

In reply to Deepak Kumar Singh

From: Chris Lew <[email protected]>

If a channel is being rapidly restarting and the kobj release worker is busy, there is a chance the the rpdev_release function will run after the channel struct itself has been released.

There should not be a need to decouple the channel from rpdev in the rpdev release since that should only happen from the close commands.

---
 drivers/rpmsg/qcom_glink_native.c | 2 --
 1 file changed, 2 deletions(-)
drivers/rpmsg/qcom_glink_native.c +0 -2
index 031bc1d..efaf32d 100644
@@ -1419,9 +1419,7 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
 static void qcom_glink_rpdev_release(struct device *dev)
 {
 	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
-	struct glink_channel *channel = to_glink_channel(rpdev->ept);
 
-	channel->rpdev = NULL;
 	kfree(rpdev);
 }
 
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#10 lore raw

Re: (subset) [PATCH V1 5/6] rpmsg: glink: Remove channel decouple from rpdev release

Quoted 10 lines · Deepak Kumar Singh

From: Chris Lew <[email protected]>

If a channel is being rapidly restarting and the kobj release worker is busy, there is a chance the the rpdev_release function will run after the channel struct itself has been released.

There should not be a need to decouple the channel from rpdev in the rpdev release since that should only happen from the close commands.

[...]

Applied, thanks!

[5/6] rpmsg: glink: Remove channel decouple from rpdev release

      commit: 343ba27b6f9d473ec3e602cc648300eb03a7fa05

Best regards,

Signature · 1 line
Bjorn Andersson <[email protected]>
#11 lore raw

PATCH 6/6 rpmsg: glink: Send READ_NOTIFY command in FIFO full case

In reply to Deepak Kumar Singh

From: Arun Kumar Neelakantam <[email protected]>

The current design sleeps unconditionally in TX FIFO full case and wakeup only after sleep timer expires which adds random delays in clients TX path.

Avoid sleep and use READ_NOTIFY command so that writer can be woken up when remote notifies about read completion by sending IRQ.

---
 drivers/rpmsg/qcom_glink_native.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)
drivers/rpmsg/qcom_glink_native.c +35 -1
index efaf32d..098039d 100644
@@ -92,6 +92,8 @@ struct glink_core_rx_intent {
  * @rcids:	idr of all channels with a known remote channel id
  * @features:	remote features
  * @intentless:	flag to indicate that there is no intent
+ * @tx_avail_notify: Waitqueue for pending tx tasks
+ * @sent_read_notify: flag to check cmd sent or not
  */
 struct qcom_glink {
 	struct device *dev;
@@ -118,6 +120,8 @@ struct qcom_glink {
 	unsigned long features;
 
 	bool intentless;
+	wait_queue_head_t tx_avail_notify;
+	bool sent_read_notify;
 };
 
 enum {
@@ -301,6 +305,20 @@ static void qcom_glink_tx_write(struct qcom_glink *glink,
 	glink->tx_pipe->write(glink->tx_pipe, hdr, hlen, data, dlen);
 }
 
+static void qcom_glink_send_read_notify(struct qcom_glink *glink)
+{
+	struct glink_msg msg;
+
+	msg.cmd = cpu_to_le16(RPM_CMD_READ_NOTIF);
+	msg.param1 = 0;
+	msg.param2 = 0;
+
+	qcom_glink_tx_write(glink, &msg, sizeof(msg), NULL, 0);
+
+	mbox_send_message(glink->mbox_chan, NULL);
+	mbox_client_txdone(glink->mbox_chan, 0);
+}
+
 static int qcom_glink_tx(struct qcom_glink *glink,
 			 const void *hdr, size_t hlen,
 			 const void *data, size_t dlen, bool wait)
@@ -321,12 +339,21 @@ static int qcom_glink_tx(struct qcom_glink *glink,
 			goto out;
 		}
 
+		if (!glink->sent_read_notify) {
+			glink->sent_read_notify = true;
+			qcom_glink_send_read_notify(glink);
+		}
+
 		/* Wait without holding the tx_lock */
 		spin_unlock_irqrestore(&glink->tx_lock, flags);
 
-		usleep_range(10000, 15000);
+		wait_event_timeout(glink->tx_avail_notify,
+				qcom_glink_tx_avail(glink) >= tlen, 10 * HZ);
 
 		spin_lock_irqsave(&glink->tx_lock, flags);
+
+		if (qcom_glink_tx_avail(glink) >= tlen)
+			glink->sent_read_notify = false;
 	}
 
 	qcom_glink_tx_write(glink, hdr, hlen, data, dlen);
@@ -1000,6 +1027,9 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
 	unsigned int cmd;
 	int ret = 0;
 
+	/* To wakeup any blocking writers */
+	wake_up_all(&glink->tx_avail_notify);
+
 	for (;;) {
 		avail = qcom_glink_rx_avail(glink);
 		if (avail < sizeof(msg))
@@ -1542,6 +1572,9 @@ static void qcom_glink_rx_close_ack(struct qcom_glink *glink, unsigned int lcid)
 	struct glink_channel *channel;
 	unsigned long flags;
 
+	/* To wakeup any blocking writers */
+	wake_up_all(&glink->tx_avail_notify);
+
 	spin_lock_irqsave(&glink->idr_lock, flags);
 	channel = idr_find(&glink->lcids, lcid);
 	if (WARN(!channel, "close ack on unknown channel\n")) {
@@ -1658,6 +1691,7 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
 	spin_lock_init(&glink->rx_lock);
 	INIT_LIST_HEAD(&glink->rx_queue);
 	INIT_WORK(&glink->rx_work, qcom_glink_work);
+	init_waitqueue_head(&glink->tx_avail_notify);
 
 	spin_lock_init(&glink->idr_lock);
 	idr_init(&glink->lcids);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#12 lore raw

Re: (subset) [PATCH V1 6/6] rpmsg: glink: Send READ_NOTIFY command in FIFO full case

Quoted 10 lines · Deepak Kumar Singh

From: Arun Kumar Neelakantam <[email protected]>

The current design sleeps unconditionally in TX FIFO full case and wakeup only after sleep timer expires which adds random delays in clients TX path.

Avoid sleep and use READ_NOTIFY command so that writer can be woken up when remote notifies about read completion by sending IRQ.

[...]

Applied, thanks!

[6/6] rpmsg: glink: Send READ_NOTIFY command in FIFO full case

      commit: b16a37e1846c9573a847a56fa2f31ba833dae45a

Best regards,

Signature · 1 line
Bjorn Andersson <[email protected]>