Glink native fixes upstreaming
Subsystems (2)
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
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
@@ -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
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
Signed-off-by Konstantin Dorfman <[email protected]>Signed-off-by Deepak Kumar Singh <[email protected]>--- drivers/rpmsg/qcom_glink_native.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/rpmsg/qcom_glink_native.c +4
-0@@ -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
PATCH 2/6 rpmsg: glink: Deny intent request if reusable intent fits
In reply to Deepak Kumar SinghFrom: 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
@@ -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
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.
Signed-off-by Chris Lew <[email protected]>Signed-off-by Deepak Kumar Singh <[email protected]>--- drivers/rpmsg/qcom_glink_native.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)drivers/rpmsg/qcom_glink_native.c +3
-1@@ -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
PATCH 3/6 rpmsg: glink: Add TX_DATA_CONT command while sending
In reply to Deepak Kumar SinghFrom: 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
@@ -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
Bjorn Andersson
AppliedRe: (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_SIZEwhich is 16k and return failure for all packets above 16k.Add
TX_DATA_CONTcommand 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]>
PATCH 4/6 rpmsg: glink: Remove the rpmsg dev in close_ack
In reply to Deepak Kumar SinghFrom: 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
@@ -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
Bjorn Andersson
AppliedRe: (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_ackon closed channel.To avoid sending invalid
open_ackcase unregister the rpmsg device after receiving thelocal_close_ackfrom 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]>
PATCH 5/6 rpmsg: glink: Remove channel decouple from rpdev release
In reply to Deepak Kumar SinghFrom: 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
@@ -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
Bjorn Andersson
AppliedRe: (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_releasefunction 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]>
PATCH 6/6 rpmsg: glink: Send READ_NOTIFY command in FIFO full case
In reply to Deepak Kumar SinghFrom: 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
@@ -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
Bjorn Andersson
AppliedRe: (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_NOTIFYcommand 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]>