[dpdk-dev,v2,2/2] igb_uio: fix interrupt enablement after FLR in VM

Message ID 1507586960-35508-1-git-send-email-jingjing.wu@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Jingjing Wu Oct. 9, 2017, 10:09 p.m. UTC
  If pass-through a VF by vfio-pci to a Qemu VM, after FLR
in VM, the interrupt setting is not recoverd correctly
to host as below:
 in VM guest:
        Capabilities: [70] MSI-X: Enable+ Count=5 Masked-
 in Host:
        Capabilities: [70] MSI-X: Enable+ Count=5 Masked-

That was because in pci_reset_function, it first reads the
PCI configure and set FLR reset, and then writes PCI configure
as restoration. But not all the writing are successful to Host.
Because vfio-pci driver doesn't allow directly write PCI MSI-X
Cap.

To fix this issue, we need to move the interrupt enablement from
igb_uio probe to open device file. While it is also the similar as
the behaviour in vfio_pci kernel module code.

Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")

Cc: stable@dpdk.org

Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
v2 change:
 - fix typo
 - remove duplicated debug info

 lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 221 ++++++++++++++++--------------
 1 file changed, 119 insertions(+), 102 deletions(-)
  

Comments

Jingjing Wu Oct. 12, 2017, 5:43 a.m. UTC | #1
Hi, Shjith

Could you help to review and verify if this fix can meet your requirement?

