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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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.

<...>

> --- 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.

<...>