virtio: make del_vq delete vq from list

This refactors find_vqs, making it more readable and robust, and fixing two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector (reported on old host kernels)

An older version of this patch was tested by Amit Shah.

This refactors find_vqs, making it more readable and robust, and fixing two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector (reported on old host kernels)

An older version of this patch was tested by Amit Shah.

Quoted 7 lines · Michael S. Tsirkin

This refactors find_vqs, making it more readable and robust, and fixing two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector (reported on old host kernels)

An older version of this patch was tested by Amit Shah.

OK, I've applied both of these; I'd like to see a new test by Amit to make sure tho.

I really like this cleanup! I looked harder at this code, and my best attempts to untangle it further came to very little. This is what I ended up with, but it's all cosmetic and can wait until next merge window. See what you think.

Thanks!
Rusty.

virtio_pci: minor MSI-X cleanups

1) Rename vp_request_vectors to vp_request_msix_vectors, and take non-MSI-X case out to caller.

2) Comment weird pci_enable_msix API
3) Rename vp_find_vq to setup_vq.
4) Fix spaces to tabs
5) Make nvectors calc internal to vp_try_to_find_vqs()

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/virtio/virtio_pci.c |   84 +++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 39 deletions(-)

drivers/virtio/virtio_pci.c +10 -20

@@ -280,25 +280,14 @@ static void vp_free_vectors(struct virti
 	vp_dev->msix_entries = NULL;
 }
 
-static int vp_request_vectors(struct virtio_device *vdev, int nvectors,
-			      bool per_vq_vectors)
+static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
+				   bool per_vq_vectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	const char *name = dev_name(&vp_dev->vdev.dev);
 	unsigned i, v;
 	int err = -ENOMEM;
 
-	if (!nvectors) {
-		/* Can't allocate MSI-X vectors, use regular interrupt */
-		vp_dev->msix_vectors = 0;
-		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
-				  IRQF_SHARED, name, vp_dev);
-		if (err)
-			return err;
-		vp_dev->intx_enabled = 1;
-		return 0;
-	}
-
 	vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
 				       GFP_KERNEL);
 	if (!vp_dev->msix_entries)

@@ -311,6 +300,7 @@ static int vp_request_vectors(struct vir
 	for (i = 0; i < nvectors; ++i)
 		vp_dev->msix_entries[i].entry = i;
 
+	/* pci_enable_msix returns positive if we can't get this many. */
 	err = pci_enable_msix(vp_dev->pci_dev, vp_dev->msix_entries, nvectors);
 	if (err > 0)
 		err = -ENOSPC;

@@ -356,10 +346,10 @@ error:
 	return err;
 }
 
-static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
-				    void (*callback)(struct virtqueue *vq),
-				    const char *name,
-				    u16 vector)
+static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
+				  void (*callback)(struct virtqueue *vq),
+				  const char *name,
+				  u16 vector)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;

@@ -408,7 +398,7 @@ static struct virtqueue *vp_find_vq(stru
 	vq->priv = info;
 	info->vq = vq;
 
-	 if (vector != VIRTIO_MSI_NO_VECTOR) {
+	if (vector != VIRTIO_MSI_NO_VECTOR) {
 		iowrite16(vector, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
 		vector = ioread16(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
 		if (vector == VIRTIO_MSI_NO_VECTOR) {

@@ -484,14 +474,36 @@ static int vp_try_to_find_vqs(struct vir
 			      struct virtqueue *vqs[],
 			      vq_callback_t *callbacks[],
 			      const char *names[],
-			      int nvectors,
+			      bool use_msix,
 			      bool per_vq_vectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	u16 vector;
-	int i, err, allocated_vectors;
+	int i, err, nvectors, allocated_vectors;

drivers/virtio/virtio_pci.c +26 -18

+	}
+
+	if (per_vq_vectors) {
+		/* Best option: one for change interrupt, one per vq. */
+		nvectors = 1;
+		for (i = 0; i < nvqs; ++i)
+			if (callbacks[i])
+				++nvectors;
+	} else {
+		/* Second best: one for change, shared one for all vqs. */
+		nvectors = 2;
+	}
+
+	err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
 	if (err)
 		goto error_request;
 

@@ -504,15 +516,17 @@ static int vp_try_to_find_vqs(struct vir
 			vector = allocated_vectors++;
 		else
 			vector = VP_MSIX_VQ_VECTOR;
-		vqs[i] = vp_find_vq(vdev, i, callbacks[i], names[i], vector);
+		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], vector);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto error_find;
 		}
 		/* allocate per-vq irq if available and necessary */
 		if (vp_dev->per_vq_vectors && vector != VIRTIO_MSI_NO_VECTOR) {
-			snprintf(vp_dev->msix_names[vector], sizeof *vp_dev->msix_names,
-				 "%s-%s", dev_name(&vp_dev->vdev.dev), names[i]);
+			snprintf(vp_dev->msix_names[vector],
+				 sizeof *vp_dev->msix_names,
+				 "%s-%s",
+				 dev_name(&vp_dev->vdev.dev), names[i]);
 			err = request_irq(vp_dev->msix_entries[vector].vector,
 					  vring_interrupt, 0,
 					  vp_dev->msix_names[vector], vqs[i]);

@@ -537,28 +551,20 @@ static int vp_find_vqs(struct virtio_dev
 		       vq_callback_t *callbacks[],
 		       const char *names[])
 {
-	int vectors = 0;
-	int i, uninitialized_var(err);
+	int err;
 
-	/* How many vectors would we like? */
-	for (i = 0; i < nvqs; ++i)
-		if (callbacks[i])
-			++vectors;
-
-	/* We want at most one vector per queue and one for config changes. */
-	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
-				 vectors + 1, true);
+	/* Try MSI-X with one vector per queue. */
+	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names, true, true);
 	if (!err)
 		return 0;
-	/* Fallback to separate vectors for config and a shared for queues. */
+	/* Fallback: MSI-X with one vector for config, one shared for queues. */
 	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
-				 2, false);
+				 true, false);
 	if (!err)
 		return 0;
 	/* Finally fall back to regular interrupts. */
-	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
-				 0, false);
-	return err;
+	return vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
+				  false, false);
 }
 
 static struct virtio_config_ops virtio_pci_config_ops = {

Quoted 7 lines · Michael S. Tsirkin

This refactors find_vqs, making it more readable and robust, and fixing two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector (reported on old host kernels)

An older version of this patch was tested by Amit Shah.

OK, I've applied both of these; I'd like to see a new test by Amit to make sure tho.

I really like this cleanup! I looked harder at this code, and my best attempts to untangle it further came to very little. This is what I ended up with, but it's all cosmetic and can wait until next merge window. See what you think.

Thanks!
Rusty.

virtio_pci: minor MSI-X cleanups

1) Rename vp_request_vectors to vp_request_msix_vectors, and take non-MSI-X case out to caller.

