[v2] ethdev: fix representor port ID search by name

Message ID 20210818140004.2812575-1-andrew.rybchenko@oktetlabs.ru (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] ethdev: fix representor port ID search by name |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/github-robot: build fail github build: failed
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-aarch64-compile-testing fail Testing issues
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-x86_64-unit-testing fail Testing issues
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS

Commit Message

Andrew Rybchenko Aug. 18, 2021, 2 p.m. UTC
  From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>

Getting a list of representors from a representor does not make sense.
Instead, a parent device should be used.

To this end, extend the rte_eth_dev_data structure to include the port ID
of the parent device for representors.

Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
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         | 17 +++++++++++++++++
 drivers/net/mlx5/windows/mlx5_os.c       | 17 +++++++++++++++++
 lib/ethdev/ethdev_driver.h               |  6 +++---
 lib/ethdev/rte_class_eth.c               | 22 ++++++++++++++++++++--
 lib/ethdev/rte_ethdev.c                  |  8 ++++----
 lib/ethdev/rte_ethdev_core.h             |  4 ++++
 11 files changed, 70 insertions(+), 9 deletions(-)
  

Comments

Xueming Li Aug. 27, 2021, 9:18 a.m. UTC | #1
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Wednesday, August 18, 2021 10:00 PM
> To: Ajit Khaparde <ajit.khaparde@broadcom.com>; Somnath Kotur <somnath.kotur@broadcom.com>; John Daley
> <johndale@cisco.com>; Hyong Youb Kim <hyonkim@cisco.com>; Beilei Xing <beilei.xing@intel.com>; Qiming Yang
> <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>; Haiyue Wang <haiyue.wang@intel.com>; Matan Azrad
> <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>; Xueming(Steven) Li <xuemingl@nvidia.com>
> Subject: [PATCH v2] ethdev: fix representor port ID search by name
> 
> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> 
> Getting a list of representors from a representor does not make sense.
> Instead, a parent device should be used.
> 
> To this end, extend the rte_eth_dev_data structure to include the port ID of the parent device for representors.
> 
> Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
> 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         | 17 +++++++++++++++++
>  drivers/net/mlx5/windows/mlx5_os.c       | 17 +++++++++++++++++
>  lib/ethdev/ethdev_driver.h               |  6 +++---
>  lib/ethdev/rte_class_eth.c               | 22 ++++++++++++++++++++--
>  lib/ethdev/rte_ethdev.c                  |  8 ++++----
>  lib/ethdev/rte_ethdev_core.h             |  4 ++++
>  11 files changed, 70 insertions(+), 9 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 5f8766aa48..a68fa7beb7 100644
> --- a/drivers/net/mlx5/linux/mlx5_os.c
> +++ b/drivers/net/mlx5/linux/mlx5_os.c
> @@ -1677,6 +1677,23 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>  	if (priv->representor) {
>  		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
>  		eth_dev->data->representor_id = priv->representor_id;
> +		MLX5_ETH_FOREACH_DEV(port_id, priv->pci_dev) {
> +			struct mlx5_priv *opriv =
> +				rte_eth_devices[port_id].data->dev_private;
> +			if (opriv &&
> +			    opriv->master &&
> +			    opriv->domain_id == priv->domain_id &&
> +			    opriv->sh == priv->sh) {
> +				eth_dev->data->parent_port_id =
> +					rte_eth_devices[port_id].data->port_id;
> +				break;
> +			}
> +		}
> +		if (port_id >= RTE_MAX_ETHPORTS) {
> +			DRV_LOG(ERR, "no master device for representor");
> +			err = ENODEV;
> +			goto error;
> +		}
>  	}
>  	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 7e1df1c751..0c5a02bfcb 100644
> --- a/drivers/net/mlx5/windows/mlx5_os.c
> +++ b/drivers/net/mlx5/windows/mlx5_os.c
> @@ -543,6 +543,23 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>  	if (priv->representor) {
>  		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
>  		eth_dev->data->representor_id = priv->representor_id;
> +		MLX5_ETH_FOREACH_DEV(port_id, priv->pci_dev) {
> +			struct mlx5_priv *opriv =
> +				rte_eth_devices[port_id].data->dev_private;
> +			if (opriv &&
> +			    opriv->master &&
> +			    opriv->domain_id == priv->domain_id &&
> +			    opriv->sh == priv->sh) {
> +				eth_dev->data->parent_port_id =
> +					rte_eth_devices[port_id].data->port_id;

Could this value different than port_id?

> +				break;
> +			}
> +		}
> +		if (port_id >= RTE_MAX_ETHPORTS) {
> +			DRV_LOG(ERR, "no master device for representor");
> +			err = ENODEV;
> +			goto error;

Here shouldn't be an error.
Parent port ID default to 0, it could be wrong if multiple PF probed, let's default to current port ID.

> +		}
>  	}
>  	/*
>  	 * Store associated network device interface index. This index diff --git a/lib/ethdev/ethdev_driver.h
> b/lib/ethdev/ethdev_driver.h index fd5b7ca550..d1a1499538 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1287,8 +1287,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 port_id
> + *  Port ID of the backing device.
>   * @param type
>   *  Representor type.
>   * @param controller
> @@ -1305,7 +1305,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 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..167d2d798c 100644
> --- a/lib/ethdev/rte_class_eth.c
> +++ b/lib/ethdev/rte_class_eth.c
> @@ -95,14 +95,32 @@ 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,
> +		/*
> +		 * rte_eth_representor_id_get expects to receive port ID of
> +		 * the master device, but in order to maintain compatibility
> +		 * with mlx5's hardware bonding and legacy representor
> +		 * specification using just VF numbers, the representor's port
> +		 * ID is tried first.
> +		 */
> +		ret = rte_eth_representor_id_get(edev->data->port_id,
>  			eth_da.type,
>  			eth_da.nb_mh_controllers == 0 ? -1 :
>  					eth_da.mh_controllers[c],
>  			eth_da.nb_ports == 0 ? -1 : eth_da.ports[p],
>  			eth_da.nb_representor_ports == 0 ? -1 :
>  					eth_da.representor_ports[f],
> -			&id) < 0)
> +			&id);
> +		if (ret == -ENOTSUP)
> +			ret = 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],
> +				eth_da.nb_ports == 0 ? -1 : eth_da.ports[p],
> +				eth_da.nb_representor_ports == 0 ? -1 :
> +						eth_da.representor_ports[f],
> +				&id);
> +		if (ret < 0)
>  			continue;
>  		if (data->representor_id == id)
>  			return 0;
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index 9d95cd11e1..228ef7bf23 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 port_id,
>  			   enum rte_eth_representor_type type,
>  			   int controller, int pf, int representor_port,
>  			   uint16_t *repr_id)
> @@ -6013,7 +6013,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(port_id, NULL);
>  	if (ret == -ENOTSUP && type == RTE_ETH_REPRESENTOR_VF &&
>  	    controller == -1 && pf == -1) {
>  		/* Direct mapping for legacy VF representor. */ @@ -6028,7 +6028,7 @@ rte_eth_representor_id_get(const struct
> rte_eth_dev *ethdev,
>  	if (info == NULL)
>  		return -ENOMEM;
>  	info->nb_ranges_alloc = n;
> -	ret = rte_eth_representor_info_get(ethdev->data->port_id, info);
> +	ret = rte_eth_representor_info_get(port_id, info);
>  	if (ret < 0)
>  		goto out;
> 
> @@ -6047,7 +6047,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,
> +				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
  
Viacheslav Galaktionov Aug. 27, 2021, 9:48 a.m. UTC | #2
On 2021-08-27 12:18, Xueming(Steven) Li wrote:
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Wednesday, August 18, 2021 10:00 PM
>> To: Ajit Khaparde <ajit.khaparde@broadcom.com>; Somnath Kotur 
>> <somnath.kotur@broadcom.com>; John Daley
>> <johndale@cisco.com>; Hyong Youb Kim <hyonkim@cisco.com>; Beilei Xing 
>> <beilei.xing@intel.com>; Qiming Yang
>> <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>; Haiyue Wang 
>> <haiyue.wang@intel.com>; Matan Azrad
>> <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Slava 
>> Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
>> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; Viacheslav Galaktionov 
>> <viacheslav.galaktionov@oktetlabs.ru>; Xueming(Steven) Li 
>> <xuemingl@nvidia.com>
>> Subject: [PATCH v2] ethdev: fix representor port ID search by name
>> 
>> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
>> 
>> Getting a list of representors from a representor does not make sense.
>> Instead, a parent device should be used.
>> 
>> To this end, extend the rte_eth_dev_data structure to include the port 
>> ID of the parent device for representors.
>> 
>> Signed-off-by: Viacheslav Galaktionov 
>> <viacheslav.galaktionov@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> ---

[snip]

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

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

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

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

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

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

[snip]
  
Xueming Li Aug. 28, 2021, 1:22 p.m. UTC | #3
> -----Original Message-----
> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> Sent: Friday, August 27, 2021 5:48 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; 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>;
> dev@dpdk.org
> Subject: Re: [PATCH v2] ethdev: fix representor port ID search by name
> 
> On 2021-08-27 12:18, Xueming(Steven) Li wrote:
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Wednesday, August 18, 2021 10:00 PM
> >> To: Ajit Khaparde <ajit.khaparde@broadcom.com>; Somnath Kotur
> >> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>; Hyong
> >> Youb Kim <hyonkim@cisco.com>; Beilei Xing <beilei.xing@intel.com>;
> >> Qiming Yang <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>;
> >> Haiyue Wang <haiyue.wang@intel.com>; Matan Azrad <matan@nvidia.com>;
> >> Shahaf Shuler <shahafs@nvidia.com>; Slava Ovsiienko
> >> <viacheslavo@nvidia.com>; NBU-Contact-Thomas Monjalon
> >> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
> >> Cc: dev@dpdk.org; Viacheslav Galaktionov
> >> <viacheslav.galaktionov@oktetlabs.ru>; Xueming(Steven) Li
> >> <xuemingl@nvidia.com>
> >> Subject: [PATCH v2] ethdev: fix representor port ID search by name
> >>
> >> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> >>
> >> Getting a list of representors from a representor does not make sense.
> >> Instead, a parent device should be used.
> >>
> >> To this end, extend the rte_eth_dev_data structure to include the
> >> port ID of the parent device for representors.
> >>
> >> Signed-off-by: Viacheslav Galaktionov
> >> <viacheslav.galaktionov@oktetlabs.ru>
> >> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> ---
> 
> [snip]
> 
> >> b/drivers/net/mlx5/windows/mlx5_os.c
> >> index 7e1df1c751..0c5a02bfcb 100644
> >> --- a/drivers/net/mlx5/windows/mlx5_os.c
> >> +++ b/drivers/net/mlx5/windows/mlx5_os.c
> >> @@ -543,6 +543,23 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
> >>  	if (priv->representor) {
> >>  		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
> >>  		eth_dev->data->representor_id = priv->representor_id;
> >> +		MLX5_ETH_FOREACH_DEV(port_id, priv->pci_dev) {
> >> +			struct mlx5_priv *opriv =
> >> +				rte_eth_devices[port_id].data->dev_private;
> >> +			if (opriv &&
> >> +			    opriv->master &&
> >> +			    opriv->domain_id == priv->domain_id &&
> >> +			    opriv->sh == priv->sh) {
> >> +				eth_dev->data->parent_port_id =
> >> +					rte_eth_devices[port_id].data->port_id;
> >
> > Could this value different than port_id?
> 
> Oh, yes, that's an oversight. Thank you!
> 
> >> +				break;
> >> +			}
> >> +		}
> >> +		if (port_id >= RTE_MAX_ETHPORTS) {
> >> +			DRV_LOG(ERR, "no master device for representor");
> >> +			err = ENODEV;
> >> +			goto error;
> >
> > Here shouldn't be an error.
> 
> What do you mean? Is it normal not to have a master device for a representor?

As discussed before, representor could exists w/o master device, special case.

> 
> > Parent port ID default to 0, it could be wrong if multiple PF probed,
> > let's default to current port ID.
> 
> What is the "current" port ID here? Do you mean the representor's port ID?

Representor port ID.

> If you are talking about the value of the port_id variable, then I suppose it could be set to RTE_MAX_ETHPORTS explicitly if a master
> device isn't found.
> 
> [snip]
  
Andrew Rybchenko Aug. 29, 2021, 8:23 a.m. UTC | #4
On 8/28/21 4:22 PM, Xueming(Steven) Li wrote:
> 
> 
>> -----Original Message-----
>> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
>> Sent: Friday, August 27, 2021 5:48 PM
>> To: Xueming(Steven) Li <xuemingl@nvidia.com>
>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; 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>;
>> dev@dpdk.org
>> Subject: Re: [PATCH v2] ethdev: fix representor port ID search by name
>>
>> On 2021-08-27 12:18, Xueming(Steven) Li wrote:
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Wednesday, August 18, 2021 10:00 PM
>>>> To: Ajit Khaparde <ajit.khaparde@broadcom.com>; Somnath Kotur
>>>> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>; Hyong
>>>> Youb Kim <hyonkim@cisco.com>; Beilei Xing <beilei.xing@intel.com>;
>>>> Qiming Yang <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>;
>>>> Haiyue Wang <haiyue.wang@intel.com>; Matan Azrad <matan@nvidia.com>;
>>>> Shahaf Shuler <shahafs@nvidia.com>; Slava Ovsiienko
>>>> <viacheslavo@nvidia.com>; NBU-Contact-Thomas Monjalon
>>>> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
>>>> Cc: dev@dpdk.org; Viacheslav Galaktionov
>>>> <viacheslav.galaktionov@oktetlabs.ru>; Xueming(Steven) Li
>>>> <xuemingl@nvidia.com>
>>>> Subject: [PATCH v2] ethdev: fix representor port ID search by name
>>>>
>>>> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
>>>>
>>>> Getting a list of representors from a representor does not make sense.
>>>> Instead, a parent device should be used.
>>>>
>>>> To this end, extend the rte_eth_dev_data structure to include the
>>>> port ID of the parent device for representors.
>>>>
>>>> Signed-off-by: Viacheslav Galaktionov
>>>> <viacheslav.galaktionov@oktetlabs.ru>
>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> ---
>>
>> [snip]
>>
>>>> b/drivers/net/mlx5/windows/mlx5_os.c
>>>> index 7e1df1c751..0c5a02bfcb 100644
>>>> --- a/drivers/net/mlx5/windows/mlx5_os.c
>>>> +++ b/drivers/net/mlx5/windows/mlx5_os.c
>>>> @@ -543,6 +543,23 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>>>>  	if (priv->representor) {
>>>>  		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
>>>>  		eth_dev->data->representor_id = priv->representor_id;
>>>> +		MLX5_ETH_FOREACH_DEV(port_id, priv->pci_dev) {
>>>> +			struct mlx5_priv *opriv =
>>>> +				rte_eth_devices[port_id].data->dev_private;
>>>> +			if (opriv &&
>>>> +			    opriv->master &&
>>>> +			    opriv->domain_id == priv->domain_id &&
>>>> +			    opriv->sh == priv->sh) {
>>>> +				eth_dev->data->parent_port_id =
>>>> +					rte_eth_devices[port_id].data->port_id;
>>>
>>> Could this value different than port_id?
>>
>> Oh, yes, that's an oversight. Thank you!
>>
>>>> +				break;
>>>> +			}
>>>> +		}
>>>> +		if (port_id >= RTE_MAX_ETHPORTS) {
>>>> +			DRV_LOG(ERR, "no master device for representor");
>>>> +			err = ENODEV;
>>>> +			goto error;
>>>
>>> Here shouldn't be an error.
>>
>> What do you mean? Is it normal not to have a master device for a representor?
> 
> As discussed before, representor could exists w/o master device, special case.

May I clarify one question. Isn't bond will be a parent for the
second PF VF representors? Will above algorithm find it?
If yes, I think we don't need self fallback here.
If no, it looks a bit wrong to me but may be acceptable for
so complicated case. If it is acceptable to you, we can put
self fallback here, but in this case we don't need
corresponding code which check self port_id first. It
would be even better this way since generic code will be
more clear.

>>
>>> Parent port ID default to 0, it could be wrong if multiple PF probed,
>>> let's default to current port ID.
>>
>> What is the "current" port ID here? Do you mean the representor's port ID?
> 
> Representor port ID.
> 
>> If you are talking about the value of the port_id variable, then I suppose it could be set to RTE_MAX_ETHPORTS explicitly if a master
>> device isn't found.
>>
>> [snip]
  
Xueming Li Aug. 29, 2021, 12:17 p.m. UTC | #5
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Sunday, August 29, 2021 4:23 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>; Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> Cc: 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>; dev@dpdk.org
> Subject: Re: [PATCH v2] ethdev: fix representor port ID search by name
> 
> On 8/28/21 4:22 PM, Xueming(Steven) Li wrote:
> >
> >
> >> -----Original Message-----
> >> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> >> Sent: Friday, August 27, 2021 5:48 PM
> >> To: Xueming(Steven) Li <xuemingl@nvidia.com>
> >> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; 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>;
> >> dev@dpdk.org
> >> Subject: Re: [PATCH v2] ethdev: fix representor port ID search by
> >> name
> >>
> >> On 2021-08-27 12:18, Xueming(Steven) Li wrote:
> >>>> -----Original Message-----
> >>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>> Sent: Wednesday, August 18, 2021 10:00 PM
> >>>> To: Ajit Khaparde <ajit.khaparde@broadcom.com>; Somnath Kotur
> >>>> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>;
> >>>> Hyong Youb Kim <hyonkim@cisco.com>; Beilei Xing
> >>>> <beilei.xing@intel.com>; Qiming Yang <qiming.yang@intel.com>; Qi
> >>>> Zhang <qi.z.zhang@intel.com>; Haiyue Wang <haiyue.wang@intel.com>;
> >>>> Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>;
> >>>> Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
> >>>> Monjalon <thomas@monjalon.net>; Ferruh Yigit
> >>>> <ferruh.yigit@intel.com>
> >>>> Cc: dev@dpdk.org; Viacheslav Galaktionov
> >>>> <viacheslav.galaktionov@oktetlabs.ru>; Xueming(Steven) Li
> >>>> <xuemingl@nvidia.com>
> >>>> Subject: [PATCH v2] ethdev: fix representor port ID search by name
> >>>>
> >>>> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> >>>>
> >>>> Getting a list of representors from a representor does not make sense.
> >>>> Instead, a parent device should be used.
> >>>>
> >>>> To this end, extend the rte_eth_dev_data structure to include the
> >>>> port ID of the parent device for representors.
> >>>>
> >>>> Signed-off-by: Viacheslav Galaktionov
> >>>> <viacheslav.galaktionov@oktetlabs.ru>
> >>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>> ---
> >>
> >> [snip]
> >>
> >>>> b/drivers/net/mlx5/windows/mlx5_os.c
> >>>> index 7e1df1c751..0c5a02bfcb 100644
> >>>> --- a/drivers/net/mlx5/windows/mlx5_os.c
> >>>> +++ b/drivers/net/mlx5/windows/mlx5_os.c
> >>>> @@ -543,6 +543,23 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
> >>>>  	if (priv->representor) {
> >>>>  		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
> >>>>  		eth_dev->data->representor_id = priv->representor_id;
> >>>> +		MLX5_ETH_FOREACH_DEV(port_id, priv->pci_dev) {
> >>>> +			struct mlx5_priv *opriv =
> >>>> +				rte_eth_devices[port_id].data->dev_private;
> >>>> +			if (opriv &&
> >>>> +			    opriv->master &&
> >>>> +			    opriv->domain_id == priv->domain_id &&
> >>>> +			    opriv->sh == priv->sh) {
> >>>> +				eth_dev->data->parent_port_id =
> >>>> +					rte_eth_devices[port_id].data->port_id;
> >>>
> >>> Could this value different than port_id?
> >>
> >> Oh, yes, that's an oversight. Thank you!
> >>
> >>>> +				break;
> >>>> +			}
> >>>> +		}
> >>>> +		if (port_id >= RTE_MAX_ETHPORTS) {
> >>>> +			DRV_LOG(ERR, "no master device for representor");
> >>>> +			err = ENODEV;
> >>>> +			goto error;
> >>>
> >>> Here shouldn't be an error.
> >>
> >> What do you mean? Is it normal not to have a master device for a representor?
> >
> > As discussed before, representor could exists w/o master device, special case.
> 
> May I clarify one question. Isn't bond will be a parent for the second PF VF representors? Will above algorithm find it?
> If yes, I think we don't need self fallback here.

Sorry maybe I was not clear enough. It's true that bond will be parent for second PF  VF representor, the above algorithm can find it.
But if second PF VF representor probe earlier than the bond, please note that bond get probed with first PF, bond won't be found.

> If no, it looks a bit wrong to me but may be acceptable for so complicated case. If it is acceptable to you, we can put self fallback here,
> but in this case we don't need corresponding code which check self port_id first. It would be even better this way since generic code
> will be more clear.

Agree, it's better to make generic ode more clear.

> 
> >>
> >>> Parent port ID default to 0, it could be wrong if multiple PF
> >>> probed, let's default to current port ID.
> >>
> >> What is the "current" port ID here? Do you mean the representor's port ID?
> >
> > Representor port ID.
> >
> >> If you are talking about the value of the port_id variable, then I
> >> suppose it could be set to RTE_MAX_ETHPORTS explicitly if a master device isn't found.
> >>
> >> [snip]
  
Andrew Rybchenko Aug. 31, 2021, 3:42 p.m. UTC | #6
On 8/29/21 3:17 PM, Xueming(Steven) Li wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Sunday, August 29, 2021 4:23 PM
>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
>> Cc: 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>; dev@dpdk.org
>> Subject: Re: [PATCH v2] ethdev: fix representor port ID search by name
>>
>> On 8/28/21 4:22 PM, Xueming(Steven) Li wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
>>>> Sent: Friday, August 27, 2021 5:48 PM
>>>> To: Xueming(Steven) Li <xuemingl@nvidia.com>
>>>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; 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>;
>>>> dev@dpdk.org
>>>> Subject: Re: [PATCH v2] ethdev: fix representor port ID search by
>>>> name
>>>>
>>>> On 2021-08-27 12:18, Xueming(Steven) Li wrote:
>>>>>> -----Original Message-----
>>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> Sent: Wednesday, August 18, 2021 10:00 PM
>>>>>> To: Ajit Khaparde <ajit.khaparde@broadcom.com>; Somnath Kotur
>>>>>> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>;
>>>>>> Hyong Youb Kim <hyonkim@cisco.com>; Beilei Xing
>>>>>> <beilei.xing@intel.com>; Qiming Yang <qiming.yang@intel.com>; Qi
>>>>>> Zhang <qi.z.zhang@intel.com>; Haiyue Wang <haiyue.wang@intel.com>;
>>>>>> Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>;
>>>>>> Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
>>>>>> Monjalon <thomas@monjalon.net>; Ferruh Yigit
>>>>>> <ferruh.yigit@intel.com>
>>>>>> Cc: dev@dpdk.org; Viacheslav Galaktionov
>>>>>> <viacheslav.galaktionov@oktetlabs.ru>; Xueming(Steven) Li
>>>>>> <xuemingl@nvidia.com>
>>>>>> Subject: [PATCH v2] ethdev: fix representor port ID search by name
>>>>>>
>>>>>> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
>>>>>>
>>>>>> Getting a list of representors from a representor does not make sense.
>>>>>> Instead, a parent device should be used.
>>>>>>
>>>>>> To this end, extend the rte_eth_dev_data structure to include the
>>>>>> port ID of the parent device for representors.
>>>>>>
>>>>>> Signed-off-by: Viacheslav Galaktionov
>>>>>> <viacheslav.galaktionov@oktetlabs.ru>
>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> ---
>>>>
>>>> [snip]
>>>>
>>>>>> b/drivers/net/mlx5/windows/mlx5_os.c
>>>>>> index 7e1df1c751..0c5a02bfcb 100644
>>>>>> --- a/drivers/net/mlx5/windows/mlx5_os.c
>>>>>> +++ b/drivers/net/mlx5/windows/mlx5_os.c
>>>>>> @@ -543,6 +543,23 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>>>>>>  	if (priv->representor) {
>>>>>>  		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
>>>>>>  		eth_dev->data->representor_id = priv->representor_id;
>>>>>> +		MLX5_ETH_FOREACH_DEV(port_id, priv->pci_dev) {
>>>>>> +			struct mlx5_priv *opriv =
>>>>>> +				rte_eth_devices[port_id].data->dev_private;
>>>>>> +			if (opriv &&
>>>>>> +			    opriv->master &&
>>>>>> +			    opriv->domain_id == priv->domain_id &&
>>>>>> +			    opriv->sh == priv->sh) {
>>>>>> +				eth_dev->data->parent_port_id =
>>>>>> +					rte_eth_devices[port_id].data->port_id;
>>>>>
>>>>> Could this value different than port_id?
>>>>
>>>> Oh, yes, that's an oversight. Thank you!
>>>>
>>>>>> +				break;
>>>>>> +			}
>>>>>> +		}
>>>>>> +		if (port_id >= RTE_MAX_ETHPORTS) {
>>>>>> +			DRV_LOG(ERR, "no master device for representor");
>>>>>> +			err = ENODEV;
>>>>>> +			goto error;
>>>>>
>>>>> Here shouldn't be an error.
>>>>
>>>> What do you mean? Is it normal not to have a master device for a representor?
>>>
>>> As discussed before, representor could exists w/o master device, special case.
>>
>> May I clarify one question. Isn't bond will be a parent for the second PF VF representors? Will above algorithm find it?
>> If yes, I think we don't need self fallback here.
> 
> Sorry maybe I was not clear enough. It's true that bond will be parent for second PF  VF representor, the above algorithm can find it.
> But if second PF VF representor probe earlier than the bond, please note that bond get probed with first PF, bond won't be found.
> 
>> If no, it looks a bit wrong to me but may be acceptable for so complicated case. If it is acceptable to you, we can put self fallback here,
>> but in this case we don't need corresponding code which check self port_id first. It would be even better this way since generic code
>> will be more clear.
> 
> Agree, it's better to make generic ode more clear.

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

>>
>>>>
>>>>> Parent port ID default to 0, it could be wrong if multiple PF
>>>>> probed, let's default to current port ID.
>>>>
>>>> What is the "current" port ID here? Do you mean the representor's port ID?
>>>
>>> Representor port ID.
>>>
>>>> If you are talking about the value of the port_id variable, then I
>>>> suppose it could be set to RTE_MAX_ETHPORTS explicitly if a master device isn't found.
>>>>
>>>> [snip]
>
  

Patch

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 5f8766aa48..a68fa7beb7 100644
--- a/drivers/net/mlx5/linux/mlx5_os.c
+++ b/drivers/net/mlx5/linux/mlx5_os.c
@@ -1677,6 +1677,23 @@  mlx5_dev_spawn(struct rte_device *dpdk_dev,
 	if (priv->representor) {
 		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
 		eth_dev->data->representor_id = priv->representor_id;
+		MLX5_ETH_FOREACH_DEV(port_id, priv->pci_dev) {
+			struct mlx5_priv *opriv =
+				rte_eth_devices[port_id].data->dev_private;
+			if (opriv &&
+			    opriv->master &&
+			    opriv->domain_id == priv->domain_id &&
+			    opriv->sh == priv->sh) {
+				eth_dev->data->parent_port_id =
+					rte_eth_devices[port_id].data->port_id;
+				break;
+			}
+		}
+		if (port_id >= RTE_MAX_ETHPORTS) {
+			DRV_LOG(ERR, "no master device for representor");
+			err = ENODEV;
+			goto error;
+		}
 	}
 	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 7e1df1c751..0c5a02bfcb 100644
--- a/drivers/net/mlx5/windows/mlx5_os.c
+++ b/drivers/net/mlx5/windows/mlx5_os.c
@@ -543,6 +543,23 @@  mlx5_dev_spawn(struct rte_device *dpdk_dev,
 	if (priv->representor) {
 		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
 		eth_dev->data->representor_id = priv->representor_id;
+		MLX5_ETH_FOREACH_DEV(port_id, priv->pci_dev) {
+			struct mlx5_priv *opriv =
+				rte_eth_devices[port_id].data->dev_private;
+			if (opriv &&
+			    opriv->master &&
+			    opriv->domain_id == priv->domain_id &&
+			    opriv->sh == priv->sh) {
+				eth_dev->data->parent_port_id =
+					rte_eth_devices[port_id].data->port_id;
+				break;
+			}
+		}
+		if (port_id >= RTE_MAX_ETHPORTS) {
+			DRV_LOG(ERR, "no master device for representor");
+			err = ENODEV;
+			goto error;
+		}
 	}
 	/*
 	 * Store associated network device interface index. This index
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index fd5b7ca550..d1a1499538 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1287,8 +1287,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 port_id
+ *  Port ID of the backing device.
  * @param type
  *  Representor type.
  * @param controller
@@ -1305,7 +1305,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 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..167d2d798c 100644
--- a/lib/ethdev/rte_class_eth.c
+++ b/lib/ethdev/rte_class_eth.c
@@ -95,14 +95,32 @@  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,
+		/*
+		 * rte_eth_representor_id_get expects to receive port ID of
+		 * the master device, but in order to maintain compatibility
+		 * with mlx5's hardware bonding and legacy representor
+		 * specification using just VF numbers, the representor's port
+		 * ID is tried first.
+		 */
+		ret = rte_eth_representor_id_get(edev->data->port_id,
 			eth_da.type,
 			eth_da.nb_mh_controllers == 0 ? -1 :
 					eth_da.mh_controllers[c],
 			eth_da.nb_ports == 0 ? -1 : eth_da.ports[p],
 			eth_da.nb_representor_ports == 0 ? -1 :
 					eth_da.representor_ports[f],
-			&id) < 0)
+			&id);
+		if (ret == -ENOTSUP)
+			ret = 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],
+				eth_da.nb_ports == 0 ? -1 : eth_da.ports[p],
+				eth_da.nb_representor_ports == 0 ? -1 :
+						eth_da.representor_ports[f],
+				&id);
+		if (ret < 0)
 			continue;
 		if (data->representor_id == id)
 			return 0;
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 9d95cd11e1..228ef7bf23 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 port_id,
 			   enum rte_eth_representor_type type,
 			   int controller, int pf, int representor_port,
 			   uint16_t *repr_id)
@@ -6013,7 +6013,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(port_id, NULL);
 	if (ret == -ENOTSUP && type == RTE_ETH_REPRESENTOR_VF &&
 	    controller == -1 && pf == -1) {
 		/* Direct mapping for legacy VF representor. */
@@ -6028,7 +6028,7 @@  rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
 	if (info == NULL)
 		return -ENOMEM;
 	info->nb_ranges_alloc = n;
-	ret = rte_eth_representor_info_get(ethdev->data->port_id, info);
+	ret = rte_eth_representor_info_get(port_id, info);
 	if (ret < 0)
 		goto out;
 
@@ -6047,7 +6047,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,
+				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 */