[dpdk-dev] [PATCH] igb_uio: remove device reset in release

[dpdk-dev] [PATCH] igb_uio: remove device reset in release

[Ferruh Yigit]

More error reported for device reset in release() [1],
when device pass-through to the guest, host kernel crash on guest exit.

Removing the reset completely.

[1]
http://dpdk.org/ml/archives/dev/2017-November/081459.html

Fixes: e3a64deae2d5 ("igb_uio: prevent reset for bnx2x devices")
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>
---
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>
Cc: Rasesh Mody <rasesh.mody@cavium.com>
Cc: Lee Roberts <lee.roberts@hpe.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Chas Williams <chas3@att.com>
---
 lib/librte_eal/linuxapp/igb_uio/compat.h  | 19 -------------------
 lib/librte_eal/linuxapp/igb_uio/igb_uio.c |  8 --------
 2 files changed, 27 deletions(-)

diff --git a/lib/librte_eal/linuxapp/igb_uio/compat.h b/lib/librte_eal/linuxapp/igb_uio/compat.h
index c8b2c9de3..ce456d4bb 100644
--- a/lib/librte_eal/linuxapp/igb_uio/compat.h
+++ b/lib/librte_eal/linuxapp/igb_uio/compat.h
@@ -132,22 +132,3 @@ static bool pci_check_and_mask_intx(struct pci_dev *pdev)
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 5, 0)
 #define HAVE_PCI_MSI_MASK_IRQ 1
 #endif
-
-#define BROADCOM_PCI_VENDOR_ID 0x14E4
-static const struct pci_device_id no_reset_pci_tbl[] = {
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x164f) }, /* 57711 */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x168a) }, /* 57800 */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x168e) }, /* 57810 */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x163d) }, /* 57811 */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x168d) }, /* 57840_OBS */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16a1) }, /* 57840_4_10 */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16a2) }, /* 57840_2_20 */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16ae) }, /* 57810_MF */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x163e) }, /* 57811_MF */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16a4) }, /* 57840_MF */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16a9) }, /* 57800_VF */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16af) }, /* 57810_VF */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x163f) }, /* 57811_VF */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16ad) }, /* 57840_VF */
-	{ 0 },
-};
diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
index 037e02267..a3a98c173 100644
--- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
+++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
@@ -348,11 +348,6 @@ igbuio_pci_open(struct uio_info *info, struct inode *inode)
 	return 0;
 }
 
-static bool is_device_excluded_from_reset(struct pci_dev *pdev)
-{
-	return !!pci_match_id(no_reset_pci_tbl, pdev);
-}
-
 static int
 igbuio_pci_release(struct uio_info *info, struct inode *inode)
 {
@@ -365,9 +360,6 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode)
 	/* stop the device from further DMA */
 	pci_clear_master(dev);
 
-	if (!is_device_excluded_from_reset(dev))
-		pci_reset_function(dev);
-
 	return 0;
 }
 
-- 
2.13.6

[dpdk-dev] [PATCH v2] igb_uio: remove device reset in release

[Ferruh Yigit]

More error reported for device reset in release() [1],
when device pass-through to the guest, host kernel crash on guest exit.

Removing the reset completely.

This is close to reverting commit b58eedfc7dd5 [2], taking into account
previous fix to remove reset in open as well [3], but not exactly same.

With latest code, interrupts are enabled in uio open() callback and
disabled in uio release() callback, so when a DPDK application exit
device interrupts are disabled. Previously interrupts were only enabled
once in igb_uio module insert and disabled in module removal.

Also with latest code device set as bus master in open() and master
cleared in release(), clearing bus master should prevent further DMA
which was one of the target of the initial patch.

The initial intention was also to reset the device to be sure it has
been left in proper state, but currently that part is missing because of
reported problem(s).

Still igb_uio should be safer comparing to the pre b58eedfc7dd5 state.

[1]
http://dpdk.org/ml/archives/dev/2017-November/081459.html

[2]
b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")

[3]
f73b38e9245d ("igb_uio: remove device reset in open")

Fixes: e3a64deae2d5 ("igb_uio: prevent reset for bnx2x devices")
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>
---
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>
Cc: Rasesh Mody <rasesh.mody@cavium.com>
Cc: Lee Roberts <lee.roberts@hpe.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Chas Williams <chas3@att.com>

v2:
More detailed commit log
---
 lib/librte_eal/linuxapp/igb_uio/compat.h  | 19 -------------------
 lib/librte_eal/linuxapp/igb_uio/igb_uio.c |  8 --------
 2 files changed, 27 deletions(-)

diff --git a/lib/librte_eal/linuxapp/igb_uio/compat.h b/lib/librte_eal/linuxapp/igb_uio/compat.h
index c8b2c9de3..ce456d4bb 100644
--- a/lib/librte_eal/linuxapp/igb_uio/compat.h
+++ b/lib/librte_eal/linuxapp/igb_uio/compat.h
@@ -132,22 +132,3 @@ static bool pci_check_and_mask_intx(struct pci_dev *pdev)
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 5, 0)
 #define HAVE_PCI_MSI_MASK_IRQ 1
 #endif
