Hi Ferruh,
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, September 11, 2017 6:22 PM
> To: Yang, Zhiyong <zhiyong.y...@intel.com>; dev@dpdk.org; Doherty, Declan
> <declan.dohe...@intel.com>; Lu, Wenzhuo <wenzhuo...@intel.com>
> Cc: tho...@monjalon.net; hemant.agra...@nxp.com; Hunt, David
> <david.h...@intel.com>
> Subject: Re: [PATCH v3 1/4] ethdev: increase port_id range
>
> 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 <zhiyong.y...@intel.com>
>
> <...>
>
> > @@ -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.
>
Ferruh, the suggestion has been discussed in the following thread. Most of
people agree on
The basic type uint16_t. :). Your suggestion was my preference previously.
At last, I make this decision to use uint16_t. You know, whatever I use, some
ones will stand out and
Say the other is better. :)
http://www.dpdk.org/dev/patchwork/patch/23208/
> <...>
>
> > --- 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.
Ok
>
> > {
> > /* 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.
Ok.
>
> > {
> > 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.
>
Ok.
> > 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.
>
Ok
> <...>
>
> > @@ -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.
Ok
>
> <...>
> > @@ -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.
>
Ok
> <...>
>
> > @@ -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.
>
Do you mean that I should remove
> > -MAP_STATIC_SYMBOL(int rte_eth_bond_8023ad_setup(uint8_t port_id, ?
> 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.
>
Yes. I'm not clear about it. Need help Ferruh.
> <...>
>
> > @@ -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 ?
>
My bad. Remove it.
> <...>
>
> > @@ -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.. ?
>
The code caused failures when I test bonding driver in test code. and I will
fix them if I trace.
> <...>
>
> > --- 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.
>
thanks
> <...>
>
> > --- 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.
>
Ok.
> > + 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.
>
Ok.
> <...>
>
> > @@ -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.
Ok. I should focus on port id range increase . :)
>
> > 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.
>
Ok
> <...>
