From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <arybchenko@solarflare.com>
Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com
[67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 3365C16E;
Mon, 18 Dec 2017 12:39:07 +0100 (CET)
X-Virus-Scanned: Proofpoint Essentials engine
Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16])
(using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits))
(No client certificate requested)
by mx1-us3.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id
C59C3980081; Mon, 18 Dec 2017 11:39:05 +0000 (UTC)
Received: from [192.168.38.17] (84.52.114.114) by ukex01.SolarFlarecom.com
(10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Mon, 18 Dec
2017 11:39:00 +0000
To: Igor Ryzhov <iryzhov@nfware.com>
CC: Olivier Matz <olivier.matz@6wind.com>, <dev@dpdk.org>, Thomas Monjalon
<thomas@monjalon.net>, Laurent Hardy <laurent.hardy@6wind.com>,
<stable@dpdk.org>
References: <20171214171531.10506-1-olivier.matz@6wind.com>
<adea1787-9c1c-3942-3b9e-ea42a2e44728@solarflare.com>
<CAF+s_FyBQfevMSaLHH9xOV=+DvTwk70XNcOrtGToumhKvs-kMA@mail.gmail.com>
From: Andrew Rybchenko <arybchenko@solarflare.com>
Message-ID: <acca024b-f96d-9d37-9f38-f63e5f7317f2@solarflare.com>
Date: Mon, 18 Dec 2017 14:38:55 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <CAF+s_FyBQfevMSaLHH9xOV=+DvTwk70XNcOrtGToumhKvs-kMA@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-GB
X-Originating-IP: [84.52.114.114]
X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To
ukex01.SolarFlarecom.com (10.17.10.4)
X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23536.003
X-TM-AS-Result: No--9.919300-0.000000-31
X-TM-AS-User-Approved-Sender: Yes
X-TM-AS-User-Blocked-Sender: No
X-MDID: 1513597146-0pDiMqbI9Y4f
Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: fix setting of MAC
address
X-BeenThere: stable@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches for DPDK stable branches <stable.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/stable>,
<mailto:stable-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/stable/>
List-Post: <mailto:stable@dpdk.org>
List-Help: <mailto:stable-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/stable>,
<mailto:stable-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 18 Dec 2017 11:39:07 -0000
On 12/18/2017 01:53 PM, Igor Ryzhov wrote:
>
> On Mon, Dec 18, 2017 at 1:35 PM, Andrew Rybchenko
> <arybchenko@solarflare.com <mailto:arybchenko@solarflare.com>> wrote:
>
> On 12/14/2017 08:15 PM, Olivier Matz wrote:
>
> From: Laurent Hardy <laurent.hardy@6wind.com
> <mailto:laurent.hardy@6wind.com>>
>
> When a new mac address is set, it is saved in dev->data->mac_addrs
> before the ethdev handler is called.
>
> First, it is inconsistent with the other ethdev functions
> rte_eth_dev_mac_addr_remove() and rte_eth_dev_mac_addr_add().
>
> Moreover, it prevents the drivers from wrongly comparing the
> old address
> and the new one, like it's done in i40evf driver:
>
> if (is_same_ether_addr(mac_addr, dev->data->mac_addrs))
> return;
>
> Fixes: 943c2d899a0c ("net/i40e: set VF MAC from VF")
> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org <mailto:stable@dpdk.org>
>
> Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com
> <mailto:laurent.hardy@6wind.com>>
> ---
> lib/librte_ether/rte_ethdev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c
> b/lib/librte_ether/rte_ethdev.c
> index 4f492e3db..297c02a54 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -2643,11 +2643,11 @@
> rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct
> ether_addr *addr)
> dev = &rte_eth_devices[port_id];
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set,
> -ENOTSUP);
> + (*dev->dev_ops->mac_addr_set)(dev, addr);
> +
> /* Update default address in NIC data structure */
> ether_addr_copy(addr, &dev->data->mac_addrs[0]);
> - (*dev->dev_ops->mac_addr_set)(dev, addr);
> -
> return 0;
> }
>
>
> NACK, unfortunately it will break net/sfc in one of branches when
> a new MAC
> is set using restart. It relies on the fact that a new MAC is
> already available in
> device data.
>
>
> Hello Andrew,
>
> Don't you think that it's not correct that net/sfc works that way?
>
> If we go further, dev->dev_ops->mac_addr_set not only should be
called before ether_addr_copy.
> It should return status code, and in case of error ether_addr_copy shouldn't be
called at all.
> Am I wrong?
Current behaviour is convenient. Alternative will require copy of MAC
address
to set in device private data and one more copy in the function to
rollback in
the case of failure. If there are good reasons to change behaviour, I don't
mind but PMDs should be reviewed carefully and fixed before the change.