DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] igb_uio: revert open and release operations
@ 2017-10-17 20:14 Ferruh Yigit
  2017-10-17 20:33 ` Thomas Monjalon
                   ` (3 more replies)
  0 siblings, 4 replies; 38+ messages in thread
From: Ferruh Yigit @ 2017-10-17 20:14 UTC (permalink / raw)
  To: Thomas Monjalon, Ferruh Yigit
  Cc: dev, Jianfeng Tan, Jingjing Wu, Shijith Thotton, Gregory Etelson,
	Harish Patil, George Prekas, stable

This reverts commit 6b9ed026a8704b9e5ee5da7997617ef7cc82e114.
This reverts commit 5f6ff30dc5075c49069d684bab229aef7ff0fdc3.
This reverts commit b58eedfc7dd57eef6d12e2c654a52c834f36084a.

There were bug reports about terminated application may leave device in
undesired state:
http://dpdk.org/ml/archives/dev/2016-November/049745.html
http://dpdk.org/ml/archives/dev/2016-November/050932.html

And a proposal to fix:
http://dpdk.org/ml/archives/dev/2016-December/051844.html

Later another proposal triggered the discussion:
http://dpdk.org/ml/archives/dev/2017-May/066317.html

Finally a fix patch pushed into v17.08:
Commit: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")

Later a regression report sent related to the pushed patch:
http://dpdk.org/ml/archives/dev/2017-September/075236.html

And a fix for regression integrated into v17.11-rc1:
http://dpdk.org/ml/archives/dev/2017-October/079166.html
Commit: 5f6ff30dc507 ("igb_uio: fix interrupt enablement after FLR in VM")
Commit: 6b9ed026a870 ("igb_uio: fix build with kernel <= 3.17")

Even after the fix qede PMD reported to be broken:
http://dpdk.org/ml/archives/dev/2017-October/079359.html

So this patch reverts original fix and related commits. The related
igb_uio code part turns back to v17.05 base.

Cc: Jianfeng Tan <jianfeng.tan@intel.com>
Cc: Jingjing Wu <jingjing.wu@intel.com>
Cc: Shijith Thotton <shijith.thotton@caviumnetworks.com>
Cc: Gregory Etelson <gregory@weka.io>
Cc: Harish Patil <harish.patil@cavium.com>
Cc: George Prekas <george.prekas@epfl.ch>

Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
It would be nice to solve this issue in LTS release, but being close to
the release and the error report without details makes it hard to work
more on this issue.

Thanks everyone who spent effort for this, hopefully we can continue to
work on next release cycle.

Jingjing, there is a i40e commit, was part of igb_uio fix patchset, is
it generic, or needs to be reverted with this patch?
Commit: 8cacf78469a7 ("net/i40e: fix VF initialization error")
---
 lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 201 +++++++++++-------------------
 1 file changed, 76 insertions(+), 125 deletions(-)

diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
index f7ef82554..786df68a2 100644
--- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
+++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
@@ -49,6 +49,7 @@ 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,
@@ -207,22 +208,80 @@ 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, void *dev_id)
+igbuio_pci_irqhandler(int irq, struct uio_info *info)
 {
-	struct rte_uio_pci_dev *udev = (struct rte_uio_pci_dev *)dev_id;
-	struct uio_info *info = &udev->info;
+	struct rte_uio_pci_dev *udev = info->priv;
 
 	/* Legacy mode need to mask in hardware */
 	if (udev->mode == RTE_INTR_MODE_LEGACY &&
 	    !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;
 }
 
+/* Remap pci resources described by bar #pci_bar in uio resource n. */
+static int
+igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
+		       int n, int pci_bar, const char *name)
+{
+	unsigned long addr, len;
+	void *internal_addr;
+
+	if (n >= ARRAY_SIZE(info->mem))
+		return -EINVAL;
+
+	addr = pci_resource_start(dev, pci_bar);
+	len = pci_resource_len(dev, pci_bar);
+	if (addr == 0 || len == 0)
+		return -1;
+	internal_addr = ioremap(addr, len);
+	if (internal_addr == NULL)
+		return -1;
+	info->mem[n].name = name;
+	info->mem[n].addr = addr;
+	info->mem[n].internal_addr = internal_addr;
+	info->mem[n].size = len;
+	info->mem[n].memtype = UIO_MEM_PHYS;
+	return 0;
+}
+
+/* Get pci port io resources described by bar #pci_bar in uio resource n. */
+static int
+igbuio_pci_setup_ioport(struct pci_dev *dev, struct uio_info *info,
+		int n, int pci_bar, const char *name)
+{
+	unsigned long addr, len;
+
+	if (n >= ARRAY_SIZE(info->port))
+		return -EINVAL;
+
+	addr = pci_resource_start(dev, pci_bar);
+	len = pci_resource_len(dev, pci_bar);
+	if (addr == 0 || len == 0)
+		return -EINVAL;
+
+	info->port[n].name = name;
+	info->port[n].start = addr;
+	info->port[n].size = len;
+	info->port[n].porttype = UIO_PORT_X86;
+
+	return 0;
+}
+
+/* Unmap previously ioremap'd resources */
+static void
+igbuio_pci_release_iomem(struct uio_info *info)
+{
+	int i;
+
+	for (i = 0; i < MAX_UIO_MAPS; i++) {
+		if (info->mem[i].internal_addr)
+			iounmap(info->mem[i].internal_addr);
+	}
+}
+
 static int
 igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
 {
@@ -252,7 +311,6 @@ igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
 			break;
 		}
 #endif
