Hello, please check my comments below:
On 12/10/25 2:29 AM, Stefan Roese wrote:
Hi Tanmay,
On 12/10/25 03:51, Zhongqiu Han wrote:
On 12/5/2025 8:06 PM, Stefan Roese wrote:
Hi Tanmay,
On 12/4/25 17:45, Tanmay Shah wrote:
Hello,
Thank You for your patch. Please find my comments below.
On 12/4/25 4:40 AM, Stefan Roese wrote:
Testing on our ZynqMP platform has shown, that some R5 messages might
get dropped under high CPU load. This patch creates a new high-prio
This commit text should be fixed. Messages are not dropped by Linux, but
R5 can't send new messages as rx vq is not processed by Linux.
Here, I would like to understand what it means by "R5 messages might
get dropped"
Even under high CPU load, the messages from R5 are stored in the
virtqueues. If Linux doesn't read it, then it is not really lost/
dropped.
Could you please explain your use case in detail and how the testing
is conducted?
Our use-case is, that we send ~4k messages per second from the R5 to
Linux - sometimes even a bit more. Normally these messages are received
okay and no messages are dropped. Sometimes, under "high CPU load"
scenarios it happens, that the R5 has to drop messages, as there is no
free space in the RPMsg buffer, which is 256 entries AFAIU. Resulting
from the Linux driver not emptying the RX queue.
Thanks for the details. Your understanding is correct.
Could you please elaborate on these virtqueues a bit? Especially why no
messages drop should happen because of these virtqueues?
AFAIK, as a transport layer based on virtqueue, rpmsg is reliable once a
message has been successfully enqueued. The observed "drop" here appears
to be on the R5 side, where the application discards messages when no
entry buffer is available.
Correct.
In the long run, while improving the Linux side is recommended,
Yes, please.
it could
also be helpful for the R5 side to implement strategies such as an
application-level buffer and retry mechanisms.
We already did this. We've added an additional buffer mechanism to the
R5, which improved this "message drop situation" a bit. Still it did not
fix it for all our high message rate situations - still resulting in
frame drops on the R5 side (the R5 is a bit resource restricted).
Improving the responsiveness on the Linux side seems to be the best way
for us to deal with this problem.
I agree to this. However, Just want to understand and cover full picture
here.
On R5 side, I am assuming open-amp library is used for the RPMsg
communication.
rpmsg_send() API will end up here:
https://github.com/OpenAMP/open-amp/blob/be5770f30516505c1a4d35efcffff9fb547f7dcf/lib/rpmsg/rpmsg_virtio.c#L384
Here, if the new buffer is not available, then R5 is supposed to wait
for 1ms before sending a new message. After 1ms, R5 will try to get
buffer again, and this continues for 15 seconds. This is the default
mechanism.
This mechanism is used in your case correctly ?
Alternatively you can register platform specific wait mechanism via this
callback:
https://github.com/OpenAMP/open-amp/blob/be5770f30516505c1a4d35efcffff9fb547f7dcf/lib/include/openamp/rpmsg_virtio.h#L42
Few questions for further understanding:
1) As per your use case, 4k per second data transfer rate must be
maintained all the time? And this is achieved with this patch?
Even after having the high priority queue, if someone wants to achieve
8k per seconds or 16k per seconds data transfer rate, at some point we
will hit this issue again.
The reliable solution would be to keep the data transfer rate
reasonable, and have solid re-try mechanism.
I am okay to take this patch in after addressing comments below but,
please make sure all above things are r5 side is working as well.
Thanks,
Tanmay
Thanks,
Stefan
Thanks,
Stefan
Thanks,
Tanmay
workqueue which is now used instead of the default system workqueue.
With this change we don't experience these message drops any more.
Signed-off-by: Stefan Roese <[email protected]>
Cc: Tanmay Shah <[email protected]>
Cc: Mathieu Poirier <[email protected]>
---
v3:
- Call cancel_work_sync() before freeing ipi (suggested by Zhongqiu
Han)
v2:
- Also call destroy_workqueue() in zynqmp_r5_cluster_exit()
(suggested by Zhongqiu Han)
- Correct call seq to avoid UAF (suggested by Zhongqiu Han)
drivers/remoteproc/xlnx_r5_remoteproc.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/
remoteproc/xlnx_r5_remoteproc.c
index feca6de68da28..308328b0b489f 100644
--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
@@ -16,6 +16,7 @@
#include <linux/of_reserved_mem.h>
#include <linux/platform_device.h>
#include <linux/remoteproc.h>
+#include <linux/workqueue.h>
#include "remoteproc_internal.h"
@@ -116,6 +117,7 @@ struct zynqmp_r5_cluster {
enum zynqmp_r5_cluster_mode mode;
int core_count;
struct zynqmp_r5_core **r5_cores;
+ struct workqueue_struct *workqueue;
};
/**
@@ -174,10 +176,18 @@ static void handle_event_notified(struct
work_struct *work)
static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
{
struct zynqmp_ipi_message *ipi_msg, *buf_msg;
+ struct zynqmp_r5_cluster *cluster;
struct mbox_info *ipi;
+ struct device *dev;
size_t len;
ipi = container_of(cl, struct mbox_info, mbox_cl);
+ dev = ipi->r5_core->dev;
+ cluster = dev_get_drvdata(dev->parent);
+ if (!cluster) {
+ dev_err(dev->parent, "Invalid driver data\n");
+ return;
+ }
/* copy data from ipi buffer to r5_core */
ipi_msg = (struct zynqmp_ipi_message *)msg;
@@ -195,7 +205,7 @@ static void zynqmp_r5_mb_rx_cb(struct
mbox_client *cl, void *msg)
if (mbox_send_message(ipi->rx_chan, NULL) < 0)
dev_err(cl->dev, "ack failed to mbox rx_chan\n");
- schedule_work(&ipi->mbox_work);
+ queue_work(cluster->workqueue, &ipi->mbox_work);
}
/**
@@ -1154,6 +1164,7 @@ static void zynqmp_r5_cluster_exit(void *data)
for (i = 0; i < cluster->core_count; i++) {
r5_core = cluster->r5_cores[i];
+ cancel_work_sync(&r5_core->ipi->mbox_work);
I see merge-conflict on top of the for-next branch. Please rebase the
patch on top of the for-next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/log/?h=for-next
zynqmp_r5_free_mbox(r5_core->ipi);
of_reserved_mem_device_release(r5_core->dev);
put_device(r5_core->dev);
@@ -1162,6 +1173,7 @@ static void zynqmp_r5_cluster_exit(void *data)
}
kfree(cluster->r5_cores);
+ destroy_workqueue(cluster->workqueue);
kfree(cluster);
platform_set_drvdata(pdev, NULL);
}
@@ -1194,11 +1206,20 @@ static int
zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
return ret;
}
+ cluster->workqueue = alloc_workqueue(dev_name(dev),
+ WQ_UNBOUND | WQ_HIGHPRI, 0);
+ if (!cluster->workqueue) {
+ dev_err_probe(dev, -ENOMEM, "cannot create workqueue\n");
+ kfree(cluster);
+ return -ENOMEM;
+ }
+
Workqueue will be unused if mbox properties are not mentioned in the
device-tree. So, we need to allocate workqueue only if IPI is setup for
at least one core. I think following logic should work:
Make decision if workqueue is needed or not, if zynqmp_r5_setup_mbox()
function is passing for atleast one core. If zynqmp_r5_setup_mbox() is
success, then set a flag to allocate workqueue, and then later right
before calling zynqmp_r5_core_init() allocate the workqueue for the cluster.
Remoteproc can be used only to load() and start() stop() fw, and RPMsg
can be optional.
Also, before calling destroy_workqueue make sure to have NULL check and
destroy only if it was allocated.
Thanks,
Tanmay
/* wire in so each core can be cleaned up at driver remove */
platform_set_drvdata(pdev, cluster);
ret = zynqmp_r5_cluster_init(cluster);
if (ret) {
+ destroy_workqueue(cluster->workqueue);
kfree(cluster);
platform_set_drvdata(pdev, NULL);
dev_err_probe(dev, ret, "Invalid r5f subsystem device
tree\n");
