[RFC] lib/librte_ether: add a return value for MAC set ops

[RFC] lib/librte_ether: add a return value for MAC set ops

[Wenzhuo Lu]

Setting the default MAC address may fail on many NICs.
But the ops return void. So, even it failed, RTE changes
the MAC address and APP doesn't know the failure.

It's not real patch, just show the idea to add a return
value for the ops.

BTW,
Seems we should do the same thing for
rte_eth_dev_mac_addr_remove as it also has chance to fail
in PMD layer.

Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 drivers/net/i40e/i40e_ethdev_vf.c  | 12 +++++++-----
 lib/librte_ether/rte_ethdev.c      |  7 +++++--
 lib/librte_ether/rte_ethdev.h      |  1 +
 lib/librte_ether/rte_ethdev_core.h |  2 +-
 4 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 6ac3f8c..1d3898b 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -120,8 +120,8 @@ static int i40evf_dev_rss_hash_update(struct rte_eth_dev *dev,
 static int i40evf_dev_rss_hash_conf_get(struct rte_eth_dev *dev,
 					struct rte_eth_rss_conf *rss_conf);
 static int i40evf_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
-static void i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
-					struct ether_addr *mac_addr);
+static int i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
+				       struct ether_addr *mac_addr);
 static int
 i40evf_dev_rx_queue_intr_enable(struct rte_eth_dev *dev, uint16_t queue_id);
 static int
@@ -2655,7 +2655,7 @@ static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
 	return ret;
 }
 
-static void
+static int
 i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
 			    struct ether_addr *mac_addr)
 {
@@ -2664,15 +2664,17 @@ static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
 
 	if (!is_valid_assigned_ether_addr(mac_addr)) {
 		PMD_DRV_LOG(ERR, "Tried to set invalid MAC address.");
-		return;
+		return -1;
 	}
 
 	if (vf->flags & I40E_FLAG_VF_MAC_BY_PF)
-		return;
+		return -1;
 
 	i40evf_del_mac_addr_by_addr(dev, (struct ether_addr *)hw->mac.addr);
 
 	i40evf_add_mac_addr(dev, mac_addr, 0, 0);
 
 	ether_addr_copy(mac_addr, (struct ether_addr *)hw->mac.addr);
+
+	return 0;
 }
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index f285ba2..869c960 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -2816,6 +2816,7 @@ struct rte_eth_dev *
 rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct ether_addr *addr)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
@@ -2825,11 +2826,13 @@ struct rte_eth_dev *
 	dev = &rte_eth_devices[port_id];
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
 
+	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
+	if (ret)
+		return -EPERM;
+
 	/* 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;
 }
 
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index ccf4a15..e3355cc 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2616,6 +2616,7 @@ int rte_eth_dev_mac_addr_add(uint16_t port_id, struct ether_addr *mac_addr,
  *   - (-ENOTSUP) if hardware doesn't support.
  *   - (-ENODEV) if *port* invalid.
  *   - (-EINVAL) if MAC address is invalid.
+ *   - (-EPERM) if the default MAC address cannot be changed.
  */
 int rte_eth_dev_default_mac_addr_set(uint16_t port_id,
 		struct ether_addr *mac_addr);
diff --git a/lib/librte_ether/rte_ethdev_core.h b/lib/librte_ether/rte_ethdev_core.h
index f44b40e..91fa1c0 100644
--- a/lib/librte_ether/rte_ethdev_core.h
+++ b/lib/librte_ether/rte_ethdev_core.h
@@ -251,7 +251,7 @@ typedef int (*eth_mac_addr_add_t)(struct rte_eth_dev *dev,
 				  uint32_t vmdq);
 /**< @internal Set a MAC address into Receive Address Address Register */
 
