casaroli commented on code in PR #21381:
URL: https://github.com/apache/nuttx/pull/21381#discussion_r4470498764
##########
drivers/rpmsg/rpmsg_virtio.c:
##########
@@ -472,8 +527,129 @@ static void rpmsg_virtio_rx_callback(FAR struct virtqueue
*vq)
RPMSG_VIRTIO_INVALIDATE(rvq->vq_ring.avail->idx);
priv->headrx = rvq->vq_ring.avail->idx;
}
+}
+
+static void rpmsg_virtio_update_tx(FAR struct rpmsg_virtio_priv_s *priv)
+{
+ FAR struct rpmsg_virtio_device *rvdev = &priv->rvdev;
+ FAR struct virtqueue *svq = rvdev->svq;
+
+ if (rpmsg_virtio_get_role(rvdev) == RPMSG_HOST)
+ {
+ RPMSG_VIRTIO_INVALIDATE(svq->vq_ring.avail->idx);
+ priv->headtx = svq->vq_ring.avail->idx;
+ }
+ else
+ {
+ RPMSG_VIRTIO_INVALIDATE(svq->vq_ring.used->idx);
+ priv->headtx = svq->vq_ring.used->idx;
+ }
+}
+
+/****************************************************************************
+ * Name: rpmsg_virtio_rx_enqueue
+ ****************************************************************************/
+
+static void rpmsg_virtio_rx_enqueue(FAR struct rpmsg_virtio_priv_s *priv,
+ FAR struct rpmsg_hdr *hdr)
+{
+ FAR struct rpmsg_virtio_node_s *node;
+ FAR struct metal_list *tmp;
+
+ tmp = metal_list_first(&priv->freerx);
+ DEBUGASSERT(tmp != NULL);
+ metal_list_del(tmp);
+ node = metal_container_of(tmp, struct rpmsg_virtio_node_s, node);
+ node->hdr = hdr;
+ metal_list_add_tail(&priv->readyrx, &node->node);
+}
+
+/****************************************************************************
+ * Name: rpmsg_virtio_rx_dispatch
+ ****************************************************************************/
+
+static void rpmsg_virtio_rx_dispatch(FAR struct rpmsg_virtio_priv_s *priv)
+{
+ FAR struct rpmsg_virtio_device *rvdev = &priv->rvdev;
+ FAR struct rpmsg_device *rdev = &rvdev->rdev;
+ FAR struct rpmsg_endpoint *ept;
+ FAR struct rpmsg_hdr *hdr;
+ bool release = false;
+ bool last = false;
+ uint32_t len;
+ uint16_t idx;
+ int status;
+
+ metal_mutex_acquire(&rdev->lock);
+ while (!last)
+ {
+ /* Process the received data from remote node */
+
+ hdr = rpmsg_virtio_get_rx_buffer(rvdev, &len, &idx, &last);
Review Comment:
Commit 092372db09c says that the rx path uses the callback's virtqueue
because `rvdev->rvq` is not set for an early interrupt, but only
`rpmsg_virtio_rx_update()` uses it. `rpmsg_virtio_get_rx_buffer()` reads
`rvdev->rvq`, which `rpmsg_init_vdev_with_config()` sets only after
`virtio_create_virtqueues()`, and `virtqueue_nused()` dereferences it.
Fix: in `rpmsg_virtio_rx_callback()` return when `rvdev->rvq` is NULL;
`rpmsg_virtio_start()` dispatches what is pending after the init.
##########
drivers/rpmsg/rpmsg_virtio.c:
##########
@@ -472,8 +527,129 @@ static void rpmsg_virtio_rx_callback(FAR struct virtqueue
*vq)
RPMSG_VIRTIO_INVALIDATE(rvq->vq_ring.avail->idx);
priv->headrx = rvq->vq_ring.avail->idx;
}
+}
+
+static void rpmsg_virtio_update_tx(FAR struct rpmsg_virtio_priv_s *priv)
+{
+ FAR struct rpmsg_virtio_device *rvdev = &priv->rvdev;
+ FAR struct virtqueue *svq = rvdev->svq;
+
+ if (rpmsg_virtio_get_role(rvdev) == RPMSG_HOST)
+ {
+ RPMSG_VIRTIO_INVALIDATE(svq->vq_ring.avail->idx);
+ priv->headtx = svq->vq_ring.avail->idx;
+ }
+ else
+ {
+ RPMSG_VIRTIO_INVALIDATE(svq->vq_ring.used->idx);
+ priv->headtx = svq->vq_ring.used->idx;
+ }
+}
+
+/****************************************************************************
+ * Name: rpmsg_virtio_rx_enqueue
+ ****************************************************************************/
+
+static void rpmsg_virtio_rx_enqueue(FAR struct rpmsg_virtio_priv_s *priv,
+ FAR struct rpmsg_hdr *hdr)
+{
+ FAR struct rpmsg_virtio_node_s *node;
+ FAR struct metal_list *tmp;
+
+ tmp = metal_list_first(&priv->freerx);
+ DEBUGASSERT(tmp != NULL);
+ metal_list_del(tmp);
+ node = metal_container_of(tmp, struct rpmsg_virtio_node_s, node);
+ node->hdr = hdr;
+ metal_list_add_tail(&priv->readyrx, &node->node);
+}
+
+/****************************************************************************
+ * Name: rpmsg_virtio_rx_dispatch
+ ****************************************************************************/
+
+static void rpmsg_virtio_rx_dispatch(FAR struct rpmsg_virtio_priv_s *priv)
+{
+ FAR struct rpmsg_virtio_device *rvdev = &priv->rvdev;
+ FAR struct rpmsg_device *rdev = &rvdev->rdev;
+ FAR struct rpmsg_endpoint *ept;
+ FAR struct rpmsg_hdr *hdr;
+ bool release = false;
+ bool last = false;
+ uint32_t len;
+ uint16_t idx;
+ int status;
+
+ metal_mutex_acquire(&rdev->lock);
Review Comment:
`rpmsg_virtio_rx_dispatch()` runs in the rx interrupt (`RTPATH rpmsg-ping in
irq=1` in the PR body) and takes `rdev->lock`, a `rmutex_t` that threads hold.
If the interrupted thread holds it, `nxrmutex_lock()` passes and the handler
changes `rvq` and `readyrx` under that thread; if another thread holds it,
`nxsem_wait_slow()` blocks the interrupted thread and the handler continues
with no lock. This is on every rx interrupt, not only for `RPMSG_PRIO_RT`. A
wdog handler that calls `nxrmutex_lock()` on a held `rmutex_t` on qemu-armv8a
gets `nxrmutex_lock returned 0` while the other thread still holds it, and from
idle `Assertion failed !((rtcb)->pid < 1): at file: semaphore/sem_wait.c:253`.
Fix: do not take `rdev->lock` in the interrupt. Keep the ring walk in the rx
thread and only post `semrx` from the interrupt, or protect the ring, the
endpoint lookup and both lists with a spinlock in every path.
##########
drivers/rpmsg/rpmsg_virtio.c:
##########
@@ -805,6 +1013,10 @@ void rpmsg_virtio_remove(FAR struct virtio_device *vdev)
rpmsg_deinit_vdev(&priv->rvdev);
+ /* Free noderx */
+
+ kmm_free(priv->noderx);
Review Comment:
`kmm_free(priv->noderx)` comes before `kthread_delete(priv->tid)`, so the rx
thread can still walk `readyrx` and `freerx` in freed memory.
Fix: free `noderx` after `kthread_delete()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