-
 	/* fall back to MSI */
 	case RTE_INTR_MODE_MSI:
 #ifndef HAVE_ALLOC_IRQ_VECTORS
@@ -291,28 +349,15 @@ igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
 	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);
-	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);
-		udev->info.irq = 0;
-	}
-
 #ifndef HAVE_ALLOC_IRQ_VECTORS
 	if (udev->mode == RTE_INTR_MODE_MSIX)
 		pci_disable_msix(udev->pdev);
@@ -325,109 +370,6 @@ igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev)
 #endif
 }
 
-
-/**
- * This gets called while opening uio device file.
- */
-static int
-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;
-}
-
-static int
-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);
-
-	pci_reset_function(dev);
-
-	return 0;
-}
-
-/* Remap pci resources described by bar #pci_bar in uio resource n. */
-static int
-igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
-		       int n, int pci_bar, const char *name)
-{
-	unsigned long addr, len;
-	void *internal_addr;
-
-	if (n >= ARRAY_SIZE(info->mem))
-		return -EINVAL;
-
-	addr = pci_resource_start(dev, pci_bar);
-	len = pci_resource_len(dev, pci_bar);
-	if (addr == 0 || len == 0)
-		return -1;
-	internal_addr = ioremap(addr, len);
-	if (internal_addr == NULL)
-		return -1;
-	info->mem[n].name = name;
-	info->mem[n].addr = addr;
-	info->mem[n].internal_addr = internal_addr;
-	info->mem[n].size = len;
-	info->mem[n].memtype = UIO_MEM_PHYS;
-	return 0;
-}
-
-/* Get pci port io resources described by bar #pci_bar in uio resource n. */
-static int
-igbuio_pci_setup_ioport(struct pci_dev *dev, struct uio_info *info,
-		int n, int pci_bar, const char *name)
-{
-	unsigned long addr, len;
-
-	if (n >= ARRAY_SIZE(info->port))
-		return -EINVAL;
-
-	addr = pci_resource_start(dev, pci_bar);
-	len = pci_resource_len(dev, pci_bar);
-	if (addr == 0 || len == 0)
-		return -EINVAL;
-
-	info->port[n].name = name;
-	info->port[n].start = addr;
-	info->port[n].size = len;
-	info->port[n].porttype = UIO_PORT_X86;
-
-	return 0;
-}
-
-/* Unmap previously ioremap'd resources */
-static void
-igbuio_pci_release_iomem(struct uio_info *info)
-{
-	int i;
-
-	for (i = 0; i < MAX_UIO_MAPS; i++) {
-		if (info->mem[i].internal_addr)
-			iounmap(info->mem[i].internal_addr);
-	}
-}
-
 static int
 igbuio_setup_bars(struct pci_dev *dev, struct uio_info *info)
 {
@@ -518,16 +460,19 @@ 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 = sysfs_create_group(&dev->dev.kobj, &dev_attr_grp);
+	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;
+
 	/* register uio driver */
 	err = uio_register_device(&dev->dev, &udev->info);
 	if (err != 0)
@@ -535,6 +480,9 @@ 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.
@@ -560,6 +508,8 @@ 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);
@@ -576,6 +526,7 @@ 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.13.6

