virtio: console: control queue race fixes

Hello,

The first patch here fixes the race seen by Miche. He hasn't yet reported back if this fixes the races he saw, but Joy Pu from Red Hat tested this patch with hot-plugging/unplugging ports in a loop. Before this patch, he saw some freezes as well as sysfs warnings. After applying the patch, all was well.

The second patch can be folded into the series fixing S4 for virtio-console. It ensures all callbacks are disabled in the freeze function. This patch has a dependency on the 1st one in the sense the double disabling of vq won't make sense w/o patch 1.

Please review and apply.

Amit Shah (2):
virtio: console: Serialise control work
virtio: console: Disable callbacks for virtqueues at start of S4
freeze

 drivers/char/virtio_console.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

1.7.7.4

We currently allow multiple instances of the control work handler to run in parallel. This isn't expected to work; serialise access by disabling interrupts on new packets from the Host and enable them when all the existing ones are consumed.

Testcase: hot-plug/unplug a port in a loop. Without this patch, some sysfs warnings are seen along with freezes. With the patch, all works fine.

---
 drivers/char/virtio_console.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

index fd2fd6f..fc54a79 100644
@@ -1471,6 +1471,7 @@ static void control_work_handler(struct work_struct *work)
 	portdev = container_of(work, struct ports_device, control_work);
 	vq = portdev->c_ivq;
 
+ start:
 	spin_lock(&portdev->cvq_lock);
 	while ((buf = virtqueue_get_buf(vq, &len))) {
 		spin_unlock(&portdev->cvq_lock);

@@ -1488,6 +1489,10 @@ static void control_work_handler(struct work_struct *work)
 		}
 	}
 	spin_unlock(&portdev->cvq_lock);
+	if (unlikely(!virtqueue_enable_cb(vq))) {
+		virtqueue_disable_cb(vq);
+		goto start;
+	}
 }
 
 static void out_intr(struct virtqueue *vq)

@@ -1538,6 +1543,7 @@ static void control_intr(struct virtqueue *vq)
 {
 	struct ports_device *portdev;
 
+	virtqueue_disable_cb(vq);
 	portdev = vq->vdev->priv;
 	schedule_work(&portdev->control_work);
 }

1.7.7.4

Wait a sec... This might *work*, but it's not correct. Remember that disabling callbacks is a hint, and it's cetainly not synchronous.

You need something else to explicitly prevent reentry.

Unapplied,
Rusty.

OK, right. I'll use the nrt workqueue to synchronise then.

Amit

To ensure we don't receive any more interrupts from the host after we enter the freeze function, disable all vq interrupts.

There wasn't any problem seen due to this in tests, but applying this patch makes the freeze case more robust.

---
 drivers/char/virtio_console.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

index fc54a79..c1c45d8 100644
@@ -1860,10 +1860,18 @@ static int virtcons_freeze(struct virtio_device *vdev)
 
 	vdev->config->reset(vdev);
 
+	virtqueue_disable_cb(portdev->c_ivq);
 	cancel_work_sync(&portdev->control_work);
+	/*
+	 * Once more: if control_work_handler() was running, it would
+	 * enable the cb as the last step.
+	 */
+	virtqueue_disable_cb(portdev->c_ivq);
 	remove_controlq_data(portdev);
 
 	list_for_each_entry(port, &portdev->ports, list) {
+		virtqueue_disable_cb(port->in_vq);
+		virtqueue_disable_cb(port->out_vq);
 		/*
 		 * We'll ask the host later if the new invocation has
 		 * the port opened or closed.

1.7.7.4

So, the first one should be cc: stable?

Done, thanks.

Cheers,
Rusty.