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 EEC78C768 for ; Fri, 26 Jun 2015 09:03:38 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP; 26 Jun 2015 00:03:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,682,1427785200"; d="scan'208";a="750789921" Received: from pgsmsx108.gar.corp.intel.com ([10.221.44.103]) by fmsmga002.fm.intel.com with ESMTP; 26 Jun 2015 00:03:36 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by PGSMSX108.gar.corp.intel.com (10.221.44.103) with Microsoft SMTP Server (TLS) id 14.3.224.2; Fri, 26 Jun 2015 15:03:27 +0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.129]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.168]) with mapi id 14.03.0224.002; Fri, 26 Jun 2015 15:03:26 +0800 From: "Wu, Jingjing" To: "'nhorman@tuxdriver.com'" Thread-Topic: [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf Thread-Index: AQHQo0YzdTWZ6+MizUa530+v3dW9rJ2+cpXQ Date: Fri, 26 Jun 2015 07:03:25 +0000 Message-ID: <9BB6961774997848B5B42BEC655768F8C59FF9@SHSMSX104.ccr.corp.intel.com> References: <1433492166-30758-1-git-send-email-jingjing.wu@intel.com> <1433917473-21508-1-git-send-email-jingjing.wu@intel.com> <1433917473-21508-2-git-send-email-jingjing.wu@intel.com> In-Reply-To: <1433917473-21508-2-git-send-email-jingjing.wu@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 26 Jun 2015 07:03:40 -0000 Hi, Neil About this patch I have an ABI concern about it.=20 This patch just renamed a struct rte_eth_vmdq_mirror_conf to rte_eth_mirror= _conf, the size and its elements don't change. As my understanding, it will= not break the ABI. And I also tested it.=20 But when I use the script ./scripts/validate-abi.sh to check. A low severit= y problem is reported in symbol "rte_eth_mirror_rule_set" - Change: "Base type of 2nd parameter mirror_conf has been changed from st= ruct rte_eth_vmdq_mirror_conf to struct rte_eth_mirror_conf." - Effect: "Replacement of parameter base type may indicate a change in its= semantic meaning." So, I'm not sure whether this patch meet the ABI policy? Additional, about the validate-abi.sh, does it mean we need to fix all the = problems it reports? Or we can decide case by case. Can a Low Severity prob= lem be acceptable? Look forward to your reply. Thanks Jingjing > -----Original Message----- > From: Wu, Jingjing > Sent: Wednesday, June 10, 2015 2:25 PM > To: dev@dpdk.org > Cc: Wu, Jingjing; Liu, Jijiang; Jiajia, SunX; Zhang, Helin > Subject: [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf >=20 > rename rte_eth_vmdq_mirror_conf to rte_eth_mirror_conf and move the > maximum rule id check from ethdev level to driver >=20 > Signed-off-by: Jingjing Wu > --- > app/test-pmd/cmdline.c | 22 +++++++++++----------- > drivers/net/ixgbe/ixgbe_ethdev.c | 11 +++++++---- > drivers/net/ixgbe/ixgbe_ethdev.h | 4 +++- > lib/librte_ether/rte_ethdev.c | 18 ++---------------- > lib/librte_ether/rte_ethdev.h | 19 ++++++++++--------- > 5 files changed, 33 insertions(+), 41 deletions(-) >=20 > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index > f01db2a..d693bde 100644 > --- a/app/test-pmd/cmdline.c > +++ b/app/test-pmd/cmdline.c > @@ -6604,11 +6604,11 @@ cmd_set_mirror_mask_parsed(void > *parsed_result, { > int ret,nb_item,i; > struct cmd_set_mirror_mask_result *res =3D parsed_result; > - struct rte_eth_vmdq_mirror_conf mr_conf; > + struct rte_eth_mirror_conf mr_conf; >=20 > - memset(&mr_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf)); > + memset(&mr_conf, 0, sizeof(struct rte_eth_mirror_conf)); >=20 > - unsigned int vlan_list[ETH_VMDQ_MAX_VLAN_FILTERS]; > + unsigned int vlan_list[ETH_MIRROR_MAX_VLANS]; >=20 > mr_conf.dst_pool =3D res->dstpool_id; >=20 > @@ -6618,11 +6618,11 @@ cmd_set_mirror_mask_parsed(void > *parsed_result, > } else if(!strcmp(res->what, "vlan-mirror")) { > mr_conf.rule_type_mask =3D ETH_VMDQ_VLAN_MIRROR; > nb_item =3D parse_item_list(res->value, "core", > - > ETH_VMDQ_MAX_VLAN_FILTERS,vlan_list,1); > + ETH_MIRROR_MAX_VLANS, vlan_list, > 1); > if (nb_item <=3D 0) > return; >=20 > - for(i=3D0; i < nb_item; i++) { > + for (i =3D 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, > } >=20 > if(!strcmp(res->on, "on")) > - ret =3D rte_eth_mirror_rule_set(res->port_id,&mr_conf, > + ret =3D rte_eth_mirror_rule_set(res->port_id, &mr_conf, > res->rule_id, 1); > else > - ret =3D rte_eth_mirror_rule_set(res->port_id,&mr_conf, > + ret =3D 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 =3D parsed_result; > - struct rte_eth_vmdq_mirror_conf mr_conf; > + struct rte_eth_mirror_conf mr_conf; >=20 > - 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 =3D 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 =3D res->dstpool_id; >=20 > if(!strcmp(res->on, "on")) > - ret =3D rte_eth_mirror_rule_set(res->port_id,&mr_conf, > + ret =3D rte_eth_mirror_rule_set(res->port_id, &mr_conf, > res->rule_id, 1); > else > - ret =3D rte_eth_mirror_rule_set(res->port_id,&mr_conf, > + ret =3D rte_eth_mirror_rule_set(res->port_id, &mr_conf, > res->rule_id, 0); >=20 > /* check the return value and print it if is < 0 */ diff --git > a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c > index 0d9f9b2..9e767fa 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > +++ b/drivers/net/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(st= ruct > 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, >=20 > 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); >=20 > if (ixgbe_vmdq_mode_check(hw) < 0) > - return (-ENOTSUP); > + return -ENOTSUP; > + > + if (rule_id >=3D IXGBE_MAX_MIRROR_RULES) > + return -EINVAL; >=20 > /* 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); >=20 > memset(&mr_info->mr_conf[rule_id], 0, > - sizeof(struct rte_eth_vmdq_mirror_conf)); > + sizeof(struct rte_eth_mirror_conf)); >=20 > /* clear PFVMCTL register */ > IXGBE_WRITE_REG(hw, IXGBE_MRCTL(rule_id), mr_ctl); diff --git > a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h > index 19237b8..755b674 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.h > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h > @@ -177,8 +177,10 @@ struct ixgbe_uta_info { > uint32_t uta_shadow[IXGBE_MAX_UTA]; > }; >=20 > +#define IXGBE_MAX_MIRROR_RULES 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_MIRROR_RULES]; > /**< store PF mirror rules configuration*/ }; >=20 > 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, >=20 > 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 =3D &rte_eth_devices[port_id]; @@ -3051,7 > +3051,7 @@ rte_eth_mirror_rule_set(uint8_t port_id, >=20 > if (mirror_conf->dst_pool >=3D 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; > } >=20 > @@ -3062,13 +3062,6 @@ rte_eth_mirror_rule_set(uint8_t port_id, > return -EINVAL; > } >=20 > - if(rule_id >=3D 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 =3D &rte_eth_devices[port_id]; > FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mirror_rule_set, - > ENOTSUP); >=20 > @@ -3085,13 +3078,6 @@ rte_eth_mirror_rule_reset(uint8_t port_id, > uint8_t rule_id) > return -ENODEV; > } >=20 > - if(rule_id >=3D 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 =3D &rte_eth_devices[port_id]; > FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mirror_rule_reset, - > ENOTSUP); >=20 > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.= h > index 16dbe00..ae22fea 100644 > --- a/lib/librte_ether/rte_ethdev.h > +++ b/lib/librte_ether/rte_ethdev.h > @@ -467,8 +467,8 @@ struct rte_eth_rss_conf { > #define ETH_VMDQ_ACCEPT_BROADCAST 0x0008 /**< accept broadcast > packets. */ > #define ETH_VMDQ_ACCEPT_MULTICAST 0x0010 /**< multicast > promiscuous. */ >=20 > -/* Definitions used for VMDQ mirror rules setting */ > -#define ETH_VMDQ_NUM_MIRROR_RULE 4 /**< Maximum nb. of mirror > rules. . */ > +/** Maximum nb. of vlan per mirror rule */ > +#define ETH_MIRROR_MAX_VLANS 64 >=20 > #define ETH_VMDQ_POOL_MIRROR 0x0001 /**< Virtual Pool Mirroring. */ > #define ETH_VMDQ_UPLINK_MIRROR 0x0002 /**< Uplink Port Mirroring. > */ @@ -480,18 +480,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]; > - /** VLAN ID list for vlan mirror. */ > + /** VLAN ID list for vlan mirroring. */ > + uint16_t vlan_id[ETH_MIRROR_MAX_VLANS]; > }; >=20 > /** > * 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 */ > + /** VLAN ID setting for VLAN mirroring. */ > + struct rte_eth_vlan_mirror vlan; > }; >=20 > /** > @@ -1211,7 +1212,7 @@ typedef int (*eth_set_vf_rate_limit_t)(struct > rte_eth_dev *dev, /**< @internal Set VF TX rate */ >=20 > 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 +3169,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); >=20 > -- > 1.9.3