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

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

[Andrew Rybchenko]

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

 drivers/net/bnxt/bnxt_reps.c             |  1 +
 drivers/net/enic/enic_vf_representor.c   |  1 +
 drivers/net/i40e/i40e_vf_representor.c   |  1 +
 drivers/net/ice/ice_dcf_vf_representor.c |  1 +
 drivers/net/ixgbe/ixgbe_vf_representor.c |  1 +
 drivers/net/mlx5/linux/mlx5_os.c         | 11 +++++++++++
 drivers/net/mlx5/windows/mlx5_os.c       | 11 +++++++++++
 lib/ethdev/ethdev_driver.h               |  6 +++---
 lib/ethdev/rte_class_eth.c               |  2 +-
 lib/ethdev/rte_ethdev.c                  |  8 ++++----
 lib/ethdev/rte_ethdev_core.h             |  4 ++++
 11 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_reps.c b/drivers/net/bnxt/bnxt_reps.c
index bdbad53b7d..902591cd39 100644
--- a/drivers/net/bnxt/bnxt_reps.c
+++ b/drivers/net/bnxt/bnxt_reps.c
@@ -187,6 +187,7 @@ int bnxt_representor_init(struct rte_eth_dev *eth_dev, void *params)
 	eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
 					RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
 	eth_dev->data->representor_id = rep_params->vf_id;
+	eth_dev->data->parent_port_id = rep_params->parent_dev->data->port_id;
 
 	rte_eth_random_addr(vf_rep_bp->dflt_mac_addr);
 	memcpy(vf_rep_bp->mac_addr, vf_rep_bp->dflt_mac_addr,
diff --git a/drivers/net/enic/enic_vf_representor.c b/drivers/net/enic/enic_vf_representor.c
index 79dd6e5640..6ee7967ce9 100644
--- a/drivers/net/enic/enic_vf_representor.c
+++ b/drivers/net/enic/enic_vf_representor.c
@@ -662,6 +662,7 @@ int enic_vf_representor_init(struct rte_eth_dev *eth_dev, void *init_params)
 	eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
 					RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
 	eth_dev->data->representor_id = vf->vf_id;
+	eth_dev->data->parent_port_id = pf->port_id;
 	eth_dev->data->mac_addrs = rte_zmalloc("enic_mac_addr_vf",
 		sizeof(struct rte_ether_addr) *
 		ENIC_UNICAST_PERFECT_FILTERS, 0);
diff --git a/drivers/net/i40e/i40e_vf_representor.c b/drivers/net/i40e/i40e_vf_representor.c
index 0481b55381..865b637585 100644
--- a/drivers/net/i40e/i40e_vf_representor.c
+++ b/drivers/net/i40e/i40e_vf_representor.c
@@ -514,6 +514,7 @@ i40e_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
 	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
 					RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
 	ethdev->data->representor_id = representor->vf_id;
+	ethdev->data->parent_port_id = pf->dev_data->parent_port_id;
 
 	/* Setting the number queues allocated to the VF */
 	ethdev->data->nb_rx_queues = vf->vsi->nb_qps;
diff --git a/drivers/net/ice/ice_dcf_vf_representor.c b/drivers/net/ice/ice_dcf_vf_representor.c
index 970461f3e9..c7cd3fd290 100644
--- a/drivers/net/ice/ice_dcf_vf_representor.c
+++ b/drivers/net/ice/ice_dcf_vf_representor.c
@@ -418,6 +418,7 @@ ice_dcf_vf_repr_init(struct rte_eth_dev *vf_rep_eth_dev, void *init_param)
 
 	vf_rep_eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
 	vf_rep_eth_dev->data->representor_id = repr->vf_id;
+	vf_rep_eth_dev->data->parent_port_id = repr->dcf_eth_dev->data->port_id;
 
 	vf_rep_eth_dev->data->mac_addrs = &repr->mac_addr;
 
diff --git a/drivers/net/ixgbe/ixgbe_vf_representor.c b/drivers/net/ixgbe/ixgbe_vf_representor.c
index d5b636a194..7a2063849e 100644
--- a/drivers/net/ixgbe/ixgbe_vf_representor.c
+++ b/drivers/net/ixgbe/ixgbe_vf_representor.c
@@ -197,6 +197,7 @@ ixgbe_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
 
 	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
 	ethdev->data->representor_id = representor->vf_id;
+	ethdev->data->parent_port_id = representor->pf_ethdev->data->port_id;
 
 	/* Set representor device ops */
 	ethdev->dev_ops = &ixgbe_vf_representor_dev_ops;
diff --git a/drivers/net/mlx5/linux/mlx5_os.c 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;
+		}
 	}
 	priv->mp_id.port_id = eth_dev->data->port_id;
 	strlcpy(priv->mp_id.name, MLX5_MP_NAME, RTE_MP_MAX_NAME_LEN);
