| Message ID | 1446217733-9887-3-git-send-email-bernard.iremonger@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 1A221936E; Fri, 30 Oct 2015 16:09:12 +0100 (CET) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id E4BFA920C for <dev@dpdk.org>; Fri, 30 Oct 2015 16:09:08 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP; 30 Oct 2015 08:09:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,219,1444719600"; d="scan'208";a="807550823" Received: from irvmail001.ir.intel.com ([163.33.26.43]) by orsmga001.jf.intel.com with ESMTP; 30 Oct 2015 08:09:00 -0700 Received: from sivswdev01.ir.intel.com (sivswdev01.ir.intel.com [10.237.217.45]) by irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id t9UF909v009023; Fri, 30 Oct 2015 15:09:00 GMT Received: from sivswdev01.ir.intel.com (localhost [127.0.0.1]) by sivswdev01.ir.intel.com with ESMTP id t9UF8xMf009950; Fri, 30 Oct 2015 15:08:59 GMT Received: (from bairemon@localhost) by sivswdev01.ir.intel.com with id t9UF8xm0009946; Fri, 30 Oct 2015 15:08:59 GMT From: Bernard Iremonger <bernard.iremonger@intel.com> To: dev@dpdk.org Date: Fri, 30 Oct 2015 15:08:27 +0000 Message-Id: <1446217733-9887-3-git-send-email-bernard.iremonger@intel.com> X-Mailer: git-send-email 1.7.4.1 In-Reply-To: <1446217733-9887-1-git-send-email-bernard.iremonger@intel.com> References: <PATCH v7> <1446217733-9887-1-git-send-email-bernard.iremonger@intel.com> Subject: [dpdk-dev] [PATCH v7 02/28] librte_ether: add fields from rte_pci_driver to rte_eth_dev_data X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Iremonger, Bernard
Oct. 30, 2015, 3:08 p.m. UTC
add dev_flags to rte_eth_dev_data, add macros for dev_flags. add kdrv to rte_eth_dev_data. add numa_node to rte_eth_dev_data. add drv_name to rte_eth_dev_data. Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com> Acked-by: Bruce Richardson <bruce.richardson@intel.com> --- lib/librte_ether/rte_ethdev.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
Comments
2015-10-30 15:08, Bernard Iremonger: > add dev_flags to rte_eth_dev_data, add macros for dev_flags. > add kdrv to rte_eth_dev_data. > add numa_node to rte_eth_dev_data. > add drv_name to rte_eth_dev_data. A commit message should explain why things are done. > + uint32_t dev_flags; /**< Flags controlling handling of device. */ Where are defined this flags? What is the scope?
2015-10-30 15:08, Bernard Iremonger: > +/** Device needs PCI BAR mapping (done with either IGB_UIO or VFIO) */ > +#define RTE_ETH_DEV_DRV_NEED_MAPPING RTE_PCI_DRV_NEED_MAPPING > +/** Device needs to be unbound even if no module is provided */ > +#define RTE_ETH_DEV_DRV_FORCE_UNBIND RTE_PCI_DRV_FORCE_UNBIND > +/** Device supports link state interrupt */ > +#define RTE_ETH_DEV_INTR_LSC RTE_PCI_DRV_INTR_LSC > +/** Device supports detaching capability */ > +#define RTE_ETH_DEV_DETACHABLE RTE_PCI_DRV_DETACHABLE Please, use an enum which has a name and can be referenced in the API, e.g. the variable X contains some X_flags. You should not try to re-use the same values as the PCI layer since it will not be possible to map it forever when new buses will enter in the game. > +/** Device is a bonded device */ > +#define RTE_ETH_DEV_BONDED 0x0020 Why not having RTE_ETH_DEV_PCAPED? ;) Please try to remove this flag.
Hi Thomas, <snip> > Subject: Re: [dpdk-dev] [PATCH v7 02/28] librte_ether: add fields from > rte_pci_driver to rte_eth_dev_data > > 2015-10-30 15:08, Bernard Iremonger: > > +/** Device needs PCI BAR mapping (done with either IGB_UIO or VFIO) > */ > > +#define RTE_ETH_DEV_DRV_NEED_MAPPING > RTE_PCI_DRV_NEED_MAPPING > > +/** Device needs to be unbound even if no module is provided */ > > +#define RTE_ETH_DEV_DRV_FORCE_UNBIND > RTE_PCI_DRV_FORCE_UNBIND > > +/** Device supports link state interrupt */ > > +#define RTE_ETH_DEV_INTR_LSC RTE_PCI_DRV_INTR_LSC > > +/** Device supports detaching capability */ #define > > +RTE_ETH_DEV_DETACHABLE RTE_PCI_DRV_DETACHABLE > > Please, use an enum which has a name and can be referenced in the API, e.g. > the variable X contains some X_flags. > You should not try to re-use the same values as the PCI layer since it will not > be possible to map it forever when new buses will enter in the game. I will use an enum instead. > > +/** Device is a bonded device */ > > +#define RTE_ETH_DEV_BONDED 0x0020 > > Why not having RTE_ETH_DEV_PCAPED? ;) > Please try to remove this flag. I will remove this flag. Regards, Bernard.
Hi Thomas, > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Sunday, November 1, 2015 9:12 PM > To: Iremonger, Bernard <bernard.iremonger@intel.com> > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v7 02/28] librte_ether: add fields from > rte_pci_driver to rte_eth_dev_data > > 2015-10-30 15:08, Bernard Iremonger: > > add dev_flags to rte_eth_dev_data, add macros for dev_flags. > > add kdrv to rte_eth_dev_data. > > add numa_node to rte_eth_dev_data. > > add drv_name to rte_eth_dev_data. > > A commit message should explain why things are done. > > > + uint32_t dev_flags; /**< Flags controlling handling of device. */ > > Where are defined this flags? What is the scope? These flags are defined in the following file: lib/librte_ether/rte_ethdev.h These flags are visible to all the vdevs and pdevs. Regards, Bernard.
2015-11-02 10:36, Iremonger, Bernard: > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > > 2015-10-30 15:08, Bernard Iremonger: > > > add dev_flags to rte_eth_dev_data, add macros for dev_flags. > > > add kdrv to rte_eth_dev_data. > > > add numa_node to rte_eth_dev_data. > > > add drv_name to rte_eth_dev_data. > > > > A commit message should explain why things are done. > > > > > + uint32_t dev_flags; /**< Flags controlling handling of device. */ > > > > Where are defined this flags? What is the scope? > > These flags are defined in the following file: > > lib/librte_ether/rte_ethdev.h > > These flags are visible to all the vdevs and pdevs. I mean it should be more explicit. Having an enum name will help. Note: I understand your patch. I'm just asking the questions an user will ask when trying to use your new API. Thanks
Hi Thomas, <snip> > Subject: Re: [dpdk-dev] [PATCH v7 02/28] librte_ether: add fields from > rte_pci_driver to rte_eth_dev_data > > 2015-11-02 10:36, Iremonger, Bernard: > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > > > 2015-10-30 15:08, Bernard Iremonger: > > > > add dev_flags to rte_eth_dev_data, add macros for dev_flags. > > > > add kdrv to rte_eth_dev_data. > > > > add numa_node to rte_eth_dev_data. > > > > add drv_name to rte_eth_dev_data. > > > > > > A commit message should explain why things are done. > > > > > > > + uint32_t dev_flags; /**< Flags controlling handling of device. > > > > +*/ > > > > > > Where are defined this flags? What is the scope? > > > > These flags are defined in the following file: > > > > lib/librte_ether/rte_ethdev.h > > > > These flags are visible to all the vdevs and pdevs. > > I mean it should be more explicit. Having an enum name will help. > Note: I understand your patch. I'm just asking the questions an user will ask > when trying to use your new API. > Thanks I will try to be clearer in the commit message. I tried an enum for the dev_flags but it does not work well. There can be multiple flags set in dev_flags, it is intended to be a bit field similar to the pci flags. I have squashed down to 19 patches now, I don't want to over squash. Regards, Bernard.
2015-11-02 16:44, Iremonger, Bernard: > Hi Thomas, > > <snip> > > > Subject: Re: [dpdk-dev] [PATCH v7 02/28] librte_ether: add fields from > > rte_pci_driver to rte_eth_dev_data > > > > 2015-11-02 10:36, Iremonger, Bernard: > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > > > > 2015-10-30 15:08, Bernard Iremonger: > > > > > add dev_flags to rte_eth_dev_data, add macros for dev_flags. > > > > > add kdrv to rte_eth_dev_data. > > > > > add numa_node to rte_eth_dev_data. > > > > > add drv_name to rte_eth_dev_data. > > > > > > > > A commit message should explain why things are done. > > > > > > > > > + uint32_t dev_flags; /**< Flags controlling handling of device. > > > > > +*/ > > > > > > > > Where are defined this flags? What is the scope? > > > > > > These flags are defined in the following file: > > > > > > lib/librte_ether/rte_ethdev.h > > > > > > These flags are visible to all the vdevs and pdevs. > > > > I mean it should be more explicit. Having an enum name will help. > > Note: I understand your patch. I'm just asking the questions an user will ask > > when trying to use your new API. > > Thanks > > I will try to be clearer in the commit message. > I tried an enum for the dev_flags but it does not work well. > There can be multiple flags set in dev_flags, it is intended to be a bit field similar to the pci flags. Yes. You can assign some bits in an enum. But if you prefer the defines, it's also OK if they are clearly identifiable: with a prefix and good comments allowing to map the structure field and the flags. > I have squashed down to 19 patches now, I don't want to over squash. The proposal of 5 patches was good.
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index 8a8c82b..1517fe2 100644 --- a/lib/librte_ether/rte_ethdev.h +++ b/lib/librte_ether/rte_ethdev.h @@ -1471,8 +1471,23 @@ struct rte_eth_dev_data { all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */ dev_started : 1, /**< Device state: STARTED(1) / STOPPED(0). */ lro : 1; /**< RX LRO is ON(1) / OFF(0) */ + uint32_t dev_flags; /**< Flags controlling handling of device. */ + enum rte_kernel_driver kdrv; /**< Kernel driver passthrough */ + int numa_node; /**< NUMA node connection */ + const char *drv_name; /**< Driver name */ }; +/** Device needs PCI BAR mapping (done with either IGB_UIO or VFIO) */ +#define RTE_ETH_DEV_DRV_NEED_MAPPING RTE_PCI_DRV_NEED_MAPPING +/** Device needs to be unbound even if no module is provided */ +#define RTE_ETH_DEV_DRV_FORCE_UNBIND RTE_PCI_DRV_FORCE_UNBIND +/** Device supports link state interrupt */ +#define RTE_ETH_DEV_INTR_LSC RTE_PCI_DRV_INTR_LSC +/** Device supports detaching capability */ +#define RTE_ETH_DEV_DETACHABLE RTE_PCI_DRV_DETACHABLE +/** Device is a bonded device */ +#define RTE_ETH_DEV_BONDED 0x0020 + /** * @internal * The pool of *rte_eth_dev* structures. The size of the pool