Re: [PATCH] ifcvf: move IRQ request/free to status change handlers

[Francesco Lavra]

On 5/11/20 11:26 AM, 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.
>
>
> > 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);

That's exactly what the code does: ifcvf_free_irq() contains a (i = 0; i 
< queues; i++) loop, and here the function is called with the `queues` 
argument set to `i`.