On 11/4/2017 3:08 AM, Stephen Hemminger wrote:
>
>
> On Nov 4, 2017 01:03, "Ferruh Yigit" <ferruh.yi...@intel.com
> <mailto:ferruh.yi...@intel.com>> wrote:
>
> On 11/3/2017 12:42 PM, Roberts, Lee A. wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-boun...@dpdk.org <mailto:dev-boun...@dpdk.org>]
> On
> Behalf Of Tan, Jianfeng
> >> Sent: Thursday, November 02, 2017 8:57 PM
> >> To: Ferruh Yigit <ferruh.yi...@intel.com
> <mailto:ferruh.yi...@intel.com>>; Thomas Monjalon <tho...@monjalon.net
> <mailto:tho...@monjalon.net>>
> >> Cc: dev@dpdk.org <mailto:dev@dpdk.org>; sta...@dpdk.org
> <mailto:sta...@dpdk.org>; Jingjing Wu <jingjing...@intel.com
> <mailto:jingjing...@intel.com>>; Shijith Thotton
> >> <shijith.thot...@caviumnetworks.com
> <mailto:shijith.thot...@caviumnetworks.com>>; Gregory Etelson
> <greg...@weka.io <mailto:greg...@weka.io>>; Harish Patil
> >> <harish.pa...@cavium.com <mailto:harish.pa...@cavium.com>>; George
> Prekas
> <george.pre...@epfl.ch <mailto:george.pre...@epfl.ch>>; Sergio Gonzalez
> Monroy
> >> <sergio.gonzalez.mon...@intel.com
> <mailto:sergio.gonzalez.mon...@intel.com>>; Rasesh Mody
> <rasesh.m...@cavium.com <mailto:rasesh.m...@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: sta...@dpdk.org <mailto:sta...@dpdk.org>
> >>>
> >>> Signed-off-by: Ferruh Yigit <ferruh.yi...@intel.com
> <mailto:ferruh.yi...@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....@intel.com
> <mailto:jianfeng....@intel.com>>
> >>
> >> Thanks,
> >> Jianfeng
> >>
> >>> ---
> >>> Cc: Jianfeng Tan <jianfeng....@intel.com
> <mailto:jianfeng....@intel.com>>
> >>> Cc: Jingjing Wu <jingjing...@intel.com <mailto:jingjing...@intel.com>>
> >>> Cc: Shijith Thotton <shijith.thot...@caviumnetworks.com
> <mailto:shijith.thot...@caviumnetworks.com>>
> >>> Cc: Gregory Etelson <greg...@weka.io <mailto:greg...@weka.io>>
> >>> Cc: Harish Patil <harish.pa...@cavium.com
> <mailto:harish.pa...@cavium.com>>
> >>> Cc: George Prekas <george.pre...@epfl.ch
> <mailto:george.pre...@epfl.ch>>
> >>> Cc: Sergio Gonzalez Monroy <sergio.gonzalez.mon...@intel.com
> <mailto:sergio.gonzalez.mon...@intel.com>>
> >>> Cc: Rasesh Mody <rasesh.m...@cavium.com
> <mailto:rasesh.m...@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
> >
> >
> >
> >
>
>
