diff mbox series

net/iavf: fix adding multicast MAC address

Message ID 20201015020204.12658-1-guinanx.sun@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series net/iavf: fix adding multicast MAC address |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Guinan Sun Oct. 15, 2020, 2:02 a.m. UTC 
  When the multicast address is added, it will flush
previous addresses first, and then add new ones.
So when adding an address that exceeds the upper limit
causes a failure, you need to add the previous address
list back. This patch fixes the issue.

Fixes: 05e4c3aff35f ("net/iavf: support multicast configuration")

Signed-off-by: Guinan Sun <guinanx.sun@intel.com>
---
 drivers/net/iavf/iavf_ethdev.c | 30 ++++++++++++++++++++++--------
 drivers/net/iavf/iavf_vchnl.c  |  3 ---
 2 files changed, 22 insertions(+), 11 deletions(-)

Comments

Peng, Yuan Oct. 15, 2020, 5:13 a.m. UTC | #1 
Test-by Peng, Yuan <yuan.peng@intel.com>


-----Original Message-----
From: dev <dev-bounces@dpdk.org> On Behalf Of Guinan Sun
Sent: Thursday, October 15, 2020 10:02 AM
To: dev@dpdk.org
Cc: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Sun, GuinanX <guinanx.sun@intel.com>
Subject: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC address

When the multicast address is added, it will flush previous addresses first, and then add new ones.
So when adding an address that exceeds the upper limit causes a failure, you need to add the previous address list back. This patch fixes the issue.

Fixes: 05e4c3aff35f ("net/iavf: support multicast configuration")

