All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>
To: Wei Dai <wei.dai@intel.com>
Cc: dev@dpdk.org, thomas.monjalon@6wind.com, harish.patil@cavium.com,
	rasesh.mody@cavium.com, stephen.hurd@broadcom.com,
	ajit.khaparde@broadcom.com, wenzhuo.lu@intel.com,
	helin.zhang@intel.com, konstantin.ananyev@intel.com,
	jingjing.wu@intel.com, jing.d.chen@intel.com,
	adrien.mazarguil@6wind.com, bruce.richardson@intel.com,
	yuanhan.liu@linux.intel.com, maxime.coquelin@redhat.com,
	stable@dpdk.org
Subject: Re: [PATCH v3 1/3] ethdev: fix adding invalid MAC addr
Date: Wed, 12 Apr 2017 11:30:20 +0200	[thread overview]
Message-ID: <20170412093020.GC16796@autoinstall.dev.6wind.com> (raw)
In-Reply-To: <1491987746-10155-2-git-send-email-wei.dai@intel.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

  reply	other threads:[~2017-04-12  9:30 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-12  9:02 [PATCH v3 0/3] MAC address fail to be added shouldn't be stored Wei Dai
2017-04-12  9:02 ` [PATCH v3 1/3] ethdev: fix adding invalid MAC addr Wei Dai
2017-04-12  9:30   ` Nélio Laranjeiro [this message]
2017-04-12  9:02 ` [PATCH v3 2/3] doc: change type of return value of adding " Wei Dai
2017-04-12  9:02 ` [PATCH v3 3/3] app/testpmd: add a command to add many MAC addrs Wei Dai
2017-04-13  8:21 ` [PATCH v4 0/3] MAC address fail to be added shouldn't be stored Wei Dai
2017-04-13 13:54   ` Ananyev, Konstantin
2017-04-13  8:21 ` [PATCH v4 1/3] ethdev: fix adding invalid MAC addr Wei Dai
2017-04-13  8:44   ` Nélio Laranjeiro
2017-04-13  9:22     ` Dai, Wei
2017-04-20  5:31   ` Yuanhan Liu
2017-04-20 21:43   ` [dpdk-stable] " Thomas Monjalon
2017-04-21  6:43   ` Lu, Wenzhuo
2017-04-29  6:09     ` Dai, Wei
2017-05-02  1:21       ` Lu, Wenzhuo
2017-05-02  1:51         ` Dai, Wei
2017-04-13  8:21 ` [PATCH v4 2/3] doc: change type of return value of adding " Wei Dai
2017-04-14 16:03   ` Mcnamara, John
2017-04-13  8:21 ` [PATCH v4 3/3] app/testpmd: add a command to add many MAC addrs Wei Dai

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170412093020.GC16796@autoinstall.dev.6wind.com \
    --to=nelio.laranjeiro@6wind.com \
    --cc=adrien.mazarguil@6wind.com \
    --cc=ajit.khaparde@broadcom.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=harish.patil@cavium.com \
    --cc=helin.zhang@intel.com \
    --cc=jing.d.chen@intel.com \
    --cc=jingjing.wu@intel.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=rasesh.mody@cavium.com \
    --cc=stable@dpdk.org \
    --cc=stephen.hurd@broadcom.com \
    --cc=thomas.monjalon@6wind.com \
    --cc=wei.dai@intel.com \
    --cc=wenzhuo.lu@intel.com \
    --cc=yuanhan.liu@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.