Re: [dpdk-dev] [PATCH 1/3] kni: support for MAC addr change

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/30/2017 3:46 AM, Hemant Agrawal wrote:
> This patch adds following:
> 1. Option to configure the mac address during create. Generate random
>    address only if the user has not provided any valid address.
> 2. Inform usespace, if mac address is being changed in linux.
> 3. Implement default handling of mac address change in the corresponding
>    ethernet device.>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

Overall lgtm, there are a few issues commented below.

Thanks,
ferruh

<...>

> @@ -269,11 +275,15 @@ The code for allocating the kernel NIC interfaces for a specific port is as foll
>                      conf.addr = dev_info.pci_dev->addr;
>                      conf.id = dev_info.pci_dev->id;
>  
> +                    /* Get the interface default mac address */
> +                    rte_eth_macaddr_get(port_id, struct ether_addr *)&conf.mac_addr);

a parentheses is missing, good to fix although this is document :)

<...>

> @@ -587,3 +603,26 @@ Currently, setting a new MTU and configuring the network interface (up/ down) ar
>              RTE_LOG(ERR, APP, "Failed to start port %d\n", port_id);
>          return ret;
>      }
> +
> +    /* Callback for request of configuring device mac address */
> +
> +    static int
> +    kni_config_mac_address(uint16_t port_id, uint8_t mac_addr[])
> +    {
> +        int ret = 0;
> +
> +        if (port_id >= rte_eth_dev_count() || port_id >= RTE_MAX_ETHPORTS) {
> +                RTE_LOG(ERR, KNI, "Invalid port id %d\n", port_id);
> +                return -EINVAL;
> +        }
> +
> +        RTE_LOG(INFO, KNI, "Configure mac address of %d", port_id);
> +        /* Configure network interface mac address */
> +        ret = rte_eth_dev_default_mac_addr_set(port_id,
> +                                               (struct ether_addr *)mac_addr);
> +        if (ret < 0)
> +                RTE_LOG(ERR, KNI, "Failed to config mac_addr for port %d\n",
> +                        port_id);
> +
> +        return ret;
> +    }


It is hard to maintain code in doc, I am aware other related code is already in
document but what do you think keeping this minimal, like:

    static int
    kni_config_mac_address(uint16_t port_id, uint8_t mac_addr[])
    {
       ....
    }


<...>

> @@ -559,6 +583,14 @@ rte_kni_handle_request(struct rte_kni *kni)
>  			req->result = kni->ops.config_network_if(\
>  					kni->ops.port_id, req->if_up);
>  		break;
> +	case RTE_KNI_REQ_CHANGE_MAC_ADDR: /* Change MAC Address */
> +		if (kni->ops.config_mac_address)
> +			req->result = kni->ops.config_mac_address(
> +					kni->ops.port_id, req->mac_addr);
> +		else
> +			req->result = kni_config_mac_address(
> +					kni->ops.port_id, req->mac_addr);

ops.port_id can be unset if there is no physically backing device the kni
interface. And I guess for that case port_id will be 0 and it will corrupt other
interface's data. There needs to find a way to handle the port_id not set case.

Since kni sample always creates a KNI interface backed by pyhsical device, this
is not an issue for kni sample app but please think about kni pmd case.

<...>

> @@ -87,6 +91,7 @@ struct rte_kni_conf {
>  	unsigned mbuf_size; /* mbuf size */
>  	struct rte_pci_addr addr;
>  	struct rte_pci_id id;
> +	char mac_addr[ETHER_ADDR_LEN]; /* MAC address assigned to KNI */
>  
>  	__extension__
>  	uint8_t force_bind : 1; /* Flag to bind kernel thread */

"struct rte_kni_conf" is a public struct. Adding a variable into the middle of
the struct will break the ABI.
But I think it is OK to add to the end, unless struct is not used as array.

<...>