From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f196.google.com (mail-wr0-f196.google.com [209.85.128.196]) by dpdk.org (Postfix) with ESMTP id 930275F21 for ; Mon, 5 Mar 2018 11:29:14 +0100 (CET) Received: by mail-wr0-f196.google.com with SMTP id v111so16650904wrb.3 for ; Mon, 05 Mar 2018 02:29:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=TdPiTz/ddDYA7TC5RHlcZABEid5PN2qj5binEWAkwjE=; b=ia/SPK+EXrxMf8hvOX2/1kIjlkfFI62m9rA2jcMUzXm9YYJ9B7dZRTzyGw7gDptpG3 h4Sgbf//JQF1LjjltWfPmjmzl0Ny/x7ev+niXbwXgaE5CdN5ZYZfiaijgRlBUuKRs2FO r4APy1f7Zmj3XNEWTZVlNZoKg7hHH/4Vo0ATlCyjwxw4+a2x8V3VKKyVEDI3tuxYqM5H TZG/MPBAnH1NvsNi/vkRHZ0Z6DXMXl6ZEPnlbSxLI7tze+qNCxArUto0jFMN+us57mh7 b1lzdxJH/Wd6jEnzMzuTnLaFL59qHQTLLR0hb5G+LivfmLzJcijnCb98z+aRV/SU8GI8 7yVg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=TdPiTz/ddDYA7TC5RHlcZABEid5PN2qj5binEWAkwjE=; b=fKo+4uySrSBYeDGwTbLidClpr6oXwSegjWX3V3+ulZ+Z4A7kD6sdJ8p5BXaDH0zyZ0 /v146gFbMcAk/k5z8+dON2CZ5zP8VR5JVfTkmlfwledJa508DrEh1kLYm3WZV364ts3j xTJMhHKCXFYjOxJNDfRuICxKVcqjxY+Xw6cKFMxs2ytfuwSG6BYWmD4rky+vs67+vLCm lkrjwobe9xaiCnzMXiwspTOzfyc9VTm8+t3Th8uIPLxUueAZcyvs4hGGTwbLpxlJP+h4 eOzGaDBdqRN2C1irm8xy091ebrFeMftxenfwE6PAvbtxCG8/Rj1HydiEiAZvK376f9Lb RtMA== X-Gm-Message-State: APf1xPBIhCNLAc5Y6jN1gKZ6DfqOL5AyKuNSFnib6gHSjtCJ/LYqPAx7 pcVjHgijAzB/AOnQG18BaJuqFg== X-Google-Smtp-Source: AG47ELvKMy1qr3+bkGMrmvPKFTL2u1UBD3S9DXvxM9VGy4/0UJFA/AoL+5+yNHhzskfJNHEWtYqihw== X-Received: by 10.223.199.10 with SMTP id k10mr11928465wrg.186.1520245754244; Mon, 05 Mar 2018 02:29:14 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 59sm10842405wro.57.2018.03.05.02.29.13 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 05 Mar 2018 02:29:13 -0800 (PST) Date: Mon, 5 Mar 2018 11:29:00 +0100 From: Adrien Mazarguil To: Olivier Matz Cc: dev@dpdk.org, Thomas Monjalon , Ferruh Yigit Message-ID: <20180305102900.GI4256@6wind.com> References: <20180227151129.30387-1-olivier.matz@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180227151129.30387-1-olivier.matz@6wind.com> Subject: Re: [dpdk-dev] [PATCH] ethdev: return diagnostic when setting MAC address X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 05 Mar 2018 10:29:14 -0000 On Tue, Feb 27, 2018 at 04:11:29PM +0100, Olivier Matz wrote: > Change the prototype and the behavior of dev_ops->eth_mac_addr_set(): a > return code is added to notify the caller (librte_ether) if an error > occurred in the PMD. > > The new default MAC address is now copied in dev->data->mac_addrs[0] > only if the operation is successful. > > The patch also updates all the PMDs accordingly. > > Signed-off-by: Olivier Matz > --- > > Hi, > > This patch is the following of the discussion we had in this thread: > https://dpdk.org/dev/patchwork/patch/32284/ > > I did my best to keep the consistency inside the PMDs. The behavior > of eth_mac_addr_set() is inspired from other fonctions in the same > PMD, usually eth_mac_addr_add(). For instance: > - dpaa and dpaa2 return 0 on error. > - some PMDs (bnxt, mlx5, ...?) do not return a -errno code (-1 or > positive values). > - some PMDs (avf, tap) check if the address is the same and return 0 > in that case. This could go in generic code? > > I tried to use the following errors when relevant: > - -EPERM when a VF is not allowed to do a change > - -ENOTSUP if the function is not supported > - -EIO if this is an unknown error from lower layer (hw or sdk) Keep in mind EIO is currently documented in ethdev as somewhat hot-plug-related, as in "device is unresponsive and likely unplugged". The reaction of a hot-plug-aware application to such an error code might be to close the device, possibly for the wrong reason. I just wanted to point it out, I don't think it's a problem for this patch but can't speak for all PMDs. > - -EINVAL for other unknown errors > > Please, PMD maintainers, feel free to comment if you ahve specific > needs for your driver. OK with the API change and it's fine for mlx4 and mlx5, with a few comments regarding the latter, please see below. > diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h > index 19c8a223d..c107794ce 100644 > --- a/drivers/net/mlx4/mlx4.h > +++ b/drivers/net/mlx4/mlx4.h > @@ -131,7 +131,7 @@ void mlx4_allmulticast_disable(struct rte_eth_dev *dev); > void mlx4_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index); > int mlx4_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr, > uint32_t index, uint32_t vmdq); > -void mlx4_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr); > +int mlx4_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr); > int mlx4_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on); > int mlx4_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats); > void mlx4_stats_reset(struct rte_eth_dev *dev); > diff --git a/drivers/net/mlx4/mlx4_ethdev.c b/drivers/net/mlx4/mlx4_ethdev.c > index 3bc692731..2442e16a6 100644 > --- a/drivers/net/mlx4/mlx4_ethdev.c > +++ b/drivers/net/mlx4/mlx4_ethdev.c > @@ -701,11 +701,14 @@ mlx4_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on) > * Pointer to Ethernet device structure. > * @param mac_addr > * MAC address to register. > + * > + * @return > + * 0 on success, negative errno value otherwise and rte_errno is set. > */ > -void > +int > mlx4_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) > { > - mlx4_mac_addr_add(dev, mac_addr, 0, 0); > + return mlx4_mac_addr_add(dev, mac_addr, 0, 0); > } > > /** > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h > index 965c19f21..42e58d7f7 100644 > --- a/drivers/net/mlx5/mlx5.h > +++ b/drivers/net/mlx5/mlx5.h > @@ -241,7 +241,7 @@ int priv_get_mac(struct priv *, uint8_t (*)[ETHER_ADDR_LEN]); > void mlx5_mac_addr_remove(struct rte_eth_dev *, uint32_t); > int mlx5_mac_addr_add(struct rte_eth_dev *, struct ether_addr *, uint32_t, > uint32_t); > -void mlx5_mac_addr_set(struct rte_eth_dev *, struct ether_addr *); > +int 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 e8a8d4594..0dc4bec46 100644 > --- a/drivers/net/mlx5/mlx5_mac.c > +++ b/drivers/net/mlx5/mlx5_mac.c > @@ -118,10 +118,13 @@ mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac, > * Pointer to Ethernet device structure. > * @param mac_addr > * MAC address to register. > + * > + * @return > + * 0 on success. > */ > -void > +int > mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) > { > DEBUG("%p: setting primary MAC address", (void *)dev); > - mlx5_mac_addr_add(dev, mac_addr, 0, 0); > + return mlx5_mac_addr_add(dev, mac_addr, 0, 0); > } With Nelio's errno rework for mlx5 [1][2], this change should end up being similar to mlx4. [1] http://dpdk.org/ml/archives/dev/2018-February/091668.html [2] http://dpdk.org/ml/archives/dev/2018-February/091678.html -- Adrien Mazarguil 6WIND