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

[Hemant Agrawal]

Hi Ferruh,

On 12/23/2017 3:25 AM, Ferruh Yigit wrote:

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.agra...@nxp.com>

Overall lgtm, there are a few issues commented below.

Thanks,
ferruh

<...>

@@ -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[])
    {
       ....
    }

agree.

<...>

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

got it. I will set it as UINT16_MAX and check for it.

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.

<...>

yes. adding it into middle breaks the ABIs. However adding into end is ok.