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

[Martin Spinler <spinler@cesnet.cz>]

On Mon, 2022-02-14 at 13:37 +0000, Ferruh Yigit wrote:
> On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote:
> > From: Martin Spinler <spinler@cesnet.cz>
> > 
> > Extend the eth_dev_ops by add/remove MAC address functions.
> > 
> > Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> > ---
> >   drivers/net/nfb/nfb_ethdev.c | 69 ++++++++++++++++++++++++++++++------
> >   1 file changed, 58 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
> > index 5d503e131a..7dec8e022e 100644
> > --- a/drivers/net/nfb/nfb_ethdev.c
> > +++ b/drivers/net/nfb/nfb_ethdev.c
> > @@ -214,7 +214,19 @@ static int
> >   nfb_eth_dev_info(struct rte_eth_dev *dev,
> >   	struct rte_eth_dev_info *dev_info)
> >   {
> > -	dev_info->max_mac_addrs = 1;
> > +	uint16_t i;
> > +	uint32_t max_mac_addrs;
> > +	struct pmd_internals *internals = dev->data->dev_private;
> > +
> > +	dev_info->max_mac_addrs = (uint32_t)-1;
> > +	for (i = 0; i < internals->max_rxmac; i++) {
> > +		max_mac_addrs = nc_rxmac_mac_address_count(internals->rxmac[i]);
> > +		dev_info->max_mac_addrs = RTE_MIN(max_mac_addrs,
> > +				dev_info->max_mac_addrs);
> > +	}
> > +	if (internals->max_rxmac == 0)
> > +		dev_info->max_mac_addrs = 0;
> > +
> 
> Not sure if 'max_mac_addrs = 0' is valid value, as far as I can see
> driver allocates memory for at least one MAC address, so there is no
> case that NIC doesn't support any MAC address.

Oh, sorry, I missed that, will be fixed in v2.

> 
> It looks like max_mac_addrs usage is not clear, what is exactly the
> 'max_rxmac' & 'rxmac[]' variables are?

rxmac is a firmware unit which represents the RX side of one physical
Ethernet port (more precisely one logical Eth channel on port) and
handles stats+CRC check+MAC filtering. So this goes through all those
units and finds the rxmac with minimal memory space for MAC addresses
and uses this value.

I'll add few comments in the code to v2.

> 
> >   	dev_info->max_rx_pktlen = (uint32_t)-1;
> >   	dev_info->max_rx_queues = dev->data->nb_rx_queues;
> >   	dev_info->max_tx_queues = dev->data->nb_tx_queues;
> > @@ -376,6 +388,18 @@ nfb_eth_dev_set_link_down(struct rte_eth_dev *dev)
> >   	return 0;
> >   }
> >   
> > +static uint64_t
> > +nfb_eth_mac_addr_conv(struct rte_ether_addr *mac_addr)
> > +{
> > +	int i;
> > +	uint64_t res = 0;
> > +	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
> > +		res <<= 8;
> > +		res |= mac_addr->addr_bytes[i] & 0xFF;
> > +	}
> > +	return res;
> > +}
> > +
> >   /**
> >    * DPDK callback to set primary MAC address.
> >    *
> > @@ -392,26 +416,47 @@ nfb_eth_mac_addr_set(struct rte_eth_dev *dev,
> >   	struct rte_ether_addr *mac_addr)
> >   {
> >   	unsigned int i;
> > -	uint64_t mac = 0;
> > +	uint64_t mac;
> >   	struct rte_eth_dev_data *data = dev->data;
> >   	struct pmd_internals *internals = (struct pmd_internals *)
> >   		data->dev_private;
> >   
> > -	if (!rte_is_valid_assigned_ether_addr(mac_addr))
> > -		return -EINVAL;
> > +	mac = nfb_eth_mac_addr_conv(mac_addr);
> > +	for (i = 0; i < internals->max_rxmac; ++i)
> > +		nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1);
> 
> This functions is to set default MAC address 'data->mac_addrs[0]',
> why same MAC set multiple times in a loop?

Unfortunately, currently we don't have firmware support for proper
separate port configuration, because DMA queues aren't bound to
specific rxmac and traffic can be mixed between them. So I think the
best way in this case is to write the same MAC address into all of the
rxmacs inside firmware.

> 
> >   
> > -	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
> > -		mac <<= 8;
> > -		mac |= mac_addr->addr_bytes[i] & 0xFF;
> > -	}
> > +	return 0;
> > +}
> >   
> > +static int
> > +nfb_eth_mac_addr_add(struct rte_eth_dev *dev,
> > +	struct rte_ether_addr *mac_addr, uint32_t index, uint32_t pool __rte_unused)
> > +{
> > +	unsigned int i;
> > +	uint64_t mac = 0;
> > +	struct rte_eth_dev_data *data = dev->data;
> > +	struct pmd_internals *internals = (struct pmd_internals *)
> > +		data->dev_private;
> > +
> > +	mac = nfb_eth_mac_addr_conv(mac_addr);
> >   	for (i = 0; i < internals->max_rxmac; ++i)
> > -		nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1);
> > +		nc_rxmac_set_mac(internals->rxmac[i], index, mac, 1);
> >   
> > -	rte_ether_addr_copy(mac_addr, data->mac_addrs);
> >   	return 0;
> >   }
> >   
> > +static void
> > +nfb_eth_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
> > +{
> > +	unsigned int i;
> > +	struct rte_eth_dev_data *data = dev->data;
> > +	struct pmd_internals *internals = (struct pmd_internals *)
> > +		data->dev_private;
> > +
> > +	for (i = 0; i < internals->max_rxmac; ++i)
> > +		nc_rxmac_set_mac(internals->rxmac[i], index, 0, 0);
> > +}
> > +
> >   static const struct eth_dev_ops ops = {
> >   	.dev_start = nfb_eth_dev_start,
> >   	.dev_stop = nfb_eth_dev_stop,
> > @@ -436,6 +481,8 @@ static const struct eth_dev_ops ops = {
> >   	.stats_get = nfb_eth_stats_get,
> >   	.stats_reset = nfb_eth_stats_reset,
> >   	.mac_addr_set = nfb_eth_mac_addr_set,
> > +	.mac_addr_add = nfb_eth_mac_addr_add,
> > +	.mac_addr_remove = nfb_eth_mac_addr_remove,
> >   };
> >   
> >   /**
> > @@ -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?

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