Re: [PATCH] ifcvf: move IRQ request/free to status change handlers
From: Jason Wang
Date: Mon May 11 2020 - 23:38:38 EST
On 2020/5/11 äå6:11, Zhu, Lingshan wrote:
On 5/11/2020 5:26 PM, Jason Wang wrote:
On 2020/5/11 äå3:19, Zhu Lingshan wrote:
This commit move IRQ request and free operations from probe()
to VIRTIO status change handler to comply with VIRTIO spec.
VIRTIO spec 1.1, section 2.1.2 Device Requirements: Device Status Field
The device MUST NOT consume buffers or send any used buffer
notifications to the driver before DRIVER_OK.
My previous explanation might be wrong here. It depends on how you
implement your hardware, if you hardware guarantee that no interrupt
will be triggered before DRIVER_OK, then it's fine.
And the main goal for this patch is to allocate the interrupt on demand.
Hi Jason,
So these code can a double assurance.
Signed-off-by: Zhu Lingshan <lingshan.zhu@xxxxxxxxx>
---
 drivers/vdpa/ifcvf/ifcvf_main.c | 119
++++++++++++++++++++++++----------------
 1 file changed, 73 insertions(+), 46 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index abf6a061..4d58bf2 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -28,6 +28,60 @@ static irqreturn_t ifcvf_intr_handler(int irq,
void *arg)
ÂÂÂÂÂ return IRQ_HANDLED;
 }
 +static void ifcvf_free_irq_vectors(void *data)
