[v3,1/6] drivers: add generic API to find PCI extended cap
Checks
Commit Message
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 | 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(-)
Comments
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.
> /** Formatting string for PCI device identifier: Ex: 0000:00:01.0 */
> #define PCI_PRI_FMT "%.4" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
> #define PCI_PRI_STR_SIZE sizeof("XXXXXXXX:XX:XX.X")
> --
> 2.17.1
>
> -----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 !!
On 26/07/20 19:47 +0000, Manish Chopra wrote:
[...]
> > > 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 !!
Reading the additional symbols, particularly about SRIOV capa,
I think you are right, it's probably better to have it all within
rte_pci.h.
To help developers, it would be better to point in the doc that the capability
IDs useable as parameter `cap` can be any from RTE_PCI_EXT_CAP_ID_*,
defined within librte_pci. The dev can then grep it.
One additional thing:
> > > +#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)
I think those macros are not useful as part of the public API, they are
only used to implement rte_pci_find_next_ext_capability(). Can you
confirm? If this is correct, I think they should be moved to the
compilation unit implementing rte_pci_find_next_ext_capability().
Regards,
> -----Original Message-----
> From: Gaëtan Rivet <grive@u256.net>
> Sent: Monday, July 27, 2020 4:18 AM
> 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: Re: [EXT] Re: [PATCH v3 1/6] drivers: add generic API to find PCI
> extended cap
>
> On 26/07/20 19:47 +0000, Manish Chopra wrote:
>
> [...]
>
> > > > 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 !!
>
> Reading the additional symbols, particularly about SRIOV capa, I think you
> are right, it's probably better to have it all within rte_pci.h.
>
> To help developers, it would be better to point in the doc that the capability
> IDs useable as parameter `cap` can be any from RTE_PCI_EXT_CAP_ID_*,
> defined within librte_pci. The dev can then grep it.
Sure, I will add the pointer to librte_pci in the comment section for
rte_pci_find_next_ext_capability()
>
> One additional thing:
>
> > > > +#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)
>
> I think those macros are not useful as part of the public API, they are only
> used to implement rte_pci_find_next_ext_capability(). Can you confirm? If
> this is correct, I think they should be moved to the compilation unit
> implementing rte_pci_find_next_ext_capability().
>
Hi Gaetan,
Yes Mostly, but there is a similar piece of code left in drivers/raw/ifpga [ifpga_pci_find_ext_capability()] only
which utilizes these symbols as well, which I did not want to be removed/cleaned up must as a part of this
series because that implementation is based on pread() instead of rte_pci_read_config(). I was not sure
if that driver can also directly use rte_pci_find_next_ext_capability() too, I do not have those fpga based devices
to test if at all I were to do that cleanup/removal now in that driver, so I didn't attempt to make such functional
changes in that driver now. [May be this can be cleaned up too later on with proper testing or may be a new API
based on pread() can be added further if those drivers can't use rte_pci_find_next_ext_capability() directly].
Thanks.
On 27/07/20 05:10 +0000, Manish Chopra wrote:
> > -----Original Message-----
> > From: Gaëtan Rivet <grive@u256.net>
> > Sent: Monday, July 27, 2020 4:18 AM
> > 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: Re: [EXT] Re: [PATCH v3 1/6] drivers: add generic API to find PCI
> > extended cap
> >
> > On 26/07/20 19:47 +0000, Manish Chopra wrote:
> >
> > [...]
> >
> > > > > 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 !!
> >
> > Reading the additional symbols, particularly about SRIOV capa, I think you
> > are right, it's probably better to have it all within rte_pci.h.
> >
> > To help developers, it would be better to point in the doc that the capability
> > IDs useable as parameter `cap` can be any from RTE_PCI_EXT_CAP_ID_*,
> > defined within librte_pci. The dev can then grep it.
>
> Sure, I will add the pointer to librte_pci in the comment section for
> rte_pci_find_next_ext_capability()
>
> >
> > One additional thing:
> >
> > > > > +#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)
> >
> > I think those macros are not useful as part of the public API, they are only
> > used to implement rte_pci_find_next_ext_capability(). Can you confirm? If
> > this is correct, I think they should be moved to the compilation unit
> > implementing rte_pci_find_next_ext_capability().
> >
>
> Hi Gaetan,
>
> Yes Mostly, but there is a similar piece of code left in drivers/raw/ifpga [ifpga_pci_find_ext_capability()] only
> which utilizes these symbols as well, which I did not want to be removed/cleaned up must as a part of this
> series because that implementation is based on pread() instead of rte_pci_read_config(). I was not sure
> if that driver can also directly use rte_pci_find_next_ext_capability() too, I do not have those fpga based devices
> to test if at all I were to do that cleanup/removal now in that driver, so I didn't attempt to make such functional
> changes in that driver now. [May be this can be cleaned up too later on with proper testing or may be a new API
> based on pread() can be added further if those drivers can't use rte_pci_find_next_ext_capability() directly].
>
> Thanks.
Ok, as it is used elsewhere my suggestion does not stand, thanks for
clarifying.
Hi,
> -----Original Message-----
> From: Manish Chopra <manishc@marvell.com>
> Sent: Monday, July 27, 2020 13:11
> To: Gaëtan Rivet <grive@u256.net>
> Cc: jerinjacobk@gmail.com; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Yigit, Ferruh <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>; Xu, Rosen
> <rosen.xu@intel.com>; Zhang, Tianfei <tianfei.zhang@intel.com>;
> heinrich.kuhn@netronome.com; Yang, Qiming <qiming.yang@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: RE: [EXT] Re: [PATCH v3 1/6] drivers: add generic API to find PCI
> extended cap
>
> > -----Original Message-----
> > From: Gaëtan Rivet <grive@u256.net>
> > Sent: Monday, July 27, 2020 4:18 AM
> > 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: Re: [EXT] Re: [PATCH v3 1/6] drivers: add generic API to find
> > PCI extended cap
> >
> > On 26/07/20 19:47 +0000, Manish Chopra wrote:
> >
> > [...]
> >
> > > > > 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 !!
> >
> > Reading the additional symbols, particularly about SRIOV capa, I think
> > you are right, it's probably better to have it all within rte_pci.h.
> >
> > To help developers, it would be better to point in the doc that the
> > capability IDs useable as parameter `cap` can be any from
> > RTE_PCI_EXT_CAP_ID_*, defined within librte_pci. The dev can then grep it.
>
> Sure, I will add the pointer to librte_pci in the comment section for
> rte_pci_find_next_ext_capability()
>
> >
> > One additional thing:
> >
> > > > > +#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)
> >
> > I think those macros are not useful as part of the public API, they
> > are only used to implement rte_pci_find_next_ext_capability(). Can you
> > confirm? If this is correct, I think they should be moved to the
> > compilation unit implementing rte_pci_find_next_ext_capability().
> >
>
> Hi Gaetan,
>
> Yes Mostly, but there is a similar piece of code left in drivers/raw/ifpga
> [ifpga_pci_find_ext_capability()] only which utilizes these symbols as well,
> which I did not want to be removed/cleaned up must as a part of this series
> because that implementation is based on pread() instead of
> rte_pci_read_config(). I was not sure if that driver can also directly use
> rte_pci_find_next_ext_capability() too, I do not have those fpga based
> devices to test if at all I were to do that cleanup/removal now in that driver,
> so I didn't attempt to make such functional changes in that driver now. [May
> be this can be cleaned up too later on with proper testing or may be a new
> API based on pread() can be added further if those drivers can't use
> rte_pci_find_next_ext_capability() directly].
For drivers/raw/ifpga is using these macros, I prefer to keep it.
> Thanks.
@@ -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)
+{
+ 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,
@@ -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
+ *
+ * @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
+ */
+__rte_experimental
+off_t rte_pci_find_next_ext_capability(struct rte_pci_device *dev,
+ uint32_t cap);
+
/**
* Register a PCI driver.
*
@@ -16,3 +16,9 @@ DPDK_20.0 {
local: *;
};
+
+EXPERIMENTAL {
+ global:
+
+ rte_pci_find_next_ext_capability;
+};
@@ -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);
@@ -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;
@@ -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
@@ -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 */
+
/** Formatting string for PCI device identifier: Ex: 0000:00:01.0 */
#define PCI_PRI_FMT "%.4" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
#define PCI_PRI_STR_SIZE sizeof("XXXXXXXX:XX:XX.X")