patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] ethdev: fix setting of MAC address
@ 2017-12-14 17:15 Olivier Matz
  2017-12-18 10:35 ` [dpdk-stable] [dpdk-dev] " Andrew Rybchenko
  2018-01-03 13:34 ` [dpdk-stable] [PATCH v2] net/i40e: fix setting of MAC address on i40evf Olivier Matz
  0 siblings, 2 replies; 15+ messages in thread
From: Olivier Matz @ 2017-12-14 17:15 UTC (permalink / raw)
  To: dev, Thomas Monjalon; +Cc: Laurent Hardy, stable

From: Laurent Hardy <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

Signed-off-by: Laurent Hardy <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;
 }
 
-- 
2.11.0

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: fix setting of MAC address
  2017-12-14 17:15 [dpdk-stable] [PATCH] ethdev: fix setting of MAC address Olivier Matz
@ 2017-12-18 10:35 ` Andrew Rybchenko
  2017-12-18 10:53   ` Igor Ryzhov
  2018-01-03 13:34 ` [dpdk-stable] [PATCH v2] net/i40e: fix setting of MAC address on i40evf Olivier Matz
  1 sibling, 1 reply; 15+ messages in thread
From: Andrew Rybchenko @ 2017-12-18 10:35 UTC (permalink / raw)
  To: Olivier Matz, dev, Thomas Monjalon; +Cc: Laurent Hardy, stable

On 12/14/2017 08:15 PM, Olivier Matz wrote:
> From: Laurent Hardy <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
>
> Signed-off-by: Laurent Hardy <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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: fix setting of MAC address
  2017-12-18 10:35 ` [dpdk-stable] [dpdk-dev] " Andrew Rybchenko
@ 2017-12-18 10:53   ` Igor Ryzhov
  2017-12-18 11:38     ` Andrew Rybchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Igor Ryzhov @ 2017-12-18 10:53 UTC (permalink / raw)
  To: Andrew Rybchenko
  Cc: Olivier Matz, dev, Thomas Monjalon, Laurent Hardy, stable

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?

Best regards,
Igor

On Mon, Dec 18, 2017 at 1:35 PM, Andrew Rybchenko <arybchenko@solarflare.com
> wrote:

> On 12/14/2017 08:15 PM, Olivier Matz wrote:
>
>> From: Laurent Hardy <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
>>
>> Signed-off-by: Laurent Hardy <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.
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: fix setting of MAC address
  2017-12-18 10:53   ` Igor Ryzhov
@ 2017-12-18 11:38     ` Andrew Rybchenko
  2017-12-19  9:29       ` Olivier MATZ
  0 siblings, 1 reply; 15+ messages in thread
From: Andrew Rybchenko @ 2017-12-18 11:38 UTC (permalink / raw)
  To: Igor Ryzhov; +Cc: Olivier Matz, dev, Thomas Monjalon, Laurent Hardy, stable

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: fix setting of MAC address
  2017-12-18 11:38     ` Andrew Rybchenko
@ 2017-12-19  9:29       ` Olivier MATZ
  2017-12-19  9:47         ` Andrew Rybchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Olivier MATZ @ 2017-12-19  9:29 UTC (permalink / raw)
  To: Andrew Rybchenko; +Cc: Igor Ryzhov, dev, Thomas Monjalon, Laurent Hardy, stable

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 :)

Thanks for the review.
Olivier

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: fix setting of MAC address
  2017-12-19  9:29       ` Olivier MATZ
@ 2017-12-19  9:47         ` Andrew Rybchenko
  2017-12-20 10:00           ` Andrew Rybchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Andrew Rybchenko @ 2017-12-19  9:47 UTC (permalink / raw)
  To: Olivier MATZ, Ivan Malov
  Cc: Igor Ryzhov, dev, Thomas Monjalon, Laurent Hardy, stable

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.

Thanks,
Andrew.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: fix setting of MAC address
  2017-12-19  9:47         ` Andrew Rybchenko
@ 2017-12-20 10:00           ` Andrew Rybchenko
  2018-01-03 13:43             ` Olivier Matz
  0 siblings, 1 reply; 15+ messages in thread
From: Andrew Rybchenko @ 2017-12-20 10:00 UTC (permalink / raw)
  To: Olivier MATZ, Ivan Malov
  Cc: Igor Ryzhov, dev, Thomas Monjalon, Laurent Hardy, stable

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-stable] [PATCH v2] net/i40e: fix setting of MAC address on i40evf
  2017-12-14 17:15 [dpdk-stable] [PATCH] ethdev: fix setting of MAC address Olivier Matz
  2017-12-18 10:35 ` [dpdk-stable] [dpdk-dev] " Andrew Rybchenko
@ 2018-01-03 13:34 ` Olivier Matz
  2018-01-04  7:39   ` Xing, Beilei
  1 sibling, 1 reply; 15+ messages in thread
From: Olivier Matz @ 2018-01-03 13:34 UTC (permalink / raw)
  To: dev, Jingjing Wu, Beilei Xing; +Cc: stable

When setting the MAC address, the ethdev layer copies the new mac
address in dev->data->mac_addrs[0] before calling the dev_ops.

Therefore, "is_same_ether_addr(mac_addr, dev->data->mac_addrs)" was
always true, and the MAC was never set. Remove this test to fix the
issue.

Fixes: 943c2d899a0c ("net/i40e: set VF MAC from VF")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/i40e/i40e_ethdev_vf.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 91b5bb033..17446ec23 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -2681,9 +2681,6 @@ i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
 		return;
 	}
 
-	if (is_same_ether_addr(mac_addr, dev->data->mac_addrs))
-		return;
-
 	if (vf->flags & I40E_FLAG_VF_MAC_BY_PF)
 		return;
 
-- 
2.11.0

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: fix setting of MAC address
  2017-12-20 10:00           ` Andrew Rybchenko