+{
+ÂÂÂ pci_free_irq_vectors(data);
+}
+
+static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
+{
+ÂÂÂ struct pci_dev *pdev = adapter->pdev;
+ÂÂÂ struct ifcvf_hw *vf = &adapter->vf;
+ÂÂÂ int i;
+
+
+ÂÂÂ for (i = 0; i < queues; i++)
+ÂÂÂÂÂÂÂ devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
+
+ÂÂÂ ifcvf_free_irq_vectors(pdev);
+}
+
+static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
+{
+ÂÂÂ struct pci_dev *pdev = adapter->pdev;
+ÂÂÂ struct ifcvf_hw *vf = &adapter->vf;
+ÂÂÂ int vector, i, ret, irq;
+
+ÂÂÂ ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ IFCVF_MAX_INTR, PCI_IRQ_MSIX);
+ÂÂÂ if (ret < 0) {
+ÂÂÂÂÂÂÂ IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
+
+ÂÂÂ for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
+ÂÂÂÂÂÂÂ snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
+ÂÂÂÂÂÂÂÂÂÂÂÂ pci_name(pdev), i);
+ÂÂÂÂÂÂÂ vector = i + IFCVF_MSI_QUEUE_OFF;
+ÂÂÂÂÂÂÂ irq = pci_irq_vector(pdev, vector);
+ÂÂÂÂÂÂÂ ret = devm_request_irq(&pdev->dev, irq,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ifcvf_intr_handler, 0,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ vf->vring[i].msix_name,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &vf->vring[i]);
+ÂÂÂÂÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂÂÂÂÂ IFCVF_ERR(pdev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "Failed to request irq for vq %d\n", i);
+ÂÂÂÂÂÂÂÂÂÂÂ ifcvf_free_irq(adapter, i);
I'm not sure this unwind is correct. It looks like we should loop and
call devm_free_irq() for virtqueue [0, i);
we have the loop in ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues),
it takes a parameter queues, and a loop
+ÂÂÂ for (i = 0; i < queues; i++)+ÂÂÂÂÂÂÂ devm_free_irq(&pdev->dev,
vf->vring[i].irq, &vf->vring[i]); will free irq for vq[0,queues)
Aha, I get this.
+
+ÂÂÂÂÂÂÂÂÂÂÂ return ret;
+ÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂ vf->vring[i].irq = irq;
+ÂÂÂ }
+
+ÂÂÂ return 0;
+}
+
 static int ifcvf_start_datapath(void *private)
 {
ÂÂÂÂÂ struct ifcvf_hw *vf = ifcvf_private_to_vf(private);
@@ -118,9 +172,12 @@ static void ifcvf_vdpa_set_status(struct
vdpa_device *vdpa_dev, u8 status)
 {
ÂÂÂÂÂ struct ifcvf_adapter *adapter;
ÂÂÂÂÂ struct ifcvf_hw *vf;
+ÂÂÂ u8 status_old;
+ÂÂÂ int ret;
 Â vf = vdpa_to_vf(vdpa_dev);
ÂÂÂÂÂ adapter = dev_get_drvdata(vdpa_dev->dev.parent);
+ÂÂÂ status_old = ifcvf_get_status(vf);
 Â if (status == 0) {
ÂÂÂÂÂÂÂÂÂ ifcvf_stop_datapath(adapter);
@@ -128,7 +185,22 @@ static void ifcvf_vdpa_set_status(struct
vdpa_device *vdpa_dev, u8 status)
ÂÂÂÂÂÂÂÂÂ return;
ÂÂÂÂÂ }
 - if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
+ÂÂÂ if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
+ÂÂÂÂÂÂÂ !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+ÂÂÂÂÂÂÂ ifcvf_stop_datapath(adapter);
+ÂÂÂÂÂÂÂ ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
+ÂÂÂ }
+
+ÂÂÂ if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+ÂÂÂÂÂÂÂ !(status_old & VIRTIO_CONFIG_S_DRIVER_OK)) {
+ÂÂÂÂÂÂÂ ret = ifcvf_request_irq(adapter);
+ÂÂÂÂÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂÂÂÂÂ status = ifcvf_get_status(vf);
+ÂÂÂÂÂÂÂÂÂÂÂ status |= VIRTIO_CONFIG_S_FAILED;
+ÂÂÂÂÂÂÂÂÂÂÂ ifcvf_set_status(vf, status);
+ÂÂÂÂÂÂÂÂÂÂÂ return;
+ÂÂÂÂÂÂÂ }
+
Have a hard though on the logic here.
This depends on the status setting from guest or userspace. Which
means it can not deal with e.g when qemu or userspace is crashed? Do
we need to care this or it's a over engineering?
Thanks
If qemu crash, I guess users may re-run qmeu / re-initialize the device, according to the spec, there should be a reset routine.
This code piece handles status change on DRIVER_OK flipping. I am not sure I get your point, mind to give more hints?
The problem is if we don't launch new qemu instance, the interrupt will
be still there?
Thanks
Thanks,
BR
Zhu Lingshan
ÂÂÂÂÂÂÂÂÂ if (ifcvf_start_datapath(adapter) < 0)
ÂÂÂÂÂÂÂÂÂÂÂÂÂ IFCVF_ERR(adapter->pdev,
 "Failed to set ifcvf vdpa status %u\n",
@@ -284,38 +356,6 @@ static void ifcvf_vdpa_set_config_cb(struct
vdpa_device *vdpa_dev,
 .set_config_cb = ifcvf_vdpa_set_config_cb,
 };
 -static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
-{
-ÂÂÂ struct pci_dev *pdev = adapter->pdev;
-ÂÂÂ struct ifcvf_hw *vf = &adapter->vf;
-ÂÂÂ int vector, i, ret, irq;
-
-
-ÂÂÂ for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
-ÂÂÂÂÂÂÂ snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
-ÂÂÂÂÂÂÂÂÂÂÂÂ pci_name(pdev), i);
-ÂÂÂÂÂÂÂ vector = i + IFCVF_MSI_QUEUE_OFF;
-ÂÂÂÂÂÂÂ irq = pci_irq_vector(pdev, vector);
-ÂÂÂÂÂÂÂ ret = devm_request_irq(&pdev->dev, irq,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ifcvf_intr_handler, 0,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ vf->vring[i].msix_name,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &vf->vring[i]);
-ÂÂÂÂÂÂÂ if (ret) {
-ÂÂÂÂÂÂÂÂÂÂÂ IFCVF_ERR(pdev,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "Failed to request irq for vq %d\n", i);
-ÂÂÂÂÂÂÂÂÂÂÂ return ret;
-ÂÂÂÂÂÂÂ }
-ÂÂÂÂÂÂÂ vf->vring[i].irq = irq;
-ÂÂÂ }
-
-ÂÂÂ return 0;
-}
-
-static void ifcvf_free_irq_vectors(void *data)
-{
-ÂÂÂ pci_free_irq_vectors(data);
-}
-
 static int ifcvf_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
 {
ÂÂÂÂÂ struct device *dev = &pdev->dev;
@@ -349,13 +389,6 @@ static int ifcvf_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
ÂÂÂÂÂÂÂÂÂ return ret;
ÂÂÂÂÂ }
 - ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ IFCVF_MAX_INTR, PCI_IRQ_MSIX);
-ÂÂÂ if (ret < 0) {
-ÂÂÂÂÂÂÂ IFCVF_ERR(pdev, "Failed to alloc irq vectors\n");
-ÂÂÂÂÂÂÂ return ret;
-ÂÂÂ }
-
ÂÂÂÂÂ ret = devm_add_action_or_reset(dev, ifcvf_free_irq_vectors,
pdev);
ÂÂÂÂÂ if (ret) {
ÂÂÂÂÂÂÂÂÂ IFCVF_ERR(pdev,
@@ -379,12 +412,6 @@ static int ifcvf_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
ÂÂÂÂÂ adapter->pdev = pdev;
ÂÂÂÂÂ adapter->vdpa.dma_dev = &pdev->dev;
 - ret = ifcvf_request_irq(adapter);
-ÂÂÂ if (ret) {
-ÂÂÂÂÂÂÂ IFCVF_ERR(pdev, "Failed to request MSI-X irq\n");
-ÂÂÂÂÂÂÂ goto err;
-ÂÂÂ }
-
ÂÂÂÂÂ ret = ifcvf_init_hw(vf, pdev);
ÂÂÂÂÂ if (ret) {
ÂÂÂÂÂÂÂÂÂ IFCVF_ERR(pdev, "Failed to init IFCVF hw\n");