Re: [dpdk-dev] [PATCH v3 1/3] ethdev: fix adding invalid MAC addr

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

Hi,

note, please use the --thread option in addition of the in-reply-to to
new series.

Small comments inline,

On Wed, Apr 12, 2017 at 05:02:24PM +0800, Wei Dai wrote:
>[...]
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> index ff903e6..91fc4c4 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -4475,26 +4475,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
>  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 -ENOTSUP;
>  	(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 -EINVAL;
> +	}
> +	re = priv_mac_addr_add(priv, index,
>  			  (const uint8_t (*)[ETHER_ADDR_LEN])
>  			  mac_addr->addr_bytes);

Please keep the coding style consistency among the file, this last
snippet should be:

        re = priv_mac_addr_add(priv, index,
                               (const uint8_t (*)[ETHER_ADDR_LEN])
                               mac_addr->addr_bytes);

>  end:
>  	priv_unlock(priv);
> +	return -re;
>  }
>  
>  /**
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index d26d465..3c5de07 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -238,8 +238,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);

In the whole file, function prototypes does not provide variable names,
please keep it as is.

Same point about the indentation, the second line should be aligned with
the '('.

>  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..3cc6f8b 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 -ENOTSUP;
>  
>  	(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 = -EINVAL;
>  		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);

Same remark here about the indentation.

>  end:
>  	priv_unlock(priv);
> +	return -re;
>  }

Thanks,

-- 
Nélio Laranjeiro
6WIND