Re: [dpdk-dev] [PATCH v2] igb_uio: add config option to control reset

[Stephen Hemminger <stephen@networkplumber.org>]

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
>
>
>
>