[dpdk-dev,v5,1/5] ethdev: add firmware version get

  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@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.
v4 changes:
* remove deprecation notice, rename API as rte_eth_dev_fw_version_get.
v5 changes:
* change API, use rte_eth_dev_fw_version_get(uint8_t port_id,
  char *fw_version, int fw_length).
---
---
 doc/guides/nics/features/default.ini   |  1 +
 doc/guides/rel_notes/deprecation.rst   |  4 ----
 doc/guides/rel_notes/release_17_02.rst |  3 +++
 lib/librte_ether/rte_ethdev.c          | 12 ++++++++++++
 lib/librte_ether/rte_ethdev.h          | 20 ++++++++++++++++++++
 lib/librte_ether/rte_ether_version.map |  1 +
 6 files changed, 37 insertions(+), 4 deletions(-)
  

On 01/08/2017 07:11 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@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.
> v4 changes:
> * remove deprecation notice, rename API as rte_eth_dev_fw_version_get.
> v5 changes:
> * change API, use rte_eth_dev_fw_version_get(uint8_t port_id,
>    char *fw_version, int fw_length).
> ---
> ---
>   doc/guides/nics/features/default.ini   |  1 +
>   doc/guides/rel_notes/deprecation.rst   |  4 ----
>   doc/guides/rel_notes/release_17_02.rst |  3 +++
>   lib/librte_ether/rte_ethdev.c          | 12 ++++++++++++
>   lib/librte_ether/rte_ethdev.h          | 20 ++++++++++++++++++++
>   lib/librte_ether/rte_ether_version.map |  1 +
>   6 files changed, 37 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 1438c77..291e03d 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 180af82..260033d 100644
> --- a/doc/guides/rel_notes/release_17_02.rst
> +++ b/doc/guides/rel_notes/release_17_02.rst
> @@ -52,6 +52,9 @@ New Features
>     See the :ref:`Generic flow API <Generic_flow_API>` documentation for more
>     information.
>   
> +* **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 280f0db..cb80476 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1586,6 +1586,18 @@ rte_eth_dev_set_rx_queue_stats_mapping(uint8_t port_id, uint16_t rx_queue_id,
>   }
>   
>   void
> +rte_eth_dev_fw_version_get(uint8_t port_id, char *fw_version, int fw_length)

May be size_t should be used for fw_length? Corresponding argument of 
the snprintf()
has size_t type, sizeof(drvinfo.fw_version) is used as value of the 
parameter.

Also the prototype does not provide a way to communicate that fw_length 
is insufficient
to store firmware version. I'd suggest snprintf()-like return value. It 
is pretty easy for PMD
to provide and convenient for the API function caller to handle.

> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_RET(port_id);
> +	dev = &rte_eth_devices[port_id];
> +
> +	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->fw_version_get);
> +	(*dev->dev_ops->fw_version_get)(dev, fw_version, fw_length);
> +}
> +
> +void
>   rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info)
>   {
>   	struct rte_eth_dev *dev;
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index fb51754..2be31d2 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -1150,6 +1150,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 void (*eth_fw_version_get_t)(struct rte_eth_dev *dev,
> +				     char *fw_version, int fw_length);
> +/**< @internal Get firmware information of an Ethernet device. */
> +
>   typedef void (*eth_rxq_info_get_t)(struct rte_eth_dev *dev,
>   	uint16_t rx_queue_id, struct rte_eth_rxq_info *qinfo);
>   
> @@ -1455,6 +1459,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. */
>   
> @@ -2395,6 +2400,21 @@ 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_length
> + *   The size of the array pointed by fw_version, which should be
> + *   large enough to store firmware version of the device.
> + */
> +void rte_eth_dev_fw_version_get(uint8_t port_id,
> +				char *fw_version, int fw_length);
> +
> +/**
>    * 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 a021781..0cf94ed 100644
> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -151,6 +151,7 @@ DPDK_17.02 {
>   	global:
>   
>   	_rte_eth_dev_reset;
> +	rte_eth_dev_fw_version_get;
>   	rte_flow_create;
>   	rte_flow_destroy;
>   	rte_flow_flush;
  

On Sun,  8 Jan 2017 12:11:31 +0800
Qiming Yang <qiming.yang@intel.com> wrote:

>  void
> +rte_eth_dev_fw_version_get(uint8_t port_id, char *fw_version, int fw_length)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_RET(port_id);
> +	dev = &rte_eth_devices[port_id];
> +
> +	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->fw_version_get);
> +	(*dev->dev_ops->fw_version_get)(dev, fw_version, fw_length);
> +}

Maybe dev argument to fw_version_get should be:
   const struct rte_eth_dev *dev
  

-----Original Message-----
From: Stephen Hemminger [mailto:stephen@networkplumber.org] 
Sent: Monday, January 9, 2017 7:05 AM
To: Yang, Qiming <qiming.yang@intel.com>
Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Helin <helin.zhang@intel.com>; Horton, Remy <remy.horton@intel.com>
Subject: Re: [dpdk-dev] [PATCH v5 1/5] ethdev: add firmware version get

On Sun,  8 Jan 2017 12:11:31 +0800
Qiming Yang <qiming.yang@intel.com> wrote:

>  void
> +rte_eth_dev_fw_version_get(uint8_t port_id, char *fw_version, int 
> +fw_length) {
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_RET(port_id);
> +	dev = &rte_eth_devices[port_id];
> +
> +	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->fw_version_get);
> +	(*dev->dev_ops->fw_version_get)(dev, fw_version, fw_length); }

Maybe dev argument to fw_version_get should be:
   const struct rte_eth_dev *dev
Qiming: do you means the argument to ops fw_version_get? 
why should add 'const'? both two are OK, but we usually use struct rte_eth_dev *dev.
  

On 09/01/2017 07:16, Yang, Qiming wrote:
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
[..]
>>  void
>> +rte_eth_dev_fw_version_get(uint8_t port_id, char *fw_version, int
>> +fw_length) {
>> +	struct rte_eth_dev *dev;
>> +
>> +	RTE_ETH_VALID_PORTID_OR_RET(port_id);
>> +	dev = &rte_eth_devices[port_id];
>> +
>> +	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->fw_version_get);
>> +	(*dev->dev_ops->fw_version_get)(dev, fw_version, fw_length); }
>
> Maybe dev argument to fw_version_get should be:
>    const struct rte_eth_dev *dev
> Qiming: do you means the argument to ops fw_version_get?
> why should add 'const'? both two are OK, but we usually use struct rte_eth_dev *dev.

Does seem a bit odd to me as I don't think any of the other rte_dev_ops 
entrypoints use const. Maybe they should but if that's now policy (I've 
been under a rock recently) probably better to do them all in a seperate 
cleanup patchset..
  

On Mon, 9 Jan 2017 10:01:40 +0000
Remy Horton <remy.horton@intel.com> wrote:

> On 09/01/2017 07:16, Yang, Qiming wrote:
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]  
> [..]
> >>  void
> >> +rte_eth_dev_fw_version_get(uint8_t port_id, char *fw_version, int
> >> +fw_length) {
> >> +	struct rte_eth_dev *dev;
> >> +
> >> +	RTE_ETH_VALID_PORTID_OR_RET(port_id);
> >> +	dev = &rte_eth_devices[port_id];
> >> +
> >> +	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->fw_version_get);
> >> +	(*dev->dev_ops->fw_version_get)(dev, fw_version, fw_length); }  
> >
> > Maybe dev argument to fw_version_get should be:
> >    const struct rte_eth_dev *dev
> > Qiming: do you means the argument to ops fw_version_get?
> > why should add 'const'? both two are OK, but we usually use struct rte_eth_dev *dev.  
> 
> Does seem a bit odd to me as I don't think any of the other rte_dev_ops 
> entrypoints use const. Maybe they should but if that's now policy (I've 
> been under a rock recently) probably better to do them all in a seperate 
> cleanup patchset..

DPDK is somewhat lazy about using const.