Re: [dpdk-dev] [PATCH v4 2/9] ethdev: support representor port list

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

>-----Original Message-----
>From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>Sent: Tuesday, January 19, 2021 5:04 PM
>To: Xueming(Steven) Li <xuemingl@nvidia.com>; NBU-Contact-Thomas
>Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
>Olivier Matz <olivier.matz@6wind.com>
>Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso
><asafp@nvidia.com>
>Subject: Re: [PATCH v4 2/9] ethdev: support representor port list
>
>On 1/19/21 11:59 AM, Xueming(Steven) Li wrote:
>> Hi Andrew,
>>
>>> -----Original Message-----
>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> Sent: Tuesday, January 19, 2021 3:46 PM
>>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; NBU-Contact-Thomas
>>> Monjalon <thomas@monjalon.net>; Ferruh Yigit
>>> <ferruh.yigit@intel.com>; Olivier Matz <olivier.matz@6wind.com>
>>> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf
>>> Penso <asafp@nvidia.com>
>>> Subject: Re: [PATCH v4 2/9] ethdev: support representor port list
>>>
>>> On 1/18/21 2:16 PM, Xueming Li wrote:
>>>> To support extended representor syntax, this patch extends the
>>>> representor list parsing to support for representor port range in
>>>> devargs, examples:
>>>>    representor=[1,2,3]         - single list
>>>>    representor=[1,3-5,7,9-11]  - list with singles and ranges
>>>>
>>>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>>>> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>>>
>>> See below
>>>
>>>> ---
>>>>  lib/librte_ethdev/ethdev_private.c | 105 ++++++++++++++---------------
>>>>  lib/librte_ethdev/ethdev_private.h |   3 -
>>>>  lib/librte_ethdev/rte_class_eth.c  |   4 +-
>>>>  lib/librte_ethdev/rte_ethdev.c     |   5 +-
>>>>  4 files changed, 54 insertions(+), 63 deletions(-)
>>>>
>>>> diff --git a/lib/librte_ethdev/ethdev_private.c
>>>> b/lib/librte_ethdev/ethdev_private.c
>>>> index c1a411dba4..12bcc7e98d 100644
>>>> --- a/lib/librte_ethdev/ethdev_private.c
>>>> +++ b/lib/librte_ethdev/ethdev_private.c
>>>> @@ -38,77 +38,71 @@ 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)
>>>> +static int
>>>> +rte_eth_devargs_enlist(uint16_t *list, uint16_t *len_list,
>>>> +		       const uint16_t max_list, uint16_t val)
>>>>  {
>>>> -	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';
>>>> +	uint16_t i;
>>>>
>>>> -	/* 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++;
>>>> +	if (*len_list >= max_list)
>>>> +		return -1;
>>>
>>> If current length is equal to max, but added value is already is in
>>> the list, it should not be an error. So, these two lines should be
>>> moved after below for loop.
>>>
>>>> +	for (i = 0; i < *len_list; i++) {
>>>> +		if (list[i] == val)
>>>> +			return 0;
>>>>  	}
>>>> +	list[(*len_list)++] = val;
>>>>  	return 0;
>>>>  }
>>>>
>>>> -static int
>>>> +static char *
>>>>  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) {
>>>> -		if (*len_list >= max_list)
>>>> -			return -ENOMEM;
>>>> -		list[(*len_list)++] = lo;
>>>> +		if (rte_eth_devargs_enlist(list, len_list, max_list, lo) != 0)
>>>> +			return NULL;
>>>>  	} else if (result == 2) {
>>>> -		if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi >
>>> RTE_MAX_ETHPORTS)
>>>
>>> Strictly speaking removal of comparision vs RTE_MAX_ETHPORTS is a
>>> separate logical change with a separate motivation.
>>
>> To make this function comment for controller/pf/vf/sf parsing, the max
>> value is passed in as parameter and checked in rte_eth_devargs_enlist().
>> Caller code in rte_eth_devargs_process_list() decide the max value.
>
>Which maximum? Maximum number of elements in array and maximum
>element value are different things.

My bad, after another check, it should be max number of elements in array,
not max element value. Will update, thanks! 
>
>[snip]