Re: [PATCH 6/6] drivers/nfb: add support for more MAC addresses

[Martin Spinler]

On Mon, 2022-02-14 at 17:54 +0000, Ferruh Yigit wrote:
> > > > @@ -530,7 +577,7 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
> > > >         eth_addr_init.addr_bytes[1] = eth_addr.addr_bytes[1];
> > > >         eth_addr_init.addr_bytes[2] = eth_addr.addr_bytes[2];
> > > >     
> > > > -       nfb_eth_mac_addr_set(dev, &eth_addr_init);
> > > > +       rte_eth_dev_default_mac_addr_set(dev->data->port_id,
> > > > &eth_addr_init);
> > > >     
> > > 
> > > I didn't get this change, why calling the API from the driver,
> > > instead of calling the dev_ops directly as original code did?
> > 
> > The API does all the checks and copies the MAC value into internal
> > data
> > struct, so our code was duplicit. I didn't realize that calling the
> > API
> > could be problem. I assume it should be reverted to the prev form?
> > 
> 
> It is not a problem, and APIs are used because of the reason you
> mentioned in a few places, but for this case I didn't get it,
> what API check is required?
> Is it 'rte_is_valid_assigned_ether_addr()' check? The mac in this
> function ('nfb_eth_dev_init()') is a hardcoded one, instead of
> validity check, why not select a valid MAC at this stage?
> 
> I mean still drop the 'rte_is_valid_assigned_ether_addr()' check
> from 'nfb_eth_mac_addr_set()', but be sure 'eth_addr_init' is
> valid MAC, will it work?

Yes, definitely a better way. Just rte_ether_addr_copy remains.

> 
> > > 
> > > >         data->promiscuous = nfb_eth_promiscuous_get(dev);
> > > >         data->all_multicast = nfb_eth_allmulticast_get(dev);
> > > 
> >