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

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

On 10/8/2021 4:41 AM, Jie Wang wrote:
> The driver may change offloads info into dev->data->dev_conf
> in dev_configure which may cause port->dev_conf and port->rx_conf
> contain outdated values.
> 
> This patch adds a new API "rte_eth_dev_conf_get()" to help users
> get device configuration info.
> 

Not sure about "configuration info", what about just 'configuration'?
Is 'info' providing additional benefit?

> Add information about the new API in release notes.
> 
> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> ---
>   doc/guides/rel_notes/release_21_11.rst |  5 +++++
>   lib/ethdev/rte_ethdev.c                | 23 +++++++++++++++++++++++
>   lib/ethdev/rte_ethdev.h                | 21 +++++++++++++++++++++
>   lib/ethdev/version.map                 |  3 +++
>   4 files changed, 52 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index f099b1cca2..c16cc83fd1 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -129,6 +129,11 @@ New Features
>     * Added tests to validate packets hard expiry.
>     * Added tests to verify tunnel header verification in IPsec inbound.
>   
> +* **Added support for users get device configuration.**
> +
> +  * Added an API which can help users get device configuration.
> +    The declarations for the API's can be found in ``rte_ethdev.h``.
> +

Can you please clarify that support is for ethdev?
Also as location of the update in the file, can you please check the section comment,
on where to place this update?

>   
>   Removed Items
>   -------------
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index daf5ca9242..ddbe34e276 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -3457,6 +3457,29 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>   	return 0;
>   }
>   
> +int
> +rte_eth_dev_conf_get(uint16_t port_id,
> +				struct rte_eth_conf *dev_conf_info)

Lines can be merged into single line.

And again what about 'dev_conf' as variable name, instead of '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",

again, can we drop 'info'?

> +			port_id);
> +		return -EINVAL;
> +	}
> +
> +	/* copy dev->data->dev_conf to dev_conf_info */

This comment is not adding more info, almost same with line below.

> +	memcpy(dev_conf_info, &dev->data->dev_conf,
> +		sizeof(struct rte_eth_conf));

Can merge the lines?
  
> +
> +	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 afdc53b674..2934b904ea 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -3082,6 +3082,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 configuration of an Ethernet device.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param dev_conf_info

what about dropping 'info' in the declaration too?

> + *   A pointer to a structure of type *rte_eth_conf* to be filled with
> + *   the configuration of the Ethernet device.
> + *   And the memory of the structure should be allocated by the caller.
> + * @return
> + *   - (0) if successful.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EINVAL) if bad parameter.
> + */
> +__rte_experimental
> +int rte_eth_dev_conf_get(uint16_t port_id,
> +				struct rte_eth_conf *dev_conf_info);
> +

Can merge the lines?

>   /**
>    * Retrieve the firmware version of a device.
>    *
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 904bce6ea1..06f1d8da48 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -247,6 +247,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_get;
>   };
>   
>   INTERNAL {
>