LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jason Wang <jasowang@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>, linux-kernel@vger.kernel.org
Cc: Amit Shah <amit@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	virtualization@lists.linux-foundation.org,
	stable@vger.kernel.org, Tiwei Bie <tiwei.bie@intel.com>,
	stable@kernel.org
Subject: Re: [PATCH 3/6] virtio_console: free buffers after reset
Date: Tue, 24 Apr 2018 10:40:18 +0800	[thread overview]
Message-ID: <27e87df7-2835-7f3e-c3da-76b844dbd780@redhat.com> (raw)
In-Reply-To: <1524248223-393618-4-git-send-email-mst@redhat.com>



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.bie@intel.com>
> Fixes: b3258ff1d6 ("virtio: Decrement avail idx on buffer detach")
> CC: stable@kernel.org
> Signed-off-by: Michael S. Tsirkin <mst@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);

  reply	other threads:[~2018-04-24  2:40 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
2018-04-20 18:18 ` [PATCH 1/6] virtio_console: don't tie bufs to a vq Michael S. Tsirkin
2018-04-21  7:30   ` Greg Kroah-Hartman
2018-04-24 18:56     ` Michael S. Tsirkin
2018-04-25  5:50       ` Greg Kroah-Hartman
2018-04-20 18:18 ` [PATCH 3/6] virtio_console: free buffers after reset Michael S. Tsirkin
2018-04-24  2:40   ` Jason Wang [this message]
2018-04-20 18:18 ` [PATCH 2/6] virtio: add ability to iterate over vqs Michael S. Tsirkin
2018-04-20 18:18 ` [PATCH 4/6] virtio_console: drop custom control queue cleanup Michael S. Tsirkin
2018-04-20 18:18 ` [PATCH 5/6] virtio_console: move removal code Michael S. Tsirkin
2018-04-20 18:18 ` [PATCH 6/6] virtio_console: reset on out of memory Michael S. Tsirkin
2018-04-24 18:41 ` [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
2018-04-25 14:01   ` Amit Shah
2018-05-03  3:34   ` Amit Shah
2018-05-03  3:45   ` Amit Shah
2018-05-03 19:28     ` Michael S. Tsirkin
2018-05-06 17:56       ` Amit Shah
2018-05-06 18:24       ` Amit Shah
2018-05-06 19:52         ` Michael S. Tsirkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=27e87df7-2835-7f3e-c3da-76b844dbd780@redhat.com \
    --to=jasowang@redhat.com \
    --cc=amit@kernel.org \
    --cc=arnd@arndb.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mst@redhat.com \
    --cc=stable@kernel.org \
    --cc=stable@vger.kernel.org \
    --cc=tiwei.bie@intel.com \
    --cc=virtualization@lists.linux-foundation.org \
    --subject='Re: [PATCH 3/6] virtio_console: free buffers after reset' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).