2) Comment weird pci_enable_msix API
3) Rename vp_find_vq to setup_vq.
4) Fix spaces to tabs
5) Make nvectors calc internal to vp_try_to_find_vqs()

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/virtio/virtio_pci.c |   84 +++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 39 deletions(-)

drivers/virtio/virtio_pci.c +10 -20

@@ -280,25 +280,14 @@ static void vp_free_vectors(struct virti
 	vp_dev->msix_entries = NULL;
 }
 
-static int vp_request_vectors(struct virtio_device *vdev, int nvectors,
-			      bool per_vq_vectors)
+static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
+				   bool per_vq_vectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	const char *name = dev_name(&vp_dev->vdev.dev);
 	unsigned i, v;
 	int err = -ENOMEM;
 
-	if (!nvectors) {
-		/* Can't allocate MSI-X vectors, use regular interrupt */
-		vp_dev->msix_vectors = 0;
-		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
-				  IRQF_SHARED, name, vp_dev);
-		if (err)
-			return err;
-		vp_dev->intx_enabled = 1;
-		return 0;
-	}
-
 	vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
 				       GFP_KERNEL);
 	if (!vp_dev->msix_entries)

@@ -311,6 +300,7 @@ static int vp_request_vectors(struct vir
 	for (i = 0; i < nvectors; ++i)
 		vp_dev->msix_entries[i].entry = i;
 
+	/* pci_enable_msix returns positive if we can't get this many. */
 	err = pci_enable_msix(vp_dev->pci_dev, vp_dev->msix_entries, nvectors);
 	if (err > 0)
 		err = -ENOSPC;

@@ -356,10 +346,10 @@ error:
 	return err;
 }
 
-static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
-				    void (*callback)(struct virtqueue *vq),
-				    const char *name,
-				    u16 vector)
+static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
+				  void (*callback)(struct virtqueue *vq),
+				  const char *name,
+				  u16 vector)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;

@@ -408,7 +398,7 @@ static struct virtqueue *vp_find_vq(stru
 	vq->priv = info;
 	info->vq = vq;
 
-	 if (vector != VIRTIO_MSI_NO_VECTOR) {
+	if (vector != VIRTIO_MSI_NO_VECTOR) {
 		iowrite16(vector, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
 		vector = ioread16(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
 		if (vector == VIRTIO_MSI_NO_VECTOR) {

@@ -484,14 +474,36 @@ static int vp_try_to_find_vqs(struct vir
 			      struct virtqueue *vqs[],
 			      vq_callback_t *callbacks[],
 			      const char *names[],
-			      int nvectors,
+			      bool use_msix,
 			      bool per_vq_vectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	u16 vector;
-	int i, err, allocated_vectors;
+	int i, err, nvectors, allocated_vectors;

drivers/virtio/virtio_pci.c +26 -18

+	}
+
+	if (per_vq_vectors) {
+		/* Best option: one for change interrupt, one per vq. */
+		nvectors = 1;
+		for (i = 0; i < nvqs; ++i)
+			if (callbacks[i])
+				++nvectors;
+	} else {
+		/* Second best: one for change, shared one for all vqs. */
+		nvectors = 2;
+	}
+
+	err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
 	if (err)
 		goto error_request;
 

@@ -504,15 +516,17 @@ static int vp_try_to_find_vqs(struct vir
 			vector = allocated_vectors++;
 		else
 			vector = VP_MSIX_VQ_VECTOR;
-		vqs[i] = vp_find_vq(vdev, i, callbacks[i], names[i], vector);
+		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], vector);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto error_find;
 		}
 		/* allocate per-vq irq if available and necessary */
 		if (vp_dev->per_vq_vectors && vector != VIRTIO_MSI_NO_VECTOR) {
-			snprintf(vp_dev->msix_names[vector], sizeof *vp_dev->msix_names,
-				 "%s-%s", dev_name(&vp_dev->vdev.dev), names[i]);
+			snprintf(vp_dev->msix_names[vector],
+				 sizeof *vp_dev->msix_names,
+				 "%s-%s",
+				 dev_name(&vp_dev->vdev.dev), names[i]);
 			err = request_irq(vp_dev->msix_entries[vector].vector,
 					  vring_interrupt, 0,
 					  vp_dev->msix_names[vector], vqs[i]);

@@ -537,28 +551,20 @@ static int vp_find_vqs(struct virtio_dev
 		       vq_callback_t *callbacks[],
 		       const char *names[])
 {
-	int vectors = 0;
-	int i, uninitialized_var(err);
+	int err;
 
-	/* How many vectors would we like? */
-	for (i = 0; i < nvqs; ++i)
-		if (callbacks[i])
-			++vectors;
-
-	/* We want at most one vector per queue and one for config changes. */
-	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
-				 vectors + 1, true);
+	/* Try MSI-X with one vector per queue. */
+	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names, true, true);
 	if (!err)
 		return 0;
-	/* Fallback to separate vectors for config and a shared for queues. */
+	/* Fallback: MSI-X with one vector for config, one shared for queues. */
 	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
-				 2, false);
+				 true, false);
 	if (!err)
 		return 0;
 	/* Finally fall back to regular interrupts. */
-	err = vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
-				 0, false);
-	return err;
+	return vp_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
+				  false, false);
 }
 
 static struct virtio_config_ops virtio_pci_config_ops = {

Quoted 20 lines · Rusty Russell

Quoted 7 lines · Michael S. Tsirkin

This refactors find_vqs, making it more readable and robust, and fixing two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector (reported on old host kernels)

An older version of this patch was tested by Amit Shah.

OK, I've applied both of these; I'd like to see a new test by Amit to make sure tho.

I really like this cleanup! I looked harder at this code, and my best attempts to untangle it further came to very little. This is what I ended up with, but it's all cosmetic and can wait until next merge window. See what you think.

Thanks!
Rusty.

Quoted 4 lines

virtio_pci: minor MSI-X cleanups

1) Rename vp_request_vectors to vp_request_msix_vectors, and take non-MSI-X case out to caller.

