Hi, Nélio Laranjeiro Thanks for your feedback. I will change them in my next version of patch set.
-Wei > -----Original Message----- > From: Nélio Laranjeiro [mailto:nelio.laranje...@6wind.com] > Sent: Monday, April 3, 2017 5:18 PM > To: Dai, Wei <wei....@intel.com> > Cc: dev@dpdk.org; thomas.monja...@6wind.com; harish.pa...@cavium.com; > rasesh.m...@cavium.com; stephen.h...@broadcom.com; > ajit.khapa...@broadcom.com; Lu, Wenzhuo <wenzhuo...@intel.com>; Zhang, > Helin <helin.zh...@intel.com>; Ananyev, Konstantin > <konstantin.anan...@intel.com>; Wu, Jingjing <jingjing...@intel.com>; Chen, > Jing D <jing.d.c...@intel.com>; adrien.mazarg...@6wind.com; Richardson, > Bruce <bruce.richard...@intel.com>; yuanhan....@linux.intel.com; > maxime.coque...@redhat.com; sta...@dpdk.org > Subject: Re: [PATCH 1/2] ethdev: fix adding invalid MAC addr > > Hi, > > Please see below some comments, > > On Sat, Apr 01, 2017 at 04:03:16PM +0800, Wei Dai wrote: > >[...] > > diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c index > >79efaaa..d61bb27 100644 > > --- a/drivers/net/mlx4/mlx4.c > > +++ b/drivers/net/mlx4/mlx4.c > > @@ -4630,26 +4630,30 @@ mlx4_mac_addr_remove(struct rte_eth_dev > *dev, uint32_t index) > > * @param vmdq > > * VMDq pool index to associate address with (ignored). > > */ > > -static void > > +static int > > Please keep function documentation up to date. > > > mlx4_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr > *mac_addr, > > uint32_t index, uint32_t vmdq) > > { > > struct priv *priv = dev->data->dev_private; > > + int re; > > > > if (mlx4_is_secondary()) > > - return; > > + return -1; > > Should not it return ENOSUP instead? > > > (void)vmdq; > > priv_lock(priv); > > DEBUG("%p: adding MAC address at index %" PRIu32, > > (void *)dev, index); > > /* Last array entry is reserved for broadcast. */ > > - if (index >= (elemof(priv->mac) - 1)) > > - goto end; > > - priv_mac_addr_add(priv, index, > > + if (index >= (elemof(priv->mac) - 1)) { > > + priv_unlock(priv); > > + return -2; > > + } > > + re = priv_mac_addr_add(priv, index, > > (const uint8_t (*)[ETHER_ADDR_LEN]) > > mac_addr->addr_bytes); > > You have an issue here, internal function of mlx drivers return positives > errno > (as described in their functions documentation). > It should be negated before returning. > > > end: > > priv_unlock(priv); > > + return re; > > } > > > > /** > > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index > > 879da5e..cca7a36 100644 > > --- a/drivers/net/mlx5/mlx5.h > > +++ b/drivers/net/mlx5/mlx5.h > > @@ -229,8 +229,8 @@ int hash_rxq_mac_addrs_add(struct hash_rxq *); > > int priv_mac_addr_add(struct priv *, unsigned int, > > const uint8_t (*)[ETHER_ADDR_LEN]); int > > priv_mac_addrs_enable(struct priv *); -void mlx5_mac_addr_add(struct > > rte_eth_dev *, struct ether_addr *, uint32_t, > > - uint32_t); > > +int mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr > *mac_addr, > > + uint32_t index, uint32_t vmdq); > > void mlx5_mac_addr_set(struct rte_eth_dev *, struct ether_addr *); > > > > /* mlx5_rss.c */ > > diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c > > index 4fcfd3b..db93f4e 100644 > > --- a/drivers/net/mlx5/mlx5_mac.c > > +++ b/drivers/net/mlx5/mlx5_mac.c > > @@ -470,26 +470,30 @@ priv_mac_addrs_enable(struct priv *priv) > > * @param vmdq > > * VMDq pool index to associate address with (ignored). > > */ > > -void > > +int > > mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr > *mac_addr, > > uint32_t index, uint32_t vmdq) > > { > > struct priv *priv = dev->data->dev_private; > > + int re; > > > > if (mlx5_is_secondary()) > > - return; > > + return -1; > > > > (void)vmdq; > > priv_lock(priv); > > DEBUG("%p: adding MAC address at index %" PRIu32, > > (void *)dev, index); > > - if (index >= RTE_DIM(priv->mac)) > > + if (index >= RTE_DIM(priv->mac)) { > > + re = -2; > > goto end; > > - priv_mac_addr_add(priv, index, > > + } > > + re = priv_mac_addr_add(priv, index, > > (const uint8_t (*)[ETHER_ADDR_LEN]) > > mac_addr->addr_bytes); > > end: > > priv_unlock(priv); > > + return re; > > } > >[...] > > Same remarks here, > > Thanks, > > -- > Nélio Laranjeiro > 6WIND
