[dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops
Ferruh Yigit
ferruh.yigit at intel.com
Fri Jan 26 17:54:43 CET 2018
On 1/26/2018 2:19 AM, Lu, Wenzhuo wrote:
> Hi Olivier,
>
>
>> -----Original Message-----
>> From: Olivier Matz [mailto:olivier.matz at 6wind.com]
>> Sent: Thursday, January 25, 2018 6:40 PM
>> To: Lu, Wenzhuo <wenzhuo.lu at intel.com>
>> Cc: dev at 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 at 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
More information about the dev
mailing list