Re: [dpdk-dev] [PATCH v3 1/4] ethdev: increase port_id range

["Yang, Zhiyong" <zhiyong.yang@intel.com>]

Hi Ferruh,

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, September 11, 2017 6:22 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>; dev@dpdk.org; Doherty, Declan
> <declan.doherty@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: thomas@monjalon.net; hemant.agrawal@nxp.com; Hunt, David
> <david.hunt@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.yang@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
> <...>