* [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops @ 2018-01-25 2:46 Wenzhuo Lu 2018-01-25 10:40 ` Olivier Matz 2018-05-22 22:47 ` Thomas Monjalon 0 siblings, 2 replies; 6+ messages in thread From: Wenzhuo Lu @ 2018-01-25 2:46 UTC (permalink / raw) To: dev; +Cc: 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops 2018-01-25 2:46 [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops Wenzhuo Lu @ 2018-01-25 10:40 ` Olivier Matz 2018-01-26 2:19 ` Lu, Wenzhuo 2018-05-22 22:47 ` Thomas Monjalon 1 sibling, 1 reply; 6+ messages in thread From: Olivier Matz @ 2018-01-25 10:40 UTC (permalink / raw) To: Wenzhuo Lu; +Cc: dev 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops 2018-01-25 10:40 ` Olivier Matz @ 2018-01-26 2:19 ` Lu, Wenzhuo 2018-01-26 16:54 ` Ferruh Yigit 0 siblings, 1 reply; 6+ messages in thread From: Lu, Wenzhuo @ 2018-01-26 2:19 UTC (permalink / raw) To: Olivier Matz; +Cc: dev 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops 2018-01-26 2:19 ` Lu, Wenzhuo @ 2018-01-26 16:54 ` Ferruh Yigit 2018-01-29 8:25 ` Olivier Matz 0 siblings, 1 reply; 6+ messages in thread From: Ferruh Yigit @ 2018-01-26 16:54 UTC (permalink / raw) To: Lu, Wenzhuo, Olivier Matz; +Cc: dev 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops 2018-01-26 16:54 ` Ferruh Yigit @ 2018-01-29 8:25 ` Olivier Matz 0 siblings, 0 replies; 6+ messages in thread From: Olivier Matz @ 2018-01-29 8:25 UTC (permalink / raw) To: Ferruh Yigit; +Cc: Lu, Wenzhuo, dev 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops 2018-01-25 2:46 [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops Wenzhuo Lu 2018-01-25 10:40 ` Olivier Matz @ 2018-05-22 22:47 ` Thomas Monjalon 1 sibling, 0 replies; 6+ messages in thread From: Thomas Monjalon @ 2018-05-22 22:47 UTC (permalink / raw) To: Wenzhuo Lu; +Cc: dev, olivier.matz 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-05-22 22:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-25 2:46 [dpdk-dev] [RFC] lib/librte_ether: add a return value for MAC set ops Wenzhuo Lu 2018-01-25 10:40 ` Olivier Matz 2018-01-26 2:19 ` Lu, Wenzhuo 2018-01-26 16:54 ` Ferruh Yigit 2018-01-29 8:25 ` Olivier Matz 2018-05-22 22:47 ` Thomas Monjalon
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).