Re: [PATCH v3 1/4] ethdev: add firmware information get

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

On 12/27/2016 12:30 PM, Qiming Yang wrote:
> This patch adds a new API 'rte_eth_dev_fw_info_get' for fetching
> firmware related information by a given device.
> 
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
> Acked-by: Remy Horton <remy.horton@intel.com>
> ---
> v2 changes:
> * modified some comment statements.
> v3 changes:
> * change API, use rte_eth_dev_fw_info_get(uint8_t port_id,
>   uint32_t *fw_major, uint32_t *fw_minor, uint32_t *fw_patch,
>   uint32_t *etrack_id) instead of rte_eth_dev_fwver_get(uint8_t port_id,
>   char *fw_version, int fw_length).
>   Add statusment in /doc/guides/nics/features/default.ini and
>   release_17_02.rst.
> ---
> ---
>  doc/guides/nics/features/default.ini   |  1 +
>  doc/guides/rel_notes/release_17_02.rst |  4 ++++
>  lib/librte_ether/rte_ethdev.c          | 14 ++++++++++++++
>  lib/librte_ether/rte_ethdev.h          | 23 +++++++++++++++++++++++
>  lib/librte_ether/rte_ether_version.map |  1 +
>  5 files changed, 43 insertions(+)

This patch also should remove deprecation notice.
Item 3 of the requested changes.

> 
> diff --git a/doc/guides/nics/features/default.ini b/doc/guides/nics/features/default.ini
> index f1bf9bf..8237ee4 100644
> --- a/doc/guides/nics/features/default.ini
> +++ b/doc/guides/nics/features/default.ini
> @@ -66,3 +66,4 @@ x86-64               =
>  Usage doc            =
>  Design doc           =
>  Perf doc             =
> +FW version           =

I am not sure about this location, I think it can be before "EEPROM
dump", what do you think?

> diff --git a/doc/guides/rel_notes/release_17_02.rst b/doc/guides/rel_notes/release_17_02.rst
> index 180af82..f6dc6c0 100644
> --- a/doc/guides/rel_notes/release_17_02.rst
> +++ b/doc/guides/rel_notes/release_17_02.rst
> @@ -52,6 +52,10 @@ New Features
>    See the :ref:`Generic flow API <Generic_flow_API>` documentation for more
>    information.
>  
> +* **Added firmware information get API.**
> + Added a new function ``rte_eth_dev_fw_info_get()`` to fetch firmware related
> + information by a given device. Information include major firmware version,
> + minor firmware version, patch number and etrack id.
>  
>  Resolved Issues
>  ---------------
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 280f0db..f399f09 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1586,6 +1586,20 @@ rte_eth_dev_set_rx_queue_stats_mapping(uint8_t port_id, uint16_t rx_queue_id,
>  }
>  
>  void
> +rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t *fw_major, uint32_t *fw_minor,
> +	uint32_t *fw_patch, uint32_t *etrack_id)

I am for rte_eth_dev_fw_version_get(), to limit the scope of the API.
And API name and eth_dev_ops name should match..

> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_RET(port_id);
> +	dev = &rte_eth_devices[port_id];
> +

What do you think setting all arguments to zero here?

> +	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->fw_version_get);
> +	(*dev->dev_ops->fw_version_get)(dev, fw_major, fw_minor,
> +					fw_patch, etrack_id);
> +}
> +

<...>

> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -156,5 +156,6 @@ DPDK_17.02 {
>  	rte_flow_flush;
>  	rte_flow_query;
>  	rte_flow_validate;
> +	rte_eth_dev_fw_info_get;

Please add this alphabetically sorted.

>  
>  } DPDK_16.11;
>