[dpdk-dev] [PATCH v9 1/5] ethdev: add firmware version get

[Andrew Rybchenko]

On 01/16/2017 08:44 AM, Qiming Yang wrote:
> This patch adds a new API 'rte_eth_dev_fw_version_get' for
> fetching firmware version by a given device.
>
> Signed-off-by: Qiming Yang <qiming.yang at intel.com>
> Acked-by: Remy Horton <remy.horton at intel.com>
> ---
>   doc/guides/nics/features/default.ini   |  1 +
>   doc/guides/rel_notes/deprecation.rst   |  4 ----
>   doc/guides/rel_notes/release_17_02.rst |  5 +++++
>   lib/librte_ether/rte_ethdev.c          | 12 ++++++++++++
>   lib/librte_ether/rte_ethdev.h          | 26 ++++++++++++++++++++++++++
>   lib/librte_ether/rte_ether_version.map |  1 +
>   6 files changed, 45 insertions(+), 4 deletions(-)
>
> diff --git a/doc/guides/nics/features/default.ini b/doc/guides/nics/features/default.ini
> index f1bf9bf..ae40d57 100644
> --- a/doc/guides/nics/features/default.ini
> +++ b/doc/guides/nics/features/default.ini
> @@ -50,6 +50,7 @@ Timesync             =
>   Basic stats          =
>   Extended stats       =
>   Stats per queue      =
> +FW version           =
>   EEPROM dump          =
>   Registers dump       =
>   Multiprocess aware   =
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 054e2e7..755dc65 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -30,10 +30,6 @@ Deprecation Notices
>     ``nb_seg_max`` and ``nb_mtu_seg_max`` providing information about number of
>     segments limit to be transmitted by device for TSO/non-TSO packets.
>   
> -* In 17.02 ABI change is planned: the ``rte_eth_dev_info`` structure
> -  will be extended with a new member ``fw_version`` in order to store
> -  the NIC firmware version.
> -
>   * ethdev: an API change is planned for 17.02 for the function
>     ``_rte_eth_dev_callback_process``. In 17.02 the function will return an ``int``
>     instead of ``void`` and a fourth parameter ``void *ret_param`` will be added.
> diff --git a/doc/guides/rel_notes/release_17_02.rst b/doc/guides/rel_notes/release_17_02.rst
> index 5762d3f..f9134bb 100644
> --- a/doc/guides/rel_notes/release_17_02.rst
> +++ b/doc/guides/rel_notes/release_17_02.rst
> @@ -66,6 +66,11 @@ New Features
>     Support for Mellanox ConnectX-5 family of 10/25/40/50/100 Gbps adapters
>     has been added to the existing mlx5 PMD.
>   
> +* **Added firmware version get API.**
> +
> +  Added a new function ``rte_eth_dev_fw_version_get()`` to fetch firmware
> +  version by a given device.
> +
>   Resolved Issues
>   ---------------
>   
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 917557a..89cffcf 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1588,6 +1588,18 @@ rte_eth_dev_set_rx_queue_stats_mapping(uint8_t port_id, uint16_t rx_queue_id,
>   			STAT_QMAP_RX);
>   }
>   
> +int
> +rte_eth_dev_fw_version_get(uint8_t port_id, char *fw_version, size_t fw_size)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fw_version_get, -ENOTSUP);
> +	return (*dev->dev_ops->fw_version_get)(dev, fw_version, fw_size);

I think it would be good to handle difference from snprintf() behaviour 
here and specify
that fw_version_get callback has exactly snprintf()-like return value. 
It would allow to avoid
duplicated code in all drivers (adding 1 for terminating null, 
conversion of success value to 0).
Also I think warning about insufficient space is not required. It could 
be intentional to call
the first time with 0 (or some small) space to get required space to be 
(re)allocated.
May be debug level message would be useful.

> +}
> +
>   void
>   rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info)
>   {
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index ded43d7..37a55ef 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -1177,6 +1177,10 @@ typedef uint32_t (*eth_rx_queue_count_t)(struct rte_eth_dev *dev,
>   typedef int (*eth_rx_descriptor_done_t)(void *rxq, uint16_t offset);
>   /**< @internal Check DD bit of specific RX descriptor */
>   
> +typedef int (*eth_fw_version_get_t)(struct rte_eth_dev *dev,
> +				     char *fw_version, size_t fw_size);
> +/**< @internal Get firmware information of an Ethernet device. */
> +

If we finally have different return value for 
rte_eth_dev_fw_version_get() and here,
it would be useful to highlight it here.

>   typedef void (*eth_rxq_info_get_t)(struct rte_eth_dev *dev,
>   	uint16_t rx_queue_id, struct rte_eth_rxq_info *qinfo);
>   
> @@ -1459,6 +1463,7 @@ struct eth_dev_ops {
>   	eth_dev_infos_get_t        dev_infos_get; /**< Get device info. */
>   	eth_rxq_info_get_t         rxq_info_get; /**< retrieve RX queue information. */
>   	eth_txq_info_get_t         txq_info_get; /**< retrieve TX queue information. */
> +	eth_fw_version_get_t       fw_version_get; /**< Get firmware version. */
>   	eth_dev_supported_ptypes_get_t dev_supported_ptypes_get;
>   	/**< Get packet types supported and identified by device. */
>   
> @@ -2396,6 +2401,27 @@ void rte_eth_macaddr_get(uint8_t port_id, struct ether_addr *mac_addr);
>   void rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info);
>   
>   /**
> + * Retrieve the firmware version of a device.
> + *
> + * @param port_id
> + *   The port identifier of the device.
> + * @param fw_version
> + *   A array pointer to store the firmware version of a device,
> + *   allocated by caller.
> + * @param fw_size
> + *   The size of the array pointed by fw_version, which should be
> + *   large enough to store firmware version of the device.
> + * @return
> + *   - (0) if successful.
> + *   - (-ENOTSUP) if operation is not supported.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (>0) if *fw_size* is not enough to store firmware version, return
> + *          the size of the non truncated string.

It is OK for me to keep 0 for success here and it is right in this case 
to include terminating null
iin the return value if size is insufficient (to cover corner case with 
empty FW version).
Please, highlight that terminating null is included here. It is the 
difference from snprintf() and
it should be 100% clear.

> + */
> +int rte_eth_dev_fw_version_get(uint8_t port_id,
> +			       char *fw_version, size_t fw_size);
> +
> +/**
>    * Retrieve the supported packet types of an Ethernet device.
>    *
>    * When a packet type is announced as supported, it *must* be recognized by
> diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
> index 0c2859e..c6c9d0d 100644
> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -146,6 +146,7 @@ DPDK_17.02 {
>   	global:
>   
>   	_rte_eth_dev_reset;
> +	rte_eth_dev_fw_version_get;
>   	rte_flow_create;
>   	rte_flow_destroy;
>   	rte_flow_flush;