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

[Hemant Agrawal <hemant.agrawal@nxp.com>]

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