[dpdk-dev] [PATCH 1/3] kni: support for MAC addr change
Ferruh Yigit
ferruh.yigit at intel.com
Fri Dec 22 22:55:52 CET 2017
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 at 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.
<...>
More information about the dev
mailing list