>-----Original Message-----
>From: Somnath Kotur <somnath.ko...@broadcom.com>
>Sent: Thursday, January 7, 2021 2:32 PM
>To: Xueming(Steven) Li <xuemi...@nvidia.com>
>Cc: NBU-Contact-Thomas Monjalon <tho...@monjalon.net>; Ferruh Yigit
><ferruh.yi...@intel.com>; Andrew Rybchenko
><andrew.rybche...@oktetlabs.ru>; Olivier Matz <olivier.m...@6wind.com>;
>Slava Ovsiienko <viachesl...@nvidia.com>; dev <dev@dpdk.org>; Asaf Penso
><as...@nvidia.com>
>Subject: Re: [dpdk-dev] [PATCH v2 1/9] ethdev: refactor representor
>infrastructure
>
>On Wed, Jan 6, 2021 at 9:48 PM Xueming Li <xuemi...@nvidia.com> wrote:
>>
>> To support extended representor syntax, this patch refactor represntor
>Typo in 'representor'
>> infrastructure:
>> 1. introduces representor type enum
>> 2. devargs representor port range extraction from partial value
>>
>> Signed-off-by: Xueming Li <xuemi...@nvidia.com>
>> ---
>> drivers/net/bnxt/bnxt_ethdev.c | 12 ++++
>> drivers/net/enic/enic_ethdev.c | 7 ++
>> drivers/net/i40e/i40e_ethdev.c | 8 +++
>> drivers/net/ixgbe/ixgbe_ethdev.c | 8 +++
>> drivers/net/mlx5/linux/mlx5_os.c | 11 ++++
>> lib/librte_ethdev/ethdev_private.c | 93 ++++++++++++---------------
>> lib/librte_ethdev/ethdev_private.h | 3 -
>> lib/librte_ethdev/rte_class_eth.c | 4 +-
>> lib/librte_ethdev/rte_ethdev.c | 5 +-
>> lib/librte_ethdev/rte_ethdev_driver.h | 7 ++
>> 10 files changed, 98 insertions(+), 60 deletions(-)
>>
>> diff --git a/drivers/net/bnxt/bnxt_ethdev.c
>b/drivers/net/bnxt/bnxt_ethdev.c
>> index 81c8f8d79d..844a6c3c66 100644
>> --- a/drivers/net/bnxt/bnxt_ethdev.c
>> +++ b/drivers/net/bnxt/bnxt_ethdev.c
>> @@ -5520,6 +5520,18 @@ static int bnxt_rep_port_probe(struct
>rte_pci_device *pci_dev,
>> int i, ret = 0;
>> struct rte_kvargs *kvlist = NULL;
>>
>> + if (eth_da->type == RTE_ETH_REPRESENTOR_NONE)
>> + return 0;
>> + if (eth_da->type != RTE_ETH_REPRESENTOR_VF) {
>> + PMD_DRV_LOG(ERR, "unsupported representor type %d\n",
>> + eth_da->type);
>> + return -ENOTSUP;
>> + }
>> + if (eth_da->type != RTE_ETH_REPRESENTOR_VF) {
>Seems like an extra 'if ' condition by mistake? Otherwise there is no
>diff b/n this 'if' condition and the one few lines above?
Thanks, good catch!
>> + PMD_DRV_LOG(ERR, "unsupported representor type %d\n",
>> + eth_da->type);
>> + return -EINVAL;
>> + }
>> num_rep = eth_da->nb_representor_ports;
>> if (num_rep > BNXT_MAX_VF_REPS) {
>> PMD_DRV_LOG(ERR, "nb_representor_ports = %d > %d MAX VF
>REPS\n",
>> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
>> index d041a6bee9..dd085caa93 100644
>> --- a/drivers/net/enic/enic_ethdev.c
>> +++ b/drivers/net/enic/enic_ethdev.c
>> @@ -1303,6 +1303,13 @@ static int eth_enic_pci_probe(struct
>rte_pci_driver *pci_drv __rte_unused,
>> if (retval)
>> return retval;
>> }
>> + if (eth_da.type == RTE_ETH_REPRESENTOR_NONE)
>> + return 0;
>> + if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
>> + ENICPMD_LOG(ERR, "unsupported representor type: %s\n",
>> + pci_dev->device.devargs->args);
>> + return -ENOTSUP;
>> + }
>> retval = rte_eth_dev_create(&pci_dev->device, pci_dev->device.name,
>> sizeof(struct enic),
>> eth_dev_pci_specific_init, pci_dev,
>> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
>> index f54769c29d..05ed2e1079 100644
>> --- a/drivers/net/i40e/i40e_ethdev.c
>> +++ b/drivers/net/i40e/i40e_ethdev.c
>> @@ -640,6 +640,14 @@ eth_i40e_pci_probe(struct rte_pci_driver *pci_drv
>__rte_unused,
>> return retval;
>> }
>>
>> + if (eth_da.type == RTE_ETH_REPRESENTOR_NONE)
>> + return 0;
>> + if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
>> + PMD_DRV_LOG(ERR, "unsupported representor type: %s\n",
>> + pci_dev->device.devargs->args);
>> + return -ENOTSUP;
>> + }
>> +
>> retval = rte_eth_dev_create(&pci_dev->device, pci_dev->device.name,
>> sizeof(struct i40e_adapter),
>> eth_dev_pci_specific_init, pci_dev,
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index 9a47a8b262..9ea0139197 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -1717,6 +1717,14 @@ eth_ixgbe_pci_probe(struct rte_pci_driver
>*pci_drv __rte_unused,
>> } else
>> memset(ð_da, 0, sizeof(eth_da));
>>
>> + if (eth_da.type == RTE_ETH_REPRESENTOR_NONE)
>> + return 0;
>> + if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
>> + PMD_DRV_LOG(ERR, "unsupported representor type: %s\n",
>> + pci_dev->device.devargs->args);
>> + return -ENOTSUP;
>> + }
>> +
>> retval = rte_eth_dev_create(&pci_dev->device, pci_dev->device.name,
>> sizeof(struct ixgbe_adapter),
>> eth_dev_pci_specific_init, pci_dev,
>> diff --git a/drivers/net/mlx5/linux/mlx5_os.c
>b/drivers/net/mlx5/linux/mlx5_os.c
>> index 6812a1f215..6981ba1f41 100644
>> --- a/drivers/net/mlx5/linux/mlx5_os.c
>> +++ b/drivers/net/mlx5/linux/mlx5_os.c
>> @@ -706,6 +706,17 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>> strerror(rte_errno));
>> return NULL;
>> }
>> + if (eth_da.type != RTE_ETH_REPRESENTOR_NONE) {
>> + /* Representor not specified. */
>> + rte_errno = EBUSY;
>> + return NULL;
>> + }
>> + if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
>> + rte_errno = ENOTSUP;
>> + DRV_LOG(ERR, "unsupported representor type: %s",
>> + dpdk_dev->devargs->args);
>> + return NULL;
>> + }
>> for (i = 0; i < eth_da.nb_representor_ports; ++i)
>> if (eth_da.representor_ports[i] ==
>> (uint16_t)switch_info->port_name)
>> diff --git a/lib/librte_ethdev/ethdev_private.c
>b/lib/librte_ethdev/ethdev_private.c
>> index 162a502fe7..c219164a4a 100644
>> --- a/lib/librte_ethdev/ethdev_private.c
>> +++ b/lib/librte_ethdev/ethdev_private.c
>> @@ -38,60 +38,13 @@ eth_find_device(const struct rte_eth_dev *start,
>rte_eth_cmp_t cmp,
>> return NULL;
>> }
>>
>> -int
>> -rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
>> - void *data)
>> -{
>> - char *str_start;
>> - int state;
>> - int result;
>> -
>> - if (*str != '[')
>> - /* Single element, not a list */
>> - return callback(str, data);
>> -
>> - /* Sanity check, then strip the brackets */
>> - str_start = &str[strlen(str) - 1];
>> - if (*str_start != ']') {
>> - RTE_LOG(ERR, EAL, "(%s): List does not end with ']'\n", str);
>> - return -EINVAL;
>> - }
>> - str++;
>> - *str_start = '\0';
>> -
>> - /* Process list elements */
>> - state = 0;
>> - while (1) {
>> - if (state == 0) {
>> - if (*str == '\0')
>> - break;
>> - if (*str != ',') {
>> - str_start = str;
>> - state = 1;
>> - }
>> - } else if (state == 1) {
>> - if (*str == ',' || *str == '\0') {
>> - if (str > str_start) {
>> - /* Non-empty string fragment */
>> - *str = '\0';
>> - result = callback(str_start, data);
>> - if (result < 0)
>> - return result;
>> - }
>> - state = 0;
>> - }
>> - }
>> - str++;
>> - }
>> - return 0;
>> -}
>> -
>> static int
>> rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
>> const uint16_t max_list)
>> {
>> uint16_t lo, hi, val;
>> int result;
>> + char *pos = str;
>>
>> result = sscanf(str, "%hu-%hu", &lo, &hi);
>> if (result == 1) {
>> @@ -99,7 +52,7 @@ rte_eth_devargs_process_range(char *str, uint16_t
>*list, uint16_t *len_list,
>> return -ENOMEM;
>> list[(*len_list)++] = lo;
>> } else if (result == 2) {
>> - if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi >
>> RTE_MAX_ETHPORTS)
>> + if (lo >= hi)
>> return -EINVAL;
>> for (val = lo; val <= hi; val++) {
>> if (*len_list >= max_list)
>> @@ -108,14 +61,52 @@ rte_eth_devargs_process_range(char *str, uint16_t
>*list, uint16_t *len_list,
>> }
>> } else
>> return -EINVAL;
>> - return 0;
>> + while (*pos != 0 && ((*pos >= '0' && *pos <= '9') || *pos == '-'))
>> + pos++;
>> + return pos - str;
>> }
>>
>> +static int
>> +rte_eth_devargs_process_list(char *str, uint16_t *list, uint16_t *len_list,
>> + const uint16_t max_list)
>> +{
>> + char *pos = str;
>> + int ret;
>> +
>> + if (*pos == '[')
>> + pos++;
>> + while (1) {
>> + ret = rte_eth_devargs_process_range(pos, list, len_list,
>> + max_list);
>> + if (ret < 0)
>> + return ret;
>> + pos += ret;
>> + if (*pos != ',') /* end of list */
>> + break;
>> + pos++;
>> + }
>> + if (*str == '[' && *pos != ']')
>> + return -EINVAL;
>> + if (*pos == ']')
>> + pos++;
>> + return pos - str;
>> +}
>> +
>> +/*
>> + * representor format:
>> + * #: range or single number of VF representor - legacy
>> + */
>> int
>> rte_eth_devargs_parse_representor_ports(char *str, void *data)
>> {
>> struct rte_eth_devargs *eth_da = data;
>> + int ret;
>>
>> - return rte_eth_devargs_process_range(str, eth_da->representor_ports,
>> + /* Number # alone implies VF */
>> + eth_da->type = RTE_ETH_REPRESENTOR_VF;
>> + ret = rte_eth_devargs_process_list(str, eth_da->representor_ports,
>> ð_da->nb_representor_ports, RTE_MAX_ETHPORTS);
>> + if (ret < 0)
>> + RTE_LOG(ERR, EAL, "wrong representor format: %s\n", str);
>> + return ret < 0 ? ret : 0;
>> }
>> diff --git a/lib/librte_ethdev/ethdev_private.h
>b/lib/librte_ethdev/ethdev_private.h
>> index 905a45c337..220ddd4408 100644
>> --- a/lib/librte_ethdev/ethdev_private.h
>> +++ b/lib/librte_ethdev/ethdev_private.h
>> @@ -26,9 +26,6 @@ eth_find_device(const struct rte_eth_dev *_start,
>rte_eth_cmp_t cmp,
>> const void *data);
>>
>> /* Parse devargs value for representor parameter. */
>> -typedef int (*rte_eth_devargs_callback_t)(char *str, void *data);
>> -int rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t
>callback,
>> - void *data);
>> int rte_eth_devargs_parse_representor_ports(char *str, void *data);
>>
>> #ifdef __cplusplus
>> diff --git a/lib/librte_ethdev/rte_class_eth.c
>b/lib/librte_ethdev/rte_class_eth.c
>> index 6338355e25..efe6149df5 100644
>> --- a/lib/librte_ethdev/rte_class_eth.c
>> +++ b/lib/librte_ethdev/rte_class_eth.c
>> @@ -77,9 +77,7 @@ eth_representor_cmp(const char *key __rte_unused,
>> if (values == NULL)
>> return -1;
>> memset(&representors, 0, sizeof(representors));
>> - ret = rte_eth_devargs_parse_list(values,
>> - rte_eth_devargs_parse_representor_ports,
>> - &representors);
>> + ret = rte_eth_devargs_parse_representor_ports(values, &representors);
>> free(values);
>> if (ret != 0)
>> return -1; /* invalid devargs value */
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 17ddacc78d..2ac51ac149 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -5542,9 +5542,8 @@ rte_eth_devargs_parse(const char *dargs, struct
>rte_eth_devargs *eth_da)
>> for (i = 0; i < args.count; i++) {
>> pair = &args.pairs[i];
>> if (strcmp("representor", pair->key) == 0) {
>> - result = rte_eth_devargs_parse_list(pair->value,
>> - rte_eth_devargs_parse_representor_ports,
>> - eth_da);
>> + result = rte_eth_devargs_parse_representor_ports(
>> + pair->value, eth_da);
>> if (result < 0)
>> goto parse_cleanup;
>> }
>> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h
>b/lib/librte_ethdev/rte_ethdev_driver.h
>> index 0eacfd8425..b66a955b18 100644
>> --- a/lib/librte_ethdev/rte_ethdev_driver.h
>> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
>> @@ -1193,6 +1193,12 @@ __rte_internal
>> int
>> rte_eth_switch_domain_free(uint16_t domain_id);
>>
>> +/** Ethernet device representor type */
>> +enum rte_eth_representor_type {
>> + RTE_ETH_REPRESENTOR_NONE, /* not a representor */
>> + RTE_ETH_REPRESENTOR_VF, /* representor of VF */
>> +};
>> +
>> /** Generic Ethernet device arguments */
>> struct rte_eth_devargs {
>> uint16_t ports[RTE_MAX_ETHPORTS];
>> @@ -1203,6 +1209,7 @@ struct rte_eth_devargs {
>> /** representor port/s identifier to enable on device */
>> uint16_t nb_representor_ports;
>> /** number of ports in representor port field */
>> + enum rte_eth_representor_type type; /* type of representor */
>> };
>>
>> /**
>> --
>> 2.25.1
>>
>
>--
>This electronic communication and the information and any files transmitted
>with it, or attached to it, are confidential and are intended solely for
>the use of the individual or entity to whom it is addressed and may contain
>information that is confidential, legally privileged, protected by privacy
>laws, or otherwise restricted from disclosure to anyone else. If you are
>not the intended recipient or the person responsible for delivering the
>e-mail to the intended recipient, you are hereby notified that any use,
>copying, distributing, dissemination, forwarding, printing, or copying of
>this e-mail is strictly prohibited. If you received this e-mail in error,
>please return the e-mail to the sender, delete it from your computer, and
>destroy any printed copy of it.