^ permalink raw reply	[flat|nested] 38+ messages in thread
* [dpdk-dev] vf init issue with patch igb_uio: issue FLR during open and release of device file
@ 2017-09-13  7:51 Yang, Qiming
  2017-10-13 14:51 ` [dpdk-dev] [PATCH] igb_uio: revert open and release operations Thomas Monjalon
  0 siblings, 1 reply; 38+ messages in thread
From: Yang, Qiming @ 2017-09-13  7:51 UTC (permalink / raw)
  To: shijith.thotton; +Cc: dev

Hi, Shijith

VF init error will happen after apply your patch, error log as below. If revert your commit, all things work well. And this issue is not only occur in i40 VF but also ixgbe.
Could you help to check it soon?

[root@localhost app]# ./testpmd -c 7 -n 4 -- -i
EAL: Detected 10 lcore(s)
EAL: No free hugepages reported in hugepages-1048576kB
EAL: Probing VFIO support...
EAL: WARNING: cpu flags constant_tsc=yes nonstop_tsc=no -> using unreliable clock cycles !
EAL: PCI device 0000:00:03.0 on NUMA socket -1
EAL:   Invalid NUMA socket, default to 0
EAL:   probe driver: 8086:154c net_i40e_vf
i40evf_init_vf(): init_adminq failed: -53
i40evf_dev_init(): Init vf failed
EAL: Requested device 0000:00:03.0 cannot be used

commit b58eedfc7dd57eef6d12e2c654a52c834f36084a
Author: Shijith Thotton <shijith.thotton@caviumnetworks.com>
Date: Fri Jul 7 16:43:51 2017 +0530
igb_uio: issue FLR during open and release of device file
Set UIO info device file operations open and release. Call pci reset
function inside open and release to clear device state at start and end.
Copied this behaviour from vfio_pci kernel module code. With this patch,
it is not mandatory to issue FLR by PMD's during init and close.
Bus master enable and disable are added in open and release respectively
to take care of device DMA.
Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
Acked-by: Gregory Etelson <gregory@weka.io>

Best Regard,
Yang Qiming

^ permalink raw reply	[flat|nested] 38+ messages in thread
end of thread, other threads:[~2017-11-03 22:39 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-17 20:14 [dpdk-dev] [PATCH] igb_uio: revert open and release operations Ferruh Yigit
2017-10-17 20:33 ` Thomas Monjalon
2017-10-18  4:50   ` Patil, Harish
2017-10-19 22:43     ` Patil, Harish
2017-10-20  1:15       ` Ferruh Yigit
2017-10-20 15:26         ` Tan, Jianfeng
2017-10-20 16:32           ` Ferruh Yigit
2017-10-20 16:55             ` [dpdk-dev] [PATCH] igb_uio: remove device reset in open Ferruh Yigit
2017-10-20 16:57               ` Ferruh Yigit
2017-10-20 19:01                 ` Gregory Etelson
2017-10-20 22:18                 ` Patil, Harish
2017-10-23 12:28                 ` Shijith Thotton
2017-10-23 16:36                   ` Ferruh Yigit
2017-10-23 19:03                     ` Shijith Thotton
2017-10-25 23:43                 ` Mody, Rasesh
2017-10-26  9:28                   ` Tan, Jianfeng
2017-10-27  0:49                     ` Ferruh Yigit
2017-11-01  6:58                       ` Mody, Rasesh
2017-11-01 14:12                         ` Stephen Hemminger
2017-11-02  8:03                           ` Mody, Rasesh
2017-11-02  8:55                             ` Ferruh Yigit
2017-11-02 17:34                               ` Mody, Rasesh
2017-11-02 18:09                                 ` Ferruh Yigit
2017-11-02 18:45                                   ` Mody, Rasesh
2017-11-03  0:31                                     ` Ferruh Yigit
2017-11-03 19:18                                       ` Mody, Rasesh
2017-11-03 19:24                                         ` Thomas Monjalon
2017-11-03 22:20                                           ` Ferruh Yigit
2017-11-03 22:39                                             ` Ferruh Yigit
2017-10-24 21:38               ` Ferruh Yigit
2017-10-18  0:14 ` [dpdk-dev] [PATCH] igb_uio: revert open and release operations Wu, Jingjing
2017-10-18  6:27 ` Shijith Thotton
2017-10-18 20:47   ` Ferruh Yigit
2017-10-24 21:32 ` Ferruh Yigit
  -- strict thread matches above, loose matches on Subject: below --
2017-09-13  7:51 [dpdk-dev] vf init issue with patch igb_uio: issue FLR during open and release of device file Yang, Qiming
2017-10-13 14:51 ` [dpdk-dev] [PATCH] igb_uio: revert open and release operations Thomas Monjalon
2017-10-13 21:05   ` Ferruh Yigit
2017-10-13 21:11     ` Thomas Monjalon
2017-10-13 21:17       ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).