Re: [PATCH 3/6] virtio_console: free buffers after reset

[Jason Wang]

On 2018年04月21日 02:18, Michael S. Tsirkin wrote:
Console driver is out of spec. The spec says:
        A driver MUST NOT decrement the available idx on a live
        virtqueue (ie. there is no way to “unexpose” buffers).
and it does exactly that by trying to detach unused buffers
without doing a device reset first.

Defer detaching the buffers until device unplug.

Of course this means we might get an interrupt for
a vq without an attached port now. Handle that by
discarding the consumed buffer.

Reported-by: Tiwei Bie <tiwei....@intel.com>
Fixes: b3258ff1d6 ("virtio: Decrement avail idx on buffer detach")
CC: sta...@kernel.org
Signed-off-by: Michael S. Tsirkin <m...@redhat.com>

I wonder whether or not we can have some BUG_ON() in 
virtqueue_detach_unused_buf() to detect such bugs (e.g by checking status?).

Thanks

---
  drivers/char/virtio_console.c | 49 +++++++++++++++++++++----------------------
  1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 3e56f32..26a66ff 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1402,7 +1402,6 @@ static int add_port(struct ports_device *portdev, u32 id)
  {
        char debugfs_name[16];
        struct port *port;
-       struct port_buffer *buf;
        dev_t devt;
        unsigned int nr_added_bufs;
        int err;
@@ -1513,8 +1512,6 @@ static int add_port(struct ports_device *portdev, u32 id)
        return 0;
  
  free_inbufs:
-       while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
-               free_buf(buf, true);
  free_device:
        device_destroy(pdrvdata.class, port->dev->devt);
  free_cdev:
@@ -1539,34 +1536,14 @@ static void remove_port(struct kref *kref)
  
  static void remove_port_data(struct port *port)
  {
-       struct port_buffer *buf;
-
        spin_lock_irq(&port->inbuf_lock);
        /* Remove unused data this port might have received. */
        discard_port_data(port);
        spin_unlock_irq(&port->inbuf_lock);
  
-	/* Remove buffers we queued up for the Host to send us data in. */
-       do {
-               spin_lock_irq(&port->inbuf_lock);
-               buf = virtqueue_detach_unused_buf(port->in_vq);
-               spin_unlock_irq(&port->inbuf_lock);
-               if (buf)
-                       free_buf(buf, true);
-       } while (buf);
-
        spin_lock_irq(&port->outvq_lock);
        reclaim_consumed_buffers(port);
        spin_unlock_irq(&port->outvq_lock);
-
-       /* Free pending buffers from the out-queue. */
-       do {
-               spin_lock_irq(&port->outvq_lock);
-               buf = virtqueue_detach_unused_buf(port->out_vq);
-               spin_unlock_irq(&port->outvq_lock);
-               if (buf)
-                       free_buf(buf, true);
-       } while (buf);
  }
  
  /*
@@ -1791,13 +1768,24 @@ static void control_work_handler(struct work_struct 
*work)
        spin_unlock(&portdev->c_ivq_lock);
  }
  
+static void flush_bufs(struct virtqueue *vq, bool can_sleep)
+{
+       struct port_buffer *buf;
+       unsigned int len;
+
+       while ((buf = virtqueue_get_buf(vq, &len)))
+               free_buf(buf, can_sleep);
+}
+
  static void out_intr(struct virtqueue *vq)
  {
        struct port *port;
  
  	port = find_port_by_vq(vq->vdev->priv, vq);
-       if (!port)
+       if (!port) {
+               flush_bufs(vq, false);
                return;
+       }
  
  	wake_up_interruptible(&port->waitqueue);
  }
@@ -1808,8 +1796,10 @@ static void in_intr(struct virtqueue *vq)
        unsigned long flags;
  
  	port = find_port_by_vq(vq->vdev->priv, vq);
-       if (!port)
+       if (!port) {
+               flush_bufs(vq, false);
                return;
+       }
  
  	spin_lock_irqsave(&port->inbuf_lock, flags);
        port->inbuf = get_inbuf(port);
@@ -1984,6 +1974,15 @@ static const struct file_operations portdev_fops = {
  
  static void remove_vqs(struct ports_device *portdev)
  {
+       struct virtqueue *vq;
+
+       virtio_device_for_each_vq(portdev->vdev, vq) {
+               struct port_buffer *buf;
+
+               flush_bufs(vq, true);
+               while ((buf = virtqueue_detach_unused_buf(vq)))
+                       free_buf(buf, true);
+       }
        portdev->vdev->config->del_vqs(portdev->vdev);
        kfree(portdev->in_vqs);
        kfree(portdev->out_vqs);