-typedef void (*eth_mac_addr_set_t)(struct rte_eth_dev *dev,
+typedef int (*eth_mac_addr_set_t)(struct rte_eth_dev *dev,
 				  struct ether_addr *mac_addr);
 /**< @internal Set a MAC address into Receive Address Address Register */
 
-- 
1.9.3

Re: [RFC] lib/librte_ether: add a return value for MAC set ops

[Olivier Matz]

Hi Wenzhuo,

On Thu, Jan 25, 2018 at 10:46:57AM +0800, Wenzhuo Lu wrote:
> Setting the default MAC address may fail on many NICs.
> But the ops return void. So, even it failed, RTE changes
> the MAC address and APP doesn't know the failure.
> 
> It's not real patch, just show the idea to add a return
> value for the ops.

Thank you for taking care of this. I had some plans to work
on it too, as discussed here:
https://dpdk.org/dev/patchwork/patch/32284/

I noticed that some PMDs try to manage the error case by
themselve by overriding the mac address in ethdev->data.
See for instance qede_mac_addr_set().

With your patch, these PMDs should be modified. No PMD
should change the value of eth_dev->data->mac_addrs.

> BTW,
> Seems we should do the same thing for
> rte_eth_dev_mac_addr_remove as it also has chance to fail
> in PMD layer.

Agree.

> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev_vf.c  | 12 +++++++-----
>  lib/librte_ether/rte_ethdev.c      |  7 +++++--
>  lib/librte_ether/rte_ethdev.h      |  1 +
>  lib/librte_ether/rte_ethdev_core.h |  2 +-
>  4 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
> index 6ac3f8c..1d3898b 100644
> --- a/drivers/net/i40e/i40e_ethdev_vf.c
> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> @@ -120,8 +120,8 @@ static int i40evf_dev_rss_hash_update(struct rte_eth_dev *dev,
>  static int i40evf_dev_rss_hash_conf_get(struct rte_eth_dev *dev,
>  					struct rte_eth_rss_conf *rss_conf);
>  static int i40evf_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
> -static void i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
> -					struct ether_addr *mac_addr);
> +static int i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
> +				       struct ether_addr *mac_addr);
>  static int
>  i40evf_dev_rx_queue_intr_enable(struct rte_eth_dev *dev, uint16_t queue_id);
>  static int
> @@ -2655,7 +2655,7 @@ static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
>  	return ret;
>  }
>  
> -static void
> +static int
>  i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
>  			    struct ether_addr *mac_addr)
>  {
> @@ -2664,15 +2664,17 @@ static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
>  
>  	if (!is_valid_assigned_ether_addr(mac_addr)) {
>  		PMD_DRV_LOG(ERR, "Tried to set invalid MAC address.");
> -		return;
> +		return -1;
>  	}
>  
>  	if (vf->flags & I40E_FLAG_VF_MAC_BY_PF)
> -		return;
> +		return -1;
>  

I wonder if returning -errno wouldn't be better?


>  	i40evf_del_mac_addr_by_addr(dev, (struct ether_addr *)hw->mac.addr);
>  
>  	i40evf_add_mac_addr(dev, mac_addr, 0, 0);
>  
>  	ether_addr_copy(mac_addr, (struct ether_addr *)hw->mac.addr);
> +
> +	return 0;
>  }
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index f285ba2..869c960 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -2816,6 +2816,7 @@ struct rte_eth_dev *
>  rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct ether_addr *addr)
>  {
>  	struct rte_eth_dev *dev;
> +	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
> @@ -2825,11 +2826,13 @@ struct rte_eth_dev *
>  	dev = &rte_eth_devices[port_id];
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
>  
> +	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
> +	if (ret)
> +		return -EPERM;
> +

And here, we could return the return code of the PMD ops instead of
-EPERM, I think it can give better idea of the error cause.


>  	/* 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;
>  }
>  
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index ccf4a15..e3355cc 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2616,6 +2616,7 @@ int rte_eth_dev_mac_addr_add(uint16_t port_id, struct ether_addr *mac_addr,
>   *   - (-ENOTSUP) if hardware doesn't support.
>   *   - (-ENODEV) if *port* invalid.
>   *   - (-EINVAL) if MAC address is invalid.
> + *   - (-EPERM) if the default MAC address cannot be changed.

Here, I suggest:

  - (-errno) for any other error returned by the PMD


Thanks
Olivier

Re: [RFC] lib/librte_ether: add a return value for MAC set ops

[Lu, Wenzhuo]

Hi Olivier,


> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Thursday, January 25, 2018 6:40 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set
> ops
> 
> Hi Wenzhuo,
> 
> On Thu, Jan 25, 2018 at 10:46:57AM +0800, Wenzhuo Lu wrote:
> > Setting the default MAC address may fail on many NICs.
> > But the ops return void. So, even it failed, RTE changes the MAC
> > address and APP doesn't know the failure.
> >
> > It's not real patch, just show the idea to add a return value for the
> > ops.
> 
> Thank you for taking care of this. I had some plans to work on it too, as
> discussed here:
> https://dpdk.org/dev/patchwork/patch/32284/
Sorry, didn't notice that. Glad to know you're working on that. So you'll continue, right?

> 
> I noticed that some PMDs try to manage the error case by themselve by
> overriding the mac address in ethdev->data.
> See for instance qede_mac_addr_set().
> 
> With your patch, these PMDs should be modified. No PMD should change
> the value of eth_dev->data->mac_addrs.
Yes. Every NIC's PMD need to be checked to make sure its behavior is by design.

> 
> > BTW,
> > Seems we should do the same thing for
> > rte_eth_dev_mac_addr_remove as it also has chance to fail in PMD
> > layer.
> 
> Agree.
> 
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > ---
> >  drivers/net/i40e/i40e_ethdev_vf.c  | 12 +++++++-----
> >  lib/librte_ether/rte_ethdev.c      |  7 +++++--
> >  lib/librte_ether/rte_ethdev.h      |  1 +
> >  lib/librte_ether/rte_ethdev_core.h |  2 +-
> >  4 files changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > b/drivers/net/i40e/i40e_ethdev_vf.c
> > index 6ac3f8c..1d3898b 100644
> > --- a/drivers/net/i40e/i40e_ethdev_vf.c
> > +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> > @@ -120,8 +120,8 @@ static int i40evf_dev_rss_hash_update(struct
> > rte_eth_dev *dev,  static int i40evf_dev_rss_hash_conf_get(struct
> rte_eth_dev *dev,
> >  					struct rte_eth_rss_conf *rss_conf);
> static int
> > i40evf_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu); -static
> > void i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
> > -					struct ether_addr *mac_addr);
> > +static int i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
> > +				       struct ether_addr *mac_addr);
> >  static int
> >  i40evf_dev_rx_queue_intr_enable(struct rte_eth_dev *dev, uint16_t
> > queue_id);  static int @@ -2655,7 +2655,7 @@ static int
> > eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
> >  	return ret;
> >  }
> >
> > -static void
> > +static int
> >  i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
> >  			    struct ether_addr *mac_addr)
> >  {
> > @@ -2664,15 +2664,17 @@ static int eth_i40evf_pci_remove(struct
> > rte_pci_device *pci_dev)
> >
> >  	if (!is_valid_assigned_ether_addr(mac_addr)) {
> >  		PMD_DRV_LOG(ERR, "Tried to set invalid MAC address.");
> > -		return;
> > +		return -1;
> >  	}
> >
> >  	if (vf->flags & I40E_FLAG_VF_MAC_BY_PF)
> > -		return;
> > +		return -1;
> >
> 
> I wonder if returning -errno wouldn't be better?
Agree.
After a quick glance at the PMD code, the code of every NIC need to be changed. I'm not familiar with much of them, so a little scared about figuring out the error number :)  But totally agree that the erron is good for the APP to know what's the problem.
The same below.

> 
> 
> >  	i40evf_del_mac_addr_by_addr(dev, (struct ether_addr *)hw-
> >mac.addr);
> >
> >  	i40evf_add_mac_addr(dev, mac_addr, 0, 0);
> >
> >  	ether_addr_copy(mac_addr, (struct ether_addr *)hw->mac.addr);
> > +
> > +	return 0;
> >  }
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index f285ba2..869c960 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -2816,6 +2816,7 @@ struct rte_eth_dev *
> > rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct ether_addr
> > *addr)  {
> >  	struct rte_eth_dev *dev;
> > +	int ret;
> >
> >  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> >
> > @@ -2825,11 +2826,13 @@ struct rte_eth_dev *
> >  	dev = &rte_eth_devices[port_id];
> >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -
> ENOTSUP);
> >
> > +	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
> > +	if (ret)
> > +		return -EPERM;
> > +
> 
> And here, we could return the return code of the PMD ops instead of -
> EPERM, I think it can give better idea of the error cause.
> 
> 
> >  	/* 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;
> >  }
> >
> > diff --git a/lib/librte_ether/rte_ethdev.h
> > b/lib/librte_ether/rte_ethdev.h index ccf4a15..e3355cc 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -2616,6 +2616,7 @@ int rte_eth_dev_mac_addr_add(uint16_t port_id,
> struct ether_addr *mac_addr,
> >   *   - (-ENOTSUP) if hardware doesn't support.
> >   *   - (-ENODEV) if *port* invalid.
> >   *   - (-EINVAL) if MAC address is invalid.
> > + *   - (-EPERM) if the default MAC address cannot be changed.
> 
> Here, I suggest:
> 
>   - (-errno) for any other error returned by the PMD
> 
> 
> Thanks
> Olivier

Re: [RFC] lib/librte_ether: add a return value for MAC set ops

[Ferruh Yigit]

On 1/26/2018 2:19 AM, Lu, Wenzhuo wrote:
> Hi Olivier,
> 
> 
>> -----Original Message-----
>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>> Sent: Thursday, January 25, 2018 6:40 PM
>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set
>> ops
>>
>> Hi Wenzhuo,
>>
>> On Thu, Jan 25, 2018 at 10:46:57AM +0800, Wenzhuo Lu wrote:
>>> Setting the default MAC address may fail on many NICs.
>>> But the ops return void. So, even it failed, RTE changes the MAC
>>> address and APP doesn't know the failure.
>>>
>>> It's not real patch, just show the idea to add a return value for the
>>> ops.
>>
>> Thank you for taking care of this. I had some plans to work on it too, as
>> discussed here:
>> https://dpdk.org/dev/patchwork/patch/32284/
> Sorry, didn't notice that. Glad to know you're working on that. So you'll continue, right?

Plan was send PMD fixes and deprecation notice on this release and send fix next
release [1].

I think PMD fixes are merged now, but deprecation notice not send yet.

[1]
https://dpdk.org/ml/archives/dev/2018-January/085195.html

> 
>>
>> I noticed that some PMDs try to manage the error case by themselve by
>> overriding the mac address in ethdev->data.
>> See for instance qede_mac_addr_set().
>>
>> With your patch, these PMDs should be modified. No PMD should change
>> the value of eth_dev->data->mac_addrs.
> Yes. Every NIC's PMD need to be checked to make sure its behavior is by design.
> 
>>
>>> BTW,
>>> Seems we should do the same thing for
>>> rte_eth_dev_mac_addr_remove as it also has chance to fail in PMD
>>> layer.
>>
>> Agree.
>>
>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>> ---
>>>  drivers/net/i40e/i40e_ethdev_vf.c  | 12 +++++++-----
>>>  lib/librte_ether/rte_ethdev.c      |  7 +++++--
>>>  lib/librte_ether/rte_ethdev.h      |  1 +
>>>  lib/librte_ether/rte_ethdev_core.h |  2 +-
>>>  4 files changed, 14 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
>>> b/drivers/net/i40e/i40e_ethdev_vf.c
>>> index 6ac3f8c..1d3898b 100644
>>> --- a/drivers/net/i40e/i40e_ethdev_vf.c
>>> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
>>> @@ -120,8 +120,8 @@ static int i40evf_dev_rss_hash_update(struct
>>> rte_eth_dev *dev,  static int i40evf_dev_rss_hash_conf_get(struct
>> rte_eth_dev *dev,
>>>  					struct rte_eth_rss_conf *rss_conf);
>> static int
>>> i40evf_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu); -static
>>> void i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
>>> -					struct ether_addr *mac_addr);
>>> +static int i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
>>> +				       struct ether_addr *mac_addr);
>>>  static int
>>>  i40evf_dev_rx_queue_intr_enable(struct rte_eth_dev *dev, uint16_t
>>> queue_id);  static int @@ -2655,7 +2655,7 @@ static int
>>> eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
>>>  	return ret;
>>>  }
>>>
>>> -static void
>>> +static int
>>>  i40evf_set_default_mac_addr(struct rte_eth_dev *dev,
>>>  			    struct ether_addr *mac_addr)
>>>  {
>>> @@ -2664,15 +2664,17 @@ static int eth_i40evf_pci_remove(struct
>>> rte_pci_device *pci_dev)
>>>
>>>  	if (!is_valid_assigned_ether_addr(mac_addr)) {
>>>  		PMD_DRV_LOG(ERR, "Tried to set invalid MAC address.");
>>> -		return;
>>> +		return -1;
>>>  	}
>>>
>>>  	if (vf->flags & I40E_FLAG_VF_MAC_BY_PF)
>>> -		return;
>>> +		return -1;
>>>
>>
>> I wonder if returning -errno wouldn't be better?
> Agree.
> After a quick glance at the PMD code, the code of every NIC need to be changed. I'm not familiar with much of them, so a little scared about figuring out the error number :)  But totally agree that the erron is good for the APP to know what's the problem.
> The same below.
> 
>>
>>
>>>  	i40evf_del_mac_addr_by_addr(dev, (struct ether_addr *)hw-
>>> mac.addr);
>>>
>>>  	i40evf_add_mac_addr(dev, mac_addr, 0, 0);
>>>
>>>  	ether_addr_copy(mac_addr, (struct ether_addr *)hw->mac.addr);
>>> +
>>> +	return 0;
>>>  }
>>> diff --git a/lib/librte_ether/rte_ethdev.c
>>> b/lib/librte_ether/rte_ethdev.c index f285ba2..869c960 100644
>>> --- a/lib/librte_ether/rte_ethdev.c
>>> +++ b/lib/librte_ether/rte_ethdev.c
>>> @@ -2816,6 +2816,7 @@ struct rte_eth_dev *
>>> rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct ether_addr
>>> *addr)  {
>>>  	struct rte_eth_dev *dev;
>>> +	int ret;
>>>
>>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>
>>> @@ -2825,11 +2826,13 @@ struct rte_eth_dev *
>>>  	dev = &rte_eth_devices[port_id];
>>>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -
>> ENOTSUP);
>>>
>>> +	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>>> +	if (ret)
>>> +		return -EPERM;
>>> +
>>
>> And here, we could return the return code of the PMD ops instead of -
>> EPERM, I think it can give better idea of the error cause.
>>
>>
>>>  	/* 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;
>>>  }
>>>
>>> diff --git a/lib/librte_ether/rte_ethdev.h
>>> b/lib/librte_ether/rte_ethdev.h index ccf4a15..e3355cc 100644
>>> --- a/lib/librte_ether/rte_ethdev.h
>>> +++ b/lib/librte_ether/rte_ethdev.h
>>> @@ -2616,6 +2616,7 @@ int rte_eth_dev_mac_addr_add(uint16_t port_id,
>> struct ether_addr *mac_addr,
>>>   *   - (-ENOTSUP) if hardware doesn't support.
>>>   *   - (-ENODEV) if *port* invalid.
>>>   *   - (-EINVAL) if MAC address is invalid.
>>> + *   - (-EPERM) if the default MAC address cannot be changed.
>>
>> Here, I suggest:
>>
>>   - (-errno) for any other error returned by the PMD
>>
>>
>> Thanks
>> Olivier

Re: [RFC] lib/librte_ether: add a return value for MAC set ops

[Olivier Matz]

Hi,

On Fri, Jan 26, 2018 at 04:54:43PM +0000, Ferruh Yigit wrote:
> On 1/26/2018 2:19 AM, Lu, Wenzhuo wrote:
> > Hi Olivier,
> > 
> > 
> >> -----Original Message-----
> >> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> >> Sent: Thursday, January 25, 2018 6:40 PM
> >> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set
> >> ops
> >>
> >> Hi Wenzhuo,
> >>
> >> On Thu, Jan 25, 2018 at 10:46:57AM +0800, Wenzhuo Lu wrote:
> >>> Setting the default MAC address may fail on many NICs.
> >>> But the ops return void. So, even it failed, RTE changes the MAC
> >>> address and APP doesn't know the failure.
> >>>
> >>> It's not real patch, just show the idea to add a return value for the
> >>> ops.
> >>
> >> Thank you for taking care of this. I had some plans to work on it too, as
> >> discussed here:
> >> https://dpdk.org/dev/patchwork/patch/32284/
> > Sorry, didn't notice that. Glad to know you're working on that. So you'll continue, right?
> 
> Plan was send PMD fixes and deprecation notice on this release and send fix next
> release [1].
> 
> I think PMD fixes are merged now, but deprecation notice not send yet.
> 
> [1]
> https://dpdk.org/ml/archives/dev/2018-January/085195.html

Yes, I'll send the deprecation notice today.

Re: [RFC] lib/librte_ether: add a return value for MAC set ops

[Thomas Monjalon]

25/01/2018 03:46, Wenzhuo Lu:
> Setting the default MAC address may fail on many NICs.
> But the ops return void. So, even it failed, RTE changes
> the MAC address and APP doesn't know the failure.
> 
> It's not real patch, just show the idea to add a return
> value for the ops.
> 
> BTW,
> Seems we should do the same thing for
> rte_eth_dev_mac_addr_remove as it also has chance to fail
> in PMD layer.
> 
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

Superseded by "ethdev: return diagnostic when setting MAC address"
	http://dpdk.org/commit/caccf8b318