From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 502031B3D6; Wed, 18 Oct 2017 02:14:50 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga104.jf.intel.com with ESMTP; 17 Oct 2017 17:14:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,393,1503385200"; d="scan'208";a="1206946183" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga001.fm.intel.com with ESMTP; 17 Oct 2017 17:14:49 -0700 Received: from fmsmsx114.amr.corp.intel.com (10.18.116.8) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 17 Oct 2017 17:14:49 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX114.amr.corp.intel.com (10.18.116.8) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 17 Oct 2017 17:14:49 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.213]) by shsmsx102.ccr.corp.intel.com ([169.254.2.175]) with mapi id 14.03.0319.002; Wed, 18 Oct 2017 08:14:47 +0800 From: "Wu, Jingjing" To: "Yigit, Ferruh" , Thomas Monjalon CC: "dev@dpdk.org" , "Tan, Jianfeng" , Shijith Thotton , Gregory Etelson , Harish Patil , George Prekas , "stable@dpdk.org" Thread-Topic: [PATCH] igb_uio: revert open and release operations Thread-Index: AQHTR4SSdXJX5kBrwEyIgFs2e2P38KLovNsw Date: Wed, 18 Oct 2017 00:14:47 +0000 Message-ID: <9BB6961774997848B5B42BEC655768F810E98D6F@SHSMSX103.ccr.corp.intel.com> References: <20171017201436.65270-1-ferruh.yigit@intel.com> In-Reply-To: <20171017201436.65270-1-ferruh.yigit@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] igb_uio: revert open and release operations X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Oct 2017 00:14:52 -0000 Hi, Ferruh This one is generic, we can keep it. Commit: 8cacf78469a7 ("net/i40e: fix VF initialization error") Thanks Jingjing > -----Original Message----- > From: Yigit, Ferruh > Sent: Wednesday, October 18, 2017 4:15 AM > To: Thomas Monjalon ; Yigit, Ferruh > > Cc: dev@dpdk.org; Tan, Jianfeng ; Wu, Jingjing > ; Shijith Thotton > ; Gregory Etelson ; > Harish Patil ; George Prekas > ; stable@dpdk.org > Subject: [PATCH] igb_uio: revert open and release operations >=20 > This reverts commit 6b9ed026a8704b9e5ee5da7997617ef7cc82e114. > This reverts commit 5f6ff30dc5075c49069d684bab229aef7ff0fdc3. > This reverts commit b58eedfc7dd57eef6d12e2c654a52c834f36084a. >=20 > 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 >=20 > And a proposal to fix: > http://dpdk.org/ml/archives/dev/2016-December/051844.html >=20 > Later another proposal triggered the discussion: > http://dpdk.org/ml/archives/dev/2017-May/066317.html >=20 > Finally a fix patch pushed into v17.08: > Commit: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of devi= ce > file") >=20 > Later a regression report sent related to the pushed patch: > http://dpdk.org/ml/archives/dev/2017-September/075236.html >=20 > 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 <=3D 3.17") >=20 > Even after the fix qede PMD reported to be broken: > http://dpdk.org/ml/archives/dev/2017-October/079359.html >=20 > So this patch reverts original fix and related commits. The related igb_u= io code > part turns back to v17.05 base. >=20 > Cc: Jianfeng Tan > Cc: Jingjing Wu > Cc: Shijith Thotton > Cc: Gregory Etelson > Cc: Harish Patil > Cc: George Prekas >=20 > Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of devic= e file") > Cc: stable@dpdk.org >=20 > Signed-off-by: Ferruh Yigit > --- > It would be nice to solve this issue in LTS release, but being close to t= he release > and the error report without details makes it hard to work more on this i= ssue. >=20 > Thanks everyone who spent effort for this, hopefully we can continue to w= ork > on next release cycle. >=20 > 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(-) >=20 > 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 { >=20 > static char *intr_mode; > static enum rte_intr_mode igbuio_intr_mode_preferred =3D > 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 =3D (struct rte_uio_pci_dev *)dev_id; > - struct uio_info *info =3D &udev->info; > + struct rte_uio_pci_dev *udev =3D info->priv; >=20 > /* Legacy mode need to mask in hardware */ > if (udev->mode =3D=3D RTE_INTR_MODE_LEGACY && > !pci_check_and_mask_intx(udev->pdev)) > return IRQ_NONE; >=20 > - uio_event_notify(info); > - > /* Message signal mode, no share IRQ and automasked */ > return IRQ_HANDLED; > } >=20 > +/* 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 >=3D ARRAY_SIZE(info->mem)) > + return -EINVAL; > + > + addr =3D pci_resource_start(dev, pci_bar); > + len =3D pci_resource_len(dev, pci_bar); > + if (addr =3D=3D 0 || len =3D=3D 0) > + return -1; > + internal_addr =3D ioremap(addr, len); > + if (internal_addr =3D=3D NULL) > + return -1; > + info->mem[n].name =3D name; > + info->mem[n].addr =3D addr; > + info->mem[n].internal_addr =3D internal_addr; > + info->mem[n].size =3D len; > + info->mem[n].memtype =3D 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 >=3D ARRAY_SIZE(info->port)) > + return -EINVAL; > + > + addr =3D pci_resource_start(dev, pci_bar); > + len =3D pci_resource_len(dev, pci_bar); > + if (addr =3D=3D 0 || len =3D=3D 0) > + return -EINVAL; > + > + info->port[n].name =3D name; > + info->port[n].start =3D addr; > + info->port[n].size =3D len; > + info->port[n].porttype =3D UIO_PORT_X86; > + > + return 0; > +} > + > +/* Unmap previously ioremap'd resources */ static void > +igbuio_pci_release_iomem(struct uio_info *info) { > + int i; > + > + for (i =3D 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 =3D UIO_IRQ_NONE; > err =3D -EINVAL; > } >=20 > - if (udev->info.irq !=3D UIO_IRQ_NONE) > - err =3D 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; > } >=20 > 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 =3D 0; > - } > - > #ifndef HAVE_ALLOC_IRQ_VECTORS > if (udev->mode =3D=3D RTE_INTR_MODE_MSIX) > pci_disable_msix(udev->pdev); > @@ -325,109 +370,6 @@ igbuio_pci_disable_interrupts(struct rte_uio_pci_de= v > *udev) #endif } >=20 > - > -/** > - * 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 =3D info->priv; > - struct pci_dev *dev =3D 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 =3D 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 =3D info->priv; > - struct pci_dev *dev =3D 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 >=3D ARRAY_SIZE(info->mem)) > - return -EINVAL; > - > - addr =3D pci_resource_start(dev, pci_bar); > - len =3D pci_resource_len(dev, pci_bar); > - if (addr =3D=3D 0 || len =3D=3D 0) > - return -1; > - internal_addr =3D ioremap(addr, len); > - if (internal_addr =3D=3D NULL) > - return -1; > - info->mem[n].name =3D name; > - info->mem[n].addr =3D addr; > - info->mem[n].internal_addr =3D internal_addr; > - info->mem[n].size =3D len; > - info->mem[n].memtype =3D 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 >=3D ARRAY_SIZE(info->port)) > - return -EINVAL; > - > - addr =3D pci_resource_start(dev, pci_bar); > - len =3D pci_resource_len(dev, pci_bar); > - if (addr =3D=3D 0 || len =3D=3D 0) > - return -EINVAL; > - > - info->port[n].name =3D name; > - info->port[n].start =3D addr; > - info->port[n].size =3D len; > - info->port[n].porttype =3D UIO_PORT_X86; > - > - return 0; > -} > - > -/* Unmap previously ioremap'd resources */ -static void - > igbuio_pci_release_iomem(struct uio_info *info) -{ > - int i; > - > - for (i =3D 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 =3D "igb_uio"; > udev->info.version =3D "0.1"; > + udev->info.handler =3D igbuio_pci_irqhandler; > udev->info.irqcontrol =3D igbuio_pci_irqcontrol; > - udev->info.open =3D igbuio_pci_open; > - udev->info.release =3D igbuio_pci_release; > udev->info.priv =3D udev; > udev->pdev =3D dev; >=20 > - err =3D sysfs_create_group(&dev->dev.kobj, &dev_attr_grp); > + err =3D igbuio_pci_enable_interrupts(udev); > if (err !=3D 0) > goto fail_release_iomem; >=20 > + err =3D sysfs_create_group(&dev->dev.kobj, &dev_attr_grp); > + if (err !=3D 0) > + goto fail_disable_interrupts; > + > /* register uio driver */ > err =3D uio_register_device(&dev->dev, &udev->info); > if (err !=3D 0) > @@ -535,6 +480,9 @@ igbuio_pci_probe(struct pci_dev *dev, const struct > pci_device_id *id) >=20 > pci_set_drvdata(dev, udev); >=20 > + 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=3Dpt. > @@ -560,6 +508,8 @@ igbuio_pci_probe(struct pci_dev *dev, const struct > pci_device_id *id) >=20 > 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) >=20 > 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