From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro Subject: Re: [PATCH 1/2] ethdev: fix adding invalid MAC addr Date: Mon, 3 Apr 2017 11:18:16 +0200 Message-ID: <20170403091816.GC16796@autoinstall.dev.6wind.com> References: <1491033797-14186-1-git-send-email-wei.dai@intel.com> <1491033797-14186-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-f178.google.com (mail-wr0-f178.google.com [209.85.128.178]) by dpdk.org (Postfix) with ESMTP id 3E55BDE0 for ; Mon, 3 Apr 2017 11:18:25 +0200 (CEST) Received: by mail-wr0-f178.google.com with SMTP id w11so159036975wrc.3 for ; Mon, 03 Apr 2017 02:18:25 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1491033797-14186-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, 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