From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by dpdk.org (Postfix) with ESMTP id 80FB1FB4B for ; Tue, 20 Dec 2016 12:20:11 +0100 (CET) Received: by mail-wm0-f67.google.com with SMTP id g23so23753809wme.1 for ; Tue, 20 Dec 2016 03:20:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=9opNJYEI1JuKbe80ed1emzp5vPMb9IMbWuaDyifNbTU=; b=piLF3NLz9+N8Qc8UXMcoKqP6zs7XQBGRSvZJDby35ZpaXO/rTG46SvJhUHwRaVWElI O11Xaqkb/pEBS+CgfrvKJ42FLP0Q1wyfVZpTWulRfPvOIPXtAEAbISscabnInTDUMYwz NO4KHgAMy6GhmbsZD8eP2MlCbaeNEw2cYIi2NSiZaLUYy0+WImRkgrpfL6HIQ9y6XQsY t9CHf7hJTDknw0VjOzWGjZMvAhnvGRD7zSa6jYAHVzubyRLcJ5HtRnhs5znUGe90zi9X ROIWoEvPBoZBadWQ9XVorrb3/psWS6AFlTVtxdMV4vRe5YqhGS0mqvViZRLnw5Cg28OT 3djQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=9opNJYEI1JuKbe80ed1emzp5vPMb9IMbWuaDyifNbTU=; b=AS6Y4ChzPpuvBs/YRSzhKIVCBNtJ04NK7+3pBXrGBBAG+5/rqRP7REioef1ubm2QR+ idXC8SKZ3dnf1BKVuwHSv/xEMyNRsEGWb/pInzuJXhKim0qUvaHTG2P5nGD8dfNAz/lI lJzvxBGiEC8Z4olbugwZckjVt1kNupL/BPixoF2GZCdJOWU/wSM30/m4bKxbA4j44TTg /elJOwL2jfgRRB4JlnsdbjJJjH3/i4EaAtOwsGQG1sZWjFpTQSiUM/AFck63gvr8AtFE ld7Un9t5OXq2ORxspxoTdkUGKyLnTy2egzrTPTegvdZ8r7ZdiKFwgqfgPcJ311rnGKq4 Qzfg== X-Gm-Message-State: AIkVDXJKez3HdFclLuWmM2kMXDm+tqTnshznFQyxp2aFXFRz2rTQkhFVLWRL/JUR07hsHVMXfIy6sB8ABqzSMg== X-Received: by 10.28.66.194 with SMTP id k63mr1532277wmi.140.1482232810885; Tue, 20 Dec 2016 03:20:10 -0800 (PST) MIME-Version: 1.0 Sender: jblunck@gmail.com Received: by 10.28.63.83 with HTTP; Tue, 20 Dec 2016 03:20:10 -0800 (PST) In-Reply-To: <20161219215944.17226-7-sthemmin@microsoft.com> References: <20161219215944.17226-1-sthemmin@microsoft.com> <20161219215944.17226-7-sthemmin@microsoft.com> From: Jan Blunck Date: Tue, 20 Dec 2016 12:20:10 +0100 X-Google-Sender-Auth: _nnmdbfH_4j-I0wzbrYnq0azv0Y Message-ID: To: Stephen Hemminger Cc: dev@dpdk.org, Stephen Hemminger Content-Type: text/plain; charset=UTF-8 Subject: Re: [dpdk-dev] [PATCH 06/13] ethdev: make dev_info generic (not just PCI) 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: , X-List-Received-Date: Tue, 20 Dec 2016 11:20:11 -0000 On Mon, Dec 19, 2016 at 10:59 PM, Stephen Hemminger wrote: > The result from rte_eth_dev_info_get should have pointer to > device not PCI device. This breaks ABI but is necessary. > > Signed-off-by: Stephen Hemminger > --- > app/test-pmd/config.c | 32 ++++++++++++++++++++++++++-- > app/test-pmd/testpmd.c | 11 ++++++++-- > app/test-pmd/testpmd.h | 32 ++++++++++++++++------------ > app/test/test_kni.c | 39 ++++++++++++++++++++++++++++------ > doc/guides/rel_notes/release_17_02.rst | 10 +++------ > lib/librte_ether/rte_ethdev.c | 3 ++- > lib/librte_ether/rte_ethdev.h | 2 +- > 7 files changed, 96 insertions(+), 33 deletions(-) > > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c > index 8cf537d5..1d0974ad 100644 > --- a/app/test-pmd/config.c > +++ b/app/test-pmd/config.c > @@ -553,6 +553,16 @@ port_id_is_invalid(portid_t port_id, enum print_warning warning) > return 1; > } > > +int > +port_is_not_pci(portid_t port_id) > +{ > + if (ports[port_id].pci_dev) > + return 0; > + > + printf("Port %u is not a PCI device\n", port_id); > + return 1; > +} > + > static int > vlan_id_is_invalid(uint16_t vlan_id) > { > @@ -565,15 +575,22 @@ vlan_id_is_invalid(uint16_t vlan_id) > static int > port_reg_off_is_invalid(portid_t port_id, uint32_t reg_off) > { > + struct rte_pci_device *pci_dev = ports[port_id].pci_dev; > uint64_t pci_len; > > + if (pci_dev == NULL) { > + printf("Port %u is not a PCI device\n", port_id); > + return 1; > + } > + > if (reg_off & 0x3) { > printf("Port register offset 0x%X not aligned on a 4-byte " > "boundary\n", > (unsigned)reg_off); > return 1; > } > - pci_len = ports[port_id].dev_info.pci_dev->mem_resource[0].len; > + > + pci_len = pci_dev->mem_resource[0].len; > if (reg_off >= pci_len) { > printf("Port %d: register offset %u (0x%X) out of port PCI " > "resource (length=%"PRIu64")\n", > @@ -607,9 +624,10 @@ port_reg_bit_display(portid_t port_id, uint32_t reg_off, uint8_t bit_x) > { > uint32_t reg_v; > > - > if (port_id_is_invalid(port_id, ENABLED_WARN)) > return; > + if (port_is_not_pci(port_id)) > + return; > if (port_reg_off_is_invalid(port_id, reg_off)) > return; > if (reg_bit_pos_is_invalid(bit_x)) > @@ -629,6 +647,8 @@ port_reg_bit_field_display(portid_t port_id, uint32_t reg_off, > > if (port_id_is_invalid(port_id, ENABLED_WARN)) > return; > + if (port_is_not_pci(port_id)) > + return; > if (port_reg_off_is_invalid(port_id, reg_off)) > return; > if (reg_bit_pos_is_invalid(bit1_pos)) > @@ -658,6 +678,8 @@ port_reg_display(portid_t port_id, uint32_t reg_off) > return; > if (port_reg_off_is_invalid(port_id, reg_off)) > return; > + if (port_is_not_pci(port_id)) > + return; > reg_v = port_id_pci_reg_read(port_id, reg_off); > display_port_reg_value(port_id, reg_off, reg_v); > } > @@ -670,6 +692,8 @@ port_reg_bit_set(portid_t port_id, uint32_t reg_off, uint8_t bit_pos, > > if (port_id_is_invalid(port_id, ENABLED_WARN)) > return; > + if (port_is_not_pci(port_id)) > + return; > if (port_reg_off_is_invalid(port_id, reg_off)) > return; > if (reg_bit_pos_is_invalid(bit_pos)) > @@ -698,6 +722,8 @@ port_reg_bit_field_set(portid_t port_id, uint32_t reg_off, > > if (port_id_is_invalid(port_id, ENABLED_WARN)) > return; > + if (port_is_not_pci(port_id)) > + return; > if (port_reg_off_is_invalid(port_id, reg_off)) > return; > if (reg_bit_pos_is_invalid(bit1_pos)) > @@ -732,6 +758,8 @@ port_reg_set(portid_t port_id, uint32_t reg_off, uint32_t reg_v) > { > if (port_id_is_invalid(port_id, ENABLED_WARN)) > return; > + if (port_is_not_pci(port_id)) > + return; > if (port_reg_off_is_invalid(port_id, reg_off)) > return; > port_id_pci_reg_write(port_id, reg_off, reg_v); > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c > index a0332c26..faf1e16d 100644 > --- a/app/test-pmd/testpmd.c > +++ b/app/test-pmd/testpmd.c > @@ -492,7 +492,6 @@ static void > init_config(void) > { > portid_t pid; > - struct rte_port *port; > struct rte_mempool *mbp; > unsigned int nb_mbuf_per_pool; > lcoreid_t lc_id; > @@ -547,9 +546,17 @@ init_config(void) > } > > FOREACH_PORT(pid, ports) { > - port = &ports[pid]; > + struct rte_port *port = &ports[pid]; > + struct rte_device *dev; > + > rte_eth_dev_info_get(pid, &port->dev_info); > > + dev = port->dev_info.device; > + if (dev->driver->type == PMD_PCI) > + port->pci_dev = container_of(dev, struct rte_pci_device, device); > + else > + port->pci_dev = NULL; > + > if (numa_support) { > if (port_numa[pid] != NUMA_NO_CONFIG) > port_per_socket[port_numa[pid]]++; > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h > index 9c1e7039..e8aca32a 100644 > --- a/app/test-pmd/testpmd.h > +++ b/app/test-pmd/testpmd.h > @@ -149,6 +149,7 @@ struct fwd_stream { > */ > struct rte_port { > uint8_t enabled; /**< Port enabled or not */ > + struct rte_pci_device *pci_dev; > struct rte_eth_dev_info dev_info; /**< PCI info + driver name */ > struct rte_eth_conf dev_conf; /**< Port configuration. */ > struct ether_addr eth_addr; /**< Port ethernet address */ > @@ -442,34 +443,36 @@ mbuf_pool_find(unsigned int sock_id) > * Read/Write operations on a PCI register of a port. > */ > static inline uint32_t > -port_pci_reg_read(struct rte_port *port, uint32_t reg_off) > +pci_reg_read(struct rte_pci_device *pci_dev, uint32_t reg_off) > { > - void *reg_addr; > + void *reg_addr > + = (char *)pci_dev->mem_resource[0].addr + reg_off; > uint32_t reg_v; > > - reg_addr = (void *) > - ((char *)port->dev_info.pci_dev->mem_resource[0].addr + > - reg_off); > reg_v = *((volatile uint32_t *)reg_addr); > return rte_le_to_cpu_32(reg_v); > } > > -#define port_id_pci_reg_read(pt_id, reg_off) \ > - port_pci_reg_read(&ports[(pt_id)], (reg_off)) > +static inline uint32_t > +port_id_pci_reg_read(portid_t pt_id, uint32_t reg_off) > +{ > + return pci_reg_read(ports[pt_id].pci_dev, reg_off); > +} > > static inline void > -port_pci_reg_write(struct rte_port *port, uint32_t reg_off, uint32_t reg_v) > +pci_reg_write(struct rte_pci_device *pci_dev, uint32_t reg_off, uint32_t reg_v) > { > - void *reg_addr; > + void *reg_addr > + = (char *)pci_dev->mem_resource[0].addr + reg_off; > > - reg_addr = (void *) > - ((char *)port->dev_info.pci_dev->mem_resource[0].addr + > - reg_off); > *((volatile uint32_t *)reg_addr) = rte_cpu_to_le_32(reg_v); > } > > -#define port_id_pci_reg_write(pt_id, reg_off, reg_value) \ > - port_pci_reg_write(&ports[(pt_id)], (reg_off), (reg_value)) > +static inline void > +port_id_pci_reg_write(portid_t pt_id, uint32_t reg_off, uint32_t reg_v) > +{ > + return pci_reg_write(ports[pt_id].pci_dev, reg_off, reg_v); > +} > > /* Prototypes */ > unsigned int parse_item_list(char* str, const char* item_name, > @@ -598,6 +601,7 @@ enum print_warning { > ENABLED_WARN = 0, > DISABLED_WARN > }; > +int port_is_not_pci(portid_t port_id); > int port_id_is_invalid(portid_t port_id, enum print_warning warning); > > /* > diff --git a/app/test/test_kni.c b/app/test/test_kni.c > index 309741cb..6b2ebbed 100644 > --- a/app/test/test_kni.c > +++ b/app/test/test_kni.c > @@ -370,6 +370,8 @@ test_kni_processing(uint8_t port_id, struct rte_mempool *mp) > struct rte_kni_conf conf; > struct rte_eth_dev_info info; > struct rte_kni_ops ops; > + struct rte_device *dev; > + struct rte_pci_device *pci_dev; > > if (!mp) > return -1; > @@ -379,8 +381,16 @@ test_kni_processing(uint8_t port_id, struct rte_mempool *mp) > memset(&ops, 0, sizeof(ops)); > > rte_eth_dev_info_get(port_id, &info); > - conf.addr = info.pci_dev->addr; > - conf.id = info.pci_dev->id; > + > + dev = info.device; > + if (dev->driver->type != PMD_PCI) { > + printf("device is not PCI\n"); > + return -1; > + } > + > + pci_dev = container_of(dev, struct rte_pci_device, device); > + conf.addr = pci_dev->addr; > + conf.id = pci_dev->id; > snprintf(conf.name, sizeof(conf.name), TEST_KNI_PORT); > > /* core id 1 configured for kernel thread */ > @@ -478,6 +488,8 @@ test_kni(void) > struct rte_kni_conf conf; > struct rte_eth_dev_info info; > struct rte_kni_ops ops; > + struct rte_device *dev; > + struct rte_pci_device *pci_dev; > > /* Initialize KNI subsytem */ > rte_kni_init(KNI_TEST_MAX_PORTS); > @@ -536,8 +548,16 @@ test_kni(void) > memset(&conf, 0, sizeof(conf)); > memset(&ops, 0, sizeof(ops)); > rte_eth_dev_info_get(port_id, &info); > - conf.addr = info.pci_dev->addr; > - conf.id = info.pci_dev->id; > + > + dev = info.device; > + if (dev->driver->type != PMD_PCI) { > + printf("device is not PCI\n"); > + return -1; > + } > + > + pci_dev = container_of(dev, struct rte_pci_device, device); > + conf.addr = pci_dev->addr; > + conf.id = pci_dev->id; > conf.group_id = (uint16_t)port_id; > conf.mbuf_size = MAX_PACKET_SZ; > > @@ -565,8 +585,15 @@ test_kni(void) > memset(&info, 0, sizeof(info)); > memset(&ops, 0, sizeof(ops)); > rte_eth_dev_info_get(port_id, &info); > - conf.addr = info.pci_dev->addr; > - conf.id = info.pci_dev->id; > + dev = info.device; > + if (dev->driver->type != PMD_PCI) { > + printf("device is not PCI\n"); > + return -1; > + } > + > + pci_dev = container_of(dev, struct rte_pci_device, device); > + conf.addr = pci_dev->addr; > + conf.id = pci_dev->id; > conf.group_id = (uint16_t)port_id; > conf.mbuf_size = MAX_PACKET_SZ; > > diff --git a/doc/guides/rel_notes/release_17_02.rst b/doc/guides/rel_notes/release_17_02.rst > index 3b650388..30b23703 100644 > --- a/doc/guides/rel_notes/release_17_02.rst > +++ b/doc/guides/rel_notes/release_17_02.rst > @@ -106,16 +106,12 @@ API Changes > ABI Changes > ----------- > > -.. This section should contain ABI changes. Sample format: > - > - * Add a short 1-2 sentence description of the ABI change that was announced in > +.. * Add a short 1-2 sentence description of the ABI change that was announced in > the previous releases and made in this release. Use fixed width quotes for > ``rte_function_names`` or ``rte_struct_names``. Use the past tense. > > - This section is a comment. do not overwrite or remove it. > - Also, make sure to start the actual text at the margin. > - ========================================================= > - > +* The ``rte_eth_dev_info`` structure no longer has pointer to PCI device, but > + instead has new_field ``device`` which is a pointer to a generic ethernet device. > > > Shared Library Versions > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c > index 1e0f2061..71a8e9b9 100644 > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -1568,7 +1568,8 @@ rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info) > > RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); > (*dev->dev_ops->dev_infos_get)(dev, dev_info); > - dev_info->pci_dev = dev->pci_dev; > + > + dev_info->device = &dev->pci_dev->device; > dev_info->driver_name = dev->data->drv_name; I don't think that exposing the device through dev_info makes this a future proof. If we want to model some kind of extensions to dev_info we should instead model this explicitly. So from my point of view the pci_dev should get removed instead. > dev_info->nb_rx_queues = dev->data->nb_rx_queues; > dev_info->nb_tx_queues = dev->data->nb_tx_queues; > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h > index 3c85e331..2b3b4014 100644 > --- a/lib/librte_ether/rte_ethdev.h > +++ b/lib/librte_ether/rte_ethdev.h > @@ -879,7 +879,7 @@ struct rte_eth_conf { > * Ethernet device information > */ > struct rte_eth_dev_info { > - struct rte_pci_device *pci_dev; /**< Device PCI information. */ > + struct rte_device *device; /**< Device information. */ > const char *driver_name; /**< Device Driver name. */ > unsigned int if_index; /**< Index to bound host interface, or 0 if none. > Use if_indextoname() to translate into an interface name. */ > -- > 2.11.0 >