@ 2018-01-03 13:43             ` Olivier Matz
  2018-01-03 13:54               ` Olivier Matz
  0 siblings, 1 reply; 15+ messages in thread
From: Olivier Matz @ 2018-01-03 13:43 UTC (permalink / raw)
  To: Andrew Rybchenko
  Cc: Ivan Malov, Igor Ryzhov, dev, Thomas Monjalon, Laurent Hardy,
	Ferruh Yigit, stable

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/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: fix setting of MAC address
  2018-01-03 13:43             ` Olivier Matz
@ 2018-01-03 13:54               ` Olivier Matz
  2018-01-03 14:12                 ` Andrew Rybchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Olivier Matz @ 2018-01-03 13:54 UTC (permalink / raw)
  To: Andrew Rybchenko
  Cc: Ivan Malov, Igor Ryzhov, dev, Thomas Monjalon, Laurent Hardy,
	Ferruh Yigit, stable

On Wed, Jan 03, 2018 at 02:43:59PM +0100, Olivier Matz wrote:
> 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/

Sorry:

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: fix setting of MAC address
  2018-01-03 13:54               ` Olivier Matz
@ 2018-01-03 14:12                 ` Andrew Rybchenko
  2018-01-08 11:59                   ` Ferruh Yigit
  0 siblings, 1 reply; 15+ messages in thread
From: Andrew Rybchenko @ 2018-01-03 14:12 UTC (permalink / raw)
  To: Olivier Matz
  Cc: Ivan Malov, Igor Ryzhov, dev, Thomas Monjalon, Laurent Hardy,
	Ferruh Yigit, stable

On 01/03/2018 04:54 PM, Olivier Matz wrote:
> On Wed, Jan 03, 2018 at 02:43:59PM +0100, Olivier Matz wrote:
>> 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?

I'm not sure if dev_ops->mac_addr_set() is a part of ABI.
It is an internal interface between rte_ethdev library and drivers. Yes, 
out-of-tree
drivers will be broken.
rte_eth_dev_default_mac_addr_set() is definitely a part of API/ABI, but 
it already
has return value.
So, I'm not sure that we have to wait for 18.05, but it is still may be 
too late for
18.02 since integration deadline is pretty close.

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

Many thanks for drivers review and fixes.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-stable] [PATCH v2] net/i40e: fix setting of MAC address on i40evf
  2018-01-03 13:34 ` [dpdk-stable] [PATCH v2] net/i40e: fix setting of MAC address on i40evf Olivier Matz
@ 2018-01-04  7:39   ` Xing, Beilei
  2018-01-08  6:43     ` [dpdk-stable] [dpdk-dev] " Zhang, Helin
  0 siblings, 1 reply; 15+ messages in thread
From: Xing, Beilei @ 2018-01-04  7:39 UTC (permalink / raw)
  To: Olivier Matz, dev, Wu, Jingjing; +Cc: stable


> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, January 3, 2018 9:34 PM
> To: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>
> Cc: stable@dpdk.org
> Subject: [PATCH v2] net/i40e: fix setting of MAC address on i40evf
> 
> When setting the MAC address, the ethdev layer copies the new mac
> address in dev->data->mac_addrs[0] before calling the dev_ops.
> 
> Therefore, "is_same_ether_addr(mac_addr, dev->data->mac_addrs)" was
> always true, and the MAC was never set. Remove this test to fix the issue.
> 
> Fixes: 943c2d899a0c ("net/i40e: set VF MAC from VF")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

Acked-by: Beilei Xing <beilei.xing@intel.com>, thanks for the fix.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/i40e: fix setting of MAC address on i40evf
  2018-01-04  7:39   ` Xing, Beilei