Thanks
Jingjing

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Tuesday, October 10, 2017 6:09 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Tan, Jianfeng
> <jianfeng.tan@intel.com>; shijith.thotton@caviumnetworks.com;
> gregory@weka.io; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
> Subject: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM
> 
> If pass-through a VF by vfio-pci to a Qemu VM, after FLR in VM, the interrupt
> setting is not recoverd correctly to host as below:
>  in VM guest:
>         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-  in Host:
>         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-
> 
> That was because in pci_reset_function, it first reads the PCI configure and set
> FLR reset, and then writes PCI configure as restoration. But not all the writing
> are successful to Host.
> Because vfio-pci driver doesn't allow directly write PCI MSI-X Cap.
> 
> To fix this issue, we need to move the interrupt enablement from igb_uio probe
> to open device file. While it is also the similar as the behaviour in vfio_pci
> kernel module code.
> 
> Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
> v2 change:
>  - fix typo
>  - remove duplicated debug info
> 
>  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 221 ++++++++++++++++--------------
>  1 file changed, 119 insertions(+), 102 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> index c8dd5f4..d943795 100644
> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> @@ -49,7 +49,6 @@ struct rte_uio_pci_dev {
> 
>  static char *intr_mode;
>  static enum rte_intr_mode igbuio_intr_mode_preferred =
> RTE_INTR_MODE_MSIX;
> -
>  /* sriov sysfs */
>  static ssize_t
>  show_max_vfs(struct device *dev, struct device_attribute *attr, @@ -144,8
> +143,10 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
>   * If yes, disable it here and will be enable later.
>   */
>  static irqreturn_t
> -igbuio_pci_irqhandler(int irq, struct uio_info *info)
> +igbuio_pci_irqhandler(int irq, void *dev_id)
>  {
> +	struct uio_device *idev = (struct uio_device *)dev_id;
> +	struct uio_info *info = idev->info;
>  	struct rte_uio_pci_dev *udev = info->priv;
> 
>  	/* Legacy mode need to mask in hardware */ @@ -153,10 +154,115
> @@ igbuio_pci_irqhandler(int irq, struct uio_info *info)
>  	    !pci_check_and_mask_intx(udev->pdev))
>  		return IRQ_NONE;
> 
> +	uio_event_notify(info);
> +
>  	/* Message signal mode, no share IRQ and automasked */
>  	return IRQ_HANDLED;
>  }
> 
> +static int
> +igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev) {
> +	int err = 0;
> +#ifndef HAVE_ALLOC_IRQ_VECTORS
> +	struct msix_entry msix_entry;
> +#endif
> +
> +	switch (igbuio_intr_mode_preferred) {
> +	case RTE_INTR_MODE_MSIX:
> +		/* Only 1 msi-x vector needed */
> +#ifndef HAVE_ALLOC_IRQ_VECTORS
> +		msix_entry.entry = 0;
> +		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
> +			dev_dbg(&udev->pdev->dev, "using MSI-X");
> +			udev->info.irq_flags = IRQF_NO_THREAD;
> +			udev->info.irq = msix_entry.vector;
> +			udev->mode = RTE_INTR_MODE_MSIX;
> +			break;
> +		}
> +#else
> +		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1)
> {
> +			dev_dbg(&udev->pdev->dev, "using MSI-X");
> +			udev->info.irq_flags = IRQF_NO_THREAD;
> +			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> +			udev->mode = RTE_INTR_MODE_MSIX;
> +			break;
> +		}
> +#endif
> +
> +	/* fall back to MSI */
> +	case RTE_INTR_MODE_MSI:
> +#ifndef HAVE_ALLOC_IRQ_VECTORS
> +		if (pci_enable_msi(udev->pdev) == 0) {
> +			dev_dbg(&udev->pdev->dev, "using MSI");
> +			udev->info.irq_flags = IRQF_NO_THREAD;
> +			udev->info.irq = udev->pdev->irq;
> +			udev->mode = RTE_INTR_MODE_MSI;
> +			break;
> +		}
> +#else
> +		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1)
> {
> +			dev_dbg(&udev->pdev->dev, "using MSI");
> +			udev->info.irq_flags = IRQF_NO_THREAD;
> +			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> +			udev->mode = RTE_INTR_MODE_MSI;
> +			break;
> +		}
> +#endif
> +	/* fall back to INTX */
> +	case RTE_INTR_MODE_LEGACY:
> +		if (pci_intx_mask_supported(udev->pdev)) {
> +			dev_dbg(&udev->pdev->dev, "using INTX");
> +			udev->info.irq_flags = IRQF_SHARED |
> IRQF_NO_THREAD;
> +			udev->info.irq = udev->pdev->irq;
> +			udev->mode = RTE_INTR_MODE_LEGACY;
> +			break;
> +		}
> +		dev_notice(&udev->pdev->dev, "PCI INTX mask not
> supported\n");
> +	/* fall back to no IRQ */
> +	case RTE_INTR_MODE_NONE:
> +		udev->mode = RTE_INTR_MODE_NONE;
> +		udev->info.irq = UIO_IRQ_NONE;
> +		break;
> +
> +	default:
> +		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
> +			igbuio_intr_mode_preferred);
> +		udev->info.irq = UIO_IRQ_NONE;
> +		err = -EINVAL;
> +	}
> +
> +	if (udev->info.irq != UIO_IRQ_NONE)
> +		err = request_irq(udev->info.irq, igbuio_pci_irqhandler,
> +				  udev->info.irq_flags, udev->info.name,
> +				  udev->info.uio_dev);
> +	dev_info(&udev->pdev->dev, "uio device registered with irq %lx\n",
> +		 udev->info.irq);
> +
> +	return err;
> +}
> +
> +static void
> +igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev) {
> +	if (udev->info.irq) {
> +		free_irq(udev->info.irq, udev->info.uio_dev);
> +		udev->info.irq = 0;
> +	}
> +
> +#ifndef HAVE_ALLOC_IRQ_VECTORS
> +	if (udev->mode == RTE_INTR_MODE_MSIX)
> +		pci_disable_msix(udev->pdev);
> +	if (udev->mode == RTE_INTR_MODE_MSI)
> +		pci_disable_msi(udev->pdev);
> +#else
> +	if (udev->mode == RTE_INTR_MODE_MSIX ||
> +	    udev->mode == RTE_INTR_MODE_MSI)
> +		pci_free_irq_vectors(udev->pdev);
> +#endif
> +}
> +
> +
>  /**
>   * This gets called while opening uio device file.
>   */
> @@ -165,12 +271,19 @@ igbuio_pci_open(struct uio_info *info, struct inode
> *inode)  {
>  	struct rte_uio_pci_dev *udev = info->priv;
>  	struct pci_dev *dev = udev->pdev;
> +	int err;
> 
>  	pci_reset_function(dev);
> 
>  	/* set bus master, which was cleared by the reset function */
>  	pci_set_master(dev);
> 
> +	/* enable interrupts */
> +	err = igbuio_pci_enable_interrupts(udev);
> +	if (err) {
> +		dev_err(&dev->dev, "Enable interrupt fails\n");
> +		return err;
> +	}
>  	return 0;
>  }
> 
> @@ -180,6 +293,9 @@ igbuio_pci_release(struct uio_info *info, struct inode
> *inode)
>  	struct rte_uio_pci_dev *udev = info->priv;
>  	struct pci_dev *dev = udev->pdev;
> 
> +	/* disable interrupts */
> +	igbuio_pci_disable_interrupts(udev);
> +
>  	/* stop the device from further DMA */
>  	pci_clear_master(dev);
> 
> @@ -250,94 +366,6 @@ igbuio_pci_release_iomem(struct uio_info *info)  }
> 
>  static int
> -igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev) -{
> -	int err = 0;
> -#ifndef HAVE_ALLOC_IRQ_VECTORS
> -	struct msix_entry msix_entry;
> -#endif
> -
> -	switch (igbuio_intr_mode_preferred) {
> -	case RTE_INTR_MODE_MSIX:
> -		/* Only 1 msi-x vector needed */
> -#ifndef HAVE_ALLOC_IRQ_VECTORS
> -		msix_entry.entry = 0;
> -		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
> -			dev_dbg(&udev->pdev->dev, "using MSI-X");
> -			udev->info.irq_flags = IRQF_NO_THREAD;
> -			udev->info.irq = msix_entry.vector;
> -			udev->mode = RTE_INTR_MODE_MSIX;
> -			break;
> -		}
> -#else
> -		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1)
> {
> -			dev_dbg(&udev->pdev->dev, "using MSI-X");
> -			udev->info.irq_flags = IRQF_NO_THREAD;
> -			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> -			udev->mode = RTE_INTR_MODE_MSIX;
> -			break;
> -		}
> -#endif
> -	/* fall back to MSI */
> -	case RTE_INTR_MODE_MSI:
> -#ifndef HAVE_ALLOC_IRQ_VECTORS
> -		if (pci_enable_msi(udev->pdev) == 0) {
> -			dev_dbg(&udev->pdev->dev, "using MSI");
> -			udev->info.irq_flags = IRQF_NO_THREAD;
> -			udev->info.irq = udev->pdev->irq;
> -			udev->mode = RTE_INTR_MODE_MSI;
> -			break;
> -		}
> -#else
> -		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1)
> {
> -			dev_dbg(&udev->pdev->dev, "using MSI");
> -			udev->info.irq_flags = IRQF_NO_THREAD;
> -			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> -			udev->mode = RTE_INTR_MODE_MSI;
> -			break;
> -		}
> -#endif
> -	/* fall back to INTX */
> -	case RTE_INTR_MODE_LEGACY:
> -		if (pci_intx_mask_supported(udev->pdev)) {
> -			dev_dbg(&udev->pdev->dev, "using INTX");
> -			udev->info.irq_flags = IRQF_SHARED |
> IRQF_NO_THREAD;
> -			udev->info.irq = udev->pdev->irq;
> -			udev->mode = RTE_INTR_MODE_LEGACY;
> -			break;
> -		}
> -		dev_notice(&udev->pdev->dev, "PCI INTX mask not
> supported\n");
> -	/* fall back to no IRQ */
> -	case RTE_INTR_MODE_NONE:
> -		udev->mode = RTE_INTR_MODE_NONE;
> -		udev->info.irq = UIO_IRQ_NONE;
> -		break;
> -
> -	default:
> -		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
> -			igbuio_intr_mode_preferred);
> -		err = -EINVAL;
> -	}
> -
> -	return err;
> -}
> -
> -static void
> -igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev) -{ -#ifndef
> HAVE_ALLOC_IRQ_VECTORS
> -	if (udev->mode == RTE_INTR_MODE_MSIX)
> -		pci_disable_msix(udev->pdev);
> -	if (udev->mode == RTE_INTR_MODE_MSI)
> -		pci_disable_msi(udev->pdev);
> -#else
> -	if (udev->mode == RTE_INTR_MODE_MSIX ||
> -	    udev->mode == RTE_INTR_MODE_MSI)
> -		pci_free_irq_vectors(udev->pdev);
> -#endif
> -}
> -
> -static int
>  igbuio_setup_bars(struct pci_dev *dev, struct uio_info *info)  {
>  	int i, iom, iop, ret;
> @@ -427,20 +455,15 @@ igbuio_pci_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
>  	/* fill uio infos */
>  	udev->info.name = "igb_uio";
>  	udev->info.version = "0.1";
> -	udev->info.handler = igbuio_pci_irqhandler;
>  	udev->info.irqcontrol = igbuio_pci_irqcontrol;
>  	udev->info.open = igbuio_pci_open;
>  	udev->info.release = igbuio_pci_release;
>  	udev->info.priv = udev;
>  	udev->pdev = dev;
> 
> -	err = igbuio_pci_enable_interrupts(udev);
> -	if (err != 0)
> -		goto fail_release_iomem;
> -
>  	err = sysfs_create_group(&dev->dev.kobj, &dev_attr_grp);
>  	if (err != 0)
> -		goto fail_disable_interrupts;
> +		goto fail_release_iomem;
> 
>  	/* register uio driver */
>  	err = uio_register_device(&dev->dev, &udev->info); @@ -449,9 +472,6
> @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> 
>  	pci_set_drvdata(dev, udev);
> 
> -	dev_info(&dev->dev, "uio device registered with irq %lx\n",
> -		 udev->info.irq);
> -
>  	/*
>  	 * Doing a harmless dma mapping for attaching the device to
>  	 * the iommu identity mapping if kernel boots with iommu=pt.
> @@ -477,8 +497,6 @@ igbuio_pci_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
> 
>  fail_remove_group:
>  	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
> -fail_disable_interrupts:
> -	igbuio_pci_disable_interrupts(udev);
>  fail_release_iomem:
>  	igbuio_pci_release_iomem(&udev->info);
>  	pci_disable_device(dev);
> @@ -495,7 +513,6 @@ igbuio_pci_remove(struct pci_dev *dev)
> 
>  	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
>  	uio_unregister_device(&udev->info);
> -	igbuio_pci_disable_interrupts(udev);
>  	igbuio_pci_release_iomem(&udev->info);
>  	pci_disable_device(dev);
>  	pci_set_drvdata(dev, NULL);
> --
> 2.7.4
  
Shijith Thotton Oct. 13, 2017, 8:12 a.m. UTC | #2
On Thu, Oct 12, 2017 at 05:43:29AM +0000, Wu, Jingjing wrote:
> Hi, Shjith
> 
> Could you help to review and verify if this fix can meet your requirement?
> 
> Thanks
> Jingjing
> 
> > -----Original Message-----
> > From: Wu, Jingjing
> > Sent: Tuesday, October 10, 2017 6:09 AM
> > To: Yigit, Ferruh <ferruh.yigit@intel.com>; Tan, Jianfeng
> > <jianfeng.tan@intel.com>; shijith.thotton@caviumnetworks.com;
> > gregory@weka.io; Xing, Beilei <beilei.xing@intel.com>
> > Cc: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
> > Subject: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM
> > 
> > If pass-through a VF by vfio-pci to a Qemu VM, after FLR in VM, the interrupt
> > setting is not recoverd correctly to host as below:
> >  in VM guest:
> >         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-  in Host:
> >         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-
> > 
> > That was because in pci_reset_function, it first reads the PCI configure and set
> > FLR reset, and then writes PCI configure as restoration. But not all the writing
> > are successful to Host.
> > Because vfio-pci driver doesn't allow directly write PCI MSI-X Cap.
> > 
> > To fix this issue, we need to move the interrupt enablement from igb_uio probe
> > to open device file. While it is also the similar as the behaviour in vfio_pci
> > kernel module code.
> > 
> > Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")
> > 
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

Tested-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>

> > ---
> > v2 change:
> >  - fix typo
> >  - remove duplicated debug info
> > 
> >  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 221 ++++++++++++++++--------------
> >  1 file changed, 119 insertions(+), 102 deletions(-)
> > 
> > diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > index c8dd5f4..d943795 100644
> > --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > @@ -49,7 +49,6 @@ struct rte_uio_pci_dev {
> > 
> >  static char *intr_mode;
> >  static enum rte_intr_mode igbuio_intr_mode_preferred =
> > RTE_INTR_MODE_MSIX;
> > -
> >  /* sriov sysfs */
> >  static ssize_t
> >  show_max_vfs(struct device *dev, struct device_attribute *attr, @@ -144,8
> > +143,10 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
> >   * If yes, disable it here and will be enable later.
> >   */
> >  static irqreturn_t
> > -igbuio_pci_irqhandler(int irq, struct uio_info *info)
> > +igbuio_pci_irqhandler(int irq, void *dev_id)
> >  {
> > +	struct uio_device *idev = (struct uio_device *)dev_id;
> > +	struct uio_info *info = idev->info;
> >  	struct rte_uio_pci_dev *udev = info->priv;
> > 
> >  	/* Legacy mode need to mask in hardware */ @@ -153,10 +154,115
> > @@ igbuio_pci_irqhandler(int irq, struct uio_info *info)
> >  	    !pci_check_and_mask_intx(udev->pdev))
> >  		return IRQ_NONE;
> > 
> > +	uio_event_notify(info);
> > +
> >  	/* Message signal mode, no share IRQ and automasked */
> >  	return IRQ_HANDLED;
> >  }
> > 
> > +static int
> > +igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev) {
> > +	int err = 0;
> > +#ifndef HAVE_ALLOC_IRQ_VECTORS
> > +	struct msix_entry msix_entry;
> > +#endif
> > +
> > +	switch (igbuio_intr_mode_preferred) {
> > +	case RTE_INTR_MODE_MSIX:
> > +		/* Only 1 msi-x vector needed */
> > +#ifndef HAVE_ALLOC_IRQ_VECTORS
> > +		msix_entry.entry = 0;
> > +		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
> > +			dev_dbg(&udev->pdev->dev, "using MSI-X");
> > +			udev->info.irq_flags = IRQF_NO_THREAD;
> > +			udev->info.irq = msix_entry.vector;
> > +			udev->mode = RTE_INTR_MODE_MSIX;
> > +			break;
> > +		}
> > +#else
> > +		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1)
> > {
> > +			dev_dbg(&udev->pdev->dev, "using MSI-X");
> > +			udev->info.irq_flags = IRQF_NO_THREAD;
> > +			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> > +			udev->mode = RTE_INTR_MODE_MSIX;
> > +			break;
> > +		}
> > +#endif
> > +
> > +	/* fall back to MSI */
> > +	case RTE_INTR_MODE_MSI:
> > +#ifndef HAVE_ALLOC_IRQ_VECTORS
> > +		if (pci_enable_msi(udev->pdev) == 0) {
> > +			dev_dbg(&udev->pdev->dev, "using MSI");
> > +			udev->info.irq_flags = IRQF_NO_THREAD;
> > +			udev->info.irq = udev->pdev->irq;
> > +			udev->mode = RTE_INTR_MODE_MSI;
> > +			break;
> > +		}
> > +#else
> > +		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1)
> > {
> > +			dev_dbg(&udev->pdev->dev, "using MSI");
> > +			udev->info.irq_flags = IRQF_NO_THREAD;
> > +			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> > +			udev->mode = RTE_INTR_MODE_MSI;
> > +			break;
> > +		}
> > +#endif
> > +	/* fall back to INTX */
> > +	case RTE_INTR_MODE_LEGACY:
> > +		if (pci_intx_mask_supported(udev->pdev)) {
> > +			dev_dbg(&udev->pdev->dev, "using INTX");
> > +			udev->info.irq_flags = IRQF_SHARED |
> > IRQF_NO_THREAD;
> > +			udev->info.irq = udev->pdev->irq;
> > +			udev->mode = RTE_INTR_MODE_LEGACY;
> > +			break;
> > +		}
> > +		dev_notice(&udev->pdev->dev, "PCI INTX mask not
> > supported\n");
> > +	/* fall back to no IRQ */
> > +	case RTE_INTR_MODE_NONE:
> > +		udev->mode = RTE_INTR_MODE_NONE;
> > +		udev->info.irq = UIO_IRQ_NONE;
> > +		break;
> > +
> > +	default:
> > +		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
> > +			igbuio_intr_mode_preferred);
> > +		udev->info.irq = UIO_IRQ_NONE;
> > +		err = -EINVAL;
> > +	}
> > +
> > +	if (udev->info.irq != UIO_IRQ_NONE)
> > +		err = request_irq(udev->info.irq, igbuio_pci_irqhandler,
> > +				  udev->info.irq_flags, udev->info.name,
> > +				  udev->info.uio_dev);
> > +	dev_info(&udev->pdev->dev, "uio device registered with irq %lx\n",
> > +		 udev->info.irq);
> > +
> > +	return err;
> > +}
> > +
> > +static void
> > +igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev) {
> > +	if (udev->info.irq) {
> > +		free_irq(udev->info.irq, udev->info.uio_dev);
> > +		udev->info.irq = 0;
> > +	}
> > +
> > +#ifndef HAVE_ALLOC_IRQ_VECTORS
> > +	if (udev->mode == RTE_INTR_MODE_MSIX)
> > +		pci_disable_msix(udev->pdev);
> > +	if (udev->mode == RTE_INTR_MODE_MSI)
> > +		pci_disable_msi(udev->pdev);
> > +#else
> > +	if (udev->mode == RTE_INTR_MODE_MSIX ||
> > +	    udev->mode == RTE_INTR_MODE_MSI)
> > +		pci_free_irq_vectors(udev->pdev);
> > +#endif
> > +}
> > +
> > +
> >  /**
> >   * This gets called while opening uio device file.
> >   */
> > @@ -165,12 +271,19 @@ igbuio_pci_open(struct uio_info *info, struct inode
> > *inode)  {
> >  	struct rte_uio_pci_dev *udev = info->priv;
> >  	struct pci_dev *dev = udev->pdev;
> > +	int err;
> > 
> >  	pci_reset_function(dev);
> > 
> >  	/* set bus master, which was cleared by the reset function */
> >  	pci_set_master(dev);
> > 
> > +	/* enable interrupts */
> > +	err = igbuio_pci_enable_interrupts(udev);
> > +	if (err) {
> > +		dev_err(&dev->dev, "Enable interrupt fails\n");
> > +		return err;
> > +	}
> >  	return 0;
> >  }
> > 
> > @@ -180,6 +293,9 @@ igbuio_pci_release(struct uio_info *info, struct inode
> > *inode)
> >  	struct rte_uio_pci_dev *udev = info->priv;
> >  	struct pci_dev *dev = udev->pdev;
> > 
> > +	/* disable interrupts */
> > +	igbuio_pci_disable_interrupts(udev);
> > +
> >  	/* stop the device from further DMA */
> >  	pci_clear_master(dev);
> > 
> > @@ -250,94 +366,6 @@ igbuio_pci_release_iomem(struct uio_info *info)  }
> > 
> >  static int
> > -igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev) -{
> > -	int err = 0;
> > -#ifndef HAVE_ALLOC_IRQ_VECTORS
> > -	struct msix_entry msix_entry;
> > -#endif
> > -
> > -	switch (igbuio_intr_mode_preferred) {
> > -	case RTE_INTR_MODE_MSIX:
> > -		/* Only 1 msi-x vector needed */
> > -#ifndef HAVE_ALLOC_IRQ_VECTORS
> > -		msix_entry.entry = 0;
> > -		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
> > -			dev_dbg(&udev->pdev->dev, "using MSI-X");
> > -			udev->info.irq_flags = IRQF_NO_THREAD;
> > -			udev->info.irq = msix_entry.vector;
> > -			udev->mode = RTE_INTR_MODE_MSIX;
> > -			break;
> > -		}
> > -#else
> > -		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1)
> > {
> > -			dev_dbg(&udev->pdev->dev, "using MSI-X");
> > -			udev->info.irq_flags = IRQF_NO_THREAD;
> > -			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> > -			udev->mode = RTE_INTR_MODE_MSIX;
> > -			break;
> > -		}
> > -#endif
> > -	/* fall back to MSI */
> > -	case RTE_INTR_MODE_MSI:
> > -#ifndef HAVE_ALLOC_IRQ_VECTORS
> > -		if (pci_enable_msi(udev->pdev) == 0) {
> > -			dev_dbg(&udev->pdev->dev, "using MSI");
> > -			udev->info.irq_flags = IRQF_NO_THREAD;
> > -			udev->info.irq = udev->pdev->irq;
> > -			udev->mode = RTE_INTR_MODE_MSI;
> > -			break;
> > -		}
> > -#else
> > -		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1)
> > {
> > -			dev_dbg(&udev->pdev->dev, "using MSI");
> > -			udev->info.irq_flags = IRQF_NO_THREAD;
> > -			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> > -			udev->mode = RTE_INTR_MODE_MSI;
> > -			break;
> > -		}
> > -#endif
> > -	/* fall back to INTX */
> > -	case RTE_INTR_MODE_LEGACY:
> > -		if (pci_intx_mask_supported(udev->pdev)) {
> > -			dev_dbg(&udev->pdev->dev, "using INTX");
> > -			udev->info.irq_flags = IRQF_SHARED |
> > IRQF_NO_THREAD;
> > -			udev->info.irq = udev->pdev->irq;
> > -			udev->mode = RTE_INTR_MODE_LEGACY;
> > -			break;
> > -		}
> > -		dev_notice(&udev->pdev->dev, "PCI INTX mask not
> > supported\n");
> > -	/* fall back to no IRQ */
> > -	case RTE_INTR_MODE_NONE:
> > -		udev->mode = RTE_INTR_MODE_NONE;
> > -		udev->info.irq = UIO_IRQ_NONE;
> > -		break;
> > -
> > -	default:
> > -		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
> > -			igbuio_intr_mode_preferred);
> > -		err = -EINVAL;
> > -	}
> > -
> > -	return err;
> > -}
> > -
> > -static void
> > -igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev) -{ -#ifndef
> > HAVE_ALLOC_IRQ_VECTORS
> > -	if (udev->mode == RTE_INTR_MODE_MSIX)
> > -		pci_disable_msix(udev->pdev);
> > -	if (udev->mode == RTE_INTR_MODE_MSI)
> > -		pci_disable_msi(udev->pdev);
> > -#else
> > -	if (udev->mode == RTE_INTR_MODE_MSIX ||
> > -	    udev->mode == RTE_INTR_MODE_MSI)
> > -		pci_free_irq_vectors(udev->pdev);
> > -#endif
> > -}
> > -
> > -static int
> >  igbuio_setup_bars(struct pci_dev *dev, struct uio_info *info)  {
> >  	int i, iom, iop, ret;
> > @@ -427,20 +455,15 @@ igbuio_pci_probe(struct pci_dev *dev, const struct
> > pci_device_id *id)
> >  	/* fill uio infos */
> >  	udev->info.name = "igb_uio";
> >  	udev->info.version = "0.1";
> > -	udev->info.handler = igbuio_pci_irqhandler;
> >  	udev->info.irqcontrol = igbuio_pci_irqcontrol;
> >  	udev->info.open = igbuio_pci_open;
> >  	udev->info.release = igbuio_pci_release;
> >  	udev->info.priv = udev;
> >  	udev->pdev = dev;
> > 
> > -	err = igbuio_pci_enable_interrupts(udev);
> > -	if (err != 0)
> > -		goto fail_release_iomem;
> > -
> >  	err = sysfs_create_group(&dev->dev.kobj, &dev_attr_grp);
> >  	if (err != 0)
> > -		goto fail_disable_interrupts;
> > +		goto fail_release_iomem;
> > 
> >  	/* register uio driver */
> >  	err = uio_register_device(&dev->dev, &udev->info); @@ -449,9 +472,6
> > @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > 
> >  	pci_set_drvdata(dev, udev);
> > 
> > -	dev_info(&dev->dev, "uio device registered with irq %lx\n",
> > -		 udev->info.irq);
> > -
> >  	/*
> >  	 * Doing a harmless dma mapping for attaching the device to
> >  	 * the iommu identity mapping if kernel boots with iommu=pt.
> > @@ -477,8 +497,6 @@ igbuio_pci_probe(struct pci_dev *dev, const struct
> > pci_device_id *id)
> > 
> >  fail_remove_group:
> >  	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
> > -fail_disable_interrupts:
> > -	igbuio_pci_disable_interrupts(udev);
> >  fail_release_iomem:
> >  	igbuio_pci_release_iomem(&udev->info);
> >  	pci_disable_device(dev);
> > @@ -495,7 +513,6 @@ igbuio_pci_remove(struct pci_dev *dev)
> > 
> >  	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
> >  	uio_unregister_device(&udev->info);
> > -	igbuio_pci_disable_interrupts(udev);
> >  	igbuio_pci_release_iomem(&udev->info);
> >  	pci_disable_device(dev);
> >  	pci_set_drvdata(dev, NULL);
> > --
> > 2.7.4

Hi Jingjing,

This patch perfectly meets requirements as both resets are retained
(open and release). Tested it with LiquidIO NIC and it works fine.
I can see MSI-X re-enabled on each run with new patch.

Gregory, Harish,
Please verify the patch on your setup if possible.

Thanks,
Shijith
  
Ferruh Yigit Oct. 13, 2017, 9:11 p.m. UTC | #3
On 10/13/2017 9:12 AM, Shijith Thotton wrote:
<...>

> Hi Jingjing,
> 
> This patch perfectly meets requirements as both resets are retained
> (open and release). Tested it with LiquidIO NIC and it works fine.
> I can see MSI-X re-enabled on each run with new patch.
> 
> Gregory, Harish,
> Please verify the patch on your setup if possible.

Hi Gregory, Harish,

Did you able to test this patch?

Thanks,
ferruh

> 
> Thanks,
> Shijith
>
  
Patil, Harish Oct. 13, 2017, 9:21 p.m. UTC | #4
-----Original Message-----
From: dev <dev-bounces@dpdk.org> on behalf of Ferruh Yigit

<ferruh.yigit@intel.com>
Date: Friday, October 13, 2017 at 2:11 PM
To: "Thotton, Shijith" <Shijith.Thotton@cavium.com>, "Wu, Jingjing"
<jingjing.wu@intel.com>, Harish Patil <Harish.Patil@cavium.com>
Cc: "Tan, Jianfeng" <jianfeng.tan@intel.com>, "gregory@weka.io"
<gregory@weka.io>, "Xing, Beilei" <beilei.xing@intel.com>, "dev@dpdk.org"
<dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2 2/2] igb_uio: fix interrupt
enablement	after FLR in VM

>On 10/13/2017 9:12 AM, Shijith Thotton wrote:

><...>

>

>> Hi Jingjing,

>> 

>> This patch perfectly meets requirements as both resets are retained

>> (open and release). Tested it with LiquidIO NIC and it works fine.

>> I can see MSI-X re-enabled on each run with new patch.

>> 

>> Gregory, Harish,

>> Please verify the patch on your setup if possible.

>

>Hi Gregory, Harish,

>

>Did you able to test this patch?

>

>Thanks,

>Ferruh


[Harish] No, I haven’t.
BTW, the igb_uio change also caused PDA (physical device assignment) mode
failure, where the entire device is PCI passed through to VM). Earlier we
reported only for SR-IOV VF passthru scenario.
Thanks.

>

>> 

>> Thanks,

>> Shijith

>> 

>
  
Gregory Etelson Oct. 15, 2017, 3:10 a.m. UTC | #5
I'll start to build setup environment this week.
Regards,
Gregory

On Sat, Oct 14, 2017 at 12:11 AM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 10/13/2017 9:12 AM, Shijith Thotton wrote:
> <...>
>
> > Hi Jingjing,
> >
> > This patch perfectly meets requirements as both resets are retained
> > (open and release). Tested it with LiquidIO NIC and it works fine.
> > I can see MSI-X re-enabled on each run with new patch.
> >
> > Gregory, Harish,
> > Please verify the patch on your setup if possible.
>
> Hi Gregory, Harish,
>
> Did you able to test this patch?
>
> Thanks,
> ferruh
>
> >
> > Thanks,
> > Shijith
> >
>
>
  
Patil, Harish Oct. 16, 2017, 10:49 p.m. UTC | #6
From: Gregory Etelson <gregory@weka.io<mailto:gregory@weka.io>>

Date: Saturday, October 14, 2017 at 8:10 PM
To: Ferruh Yigit <ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>>
Cc: "Thotton, Shijith" <Shijith.Thotton@cavium.com<mailto:Shijith.Thotton@cavium.com>>, "Wu, Jingjing" <jingjing.wu@intel.com<mailto:jingjing.wu@intel.com>>, Harish Patil <Harish.Patil@cavium.com<mailto:Harish.Patil@cavium.com>>, "Tan, Jianfeng" <jianfeng.tan@intel.com<mailto:jianfeng.tan@intel.com>>, "Xing, Beilei" <beilei.xing@intel.com<mailto:beilei.xing@intel.com>>, "dev@dpdk.org<mailto:dev@dpdk.org>" <dev@dpdk.org<mailto:dev@dpdk.org>>, "stable@dpdk.org<mailto:stable@dpdk.org>" <stable@dpdk.org<mailto:stable@dpdk.org>>
Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM

I'll start to build setup environment this week.
Regards,
Gregory

On Sat, Oct 14, 2017 at 12:11 AM, Ferruh Yigit <ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>> wrote:
On 10/13/2017 9:12 AM, Shijith Thotton wrote:
<...>

> Hi Jingjing,

>

> This patch perfectly meets requirements as both resets are retained

> (open and release). Tested it with LiquidIO NIC and it works fine.

> I can see MSI-X re-enabled on each run with new patch.

>

> Gregory, Harish,

> Please verify the patch on your setup if possible.


Hi Gregory, Harish,

Did you able to test this patch?

Thanks,
ferruh

>

> Thanks,

> Shijith

>



Hi all,
Its not working for qede VF device.
So request to back out all igb_uio changes related to the patch:
igb_uio: issue FLR during open and release of device file

Thanks,
Harish
  
Ferruh Yigit Oct. 16, 2017, 11:52 p.m. UTC | #7
On 10/16/2017 3:49 PM, Patil, Harish wrote:
> From: Gregory Etelson <gregory@weka.io <mailto:gregory@weka.io>>
> Date: Saturday, October 14, 2017 at 8:10 PM
> To: Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>
> Cc: "Thotton, Shijith" <Shijith.Thotton@cavium.com
> <mailto:Shijith.Thotton@cavium.com>>, "Wu, Jingjing"
> <jingjing.wu@intel.com <mailto:jingjing.wu@intel.com>>, Harish Patil
> <Harish.Patil@cavium.com <mailto:Harish.Patil@cavium.com>>, "Tan,
> Jianfeng" <jianfeng.tan@intel.com <mailto:jianfeng.tan@intel.com>>,
> "Xing, Beilei" <beilei.xing@intel.com <mailto:beilei.xing@intel.com>>,
> "dev@dpdk.org <mailto:dev@dpdk.org>" <dev@dpdk.org
> <mailto:dev@dpdk.org>>, "stable@dpdk.org <mailto:stable@dpdk.org>"
> <stable@dpdk.org <mailto:stable@dpdk.org>>
> Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR
> in VM
> 
>     I'll start to build setup environment this week.
>     Regards,
>     Gregory
> 
>     On Sat, Oct 14, 2017 at 12:11 AM, Ferruh Yigit
>     <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
> 
>         On 10/13/2017 9:12 AM, Shijith Thotton wrote:
>         <...>
> 
>         > Hi Jingjing,
>         >
>         > This patch perfectly meets requirements as both resets are retained
>         > (open and release). Tested it with LiquidIO NIC and it works fine.
>         > I can see MSI-X re-enabled on each run with new patch.
>         >
>         > Gregory, Harish,
>         > Please verify the patch on your setup if possible.
> 
>         Hi Gregory, Harish,
> 
>         Did you able to test this patch?
> 
>         Thanks,
>         ferruh
> 
>         >
>         > Thanks,
>         > Shijith
>         >
> 
> 
> 
> Hi all,
> Its not working for qede VF device.
> So request to back out all igb_uio changes related to the patch:
> igb_uio: issue FLR during open and release of device file 

Hi Harish,

Thanks for testing. We tried.

For this release, agreed to revert back to original state, I will send a
patch soon.


But for further investigation in next release, can you please share more
details? What is not working exactly, what is your setup, any
kernel/DPDK log to share?

Thanks,
ferruh

> 
> Thanks,
> Harish
>
  
Patil, Harish Oct. 17, 2017, 1:32 a.m. UTC | #8
-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com>

Date: Monday, October 16, 2017 at 4:52 PM
To: Harish Patil <Harish.Patil@cavium.com>, Gregory Etelson
<gregory@weka.io>
Cc: "Thotton, Shijith" <Shijith.Thotton@cavium.com>, "Wu, Jingjing"
<jingjing.wu@intel.com>, "Tan, Jianfeng" <jianfeng.tan@intel.com>, "Xing,
Beilei" <beilei.xing@intel.com>, "dev@dpdk.org" <dev@dpdk.org>,
"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in
VM

>On 10/16/2017 3:49 PM, Patil, Harish wrote:

>> From: Gregory Etelson <gregory@weka.io <mailto:gregory@weka.io>>

>> Date: Saturday, October 14, 2017 at 8:10 PM

>> To: Ferruh Yigit <ferruh.yigit@intel.com

>><mailto:ferruh.yigit@intel.com>>

>> Cc: "Thotton, Shijith" <Shijith.Thotton@cavium.com

>> <mailto:Shijith.Thotton@cavium.com>>, "Wu, Jingjing"

>> <jingjing.wu@intel.com <mailto:jingjing.wu@intel.com>>, Harish Patil

>> <Harish.Patil@cavium.com <mailto:Harish.Patil@cavium.com>>, "Tan,

>> Jianfeng" <jianfeng.tan@intel.com <mailto:jianfeng.tan@intel.com>>,

>> "Xing, Beilei" <beilei.xing@intel.com <mailto:beilei.xing@intel.com>>,

>> "dev@dpdk.org <mailto:dev@dpdk.org>" <dev@dpdk.org

>> <mailto:dev@dpdk.org>>, "stable@dpdk.org <mailto:stable@dpdk.org>"

>> <stable@dpdk.org <mailto:stable@dpdk.org>>

>> Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR

>> in VM

>> 

>>     I'll start to build setup environment this week.

>>     Regards,

>>     Gregory

>> 

>>     On Sat, Oct 14, 2017 at 12:11 AM, Ferruh Yigit

>>     <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:

>> 

>>         On 10/13/2017 9:12 AM, Shijith Thotton wrote:

>>         <...>

>> 

>>         > Hi Jingjing,

>>         >

>>         > This patch perfectly meets requirements as both resets are

>>retained

>>         > (open and release). Tested it with LiquidIO NIC and it works

>>fine.

>>         > I can see MSI-X re-enabled on each run with new patch.

>>         >

>>         > Gregory, Harish,

>>         > Please verify the patch on your setup if possible.

>> 

>>         Hi Gregory, Harish,

>> 

>>         Did you able to test this patch?

>> 

>>         Thanks,

>>         ferruh

>> 

>>         >

>>         > Thanks,

>>         > Shijith

>>         >

>> 

>> 

>> 

>> Hi all,

>> Its not working for qede VF device.

>> So request to back out all igb_uio changes related to the patch:

>> igb_uio: issue FLR during open and release of device file

>

>Hi Harish,

>

>Thanks for testing. We tried.

>

>For this release, agreed to revert back to original state, I will send a

>patch soon.

>

>

>But for further investigation in next release, can you please share more

>details? What is not working exactly, what is your setup, any

>kernel/DPDK log to share?

>

>Thanks,

>Ferruh


Okay, sure.
thanks.
>

>> 

>> Thanks,

>> Harish

>> 

>
  
Jingjing Wu Oct. 17, 2017, 1:37 a.m. UTC | #9
> -----Original Message-----

> From: Patil, Harish [mailto:Harish.Patil@cavium.com]

> Sent: Tuesday, October 17, 2017 9:32 AM

> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Gregory Etelson <gregory@weka.io>

> Cc: Thotton, Shijith <Shijith.Thotton@cavium.com>; Wu, Jingjing

> <jingjing.wu@intel.com>; Tan, Jianfeng <jianfeng.tan@intel.com>; Xing, Beilei

> <beilei.xing@intel.com>; dev@dpdk.org; stable@dpdk.org

> Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM

> 

> 

> 

> -----Original Message-----

> From: Ferruh Yigit <ferruh.yigit@intel.com>

> Date: Monday, October 16, 2017 at 4:52 PM

> To: Harish Patil <Harish.Patil@cavium.com>, Gregory Etelson

> <gregory@weka.io>

> Cc: "Thotton, Shijith" <Shijith.Thotton@cavium.com>, "Wu, Jingjing"

> <jingjing.wu@intel.com>, "Tan, Jianfeng" <jianfeng.tan@intel.com>, "Xing,

> Beilei" <beilei.xing@intel.com>, "dev@dpdk.org" <dev@dpdk.org>,

> "stable@dpdk.org" <stable@dpdk.org>

> Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in

> VM

> 

> >On 10/16/2017 3:49 PM, Patil, Harish wrote:

> >> From: Gregory Etelson <gregory@weka.io <mailto:gregory@weka.io>>

> >> Date: Saturday, October 14, 2017 at 8:10 PM

> >> To: Ferruh Yigit <ferruh.yigit@intel.com

> >><mailto:ferruh.yigit@intel.com>>

> >> Cc: "Thotton, Shijith" <Shijith.Thotton@cavium.com

> >> <mailto:Shijith.Thotton@cavium.com>>, "Wu, Jingjing"

> >> <jingjing.wu@intel.com <mailto:jingjing.wu@intel.com>>, Harish Patil

> >> <Harish.Patil@cavium.com <mailto:Harish.Patil@cavium.com>>, "Tan,

> >> Jianfeng" <jianfeng.tan@intel.com <mailto:jianfeng.tan@intel.com>>,

> >> "Xing, Beilei" <beilei.xing@intel.com <mailto:beilei.xing@intel.com>>,

> >> "dev@dpdk.org <mailto:dev@dpdk.org>" <dev@dpdk.org

> >> <mailto:dev@dpdk.org>>, "stable@dpdk.org <mailto:stable@dpdk.org>"

> >> <stable@dpdk.org <mailto:stable@dpdk.org>>

> >> Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR

> >> in VM

> >>

> >>     I'll start to build setup environment this week.

> >>     Regards,

> >>     Gregory

> >>

> >>     On Sat, Oct 14, 2017 at 12:11 AM, Ferruh Yigit

> >>     <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:

> >>

> >>         On 10/13/2017 9:12 AM, Shijith Thotton wrote:

> >>         <...>

> >>

> >>         > Hi Jingjing,

> >>         >

> >>         > This patch perfectly meets requirements as both resets are

> >>retained

> >>         > (open and release). Tested it with LiquidIO NIC and it works

> >>fine.

> >>         > I can see MSI-X re-enabled on each run with new patch.

> >>         >

> >>         > Gregory, Harish,

> >>         > Please verify the patch on your setup if possible.

> >>

> >>         Hi Gregory, Harish,

> >>

> >>         Did you able to test this patch?

> >>

> >>         Thanks,

> >>         ferruh

> >>

> >>         >

> >>         > Thanks,

> >>         > Shijith

> >>         >

> >>

> >>

> >>

> >> Hi all,

> >> Its not working for qede VF device.

> >> So request to back out all igb_uio changes related to the patch:

> >> igb_uio: issue FLR during open and release of device file

> >

> >Hi Harish,

> >

> >Thanks for testing. We tried.

> >

> >For this release, agreed to revert back to original state, I will send a

> >patch soon.

> >

> >

> >But for further investigation in next release, can you please share more

> >details? What is not working exactly, what is your setup, any

> >kernel/DPDK log to share?

> >

> >Thanks,

> >Ferruh

> 

> Okay, sure.

> thanks.

> >

> >>

> >> Thanks,

> >> Harish

> >>

> >

Can you share more details? Only doesn't work in vfio pci passthrough?
  

Patch

diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
index c8dd5f4..d943795 100644
--- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
+++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
@@ -49,7 +49,6 @@  struct rte_uio_pci_dev {
 
 static char *intr_mode;
 static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
-
 /* sriov sysfs */
 static ssize_t
 show_max_vfs(struct device *dev, struct device_attribute *attr,
@@ -144,8 +143,10 @@  igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
  * If yes, disable it here and will be enable later.
  */
 static irqreturn_t
-igbuio_pci_irqhandler(int irq, struct uio_info *info)
+igbuio_pci_irqhandler(int irq, void *dev_id)
 {
+	struct uio_device *idev = (struct uio_device *)dev_id;
+	struct uio_info *info = idev->info;
 	struct rte_uio_pci_dev *udev = info->priv;
 
 	/* Legacy mode need to mask in hardware */
@@ -153,10 +154,115 @@  igbuio_pci_irqhandler(int irq, struct uio_info *info)
 	    !pci_check_and_mask_intx(udev->pdev))
 		return IRQ_NONE;
 
+	uio_event_notify(info);
+
 	/* Message signal mode, no share IRQ and automasked */
 	return IRQ_HANDLED;
 }
 
+static int
+igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
+{
+	int err = 0;
+#ifndef HAVE_ALLOC_IRQ_VECTORS
+	struct msix_entry msix_entry;
+#endif
+
+	switch (igbuio_intr_mode_preferred) {
+	case RTE_INTR_MODE_MSIX:
+		/* Only 1 msi-x vector needed */
+#ifndef HAVE_ALLOC_IRQ_VECTORS
+		msix_entry.entry = 0;
+		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
+			dev_dbg(&udev->pdev->dev, "using MSI-X");
+			udev->info.irq_flags = IRQF_NO_THREAD;
+			udev->info.irq = msix_entry.vector;
+			udev->mode = RTE_INTR_MODE_MSIX;
+			break;
+		}
+#else
+		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1) {
+			dev_dbg(&udev->pdev->dev, "using MSI-X");
+			udev->info.irq_flags = IRQF_NO_THREAD;
+			udev->info.irq = pci_irq_vector(udev->pdev, 0);
+			udev->mode = RTE_INTR_MODE_MSIX;
+			break;
+		}
+#endif
+
+	/* fall back to MSI */
+	case RTE_INTR_MODE_MSI:
+#ifndef HAVE_ALLOC_IRQ_VECTORS
+		if (pci_enable_msi(udev->pdev) == 0) {
+			dev_dbg(&udev->pdev->dev, "using MSI");
+			udev->info.irq_flags = IRQF_NO_THREAD;
+			udev->info.irq = udev->pdev->irq;
+			udev->mode = RTE_INTR_MODE_MSI;
+			break;
+		}
+#else
+		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1) {
+			dev_dbg(&udev->pdev->dev, "using MSI");
+			udev->info.irq_flags = IRQF_NO_THREAD;
+			udev->info.irq = pci_irq_vector(udev->pdev, 0);
+			udev->mode = RTE_INTR_MODE_MSI;
+			break;
+		}
+#endif
+	/* fall back to INTX */
+	case RTE_INTR_MODE_LEGACY:
+		if (pci_intx_mask_supported(udev->pdev)) {
+			dev_dbg(&udev->pdev->dev, "using INTX");
+			udev->info.irq_flags = IRQF_SHARED | IRQF_NO_THREAD;
+			udev->info.irq = udev->pdev->irq;
+			udev->mode = RTE_INTR_MODE_LEGACY;
+			break;
+		}
+		dev_notice(&udev->pdev->dev, "PCI INTX mask not supported\n");
+	/* fall back to no IRQ */
+	case RTE_INTR_MODE_NONE:
+		udev->mode = RTE_INTR_MODE_NONE;
+		udev->info.irq = UIO_IRQ_NONE;
+		break;
+
+	default:
+		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
+			igbuio_intr_mode_preferred);
+		udev->info.irq = UIO_IRQ_NONE;
+		err = -EINVAL;
+	}
+
+	if (udev->info.irq != UIO_IRQ_NONE)
+		err = request_irq(udev->info.irq, igbuio_pci_irqhandler,
+				  udev->info.irq_flags, udev->info.name,
+				  udev->info.uio_dev);
+	dev_info(&udev->pdev->dev, "uio device registered with irq %lx\n",
+		 udev->info.irq);
+
+	return err;
+}
+
+static void
+igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev)
+{
+	if (udev->info.irq) {
+		free_irq(udev->info.irq, udev->info.uio_dev);
+		udev->info.irq = 0;
+	}
+
+#ifndef HAVE_ALLOC_IRQ_VECTORS
+	if (udev->mode == RTE_INTR_MODE_MSIX)
+		pci_disable_msix(udev->pdev);
+	if (udev->mode == RTE_INTR_MODE_MSI)
+		pci_disable_msi(udev->pdev);
+#else
+	if (udev->mode == RTE_INTR_MODE_MSIX ||
+	    udev->mode == RTE_INTR_MODE_MSI)
+		pci_free_irq_vectors(udev->pdev);
+#endif
+}
+
+
 /**
  * This gets called while opening uio device file.
  */