-
-#define BROADCOM_PCI_VENDOR_ID 0x14E4
-static const struct pci_device_id no_reset_pci_tbl[] = {
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x164f) }, /* 57711 */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x168a) }, /* 57800 */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x168e) }, /* 57810 */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x163d) }, /* 57811 */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x168d) }, /* 57840_OBS */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16a1) }, /* 57840_4_10 */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16a2) }, /* 57840_2_20 */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16ae) }, /* 57810_MF */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x163e) }, /* 57811_MF */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16a4) }, /* 57840_MF */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16a9) }, /* 57800_VF */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16af) }, /* 57810_VF */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x163f) }, /* 57811_VF */
-	{ PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x16ad) }, /* 57840_VF */
-	{ 0 },
-};
diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
index 037e02267..a3a98c173 100644
--- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
+++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
@@ -348,11 +348,6 @@ igbuio_pci_open(struct uio_info *info, struct inode *inode)
 	return 0;
 }
 
-static bool is_device_excluded_from_reset(struct pci_dev *pdev)
-{
-	return !!pci_match_id(no_reset_pci_tbl, pdev);
-}
-
 static int
 igbuio_pci_release(struct uio_info *info, struct inode *inode)
 {
@@ -365,9 +360,6 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode)
 	/* stop the device from further DMA */
 	pci_clear_master(dev);
 
-	if (!is_device_excluded_from_reset(dev))
-		pci_reset_function(dev);
-
 	return 0;
 }
 
-- 
2.13.6

Re: [dpdk-dev] [PATCH v2] igb_uio: remove device reset in release

[Thomas Monjalon]

07/11/2017 23:29, Ferruh Yigit:
> More error reported for device reset in release() [1],
> when device pass-through to the guest, host kernel crash on guest exit.
> 
> Removing the reset completely.
> 
> This is close to reverting commit b58eedfc7dd5 [2], taking into account
> previous fix to remove reset in open as well [3], but not exactly same.
> 
> With latest code, interrupts are enabled in uio open() callback and
> disabled in uio release() callback, so when a DPDK application exit
> device interrupts are disabled. Previously interrupts were only enabled
> once in igb_uio module insert and disabled in module removal.
> 
> Also with latest code device set as bus master in open() and master
> cleared in release(), clearing bus master should prevent further DMA
> which was one of the target of the initial patch.
> 
> The initial intention was also to reset the device to be sure it has
> been left in proper state, but currently that part is missing because of
> reported problem(s).
> 
> Still igb_uio should be safer comparing to the pre b58eedfc7dd5 state.
> 
> [1]
> http://dpdk.org/ml/archives/dev/2017-November/081459.html
> 
> [2]
> b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")
> 
> [3]
> f73b38e9245d ("igb_uio: remove device reset in open")
> 
> Fixes: e3a64deae2d5 ("igb_uio: prevent reset for bnx2x devices")
> 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>

We can say you tried hard to make igb_uio cleaner and safer :)

Applied, thanks for the detailed explanations.

Re: [dpdk-dev] [PATCH v2] igb_uio: remove device reset in release

[Wu, Jingjing]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, November 8, 2017 7:57 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Tan, Jianfeng <jianfeng.tan@intel.com>;
> Wu, Jingjing <jingjing.wu@intel.com>; Shijith Thotton
> <shijith.thotton@caviumnetworks.com>; Gregory Etelson <gregory@weka.io>;
> Harish Patil <harish.patil@cavium.com>; George Prekas
> <george.prekas@epfl.ch>; Gonzalez Monroy, Sergio
> <sergio.gonzalez.monroy@intel.com>; Rasesh Mody
> <rasesh.mody@cavium.com>; Lee Roberts <lee.roberts@hpe.com>; Stephen
> Hemminger <stephen@networkplumber.org>; Chas Williams <chas3@att.com>
> Subject: Re: [dpdk-dev] [PATCH v2] igb_uio: remove device reset in release
> 
> 07/11/2017 23:29, Ferruh Yigit:
> > More error reported for device reset in release() [1], when device
> > pass-through to the guest, host kernel crash on guest exit.
> >
> > Removing the reset completely.
> >
> > This is close to reverting commit b58eedfc7dd5 [2], taking into
> > account previous fix to remove reset in open as well [3], but not exactly same.
> >
> > With latest code, interrupts are enabled in uio open() callback and
> > disabled in uio release() callback, so when a DPDK application exit
> > device interrupts are disabled. Previously interrupts were only
> > enabled once in igb_uio module insert and disabled in module removal.
> >
> > Also with latest code device set as bus master in open() and master
> > cleared in release(), clearing bus master should prevent further DMA
> > which was one of the target of the initial patch.
> >
> > The initial intention was also to reset the device to be sure it has
> > been left in proper state, but currently that part is missing because
> > of reported problem(s).
> >
> > Still igb_uio should be safer comparing to the pre b58eedfc7dd5 state.
> >
> > [1]
> > http://dpdk.org/ml/archives/dev/2017-November/081459.html
> >
> > [2]
> > b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device
> > file")
> >
> > [3]
> > f73b38e9245d ("igb_uio: remove device reset in open")
> >
> > Fixes: e3a64deae2d5 ("igb_uio: prevent reset for bnx2x devices")
> > 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>
> 
> We can say you tried hard to make igb_uio cleaner and safer :)
> 
That's true! Thanks a lot, Ferruh!!

> Applied, thanks for the detailed explanations.