Re: [dpdk-dev] [PATCH 1/3] ethdev: rename rte_eth_vmdq_mirror_conf

["Liu, Jijiang" <jijiang.liu@intel.com>]

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Wednesday, May 13, 2015 4:47 PM
> To: dev@dpdk.org
> Cc: Wu, Jingjing; Jiajia, SunX; Zhang, Helin; Liu, Jijiang
> Subject: [PATCH 1/3] ethdev: rename rte_eth_vmdq_mirror_conf
> 
> This patch renames rte_eth_vmdq_mirror_conf to rte_eth_mirror_conf and
> move the maximum number check from ethdev level to driver
> 
> Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> ---
>  app/test-pmd/cmdline.c              | 18 +++++++++---------
>  lib/librte_ether/rte_ethdev.c       | 18 ++----------------
>  lib/librte_ether/rte_ethdev.h       | 19 +++++++++++--------
>  lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 11 +++++++----
> lib/librte_pmd_ixgbe/ixgbe_ethdev.h |  4 +++-
>  5 files changed, 32 insertions(+), 38 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> f01db2a..9c1fb43 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -6604,9 +6604,9 @@ cmd_set_mirror_mask_parsed(void
> *parsed_result,  {
>  	int ret,nb_item,i;
>  	struct cmd_set_mirror_mask_result *res = parsed_result;
> -	struct rte_eth_vmdq_mirror_conf mr_conf;
> +	struct rte_eth_mirror_conf mr_conf;
> 
> -	memset(&mr_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf));
> +	memset(&mr_conf, 0, sizeof(struct rte_eth_mirror_conf));
> 
>  	unsigned int vlan_list[ETH_VMDQ_MAX_VLAN_FILTERS];
> 
> @@ -6622,7 +6622,7 @@ cmd_set_mirror_mask_parsed(void
> *parsed_result,
>  		if (nb_item <= 0)
>  			return;
> 
> -		for(i=0; i < nb_item; i++) {
> +		for (i = 0; i < nb_item; i++) {
>  			if (vlan_list[i] > ETHER_MAX_VLAN_ID) {
>  				printf("Invalid vlan_id: must be < 4096\n");
>  				return;
> @@ -6634,10 +6634,10 @@ cmd_set_mirror_mask_parsed(void
> *parsed_result,
>  	}
> 
>  	if(!strcmp(res->on, "on"))
> -		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> +		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
>  						res->rule_id, 1);
>  	else
> -		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> +		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
>  						res->rule_id, 0);
>  	if(ret < 0)
>  		printf("mirror rule add error: (%s)\n", strerror(-ret)); @@ -
> 6711,9 +6711,9 @@ cmd_set_mirror_link_parsed(void *parsed_result,  {
>  	int ret;
>  	struct cmd_set_mirror_link_result *res = parsed_result;
> -	struct rte_eth_vmdq_mirror_conf mr_conf;
> +	struct rte_eth_mirror_conf mr_conf;
> 
> -	memset(&mr_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf));
> +	memset(&mr_conf, 0, sizeof(struct rte_eth_mirror_conf));
>  	if(!strcmp(res->what, "uplink-mirror")) {
>  		mr_conf.rule_type_mask = ETH_VMDQ_UPLINK_MIRROR;
>  	}else if(!strcmp(res->what, "downlink-mirror")) @@ -6722,10
> +6722,10 @@ cmd_set_mirror_link_parsed(void *parsed_result,
>  	mr_conf.dst_pool = res->dstpool_id;
> 
>  	if(!strcmp(res->on, "on"))
> -		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> +		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
>  						res->rule_id, 1);
>  	else
> -		ret = rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> +		ret = rte_eth_mirror_rule_set(res->port_id, &mr_conf,
>  						res->rule_id, 0);
> 
>  	/* check the return value and print it if is < 0 */ diff --git
> a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index
> 024fe8b..43c7295 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -3034,7 +3034,7 @@ int rte_eth_set_vf_rate_limit(uint8_t port_id,
> uint16_t vf, uint16_t tx_rate,
> 
>  int
>  rte_eth_mirror_rule_set(uint8_t port_id,
> -			struct rte_eth_vmdq_mirror_conf *mirror_conf,
> +			struct rte_eth_mirror_conf *mirror_conf,
>  			uint8_t rule_id, uint8_t on)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id]; @@ -3051,7
> +3051,7 @@ rte_eth_mirror_rule_set(uint8_t port_id,
> 
>  	if (mirror_conf->dst_pool >= ETH_64_POOLS) {
>  		PMD_DEBUG_TRACE("Invalid dst pool, pool id must"
> -			"be 0-%d\n",ETH_64_POOLS - 1);
> +			"be 0-%d\n", ETH_64_POOLS - 1);
>  		return -EINVAL;
>  	}
> 
> @@ -3062,13 +3062,6 @@ rte_eth_mirror_rule_set(uint8_t port_id,
>  		return -EINVAL;
>  	}
> 
> -	if(rule_id >= ETH_VMDQ_NUM_MIRROR_RULE)
> -	{
> -		PMD_DEBUG_TRACE("Invalid rule_id, rule_id must be 0-
> %d\n",
> -			ETH_VMDQ_NUM_MIRROR_RULE - 1);
> -		return -EINVAL;
> -	}
> -
>  	dev = &rte_eth_devices[port_id];
>  	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mirror_rule_set, -
> ENOTSUP);
> 
> @@ -3085,13 +3078,6 @@ rte_eth_mirror_rule_reset(uint8_t port_id,
> uint8_t rule_id)
>  		return -ENODEV;
>  	}
> 
> -	if(rule_id >= ETH_VMDQ_NUM_MIRROR_RULE)
> -	{
> -		PMD_DEBUG_TRACE("Invalid rule_id, rule_id must be 0-
> %d\n",
> -			ETH_VMDQ_NUM_MIRROR_RULE-1);
> -		return -EINVAL;
> -	}
> -
>  	dev = &rte_eth_devices[port_id];
>  	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mirror_rule_reset, -
> ENOTSUP);
> 
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 16dbe00..562b190 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -467,8 +467,10 @@ struct rte_eth_rss_conf {
>  #define ETH_VMDQ_ACCEPT_BROADCAST   0x0008 /**< accept broadcast
> packets. */
>  #define ETH_VMDQ_ACCEPT_MULTICAST   0x0010 /**< multicast
> promiscuous. */
> 
> -/* Definitions used for VMDQ mirror rules setting */
> -#define ETH_VMDQ_NUM_MIRROR_RULE     4 /**< Maximum nb. of mirror
> rules. . */
> +/* Definitions used for mirror rules setting */
> +#define ETH_MAX_NUM_MIRROR_RULE        16  /**< Maximum nb. of
> mirror rules. */
> +#define ETH_MIRROR_MAX_NUM_VLANS       64
> +/** Maximum nb. of vlan per mirror rule*/
> 
>  #define ETH_VMDQ_POOL_MIRROR    0x0001 /**< Virtual Pool Mirroring.
> */
>  #define ETH_VMDQ_UPLINK_MIRROR  0x0002 /**< Uplink Port Mirroring.
> */ @@ -480,18 +482,19 @@ struct rte_eth_rss_conf {
>   */
>  struct rte_eth_vlan_mirror {
>  	uint64_t vlan_mask; /**< mask for valid VLAN ID. */
> -	uint16_t vlan_id[ETH_VMDQ_MAX_VLAN_FILTERS];
> +	uint16_t vlan_id[ETH_MIRROR_MAX_NUM_VLANS];
>  	/** VLAN ID list for vlan mirror. */

The comment doesn't follow coding rule, which should be /**< VLAN ID list for vlan mirror. */
>  };
> 
>  /**
>   * A structure used to configure traffic mirror of an Ethernet port.
>   */
> -struct rte_eth_vmdq_mirror_conf {
> +struct rte_eth_mirror_conf {
>  	uint8_t rule_type_mask; /**< Mirroring rule type mask we want to
> set */
> -	uint8_t dst_pool; /**< Destination pool for this mirror rule. */
> +	uint8_t dst_pool;  /**< Destination pool for this mirror rule. */
>  	uint64_t pool_mask; /**< Bitmap of pool for pool mirroring */
> -	struct rte_eth_vlan_mirror vlan; /**< VLAN ID setting for VLAN
> mirroring */
> +	struct rte_eth_vlan_mirror vlan;
> +	/** VLAN ID setting for VLAN mirroring */
The comment doesn't follow coding rule, which should be /**< VLAN ID setting for VLAN mirroring*/,

Could you please check other places?
>  };
> 
>  /**
> @@ -1211,7 +1214,7 @@ typedef int (*eth_set_vf_rate_limit_t)(struct
> rte_eth_dev *dev,  /**< @internal Set VF TX rate */
> 
>  typedef int (*eth_mirror_rule_set_t)(struct rte_eth_dev *dev,
> -				  struct rte_eth_vmdq_mirror_conf
> *mirror_conf,
> +				  struct rte_eth_mirror_conf *mirror_conf,
>  				  uint8_t rule_id,
>  				  uint8_t on);
>  /**< @internal Add a traffic mirroring rule on an Ethernet device */ @@ -
> 3168,7 +3171,7 @@ rte_eth_dev_set_vf_vlan_filter(uint8_t port, uint16_t
> vlan_id,
>   *   - (-EINVAL) if the mr_conf information is not correct.
>   */
>  int rte_eth_mirror_rule_set(uint8_t port_id,
> -			struct rte_eth_vmdq_mirror_conf *mirror_conf,
> +			struct rte_eth_mirror_conf *mirror_conf,
>  			uint8_t rule_id,
>  			uint8_t on);
> 
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> index 5f9a1cf..2e19314 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> @@ -209,7 +209,7 @@ static int ixgbe_set_pool_tx(struct rte_eth_dev
> *dev,uint16_t pool,uint8_t on);  static int ixgbe_set_pool_vlan_filter(struct
> rte_eth_dev *dev, uint16_t vlan,
>  		uint64_t pool_mask,uint8_t vlan_on);
>  static int ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
> -		struct rte_eth_vmdq_mirror_conf *mirror_conf,
> +		struct rte_eth_mirror_conf *mirror_conf,
>  		uint8_t rule_id, uint8_t on);
>  static int ixgbe_mirror_rule_reset(struct rte_eth_dev *dev,
>  		uint8_t	rule_id);
> @@ -3388,7 +3388,7 @@ ixgbe_set_pool_vlan_filter(struct rte_eth_dev
> *dev, uint16_t vlan,
> 
>  static int
>  ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
> -			struct rte_eth_vmdq_mirror_conf *mirror_conf,
> +			struct rte_eth_mirror_conf *mirror_conf,
>  			uint8_t rule_id, uint8_t on)
>  {
>  	uint32_t mr_ctl,vlvf;
> @@ -3412,7 +3412,10 @@ ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
>  		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> 
>  	if (ixgbe_vmdq_mode_check(hw) < 0)
> -		return (-ENOTSUP);
> +		return -ENOTSUP;
> +
> +	if (rule_id >= IXGBE_MAX_NUM_MIRROR_RULE)
> +		return -EINVAL;
> 
>  	/* Check if vlan mask is valid */
>  	if ((mirror_conf->rule_type_mask & ETH_VMDQ_VLAN_MIRROR) &&
> (on)) { @@ -3526,7 +3529,7 @@ ixgbe_mirror_rule_reset(struct rte_eth_dev
> *dev, uint8_t rule_id)
>  		return (-ENOTSUP);
> 
>  	memset(&mr_info->mr_conf[rule_id], 0,
> -		sizeof(struct rte_eth_vmdq_mirror_conf));
> +		sizeof(struct rte_eth_mirror_conf));
> 
>  	/* clear PFVMCTL register */
>  	IXGBE_WRITE_REG(hw, IXGBE_MRCTL(rule_id), mr_ctl); diff --git
> a/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
> b/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
> index 419ea5d..6422000 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
> +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
> @@ -177,8 +177,10 @@ struct ixgbe_uta_info {
>  	uint32_t uta_shadow[IXGBE_MAX_UTA];
>  };
> 
> +#define IXGBE_MAX_NUM_MIRROR_RULE 4  /* Maximum nb. of mirror
> rules. */
> +
>  struct ixgbe_mirror_info {
> -	struct rte_eth_vmdq_mirror_conf
> mr_conf[ETH_VMDQ_NUM_MIRROR_RULE];
> +	struct rte_eth_mirror_conf
> mr_conf[IXGBE_MAX_NUM_MIRROR_RULE];
>  	/**< store PF mirror rules configuration*/  };
> 
> --
> 1.9.3