diff --git a/drivers/net/mlx5/windows/mlx5_os.c b/drivers/net/mlx5/windows/mlx5_os.c
index e30b682822..037c928dc1 100644
--- a/drivers/net/mlx5/windows/mlx5_os.c
+++ b/drivers/net/mlx5/windows/mlx5_os.c
@@ -506,6 +506,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;
+		}
 	}
 	/*
 	 * Store associated network device interface index. This index
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 40e474aa7e..07f6d1f9a4 100644
--- 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,
 			   enum rte_eth_representor_type type,
 			   int controller, int pf, int representor_port,
 			   uint16_t *repr_id);
diff --git a/lib/ethdev/rte_class_eth.c b/lib/ethdev/rte_class_eth.c
index 1fe5fa1f36..e3b7ab9728 100644
--- a/lib/ethdev/rte_class_eth.c
+++ b/lib/ethdev/rte_class_eth.c
@@ -95,7 +95,7 @@ eth_representor_cmp(const char *key __rte_unused,
 		c = i / (np * nf);
 		p = (i / nf) % np;
 		f = i % nf;
-		if (rte_eth_representor_id_get(edev,
+		if (rte_eth_representor_id_get(edev->data->parent_port_id,
 			eth_da.type,
 			eth_da.nb_mh_controllers == 0 ? -1 :
 					eth_da.mh_controllers[c],
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 6ebf52b641..acda1d43fb 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5997,7 +5997,7 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
 }
 
 int
-rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
+rte_eth_representor_id_get(uint16_t parent_port_id,
 			   enum rte_eth_representor_type type,
 			   int controller, int pf, int representor_port,
 			   uint16_t *repr_id)
@@ -6012,7 +6012,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
 		return -EINVAL;
 
 	/* Get PMD representor range info. */
