virtio: console: regression in virtqueue_add_buf() change

Hi Rusty,

The linux-next kernel was failing my virtio-console test suite for a while. I looked into it today, and it's due to the virtqueue_add_buf() change that doesn't return > 0 values anymore. I found your commit that adjusts virtio_console.c, but you missed one instance where the return value mattered, and as a result not enough buffers were queued for the host to send in data.

This resulted in the port's name to be not populated in sysfs, which meant udev didn't create any symlinks in /dev/virtio-ports/.

I've just updated your commit with the small diff (attached below). Please put this commit after

commit e794093a52cdfef09b3fdb6294b75ab8cacb30a8

Author: Rusty Russell <[email protected]>
Date:   Tue Oct 16 23:56:14 2012 +1030

virtio_net: don't rely on virtqueue_add_buf() returning capacity.

and before

commit 08d088e8357b3c031db7de006247f613c7f136ab

Author: Rusty Russell <[email protected]>
Date:   Tue Oct 16 23:56:15 2012 +1030

virtio: make virtqueue_add_buf() returning 0 on success, not capacity.

in the pull request you send so that there's no regression in virtio_console on bisection.

(commit ids from linux-next/master).

I didn't see your linux-next commit

commit 80162488188c239733433cff98ece5cde18b8b3b

Author: Rusty Russell <[email protected]>
Date:   Tue Oct 16 23:56:15 2012 +1030

virtio: console: make it clear that virtqueue_add_buf() no longer returns > 0

on a mailing list (please CC me!), and I missed reviewing the virtio changes, but glad that the testsuite caught this :)

Diff relative to your patch:

index 82ebe02..6a36994 100644
@@ -396,6 +396,8 @@ static int add_inbuf(struct virtqueue *vq, struct port_buffer *buf)
 
 	ret = virtqueue_add_buf(vq, sg, 0, 1, buf, GFP_ATOMIC);
 	virtqueue_kick(vq);
+	if (!ret)
+		ret = vq->num_free;
 	return ret;
 }
 

Please apply.

Rusty Russell (1):
virtio: console: virtqueue_add_buf() no longer returns > 0

 drivers/char/virtio_console.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

1.8.0.1

From: Rusty Russell <[email protected]>

We simplified virtqueue_add_buf(), make it clear in the callers. Use the newly-exposed vq->num_free in the two places that used the previous +ve return value.

---
 drivers/char/virtio_console.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

index 8ab9c3d..6a36994 100644
@@ -396,6 +396,8 @@ static int add_inbuf(struct virtqueue *vq, struct port_buffer *buf)
 
 	ret = virtqueue_add_buf(vq, sg, 0, 1, buf, GFP_ATOMIC);
 	virtqueue_kick(vq);
+	if (!ret)
+		ret = vq->num_free;
 	return ret;
 }
 

@@ -459,7 +461,7 @@ static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
 	vq = portdev->c_ovq;
 
 	sg_init_one(sg, &cpkt, sizeof(cpkt));
-	if (virtqueue_add_buf(vq, sg, 1, 0, &cpkt, GFP_ATOMIC) >= 0) {
+	if (virtqueue_add_buf(vq, sg, 1, 0, &cpkt, GFP_ATOMIC) == 0) {
 		virtqueue_kick(vq);
 		while (!virtqueue_get_buf(vq, &len))
 			cpu_relax();

@@ -524,7 +526,7 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 			      struct buffer_token *tok, bool nonblock)
 {
 	struct virtqueue *out_vq;
-	ssize_t ret;
+	int err;
 	unsigned long flags;
 	unsigned int len;
 

@@ -534,17 +536,17 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 
 	reclaim_consumed_buffers(port);
 
-	ret = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
+	err = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
 
 	/* Tell Host to go! */
 	virtqueue_kick(out_vq);
 
-	if (ret < 0) {
+	if (err) {
 		in_count = 0;
 		goto done;
 	}
 
-	if (ret == 0)
+	if (out_vq->num_free == 0)
 		port->outvq_full = true;
 
 	if (nonblock)

1.8.0.1

I will append it for now, then fold it before pushing to Linus. I try not to rebase linux-next until just before the push.

Thanks,
Rusty.