I'm not sure this change was for the best: we still have a separate code path under if !use_msix, only in another place now. See below. And this seems to break the symmetry between request_ and free_vectors.

Review

Quoted 9 lines

(quoted diff) +8 -1

-	err = vp_request_vectors(vdev, nvectors, per_vq_vectors);
+	if (!use_msix) {
+		/* Old style: one normal interrupt for change and all vqs. */
+		vp_dev->msix_vectors = 0;
+		vp_dev->per_vq_vectors = false;
+		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
+				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
+		if (!err)
+			vp_dev->intx_enabled = 1;

shorter as vp_dev->intx_enabled = !err

Review

Quoted 1 line

(quoted diff) +1 -0

+		return err;

Is that all? Don't we need to create the vqs?

Quoted 20 lines · Rusty Russell

Quoted 7 lines · Michael S. Tsirkin

This refactors find_vqs, making it more readable and robust, and fixing two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector (reported on old host kernels)

An older version of this patch was tested by Amit Shah.

OK, I've applied both of these; I'd like to see a new test by Amit to make sure tho.

I really like this cleanup! I looked harder at this code, and my best attempts to untangle it further came to very little. This is what I ended up with, but it's all cosmetic and can wait until next merge window. See what you think.

Thanks!
Rusty.

Quoted 4 lines

virtio_pci: minor MSI-X cleanups

1) Rename vp_request_vectors to vp_request_msix_vectors, and take non-MSI-X case out to caller.

I'm not sure this change was for the best: we still have a separate code path under if !use_msix, only in another place now. See below. And this seems to break the symmetry between request_ and free_vectors.

Review

Quoted 9 lines

(quoted diff) +8 -1

-	err = vp_request_vectors(vdev, nvectors, per_vq_vectors);
+	if (!use_msix) {
+		/* Old style: one normal interrupt for change and all vqs. */
+		vp_dev->msix_vectors = 0;
+		vp_dev->per_vq_vectors = false;
+		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
+				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
+		if (!err)
+			vp_dev->intx_enabled = 1;

shorter as vp_dev->intx_enabled = !err

Review

Quoted 1 line

(quoted diff) +1 -0

+		return err;

Is that all? Don't we need to create the vqs?

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Rusty Russell

Quoted 7 lines · Michael S. Tsirkin

This refactors find_vqs, making it more readable and robust, and fixing two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector (reported on old host kernels)

An older version of this patch was tested by Amit Shah.

OK, I've applied both of these; I'd like to see a new test by Amit to make sure tho.

I really like this cleanup! I looked harder at this code, and my best attempts to untangle it further came to very little. This is what I ended up with, but it's all cosmetic and can wait until next merge window. See what you think.

Thanks!
Rusty.

Quoted 8 lines

Quoted 4 lines

virtio_pci: minor MSI-X cleanups

1) Rename vp_request_vectors to vp_request_msix_vectors, and take non-MSI-X case out to caller.

I'm not sure this change was for the best: we still have a separate code path under if !use_msix, only in another place now. See below. And this seems to break the symmetry between request_ and free_vectors.

Yes, but unfortunately request_vectors was never really symmetrical :(

That's because we didn't do the request_irq's for the per_vector case, because we don't have the names. This is what prevented me from doing a nice encapsulation.

Quoted 12 lines

Review

Quoted 9 lines

(quoted diff) +8 -1

-	err = vp_request_vectors(vdev, nvectors, per_vq_vectors);
+	if (!use_msix) {
+		/* Old style: one normal interrupt for change and all vqs. */
+		vp_dev->msix_vectors = 0;
+		vp_dev->per_vq_vectors = false;
+		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
+				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
+		if (!err)
+			vp_dev->intx_enabled = 1;

shorter as vp_dev->intx_enabled = !err

Quoted 3 lines

drivers/virtio/virtio_pci.c static int vp_try_to_find_vqs(struct vir · line 491

Quoted 1 line

drivers/virtio/virtio_pci.c +1 -0

+		return err;

Is that all? Don't we need to create the vqs?

Oh, yeah :)

This patch applies on top. Basically reverts that part, and renames vector to msix_vector, which I think makes the code more readable.

virtio: more PCI minor cleanups

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/virtio/virtio_pci.c |   73 +++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 37 deletions(-)

drivers/virtio/virtio_pci.c +2 -2

 };
 
 /* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */

@@ -349,7 +349,7 @@ error:
 static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 				  void (*callback)(struct virtqueue *vq),
 				  const char *name,
-				  u16 vector)
+				  u16 msix_vector)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;

@@ -374,7 +374,7 @@ static struct virtqueue *setup_vq(struct
 
 	info->queue_index = index;
 	info->num = num;
-	info->vector = vector;
+	info->msix_vector = msix_vector;
 
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
 	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);

drivers/virtio/virtio_pci.c +3 -1

+		if (msix_vector == VIRTIO_MSI_NO_VECTOR) {
 			err = -EBUSY;
 			goto out_assign;
 		}

