Re: [dpdk-dev] [PATCH 3/3] igb_uio: remove sys files for setting pci config space

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 21 Dec 2015 10:38:06 +0800
Helin Zhang <helin.zhang@intel.com> wrote:

> Sys files of 'extended_tag' and 'max_read_request_size' are
> useless, as nobody will use them for setting pci config space.
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> ---
>  doc/guides/linux_gsg/enable_func.rst      |  22 ------
>  doc/guides/rel_notes/deprecation.rst      |   3 +
>  doc/guides/rel_notes/release_2_3.rst      |   6 ++
>  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 108 ------------------------------
>  4 files changed, 9 insertions(+), 130 deletions(-)
> 
> diff --git a/doc/guides/linux_gsg/enable_func.rst b/doc/guides/linux_gsg/enable_func.rst
> index c3fa6d3..ec0e04d 100644
> --- a/doc/guides/linux_gsg/enable_func.rst
> +++ b/doc/guides/linux_gsg/enable_func.rst
> @@ -186,28 +186,6 @@ Check with the local Intel's Network Division application engineers for firmware
>  The base driver to support firmware version of FVL3E will be integrated in the next
>  DPDK release, so currently the validated firmware version is 4.2.6.
>  
> -Enabling Extended Tag and Setting Max Read Request Size
> -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> -
> -PCI configurations of ``extended_tag`` and max _read_requ st_size have big impacts on performance of small packets on 40G NIC.
> -Enabling extended_tag and setting ``max_read_request_size`` to small size such as 128 bytes provide great helps to high performance of small packets.
> -
> -*   These can be done in some BIOS implementations.
> -
> -*   For other BIOS implementations, PCI configurations can be changed by using command of ``setpci``, or special configurations in DPDK config file of ``common_linux``.
> -
> -    *   Bits 7:5 at address of 0xA8 of each PCI device is used for setting the max_read_request_size,
> -        and bit 8 of 0xA8 of each PCI device is used for enabling/disabling the extended_tag.
> -        lspci and setpci can be used to read the values of 0xA8 and then write it back after being changed.
> -
> -    *   In config file of common_linux, below three configurations can be changed for the same purpose.
> -
> -        ``CONFIG_RTE_PCI_CONFIG``
> -
> -        ``CONFIG_RTE_PCI_EXTENDED_TAG``
> -
> -        ``CONFIG_RTE_PCI_MAX_READ_REQUEST_SIZE``
> -
>  Use 16 Bytes RX Descriptor Size
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index e94d4a2..7438f80 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -49,3 +49,6 @@ Deprecation Notices
>    commands (such as RETA update in testpmd).  This should impact
>    CMDLINE_PARSE_RESULT_BUFSIZE, STR_TOKEN_SIZE and RDLINE_BUF_SIZE.
>    It should be integrated in release 2.3.
> +
> +* The eal function of pci_config_space_set is deprecated in release 2.3, and
> +  will be removed from 2.4.
> diff --git a/doc/guides/rel_notes/release_2_3.rst b/doc/guides/rel_notes/release_2_3.rst
> index efd258b..ed10d94 100644
> --- a/doc/guides/rel_notes/release_2_3.rst
> +++ b/doc/guides/rel_notes/release_2_3.rst
> @@ -16,6 +16,12 @@ Resolved Issues
>  EAL
>  ~~~
>  
> +* **eal/linux: removed sys files for pci config space.**
> +
> +  Removed sys files of 'extended_tag' and 'max_read_request_size' and
> +  their relavant operations, as they shouldn't be done in eal for all
> +  possible devices.
> +
>  
>  Drivers
>  ~~~~~~~
> diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> index f5617d2..054d053 100644
> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> @@ -40,15 +40,6 @@
>  
>  #include "compat.h"
>  
> -#ifdef RTE_PCI_CONFIG
> -#define PCI_SYS_FILE_BUF_SIZE      10
> -#define PCI_DEV_CAP_REG            0xA4
> -#define PCI_DEV_CTRL_REG           0xA8
> -#define PCI_DEV_CAP_EXT_TAG_MASK   0x20
> -#define PCI_DEV_CTRL_EXT_TAG_SHIFT 8
> -#define PCI_DEV_CTRL_EXT_TAG_MASK  (1 << PCI_DEV_CTRL_EXT_TAG_SHIFT)
> -#endif
> -
>  /**
>   * A structure describing the private information for a uio device.
>   */
> @@ -90,109 +81,10 @@ store_max_vfs(struct device *dev, struct device_attribute *attr,
>  	return err ? err : count;
>  }
>  
> -#ifdef RTE_PCI_CONFIG
> -static ssize_t
> -show_extended_tag(struct device *dev, struct device_attribute *attr, char *buf)
> -{
> -	struct pci_dev *pci_dev = to_pci_dev(dev);
> -	uint32_t val = 0;
> -
> -	pci_read_config_dword(pci_dev, PCI_DEV_CAP_REG, &val);
> -	if (!(val & PCI_DEV_CAP_EXT_TAG_MASK)) /* Not supported */
> -		return snprintf(buf, PCI_SYS_FILE_BUF_SIZE, "%s\n", "invalid");
> -
> -	val = 0;
> -	pci_bus_read_config_dword(pci_dev->bus, pci_dev->devfn,
> -					PCI_DEV_CTRL_REG, &val);
> -
> -	return snprintf(buf, PCI_SYS_FILE_BUF_SIZE, "%s\n",
> -		(val & PCI_DEV_CTRL_EXT_TAG_MASK) ? "on" : "off");
> -}
> -
> -static ssize_t
> -store_extended_tag(struct device *dev,
> -		   struct device_attribute *attr,
> -		   const char *buf,
> -		   size_t count)
> -{
> -	struct pci_dev *pci_dev = to_pci_dev(dev);
> -	uint32_t val = 0, enable;
> -
> -	if (strncmp(buf, "on", 2) == 0)
> -		enable = 1;
> -	else if (strncmp(buf, "off", 3) == 0)
> -		enable = 0;
> -	else
> -		return -EINVAL;
> -
> -	pci_cfg_access_lock(pci_dev);
> -	pci_bus_read_config_dword(pci_dev->bus, pci_dev->devfn,
> -					PCI_DEV_CAP_REG, &val);
> -	if (!(val & PCI_DEV_CAP_EXT_TAG_MASK)) { /* Not supported */
> -		pci_cfg_access_unlock(pci_dev);
> -		return -EPERM;
> -	}
> -
> -	val = 0;
> -	pci_bus_read_config_dword(pci_dev->bus, pci_dev->devfn,
> -					PCI_DEV_CTRL_REG, &val);
> -	if (enable)
> -		val |= PCI_DEV_CTRL_EXT_TAG_MASK;
> -	else
> -		val &= ~PCI_DEV_CTRL_EXT_TAG_MASK;
> -	pci_bus_write_config_dword(pci_dev->bus, pci_dev->devfn,
> -					PCI_DEV_CTRL_REG, val);
> -	pci_cfg_access_unlock(pci_dev);
> -
> -	return count;
> -}
> -
> -static ssize_t
> -show_max_read_request_size(struct device *dev,
> -			   struct device_attribute *attr,
> -			   char *buf)
> -{
> -	struct pci_dev *pci_dev = to_pci_dev(dev);
> -	int val = pcie_get_readrq(pci_dev);
> -
> -	return snprintf(buf, PCI_SYS_FILE_BUF_SIZE, "%d\n", val);
> -}
> -
> -static ssize_t
> -store_max_read_request_size(struct device *dev,
> -			    struct device_attribute *attr,
> -			    const char *buf,
> -			    size_t count)
> -{
> -	struct pci_dev *pci_dev = to_pci_dev(dev);
> -	unsigned long size = 0;
> -	int ret;
> -
> -	if (0 != kstrtoul(buf, 0, &size))
> -		return -EINVAL;
> -
> -	ret = pcie_set_readrq(pci_dev, (int)size);
> -	if (ret < 0)
> -		return ret;
> -
> -	return count;
> -}
> -#endif
> -
>  static DEVICE_ATTR(max_vfs, S_IRUGO | S_IWUSR, show_max_vfs, store_max_vfs);
> -#ifdef RTE_PCI_CONFIG
> -static DEVICE_ATTR(extended_tag, S_IRUGO | S_IWUSR, show_extended_tag,
> -	store_extended_tag);
> -static DEVICE_ATTR(max_read_request_size, S_IRUGO | S_IWUSR,
> -	show_max_read_request_size, store_max_read_request_size);
> -#endif
>  
>  static struct attribute *dev_attrs[] = {
>  	&dev_attr_max_vfs.attr,
> -#ifdef RTE_PCI_CONFIG
> -	&dev_attr_extended_tag.attr,
> -	&dev_attr_max_read_request_size.attr,
> -#endif
>  	NULL,
>  };
>  

Agreed, the current way was a mess and it is always possible to change
pci settings easier with setpci anyway.

Acked-by: Stephen Hemminger <stephen.hemminger@networkplumber.org>