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

[Andrew Rybchenko]

On 8/29/21 3:17 PM, Xueming(Steven) Li wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
>> Sent: Sunday, August 29, 2021 4:23 PM
>> To: Xueming(Steven) Li <xuemingl at nvidia.com>; Viacheslav Galaktionov <viacheslav.galaktionov at oktetlabs.ru>
>> Cc: Ajit Khaparde <ajit.khaparde at broadcom.com>; Somnath Kotur <somnath.kotur at broadcom.com>; John Daley
>> <johndale at cisco.com>; Hyong Youb Kim <hyonkim at cisco.com>; Beilei Xing <beilei.xing at intel.com>; Qiming Yang
>> <qiming.yang at intel.com>; Qi Zhang <qi.z.zhang at intel.com>; Haiyue Wang <haiyue.wang at intel.com>; Matan Azrad
>> <matan at nvidia.com>; Shahaf Shuler <shahafs at nvidia.com>; Slava Ovsiienko <viacheslavo at nvidia.com>; NBU-Contact-Thomas
>> Monjalon <thomas at monjalon.net>; Ferruh Yigit <ferruh.yigit at intel.com>; dev at dpdk.org
>> Subject: Re: [PATCH v2] ethdev: fix representor port ID search by name
>>
>> On 8/28/21 4:22 PM, Xueming(Steven) Li wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Viacheslav Galaktionov <viacheslav.galaktionov at oktetlabs.ru>
>>>> Sent: Friday, August 27, 2021 5:48 PM
>>>> To: Xueming(Steven) Li <xuemingl at nvidia.com>
>>>> Cc: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>; Ajit Khaparde
>>>> <ajit.khaparde at broadcom.com>; Somnath Kotur
>>>> <somnath.kotur at broadcom.com>; John Daley <johndale at cisco.com>; Hyong
>>>> Youb Kim <hyonkim at cisco.com>; Beilei Xing <beilei.xing at intel.com>;
>>>> Qiming Yang <qiming.yang at intel.com>; Qi Zhang <qi.z.zhang at intel.com>;
>>>> Haiyue Wang <haiyue.wang at intel.com>; Matan Azrad <matan at nvidia.com>;
>>>> Shahaf Shuler <shahafs at nvidia.com>; Slava Ovsiienko
>>>> <viacheslavo at nvidia.com>; NBU-Contact-Thomas Monjalon
>>>> <thomas at monjalon.net>; Ferruh Yigit <ferruh.yigit at intel.com>;
>>>> dev at dpdk.org
>>>> Subject: Re: [PATCH v2] ethdev: fix representor port ID search by
>>>> name
>>>>
>>>> On 2021-08-27 12:18, Xueming(Steven) Li wrote:
>>>>>> -----Original Message-----
>>>>>> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
>>>>>> Sent: Wednesday, August 18, 2021 10:00 PM
>>>>>> To: Ajit Khaparde <ajit.khaparde at broadcom.com>; Somnath Kotur
>>>>>> <somnath.kotur at broadcom.com>; John Daley <johndale at cisco.com>;
>>>>>> Hyong Youb Kim <hyonkim at cisco.com>; Beilei Xing
>>>>>> <beilei.xing at intel.com>; Qiming Yang <qiming.yang at intel.com>; Qi
>>>>>> Zhang <qi.z.zhang at intel.com>; Haiyue Wang <haiyue.wang at intel.com>;
>>>>>> Matan Azrad <matan at nvidia.com>; Shahaf Shuler <shahafs at nvidia.com>;
>>>>>> Slava Ovsiienko <viacheslavo at nvidia.com>; NBU-Contact-Thomas
>>>>>> Monjalon <thomas at monjalon.net>; Ferruh Yigit
>>>>>> <ferruh.yigit at intel.com>
>>>>>> Cc: dev at dpdk.org; Viacheslav Galaktionov
>>>>>> <viacheslav.galaktionov at oktetlabs.ru>; Xueming(Steven) Li
>>>>>> <xuemingl at nvidia.com>
>>>>>> Subject: [PATCH v2] ethdev: fix representor port ID search by name
>>>>>>
>>>>>> From: Viacheslav Galaktionov <viacheslav.galaktionov at 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 at oktetlabs.ru>
>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko at 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?
>>>
>>> As discussed before, representor could exists w/o master device, special case.
>>
>> May I clarify one question. Isn't bond will be a parent for the second PF VF representors? Will above algorithm find it?
>> If yes, I think we don't need self fallback here.
> 
> Sorry maybe I was not clear enough. It's true that bond will be parent for second PF  VF representor, the above algorithm can find it.
> But if second PF VF representor probe earlier than the bond, please note that bond get probed with first PF, bond won't be found.
> 
>> If no, it looks a bit wrong to me but may be acceptable for so complicated case. If it is acceptable to you, we can put self fallback here,
>> but in this case we don't need corresponding code which check self port_id first. It would be even better this way since generic code
>> will be more clear.
> 
> Agree, it's better to make generic ode more clear.

Very good. I see your point I'll send v4 tomorrow.

>>
>>>>
>>>>> 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?
>>>
>>> Representor 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]
>