Re: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial
flat view
From: Rusty Russell <hidden>
Date: 2012-10-23 01:47:49
sjur.brandeland@stericsson.com writes:
From: Sjur Brændeland <redacted> Add a simple serial connection driver called VIRTIO_ID_RPROC_SERIAL (11) for communicating with a remote processor in an asymmetric multi-processing configuration. This implementation reuses the existing virtio_console implementation, and adds support for DMA allocation of data buffers and disables use of tty console and the virtio control queue. Signed-off-by: Sjur Brændeland <redacted>
The free-outside-interrupt issue is usually dealt with by offloading to a wq, but your variant works (and isn't too ugly).
quoted hunk ↗ jump to hunk
+ /* dma_free_coherent requires interrupts to be enabled. */ + if (!can_sleep) { + /* queue up dma-buffers to be freed later */ + spin_lock_irqsave(&dma_bufs_lock, flags); + list_add_tail(&buf->list, &pending_free_dma_bufs); + spin_unlock_irqrestore(&dma_bufs_lock, flags); + return; + } + dma_free_coherent(buf->dev, buf->size, buf->buf, buf->dma); + + /* Release device refcnt and allow it to be freed */ + put_device(buf->dev);
...
quoted hunk ↗ jump to hunk
+static void reclaim_dma_bufs(void) +{ + unsigned long flags; + struct port_buffer *buf, *tmp; + LIST_HEAD(tmp_list); + + if (list_empty(&pending_free_dma_bufs)) + return; + + /* Create a copy of the pending_free_dma_bufs while holding the lock */ + spin_lock_irqsave(&dma_bufs_lock, flags); + list_cut_position(&tmp_list, &pending_free_dma_bufs, + pending_free_dma_bufs.prev); + spin_unlock_irqrestore(&dma_bufs_lock, flags); + + /* Release the dma buffers, without irqs enabled */ + list_for_each_entry_safe(buf, tmp, &tmp_list, list) { + list_del(&buf->list); + free_buf(buf, true); + } +}
Looks like this should be an easy noop even if !is_rproc_serial.
quoted hunk ↗ jump to hunk
+ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size, int pages) { struct port_buffer *buf; + if (is_rproc_serial(vq->vdev)) + reclaim_dma_bufs(); +
...
quoted hunk ↗ jump to hunk
@@ -904,6 +1000,8 @@ static int port_fops_release(struct inode *inode, struct file *filp) reclaim_consumed_buffers(port); spin_unlock_irq(&port->outvq_lock); + if (is_rproc_serial(port->portdev->vdev)) + reclaim_dma_bufs();
So these are redundant.
quoted hunk ↗ jump to hunk
@@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port) /* Remove buffers we queued up for the Host to send us data in. */ while ((buf = virtqueue_detach_unused_buf(port->in_vq))) - free_buf(buf); + free_buf(buf, true); + + /* + * Remove buffers from out queue for rproc-serial. We cannot afford + * to leak any DMA mem, so reclaim this memory even if this might be + * racy for the remote processor. + */ + if (is_rproc_serial(port->portdev->vdev)) + while ((buf = virtqueue_detach_unused_buf(port->out_vq))) + free_buf(buf, true); }
This seems wrong; either this is needed even if !is_rproc_serial(), or it's not necessary as the out_vq is empty. Every path I can see has the device reset (in which case the queues should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE event (in which case, the same). I think we can have non-blocking writes which could leave buffers in out_vq: Amit?
quoted hunk ↗ jump to hunk
static void __exit fini(void) { + reclaim_dma_bufs();
Hmm, you didn't protect it here anyway... Cheers, Rusty. _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization