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; 35+ 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] 35+ messages in thread
* Re: [dpdk-dev] [PATCH] igb_uio: remove device reset in open
@ 2017-10-22 16:06 Patil, Harish
  0 siblings, 0 replies; 35+ messages in thread
From: Patil, Harish @ 2017-10-22 16:06 UTC (permalink / raw)
  To: Ferruh Yigit, Jingjing Wu, Thotton, Shijith, Gregory Etelson
  Cc: Thomas Monjalon, dev, stable, Jianfeng Tan, George Prekas,
	Sergio Gonzalez Monroy

-----Original Message-----
From: dev <dev-bounces@dpdk.org> on behalf of Harish Patil
<Harish.Patil@cavium.com>
Date: Friday, October 20, 2017 at 3:18 PM
To: Ferruh Yigit <ferruh.yigit@intel.com>, Jingjing Wu
<jingjing.wu@intel.com>, "Thotton, Shijith" <Shijith.Thotton@cavium.com>,
Gregory Etelson <gregory@weka.io>
Cc: Thomas Monjalon <thomas@monjalon.net>, "dev@dpdk.org" <dev@dpdk.org>,
"stable@dpdk.org" <stable@dpdk.org>, Jianfeng Tan
<jianfeng.tan@intel.com>, George Prekas <george.prekas@epfl.ch>, Sergio
Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
Subject: Re: [dpdk-dev] [PATCH] igb_uio: remove device reset in open

>[This sender failed our fraud detection checks and may not be who they
>appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing]
>
>-----Original Message-----
>From: Ferruh Yigit <ferruh.yigit@intel.com>
>Date: Friday, October 20, 2017 at 9:57 AM
>To: Jingjing Wu <jingjing.wu@intel.com>, "Thotton, Shijith"
><Shijith.Thotton@cavium.com>, Gregory Etelson <gregory@weka.io>, Harish
>Patil <Harish.Patil@cavium.com>
>Cc: Thomas Monjalon <thomas@monjalon.net>, "dev@dpdk.org" <dev@dpdk.org>,
>"stable@dpdk.org" <stable@dpdk.org>, Jianfeng Tan
><jianfeng.tan@intel.com>, George Prekas <george.prekas@epfl.ch>, Sergio
>Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
>Subject: Re: [PATCH] igb_uio: remove device reset in open
>
>>On 10/20/2017 9:55 AM, Ferruh Yigit wrote:
>>> Remove device reset during application start, the reset for application
>>> exit still there.
>>>
>>> Reset in open removed because of following comments:
>>> 1- Device reset not completed when VF driver loaded, which cause VF PMD
>>>    initialization error.
>>>    Adding delay can solve the issue but will increase driver load time.
>>>
>>> 2- Reset will be issues all devices unconditionally, not very efficient
>>>    way.
>>>
>>> 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>
>>
>>Hi Jingjing, Shijith, Gregory, Harish,
>>
>>Can you please test this on top of current master (which has already
>>Jingjin's
>>fix) ?
>>
>>Thanks,
>>Ferruh
>
>
>Ferruh,
>Sure, will try and get back to you.
>Thanks.

Ferruh and all,
This takes care of our SR-IOV VF issue.
Thanks.

>>
>>> ---
>>> 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>
>>> Cc: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
>>> ---
>>>  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 2 --
>>>  1 file changed, 2 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..fd320d87d 100644
>>> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
>>> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
>>> @@ -336,8 +336,6 @@ igbuio_pci_open(struct uio_info *info, struct inode
>>>*inode)
>>>      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);
>>>
>>>
>>
>


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

Thread overview: 35+ 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
2017-10-22 16:06 [dpdk-dev] [PATCH] igb_uio: remove device reset in open Patil, Harish

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).