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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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