Re: [PATCH 1/3] kni: support for MAC addr change
From: Ferruh Yigit <hidden>
Date: 2017-12-22 21:55:54
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 <redacted>
Overall lgtm, there are a few issues commented below. Thanks, ferruh <...>
quoted hunk ↗ jump to hunk
@@ -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 :) <...>
quoted hunk ↗ jump to hunk
@@ -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[])
{
....
}
<...>
quoted hunk ↗ jump to hunk
@@ -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. <...>
quoted hunk ↗ jump to hunk
@@ -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. <...>