From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E75D8A0526; Sat, 25 Jul 2020 19:32:34 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 465731C02D; Sat, 25 Jul 2020 19:32:34 +0200 (CEST) Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [217.70.178.232]) by dpdk.org (Postfix) with ESMTP id C30C01BFE3 for ; Sat, 25 Jul 2020 19:32:32 +0200 (CEST) Received: from u256.net (lfbn-idf2-1-1144-40.w90-92.abo.wanadoo.fr [90.92.205.40]) (Authenticated sender: grive@u256.net) by relay12.mail.gandi.net (Postfix) with ESMTPSA id 9B7F1200007; Sat, 25 Jul 2020 17:32:28 +0000 (UTC) Date: Sat, 25 Jul 2020 19:32:24 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Manish Chopra Cc: jerinjacobk@gmail.com, jerinj@marvell.com, ferruh.yigit@intel.com, dev@dpdk.org, irusskikh@marvell.com, rmody@marvell.com, 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 Message-ID: <20200725173224.okzt3owicgdjun5s@u256.net> References: <20200724103846.12640-1-manishc@marvell.com> <20200724103846.12640-2-manishc@marvell.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200724103846.12640-2-manishc@marvell.com> Subject: Re: [dpdk-dev] [PATCH v3 1/6] drivers: add generic API to find PCI extended cap X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > Signed-off-by: Igor Russkikh > --- > 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 > #include > > + > +/* > + * 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 > -- Gaëtan