Re: [dpdk-dev] [EXT] Re: [PATCH v3 1/6] drivers: add generic API to find PCI extended cap

[Manish Chopra <manishc@marvell.com>]

> -----Original Message-----
> From: Gaëtan Rivet <grive@u256.net>
> Sent: Saturday, July 25, 2020 11:02 PM
> To: Manish Chopra <manishc@marvell.com>
> Cc: jerinjacobk@gmail.com; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> ferruh.yigit@intel.com; dev@dpdk.org; Igor Russkikh
> <irusskikh@marvell.com>; Rasesh Mody <rmody@marvell.com>; GR-Everest-
> DPDK-Dev <GR-Everest-DPDK-Dev@marvell.com>; rosen.xu@intel.com;
> tianfei.zhang@intel.com; heinrich.kuhn@netronome.com;
> qiming.yang@intel.com; qi.z.zhang@intel.com
> Subject: [EXT] Re: [PATCH v3 1/6] drivers: add generic API to find PCI
> extended cap
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hello Manish,
> 
> I have just a few nits below,
> 
> On 24/07/20 03:38 -0700, Manish Chopra wrote:
> > By adding generic API, this patch removes individual functions/defines
> > implemented by drivers to find PCI extended capability.
> 
> "to find extended PCI capabilities" sounds better.
> 
> >
> > Signed-off-by: Manish Chopra <manishc@marvell.com>
> > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> > ---
> >  drivers/bus/pci/pci_common.c               | 42 ++++++++++++++++++
> >  drivers/bus/pci/rte_bus_pci.h              | 19 ++++++++
> >  drivers/bus/pci/rte_bus_pci_version.map    |  6 +++
> >  drivers/net/ice/ice_ethdev.c               | 51 +---------------------
> >  drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 48 +-------------------
> >  drivers/raw/ifpga/ifpga_rawdev.c           |  6 ---
> >  lib/librte_pci/rte_pci.h                   | 16 +++++++
> >  7 files changed, 87 insertions(+), 101 deletions(-)
> >
> > diff --git a/drivers/bus/pci/pci_common.c
> > b/drivers/bus/pci/pci_common.c index a8e5fd52c..b877d10e9 100644
> > --- a/drivers/bus/pci/pci_common.c
> > +++ b/drivers/bus/pci/pci_common.c
> > @@ -665,6 +665,48 @@ rte_pci_get_iommu_class(void)
> >  	return iova_mode;
> >  }
> >
> > +off_t rte_pci_find_next_ext_capability(struct rte_pci_device *dev,
> > +uint32_t cap) {
> 
> Coding style asks for the return type on its own line, then the function name
> etc.
> 
> > +	off_t offset = RTE_PCI_CFG_SPACE_SIZE;
> > +	uint32_t header;
> > +	int ttl;
> > +
> > +	/* minimum 8 bytes per capability */
> > +	ttl = (RTE_PCI_CFG_SPACE_EXP_SIZE - RTE_PCI_CFG_SPACE_SIZE) / 8;
> > +
> > +	if (rte_pci_read_config(dev, &header, 4, offset) < 0) {
> > +		RTE_LOG(ERR, EAL, "error in 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 (RTE_PCI_EXT_CAP_ID(header) == cap)
> > +			return offset;
> > +
> > +		offset = RTE_PCI_EXT_CAP_NEXT(header);
> > +
> > +		if (offset < RTE_PCI_CFG_SPACE_SIZE)
> > +			break;
> > +
> > +		if (rte_pci_read_config(dev, &header, 4, offset) < 0) {
> > +			RTE_LOG(ERR, EAL,
> > +				"error in reading extended capabilities\n");
> > +			return -1;
> > +		}
> > +
> > +		ttl--;
> > +	}
> > +
> > +	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..de1ed9807 100644
> > --- a/drivers/bus/pci/rte_bus_pci.h
> > +++ b/drivers/bus/pci/rte_bus_pci.h
> > @@ -224,6 +224,25 @@ 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
> > + *
> > + *  @param cap
> > + *    Extended capability to find
> 
> Reading the rest of the file, I see some doc with sentences finished by
> periods, and not others. Going forward we should be consistent, and write
> documentation with grammatically correct sentences, so with the periods.
> 
> > + *
> > + *  @return
> > + *  > 0: The offset of the next matching extended capability structure
> > + *       within the device's PCI configuration space
> > + *  < 0: An error in PCI config space read
> > + *  = 0: Device does not support it
> 
> Thanks for the additional details, though the periods are still missing.
> 
> > + */
> > +__rte_experimental
> > +off_t rte_pci_find_next_ext_capability(struct rte_pci_device *dev,
> > +				       uint32_t cap);
> > +
> >  /**
> >   * Register a PCI driver.
> >   *
> > diff --git a/drivers/bus/pci/rte_bus_pci_version.map
> > b/drivers/bus/pci/rte_bus_pci_version.map
> > index 012d817e1..b5322660d 100644
> > --- a/drivers/bus/pci/rte_bus_pci_version.map
> > +++ b/drivers/bus/pci/rte_bus_pci_version.map
> > @@ -16,3 +16,9 @@ DPDK_20.0 {
> >
> >  	local: *;
> >  };
> > +
> > +EXPERIMENTAL {
> > +	global:
> > +
> > +	rte_pci_find_next_ext_capability;
> > +};
> > diff --git a/drivers/net/ice/ice_ethdev.c
> > b/drivers/net/ice/ice_ethdev.c index 7dd3fcd27..6c8cbea5c 100644
> > --- a/drivers/net/ice/ice_ethdev.c
> > +++ b/drivers/net/ice/ice_ethdev.c
> > @@ -1730,53 +1730,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.
> > @@ -1784,12 +1737,12 @@ ice_pci_find_next_ext_capability(struct
> > rte_pci_device *dev, int cap)  static int
> > ice_pkg_file_search_path(struct rte_pci_device *pci_dev, char
> > *pkg_file)  {
> > -	int pos;
> > +	off_t pos;
> >  	char opt_ddp_filename[ICE_MAX_PKG_FILENAME_SIZE];
> >  	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,
> > +RTE_PCI_EXT_CAP_ID_DSN);
> >
> >  	if (pos) {
> >  		rte_pci_read_config(pci_dev, &dsn_low, 4, pos + 4); diff --git
> > a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> > b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> > index 0b9db974e..dbab4f8cb 100644
> > --- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> > +++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> > @@ -746,59 +746,15 @@ 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)
> > {
> >  	uint16_t tmp;
> >  	uint8_t serial[6];
> >  	int serial_len = 6;
> > -	int pos;
> > +	off_t pos;
> >
> > -	pos = nfp_pci_find_next_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
> > +	pos = rte_pci_find_next_ext_capability(dev,
> RTE_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/drivers/raw/ifpga/ifpga_rawdev.c
> > b/drivers/raw/ifpga/ifpga_rawdev.c
> > index cc25c662b..f8205a3c7 100644
> > --- a/drivers/raw/ifpga/ifpga_rawdev.c
> > +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> > @@ -41,12 +41,6 @@
> >  #include "ifpga_rawdev.h"
> >  #include "ipn3ke_rawdev_api.h"
> >
> > -#define RTE_PCI_EXT_CAP_ID_ERR           0x01	/* Advanced Error
> Reporting */
> > -#define RTE_PCI_CFG_SPACE_SIZE           256
> > -#define RTE_PCI_CFG_SPACE_EXP_SIZE       4096
> > -#define RTE_PCI_EXT_CAP_ID(header)       (int)(header & 0x0000ffff)
> > -#define RTE_PCI_EXT_CAP_NEXT(header)     ((header >> 20) & 0xffc)
> > -
> >  #define PCI_VENDOR_ID_INTEL          0x8086
> >  /* PCI Device ID */
> >  #define PCIE_DEVICE_ID_PF_INT_5_X    0xBCBD
> > diff --git a/lib/librte_pci/rte_pci.h b/lib/librte_pci/rte_pci.h index
> > a03235da1..fec51e15a 100644
> > --- a/lib/librte_pci/rte_pci.h
> > +++ b/lib/librte_pci/rte_pci.h
> > @@ -22,6 +22,22 @@ extern "C" {
> >  #include <inttypes.h>
> >  #include <sys/types.h>
> >
> > +
> > +/*
> > + * Conventional PCI and PCI-X Mode 1 devices have 256 bytes of
> > + * configuration space.  PCI-X Mode 2 and PCIe devices have 4096
> > +bytes of
> > + * configuration space.
> > + */
> > +#define RTE_PCI_CFG_SPACE_SIZE		256
> > +#define RTE_PCI_CFG_SPACE_EXP_SIZE	4096
> > +
> > +/* Extended Capabilities (PCI-X 2.0 and Express) */
> > +#define RTE_PCI_EXT_CAP_ID(header)	(header & 0x0000ffff)
> > +#define RTE_PCI_EXT_CAP_NEXT(header)	((header >> 20) & 0xffc)
> > +
> > +#define RTE_PCI_EXT_CAP_ID_ERR	0x01	/* Advanced Error Reporting
> */
> > +#define RTE_PCI_EXT_CAP_ID_DSN	0x03	/* Device Serial Number */
> > +
> 
> I understand that it is more natural to have those defines in the PCI lib, but I
> think there is no point in adding them in a separate lib, while the function
> using those is in the PCI bus.
> 
> I think the defines should be put right before the
> rte_pci_find_next_ext_capability() prototype in
> drivers/bus/pci/rte_bus_pci.h.

Hello Gaetan,

I think these comes in the category of all RTE_PCI_* generic defines (not just use in drivers/bus/pci/),
Since caller of the API also need to use it, For example, couple of RTE_PCI_* were added in patch #2
used by qede drivers specifically. rte_pci.h sounds more generic than rte_bus_pci.h hence I thought it
is the suitable place to consolidate them in there.

Thanks !!