Signed-off-by: Guinan Sun <guinanx.sun@intel.com>
---
 drivers/net/iavf/iavf_ethdev.c | 30 ++++++++++++++++++++++--------  drivers/net/iavf/iavf_vchnl.c  |  3 ---
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c index e68e3bc71..042edadd9 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -164,7 +164,14 @@ iavf_set_mc_addr_list(struct rte_eth_dev *dev,
 	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
 	struct iavf_adapter *adapter =
 		IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
-	int err;
+	int err, temp_err;
+
+	if (mc_addrs_num > IAVF_NUM_MACADDR_MAX) {
+		PMD_DRV_LOG(ERR,
+			    "can't add more than a limited number (%u) of addresses.",
+			    (uint32_t)IAVF_NUM_MACADDR_MAX);
+		return -EINVAL;
+	}
 
 	/* flush previous addresses */
 	err = iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf->mc_addrs_num, @@ -172,17 +179,24 @@ iavf_set_mc_addr_list(struct rte_eth_dev *dev,
 	if (err)
 		return err;
 
-	vf->mc_addrs_num = 0;
-
 	/* add new ones */
 	err = iavf_add_del_mc_addr_list(adapter, mc_addrs, mc_addrs_num, true);
-	if (err)
-		return err;
 
-	vf->mc_addrs_num = mc_addrs_num;
-	memcpy(vf->mc_addrs, mc_addrs, mc_addrs_num * sizeof(*mc_addrs));
+	if (err) {
+		/* When adding new addresses fail, need to add the
+		 * previous addresses back.
+		 */
+		temp_err = iavf_add_del_mc_addr_list(adapter, vf->mc_addrs,
+						     vf->mc_addrs_num, true);
+		if (temp_err)
+			return temp_err;
+	} else {
+		vf->mc_addrs_num = mc_addrs_num;
+		memcpy(vf->mc_addrs,
+		       mc_addrs, mc_addrs_num * sizeof(*mc_addrs));
+	}
 
-	return 0;
+	return err;
 }
 
 static int
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c index db0b76876..a2295f879 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -1107,9 +1107,6 @@ iavf_add_del_mc_addr_list(struct iavf_adapter *adapter,
 	if (mc_addrs == NULL || mc_addrs_num == 0)
 		return 0;
 
-	if (mc_addrs_num > IAVF_NUM_MACADDR_MAX)
-		return -EINVAL;
-
 	list = (struct virtchnl_ether_addr_list *)cmd_buffer;
 	list->vsi_id = vf->vsi_res->vsi_id;
 	list->num_elements = mc_addrs_num;
--
2.13.6
Xing, Beilei Oct. 15, 2020, 5:37 a.m. UTC | #2 
> -----Original Message-----
> From: Peng, Yuan <yuan.peng@intel.com>
> Sent: Thursday, October 15, 2020 1:14 PM
> To: Sun, GuinanX <guinanx.sun@intel.com>; dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>; Sun, GuinanX <guinanx.sun@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC address
> 
> Test-by Peng, Yuan <yuan.peng@intel.com>
> 
> 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Guinan Sun
> Sent: Thursday, October 15, 2020 10:02 AM
> To: dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>; Sun, GuinanX <guinanx.sun@intel.com>
> Subject: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC address
> 
> When the multicast address is added, it will flush previous addresses first, and
> then add new ones.
> So when adding an address that exceeds the upper limit causes a failure, you
> need to add the previous address list back. This patch fixes the issue.
> 
> Fixes: 05e4c3aff35f ("net/iavf: support multicast configuration")
> 
> Signed-off-by: Guinan Sun <guinanx.sun@intel.com>
> ---
>  drivers/net/iavf/iavf_ethdev.c | 30 ++++++++++++++++++++++--------
> drivers/net/iavf/iavf_vchnl.c  |  3 ---
>  2 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
> index e68e3bc71..042edadd9 100644
> --- a/drivers/net/iavf/iavf_ethdev.c
> +++ b/drivers/net/iavf/iavf_ethdev.c
> @@ -164,7 +164,14 @@ iavf_set_mc_addr_list(struct rte_eth_dev *dev,
>  	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data-
> >dev_private);
>  	struct iavf_adapter *adapter =
>  		IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> -	int err;
> +	int err, temp_err;
> +
> +	if (mc_addrs_num > IAVF_NUM_MACADDR_MAX) {
> +		PMD_DRV_LOG(ERR,
> +			    "can't add more than a limited number (%u) of
> addresses.",
> +			    (uint32_t)IAVF_NUM_MACADDR_MAX);
> +		return -EINVAL;
> +	}
> 
>  	/* flush previous addresses */
>  	err = iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf-
> >mc_addrs_num, @@ -172,17 +179,24 @@ iavf_set_mc_addr_list(struct
> rte_eth_dev *dev,
>  	if (err)
>  		return err;
> 
> -	vf->mc_addrs_num = 0;
> -
>  	/* add new ones */
>  	err = iavf_add_del_mc_addr_list(adapter, mc_addrs, mc_addrs_num,
> true);
> -	if (err)
> -		return err;
> 
> -	vf->mc_addrs_num = mc_addrs_num;
> -	memcpy(vf->mc_addrs, mc_addrs, mc_addrs_num *
> sizeof(*mc_addrs));
> +	if (err) {
> +		/* When adding new addresses fail, need to add the
> +		 * previous addresses back.
> +		 */
> +		temp_err = iavf_add_del_mc_addr_list(adapter, vf->mc_addrs,
> +						     vf->mc_addrs_num, true);

Can we reuse err here?

> +		if (temp_err)
> +			return temp_err;
> +	} else {
> +		vf->mc_addrs_num = mc_addrs_num;
> +		memcpy(vf->mc_addrs,
> +		       mc_addrs, mc_addrs_num * sizeof(*mc_addrs));
> +	}
> 
> -	return 0;
> +	return err;
>  }
> 
>  static int
> diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c index
> db0b76876..a2295f879 100644
> --- a/drivers/net/iavf/iavf_vchnl.c
> +++ b/drivers/net/iavf/iavf_vchnl.c
> @@ -1107,9 +1107,6 @@ iavf_add_del_mc_addr_list(struct iavf_adapter
> *adapter,
>  	if (mc_addrs == NULL || mc_addrs_num == 0)
>  		return 0;
> 
> -	if (mc_addrs_num > IAVF_NUM_MACADDR_MAX)
> -		return -EINVAL;
> -
>  	list = (struct virtchnl_ether_addr_list *)cmd_buffer;
>  	list->vsi_id = vf->vsi_res->vsi_id;
>  	list->num_elements = mc_addrs_num;
> --
> 2.13.6
Guinan Sun Oct. 15, 2020, 5:57 a.m. UTC | #3 
Hi beilei

> -----Original Message-----
> From: Xing, Beilei <beilei.xing@intel.com>
> Sent: Thursday, October 15, 2020 1:37 PM
> To: Peng, Yuan <yuan.peng@intel.com>; Sun, GuinanX
> <guinanx.sun@intel.com>; dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> Sun, GuinanX <guinanx.sun@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC address
> 
> 
> 
> > -----Original Message-----
> > From: Peng, Yuan <yuan.peng@intel.com>
> > Sent: Thursday, October 15, 2020 1:14 PM
> > To: Sun, GuinanX <guinanx.sun@intel.com>; dev@dpdk.org
> > Cc: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Sun,
> > GuinanX <guinanx.sun@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC
> > address
> >
> > Test-by Peng, Yuan <yuan.peng@intel.com>
> >
> >
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Guinan Sun
> > Sent: Thursday, October 15, 2020 10:02 AM
> > To: dev@dpdk.org
> > Cc: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Sun,
> > GuinanX <guinanx.sun@intel.com>
> > Subject: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC address
> >
> > When the multicast address is added, it will flush previous addresses
> > first, and then add new ones.
> > So when adding an address that exceeds the upper limit causes a
> > failure, you need to add the previous address list back. This patch fixes the
> issue.
> >
> > Fixes: 05e4c3aff35f ("net/iavf: support multicast configuration")
> >
> > Signed-off-by: Guinan Sun <guinanx.sun@intel.com>
> > ---
> >  drivers/net/iavf/iavf_ethdev.c | 30 ++++++++++++++++++++++--------
> > drivers/net/iavf/iavf_vchnl.c  |  3 ---
> >  2 files changed, 22 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/iavf/iavf_ethdev.c
> > b/drivers/net/iavf/iavf_ethdev.c index e68e3bc71..042edadd9 100644
> > --- a/drivers/net/iavf/iavf_ethdev.c
> > +++ b/drivers/net/iavf/iavf_ethdev.c
> > @@ -164,7 +164,14 @@ iavf_set_mc_addr_list(struct rte_eth_dev *dev,
> > struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data-
> > >dev_private);
> >  struct iavf_adapter *adapter =
> >  IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > -int err;
> > +int err, temp_err;
> > +
> > +if (mc_addrs_num > IAVF_NUM_MACADDR_MAX) { PMD_DRV_LOG(ERR,
> > +    "can't add more than a limited number (%u) of
> > addresses.",
> > +    (uint32_t)IAVF_NUM_MACADDR_MAX);
> > +return -EINVAL;
> > +}
> >
> >  /* flush previous addresses */
> >  err = iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf-
> > >mc_addrs_num, @@ -172,17 +179,24 @@ iavf_set_mc_addr_list(struct
> > rte_eth_dev *dev,
> >  if (err)
> >  return err;
> >
> > -vf->mc_addrs_num = 0;
> > -
> >  /* add new ones */
> >  err = iavf_add_del_mc_addr_list(adapter, mc_addrs, mc_addrs_num,
> > true); -if (err) -return err;
> >
> > -vf->mc_addrs_num = mc_addrs_num;
> > -memcpy(vf->mc_addrs, mc_addrs, mc_addrs_num * sizeof(*mc_addrs));
> > +if (err) {
> > +/* When adding new addresses fail, need to add the
> > + * previous addresses back.
> > + */
> > +temp_err = iavf_add_del_mc_addr_list(adapter, vf->mc_addrs,
> > +     vf->mc_addrs_num, true);
> 
> Can we reuse err here?

This err cannot be reused.

When performing mac address addition, the testpmd side will first add the mac address to mac_pool.
When the driver layer returns that it failed to add an address, the tespmd layer will delete the failed address from mac_pool.

If err is reused, the successful result of executing the add back address will overwrite the previous err, causing problems with the address stored in mac_pool on the testpmd side.

> 
> > +if (temp_err)
> > +return temp_err;
> > +} else {
> > +vf->mc_addrs_num = mc_addrs_num;
> > +memcpy(vf->mc_addrs,
> > +       mc_addrs, mc_addrs_num * sizeof(*mc_addrs)); }
> >
> > -return 0;
> > +return err;
> >  }
> >
> >  static int
> > diff --git a/drivers/net/iavf/iavf_vchnl.c
> > b/drivers/net/iavf/iavf_vchnl.c index
> > db0b76876..a2295f879 100644
> > --- a/drivers/net/iavf/iavf_vchnl.c
> > +++ b/drivers/net/iavf/iavf_vchnl.c
> > @@ -1107,9 +1107,6 @@ iavf_add_del_mc_addr_list(struct iavf_adapter
> > *adapter,  if (mc_addrs == NULL || mc_addrs_num == 0)  return 0;
> >
> > -if (mc_addrs_num > IAVF_NUM_MACADDR_MAX) -return -EINVAL;
> > -
> >  list = (struct virtchnl_ether_addr_list *)cmd_buffer;  list->vsi_id =
> > vf->vsi_res->vsi_id;  list->num_elements = mc_addrs_num;
> > --
> > 2.13.6
>
Xing, Beilei Oct. 15, 2020, 6:43 a.m. UTC | #4 
> -----Original Message-----
> From: Sun, GuinanX <guinanx.sun@intel.com>
> Sent: Thursday, October 15, 2020 1:57 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Peng, Yuan <yuan.peng@intel.com>;
> dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC address
> 
> Hi beilei
> 
> > -----Original Message-----
> > From: Xing, Beilei <beilei.xing@intel.com>
> > Sent: Thursday, October 15, 2020 1:37 PM
> > To: Peng, Yuan <yuan.peng@intel.com>; Sun, GuinanX
> > <guinanx.sun@intel.com>; dev@dpdk.org
> > Cc: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; Sun, GuinanX <guinanx.sun@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC
> > address
> >
> >
> >
> > > -----Original Message-----
> > > From: Peng, Yuan <yuan.peng@intel.com>
> > > Sent: Thursday, October 15, 2020 1:14 PM
> > > To: Sun, GuinanX <guinanx.sun@intel.com>; dev@dpdk.org
> > > Cc: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Sun,
> > > GuinanX <guinanx.sun@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC
> > > address
> > >
> > > Test-by Peng, Yuan <yuan.peng@intel.com>
> > >
> > >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Guinan Sun
> > > Sent: Thursday, October 15, 2020 10:02 AM
> > > To: dev@dpdk.org
> > > Cc: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Sun,
> > > GuinanX <guinanx.sun@intel.com>
> > > Subject: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC
> > > address
> > >
> > > When the multicast address is added, it will flush previous
> > > addresses first, and then add new ones.
> > > So when adding an address that exceeds the upper limit causes a
> > > failure, you need to add the previous address list back. This patch
> > > fixes the
> > issue.
> > >
> > > Fixes: 05e4c3aff35f ("net/iavf: support multicast configuration")
> > >
> > > Signed-off-by: Guinan Sun <guinanx.sun@intel.com>
> > > ---
> > >  drivers/net/iavf/iavf_ethdev.c | 30 ++++++++++++++++++++++--------
> > > drivers/net/iavf/iavf_vchnl.c  |  3 ---
> > >  2 files changed, 22 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/net/iavf/iavf_ethdev.c
> > > b/drivers/net/iavf/iavf_ethdev.c index e68e3bc71..042edadd9 100644
> > > --- a/drivers/net/iavf/iavf_ethdev.c
> > > +++ b/drivers/net/iavf/iavf_ethdev.c
> > > @@ -164,7 +164,14 @@ iavf_set_mc_addr_list(struct rte_eth_dev *dev,
> > > struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data-
> > > >dev_private);
> > >  struct iavf_adapter *adapter =
> > >  IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > > -int err;
> > > +int err, temp_err;
> > > +
> > > +if (mc_addrs_num > IAVF_NUM_MACADDR_MAX) { PMD_DRV_LOG(ERR,
> > > +    "can't add more than a limited number (%u) of
> > > addresses.",
> > > +    (uint32_t)IAVF_NUM_MACADDR_MAX); return -EINVAL; }
> > >
> > >  /* flush previous addresses */
> > >  err = iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf-
> > > >mc_addrs_num, @@ -172,17 +179,24 @@ iavf_set_mc_addr_list(struct
> > > rte_eth_dev *dev,
> > >  if (err)
> > >  return err;
> > >
> > > -vf->mc_addrs_num = 0;
> > > -
> > >  /* add new ones */
> > >  err = iavf_add_del_mc_addr_list(adapter, mc_addrs, mc_addrs_num,
> > > true); -if (err) -return err;
> > >
> > > -vf->mc_addrs_num = mc_addrs_num;
> > > -memcpy(vf->mc_addrs, mc_addrs, mc_addrs_num * sizeof(*mc_addrs));
> > > +if (err) {
> > > +/* When adding new addresses fail, need to add the
> > > + * previous addresses back.
> > > + */
> > > +temp_err = iavf_add_del_mc_addr_list(adapter, vf->mc_addrs,
> > > +     vf->mc_addrs_num, true);
> >
> > Can we reuse err here?
> 
> This err cannot be reused.
> 
> When performing mac address addition, the testpmd side will first add the
> mac address to mac_pool.
> When the driver layer returns that it failed to add an address, the tespmd layer
> will delete the failed address from mac_pool.
> 
> If err is reused, the successful result of executing the add back address will
> overwrite the previous err, causing problems with the address stored in
> mac_pool on the testpmd side.
> 

Yes, I missed return err in the end of the function.
Little comments, can we use 'ret' which is more common to indicate return value?

> >
> > > +if (temp_err)
> > > +return temp_err;
> > > +} else {
> > > +vf->mc_addrs_num = mc_addrs_num;
> > > +memcpy(vf->mc_addrs,
> > > +       mc_addrs, mc_addrs_num * sizeof(*mc_addrs)); }
> > >
> > > -return 0;
> > > +return err;
> > >  }
> > >
> > >  static int
> > > diff --git a/drivers/net/iavf/iavf_vchnl.c
> > > b/drivers/net/iavf/iavf_vchnl.c index
> > > db0b76876..a2295f879 100644
> > > --- a/drivers/net/iavf/iavf_vchnl.c
> > > +++ b/drivers/net/iavf/iavf_vchnl.c
> > > @@ -1107,9 +1107,6 @@ iavf_add_del_mc_addr_list(struct iavf_adapter
> > > *adapter,  if (mc_addrs == NULL || mc_addrs_num == 0)  return 0;
> > >
> > > -if (mc_addrs_num > IAVF_NUM_MACADDR_MAX) -return -EINVAL;
> > > -
> > >  list = (struct virtchnl_ether_addr_list *)cmd_buffer;  list->vsi_id
> > > =
> > > vf->vsi_res->vsi_id;  list->num_elements = mc_addrs_num;
> > > --
> > > 2.13.6
> >
>
Guinan Sun Oct. 15, 2020, 6:46 a.m. UTC | #5 
Hi Beilei

> -----Original Message-----
> From: Xing, Beilei <beilei.xing@intel.com>
> Sent: Thursday, October 15, 2020 2:44 PM
> To: Sun, GuinanX <guinanx.sun@intel.com>; Peng, Yuan
> <yuan.peng@intel.com>; dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC address
> 
> 
> 
> > -----Original Message-----
> > From: Sun, GuinanX <guinanx.sun@intel.com>
> > Sent: Thursday, October 15, 2020 1:57 PM
> > To: Xing, Beilei <beilei.xing@intel.com>; Peng, Yuan
> > <yuan.peng@intel.com>; dev@dpdk.org
> > Cc: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC
> > address
> >
> > Hi beilei
> >
> > > -----Original Message-----
> > > From: Xing, Beilei <beilei.xing@intel.com>
> > > Sent: Thursday, October 15, 2020 1:37 PM
> > > To: Peng, Yuan <yuan.peng@intel.com>; Sun, GuinanX
> > > <guinanx.sun@intel.com>; dev@dpdk.org
> > > Cc: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>; Sun, GuinanX <guinanx.sun@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC
> > > address
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Peng, Yuan <yuan.peng@intel.com>
> > > > Sent: Thursday, October 15, 2020 1:14 PM
> > > > To: Sun, GuinanX <guinanx.sun@intel.com>; dev@dpdk.org
> > > > Cc: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > > <jingjing.wu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Sun,
> > > > GuinanX <guinanx.sun@intel.com>
> > > > Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC
> > > > address
> > > >
> > > > Test-by Peng, Yuan <yuan.peng@intel.com>
> > > >
> > > >
> > > > -----Original Message-----
> > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Guinan Sun
> > > > Sent: Thursday, October 15, 2020 10:02 AM
> > > > To: dev@dpdk.org
> > > > Cc: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > > <jingjing.wu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Sun,
> > > > GuinanX <guinanx.sun@intel.com>
> > > > Subject: [dpdk-dev] [PATCH] net/iavf: fix adding multicast MAC
> > > > address
> > > >
> > > > When the multicast address is added, it will flush previous
> > > > addresses first, and then add new ones.
> > > > So when adding an address that exceeds the upper limit causes a
> > > > failure, you need to add the previous address list back. This
> > > > patch fixes the
> > > issue.
> > > >
> > > > Fixes: 05e4c3aff35f ("net/iavf: support multicast configuration")
> > > >
> > > > Signed-off-by: Guinan Sun <guinanx.sun@intel.com>
> > > > ---
> > > >  drivers/net/iavf/iavf_ethdev.c | 30
> > > > ++++++++++++++++++++++-------- drivers/net/iavf/iavf_vchnl.c  |  3
> > > > ---
> > > >  2 files changed, 22 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/net/iavf/iavf_ethdev.c
> > > > b/drivers/net/iavf/iavf_ethdev.c index e68e3bc71..042edadd9 100644
> > > > --- a/drivers/net/iavf/iavf_ethdev.c
> > > > +++ b/drivers/net/iavf/iavf_ethdev.c
> > > > @@ -164,7 +164,14 @@ iavf_set_mc_addr_list(struct rte_eth_dev
> > > > *dev, struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data-
> > > > >dev_private);
> > > >  struct iavf_adapter *adapter =
> > > >  IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > > > -int err;
> > > > +int err, temp_err;
> > > > +
> > > > +if (mc_addrs_num > IAVF_NUM_MACADDR_MAX) { PMD_DRV_LOG(ERR,
> > > > +    "can't add more than a limited number (%u) of
> > > > addresses.",
> > > > +    (uint32_t)IAVF_NUM_MACADDR_MAX); return -EINVAL; }
> > > >
> > > >  /* flush previous addresses */
> > > >  err = iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf-
> > > > >mc_addrs_num, @@ -172,17 +179,24 @@ iavf_set_mc_addr_list(struct
> > > > rte_eth_dev *dev,
> > > >  if (err)
> > > >  return err;
> > > >
> > > > -vf->mc_addrs_num = 0;
> > > > -
> > > >  /* add new ones */
> > > >  err = iavf_add_del_mc_addr_list(adapter, mc_addrs, mc_addrs_num,
> > > > true); -if (err) -return err;
> > > >
> > > > -vf->mc_addrs_num = mc_addrs_num;
> > > > -memcpy(vf->mc_addrs, mc_addrs, mc_addrs_num * sizeof(*mc_addrs));
> > > > +if (err) {
> > > > +/* When adding new addresses fail, need to add the
> > > > + * previous addresses back.
> > > > + */
> > > > +temp_err = iavf_add_del_mc_addr_list(adapter, vf->mc_addrs,
> > > > +     vf->mc_addrs_num, true);
> > >
> > > Can we reuse err here?
> >
> > This err cannot be reused.
> >
> > When performing mac address addition, the testpmd side will first add
> > the mac address to mac_pool.
> > When the driver layer returns that it failed to add an address, the
> > tespmd layer will delete the failed address from mac_pool.
> >
> > If err is reused, the successful result of executing the add back
> > address will overwrite the previous err, causing problems with the
> > address stored in mac_pool on the testpmd side.
> >
> 
> Yes, I missed return err in the end of the function.
> Little comments, can we use 'ret' which is more common to indicate return
> value?

OK, patch V2 will fix it.

> 
> > >
> > > > +if (temp_err)
> > > > +return temp_err;
> > > > +} else {
> > > > +vf->mc_addrs_num = mc_addrs_num;
> > > > +memcpy(vf->mc_addrs,
> > > > +       mc_addrs, mc_addrs_num * sizeof(*mc_addrs)); }
> > > >
> > > > -return 0;
> > > > +return err;
> > > >  }
> > > >
> > > >  static int
> > > > diff --git a/drivers/net/iavf/iavf_vchnl.c
> > > > b/drivers/net/iavf/iavf_vchnl.c index
> > > > db0b76876..a2295f879 100644
> > > > --- a/drivers/net/iavf/iavf_vchnl.c
> > > > +++ b/drivers/net/iavf/iavf_vchnl.c
> > > > @@ -1107,9 +1107,6 @@ iavf_add_del_mc_addr_list(struct
> > > > iavf_adapter *adapter,  if (mc_addrs == NULL || mc_addrs_num == 0)
> > > > return 0;
> > > >
> > > > -if (mc_addrs_num > IAVF_NUM_MACADDR_MAX) -return -EINVAL;
> > > > -
> > > >  list = (struct virtchnl_ether_addr_list *)cmd_buffer;
> > > > list->vsi_id =
> > > > vf->vsi_res->vsi_id;  list->num_elements = mc_addrs_num;
> > > > --
> > > > 2.13.6
> > >
> >
>
diff mbox series

Patch

diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
index e68e3bc71..042edadd9 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -164,7 +164,14 @@  iavf_set_mc_addr_list(struct rte_eth_dev *dev,
 	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
 	struct iavf_adapter *adapter =
 		IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
-	int err;
+	int err, temp_err;
+
+	if (mc_addrs_num > IAVF_NUM_MACADDR_MAX) {
+		PMD_DRV_LOG(ERR,
+			    "can't add more than a limited number (%u) of addresses.",
+			    (uint32_t)IAVF_NUM_MACADDR_MAX);
+		return -EINVAL;
+	}
 
 	/* flush previous addresses */
 	err = iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf->mc_addrs_num,
@@ -172,17 +179,24 @@  iavf_set_mc_addr_list(struct rte_eth_dev *dev,
 	if (err)
 		return err;
 
-	vf->mc_addrs_num = 0;
-
 	/* add new ones */
 	err = iavf_add_del_mc_addr_list(adapter, mc_addrs, mc_addrs_num, true);
-	if (err)
-		return err;
 
-	vf->mc_addrs_num = mc_addrs_num;
-	memcpy(vf->mc_addrs, mc_addrs, mc_addrs_num * sizeof(*mc_addrs));
+	if (err) {
+		/* When adding new addresses fail, need to add the
+		 * previous addresses back.
+		 */
+		temp_err = iavf_add_del_mc_addr_list(adapter, vf->mc_addrs,
+						     vf->mc_addrs_num, true);
+		if (temp_err)
+			return temp_err;
+	} else {
+		vf->mc_addrs_num = mc_addrs_num;
+		memcpy(vf->mc_addrs,
+		       mc_addrs, mc_addrs_num * sizeof(*mc_addrs));
+	}
 
-	return 0;
+	return err;
 }
 
 static int
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index db0b76876..a2295f879 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -1107,9 +1107,6 @@  iavf_add_del_mc_addr_list(struct iavf_adapter *adapter,
 	if (mc_addrs == NULL || mc_addrs_num == 0)
 		return 0;
 
-	if (mc_addrs_num > IAVF_NUM_MACADDR_MAX)
-		return -EINVAL;
-
 	list = (struct virtchnl_ether_addr_list *)cmd_buffer;
 	list->vsi_id = vf->vsi_res->vsi_id;
 	list->num_elements = mc_addrs_num;