diff mbox series

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

Message ID 20220214112541.29782-6-spinler@cesnet.cz (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [1/6] net/nfb: add missing libfdt dependency for build |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS

Commit Message

Martin Spinler Feb. 14, 2022, 11:25 a.m. UTC 
  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(-)

Comments

Ferruh Yigit Feb. 14, 2022, 1:37 p.m. UTC | #1 
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.

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

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

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

>   	data->promiscuous = nfb_eth_promiscuous_get(dev);
>   	data->all_multicast = nfb_eth_allmulticast_get(dev);
Martin Spinler Feb. 14, 2022, 4:53 p.m. UTC | #2 
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);
>
Ferruh Yigit Feb. 14, 2022, 5:54 p.m. UTC | #3 
On 2/14/2022 4:53 PM, Martin Spinler wrote:
> 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.
> 

Got it.

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

Thanks.

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

I see the usage now, that looks OK.

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

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?

>>
>>>    	data->promiscuous = nfb_eth_promiscuous_get(dev);
>>>    	data->all_multicast = nfb_eth_allmulticast_get(dev);
>>
>
Martin Spinler Feb. 15, 2022, 1:02 p.m. UTC | #4 
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);
> > > 
> >
diff mbox series

Patch

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;
+
 	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);
 
-	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);
 
 	data->promiscuous = nfb_eth_promiscuous_get(dev);
 	data->all_multicast = nfb_eth_allmulticast_get(dev);