> -----Original Message-----
> From: Gaëtan Rivet <gr...@u256.net>
> Sent: Saturday, July 25, 2020 11:02 PM
> To: Manish Chopra <mani...@marvell.com>
> Cc: jerinjac...@gmail.com; Jerin Jacob Kollanukkaran <jer...@marvell.com>;
> ferruh.yi...@intel.com; dev@dpdk.org; Igor Russkikh
> <irussk...@marvell.com>; Rasesh Mody <rm...@marvell.com>; GR-Everest-
> DPDK-Dev <gr-everest-dpdk-...@marvell.com>; rosen...@intel.com;
> tianfei.zh...@intel.com; heinrich.k...@netronome.com;
> qiming.y...@intel.com; qi.z.zh...@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 <mani...@marvell.com>
> > Signed-off-by: Igor Russkikh <irussk...@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 !!
