From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro Subject: Re: [PATCH v3 1/3] ethdev: fix adding invalid MAC addr Date: Wed, 12 Apr 2017 11:30:20 +0200 Message-ID: <20170412093020.GC16796@autoinstall.dev.6wind.com> References: <1491987746-10155-1-git-send-email-wei.dai@intel.com> <1491987746-10155-2-git-send-email-wei.dai@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit 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 To: Wei Dai Return-path: Received: from mail-wr0-f171.google.com (mail-wr0-f171.google.com [209.85.128.171]) by dpdk.org (Postfix) with ESMTP id C5957316B for ; Wed, 12 Apr 2017 11:30:28 +0200 (CEST) Received: by mail-wr0-f171.google.com with SMTP id c55so13472909wrc.3 for ; Wed, 12 Apr 2017 02:30:28 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1491987746-10155-2-git-send-email-wei.dai@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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