Re: [PATCH] ethdev: introduce ethdev dump API

["Min Hu (Connor)" <humin29@huawei.com>]

HI, Ferruh,
   v2 has been sent according comments, please check it, thanks.

在 2022/2/7 19:46, Ferruh Yigit 写道:
> 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 {
> 
> .