* [dpdk-dev] [PATCH] igb_uio: add config option to control reset
@ 2017-11-03 0:30 Ferruh Yigit
2017-11-03 0:51 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
0 siblings, 1 reply; 7+ messages in thread
From: Ferruh Yigit @ 2017-11-03 0:30 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, Ferruh Yigit, stable, Jianfeng Tan, Jingjing Wu,
Shijith Thotton, Gregory Etelson, Harish Patil, George Prekas,
Sergio Gonzalez Monroy, Rasesh Mody
Adding a compile time configuration option to control device reset done
during DPDK application exit.
Config option is CONFIG_RTE_EAL_IGB_UIO_RESET and enabled by default,
so by defualt reset will happen. Having this reset is safer to be sure
device left in a proper case.
But for special cases [1] it is possible to disable the config option
to prevent the device reset.
[1]
http://dpdk.org/ml/archives/dev/2017-November/080927.html
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>
---
config/common_base | 1 +
config/common_linuxapp | 1 +
lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 2 ++
3 files changed, 4 insertions(+)
diff --git a/config/common_base b/config/common_base
index 82ee75456..2a9947420 100644
--- a/config/common_base
+++ b/config/common_base
@@ -102,6 +102,7 @@ CONFIG_RTE_LIBEAL_USE_HPET=n
CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
CONFIG_RTE_EAL_IGB_UIO=n
+CONFIG_RTE_EAL_IGB_UIO_RESET=n
CONFIG_RTE_EAL_VFIO=n
CONFIG_RTE_MALLOC_DEBUG=n
CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 74c7d64ec..b3a602909 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -37,6 +37,7 @@ CONFIG_RTE_EXEC_ENV_LINUXAPP=y
CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=y
CONFIG_RTE_EAL_IGB_UIO=y
+CONFIG_RTE_EAL_IGB_UIO_RESET=y
CONFIG_RTE_EAL_VFIO=y
CONFIG_RTE_KNI_KMOD=y
CONFIG_RTE_LIBRTE_KNI=y
diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
index fd320d87d..0325722c0 100644
--- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
+++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
@@ -360,7 +360,9 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode)
/* stop the device from further DMA */
pci_clear_master(dev);
+#ifdef RTE_EAL_IGB_UIO_RESET
pci_reset_function(dev);
+#endif
return 0;
}
--
2.13.6
^ permalink raw reply [flat|nested] 7+ messages in thread* [dpdk-dev] [PATCH v2] igb_uio: add config option to control reset 2017-11-03 0:30 [dpdk-dev] [PATCH] igb_uio: add config option to control reset Ferruh Yigit @ 2017-11-03 0:51 ` Ferruh Yigit 2017-11-03 2:57 ` Tan, Jianfeng 0 siblings, 1 reply; 7+ messages in thread From: Ferruh Yigit @ 2017-11-03 0:51 UTC (permalink / raw) To: Thomas Monjalon Cc: dev, Ferruh Yigit, stable, Jianfeng Tan, Jingjing Wu, Shijith Thotton, Gregory Etelson, Harish Patil, George Prekas, Sergio Gonzalez Monroy, Rasesh Mody Adding a compile time configuration option to control device reset done during DPDK application exit. Config option is CONFIG_RTE_EAL_IGB_UIO_RESET and enabled by default, so by default reset will happen. Having this reset is safer to be sure device left in a proper case. But for special cases [1] it is possible to disable the config option to prevent the device reset. [1] http://dpdk.org/ml/archives/dev/2017-November/080927.html 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> v2: * fix typo in commit log --- config/common_base | 1 + config/common_linuxapp | 1 + lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 2 ++ 3 files changed, 4 insertions(+) diff --git a/config/common_base b/config/common_base index 82ee75456..2a9947420 100644 --- a/config/common_base +++ b/config/common_base @@ -102,6 +102,7 @@ CONFIG_RTE_LIBEAL_USE_HPET=n CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n CONFIG_RTE_EAL_IGB_UIO=n +CONFIG_RTE_EAL_IGB_UIO_RESET=n CONFIG_RTE_EAL_VFIO=n CONFIG_RTE_MALLOC_DEBUG=n CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n diff --git a/config/common_linuxapp b/config/common_linuxapp index 74c7d64ec..b3a602909 100644 --- a/config/common_linuxapp +++ b/config/common_linuxapp @@ -37,6 +37,7 @@ CONFIG_RTE_EXEC_ENV_LINUXAPP=y CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=y CONFIG_RTE_EAL_IGB_UIO=y +CONFIG_RTE_EAL_IGB_UIO_RESET=y CONFIG_RTE_EAL_VFIO=y CONFIG_RTE_KNI_KMOD=y CONFIG_RTE_LIBRTE_KNI=y diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c index fd320d87d..0325722c0 100644 --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c @@ -360,7 +360,9 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode) /* stop the device from further DMA */ pci_clear_master(dev); +#ifdef RTE_EAL_IGB_UIO_RESET pci_reset_function(dev); +#endif return 0; } -- 2.13.6 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH v2] igb_uio: add config option to control reset 2017-11-03 0:51 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit @ 2017-11-03 2:57 ` Tan, Jianfeng 2017-11-03 19:42 ` Roberts, Lee A. 0 siblings, 1 reply; 7+ messages in thread From: Tan, Jianfeng @ 2017-11-03 2:57 UTC (permalink / raw) To: Ferruh Yigit, Thomas Monjalon Cc: dev, stable, Jingjing Wu, Shijith Thotton, Gregory Etelson, Harish Patil, George Prekas, Sergio Gonzalez Monroy, Rasesh Mody On 11/3/2017 8:51 AM, Ferruh Yigit wrote: > Adding a compile time configuration option to control device reset done > during DPDK application exit. > > Config option is CONFIG_RTE_EAL_IGB_UIO_RESET and enabled by default, > so by default reset will happen. Having this reset is safer to be sure > device left in a proper case. > > But for special cases [1] it is possible to disable the config option > to prevent the device reset. > > [1] > http://dpdk.org/ml/archives/dev/2017-November/080927.html > > 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> Realize that we do have a pci_clear_master() in the release() to disable the DMA from device until the next open() will enable the DMA again . Here is my: Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com> Thanks, Jianfeng > --- > 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> > > v2: > * fix typo in commit log > --- > config/common_base | 1 + > config/common_linuxapp | 1 + > lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 2 ++ > 3 files changed, 4 insertions(+) > > diff --git a/config/common_base b/config/common_base > index 82ee75456..2a9947420 100644 > --- a/config/common_base > +++ b/config/common_base > @@ -102,6 +102,7 @@ CONFIG_RTE_LIBEAL_USE_HPET=n > CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n > CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n > CONFIG_RTE_EAL_IGB_UIO=n > +CONFIG_RTE_EAL_IGB_UIO_RESET=n > CONFIG_RTE_EAL_VFIO=n > CONFIG_RTE_MALLOC_DEBUG=n > CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n > diff --git a/config/common_linuxapp b/config/common_linuxapp > index 74c7d64ec..b3a602909 100644 > --- a/config/common_linuxapp > +++ b/config/common_linuxapp > @@ -37,6 +37,7 @@ CONFIG_RTE_EXEC_ENV_LINUXAPP=y > > CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=y > CONFIG_RTE_EAL_IGB_UIO=y > +CONFIG_RTE_EAL_IGB_UIO_RESET=y > CONFIG_RTE_EAL_VFIO=y > CONFIG_RTE_KNI_KMOD=y > CONFIG_RTE_LIBRTE_KNI=y > diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c > index fd320d87d..0325722c0 100644 > --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c > +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c > @@ -360,7 +360,9 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode) > /* stop the device from further DMA */ > pci_clear_master(dev); > > +#ifdef RTE_EAL_IGB_UIO_RESET > pci_reset_function(dev); > +#endif > > return 0; > } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH v2] igb_uio: add config option to control reset 2017-11-03 2:57 ` Tan, Jianfeng @ 2017-11-03 19:42 ` Roberts, Lee A. 2017-11-03 22:03 ` Ferruh Yigit 0 siblings, 1 reply; 7+ messages in thread From: Roberts, Lee A. @ 2017-11-03 19:42 UTC (permalink / raw) To: Tan, Jianfeng, Ferruh Yigit, Thomas Monjalon Cc: dev, stable, Jingjing Wu, Shijith Thotton, Gregory Etelson, Harish Patil, George Prekas, Sergio Gonzalez Monroy, Rasesh Mody > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tan, Jianfeng > Sent: Thursday, November 02, 2017 8:57 PM > To: Ferruh Yigit <ferruh.yigit@intel.com>; Thomas Monjalon <thomas@monjalon.net> > Cc: dev@dpdk.org; stable@dpdk.org; Jingjing Wu <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>; Sergio Gonzalez Monroy > <sergio.gonzalez.monroy@intel.com>; Rasesh Mody <rasesh.mody@cavium.com> > Subject: Re: [dpdk-dev] [PATCH v2] igb_uio: add config option to control reset > > > > On 11/3/2017 8:51 AM, Ferruh Yigit wrote: > > Adding a compile time configuration option to control device reset done > > during DPDK application exit. > > > > Config option is CONFIG_RTE_EAL_IGB_UIO_RESET and enabled by default, > > so by default reset will happen. Having this reset is safer to be sure > > device left in a proper case. > > > > But for special cases [1] it is possible to disable the config option > > to prevent the device reset. > > > > [1] > > http://dpdk.org/ml/archives/dev/2017-November/080927.html > > > > 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> > > Realize that we do have a pci_clear_master() in the release() to disable > the DMA from device until the next open() will enable the DMA again . > Here is my: > > Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com> > > Thanks, > Jianfeng > > > --- > > 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> > > > > v2: > > * fix typo in commit log > > --- > > config/common_base | 1 + > > config/common_linuxapp | 1 + > > lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 2 ++ > > 3 files changed, 4 insertions(+) > > > > diff --git a/config/common_base b/config/common_base > > index 82ee75456..2a9947420 100644 > > --- a/config/common_base > > +++ b/config/common_base > > @@ -102,6 +102,7 @@ CONFIG_RTE_LIBEAL_USE_HPET=n > > CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n > > CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n > > CONFIG_RTE_EAL_IGB_UIO=n > > +CONFIG_RTE_EAL_IGB_UIO_RESET=n > > CONFIG_RTE_EAL_VFIO=n > > CONFIG_RTE_MALLOC_DEBUG=n > > CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n > > diff --git a/config/common_linuxapp b/config/common_linuxapp > > index 74c7d64ec..b3a602909 100644 > > --- a/config/common_linuxapp > > +++ b/config/common_linuxapp > > @@ -37,6 +37,7 @@ CONFIG_RTE_EXEC_ENV_LINUXAPP=y > > > > CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=y > > CONFIG_RTE_EAL_IGB_UIO=y > > +CONFIG_RTE_EAL_IGB_UIO_RESET=y > > CONFIG_RTE_EAL_VFIO=y > > CONFIG_RTE_KNI_KMOD=y > > CONFIG_RTE_LIBRTE_KNI=y > > diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c > > index fd320d87d..0325722c0 100644 > > --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c > > +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c > > @@ -360,7 +360,9 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode) > > /* stop the device from further DMA */ > > pci_clear_master(dev); > > > > +#ifdef RTE_EAL_IGB_UIO_RESET > > pci_reset_function(dev); > > +#endif > > > > return 0; > > } A compile time configuration option makes life very difficult for application providers. Consider the case where an application such as Open vSwitch with DPDK support is being provided with a Linux distribution. One would want the Open vSwitch binary to support as many vendor NICs as possible---without the need to recompile. With a change such as this, one would need to have different versions of the kernel igb_uio module to support different NICs. The Linux kernel is already aware of, and provides work-arounds for, various PCI quirks. For example, see linux/drivers/pci/quirks.c (http://elixir.free-electrons.com/linux/latest/source/drivers/pci/quirks.c). At this point in igb_uio.c, one is aware of the struct pci_dev "dev" for the device in question. Access to the vendor and device information should be simple: struct pci_dev { struct list_head bus_list; /* node in per-bus list */ struct pci_bus *bus; /* bus this device is on */ struct pci_bus *subordinate; /* bus this device bridges to */ void *sysdata; /* hook for sys-specific extension */ struct proc_dir_entry *procent; /* device entry in /proc/bus/pci */ struct pci_slot *slot; /* Physical slot this device is in */ unsigned int devfn; /* encoded device & function index */ unsigned short vendor; unsigned short device; unsigned short subsystem_vendor; unsigned short subsystem_device; ... One could imagine using logic to implement corresponding PCI quirks that can be evaluated at runtime. For example (in pseudocode), if not (vendor = "Cavium" and device = "bnx2x") then pci_reset_function(dev); There are other possible implementations. If there are enough quirks, one might have action functions defined---and a table of function pointers associated with each PMD to select the proper action. - Lee Roberts ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH v2] igb_uio: add config option to control reset 2017-11-03 19:42 ` Roberts, Lee A. @ 2017-11-03 22:03 ` Ferruh Yigit 2017-11-04 10:08 ` Stephen Hemminger 0 siblings, 1 reply; 7+ messages in thread From: Ferruh Yigit @ 2017-11-03 22:03 UTC (permalink / raw) To: Roberts, Lee A., Tan, Jianfeng, Thomas Monjalon Cc: dev, stable, Wu, Jingjing, Shijith Thotton, Gregory Etelson, Harish Patil, George Prekas, Gonzalez Monroy, Sergio, Rasesh Mody On 11/3/2017 12:42 PM, Roberts, Lee A. wrote: > > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tan, Jianfeng >> Sent: Thursday, November 02, 2017 8:57 PM >> To: Ferruh Yigit <ferruh.yigit@intel.com>; Thomas Monjalon <thomas@monjalon.net> >> Cc: dev@dpdk.org; stable@dpdk.org; Jingjing Wu <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>; Sergio Gonzalez Monroy >> <sergio.gonzalez.monroy@intel.com>; Rasesh Mody <rasesh.mody@cavium.com> >> Subject: Re: [dpdk-dev] [PATCH v2] igb_uio: add config option to control reset >> >> >> >> On 11/3/2017 8:51 AM, Ferruh Yigit wrote: >>> Adding a compile time configuration option to control device reset done >>> during DPDK application exit. >>> >>> Config option is CONFIG_RTE_EAL_IGB_UIO_RESET and enabled by default, >>> so by default reset will happen. Having this reset is safer to be sure >>> device left in a proper case. >>> >>> But for special cases [1] it is possible to disable the config option >>> to prevent the device reset. >>> >>> [1] >>> http://dpdk.org/ml/archives/dev/2017-November/080927.html >>> >>> 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> >> >> Realize that we do have a pci_clear_master() in the release() to disable >> the DMA from device until the next open() will enable the DMA again . >> Here is my: >> >> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com> >> >> Thanks, >> Jianfeng >> >>> --- >>> 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> >>> >>> v2: >>> * fix typo in commit log >>> --- >>> config/common_base | 1 + >>> config/common_linuxapp | 1 + >>> lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 2 ++ >>> 3 files changed, 4 insertions(+) >>> >>> diff --git a/config/common_base b/config/common_base >>> index 82ee75456..2a9947420 100644 >>> --- a/config/common_base >>> +++ b/config/common_base >>> @@ -102,6 +102,7 @@ CONFIG_RTE_LIBEAL_USE_HPET=n >>> CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n >>> CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n >>> CONFIG_RTE_EAL_IGB_UIO=n >>> +CONFIG_RTE_EAL_IGB_UIO_RESET=n >>> CONFIG_RTE_EAL_VFIO=n >>> CONFIG_RTE_MALLOC_DEBUG=n >>> CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n >>> diff --git a/config/common_linuxapp b/config/common_linuxapp >>> index 74c7d64ec..b3a602909 100644 >>> --- a/config/common_linuxapp >>> +++ b/config/common_linuxapp >>> @@ -37,6 +37,7 @@ CONFIG_RTE_EXEC_ENV_LINUXAPP=y >>> >>> CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=y >>> CONFIG_RTE_EAL_IGB_UIO=y >>> +CONFIG_RTE_EAL_IGB_UIO_RESET=y >>> CONFIG_RTE_EAL_VFIO=y >>> CONFIG_RTE_KNI_KMOD=y >>> CONFIG_RTE_LIBRTE_KNI=y >>> diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c >>> index fd320d87d..0325722c0 100644 >>> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c >>> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c >>> @@ -360,7 +360,9 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode) >>> /* stop the device from further DMA */ >>> pci_clear_master(dev); >>> >>> +#ifdef RTE_EAL_IGB_UIO_RESET >>> pci_reset_function(dev); >>> +#endif >>> >>> return 0; >>> } > > A compile time configuration option makes life very difficult for application providers. > > Consider the case where an application such as Open vSwitch with DPDK support is being provided > with a Linux distribution. One would want the Open vSwitch binary to support as many vendor NICs > as possible---without the need to recompile. With a change such as this, one would need to have > different versions of the kernel igb_uio module to support different NICs. Agreed, I am against adding more compile time options although I am end up sending a few of them these days. > The Linux kernel is already aware of, and provides work-arounds for, various PCI quirks. > For example, see linux/drivers/pci/quirks.c (http://elixir.free-electrons.com/linux/latest/source/drivers/pci/quirks.c). > > At this point in igb_uio.c, one is aware of the struct pci_dev "dev" for the device in question. > Access to the vendor and device information should be simple: > > struct pci_dev { > struct list_head bus_list;/* node in per-bus list */ > struct pci_bus*bus;/* bus this device is on */ > struct pci_bus*subordinate;/* bus this device bridges to */ > > void*sysdata;/* hook for sys-specific extension */ > struct proc_dir_entry *procent;/* device entry in /proc/bus/pci */ > struct pci_slot*slot;/* Physical slot this device is in */ > > unsigned intdevfn;/* encoded device & function index */ > unsigned shortvendor; > unsigned shortdevice; > unsigned shortsubsystem_vendor; > unsigned shortsubsystem_device; > ... > > One could imagine using logic to implement corresponding PCI quirks that can be evaluated > at runtime. For example (in pseudocode), > > if not (vendor = "Cavium" and device = "bnx2x") > then pci_reset_function(dev); It wouldn't be nice to add device specific checks into generic igb_uio module, but also it is not nice to add compile time option, comparing two I would be OK to device checks. What do you think about following? If there is no objection and Rasesh confirms that patch is working, I can send a proper patch for it. diff --git a/lib/librte_eal/linuxapp/igb_uio/compat.h b/lib/librte_eal/linuxapp/igb_uio/compat.h index 30508f35c..264206af3 100644 --- a/lib/librte_eal/linuxapp/igb_uio/compat.h +++ b/lib/librte_eal/linuxapp/igb_uio/compat.h @@ -134,3 +134,21 @@ static bool pci_check_and_mask_intx(struct pci_dev *pdev) #endif +#define BROADCOM_PCI_VENDOR_ID 0x14E4 +static const struct pci_device_id no_reset_pci_tbl[] = { + { PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x168a) }, /* 57800 */ + { PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x164f) }, /* 57711 */ + { 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 fd320d87d..b0d92b51e 100644 --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c @@ -348,6 +348,14 @@ igbuio_pci_open(struct uio_info *info, struct inode *inode) return 0; } +static int is_device_excluded_from_reset(struct pci_dev *pdev) +{ + if (pci_match_id(no_reset_pci_tbl, pdev)) + return 1; + + return 0; +} + static int igbuio_pci_release(struct uio_info *info, struct inode *inode) { @@ -360,7 +368,8 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode) /* stop the device from further DMA */ pci_clear_master(dev); - pci_reset_function(dev); + if (!is_device_excluded_from_reset(dev)) + pci_reset_function(dev); return 0; } > > There are other possible implementations. If there are enough quirks, one might have action > functions defined---and a table of function pointers associated with each PMD to select the > proper action. > > - Lee Roberts > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH v2] igb_uio: add config option to control reset 2017-11-03 22:03 ` Ferruh Yigit @ 2017-11-04 10:08 ` Stephen Hemminger 2017-11-06 18:41 ` Ferruh Yigit 0 siblings, 1 reply; 7+ messages in thread From: Stephen Hemminger @ 2017-11-04 10:08 UTC (permalink / raw) To: Ferruh Yigit Cc: Roberts, Lee A., Tan, Jianfeng, Thomas Monjalon, dev, stable, Wu, Jingjing, Shijith Thotton, Gregory Etelson, Harish Patil, George Prekas, Gonzalez Monroy, Sergio, Rasesh Mody On Nov 4, 2017 01:03, "Ferruh Yigit" <ferruh.yigit@intel.com> wrote: On 11/3/2017 12:42 PM, Roberts, Lee A. wrote: > > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tan, Jianfeng >> Sent: Thursday, November 02, 2017 8:57 PM >> To: Ferruh Yigit <ferruh.yigit@intel.com>; Thomas Monjalon < thomas@monjalon.net> >> Cc: dev@dpdk.org; stable@dpdk.org; Jingjing Wu <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>; Sergio Gonzalez Monroy >> <sergio.gonzalez.monroy@intel.com>; Rasesh Mody <rasesh.mody@cavium.com> >> Subject: Re: [dpdk-dev] [PATCH v2] igb_uio: add config option to control reset >> >> >> >> On 11/3/2017 8:51 AM, Ferruh Yigit wrote: >>> Adding a compile time configuration option to control device reset done >>> during DPDK application exit. >>> >>> Config option is CONFIG_RTE_EAL_IGB_UIO_RESET and enabled by default, >>> so by default reset will happen. Having this reset is safer to be sure >>> device left in a proper case. >>> >>> But for special cases [1] it is possible to disable the config option >>> to prevent the device reset. >>> >>> [1] >>> http://dpdk.org/ml/archives/dev/2017-November/080927.html >>> >>> 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> >> >> Realize that we do have a pci_clear_master() in the release() to disable >> the DMA from device until the next open() will enable the DMA again . >> Here is my: >> >> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com> >> >> Thanks, >> Jianfeng >> >>> --- >>> 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> >>> >>> v2: >>> * fix typo in commit log >>> --- >>> config/common_base | 1 + >>> config/common_linuxapp | 1 + >>> lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 2 ++ >>> 3 files changed, 4 insertions(+) >>> >>> diff --git a/config/common_base b/config/common_base >>> index 82ee75456..2a9947420 100644 >>> --- a/config/common_base >>> +++ b/config/common_base >>> @@ -102,6 +102,7 @@ CONFIG_RTE_LIBEAL_USE_HPET=n >>> CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n >>> CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n >>> CONFIG_RTE_EAL_IGB_UIO=n >>> +CONFIG_RTE_EAL_IGB_UIO_RESET=n >>> CONFIG_RTE_EAL_VFIO=n >>> CONFIG_RTE_MALLOC_DEBUG=n >>> CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n >>> diff --git a/config/common_linuxapp b/config/common_linuxapp >>> index 74c7d64ec..b3a602909 100644 >>> --- a/config/common_linuxapp >>> +++ b/config/common_linuxapp >>> @@ -37,6 +37,7 @@ CONFIG_RTE_EXEC_ENV_LINUXAPP=y >>> >>> CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=y >>> CONFIG_RTE_EAL_IGB_UIO=y >>> +CONFIG_RTE_EAL_IGB_UIO_RESET=y >>> CONFIG_RTE_EAL_VFIO=y >>> CONFIG_RTE_KNI_KMOD=y >>> CONFIG_RTE_LIBRTE_KNI=y >>> diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c >>> index fd320d87d..0325722c0 100644 >>> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c >>> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c >>> @@ -360,7 +360,9 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode) >>> /* stop the device from further DMA */ >>> pci_clear_master(dev); >>> >>> +#ifdef RTE_EAL_IGB_UIO_RESET >>> pci_reset_function(dev); >>> +#endif >>> >>> return 0; >>> } > > A compile time configuration option makes life very difficult for application providers. > > Consider the case where an application such as Open vSwitch with DPDK support is being provided > with a Linux distribution. One would want the Open vSwitch binary to support as many vendor NICs > as possible---without the need to recompile. With a change such as this, one would need to have > different versions of the kernel igb_uio module to support different NICs. Agreed, I am against adding more compile time options although I am end up sending a few of them these days. > The Linux kernel is already aware of, and provides work-arounds for, various PCI quirks. > For example, see linux/drivers/pci/quirks.c (http://elixir.free-electrons. com/linux/latest/source/drivers/pci/quirks.c). > > At this point in igb_uio.c, one is aware of the struct pci_dev "dev" for the device in question. > Access to the vendor and device information should be simple: > > struct pci_dev { > struct list_head bus_list;/* node in per-bus list */ > struct pci_bus*bus;/* bus this device is on */ > struct pci_bus*subordinate;/* bus this device bridges to */ > > void*sysdata;/* hook for sys-specific extension */ > struct proc_dir_entry *procent;/* device entry in /proc/bus/pci */ > struct pci_slot*slot;/* Physical slot this device is in */ > > unsigned intdevfn;/* encoded device & function index */ > unsigned shortvendor; > unsigned shortdevice; > unsigned shortsubsystem_vendor; > unsigned shortsubsystem_device; > ... > > One could imagine using logic to implement corresponding PCI quirks that can be evaluated > at runtime. For example (in pseudocode), > > if not (vendor = "Cavium" and device = "bnx2x") > then pci_reset_function(dev); It wouldn't be nice to add device specific checks into generic igb_uio module, but also it is not nice to add compile time option, comparing two I would be OK to device checks. What do you think about following? If there is no objection and Rasesh confirms that patch is working, I can send a proper patch for it. diff --git a/lib/librte_eal/linuxapp/igb_uio/compat.h b/lib/librte_eal/linuxapp/igb_uio/compat.h index 30508f35c..264206af3 100644 --- a/lib/librte_eal/linuxapp/igb_uio/compat.h +++ b/lib/librte_eal/linuxapp/igb_uio/compat.h @@ -134,3 +134,21 @@ static bool pci_check_and_mask_intx(struct pci_dev *pdev) #endif +#define BROADCOM_PCI_VENDOR_ID 0x14E4 +static const struct pci_device_id no_reset_pci_tbl[] = { + { PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x168a) }, /* 57800 */ + { PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x164f) }, /* 57711 */ + { 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 fd320d87d..b0d92b51e 100644 --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c @@ -348,6 +348,14 @@ igbuio_pci_open(struct uio_info *info, struct inode *inode) return 0; } +static int is_device_excluded_from_reset(struct pci_dev *pdev) +{ + if (pci_match_id(no_reset_pci_tbl, pdev)) + return 1; + + return 0; +} + Personal preference is for more concise: static book is_device_excluded(const 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) { @@ -360,7 +368,8 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode) /* stop the device from further DMA */ pci_clear_master(dev); - pci_reset_function(dev); + if (!is_device_excluded_from_reset(dev)) + pci_reset_function(dev); return 0; } > > There are other possible implementations. If there are enough quirks, one might have action > functions defined---and a table of function pointers associated with each PMD to select the > proper action. > > - Lee Roberts > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH v2] igb_uio: add config option to control reset 2017-11-04 10:08 ` Stephen Hemminger @ 2017-11-06 18:41 ` Ferruh Yigit 0 siblings, 0 replies; 7+ messages in thread From: Ferruh Yigit @ 2017-11-06 18:41 UTC (permalink / raw) To: Stephen Hemminger Cc: Roberts, Lee A., Tan, Jianfeng, Thomas Monjalon, dev, stable, Wu, Jingjing, Shijith Thotton, Gregory Etelson, Harish Patil, George Prekas, Gonzalez Monroy, Sergio, Rasesh Mody On 11/4/2017 3:08 AM, Stephen Hemminger wrote: > > > On Nov 4, 2017 01:03, "Ferruh Yigit" <ferruh.yigit@intel.com > <mailto:ferruh.yigit@intel.com>> wrote: > > On 11/3/2017 12:42 PM, Roberts, Lee A. wrote: > > > > > >> -----Original Message----- > >> From: dev [mailto:dev-bounces@dpdk.org <mailto:dev-bounces@dpdk.org>] On > Behalf Of Tan, Jianfeng > >> Sent: Thursday, November 02, 2017 8:57 PM > >> To: Ferruh Yigit <ferruh.yigit@intel.com > <mailto:ferruh.yigit@intel.com>>; Thomas Monjalon <thomas@monjalon.net > <mailto:thomas@monjalon.net>> > >> Cc: dev@dpdk.org <mailto:dev@dpdk.org>; stable@dpdk.org > <mailto:stable@dpdk.org>; Jingjing Wu <jingjing.wu@intel.com > <mailto:jingjing.wu@intel.com>>; Shijith Thotton > >> <shijith.thotton@caviumnetworks.com > <mailto:shijith.thotton@caviumnetworks.com>>; Gregory Etelson > <gregory@weka.io <mailto:gregory@weka.io>>; Harish Patil > >> <harish.patil@cavium.com <mailto:harish.patil@cavium.com>>; George Prekas > <george.prekas@epfl.ch <mailto:george.prekas@epfl.ch>>; Sergio Gonzalez Monroy > >> <sergio.gonzalez.monroy@intel.com > <mailto:sergio.gonzalez.monroy@intel.com>>; Rasesh Mody > <rasesh.mody@cavium.com <mailto:rasesh.mody@cavium.com>> > >> Subject: Re: [dpdk-dev] [PATCH v2] igb_uio: add config option to control > reset > >> > >> > >> > >> On 11/3/2017 8:51 AM, Ferruh Yigit wrote: > >>> Adding a compile time configuration option to control device reset done > >>> during DPDK application exit. > >>> > >>> Config option is CONFIG_RTE_EAL_IGB_UIO_RESET and enabled by default, > >>> so by default reset will happen. Having this reset is safer to be sure > >>> device left in a proper case. > >>> > >>> But for special cases [1] it is possible to disable the config option > >>> to prevent the device reset. > >>> > >>> [1] > >>> http://dpdk.org/ml/archives/dev/2017-November/080927.html > <http://dpdk.org/ml/archives/dev/2017-November/080927.html> > >>> > >>> Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of > device file") > >>> Cc: stable@dpdk.org <mailto:stable@dpdk.org> > >>> > >>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com > <mailto:ferruh.yigit@intel.com>> > >> > >> Realize that we do have a pci_clear_master() in the release() to disable > >> the DMA from device until the next open() will enable the DMA again . > >> Here is my: > >> > >> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com > <mailto:jianfeng.tan@intel.com>> > >> > >> Thanks, > >> Jianfeng > >> > >>> --- > >>> Cc: Jianfeng Tan <jianfeng.tan@intel.com <mailto:jianfeng.tan@intel.com>> > >>> Cc: Jingjing Wu <jingjing.wu@intel.com <mailto:jingjing.wu@intel.com>> > >>> Cc: Shijith Thotton <shijith.thotton@caviumnetworks.com > <mailto:shijith.thotton@caviumnetworks.com>> > >>> Cc: Gregory Etelson <gregory@weka.io <mailto:gregory@weka.io>> > >>> Cc: Harish Patil <harish.patil@cavium.com <mailto:harish.patil@cavium.com>> > >>> Cc: George Prekas <george.prekas@epfl.ch <mailto:george.prekas@epfl.ch>> > >>> Cc: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com > <mailto:sergio.gonzalez.monroy@intel.com>> > >>> Cc: Rasesh Mody <rasesh.mody@cavium.com <mailto:rasesh.mody@cavium.com>> > >>> > >>> v2: > >>> * fix typo in commit log > >>> --- > >>> config/common_base | 1 + > >>> config/common_linuxapp | 1 + > >>> lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 2 ++ > >>> 3 files changed, 4 insertions(+) > >>> > >>> diff --git a/config/common_base b/config/common_base > >>> index 82ee75456..2a9947420 100644 > >>> --- a/config/common_base > >>> +++ b/config/common_base > >>> @@ -102,6 +102,7 @@ CONFIG_RTE_LIBEAL_USE_HPET=n > >>> CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n > >>> CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n > >>> CONFIG_RTE_EAL_IGB_UIO=n > >>> +CONFIG_RTE_EAL_IGB_UIO_RESET=n > >>> CONFIG_RTE_EAL_VFIO=n > >>> CONFIG_RTE_MALLOC_DEBUG=n > >>> CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n > >>> diff --git a/config/common_linuxapp b/config/common_linuxapp > >>> index 74c7d64ec..b3a602909 100644 > >>> --- a/config/common_linuxapp > >>> +++ b/config/common_linuxapp > >>> @@ -37,6 +37,7 @@ CONFIG_RTE_EXEC_ENV_LINUXAPP=y > >>> > >>> CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=y > >>> CONFIG_RTE_EAL_IGB_UIO=y > >>> +CONFIG_RTE_EAL_IGB_UIO_RESET=y > >>> CONFIG_RTE_EAL_VFIO=y > >>> CONFIG_RTE_KNI_KMOD=y > >>> CONFIG_RTE_LIBRTE_KNI=y > >>> diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c > b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c > >>> index fd320d87d..0325722c0 100644 > >>> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c > >>> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c > >>> @@ -360,7 +360,9 @@ igbuio_pci_release(struct uio_info *info, struct > inode *inode) > >>> /* stop the device from further DMA */ > >>> pci_clear_master(dev); > >>> > >>> +#ifdef RTE_EAL_IGB_UIO_RESET > >>> pci_reset_function(dev); > >>> +#endif > >>> > >>> return 0; > >>> } > > > > A compile time configuration option makes life very difficult for > application providers. > > > > Consider the case where an application such as Open vSwitch with DPDK > support is being provided > > with a Linux distribution. One would want the Open vSwitch binary to > support as many vendor NICs > > as possible---without the need to recompile. With a change such as this, > one would need to have > > different versions of the kernel igb_uio module to support different NICs. > > Agreed, I am against adding more compile time options although I am end up > sending a few of them these days. > > > The Linux kernel is already aware of, and provides work-arounds for, > various PCI quirks. > > For example, see linux/drivers/pci/quirks.c > (http://elixir.free-electrons.com/linux/latest/source/drivers/pci/quirks.c > <http://elixir.free-electrons.com/linux/latest/source/drivers/pci/quirks.c>). > > > > At this point in igb_uio.c, one is aware of the struct pci_dev "dev" for > the device in question. > > Access to the vendor and device information should be simple: > > > > struct pci_dev { > > struct list_head bus_list;/* node in per-bus list */ > > struct pci_bus*bus;/* bus this device is on */ > > struct pci_bus*subordinate;/* bus this device bridges to */ > > > > void*sysdata;/* hook for sys-specific extension */ > > struct proc_dir_entry *procent;/* device entry in /proc/bus/pci */ > > struct pci_slot*slot;/* Physical slot this device is in */ > > > > unsigned intdevfn;/* encoded device & function index */ > > unsigned shortvendor; > > unsigned shortdevice; > > unsigned shortsubsystem_vendor; > > unsigned shortsubsystem_device; > > ... > > > > One could imagine using logic to implement corresponding PCI quirks that > can be evaluated > > at runtime. For example (in pseudocode), > > > > if not (vendor = "Cavium" and device = "bnx2x") > > then pci_reset_function(dev); > > It wouldn't be nice to add device specific checks into generic igb_uio module, > but also it is not nice to add compile time option, comparing two I would be OK > to device checks. > > What do you think about following? > If there is no objection and Rasesh confirms that patch is working, I can send a > proper patch for it. > > > > diff --git a/lib/librte_eal/linuxapp/igb_uio/compat.h > b/lib/librte_eal/linuxapp/igb_uio/compat.h > index 30508f35c..264206af3 100644 > --- a/lib/librte_eal/linuxapp/igb_uio/compat.h > +++ b/lib/librte_eal/linuxapp/igb_uio/compat.h > @@ -134,3 +134,21 @@ static bool pci_check_and_mask_intx(struct pci_dev *pdev) > #endif > > > +#define BROADCOM_PCI_VENDOR_ID 0x14E4 > +static const struct pci_device_id no_reset_pci_tbl[] = { > + { PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x168a) }, /* 57800 */ > + { PCI_DEVICE(BROADCOM_PCI_VENDOR_ID, 0x164f) }, /* 57711 */ > + { 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 fd320d87d..b0d92b51e 100644 > --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c > +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c > @@ -348,6 +348,14 @@ igbuio_pci_open(struct uio_info *info, struct inode *inode) > return 0; > } > > +static int is_device_excluded_from_reset(struct pci_dev *pdev) > +{ > + if (pci_match_id(no_reset_pci_tbl, pdev)) > + return 1; > + > + return 0; > +} > + > > > Personal preference is for more concise: > static book is_device_excluded(const struct pci_dev *pdev) > { > return pci_match_id(no_reset_pci_tbl, pdev); > > } I will update function, but I am for keeping function name to clarify what device is excluded from. > > static int > igbuio_pci_release(struct uio_info *info, struct inode *inode) > { > @@ -360,7 +368,8 @@ igbuio_pci_release(struct uio_info *info, struct inode > *inode) > /* stop the device from further DMA */ > pci_clear_master(dev); > > - pci_reset_function(dev); > + if (!is_device_excluded_from_reset(dev)) > + pci_reset_function(dev); > > return 0; > } > > > > > > There are other possible implementations. If there are enough quirks, one > might have action > > functions defined---and a table of function pointers associated with each > PMD to select the > > proper action. > > > > - Lee Roberts > > > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-11-06 18:41 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-03 0:30 [dpdk-dev] [PATCH] igb_uio: add config option to control reset Ferruh Yigit 2017-11-03 0:51 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit 2017-11-03 2:57 ` Tan, Jianfeng 2017-11-03 19:42 ` Roberts, Lee A. 2017-11-03 22:03 ` Ferruh Yigit 2017-11-04 10:08 ` Stephen Hemminger 2017-11-06 18:41 ` Ferruh Yigit
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).