@@ -462,7 +462,8 @@ static void vp_del_vqs(struct virtio_dev
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
 		info = vq->priv;
 		if (vp_dev->per_vq_vectors)
-			free_irq(vp_dev->msix_entries[info->vector].vector, vq);
+			free_irq(vp_dev->msix_entries[info->msix_vector].vector,
+				 vq);
 		vp_del_vq(vq);
 	}
 	vp_dev->per_vq_vectors = false;

@@ -478,58 +479,56 @@ static int vp_try_to_find_vqs(struct vir
 			      bool per_vq_vectors)

drivers/virtio/virtio_pci.c +14 -24

+		}
+
+		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
+		if (err)
+			goto error_request;
 	}
 
-	if (per_vq_vectors) {
-		/* Best option: one for change interrupt, one per vq. */
-		nvectors = 1;
-		for (i = 0; i < nvqs; ++i)
-			if (callbacks[i])
-				++nvectors;
-	} else {
-		/* Second best: one for change, shared one for all vqs. */
-		nvectors = 2;
-	}
-
-	err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
-	if (err)
-		goto error_request;
-
 	vp_dev->per_vq_vectors = per_vq_vectors;
 	allocated_vectors = vp_dev->msix_used_vectors;
 	for (i = 0; i < nvqs; ++i) {
 		if (!callbacks[i] || !vp_dev->msix_enabled)
-			vector = VIRTIO_MSI_NO_VECTOR;
+			msix_vector = VIRTIO_MSI_NO_VECTOR;
 		else if (vp_dev->per_vq_vectors)
-			vector = allocated_vectors++;
+			msix_vector = allocated_vectors++;
 		else
-			vector = VP_MSIX_VQ_VECTOR;
-		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], vector);
+			msix_vector = VP_MSIX_VQ_VECTOR;
+		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vector);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto error_find;
 		}
 		/* allocate per-vq irq if available and necessary */
-		if (vp_dev->per_vq_vectors && vector != VIRTIO_MSI_NO_VECTOR) {
-			snprintf(vp_dev->msix_names[vector],
+		if (vp_dev->per_vq_vectors) {
+			snprintf(vp_dev->msix_names[msix_vector],
 				 sizeof *vp_dev->msix_names,
 				 "%s-%s",
 				 dev_name(&vp_dev->vdev.dev), names[i]);
-			err = request_irq(vp_dev->msix_entries[vector].vector,
-					  vring_interrupt, 0,
-					  vp_dev->msix_names[vector], vqs[i]);
+			err = request_irq(msix_vector, vring_interrupt, 0,
+					  vp_dev->msix_names[msix_vector],
+					  vqs[i]);
 			if (err) {
 				vp_del_vq(vqs[i]);
 				goto error_find;

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Rusty Russell

Quoted 7 lines · Michael S. Tsirkin

This refactors find_vqs, making it more readable and robust, and fixing two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector (reported on old host kernels)

An older version of this patch was tested by Amit Shah.

OK, I've applied both of these; I'd like to see a new test by Amit to make sure tho.

I really like this cleanup! I looked harder at this code, and my best attempts to untangle it further came to very little. This is what I ended up with, but it's all cosmetic and can wait until next merge window. See what you think.

Thanks!
Rusty.

Quoted 8 lines

Quoted 4 lines

virtio_pci: minor MSI-X cleanups

1) Rename vp_request_vectors to vp_request_msix_vectors, and take non-MSI-X case out to caller.

I'm not sure this change was for the best: we still have a separate code path under if !use_msix, only in another place now. See below. And this seems to break the symmetry between request_ and free_vectors.

Yes, but unfortunately request_vectors was never really symmetrical :(

That's because we didn't do the request_irq's for the per_vector case, because we don't have the names. This is what prevented me from doing a nice encapsulation.

Quoted 12 lines

Review

Quoted 9 lines

(quoted diff) +8 -1

-	err = vp_request_vectors(vdev, nvectors, per_vq_vectors);
+	if (!use_msix) {
+		/* Old style: one normal interrupt for change and all vqs. */
+		vp_dev->msix_vectors = 0;
+		vp_dev->per_vq_vectors = false;
+		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
+				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
+		if (!err)
+			vp_dev->intx_enabled = 1;

shorter as vp_dev->intx_enabled = !err

Quoted 3 lines

drivers/virtio/virtio_pci.c static int vp_try_to_find_vqs(struct vir · line 491

Quoted 1 line

drivers/virtio/virtio_pci.c +1 -0

+		return err;

Is that all? Don't we need to create the vqs?

Oh, yeah :)

This patch applies on top. Basically reverts that part, and renames vector to msix_vector, which I think makes the code more readable.

virtio: more PCI minor cleanups

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/virtio/virtio_pci.c |   73 +++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 37 deletions(-)

drivers/virtio/virtio_pci.c +2 -2

 };
 
 /* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */

@@ -349,7 +349,7 @@ error:
 static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 				  void (*callback)(struct virtqueue *vq),
 				  const char *name,
-				  u16 vector)
+				  u16 msix_vector)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;

@@ -374,7 +374,7 @@ static struct virtqueue *setup_vq(struct
 
 	info->queue_index = index;
 	info->num = num;
-	info->vector = vector;
+	info->msix_vector = msix_vector;
 
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
 	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);

drivers/virtio/virtio_pci.c +3 -1

+		if (msix_vector == VIRTIO_MSI_NO_VECTOR) {
 			err = -EBUSY;
 			goto out_assign;
 		}

@@ -462,7 +462,8 @@ static void vp_del_vqs(struct virtio_dev
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
 		info = vq->priv;
 		if (vp_dev->per_vq_vectors)
-			free_irq(vp_dev->msix_entries[info->vector].vector, vq);
+			free_irq(vp_dev->msix_entries[info->msix_vector].vector,
+				 vq);
 		vp_del_vq(vq);
 	}
 	vp_dev->per_vq_vectors = false;

@@ -478,58 +479,56 @@ static int vp_try_to_find_vqs(struct vir
 			      bool per_vq_vectors)

drivers/virtio/virtio_pci.c +14 -24

+		}
+
+		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
+		if (err)
+			goto error_request;
 	}
 
