On Wed, Dec 10, 2025 at 12:28:52PM -0600, Tanmay Shah wrote: > 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. >
I agree. > > > > > 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. > Right, I also think this patch is not the right solution. > 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. Tanmay is correct on all front. > > 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"); > > > > > > > > > > > > > > > > > >
