[dpdk-dev] replay MAC address in rte_eth_dev_config_restore()

[dpdk-dev] replay MAC address in rte_eth_dev_config_restore()

[Steve Shin (jonshin)]

Hi,

I have a question on MAC address replay logic in rte_eth_dev_config_restore():

lib/librte_ether/rte_ethdev.c:
code snippet of rte_eth_dev_config_restore()
~~
        /* replay MAC address configuration */
        for (i = 0; i < dev_info.max_mac_addrs; i++) {
                addr = dev->data->mac_addrs[i];

                /* skip zero address */
                if (is_zero_ether_addr(&addr))
                        continue;

                /* add address to the hardware */
                if  (*dev->dev_ops->mac_addr_add &&
                        (dev->data->mac_pool_sel[i] & (1ULL << pool)))
                        (*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
                else {
                        RTE_PMD_DEBUG_TRACE("port %d: MAC address array not supported\n",
                                        port_id);
                        /* exit the loop but not return an error */
                        break;
                }
        }

dev->data->mac_addrs[0] is filled with a HW MAC address (by default) when a device is initialized. But there’s no flag setting for dev->data->mac_pool_sel[0].
As the value of “(dev->data->mac_pool_sel[i] & (1ULL << pool)) “ becomes 0 for i=0, the replay logic will be stopped without processing the rest of MAC addresses in dev->data->mac_addrs[] list.
Shouldn’t ‘break’ statement be replaced with ‘continue’ to process other MACs in the list?

Any feedback would be appreciated.

Regards,
Steve

Re: [dpdk-dev] replay MAC address in rte_eth_dev_config_restore()

[Ferruh Yigit]

On 1/18/2017 4:12 AM, Steve Shin (jonshin) wrote:
> Hi,
> 
> I have a question on MAC address replay logic in rte_eth_dev_config_restore():
> 
> lib/librte_ether/rte_ethdev.c:
> code snippet of rte_eth_dev_config_restore()
> ~~
>         /* replay MAC address configuration */
>         for (i = 0; i < dev_info.max_mac_addrs; i++) {
>                 addr = dev->data->mac_addrs[i];
> 
>                 /* skip zero address */
>                 if (is_zero_ether_addr(&addr))
>                         continue;
> 
>                 /* add address to the hardware */
>                 if  (*dev->dev_ops->mac_addr_add &&
>                         (dev->data->mac_pool_sel[i] & (1ULL << pool)))
>                         (*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
>                 else {
>                         RTE_PMD_DEBUG_TRACE("port %d: MAC address array not supported\n",
>                                         port_id);
>                         /* exit the loop but not return an error */
>                         break;
>                 }
>         }
> 
> dev->data->mac_addrs[0] is filled with a HW MAC address (by default) when a device is initialized. But there’s no flag setting for dev->data->mac_pool_sel[0].
> As the value of “(dev->data->mac_pool_sel[i] & (1ULL << pool)) “ becomes 0 for i=0, the replay logic will be stopped without processing the rest of MAC addresses in dev->data->mac_addrs[] list.
> Shouldn’t ‘break’ statement be replaced with ‘continue’ to process other MACs in the list?

Hi Steve,

Looks like you are right. The commit that pool check added (4bdefaad) says:
"
Fix bug in rte_eth_dev_config_restore function, which will restore all
MAC address to default pool.
"
So intention looks like not stop adding mac_addr loop if mac is not in
pool, but instead not adding a mac address if it is not part of pool.

But pool check seems added later, and initial "break" is added for
"dev_ops->mac_addr_add" check.

Logic can be:

if (dev_ops->mac_addr_add)
	if (mac in pool)
		dev_ops->mac_addr_add()
	else
		continue
else
	break


But of course, instead of checking the dev_ops in a loop for same
device, it makes more sense to remove it out of loop.

Are you planning to send a patch for this issue?
It is easier to discuss code based on patches..

> 
> Any feedback would be appreciated.
> 
> Regards,
> Steve
> 

Re: [dpdk-dev] replay MAC address in rte_eth_dev_config_restore()

[Steve Shin (jonshin)]

Thanks Ferruh for your quick turnaround!

Sure, I’ll send a patch for your review.

Regards,
Steve

On 1/18/17, 10:39 AM, "Ferruh Yigit" <ferruh.yigit@intel.com> wrote:

    On 1/18/2017 4:12 AM, Steve Shin (jonshin) wrote:
    > Hi,
    > 
    > I have a question on MAC address replay logic in rte_eth_dev_config_restore():
    > 
    > lib/librte_ether/rte_ethdev.c:
    > code snippet of rte_eth_dev_config_restore()
    > ~~
    >         /* replay MAC address configuration */
    >         for (i = 0; i < dev_info.max_mac_addrs; i++) {
    >                 addr = dev->data->mac_addrs[i];
    > 
    >                 /* skip zero address */
    >                 if (is_zero_ether_addr(&addr))
    >                         continue;
    > 
    >                 /* add address to the hardware */
    >                 if  (*dev->dev_ops->mac_addr_add &&
    >                         (dev->data->mac_pool_sel[i] & (1ULL << pool)))
    >                         (*dev->dev_ops->mac_addr_add)(dev, &addr, i, pool);
    >                 else {
    >                         RTE_PMD_DEBUG_TRACE("port %d: MAC address array not supported\n",
    >                                         port_id);
    >                         /* exit the loop but not return an error */
    >                         break;
    >                 }
    >         }
    > 
    > dev->data->mac_addrs[0] is filled with a HW MAC address (by default) when a device is initialized. But there’s no flag setting for dev->data->mac_pool_sel[0].
    > As the value of “(dev->data->mac_pool_sel[i] & (1ULL << pool)) “ becomes 0 for i=0, the replay logic will be stopped without processing the rest of MAC addresses in dev->data->mac_addrs[] list.
    > Shouldn’t ‘break’ statement be replaced with ‘continue’ to process other MACs in the list?
    
    Hi Steve,
    
    Looks like you are right. The commit that pool check added (4bdefaad) says:
    "
    Fix bug in rte_eth_dev_config_restore function, which will restore all
    MAC address to default pool.
    "
    So intention looks like not stop adding mac_addr loop if mac is not in
    pool, but instead not adding a mac address if it is not part of pool.
    
    But pool check seems added later, and initial "break" is added for
    "dev_ops->mac_addr_add" check.
    
    Logic can be:
    
    if (dev_ops->mac_addr_add)
    	if (mac in pool)
    		dev_ops->mac_addr_add()
    	else
    		continue
    else
    	break
    
    
    But of course, instead of checking the dev_ops in a loop for same
    device, it makes more sense to remove it out of loop.
    
    Are you planning to send a patch for this issue?
    It is easier to discuss code based on patches..
    
    > 
    > Any feedback would be appreciated.
    > 
    > Regards,
    > Steve
    >