Re: [PATCH] ethdev: introduce ethdev dump API

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

On 2/7/2022 1:47 AM, Min Hu (Connor) wrote:
> Added the ethdev dump API which provides functions for query private info

Isn't API and function are same thing in this contexts?

> from device. There exists many private properties in different PMD drivers,
> such as adapter state, Rx/Tx func algorithm in hns3 PMD. The information of
> these properties is important for debug. As the information is private,
> the new API is introduced.>

In the patch title 'ethdev' is duplicated, can you fix it?

  
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Ray Kinsella <mdr@ashroe.eu>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
>   doc/guides/rel_notes/release_22_03.rst |  7 +++++++
>   lib/ethdev/ethdev_driver.h             | 17 +++++++++++++++++
>   lib/ethdev/rte_ethdev.c                | 15 +++++++++++++++
>   lib/ethdev/rte_ethdev.h                | 16 ++++++++++++++++
>   lib/ethdev/version.map                 |  3 +++
>   5 files changed, 58 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_22_03.rst b/doc/guides/rel_notes/release_22_03.rst
> index b20716c521..5895194cde 100644
> --- a/doc/guides/rel_notes/release_22_03.rst
> +++ b/doc/guides/rel_notes/release_22_03.rst
> @@ -92,6 +92,13 @@ New Features
>     * Called ``rte_ipv4/6_udptcp_cksum_mbuf()`` functions in testpmd csum mode
>       to support software UDP/TCP checksum over multiple segments.
>   
> +* **Added the private ethdev dump API, for query private info of ethdev.**
> +
> +  Added the private ethdev dump API which provides functions for query
> +  private info from device. There exists many private properties in
> +  different PMD drivers. The information of these properties is important
> +  for debug. As the information is private, the new API is introduced.
> +
>   

Can you please move the update up, just above the ethdev PMD updates?

>   Removed Items
>   -------------
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 7d27781f7d..d41d8a2c9d 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -990,6 +990,20 @@ typedef int (*eth_representor_info_get_t)(struct rte_eth_dev *dev,
>   typedef int (*eth_rx_metadata_negotiate_t)(struct rte_eth_dev *dev,
>   				       uint64_t *features);
>   
> +/**
> + * @internal
> + * Dump ethdev private info to a file.
> + *

It doesn't dump the 'ethdev' private info, it dumps the private info from device.

> + * @param file
> + *   A pointer to a file for output.
> + * @param dev
> + *   Port (ethdev) handle.
> + *
> + * @return
> + *   Negative errno value on error, positive value on success.
> + */
> +typedef int (*eth_dev_priv_dump_t)(FILE *file, struct rte_eth_dev *dev);
> +
>   /**
>    * @internal A structure containing the functions exported by an Ethernet driver.
>    */
> @@ -1186,6 +1200,9 @@ struct eth_dev_ops {
>   	 * kinds of metadata to the PMD
>   	 */
>   	eth_rx_metadata_negotiate_t rx_metadata_negotiate;
> +
> +	/** Dump ethdev private info */

Ditto, not 'ethdev' private info.

> +	eth_dev_priv_dump_t eth_dev_priv_dump;
>   };
>   
>   /**
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 917a320afa..07838cc2d9 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -6485,6 +6485,21 @@ rte_eth_rx_metadata_negotiate(uint16_t port_id, uint64_t *features)
>   		       (*dev->dev_ops->rx_metadata_negotiate)(dev, features));
>   }
>   
> +int
> +rte_eth_dev_priv_dump(FILE *file, uint16_t port_id)
> +{
> +	struct rte_eth_dev *dev;
> +	int ret;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +

What do you think to check 'file' pointer against NULL before passing it to
dev_ops?

> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->eth_dev_priv_dump, -ENOTSUP);
> +	ret = (*dev->dev_ops->eth_dev_priv_dump)(file, dev);

Should 'eth_err()' used here?

> +
> +	return ret;
> +}
> +
>   RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
>   
>   RTE_INIT(ethdev_init_telemetry)
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 147cc1ced3..1c00fdbcd2 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -5902,6 +5902,22 @@ rte_eth_tx_buffer(uint16_t port_id, uint16_t queue_id,
>   	return rte_eth_tx_buffer_flush(port_id, queue_id, buffer);
>   }
>   
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Dump ethdev private info to a file.
> + *

Ditto.

Should we highlight that provided data and the order depends on the PMD?

> + * @param file
> + *   A pointer to a file for output.
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @return
> + *   Negative errno value on error, positive value on success.


What does 'errno' mean here, does the API cause errno to be set and return it?


Also it is possible to list possible specific error values, as done with other
API documentation, like:
(0) if successful.
(-ENODEV) if *port_id* is invalid.
(-ENOTSUP) if hardware doesn't support.
....

> + */
> +__rte_experimental
> +int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
> +

What do you think to have the 'port_id' as first argument to be consistent
with the other APIs?

>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 1f7359c846..376ea139aa 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -256,6 +256,9 @@ EXPERIMENTAL {
>   	rte_flow_flex_item_create;
>   	rte_flow_flex_item_release;
>   	rte_flow_pick_transfer_proxy;
> +
> +	# added in 22.03
> +	rte_eth_dev_priv_dump;
>   };
>   
>   INTERNAL {