[dpdk-dev] [PATCH 1/2] ethdev: fix adding invalid MAC addr

[Dai, Wei]

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.laranjeiro at 6wind.com]
> Sent: Monday, April 3, 2017 5:18 PM
> To: Dai, Wei <wei.dai at intel.com>
> Cc: dev at dpdk.org; thomas.monjalon at 6wind.com; harish.patil at cavium.com;
> rasesh.mody at cavium.com; stephen.hurd at broadcom.com;
> ajit.khaparde at broadcom.com; Lu, Wenzhuo <wenzhuo.lu at intel.com>; Zhang,
> Helin <helin.zhang at intel.com>; Ananyev, Konstantin
> <konstantin.ananyev at intel.com>; Wu, Jingjing <jingjing.wu at intel.com>; Chen,
> Jing D <jing.d.chen at intel.com>; adrien.mazarguil at 6wind.com; Richardson,
> Bruce <bruce.richardson at intel.com>; yuanhan.liu at linux.intel.com;
> maxime.coquelin at redhat.com; stable at 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