On 11/30/2017 11:49 AM, Vipin Varghese wrote:
> One of the uses cases from customer site is use TAP PMD to intake user
> specific MAC address during probe. This allows applications make use
> of interfaces with desired MAC.
>
> Extending MAC argumentinfrastructure for tap PMD; we pass custom MAC
> address in string format (sample - 11:22:33:44:55:66).
Overall lgtm, please check a few comments below.
>
> Signed-off-by: Vipin Varghese <vipin.vargh...@intel.com>
> ---
> drivers/net/tap/rte_eth_tap.c | 56
> +++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 52 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/tap/rte_eth_tap.c
> b/drivers/net/tap/rte_eth_tap.c index 6b27679..0c53458 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -81,6 +81,8 @@
> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0) #define
> FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
>
> +static unsigned char user_mac[ETHER_ADDR_LEN];
> +
> static struct rte_vdev_driver pmd_tap_drv;
>
> static const char *valid_arguments[] = { @@ -1291,13 +1293,20 @@ enum
> ioctl_mode {
> pmd->txq[i].fd = -1;
> }
>
> - if (fixed_mac_type) {
> + if (fixed_mac_type == 1) {
Instead of hardcoded type values 1 & 2, can you please use macros?
>> Ok, Adding MACROs MAC_STRING_NULL, MAC_STRING_FIXED and MAC_STRING_USER
> /* fixed mac = 00:64:74:61:70:<iface_idx> */
> static int iface_idx;
> char mac[ETHER_ADDR_LEN] = "\0dtap";
>
> mac[ETHER_ADDR_LEN - 1] = iface_idx++;
> rte_memcpy(&pmd->eth_addr, mac, ETHER_ADDR_LEN);
> + } else if (fixed_mac_type == 2) {
> + /* user mac is recieved */
s/recieved/received
>> Ok
> + RTE_LOG(INFO, PMD,
> + "Using user MAC (%02x:%02x:%02x:%02x:%02x:%02x)\n",
> + user_mac[0], user_mac[1], user_mac[2],
> + user_mac[3], user_mac[4], user_mac[5]);
> + rte_memcpy(&pmd->eth_addr, user_mac, ETHER_ADDR_LEN);
> } else {
> eth_random_addr((uint8_t *)&pmd->eth_addr);
> }
> @@ -1471,9 +1480,48 @@ enum ioctl_mode {
> const char *value,
> void *extra_args)
> {
> - if (value &&
> - !strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED)))
> - *(int *)extra_args = 1;
> + char macTemp[20], *macByte = NULL;
> + unsigned int index = 0;
> +
> + if (value) {
> + if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
> + strlen(ETH_TAP_MAC_FIXED))) {
> + *(int *)extra_args = 1;
> + } else {
> + RTE_LOG(INFO, PMD, "User shared MAC param (%s)\n",
> + value);
Is this log needs to be in INFO level, since there is already and info level
log when MAC set, what about making this debug?
>> Ok
> +
> + /* desired format aa:bb:cc:dd:ee:ff:11 */
Can you please update RTE_PMD_REGISTER_PARAM_STRING with new param, put
expected format info there as well?
>> Ok
> + if (strlen(value) == 17) {
> + strncpy(macTemp, value, 18);
> +
> + macByte = strtok(macTemp, ":");
> + while ((macByte != NULL) &&
> + (strspn(macByte,
> "0123456789ABCDEFabcdef")) &&
> + (strspn((macByte + 1),
> "0123456789ABCDEFabcdef")) &&
> + (strlen(macByte) == 2)) {
> + user_mac[index] = strtoul(macByte,
> NULL, 16);
> + macByte = strtok(NULL, ":");
> + index += 1;
> + }
I would extract the string to mac logic into its own function, but up to you.
>> not done
> +
> + if (index != 6) {
> + RTE_LOG(ERR, PMD, " failure in MAC
> value (%s) at %u\n",
> + macByte, index + 1);
> + return -1;
And this is not related to this patch, but just as reminder, when a virtual
driver probe fails vdev bus stops probing with an error, so all remaining
virtual devices are not probed, in case one might want to fix ;)
> + }
> +
> + RTE_LOG(DEBUG, PMD, " User MAC (%s)
> considered\n",
"User defined MAC" ? And can you please add an port identifier, port_id or
device name or interface name, for the case there are multiple tap device to
identify which one get user defined MAC.
VV> not done, each device in DPDK rte_eal_init is sequentially scanned and
probed, hence log starts with 'iface,[parameters]', then probe, create and init
is called for TAP PMD one after another.
With minimalistic logging style the current format is kept.
> + value);
> + *(int *)extra_args = 2;
> + } else {
> + RTE_LOG(ERR, PMD, " Mismatch on format for
> (%s)\n",
> + value);
> + return -1;
> + }
> + }
> + }
> +
> return 0;
> }
>
>
