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

[Lu, Wenzhuo]

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?

> 
> 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