Re: [dpdk-dev] [PATCH] ethdev: fix representor port ID search by name

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

On 7/19/21 9:58 AM, Xueming(Steven) Li wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Tuesday, July 13, 2021 12:18 AM
>> To: Ajit Khaparde <ajit.khaparde@broadcom.com>; Somnath Kotur <somnath.kotur@broadcom.com>; John Daley
>> <johndale@cisco.com>; Hyong Youb Kim <hyonkim@cisco.com>; Beilei Xing <beilei.xing@intel.com>; Qiming Yang
>> <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>; Haiyue Wang <haiyue.wang@intel.com>; Matan Azrad
>> <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
>> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Xueming(Steven) Li <xuemingl@nvidia.com>
>> Cc: dev@dpdk.org; Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>; stable@dpdk.org
>> Subject: [PATCH] ethdev: fix representor port ID search by name
>>
>> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
>>
>> Fix representor port ID search by name if the representor itself does not provide representors info. Getting a list of representors from
>> a representor does not make sense. Instead, a parent device should be used.
>>
>> To this end, extend the rte_eth_dev_data structure to include the port ID of the parent device for representors.
>>
>> Fixes: df7547a6a2cc ("ethdev: add helper function to get representor ID")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> ---
>> The new field is added into the hole in rte_eth_dev_data structure.
>> The patch does not change ABI, but extra care is required since ABI check is disabled for the structure because of the libabigail bug [1].
>>
>> Potentially it is bad for out-of-tree drivers which implement representors but do not fill in a new parert_port_id field in
>> rte_eth_dev_data structure. Do we care?
>>
>> May be the patch should add lines to release notes, but I'd like to get initial feedback first.
>>
>> mlx5 changes should be reviwed by maintainers very carefully, since we are not sure if we patch it correctly.
>>
>> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=28060

[snip]

>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -1248,8 +1248,8 @@ struct rte_eth_devargs {
>>    * For backward compatibility, if no representor info, direct
>>    * map legacy VF (no controller and pf).
>>    *
>> - * @param ethdev
>> - *  Handle of ethdev port.
>> + * @param parent_port_id
>> + *  Port ID of the backing device.
>>    * @param type
>>    *  Representor type.
>>    * @param controller
>> @@ -1266,7 +1266,7 @@ struct rte_eth_devargs {
>>    */
>>   __rte_internal
>>   int
>> -rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
>> +rte_eth_representor_id_get(uint16_t parent_port_id,
> 
> It make more sense to get representor info from parent port. Representor is a member of switch domain, PMD owns
> the information of  the representor owner port and info of representors. This change looks better, but not sure
> whether it valuable to introduce a new member to the EAL data structure.

IMHO, it is simply incorrect to return representors info on a
representor itself. Representor info is an information which
representors may be populated using the device.

If above statement is correct, we need a way to get parent device
by representor to do name to representor ID mapping. I see two
options to do it:
  A. Dedicated field in rte_eth_dev_data as the patch does.
  B. Dedicated ethdev op (since representor knows parent port ID anyway).
We have chosen (A) because of simplicity.