Re: [dpdk-dev] [PATCH v2 2/7] drivers: add generic API to find PCI extended cap

["Gaëtan Rivet" <grive@u256.net>]

On 13/07/20 08:13 -0700, Manish Chopra wrote:
> By adding generic API, this patch removes individual
> functions/defines implemented by drivers to find PCI
> extended capability.
> 
> Signed-off-by: Manish Chopra <manishc@marvell.com>
> Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> ---
>  drivers/bus/pci/pci_common.c               | 41 +++++++++++++++++
>  drivers/bus/pci/rte_bus_pci.h              | 11 +++++
>  drivers/net/ice/ice_ethdev.c               | 53 ++--------------------
>  drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 48 ++------------------
>  lib/librte_pci/rte_pci_regs.h              |  2 +-
>  5 files changed, 60 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> index a8e5fd52c..5117c8e7b 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -23,6 +23,7 @@
>  #include <rte_common.h>
>  #include <rte_devargs.h>
>  #include <rte_vfio.h>
> +#include <rte_pci_regs.h>
>  
>  #include "private.h"
>  
> @@ -665,6 +666,46 @@ rte_pci_get_iommu_class(void)
>  	return iova_mode;
>  }
>  
> +int rte_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap)
> +{
> +	int pos = PCI_CFG_SPACE_SIZE;

pos is somewhat nondescript. position is long however.
offset seems more appropriate, short but describing properly the value.

Remember to prefix all those defines with 'RTE_'.

> +	uint32_t header;
> +	int ttl;
> +
> +	/* minimum 8 bytes per capability */
> +	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> +
> +	if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> +		RTE_LOG(ERR, EAL, "error in reading extended capabilities\n");
> +		return -EINVAL;

The -EINVAL here is not useful. It is not used by callers anyway. It
can be simplified to -1.

> +	}
> +
> +	/*
> +	 * If we have no capabilities, this is indicated by cap ID,
> +	 * cap version and next pointer all being 0.
> +	 */
> +	if (header == 0)
> +		return 0;
> +
> +	while (ttl-- > 0) {

Not a fan of this, the checkpatch-compliant form of (ttl --> 0).
Decrementing the value within the loop conditional is generally frowned
upon and should be avoided.

    while (ttl != 0) {
            [...]
            ttl -= 1;
    }

> +		if (PCI_EXT_CAP_ID(header) == cap)

This define would originally resolve to uint32_t, which is why you had to cast it to
int in the macro itself.

This is not ok, the cap type should already be in sync (uint32_t).
Casting should not be needed. It should not be used just to shut up a
warning about signed to unsigned comparison.

> +			return pos;

Similarly for the return type, it is used as off_t afterward when calling
rte_pci_read_config(). All users of this API will be encouraged to use
an int where they should be using off_t instead (even if POSIX defines
it as signed int anyway).

It's really messy that the return type is an int that will be used
either as <0 on read error, 0 on missing cap, >0 for proper offset.
Separating the error channel from the payload won't make the API cleaner
though, so let's keep it that way. However, given that POSIX defines
off_t as a signed type anyway, let's use off_t directly, -1 for error, 0
for no error but no header, and >0 for a proper offset.

This means this function will not be a transparent drop-in for the
previous implem, but so be it.

> +
> +		pos = PCI_EXT_CAP_NEXT(header);
> +
> +		if (pos < PCI_CFG_SPACE_SIZE)
> +			break;
> +
> +		if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> +			RTE_LOG(ERR, EAL,
> +				"error in reading extended capabilities\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  struct rte_pci_bus rte_pci_bus = {
>  	.bus = {
>  		.scan = rte_pci_scan,
> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
> index 29bea6d70..3cc66220a 100644
> --- a/drivers/bus/pci/rte_bus_pci.h
> +++ b/drivers/bus/pci/rte_bus_pci.h
> @@ -224,6 +224,17 @@ void rte_pci_unmap_device(struct rte_pci_device *dev);
>   */
>  void rte_pci_dump(FILE *f);
>  
> +/**
> + * Find device's extended capability
> + *
> + *  @param dev
> + *    A pointer to rte_pci_device structure

Missing a period here.

> + *
> + *  @param cap
> + *    Extended capability to find

ditto.

> + */

The doc is missing the return values and their signification. It
returns an offset where the ext. cap can be read in the PCI config?
Users should be able to understand this reading only this comment.

Also, 0 meaning the cap is not available is critical to know.

> +int rte_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap);
> +
>  /**
>   * Register a PCI driver.
>   *
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 3534d18ca..0724324d2 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -4,6 +4,8 @@
>  
>  #include <rte_string_fns.h>
>  #include <rte_ethdev_pci.h>
> +#include <rte_bus_pci.h>
> +#include <rte_pci_regs.h>
>  
>  #include <stdio.h>
>  #include <sys/types.h>
> @@ -1730,53 +1732,6 @@ ice_pf_setup(struct ice_pf *pf)
>  	return 0;
>  }
>  
> -/* PCIe configuration space setting */
> -#define PCI_CFG_SPACE_SIZE          256
> -#define PCI_CFG_SPACE_EXP_SIZE      4096
> -#define PCI_EXT_CAP_ID(header)      (int)((header) & 0x0000ffff)
> -#define PCI_EXT_CAP_NEXT(header)    (((header) >> 20) & 0xffc)
> -#define PCI_EXT_CAP_ID_DSN          0x03
> -
> -static int
> -ice_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap)
> -{
> -	uint32_t header;
> -	int ttl;
> -	int pos = PCI_CFG_SPACE_SIZE;
> -
> -	/* minimum 8 bytes per capability */
> -	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> -
> -	if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> -		PMD_INIT_LOG(ERR, "ice error reading extended capabilities\n");
> -		return -1;
> -	}
> -
> -	/*
> -	 * If we have no capabilities, this is indicated by cap ID,
> -	 * cap version and next pointer all being 0.
> -	 */
> -	if (header == 0)
> -		return 0;
> -
> -	while (ttl-- > 0) {
> -		if (PCI_EXT_CAP_ID(header) == cap)
> -			return pos;
> -
> -		pos = PCI_EXT_CAP_NEXT(header);
> -
> -		if (pos < PCI_CFG_SPACE_SIZE)
> -			break;
> -
> -		if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> -			PMD_INIT_LOG(ERR, "ice error reading extended capabilities\n");
> -			return -1;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>  /*
>   * Extract device serial number from PCIe Configuration Space and
>   * determine the pkg file path according to the DSN.
> @@ -1789,9 +1744,9 @@ ice_pkg_file_search_path(struct rte_pci_device *pci_dev, char *pkg_file)
>  	uint32_t dsn_low, dsn_high;
>  	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
>  
> -	pos = ice_pci_find_next_ext_capability(pci_dev, PCI_EXT_CAP_ID_DSN);
> +	pos = rte_pci_find_next_ext_capability(pci_dev, PCI_EXT_CAP_ID_DSN);
>  
> -	if (pos) {
> +	if (pos > 0) {

You are fixing a bug in the ICE driver here.
On PCI config read error, -1 is returned, meaning the offset (-1 + 4) is used
to read 4 bytes, which does not seem ok.

This should be fixed in a separate commit to allow a backport.

>  		rte_pci_read_config(pci_dev, &dsn_low, 4, pos + 4);
>  		rte_pci_read_config(pci_dev, &dsn_high, 4, pos + 8);
>  		snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
> diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> index 0b9db974e..b11d8148a 100644
> --- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> +++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> @@ -35,6 +35,8 @@
>  
>  #include <rte_ethdev_pci.h>
>  #include <rte_string_fns.h>
> +#include <rte_bus_pci.h>
> +#include <rte_pci_regs.h>
>  
>  #include "nfp_cpp.h"
>  #include "nfp_target.h"
> @@ -746,50 +748,6 @@ nfp6000_set_interface(struct rte_pci_device *dev, struct nfp_cpp *cpp)
>  	return 0;
>  }
>  
> -#define PCI_CFG_SPACE_SIZE	256
> -#define PCI_CFG_SPACE_EXP_SIZE	4096
> -#define PCI_EXT_CAP_ID(header)		(int)(header & 0x0000ffff)
> -#define PCI_EXT_CAP_NEXT(header)	((header >> 20) & 0xffc)
> -#define PCI_EXT_CAP_ID_DSN	0x03
> -static int
> -nfp_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap)
> -{
> -	uint32_t header;
> -	int ttl;
> -	int pos = PCI_CFG_SPACE_SIZE;
> -
> -	/* minimum 8 bytes per capability */
> -	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> -
> -	if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> -		printf("nfp error reading extended capabilities\n");
> -		return -1;
> -	}
> -
> -	/*
> -	 * If we have no capabilities, this is indicated by cap ID,
> -	 * cap version and next pointer all being 0.
> -	 */
> -	if (header == 0)
> -		return 0;
> -
> -	while (ttl-- > 0) {
> -		if (PCI_EXT_CAP_ID(header) == cap)
> -			return pos;
> -
> -		pos = PCI_EXT_CAP_NEXT(header);
> -		if (pos < PCI_CFG_SPACE_SIZE)
> -			break;
> -
> -		if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> -			printf("nfp error reading extended capabilities\n");
> -			return -1;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>  static int
>  nfp6000_set_serial(struct rte_pci_device *dev, struct nfp_cpp *cpp)
>  {
> @@ -798,7 +756,7 @@ nfp6000_set_serial(struct rte_pci_device *dev, struct nfp_cpp *cpp)
>  	int serial_len = 6;
>  	int pos;
>  
> -	pos = nfp_pci_find_next_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
> +	pos = rte_pci_find_next_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
>  	if (pos <= 0) {
>  		printf("PCI_EXT_CAP_ID_DSN not found. nfp set serial failed\n");
>  		return -1;
> diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> index 1d11f4de5..108193049 100644
> --- a/lib/librte_pci/rte_pci_regs.h
> +++ b/lib/librte_pci/rte_pci_regs.h
> @@ -686,7 +686,7 @@
>  #define PCI_EXP_SLTSTA2		58	/* Slot Status 2 */
>  
>  /* Extended Capabilities (PCI-X 2.0 and Express) */
> -#define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)
> +#define PCI_EXT_CAP_ID(header)		(int)(header & 0x0000ffff)
>  #define PCI_EXT_CAP_VER(header)		((header >> 16) & 0xf)
>  #define PCI_EXT_CAP_NEXT(header)	((header >> 20) & 0xffc)
>  
> -- 
> 2.17.1
> 

-- 
Gaëtan