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

[Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>]

On 2021-08-27 12:18, Xueming(Steven) Li wrote:
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Wednesday, August 18, 2021 10:00 PM
>> 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>
>> Cc: dev@dpdk.org; Viacheslav Galaktionov 
>> <viacheslav.galaktionov@oktetlabs.ru>; Xueming(Steven) Li 
>> <xuemingl@nvidia.com>
>> Subject: [PATCH v2] ethdev: fix representor port ID search by name
>> 
>> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
>> 
>> 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.
>> 
>> Signed-off-by: Viacheslav Galaktionov 
>> <viacheslav.galaktionov@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> ---

[snip]

>> b/drivers/net/mlx5/windows/mlx5_os.c
>> index 7e1df1c751..0c5a02bfcb 100644
>> --- a/drivers/net/mlx5/windows/mlx5_os.c
>> +++ b/drivers/net/mlx5/windows/mlx5_os.c
>> @@ -543,6 +543,23 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>>  	if (priv->representor) {
>>  		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
>>  		eth_dev->data->representor_id = priv->representor_id;
>> +		MLX5_ETH_FOREACH_DEV(port_id, priv->pci_dev) {
>> +			struct mlx5_priv *opriv =
>> +				rte_eth_devices[port_id].data->dev_private;
>> +			if (opriv &&
>> +			    opriv->master &&
>> +			    opriv->domain_id == priv->domain_id &&
>> +			    opriv->sh == priv->sh) {
>> +				eth_dev->data->parent_port_id =
>> +					rte_eth_devices[port_id].data->port_id;
> 
> Could this value different than port_id?

Oh, yes, that's an oversight. Thank you!

>> +				break;
>> +			}
>> +		}
>> +		if (port_id >= RTE_MAX_ETHPORTS) {
>> +			DRV_LOG(ERR, "no master device for representor");
>> +			err = ENODEV;
>> +			goto error;
> 
> Here shouldn't be an error.

What do you mean? Is it normal not to have a master device for a 
representor?

> Parent port ID default to 0, it could be wrong if multiple PF probed,
> let's default to current port ID.

What is the "current" port ID here? Do you mean the representor's port 
ID?
If you are talking about the value of the port_id variable, then I 
suppose it
could be set to RTE_MAX_ETHPORTS explicitly if a master device isn't 
found.

[snip]