Re: [dpdk-dev] [PATCH v3] app/testpmd: add command to print representor info

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

Hi Xiaoyun,

On 10/11/21 11:32 AM, Li, Xiaoyun wrote:
> Hi
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Wednesday, September 15, 2021 21:48
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>
>> Cc: dev@dpdk.org; Xueming Li <xuemingl@nvidia.com>; Viacheslav Galaktionov
>> <viacheslav.galaktionov@oktetlabs.ru>; Andy Moreton <amoreton@xilinx.com>
>> Subject: [PATCH v3] app/testpmd: add command to print representor info
>>
>> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
>>
>> Make it simpler to debug configurations and code related to the representor
>> info API.
>>
>> Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>> Reviewed-by: Xueming(Steven) Li <xuemingl@nvidia.com>
>> ---
>> v3:
>>     - change command to "show port info (port_id) representor"
>>
>> v2:
>>     - change output format to log just one line per range
>>
>>  app/test-pmd/cmdline.c | 137
>> +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 137 insertions(+)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>> 7dd3965d6f..2f24d33337 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -162,6 +162,10 @@ static void cmd_help_long_parsed(void *parsed_result,
>>  			"show port (info|stats|summary|xstats|fdir|dcb_tc)
>> (port_id|all)\n"
>>  			"    Display information for port_id, or all.\n\n"
>>
>> +			"show port info (port_id) representor\n"
>> +			"    Show supported representors"
>> +			" for a specific port\n\n"
> 
> Why not just put "    Show supported representors for a specific port\n\n" in one sentence?
> 

Thanks, done in v4.

>> +
>>  			"show port port_id (module_eeprom|eeprom)\n"
>>  			"    Display the module EEPROM or EEPROM
>> information for port_id.\n\n"
>>
>> @@ -7904,6 +7908,138 @@ cmdline_parse_inst_t cmd_showport = {
>>  	},
>>  };
>>
> <snip>
>> +		printf("Controller %d, PF %d", range->controller, range->pf);
>> +
>> +		switch (range->type) {
>> +		case RTE_ETH_REPRESENTOR_NONE:
>> +			printf(", NONE\n");
>> +			break;
>> +		case RTE_ETH_REPRESENTOR_VF:
>> +			if (range_diff > 0) {
>> +				printf(", VF %d..%d\n", range->vf,
>> +				       range->vf + range_diff);
>> +			} else {
>> +				printf(", VF %d\n", range->vf);
>> +			}
> 
> There's no need of braces for this "if else" like others.

Fixed in v4.

> 
> BRs
> Xiaoyun
> 
>> +			break;
>> +		case RTE_ETH_REPRESENTOR_SF:
>> +			printf(", SF %d\n", range->sf);
>> +			break;
>> +		case RTE_ETH_REPRESENTOR_PF:
>> +			if (range_diff > 0)
>> +				printf("..%d\n", range->pf + range_diff);
>> +			else
>> +				printf("\n");
>> +			break;
>> +		default:
>> +			printf(", UNKNOWN TYPE %d\n", range->type);
>> +			break;
>> +		}
>> +	}
>> +
>> +	free(info);
>> +}
>> +
> <snip>
>> --
>> 2.30.2

Many thanks for review notes,
Andrew.