From: Ferruh Yigit <ferruh.yigit@intel.com>
To: "Lu, Wenzhuo" <wenzhuo.lu@intel.com>,
Olivier Matz <olivier.matz@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops
Date: Fri, 26 Jan 2018 16:54:43 +0000 [thread overview]
Message-ID: <460287e3-db94-b677-e262-4c45a59fb486@intel.com> (raw)
In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC09093B720B5F@shsmsx102.ccr.corp.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
next prev parent reply other threads:[~2018-01-26 16:54 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-25 2:46 Wenzhuo Lu
2018-01-25 10:40 ` Olivier Matz
2018-01-26 2:19 ` Lu, Wenzhuo
2018-01-26 16:54 ` Ferruh Yigit [this message]
2018-01-29 8:25 ` Olivier Matz
2018-05-22 22:47 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=460287e3-db94-b677-e262-4c45a59fb486@intel.com \
--to=ferruh.yigit@intel.com \
--cc=dev@dpdk.org \
--cc=olivier.matz@6wind.com \
--cc=wenzhuo.lu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).