All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] igb_uio: add config option to control reset
@ 2017-11-03  0:30 Ferruh Yigit
  2017-11-03  0:51 ` [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 related	[flat|nested] 7+ messages in thread
* [PATCH v2] igb_uio: add config option to control reset
  2017-11-03  0:30 [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 related	[flat|nested] 7+ messages in thread
* Re: [PATCH v2] igb_uio: add config option to control reset
  2017-11-03  0:51 ` [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: [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: [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 related	[flat|nested] 7+ messages in thread
* Re: [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 related	[flat|nested] 7+ messages in thread
* Re: [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 [PATCH] igb_uio: add config option to control reset Ferruh Yigit
2017-11-03  0:51 ` [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.