Re: [dpdk-dev] [PATCH v8 1/2] ethdev: add an API to get device configuration info

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 8/27/2021 9:17 AM, Jie Wang wrote:
> This patch adds a new API "rte_eth_dev_conf_info_get()" to help users get
> device configuration info.
> 
> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> ---
>  lib/ethdev/rte_ethdev.c | 28 ++++++++++++++++++++++++++++
>  lib/ethdev/rte_ethdev.h | 21 +++++++++++++++++++++
>  lib/ethdev/version.map  |  3 +++
>  3 files changed, 52 insertions(+)
> 

Can you add a release notes update for this?

> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 9d95cd11e1..bbe24af662 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -3458,6 +3458,34 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>  	return 0;
>  }
>  
> +int
> +rte_eth_dev_conf_info_get(uint16_t port_id,
> +				struct rte_eth_conf *dev_conf_info)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (dev_conf_info == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u configuration info to NULL\n",

Can you break the line after 'ERR', to reduce the column length?

> +			port_id);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Init dev_conf_info before port_id check since caller does not have
> +	 * return status and does not know if get is successful or not.
> +	 */
> +	memset(dev_conf_info, 0, sizeof(struct rte_eth_conf));
> +

I didn't get the reasoning for the memset, won't it be overwriten anyway by
below 'memcpy'?

> +	/* copy dev->data->dev_conf to dev_conf_info */
> +	memcpy(dev_conf_info, &dev->data->dev_conf,
> +		sizeof(struct rte_eth_conf));
> +
> +	return 0;
> +}
> +
>  int
>  rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
>  				 uint32_t *ptypes, int num)
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index d2b27c351f..3c5b02a133 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -3058,6 +3058,27 @@ int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
>   */
>  int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
>  
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Retrieve the contextual information of an Ethernet device.
> + *

'rte_eth_dev_info_get()' is using "contextual information" but for this API can
we say 'configuration'?

> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param dev_conf_info
> + *   A pointer to a structure of type *rte_eth_dev* to be filled with
> + *   the contextual information of the Ethernet device.

Just to make it more clear, what do you think to highlight that memory should be
allocated by the caller?

> + * @return
> + *   - (0) if successful.
> + *   - (-ENOTSUP) if support for dev_infos_get() does not exist for the device.

I think '-ENOTSUP' is not returned by the API.

> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EINVAL) if bad parameter.
> + */
> +__rte_experimental
> +int rte_eth_dev_conf_info_get(uint16_t port_id,
> +				struct rte_eth_conf *dev_conf_info);
> +
>  /**
>   * Retrieve the firmware version of a device.
>   *
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 44d30b05ae..40539f99f9 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -249,6 +249,9 @@ EXPERIMENTAL {
>  	rte_mtr_meter_policy_delete;
>  	rte_mtr_meter_policy_update;
>  	rte_mtr_meter_policy_validate;
> +
> +	# added in 21.11
> +	rte_eth_dev_conf_info_get;
>  };
>  
>  INTERNAL {
>