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

[Xueming(Steven) Li]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
> Sent: Sunday, August 1, 2021 4:50 PM
> To: Xueming(Steven) Li <xuemingl at nvidia.com>; 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>; stable at dpdk.org
> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> 
> On 7/29/21 7:20 AM, Xueming(Steven) Li wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
> >> Sent: Tuesday, July 13, 2021 12:18 AM
> >> 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>;
> >> Xueming(Steven) Li <xuemingl at nvidia.com>
> >> Cc: dev at dpdk.org; Viacheslav Galaktionov
> >> <viacheslav.galaktionov at oktetlabs.ru>; stable at dpdk.org
> >> Subject: [PATCH] ethdev: fix representor port ID search by name
> >>
> >> From: Viacheslav Galaktionov <viacheslav.galaktionov at 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 at dpdk.org
> >>
> >> Signed-off-by: Viacheslav Galaktionov
> >> <viacheslav.galaktionov at oktetlabs.ru>
> >> Signed-off-by: Andrew Rybchenko <andrew.rybchenko at 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]
> 
> >> b/drivers/net/mlx5/linux/mlx5_os.c
> >> index be22d9cbd2..5550d30628 100644
> >> --- a/drivers/net/mlx5/linux/mlx5_os.c
> >> +++ b/drivers/net/mlx5/linux/mlx5_os.c
> >> @@ -1511,6 +1511,17 @@ 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) {
> >> +			const struct mlx5_priv *opriv =
> >> +				rte_eth_devices[port_id].data->dev_private;
> >> +
> >> +			if (!opriv ||
> >> +			    opriv->sh != priv->sh ||
> >> +			    opriv->representor)
> >> +				continue;
> >> +			eth_dev->data->parent_port_id = port_id;
> >> +			break;
> >> +		}
> >
> > At line 126, there is a logic that locate priv->domain_id, parent port_id could be found there.
> 
> Do you mean line 1260? The comment above says "Look for sibling devices in order to reuse their switch domain if any, otherwise
> allocate one.".
> So, it is not a parent. Is the comment misleading and parent matches the search criteria as well? But in any case, we should guarantee
> that it is a parent port, not a sibling port. So, we need extra criteria to match parent port only.

Yes, you are correct. How about mlx5_find_master_dev()? It locate master port in same switch domain.