From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 698056D45 for ; Mon, 11 Sep 2017 12:21:35 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Sep 2017 03:21:34 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,377,1500966000"; d="scan'208";a="899065054" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.57]) ([10.237.220.57]) by FMSMGA003.fm.intel.com with ESMTP; 11 Sep 2017 03:21:32 -0700 To: Zhiyong Yang , dev@dpdk.org, Declan Doherty , Wenzhuo Lu Cc: thomas@monjalon.net, hemant.agrawal@nxp.com, david.hunt@intel.com References: <20170904055734.21354-1-zhiyong.yang@intel.com> <20170909144727.46388-1-zhiyong.yang@intel.com> <20170909144727.46388-2-zhiyong.yang@intel.com> From: Ferruh Yigit Message-ID: Date: Mon, 11 Sep 2017 11:21:32 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20170909144727.46388-2-zhiyong.yang@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3 1/4] ethdev: increase port_id range 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: Mon, 11 Sep 2017 10:21:35 -0000 On 9/9/2017 3:47 PM, Zhiyong Yang wrote: > Extend port_id definition from uint8_t to uint16_t in lib and drivers > data structures, specifically rte_eth_dev_data. > Modify the APIs, drivers and app using port_id at the same time. > > Fix some checkpatch issues from the original code and remove some > unnecessary cast operations. > > Signed-off-by: Zhiyong Yang <...> > @@ -283,7 +283,7 @@ enum dcb_mode_enable > #define MAX_RX_QUEUE_STATS_MAPPINGS 4096 /* MAX_PORT of 32 @ 128 rx_queues/port */ > > struct queue_stats_mappings { > - uint8_t port_id; > + uint16_t port_id; Can this be "portid_t port_id;" ? For testpmd, portid_t can be used for all port_id declarations. <...> > --- a/drivers/net/bnx2x/bnx2x.c > +++ b/drivers/net/bnx2x/bnx2x.c > @@ -703,7 +703,7 @@ bnx2x_gpio_mult_write(struct bnx2x_softc *sc, uint8_t pins, uint32_t mode) > > static int > bnx2x_gpio_int_write(struct bnx2x_softc *sc, int gpio_num, uint32_t mode, > - uint8_t port) > + uint8_t port) If port storage type will not change, no need to update this line. It is good to fix syntax the lines touched, but for the lines not updated please don't fix the syntax in this patch. > { > /* The GPIO should be swapped if swap register is set and active */ > int gpio_port = ((REG_RD(sc, NIG_REG_PORT_SWAP) && > @@ -749,7 +749,7 @@ bnx2x_gpio_int_write(struct bnx2x_softc *sc, int gpio_num, uint32_t mode, > } > > uint32_t > -elink_cb_gpio_read(struct bnx2x_softc * sc, uint16_t gpio_num, uint8_t port) > +elink_cb_gpio_read(struct bnx2x_softc *sc, uint16_t gpio_num, uint8_t port) Same here. > { > return bnx2x_gpio_read(sc, gpio_num, port); > } > diff --git a/drivers/net/bnx2x/bnx2x_rxtx.h b/drivers/net/bnx2x/bnx2x_rxtx.h > index 2e38ec26a..48d540476 100644 > --- a/drivers/net/bnx2x/bnx2x_rxtx.h > +++ b/drivers/net/bnx2x/bnx2x_rxtx.h > @@ -41,7 +41,7 @@ struct bnx2x_rx_queue { > uint16_t rx_cq_head; /**< Index of current rcq bd. */ > uint16_t rx_cq_tail; /**< Index of last rcq bd. */ > uint16_t queue_id; /**< RX queue index. */ > - uint8_t port_id; /**< Device port identifier. */ > + uint16_t port_id; /**< Device port identifier. */ Please fix comment allignment. > struct bnx2x_softc *sc; /**< Ptr to dev_private data. */ > }; <...> > @@ -500,7 +501,7 @@ elink_status_t elink_phy_probe(struct elink_params *params); > > /* Checks if fan failure detection is required on one of the phys on board */ > uint8_t elink_fan_failure_det_req(struct bnx2x_softc *sc, uint32_t shmem_base, > - uint32_t shmem2_base, uint8_t port); > + uint32_t shmem2_base, uint8_t port); no change, please drop. <...> > @@ -511,7 +511,6 @@ mux_machine(struct bond_dev_private *internals, uint8_t slave_id) > ACTOR_STATE_CLR(port, SYNCHRONIZATION); > MODE4_DEBUG("Out of sync -> ATTACHED\n"); > } > - Please drop this one. <...> > @@ -1022,12 +1022,12 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev, uint8_t slave_id) > > int > bond_mode_8023ad_deactivate_slave(struct rte_eth_dev *bond_dev, > - uint8_t slave_id) > + uint16_t slave_id) The coding style for multiple lines in a function call is two tabs or alternatively allign to the paranthesis. Original code synyax looks good here, no need to update. <...> > @@ -1536,17 +1536,12 @@ rte_eth_bond_8023ad_setup_v1708(uint8_t port_id, > return 0; > } > BIND_DEFAULT_SYMBOL(rte_eth_bond_8023ad_setup, _v1708, 17.08); > -MAP_STATIC_SYMBOL(int rte_eth_bond_8023ad_setup(uint8_t port_id, > +MAP_STATIC_SYMBOL(int rte_eth_bond_8023ad_setup(uint16_t port_id, Hmm, this is tricky! The macro MAP_STATIC_SYMBOL is used for ABI versioning, but changing the port_id storage type breaks the ABI already. ABI versioning can be removed completely. Cc'ed Declan. Which also reminds me that bonding LIBABIVER needs to be updated. This is also required for all i40e, ixgbe and bnxt. Please let me know if you need help here. <...> > @@ -1622,12 +1618,13 @@ rte_eth_bond_8023ad_ext_collect(uint8_t port_id, uint8_t slave_id, int enabled) > ACTOR_STATE_SET(port, COLLECTING); > else > ACTOR_STATE_CLR(port, COLLECTING); > - > + printf("enabled port->actor_state = %d \r\n", port->actor_state); Is this a git rebase error ? <...> > @@ -586,13 +588,14 @@ rte_eth_bond_active_slaves_get(uint8_t bonded_port_id, uint8_t slaves[], > if (internals->active_slave_count > len) > return -1; > > - memcpy(slaves, internals->active_slaves, internals->active_slave_count); > + memcpy(slaves, internals->active_slaves, > + internals->active_slave_count * sizeof(internals->active_slaves[0])); Good catch! I wonder if there are more like this, did you traced all memcpy, memset, etc.. ? <...> > --- a/drivers/net/ixgbe/rte_pmd_ixgbe.c > +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.c > @@ -38,7 +38,7 @@ > #include "rte_pmd_ixgbe.h" > > int > -rte_pmd_ixgbe_set_vf_mac_addr(uint8_t port, uint16_t vf, > +rte_pmd_ixgbe_set_vf_mac_addr(uint16_t port, uint16_t vf, > struct ether_addr *mac_addr) ixgbe LIBABIVER also needs to be updated. I have just recognized that release notes missing this library, I will add. <...> > --- a/drivers/net/vmxnet3/vmxnet3_ring.h > +++ b/drivers/net/vmxnet3/vmxnet3_ring.h > @@ -143,8 +143,8 @@ typedef struct vmxnet3_tx_queue { > struct vmxnet3_txq_stats stats; > const struct rte_memzone *mz; > bool stopped; > - uint16_t queue_id; /**< Device TX queue index. */ > - uint8_t port_id; /**< Device port identifier. */ > + uint16_t queue_id; /**< Device TX queue index. */ No need to change "queue_id" here, if this is for comment allignment, please allign the port_id one. > + uint16_t port_id; /**< Device port identifier. */ > uint16_t txdata_desc_size; > } vmxnet3_tx_queue_t; > > @@ -178,8 +178,8 @@ typedef struct vmxnet3_rx_queue { > struct vmxnet3_rxq_stats stats; > const struct rte_memzone *mz; > bool stopped; > - uint16_t queue_id; /**< Device RX queue index. */ > - uint8_t port_id; /**< Device port identifier. */ > + uint16_t queue_id; /**< Device RX queue index. */ same as above. <...> > @@ -94,8 +94,7 @@ rte_port_ethdev_reader_create(void *params, int socket_id) > static int > rte_port_ethdev_reader_rx(void *port, struct rte_mbuf **pkts, uint32_t n_pkts) > { > - struct rte_port_ethdev_reader *p = > - port; > + struct rte_port_ethdev_reader *p = port; This is a good syntax correction, but this patch is already big, please drop these ones. > uint16_t rx_pkt_cnt; > > rx_pkt_cnt = rte_eth_rx_burst(p->port_id, p->queue_id, pkts, n_pkts); > @@ -119,8 +118,7 @@ rte_port_ethdev_reader_free(void *port) > static int rte_port_ethdev_reader_stats_read(void *port, > struct rte_port_in_stats *stats, int clear) > { > - struct rte_port_ethdev_reader *p = > - port; > + struct rte_port_ethdev_reader *p = port; same as above, and there are few more below. <...>