[dpdk-dev] [PATCH 1/3] kni: support for MAC addr change
Hemant Agrawal
hemant.agrawal at nxp.com
Tue Dec 26 11:08:13 CET 2017
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.agrawal at 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.
More information about the dev
mailing list