@@ -165,12 +271,19 @@  igbuio_pci_open(struct uio_info *info, struct inode *inode)
 {
 	struct rte_uio_pci_dev *udev = info->priv;
 	struct pci_dev *dev = udev->pdev;
+	int err;
 
 	pci_reset_function(dev);
 
 	/* set bus master, which was cleared by the reset function */
 	pci_set_master(dev);
 
+	/* enable interrupts */
+	err = igbuio_pci_enable_interrupts(udev);
+	if (err) {
+		dev_err(&dev->dev, "Enable interrupt fails\n");
+		return err;
+	}
 	return 0;
 }
 
@@ -180,6 +293,9 @@  igbuio_pci_release(struct uio_info *info, struct inode *inode)
 	struct rte_uio_pci_dev *udev = info->priv;
 	struct pci_dev *dev = udev->pdev;
 
+	/* disable interrupts */
+	igbuio_pci_disable_interrupts(udev);
+
 	/* stop the device from further DMA */
 	pci_clear_master(dev);
 
@@ -250,94 +366,6 @@  igbuio_pci_release_iomem(struct uio_info *info)
 }
 
 static int
-igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
-{
-	int err = 0;
-#ifndef HAVE_ALLOC_IRQ_VECTORS
-	struct msix_entry msix_entry;
-#endif
-
-	switch (igbuio_intr_mode_preferred) {
-	case RTE_INTR_MODE_MSIX:
-		/* Only 1 msi-x vector needed */
-#ifndef HAVE_ALLOC_IRQ_VECTORS
-		msix_entry.entry = 0;
-		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
-			dev_dbg(&udev->pdev->dev, "using MSI-X");
-			udev->info.irq_flags = IRQF_NO_THREAD;
-			udev->info.irq = msix_entry.vector;
-			udev->mode = RTE_INTR_MODE_MSIX;
-			break;
-		}
-#else
-		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1) {
-			dev_dbg(&udev->pdev->dev, "using MSI-X");
-			udev->info.irq_flags = IRQF_NO_THREAD;
-			udev->info.irq = pci_irq_vector(udev->pdev, 0);
-			udev->mode = RTE_INTR_MODE_MSIX;
-			break;
-		}
-#endif
-	/* fall back to MSI */
-	case RTE_INTR_MODE_MSI:
-#ifndef HAVE_ALLOC_IRQ_VECTORS
-		if (pci_enable_msi(udev->pdev) == 0) {
-			dev_dbg(&udev->pdev->dev, "using MSI");
-			udev->info.irq_flags = IRQF_NO_THREAD;
-			udev->info.irq = udev->pdev->irq;
-			udev->mode = RTE_INTR_MODE_MSI;
-			break;
-		}
-#else
-		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1) {
-			dev_dbg(&udev->pdev->dev, "using MSI");
-			udev->info.irq_flags = IRQF_NO_THREAD;
-			udev->info.irq = pci_irq_vector(udev->pdev, 0);
-			udev->mode = RTE_INTR_MODE_MSI;
-			break;
-		}
-#endif
-	/* fall back to INTX */
-	case RTE_INTR_MODE_LEGACY:
-		if (pci_intx_mask_supported(udev->pdev)) {
-			dev_dbg(&udev->pdev->dev, "using INTX");
-			udev->info.irq_flags = IRQF_SHARED | IRQF_NO_THREAD;
-			udev->info.irq = udev->pdev->irq;
-			udev->mode = RTE_INTR_MODE_LEGACY;
-			break;
-		}
-		dev_notice(&udev->pdev->dev, "PCI INTX mask not supported\n");
-	/* fall back to no IRQ */
-	case RTE_INTR_MODE_NONE:
-		udev->mode = RTE_INTR_MODE_NONE;
-		udev->info.irq = UIO_IRQ_NONE;
-		break;
-
-	default:
-		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
-			igbuio_intr_mode_preferred);
-		err = -EINVAL;
-	}
-
-	return err;
-}
-
-static void
-igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev)
-{
-#ifndef HAVE_ALLOC_IRQ_VECTORS
-	if (udev->mode == RTE_INTR_MODE_MSIX)
-		pci_disable_msix(udev->pdev);
-	if (udev->mode == RTE_INTR_MODE_MSI)
-		pci_disable_msi(udev->pdev);
-#else
-	if (udev->mode == RTE_INTR_MODE_MSIX ||
-	    udev->mode == RTE_INTR_MODE_MSI)
-		pci_free_irq_vectors(udev->pdev);
-#endif
-}
-
-static int
 igbuio_setup_bars(struct pci_dev *dev, struct uio_info *info)
 {
 	int i, iom, iop, ret;
@@ -427,20 +455,15 @@  igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* fill uio infos */
 	udev->info.name = "igb_uio";
 	udev->info.version = "0.1";
-	udev->info.handler = igbuio_pci_irqhandler;
 	udev->info.irqcontrol = igbuio_pci_irqcontrol;
 	udev->info.open = igbuio_pci_open;
 	udev->info.release = igbuio_pci_release;
 	udev->info.priv = udev;
 	udev->pdev = dev;
 
-	err = igbuio_pci_enable_interrupts(udev);
-	if (err != 0)
-		goto fail_release_iomem;
-
 	err = sysfs_create_group(&dev->dev.kobj, &dev_attr_grp);
 	if (err != 0)
-		goto fail_disable_interrupts;
+		goto fail_release_iomem;
 
 	/* register uio driver */
 	err = uio_register_device(&dev->dev, &udev->info);
@@ -449,9 +472,6 @@  igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	pci_set_drvdata(dev, udev);
 
-	dev_info(&dev->dev, "uio device registered with irq %lx\n",
-		 udev->info.irq);
-
 	/*
 	 * Doing a harmless dma mapping for attaching the device to
 	 * the iommu identity mapping if kernel boots with iommu=pt.
@@ -477,8 +497,6 @@  igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 fail_remove_group:
 	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
-fail_disable_interrupts:
-	igbuio_pci_disable_interrupts(udev);
 fail_release_iomem:
 	igbuio_pci_release_iomem(&udev->info);
 	pci_disable_device(dev);
@@ -495,7 +513,6 @@  igbuio_pci_remove(struct pci_dev *dev)
 
 	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
 	uio_unregister_device(&udev->info);
-	igbuio_pci_disable_interrupts(udev);
 	igbuio_pci_release_iomem(&udev->info);
 	pci_disable_device(dev);
 	pci_set_drvdata(dev, NULL);