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 D7EA9A0540; Mon, 20 Jul 2020 11:36:43 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 348072B86; Mon, 20 Jul 2020 11:36:42 +0200 (CEST) Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net [217.70.183.198]) by dpdk.org (Postfix) with ESMTP id 5FA2F29CB for ; Mon, 20 Jul 2020 11:36:40 +0200 (CEST) X-Originating-IP: 90.92.205.40 Received: from u256.net (lfbn-idf2-1-1144-40.w90-92.abo.wanadoo.fr [90.92.205.40]) (Authenticated sender: grive@u256.net) by relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 7C3E9C0012; Mon, 20 Jul 2020 09:36:39 +0000 (UTC) Date: Mon, 20 Jul 2020 11:36:34 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Manish Chopra Cc: jerinj@marvell.com, ferruh.yigit@intel.com, dev@dpdk.org Message-ID: <20200720093634.wdpvteicbtqiowik@u256.net> References: <20200713151319.17547-1-manishc@marvell.com> <20200713151319.17547-3-manishc@marvell.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200713151319.17547-3-manishc@marvell.com> Subject: Re: [dpdk-dev] [PATCH v2 2/7] 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" 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 > Signed-off-by: Igor Russkikh > --- > 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 > #include > #include > +#include > > #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 > #include > +#include > +#include > > #include > #include > @@ -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 > #include > +#include > +#include > > #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