-	ret = rte_eth_representor_info_get(ethdev->data->port_id, NULL);
+	ret = rte_eth_representor_info_get(parent_port_id, NULL);
 	if (ret == -ENOTSUP && type == RTE_ETH_REPRESENTOR_VF &&
 	    controller == -1 && pf == -1) {
 		/* Direct mapping for legacy VF representor. */
@@ -6026,7 +6026,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
 	info = calloc(1, size);
 	if (info == NULL)
 		return -ENOMEM;
-	ret = rte_eth_representor_info_get(ethdev->data->port_id, info);
+	ret = rte_eth_representor_info_get(parent_port_id, info);
 	if (ret < 0)
 		goto out;
 
@@ -6045,7 +6045,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
 			continue;
 		if (info->ranges[i].id_end < info->ranges[i].id_base) {
 			RTE_LOG(WARNING, EAL, "Port %hu invalid representor ID Range %u - %u, entry %d\n",
-				ethdev->data->port_id, info->ranges[i].id_base,
+				parent_port_id, info->ranges[i].id_base,
 				info->ranges[i].id_end, i);
 			continue;
 
diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
index edf96de2dc..13cb84b52f 100644
--- a/lib/ethdev/rte_ethdev_core.h
+++ b/lib/ethdev/rte_ethdev_core.h
@@ -185,6 +185,10 @@ struct rte_eth_dev_data {
 			/**< Switch-specific identifier.
 			 *   Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags.
 			 */
+	uint16_t parent_port_id;
+			/**< Port ID of the backing device.
+			 *   Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags.
+			 */
 
 	pthread_mutex_t flow_ops_mutex; /**< rte_flow ops mutex. */
 	uint64_t reserved_64s[4]; /**< Reserved for future fields */
-- 
2.30.2

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

[Xueming(Steven) Li]

> -----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
> 
>  drivers/net/bnxt/bnxt_reps.c             |  1 +
>  drivers/net/enic/enic_vf_representor.c   |  1 +
>  drivers/net/i40e/i40e_vf_representor.c   |  1 +
>  drivers/net/ice/ice_dcf_vf_representor.c |  1 +  drivers/net/ixgbe/ixgbe_vf_representor.c |  1 +
>  drivers/net/mlx5/linux/mlx5_os.c         | 11 +++++++++++
>  drivers/net/mlx5/windows/mlx5_os.c       | 11 +++++++++++
>  lib/ethdev/ethdev_driver.h               |  6 +++---
>  lib/ethdev/rte_class_eth.c               |  2 +-
>  lib/ethdev/rte_ethdev.c                  |  8 ++++----
>  lib/ethdev/rte_ethdev_core.h             |  4 ++++
>  11 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/bnxt/bnxt_reps.c b/drivers/net/bnxt/bnxt_reps.c index bdbad53b7d..902591cd39 100644
> --- a/drivers/net/bnxt/bnxt_reps.c
> +++ b/drivers/net/bnxt/bnxt_reps.c
> @@ -187,6 +187,7 @@ int bnxt_representor_init(struct rte_eth_dev *eth_dev, void *params)
>  	eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
>  					RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
>  	eth_dev->data->representor_id = rep_params->vf_id;
> +	eth_dev->data->parent_port_id = rep_params->parent_dev->data->port_id;
> 
>  	rte_eth_random_addr(vf_rep_bp->dflt_mac_addr);
>  	memcpy(vf_rep_bp->mac_addr, vf_rep_bp->dflt_mac_addr, diff --git a/drivers/net/enic/enic_vf_representor.c
> b/drivers/net/enic/enic_vf_representor.c
> index 79dd6e5640..6ee7967ce9 100644
> --- a/drivers/net/enic/enic_vf_representor.c
> +++ b/drivers/net/enic/enic_vf_representor.c
> @@ -662,6 +662,7 @@ int enic_vf_representor_init(struct rte_eth_dev *eth_dev, void *init_params)
>  	eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
>  					RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
>  	eth_dev->data->representor_id = vf->vf_id;
> +	eth_dev->data->parent_port_id = pf->port_id;
>  	eth_dev->data->mac_addrs = rte_zmalloc("enic_mac_addr_vf",
>  		sizeof(struct rte_ether_addr) *
>  		ENIC_UNICAST_PERFECT_FILTERS, 0);
> diff --git a/drivers/net/i40e/i40e_vf_representor.c b/drivers/net/i40e/i40e_vf_representor.c
> index 0481b55381..865b637585 100644
> --- a/drivers/net/i40e/i40e_vf_representor.c
> +++ b/drivers/net/i40e/i40e_vf_representor.c
> @@ -514,6 +514,7 @@ i40e_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
>  	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
>  					RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
>  	ethdev->data->representor_id = representor->vf_id;
> +	ethdev->data->parent_port_id = pf->dev_data->parent_port_id;
> 
>  	/* Setting the number queues allocated to the VF */
>  	ethdev->data->nb_rx_queues = vf->vsi->nb_qps; diff --git a/drivers/net/ice/ice_dcf_vf_representor.c
> b/drivers/net/ice/ice_dcf_vf_representor.c
> index 970461f3e9..c7cd3fd290 100644
> --- a/drivers/net/ice/ice_dcf_vf_representor.c
> +++ b/drivers/net/ice/ice_dcf_vf_representor.c
> @@ -418,6 +418,7 @@ ice_dcf_vf_repr_init(struct rte_eth_dev *vf_rep_eth_dev, void *init_param)
> 
>  	vf_rep_eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
>  	vf_rep_eth_dev->data->representor_id = repr->vf_id;
> +	vf_rep_eth_dev->data->parent_port_id =
> +repr->dcf_eth_dev->data->port_id;
> 
>  	vf_rep_eth_dev->data->mac_addrs = &repr->mac_addr;
> 
> diff --git a/drivers/net/ixgbe/ixgbe_vf_representor.c b/drivers/net/ixgbe/ixgbe_vf_representor.c
> index d5b636a194..7a2063849e 100644
> --- a/drivers/net/ixgbe/ixgbe_vf_representor.c
> +++ b/drivers/net/ixgbe/ixgbe_vf_representor.c
> @@ -197,6 +197,7 @@ ixgbe_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
> 
>  	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
>  	ethdev->data->representor_id = representor->vf_id;
> +	ethdev->data->parent_port_id = representor->pf_ethdev->data->port_id;
> 
>  	/* Set representor device ops */
>  	ethdev->dev_ops = &ixgbe_vf_representor_dev_ops; diff --git a/drivers/net/mlx5/linux/mlx5_os.c
> 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;
> +		}
>  	}
>  	priv->mp_id.port_id = eth_dev->data->port_id;
>  	strlcpy(priv->mp_id.name, MLX5_MP_NAME, RTE_MP_MAX_NAME_LEN); diff --git a/drivers/net/mlx5/windows/mlx5_os.c
> b/drivers/net/mlx5/windows/mlx5_os.c
> index e30b682822..037c928dc1 100644
> --- a/drivers/net/mlx5/windows/mlx5_os.c
> +++ b/drivers/net/mlx5/windows/mlx5_os.c
> @@ -506,6 +506,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;
> +		}
>  	}
>  	/*
>  	 * Store associated network device interface index. This index diff --git a/lib/ethdev/ethdev_driver.h
> b/lib/ethdev/ethdev_driver.h index 40e474aa7e..07f6d1f9a4 100644
> --- 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.

>  			   enum rte_eth_representor_type type,
>  			   int controller, int pf, int representor_port,
>  			   uint16_t *repr_id);
> diff --git a/lib/ethdev/rte_class_eth.c b/lib/ethdev/rte_class_eth.c index 1fe5fa1f36..e3b7ab9728 100644
> --- a/lib/ethdev/rte_class_eth.c
> +++ b/lib/ethdev/rte_class_eth.c
> @@ -95,7 +95,7 @@ eth_representor_cmp(const char *key __rte_unused,
>  		c = i / (np * nf);
>  		p = (i / nf) % np;
>  		f = i % nf;
> -		if (rte_eth_representor_id_get(edev,
> +		if (rte_eth_representor_id_get(edev->data->parent_port_id,
>  			eth_da.type,
>  			eth_da.nb_mh_controllers == 0 ? -1 :
>  					eth_da.mh_controllers[c],
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index 6ebf52b641..acda1d43fb 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -5997,7 +5997,7 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)  }
> 
>  int
> -rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
> +rte_eth_representor_id_get(uint16_t parent_port_id,
>  			   enum rte_eth_representor_type type,
>  			   int controller, int pf, int representor_port,
>  			   uint16_t *repr_id)
> @@ -6012,7 +6012,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
>  		return -EINVAL;
> 
>  	/* Get PMD representor range info. */
> -	ret = rte_eth_representor_info_get(ethdev->data->port_id, NULL);
> +	ret = rte_eth_representor_info_get(parent_port_id, NULL);
>  	if (ret == -ENOTSUP && type == RTE_ETH_REPRESENTOR_VF &&
>  	    controller == -1 && pf == -1) {
>  		/* Direct mapping for legacy VF representor. */ @@ -6026,7 +6026,7 @@ rte_eth_representor_id_get(const struct
> rte_eth_dev *ethdev,
>  	info = calloc(1, size);
>  	if (info == NULL)
>  		return -ENOMEM;
> -	ret = rte_eth_representor_info_get(ethdev->data->port_id, info);
> +	ret = rte_eth_representor_info_get(parent_port_id, info);
>  	if (ret < 0)
>  		goto out;
> 
> @@ -6045,7 +6045,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
>  			continue;
>  		if (info->ranges[i].id_end < info->ranges[i].id_base) {
>  			RTE_LOG(WARNING, EAL, "Port %hu invalid representor ID Range %u - %u, entry %d\n",
> -				ethdev->data->port_id, info->ranges[i].id_base,
> +				parent_port_id, info->ranges[i].id_base,
>  				info->ranges[i].id_end, i);
>  			continue;
> 
> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h index edf96de2dc..13cb84b52f 100644
> --- a/lib/ethdev/rte_ethdev_core.h
> +++ b/lib/ethdev/rte_ethdev_core.h
> @@ -185,6 +185,10 @@ struct rte_eth_dev_data {
>  			/**< Switch-specific identifier.
>  			 *   Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags.
>  			 */
> +	uint16_t parent_port_id;
> +			/**< Port ID of the backing device.
> +			 *   Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags.
> +			 */
> 
>  	pthread_mutex_t flow_ops_mutex; /**< rte_flow ops mutex. */
>  	uint64_t reserved_64s[4]; /**< Reserved for future fields */
> --
> 2.30.2

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

[Andrew Rybchenko]

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.

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

[Xueming(Steven) Li]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Monday, July 19, 2021 4:46 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> 
> 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.

Just recalled that representor port could be probed w/o owner PF, is a force for parent port?

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

[Andrew Rybchenko]

On 7/19/21 2:54 PM, Xueming(Steven) Li wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Monday, July 19, 2021 4:46 PM
>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
>> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
>>
>> 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.
> 
> Just recalled that representor port could be probed w/o owner PF, is a force for parent port?

I thought that it is impossible and parent port is absolutely required
for a representor. Could you provide an example and explain how will it
work?

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

[Xueming(Steven) Li]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Monday, July 19, 2021 8:36 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> 
> On 7/19/21 2:54 PM, Xueming(Steven) Li wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Monday, July 19, 2021 4:46 PM
> >> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
> >> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> >>
> >> 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.
> >
> > Just recalled that representor port could be probed w/o owner PF, is a force for parent port?
> 
> I thought that it is impossible and parent port is absolutely required for a representor. Could you provide an example and explain how
> will it work?

In case of bonding, PF0 and PF1 become one PF port `bond0`, PCI address is PF0.
	-a <PF0>,representor=pf[0-1]vf[0-99] // this is the syntax we proposed.

To be backward compatible, also support the following 2 devargs:
	-a <pf0>,representor=[0-99] // probe bond0 and representor on pf0
	-a <pf1>,representor=[0-99] // probe representors on pf1.
If devargs start with PF1 devargs, no owner PF1 created as it disabled in bonding. Can't create bond0(PF0) automatically here as 
device is located by PCI address(PF1) from devargs.

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

[Andrew Rybchenko]

On 7/19/21 3:50 PM, Xueming(Steven) Li wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Monday, July 19, 2021 8:36 PM
>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
>> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
>>
>> On 7/19/21 2:54 PM, Xueming(Steven) Li wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Monday, July 19, 2021 4:46 PM
>>>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
>>>> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
>>>>
>>>> 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.
>>>
>>> Just recalled that representor port could be probed w/o owner PF, is a force for parent port?
>>
>> I thought that it is impossible and parent port is absolutely required for a representor. Could you provide an example and explain how
>> will it work?
> 
> In case of bonding, PF0 and PF1 become one PF port `bond0`, PCI address is PF0.
> 	-a <PF0>,representor=pf[0-1]vf[0-99] // this is the syntax we proposed.

Is it net/bonding or vendor-specific bonding in HW?
If I remember correctly in the case of net/bonding we have ethdev ports
for bonded devices.

> 
> To be backward compatible, also support the following 2 devargs:
> 	-a <pf0>,representor=[0-99] // probe bond0 and representor on pf0
> 	-a <pf1>,representor=[0-99] // probe representors on pf1.
> If devargs start with PF1 devargs, no owner PF1 created as it disabled in bonding. Can't create bond0(PF0) automatically here as
> device is located by PCI address(PF1) from devargs.

So, I guess the problem is vendor-specific bonding in HW. Anyway
legacy backward compatible representor spec should not require
representors info since it worked before without it. So, it does
not sound like a reason to have representors info on a representor
itself.

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

[Xueming(Steven) Li]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Tuesday, July 20, 2021 5:00 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> 
> On 7/19/21 3:50 PM, Xueming(Steven) Li wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Monday, July 19, 2021 8:36 PM
> >> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
> >> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> >>
> >> On 7/19/21 2:54 PM, Xueming(Steven) Li wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>> Sent: Monday, July 19, 2021 4:46 PM
> >>>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
> >>>> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> >>>>
> >>>> 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.
> >>>
> >>> Just recalled that representor port could be probed w/o owner PF, is a force for parent port?
> >>
> >> I thought that it is impossible and parent port is absolutely
> >> required for a representor. Could you provide an example and explain how will it work?
> >
> > In case of bonding, PF0 and PF1 become one PF port `bond0`, PCI address is PF0.
> > 	-a <PF0>,representor=pf[0-1]vf[0-99] // this is the syntax we proposed.
> 
> Is it net/bonding or vendor-specific bonding in HW?
> If I remember correctly in the case of net/bonding we have ethdev ports for bonded devices.

Not net/bonding pmd, it's Linux bonding, supported by hw driver.

> 
> >
> > To be backward compatible, also support the following 2 devargs:
> > 	-a <pf0>,representor=[0-99] // probe bond0 and representor on pf0
> > 	-a <pf1>,representor=[0-99] // probe representors on pf1.
> > If devargs start with PF1 devargs, no owner PF1 created as it disabled
> > in bonding. Can't create bond0(PF0) automatically here as device is located by PCI address(PF1) from devargs.
> 
> So, I guess the problem is vendor-specific bonding in HW. Anyway legacy backward compatible representor spec should not require
> representors info since it worked before without it. So, it does not sound like a reason to have representors info on a representor
> itself.

Legacy backward logic could be something like this: if PF owner port found, use it, fallback to current representor.
This won't break anything I guess, how do you think?

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

[Xueming(Steven) Li]

> -----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
> 
>  drivers/net/bnxt/bnxt_reps.c             |  1 +
>  drivers/net/enic/enic_vf_representor.c   |  1 +
>  drivers/net/i40e/i40e_vf_representor.c   |  1 +
>  drivers/net/ice/ice_dcf_vf_representor.c |  1 +  drivers/net/ixgbe/ixgbe_vf_representor.c |  1 +
>  drivers/net/mlx5/linux/mlx5_os.c         | 11 +++++++++++
>  drivers/net/mlx5/windows/mlx5_os.c       | 11 +++++++++++
>  lib/ethdev/ethdev_driver.h               |  6 +++---
>  lib/ethdev/rte_class_eth.c               |  2 +-
>  lib/ethdev/rte_ethdev.c                  |  8 ++++----
>  lib/ethdev/rte_ethdev_core.h             |  4 ++++
>  11 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/bnxt/bnxt_reps.c b/drivers/net/bnxt/bnxt_reps.c index bdbad53b7d..902591cd39 100644
> --- a/drivers/net/bnxt/bnxt_reps.c
> +++ b/drivers/net/bnxt/bnxt_reps.c
> @@ -187,6 +187,7 @@ int bnxt_representor_init(struct rte_eth_dev *eth_dev, void *params)
>  	eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
>  					RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
>  	eth_dev->data->representor_id = rep_params->vf_id;
> +	eth_dev->data->parent_port_id = rep_params->parent_dev->data->port_id;
> 
>  	rte_eth_random_addr(vf_rep_bp->dflt_mac_addr);
>  	memcpy(vf_rep_bp->mac_addr, vf_rep_bp->dflt_mac_addr, diff --git a/drivers/net/enic/enic_vf_representor.c
> b/drivers/net/enic/enic_vf_representor.c
> index 79dd6e5640..6ee7967ce9 100644
> --- a/drivers/net/enic/enic_vf_representor.c
> +++ b/drivers/net/enic/enic_vf_representor.c
> @@ -662,6 +662,7 @@ int enic_vf_representor_init(struct rte_eth_dev *eth_dev, void *init_params)
>  	eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
>  					RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
>  	eth_dev->data->representor_id = vf->vf_id;
> +	eth_dev->data->parent_port_id = pf->port_id;
>  	eth_dev->data->mac_addrs = rte_zmalloc("enic_mac_addr_vf",
>  		sizeof(struct rte_ether_addr) *
>  		ENIC_UNICAST_PERFECT_FILTERS, 0);
> diff --git a/drivers/net/i40e/i40e_vf_representor.c b/drivers/net/i40e/i40e_vf_representor.c
> index 0481b55381..865b637585 100644
> --- a/drivers/net/i40e/i40e_vf_representor.c
> +++ b/drivers/net/i40e/i40e_vf_representor.c
> @@ -514,6 +514,7 @@ i40e_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
>  	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
>  					RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
>  	ethdev->data->representor_id = representor->vf_id;
> +	ethdev->data->parent_port_id = pf->dev_data->parent_port_id;
> 
>  	/* Setting the number queues allocated to the VF */
>  	ethdev->data->nb_rx_queues = vf->vsi->nb_qps; diff --git a/drivers/net/ice/ice_dcf_vf_representor.c
> b/drivers/net/ice/ice_dcf_vf_representor.c
> index 970461f3e9..c7cd3fd290 100644
> --- a/drivers/net/ice/ice_dcf_vf_representor.c
> +++ b/drivers/net/ice/ice_dcf_vf_representor.c
> @@ -418,6 +418,7 @@ ice_dcf_vf_repr_init(struct rte_eth_dev *vf_rep_eth_dev, void *init_param)
> 
>  	vf_rep_eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
>  	vf_rep_eth_dev->data->representor_id = repr->vf_id;
> +	vf_rep_eth_dev->data->parent_port_id =
> +repr->dcf_eth_dev->data->port_id;
> 
>  	vf_rep_eth_dev->data->mac_addrs = &repr->mac_addr;
> 
> diff --git a/drivers/net/ixgbe/ixgbe_vf_representor.c b/drivers/net/ixgbe/ixgbe_vf_representor.c
> index d5b636a194..7a2063849e 100644
> --- a/drivers/net/ixgbe/ixgbe_vf_representor.c
> +++ b/drivers/net/ixgbe/ixgbe_vf_representor.c
> @@ -197,6 +197,7 @@ ixgbe_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
> 
>  	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
>  	ethdev->data->representor_id = representor->vf_id;
> +	ethdev->data->parent_port_id = representor->pf_ethdev->data->port_id;
> 
>  	/* Set representor device ops */
>  	ethdev->dev_ops = &ixgbe_vf_representor_dev_ops; diff --git a/drivers/net/mlx5/linux/mlx5_os.c
> 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.

>  	}
>  	priv->mp_id.port_id = eth_dev->data->port_id;
>  	strlcpy(priv->mp_id.name, MLX5_MP_NAME, RTE_MP_MAX_NAME_LEN); diff --git a/drivers/net/mlx5/windows/mlx5_os.c
> b/drivers/net/mlx5/windows/mlx5_os.c
> index e30b682822..037c928dc1 100644
> --- a/drivers/net/mlx5/windows/mlx5_os.c
> +++ b/drivers/net/mlx5/windows/mlx5_os.c
> @@ -506,6 +506,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;
> +		}
>  	}
>  	/*
>  	 * Store associated network device interface index. This index diff --git a/lib/ethdev/ethdev_driver.h
> b/lib/ethdev/ethdev_driver.h index 40e474aa7e..07f6d1f9a4 100644
> --- 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,
>  			   enum rte_eth_representor_type type,
>  			   int controller, int pf, int representor_port,
>  			   uint16_t *repr_id);
> diff --git a/lib/ethdev/rte_class_eth.c b/lib/ethdev/rte_class_eth.c index 1fe5fa1f36..e3b7ab9728 100644
> --- a/lib/ethdev/rte_class_eth.c
> +++ b/lib/ethdev/rte_class_eth.c
> @@ -95,7 +95,7 @@ eth_representor_cmp(const char *key __rte_unused,
>  		c = i / (np * nf);
>  		p = (i / nf) % np;
>  		f = i % nf;
> -		if (rte_eth_representor_id_get(edev,
> +		if (rte_eth_representor_id_get(edev->data->parent_port_id,
>  			eth_da.type,
>  			eth_da.nb_mh_controllers == 0 ? -1 :
>  					eth_da.mh_controllers[c],
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index 6ebf52b641..acda1d43fb 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -5997,7 +5997,7 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)  }
> 
>  int
> -rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
> +rte_eth_representor_id_get(uint16_t parent_port_id,
>  			   enum rte_eth_representor_type type,
>  			   int controller, int pf, int representor_port,
>  			   uint16_t *repr_id)
> @@ -6012,7 +6012,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
>  		return -EINVAL;
> 
>  	/* Get PMD representor range info. */
> -	ret = rte_eth_representor_info_get(ethdev->data->port_id, NULL);
> +	ret = rte_eth_representor_info_get(parent_port_id, NULL);
>  	if (ret == -ENOTSUP && type == RTE_ETH_REPRESENTOR_VF &&
>  	    controller == -1 && pf == -1) {
>  		/* Direct mapping for legacy VF representor. */ @@ -6026,7 +6026,7 @@ rte_eth_representor_id_get(const struct
> rte_eth_dev *ethdev,
>  	info = calloc(1, size);
>  	if (info == NULL)
>  		return -ENOMEM;
> -	ret = rte_eth_representor_info_get(ethdev->data->port_id, info);
> +	ret = rte_eth_representor_info_get(parent_port_id, info);
>  	if (ret < 0)
>  		goto out;
> 
> @@ -6045,7 +6045,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
>  			continue;
>  		if (info->ranges[i].id_end < info->ranges[i].id_base) {
>  			RTE_LOG(WARNING, EAL, "Port %hu invalid representor ID Range %u - %u, entry %d\n",
> -				ethdev->data->port_id, info->ranges[i].id_base,
> +				parent_port_id, info->ranges[i].id_base,
>  				info->ranges[i].id_end, i);
>  			continue;
> 
> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h index edf96de2dc..13cb84b52f 100644
> --- a/lib/ethdev/rte_ethdev_core.h
> +++ b/lib/ethdev/rte_ethdev_core.h
> @@ -185,6 +185,10 @@ struct rte_eth_dev_data {
>  			/**< Switch-specific identifier.
>  			 *   Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags.
>  			 */
> +	uint16_t parent_port_id;
> +			/**< Port ID of the backing device.
> +			 *   Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags.
> +			 */
> 
>  	pthread_mutex_t flow_ops_mutex; /**< rte_flow ops mutex. */
>  	uint64_t reserved_64s[4]; /**< Reserved for future fields */
> --
> 2.30.2

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

[Andrew Rybchenko]

On 7/29/21 7:13 AM, Xueming(Steven) Li wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Tuesday, July 20, 2021 5:00 PM
>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
>> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
>>
>> On 7/19/21 3:50 PM, Xueming(Steven) Li wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Monday, July 19, 2021 8:36 PM
>>>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
>>>> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
>>>>
>>>> On 7/19/21 2:54 PM, Xueming(Steven) Li wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> Sent: Monday, July 19, 2021 4:46 PM
>>>>>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
>>>>>> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
>>>>>>
>>>>>> 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.
>>>>>
>>>>> Just recalled that representor port could be probed w/o owner PF, is a force for parent port?
>>>>
>>>> I thought that it is impossible and parent port is absolutely
>>>> required for a representor. Could you provide an example and explain how will it work?
>>>
>>> In case of bonding, PF0 and PF1 become one PF port `bond0`, PCI address is PF0.
>>> 	-a <PF0>,representor=pf[0-1]vf[0-99] // this is the syntax we proposed.
>>
>> Is it net/bonding or vendor-specific bonding in HW?
>> If I remember correctly in the case of net/bonding we have ethdev ports for bonded devices.
> 
> Not net/bonding pmd, it's Linux bonding, supported by hw driver.

Got it.

>>
>>>
>>> To be backward compatible, also support the following 2 devargs:
>>> 	-a <pf0>,representor=[0-99] // probe bond0 and representor on pf0
>>> 	-a <pf1>,representor=[0-99] // probe representors on pf1.
>>> If devargs start with PF1 devargs, no owner PF1 created as it disabled
>>> in bonding. Can't create bond0(PF0) automatically here as device is located by PCI address(PF1) from devargs.
>>
>> So, I guess the problem is vendor-specific bonding in HW. Anyway legacy backward compatible representor spec should not require
>> representors info since it worked before without it. So, it does not sound like a reason to have representors info on a representor
>> itself.
> 
> Legacy backward logic could be something like this: if PF owner port found, use it, fallback to current representor.
> This won't break anything I guess, how do you think? 

Logically even in legacy backward compatibility PF1 VFs representors
have parent port ID - PF0 which is a bond of PF0 & PF1. So,
parent_port_id should be filled in. In this case eth_representor_cmp()
will do rte_eth_representor_id_get(PF0-bond-id, -1, -1, VF, &id) which
will return PF0 VF representor ID. Most likely it will even match and
everything works, but it is still incorrect.

In fact, I have another idea. Try to do:
rte_eth_representor_id_get(representor-port-id, ...) first
for the backward compatibility case and, if not supported, do
it on parent port ID.

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

[Andrew Rybchenko]

On 7/29/21 7:20 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]

>> 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.

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

[Xueming(Steven) Li]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Sunday, August 1, 2021 4:50 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@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@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]
> 
> >> 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.

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

[Xueming(Steven) Li]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Sunday, August 1, 2021 4:40 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> 
> On 7/29/21 7:13 AM, Xueming(Steven) Li wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Tuesday, July 20, 2021 5:00 PM
> >> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
> >> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> >>
> >> On 7/19/21 3:50 PM, Xueming(Steven) Li wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>> Sent: Monday, July 19, 2021 8:36 PM
> >>>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
> >>>> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> >>>>
> >>>> On 7/19/21 2:54 PM, Xueming(Steven) Li wrote:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>>>> Sent: Monday, July 19, 2021 4:46 PM
> >>>>>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; 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>; stable@dpdk.org
> >>>>>> Subject: Re: [PATCH] ethdev: fix representor port ID search by
> >>>>>> name
> >>>>>>
> >>>>>> 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.
> >>>>>
> >>>>> Just recalled that representor port could be probed w/o owner PF, is a force for parent port?
> >>>>
> >>>> I thought that it is impossible and parent port is absolutely
> >>>> required for a representor. Could you provide an example and explain how will it work?
> >>>
> >>> In case of bonding, PF0 and PF1 become one PF port `bond0`, PCI address is PF0.
> >>> 	-a <PF0>,representor=pf[0-1]vf[0-99] // this is the syntax we proposed.
> >>
> >> Is it net/bonding or vendor-specific bonding in HW?
> >> If I remember correctly in the case of net/bonding we have ethdev ports for bonded devices.
> >
> > Not net/bonding pmd, it's Linux bonding, supported by hw driver.
> 
> Got it.
> 
> >>
> >>>
> >>> To be backward compatible, also support the following 2 devargs:
> >>> 	-a <pf0>,representor=[0-99] // probe bond0 and representor on pf0
> >>> 	-a <pf1>,representor=[0-99] // probe representors on pf1.
> >>> If devargs start with PF1 devargs, no owner PF1 created as it
> >>> disabled in bonding. Can't create bond0(PF0) automatically here as device is located by PCI address(PF1) from devargs.
> >>
> >> So, I guess the problem is vendor-specific bonding in HW. Anyway
> >> legacy backward compatible representor spec should not require
> >> representors info since it worked before without it. So, it does not sound like a reason to have representors info on a representor
> itself.
> >
> > Legacy backward logic could be something like this: if PF owner port found, use it, fallback to current representor.
> > This won't break anything I guess, how do you think?
> 
> Logically even in legacy backward compatibility PF1 VFs representors have parent port ID - PF0 which is a bond of PF0 & PF1. So,
> parent_port_id should be filled in. In this case eth_representor_cmp() will do rte_eth_representor_id_get(PF0-bond-id, -1, -1, VF, &id)
> which will return PF0 VF representor ID. Most likely it will even match and everything works, but it is still incorrect.

The PF0, bond of PF0 and PF1 will return representor info for VF/SFs under both PFs. It should work.

> 
> In fact, I have another idea. Try to do:
> rte_eth_representor_id_get(representor-port-id, ...) first for the backward compatibility case and, if not supported, do it on parent
> port ID.

Looks good to me