@ 2018-01-08  6:43     ` Zhang, Helin
  0 siblings, 0 replies; 15+ messages in thread
From: Zhang, Helin @ 2018-01-08  6:43 UTC (permalink / raw)
  To: Xing, Beilei, Olivier Matz, dev, Wu, Jingjing; +Cc: stable



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xing, Beilei
> Sent: Thursday, January 4, 2018 3:40 PM
> To: Olivier Matz; dev@dpdk.org; Wu, Jingjing
> Cc: stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: fix setting of MAC address on
> i40evf
> 
> 
> > -----Original Message-----
> > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > Sent: Wednesday, January 3, 2018 9:34 PM
> > To: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>
> > Cc: stable@dpdk.org
> > Subject: [PATCH v2] net/i40e: fix setting of MAC address on i40evf
> >
> > When setting the MAC address, the ethdev layer copies the new mac
> > address in dev->data->mac_addrs[0] before calling the dev_ops.
> >
> > Therefore, "is_same_ether_addr(mac_addr, dev->data->mac_addrs)" was
> > always true, and the MAC was never set. Remove this test to fix the issue.
> >
> > Fixes: 943c2d899a0c ("net/i40e: set VF MAC from VF")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Acked-by: Beilei Xing <beilei.xing@intel.com>, thanks for the fix.
Applied to dpdk-next-net-intel, thanks!

/Helin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: fix setting of MAC address
  2018-01-03 14:12                 ` Andrew Rybchenko
@ 2018-01-08 11:59                   ` Ferruh Yigit
  2018-01-08 14:23                     ` Olivier Matz
  0 siblings, 1 reply; 15+ messages in thread
From: Ferruh Yigit @ 2018-01-08 11:59 UTC (permalink / raw)
  To: Andrew Rybchenko, Olivier Matz
  Cc: Ivan Malov, Igor Ryzhov, dev, Thomas Monjalon, Laurent Hardy, stable

On 1/3/2018 2:12 PM, Andrew Rybchenko wrote:
> On 01/03/2018 04:54 PM, Olivier Matz wrote:
>> On Wed, Jan 03, 2018 at 02:43:59PM +0100, Olivier Matz wrote:
>>> 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?
> 
> I'm not sure if dev_ops->mac_addr_set() is a part of ABI.
> It is an internal interface between rte_ethdev library and drivers. Yes, out-of-tree
> drivers will be broken.
> rte_eth_dev_default_mac_addr_set() is definitely a part of API/ABI, but it already
> has return value.
> So, I'm not sure that we have to wait for 18.05, but it is still may be too late for
> 18.02 since integration deadline is pretty close.

I think there is no API/ABI breakage, but it can be good to announce the change
and give time for modifications.

+1 for proposal.

> 
>> [1] https://dpdk.org/dev/patchwork/patch/32855/
>> [2] https://dpdk.org/dev/patchwork/patch/32856/
>> [3] https://dpdk.org/dev/patchwork/patch/32857/
> 
> Many thanks for drivers review and fixes.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: fix setting of MAC address
  2018-01-08 11:59                   ` Ferruh Yigit
@ 2018-01-08 14:23                     ` Olivier Matz
  0 siblings, 0 replies; 15+ messages in thread
From: Olivier Matz @ 2018-01-08 14:23 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Andrew Rybchenko, Ivan Malov, Igor Ryzhov, dev, Thomas Monjalon,
	Laurent Hardy, stable

On Mon, Jan 08, 2018 at 11:59:09AM +0000, Ferruh Yigit wrote:
> On 1/3/2018 2:12 PM, Andrew Rybchenko wrote:
> > On 01/03/2018 04:54 PM, Olivier Matz wrote:
> >> On Wed, Jan 03, 2018 at 02:43:59PM +0100, Olivier Matz wrote:
> >>> 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?
> > 
> > I'm not sure if dev_ops->mac_addr_set() is a part of ABI.
> > It is an internal interface between rte_ethdev library and drivers. Yes, out-of-tree
> > drivers will be broken.
> > rte_eth_dev_default_mac_addr_set() is definitely a part of API/ABI, but it already
> > has return value.
> > So, I'm not sure that we have to wait for 18.05, but it is still may be too late for
> > 18.02 since integration deadline is pretty close.
> 
> I think there is no API/ABI breakage, but it can be good to announce the change
> and give time for modifications.
> 
> +1 for proposal.

Thanks Andrew and Ferruh for the feedback.
I'll send an RFC for this soon.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2018-01-08 14:23 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-14 17:15 [dpdk-stable] [PATCH] ethdev: fix setting of MAC address Olivier Matz
2017-12-18 10:35 ` [dpdk-stable] [dpdk-dev] " Andrew Rybchenko
2017-12-18 10:53   ` Igor Ryzhov
2017-12-18 11:38     ` Andrew Rybchenko
2017-12-19  9:29       ` Olivier MATZ
2017-12-19  9:47         ` Andrew Rybchenko
2017-12-20 10:00           ` Andrew Rybchenko
2018-01-03 13:43             ` Olivier Matz
2018-01-03 13:54               ` Olivier Matz
2018-01-03 14:12                 ` Andrew Rybchenko
2018-01-08 11:59                   ` Ferruh Yigit
2018-01-08 14:23                     ` Olivier Matz
2018-01-03 13:34 ` [dpdk-stable] [PATCH v2] net/i40e: fix setting of MAC address on i40evf Olivier Matz
2018-01-04  7:39   ` Xing, Beilei
2018-01-08  6:43     ` [dpdk-stable] [dpdk-dev] " Zhang, Helin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).