>
> On Fri, Aug 09, 2019 at 12:18:46PM +0530, Pankaj Gupta wrote:
> > The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> > deferred detaching of unused buffer to virtio device unplug time.
> > This causes unplug/replug of single port in virtio device with an
> > error "Error allocating inbufs\n". As we don't free the unused buffers
> > attached with the port. Re-plug the same port tries to allocate new
> > buffers in virtqueue and results in this error if queue is full.
>
> So why not reuse the buffers that are already there in this case?
> Seems quite possible.
I took this approach because reusing the buffers will involve tweaking
the existing core functionality like managing the the virt queue indexes.
Compared to that deleting the buffers while hot-unplugging port is simple
and was working fine before. It seems logically correct as well.
I agree we need a spec change for this.
>
> > This patch removes the unused buffers in vq's when we unplug the port.
> > This is the best we can do as we cannot call device_reset because virtio
> > device is still active.
> >
> > Reported-by: Xiaohui Li <xiaohli@xxxxxxxxxx>
> > Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Pankaj Gupta <pagupta@xxxxxxxxxx>
>
> This is really a revert of a7a69ec0d8e4, just tagged confusingly.
>
> And the original is also supposed to be a bugfix.
> So how will the original bug be fixed?
Yes, Even I was confused while adding this tag.
I will remove remove 'fixes' tag completely for this patch?
because its a revert to original behavior which also is a bugfix.
>
> "this is the best we can do" is rarely the case.
>
> I am not necessarily against the revert. But if we go that way then what
> we need to do is specify the behaviour in the spec, since strict spec
> compliance is exactly what the original patch was addressing.
Agree.
>
> In particular, we'd document that console has a special property that
> when port is detached virtqueue is considered stopped, device must not
> use any buffers, and it is legal to take buffers out of the device.
Yes. This documents the exact scenario. Thanks.
You want me to send a patch for the spec change?
Best regards,
Pankaj
>
>
>
> > ---
> > drivers/char/virtio_console.c | 14 +++++++++++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index 7270e7b69262..e8be82f1bae9 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -1494,15 +1494,25 @@ static void remove_port(struct kref *kref)
> > kfree(port);
> > }
> >
> > +static void remove_unused_bufs(struct virtqueue *vq)
> > +{
> > + struct port_buffer *buf;
> > +
> > + while ((buf = virtqueue_detach_unused_buf(vq)))
> > + free_buf(buf, true);
> > +}
> > +
> > static void remove_port_data(struct port *port)
> > {
> > spin_lock_irq(&port->inbuf_lock);
> > /* Remove unused data this port might have received. */
> > discard_port_data(port);
> > + remove_unused_bufs(port->in_vq);
> > spin_unlock_irq(&port->inbuf_lock);
> >
> > spin_lock_irq(&port->outvq_lock);
> > reclaim_consumed_buffers(port);
> > + remove_unused_bufs(port->out_vq);
> > spin_unlock_irq(&port->outvq_lock);
> > }
> >
> > @@ -1938,11 +1948,9 @@ 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);
> > + remove_unused_bufs(vq);
> > }
> > portdev->vdev->config->del_vqs(portdev->vdev);
> > kfree(portdev->in_vqs);
> > --
> > 2.21.0
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