-	if (per_vq_vectors) {
-		/* Best option: one for change interrupt, one per vq. */
-		nvectors = 1;
-		for (i = 0; i < nvqs; ++i)
-			if (callbacks[i])
-				++nvectors;
-	} else {
-		/* Second best: one for change, shared one for all vqs. */
-		nvectors = 2;
-	}
-
-	err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
-	if (err)
-		goto error_request;
-
 	vp_dev->per_vq_vectors = per_vq_vectors;
 	allocated_vectors = vp_dev->msix_used_vectors;
 	for (i = 0; i < nvqs; ++i) {
 		if (!callbacks[i] || !vp_dev->msix_enabled)
-			vector = VIRTIO_MSI_NO_VECTOR;
+			msix_vector = VIRTIO_MSI_NO_VECTOR;
 		else if (vp_dev->per_vq_vectors)
-			vector = allocated_vectors++;
+			msix_vector = allocated_vectors++;
 		else
-			vector = VP_MSIX_VQ_VECTOR;
-		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], vector);
+			msix_vector = VP_MSIX_VQ_VECTOR;
+		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vector);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto error_find;
 		}
 		/* allocate per-vq irq if available and necessary */
-		if (vp_dev->per_vq_vectors && vector != VIRTIO_MSI_NO_VECTOR) {
-			snprintf(vp_dev->msix_names[vector],
+		if (vp_dev->per_vq_vectors) {
+			snprintf(vp_dev->msix_names[msix_vector],
 				 sizeof *vp_dev->msix_names,
 				 "%s-%s",
 				 dev_name(&vp_dev->vdev.dev), names[i]);
-			err = request_irq(vp_dev->msix_entries[vector].vector,
-					  vring_interrupt, 0,
-					  vp_dev->msix_names[vector], vqs[i]);
+			err = request_irq(msix_vector, vring_interrupt, 0,
+					  vp_dev->msix_names[msix_vector],
+					  vqs[i]);
 			if (err) {
 				vp_del_vq(vqs[i]);
 				goto error_find;

Quoted 3 lines

That's because we didn't do the request_irq's for the per_vector case, because we don't have the names. This is what prevented me from doing a nice encapsulation.

Yes. But let's split free_vectors out into free_msix_vectors and free_intx as well?

Yes, I agree, this is good cleanup, structure field names should be desriptive. Are you sure we want to make all local variables named vector renamed to msix_vector though? CodingStyle says local var names should be short ... A couple of ideas below.

Review

Quoted 7 lines

(quoted diff) +3 -4

-	if (vector != VIRTIO_MSI_NO_VECTOR) {
-		iowrite16(vector, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
-		vector = ioread16(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
-		if (vector == VIRTIO_MSI_NO_VECTOR) {
+	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
+		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
+		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);

checkpatch will complain that space is lacking around "+". We won't have a problem if we keep it named vector.

Review

Quoted 4 lines

(quoted diff) +0 -1

 	if (!use_msix) {
 		/* Old style: one normal interrupt for change and all vqs. */
 		vp_dev->msix_vectors = 0;
-		vp_dev->per_vq_vectors = false;

I know it's enough to look at msix_vectors, but isn't it cleaner to have per_vq_vectors consistent as well? E.g. del_vqs seems to only look at per_vq_vectors.

Review

Quoted 7 lines

(quoted diff) +2 -3

 		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
 				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
-		if (!err)
-			vp_dev->intx_enabled = 1;
-		return err;
+		if (err)
+			return err;

Maybe move this part out into a separate function? This way vp_try_to_find_vqs does high-level processing.

Quoted 5 lines

(quoted diff) +5 -0

+	} else {
+		if (per_vq_vectors) {
+			/* Best option: one for change interrupt, one per vq. */
+			nvectors = 1;
+			for (i = 0; i < nvqs; ++i)

Review

Quoted 5 lines

(quoted diff) +5 -0

+				if (callbacks[i])
+					++nvectors;
+		} else {
+			/* Second best: one for change, shared for all vqs. */
+			nvectors = 2;

Out of curiosity: why do you put {} here? They aren't strictly necessary ...

Quoted 3 lines

That's because we didn't do the request_irq's for the per_vector case, because we don't have the names. This is what prevented me from doing a nice encapsulation.

Yes. But let's split free_vectors out into free_msix_vectors and free_intx as well?

Yes, I agree, this is good cleanup, structure field names should be desriptive. Are you sure we want to make all local variables named vector renamed to msix_vector though? CodingStyle says local var names should be short ... A couple of ideas below.

Review

Quoted 7 lines

(quoted diff) +3 -4

-	if (vector != VIRTIO_MSI_NO_VECTOR) {
-		iowrite16(vector, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
-		vector = ioread16(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
-		if (vector == VIRTIO_MSI_NO_VECTOR) {
+	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
+		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
+		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);

checkpatch will complain that space is lacking around "+". We won't have a problem if we keep it named vector.

Review

Quoted 4 lines

(quoted diff) +0 -1

 	if (!use_msix) {
 		/* Old style: one normal interrupt for change and all vqs. */
 		vp_dev->msix_vectors = 0;
-		vp_dev->per_vq_vectors = false;

I know it's enough to look at msix_vectors, but isn't it cleaner to have per_vq_vectors consistent as well? E.g. del_vqs seems to only look at per_vq_vectors.

Review

Quoted 7 lines

(quoted diff) +2 -3

 		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
 				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
-		if (!err)
-			vp_dev->intx_enabled = 1;
-		return err;
+		if (err)
+			return err;

Maybe move this part out into a separate function? This way vp_try_to_find_vqs does high-level processing.

Quoted 5 lines

(quoted diff) +5 -0

+	} else {
+		if (per_vq_vectors) {
+			/* Best option: one for change interrupt, one per vq. */
+			nvectors = 1;
+			for (i = 0; i < nvqs; ++i)

Review

Quoted 5 lines

(quoted diff) +5 -0

+				if (callbacks[i])
+					++nvectors;
+		} else {
+			/* Second best: one for change, shared for all vqs. */
+			nvectors = 2;

Out of curiosity: why do you put {} here? They aren't strictly necessary ...

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines

That's because we didn't do the request_irq's for the per_vector case, because we don't have the names. This is what prevented me from doing a nice encapsulation.

Yes. But let's split free_vectors out into free_msix_vectors and free_intx as well?

Perhaps. Patch welcome :)

Quoted 4 lines

Yes, I agree, this is good cleanup, structure field names should be desriptive. Are you sure we want to make all local variables named vector renamed to msix_vector though? CodingStyle says local var names should be short ... A couple of ideas below.

msix_vec would work.

But vector for something which isn't always the vector is misleading, IMHO.

Quoted 10 lines

Review

Quoted 7 lines

(quoted diff) +3 -4

-	if (vector != VIRTIO_MSI_NO_VECTOR) {
-		iowrite16(vector, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
-		vector = ioread16(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
-		if (vector == VIRTIO_MSI_NO_VECTOR) {
+	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
+		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
+		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);

checkpatch will complain that space is lacking around "+". We won't have a problem if we keep it named vector.

Yeah, but OTOH ignoring checkpatch warnings is good for the soul.

Quoted 8 lines

drivers/virtio/virtio_pci.c static int vp_try_to_find_vqs(struct vir

Quoted 4 lines

drivers/virtio/virtio_pci.c +0 -1

 	if (!use_msix) {
 		/* Old style: one normal interrupt for change and all vqs. */
 		vp_dev->msix_vectors = 0;
-		vp_dev->per_vq_vectors = false;

I know it's enough to look at msix_vectors, but isn't it cleaner to have per_vq_vectors consistent as well? E.g. del_vqs seems to only look at per_vq_vectors.

This should in fact be:

(quoted diff) +3 -5

 	if (!use_msix) {
 		/* Old style: one normal interrupt for change and all vqs. */
-		vp_dev->msix_vectors = 0;
-		vp_dev->per_vq_vectors = false;
 		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
 				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
-		if (!err)
-			vp_dev->intx_enabled = 1;
-		return err;
+		if (err)
+			return err;
+		vp_dev->intx_enabled = 1;

The msix fields should all be ignored if msix_enabled is false.

Think about
running valgrind (or equiv) over the code: if you initialize something which shouldn't be accessed, valgrind won't spot it.

For similar reasons, I dislike memseting structs to 0, or kzallocing them.

Quoted 10 lines

Review

Quoted 7 lines

(quoted diff) +2 -3

 		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
 				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
-		if (!err)
-			vp_dev->intx_enabled = 1;
-		return err;
+		if (err)
+			return err;

Maybe move this part out into a separate function? This way vp_try_to_find_vqs does high-level processing.

Nice. Moved this to vp_request_intx().

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+	} else {
+		if (per_vq_vectors) {
+			/* Best option: one for change interrupt, one per vq. */
+			nvectors = 1;
+			for (i = 0; i < nvqs; ++i)

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+				if (callbacks[i])
+					++nvectors;
+		} else {
+			/* Second best: one for change, shared for all vqs. */
+			nvectors = 2;

Out of curiosity: why do you put {} here? They aren't strictly necessary ...

Comment made it multiline, looked a bit neater?

Here's the diff result:

drivers/virtio/virtio_pci.c +28 -18

@@ -346,10 +346,22 @@ error:
 	return err;
 }
 
+static int vp_request_intx(struct virtio_device *vdev)
+{
+	int err;
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+
+	err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
+			  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
+	if (!err)
+		vp_dev->intx_enabled = 1;
+	return err;
+}
+
 static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 				  void (*callback)(struct virtqueue *vq),
 				  const char *name,
-				  u16 msix_vector)
+				  u16 msix_vec)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;

@@ -374,7 +386,7 @@ static struct virtqueue *setup_vq(struct
 
 	info->queue_index = index;
 	info->num = num;
-	info->msix_vector = msix_vector;
+	info->msix_vector = msix_vec;
 
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
 	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);

@@ -398,10 +410,10 @@ static struct virtqueue *setup_vq(struct
 	vq->priv = info;
 	info->vq = vq;
 
-	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
-		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
-		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
-		if (msix_vector == VIRTIO_MSI_NO_VECTOR) {
+	if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
+		iowrite16(msix_vec, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
+		msix_vec = ioread16(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
+		if (msix_vec == VIRTIO_MSI_NO_VECTOR) {
 			err = -EBUSY;
 			goto out_assign;
 		}

@@ -479,16 +491,14 @@ static int vp_try_to_find_vqs(struct vir
 			      bool per_vq_vectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	u16 msix_vector;
+	u16 msix_vec;
 	int i, err, nvectors, allocated_vectors;
 
 	if (!use_msix) {
 		/* Old style: one normal interrupt for change and all vqs. */
-		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
-				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
+		err = vp_request_intx(vdev);
 		if (err)
-			return err;
-		vp_dev->intx_enabled = 1;
+			goto error_request;
 	} else {
 		if (per_vq_vectors) {
 			/* Best option: one for change interrupt, one per vq. */

@@ -510,24 +520,24 @@ static int vp_try_to_find_vqs(struct vir
 	allocated_vectors = vp_dev->msix_used_vectors;
 	for (i = 0; i < nvqs; ++i) {
 		if (!callbacks[i] || !vp_dev->msix_enabled)
-			msix_vector = VIRTIO_MSI_NO_VECTOR;
+			msix_vec = VIRTIO_MSI_NO_VECTOR;
 		else if (vp_dev->per_vq_vectors)
-			msix_vector = allocated_vectors++;
+			msix_vec = allocated_vectors++;
 		else
-			msix_vector = VP_MSIX_VQ_VECTOR;
-		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vector);
+			msix_vec = VP_MSIX_VQ_VECTOR;
+		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vec);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto error_find;
 		}
 		/* allocate per-vq irq if available and necessary */
 		if (vp_dev->per_vq_vectors) {
-			snprintf(vp_dev->msix_names[msix_vector],
+			snprintf(vp_dev->msix_names[msix_vec],
 				 sizeof *vp_dev->msix_names,
 				 "%s-%s",
 				 dev_name(&vp_dev->vdev.dev), names[i]);
-			err = request_irq(msix_vector, vring_interrupt, 0,
-					  vp_dev->msix_names[msix_vector],
+			err = request_irq(msix_vec, vring_interrupt, 0,
+					  vp_dev->msix_names[msix_vec],
 					  vqs[i]);
 			if (err) {
 				vp_del_vq(vqs[i]);

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines

That's because we didn't do the request_irq's for the per_vector case, because we don't have the names. This is what prevented me from doing a nice encapsulation.

Yes. But let's split free_vectors out into free_msix_vectors and free_intx as well?

Perhaps. Patch welcome :)

Quoted 4 lines

Yes, I agree, this is good cleanup, structure field names should be desriptive. Are you sure we want to make all local variables named vector renamed to msix_vector though? CodingStyle says local var names should be short ... A couple of ideas below.

msix_vec would work.

But vector for something which isn't always the vector is misleading, IMHO.

Quoted 10 lines

Review

Quoted 7 lines

(quoted diff) +3 -4

-	if (vector != VIRTIO_MSI_NO_VECTOR) {
-		iowrite16(vector, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
-		vector = ioread16(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
-		if (vector == VIRTIO_MSI_NO_VECTOR) {
+	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
+		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
+		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);

checkpatch will complain that space is lacking around "+". We won't have a problem if we keep it named vector.

Yeah, but OTOH ignoring checkpatch warnings is good for the soul.

Quoted 8 lines

drivers/virtio/virtio_pci.c static int vp_try_to_find_vqs(struct vir

Quoted 4 lines

drivers/virtio/virtio_pci.c +0 -1

 	if (!use_msix) {
 		/* Old style: one normal interrupt for change and all vqs. */
 		vp_dev->msix_vectors = 0;
-		vp_dev->per_vq_vectors = false;

I know it's enough to look at msix_vectors, but isn't it cleaner to have per_vq_vectors consistent as well? E.g. del_vqs seems to only look at per_vq_vectors.

This should in fact be:

(quoted diff) +3 -5

 	if (!use_msix) {
 		/* Old style: one normal interrupt for change and all vqs. */
-		vp_dev->msix_vectors = 0;
-		vp_dev->per_vq_vectors = false;
 		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
 				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
-		if (!err)
-			vp_dev->intx_enabled = 1;
-		return err;
+		if (err)
+			return err;
+		vp_dev->intx_enabled = 1;

The msix fields should all be ignored if msix_enabled is false.

Think about
running valgrind (or equiv) over the code: if you initialize something which shouldn't be accessed, valgrind won't spot it.

For similar reasons, I dislike memseting structs to 0, or kzallocing them.

Quoted 10 lines

Review

Quoted 7 lines

(quoted diff) +2 -3

 		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
 				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
-		if (!err)
-			vp_dev->intx_enabled = 1;
-		return err;
+		if (err)
+			return err;

Maybe move this part out into a separate function? This way vp_try_to_find_vqs does high-level processing.

Nice. Moved this to vp_request_intx().

Quoted 5 lines

Quoted 5 lines

(quoted diff) +5 -0

+	} else {
+		if (per_vq_vectors) {
+			/* Best option: one for change interrupt, one per vq. */
+			nvectors = 1;
+			for (i = 0; i < nvqs; ++i)

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+				if (callbacks[i])
+					++nvectors;
+		} else {
+			/* Second best: one for change, shared for all vqs. */
+			nvectors = 2;

Out of curiosity: why do you put {} here? They aren't strictly necessary ...

Comment made it multiline, looked a bit neater?

Here's the diff result:

drivers/virtio/virtio_pci.c +28 -18

@@ -346,10 +346,22 @@ error:
 	return err;
 }
 
+static int vp_request_intx(struct virtio_device *vdev)
+{
+	int err;
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+
+	err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
+			  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
+	if (!err)
+		vp_dev->intx_enabled = 1;
+	return err;
+}
+
 static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 				  void (*callback)(struct virtqueue *vq),
 				  const char *name,
-				  u16 msix_vector)
+				  u16 msix_vec)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;

@@ -374,7 +386,7 @@ static struct virtqueue *setup_vq(struct
 
 	info->queue_index = index;
 	info->num = num;
-	info->msix_vector = msix_vector;
+	info->msix_vector = msix_vec;
 
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
 	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);

@@ -398,10 +410,10 @@ static struct virtqueue *setup_vq(struct
 	vq->priv = info;
 	info->vq = vq;
 
-	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
-		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
-		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
-		if (msix_vector == VIRTIO_MSI_NO_VECTOR) {
+	if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
+		iowrite16(msix_vec, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
+		msix_vec = ioread16(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
+		if (msix_vec == VIRTIO_MSI_NO_VECTOR) {
 			err = -EBUSY;
 			goto out_assign;
 		}

@@ -479,16 +491,14 @@ static int vp_try_to_find_vqs(struct vir
 			      bool per_vq_vectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	u16 msix_vector;
+	u16 msix_vec;
 	int i, err, nvectors, allocated_vectors;
 
 	if (!use_msix) {
 		/* Old style: one normal interrupt for change and all vqs. */
-		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
-				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
+		err = vp_request_intx(vdev);
 		if (err)
-			return err;
-		vp_dev->intx_enabled = 1;
+			goto error_request;
 	} else {
 		if (per_vq_vectors) {
 			/* Best option: one for change interrupt, one per vq. */

@@ -510,24 +520,24 @@ static int vp_try_to_find_vqs(struct vir
 	allocated_vectors = vp_dev->msix_used_vectors;
 	for (i = 0; i < nvqs; ++i) {
 		if (!callbacks[i] || !vp_dev->msix_enabled)
-			msix_vector = VIRTIO_MSI_NO_VECTOR;
+			msix_vec = VIRTIO_MSI_NO_VECTOR;
 		else if (vp_dev->per_vq_vectors)
-			msix_vector = allocated_vectors++;
+			msix_vec = allocated_vectors++;
 		else
-			msix_vector = VP_MSIX_VQ_VECTOR;
-		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vector);
+			msix_vec = VP_MSIX_VQ_VECTOR;
+		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vec);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto error_find;
 		}
 		/* allocate per-vq irq if available and necessary */
 		if (vp_dev->per_vq_vectors) {
-			snprintf(vp_dev->msix_names[msix_vector],
+			snprintf(vp_dev->msix_names[msix_vec],
 				 sizeof *vp_dev->msix_names,
 				 "%s-%s",
 				 dev_name(&vp_dev->vdev.dev), names[i]);
-			err = request_irq(msix_vector, vring_interrupt, 0,
-					  vp_dev->msix_names[msix_vector],
+			err = request_irq(msix_vec, vring_interrupt, 0,
+					  vp_dev->msix_names[msix_vec],
 					  vqs[i]);
 			if (err) {
 				vp_del_vq(vqs[i]);

Quoted 5 lines · Rusty Russell

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines

That's because we didn't do the request_irq's for the per_vector case, because we don't have the names. This is what prevented me from doing a nice encapsulation.

Quoted 4 lines

Quoted 2 lines

Yes. But let's split free_vectors out into free_msix_vectors and free_intx as well?

Perhaps. Patch welcome :)

Could you put the end result somewhere so I can work on top of it?

Quoted 2 lines

But vector for something which isn't always the vector is misleading, IMHO.

I think you mean it's isn't always used? It's always a vector ...

BTW, let's get rid of msix_enabled completely?
We can always use msix_vectors ...

Quoted 5 lines · Rusty Russell

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines

That's because we didn't do the request_irq's for the per_vector case, because we don't have the names. This is what prevented me from doing a nice encapsulation.

Quoted 4 lines

Quoted 2 lines

Yes. But let's split free_vectors out into free_msix_vectors and free_intx as well?

Perhaps. Patch welcome :)

Could you put the end result somewhere so I can work on top of it?

Quoted 2 lines

But vector for something which isn't always the vector is misleading, IMHO.

I think you mean it's isn't always used? It's always a vector ...

BTW, let's get rid of msix_enabled completely?
We can always use msix_vectors ...

Quoted 6 lines · Michael S. Tsirkin

Quoted 5 lines · Rusty Russell

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines

That's because we didn't do the request_irq's for the per_vector case, because we don't have the names. This is what prevented me from doing a nice encapsulation.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Yes. But let's split free_vectors out into free_msix_vectors and free_intx as well?

Perhaps. Patch welcome :)

Could you put the end result somewhere so I can work on top of it?

Sure, it'll hit linux-next tomorrow, otherwise you can steal from
http://ozlabs.org/~rusty/kernel/rr-latest (virtio:pci-minor-cleanups.patch and virtio:pci-minor-cleanups-fix.patch).

Quoted 4 lines

Quoted 2 lines

But vector for something which isn't always the vector is misleading, IMHO.

I think you mean it's isn't always used? It's always a vector ...

The non-MSI case, it's set to VIRTIO_MSI_NO_VECTOR, and we use a normal interrupt vector.

Quoted 2 lines

BTW, let's get rid of msix_enabled completely?
We can always use msix_vectors ...

That would be nice. But yes, requiring more audit.

Ideally, if msix_vectors == 0, implies intx_enabled.

Thanks,
Rusty.

Quoted 6 lines · Michael S. Tsirkin

Quoted 5 lines · Rusty Russell

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines

That's because we didn't do the request_irq's for the per_vector case, because we don't have the names. This is what prevented me from doing a nice encapsulation.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Yes. But let's split free_vectors out into free_msix_vectors and free_intx as well?

Perhaps. Patch welcome :)

Could you put the end result somewhere so I can work on top of it?

Sure, it'll hit linux-next tomorrow, otherwise you can steal from
http://ozlabs.org/~rusty/kernel/rr-latest (virtio:pci-minor-cleanups.patch and virtio:pci-minor-cleanups-fix.patch).

Quoted 4 lines

Quoted 2 lines

But vector for something which isn't always the vector is misleading, IMHO.

I think you mean it's isn't always used? It's always a vector ...

The non-MSI case, it's set to VIRTIO_MSI_NO_VECTOR, and we use a normal interrupt vector.

Quoted 2 lines

BTW, let's get rid of msix_enabled completely?
We can always use msix_vectors ...

That would be nice. But yes, requiring more audit.

Ideally, if msix_vectors == 0, implies intx_enabled.

Thanks,
Rusty.

Quoted 7 lines · Michael S. Tsirkin

This refactors find_vqs, making it more readable and robust, and fixing two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector (reported on old host kernels)

An older version of this patch was tested by Amit Shah.

OK, I've applied both of these; I'd like to see a new test by Amit to make sure tho.

Quoted 7 lines · Michael S. Tsirkin

This refactors find_vqs, making it more readable and robust, and fixing two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector (reported on old host kernels)

An older version of this patch was tested by Amit Shah.

OK, I've applied both of these; I'd like to see a new test by Amit to make sure tho.

Quoted 7 lines · Rusty Russell

Quoted 6 lines · Michael S. Tsirkin

This refactors find_vqs, making it more readable and robust, and fixing two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector (reported on old host kernels)

Quoted 4 lines

Quoted 1 line

An older version of this patch was tested by Amit Shah.

OK, I've applied both of these; I'd like to see a new test by Amit to make sure tho.

Tested these patches as well. They work fine.

Tested-by Amit Shah <[email protected]>

Quoted 7 lines · Rusty Russell

Quoted 6 lines · Michael S. Tsirkin

This refactors find_vqs, making it more readable and robust, and fixing two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector (reported on old host kernels)

Quoted 4 lines

Quoted 1 line

An older version of this patch was tested by Amit Shah.

OK, I've applied both of these; I'd like to see a new test by Amit to make sure tho.

Tested these patches as well. They work fine.

Tested-by Amit Shah <[email protected]>