Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: fix setting of MAC address

[Olivier Matz <olivier.matz@6wind.com>]

Hi,

On Wed, Dec 20, 2017 at 01:00:01PM +0300, Andrew Rybchenko wrote:
> On 12/19/2017 12:47 PM, Andrew Rybchenko wrote:
> > On 12/19/2017 12:29 PM, Olivier MATZ wrote:
> > > Hi,
> > > 
> > > On Mon, Dec 18, 2017 at 02:38:55PM +0300, Andrew Rybchenko wrote:
> > > > 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?
> > > Yes, I also have the same feeling.
> > > 
> > > > 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.
> > > Right. The first version of the patch was just a fix of the i40e code,
> > > which was of course less risky. But we finally decided to to that way
> > > for consistency.
> > > 
> > > I will review the other PMDs and send a v2 that should not break
> > > them. If you have any guidelines for net/sfc, they will be welcome :)
> > 
> > We'll send net/sfc patch today/tomorrow to fix it.
> 
> Done [1]
> 
> [1] http://dpdk.org/ml/archives/dev/2017-December/084526.html

I've walked through the PMDs as suggested by Andrew, and there was
indeed some conflicts with the initial patch. I've just submitted the
patch for vmxnet3 [1] and bnxt [2].

But there is still an issue with the qede driver, that overwrites the
MAC address in dev->data by the previous one if it cannot be set. It
seems it's the only driver that does this in error case, but anyway,
this behavior will be broken by the initial patch.

So I submitted a v2 that only changes the behavior for i40evf [3].

I propose to include these 3 patches for 18.02, and announce an ABI
change for 18.05 to add a return value to dev_ops->mac_addr_set() and
move the ether_addr_copy() after the callback, only in case of success.

Any opinions?


[1] https://dpdk.org/dev/patchwork/patch/32855/
[2] https://dpdk.org/dev/patchwork/patch/32855/
[3] https://dpdk.org/dev/patchwork/patch/32855/