[dpdk-dev,v3,1/4] ethdev: add firmware information get

  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(+)
  

2016-12-27 20:30, Qiming Yang:
>  /**
> + * Retrieve the firmware version of a device.
> + *
> + * @param port_id
> + *   The port identifier of the device.
> + * @param fw_major
> + *   A array pointer to store the major firmware version of a device.
> + * @param fw_minor
> + *   A array pointer to store the minor firmware version of a device.
> + * @param fw_patch
> + *   A array pointer to store the firmware patch number of a device.
> + * @param etrack_id
> + *   A array pointer to store the nvm version of a device.
> + */
> +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 have a reserve about the naming etrack_id.
Please could you point to a document explaining this ID?
Is it known outside of Intel?
  

Hi, Ferruh
Please see the question below. In my opinion, etrack_id is just a name used to define the ID of one NIC.
In kernel version ethtool, it will print this ID in the line of firmware verison. 
I know what is etrack_id mean, but I really don't know why this named etrack_id.
Can you explain this question?
 
-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
Sent: Tuesday, January 3, 2017 4:40 PM
To: Yang, Qiming <qiming.yang@intel.com>
Subject: Re: [PATCH v3 1/4] ethdev: add firmware information get

Please reply below the question and on the mailing list.
You'll have to explain why this name etrack_id.

2017-01-03 03:28, Yang, Qiming:
> Hi, Thomas
> etrack_id is not a terminology, it's decided by me.
> Which is store the unique number of the firmware.
> firmware-version: 5.04 0x800024ca
> 800024ca is the etrack_id of this NIC.
> 
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
> Sent: Monday, January 2, 2017 11:39 PM
> To: Yang, Qiming <qiming.yang@intel.com>
> Cc: dev@dpdk.org; Horton, Remy <remy.horton@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [PATCH v3 1/4] ethdev: add firmware information get
> 
> 2016-12-27 20:30, Qiming Yang:
> >  /**
> > + * Retrieve the firmware version of a device.
> > + *
> > + * @param port_id
> > + *   The port identifier of the device.
> > + * @param fw_major
> > + *   A array pointer to store the major firmware version of a device.
> > + * @param fw_minor
> > + *   A array pointer to store the minor firmware version of a device.
> > + * @param fw_patch
> > + *   A array pointer to store the firmware patch number of a device.
> > + * @param etrack_id
> > + *   A array pointer to store the nvm version of a device.
> > + */
> > +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 have a reserve about the naming etrack_id.
> Please could you point to a document explaining this ID?
> Is it known outside of Intel?
  

On 1/3/2017 9:05 AM, Yang, Qiming wrote:
> Hi, Ferruh
> Please see the question below. In my opinion, etrack_id is just a name used to define the ID of one NIC.
> In kernel version ethtool, it will print this ID in the line of firmware verison. 
> I know what is etrack_id mean, but I really don't know why this named etrack_id.

Hi Qiming,

I suggested the API based on fields you already used in your patch.

So, this API is to get FW version, is etrack_id something that defines
(part of) firmware version?

Thanks,
ferruh


> Can you explain this question?
>  
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
> Sent: Tuesday, January 3, 2017 4:40 PM
> To: Yang, Qiming <qiming.yang@intel.com>
> Subject: Re: [PATCH v3 1/4] ethdev: add firmware information get
> 
> Please reply below the question and on the mailing list.
> You'll have to explain why this name etrack_id.
> 
> 2017-01-03 03:28, Yang, Qiming:
>> Hi, Thomas
>> etrack_id is not a terminology, it's decided by me.
>> Which is store the unique number of the firmware.
>> firmware-version: 5.04 0x800024ca
>> 800024ca is the etrack_id of this NIC.
>>
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
>> Sent: Monday, January 2, 2017 11:39 PM
>> To: Yang, Qiming <qiming.yang@intel.com>
>> Cc: dev@dpdk.org; Horton, Remy <remy.horton@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Subject: Re: [PATCH v3 1/4] ethdev: add firmware information get
>>
>> 2016-12-27 20:30, Qiming Yang:
>>>  /**
>>> + * Retrieve the firmware version of a device.
>>> + *
>>> + * @param port_id
>>> + *   The port identifier of the device.
>>> + * @param fw_major
>>> + *   A array pointer to store the major firmware version of a device.
>>> + * @param fw_minor
>>> + *   A array pointer to store the minor firmware version of a device.
>>> + * @param fw_patch
>>> + *   A array pointer to store the firmware patch number of a device.
>>> + * @param etrack_id
>>> + *   A array pointer to store the nvm version of a device.
>>> + */
>>> +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 have a reserve about the naming etrack_id.
>> Please could you point to a document explaining this ID?
>> Is it known outside of Intel?
> 
>
  

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;
>
  

Yes, in my opinion it is. And I use this name already exist in the share code from ND team.

-----Original Message-----
From: Yigit, Ferruh 
Sent: Tuesday, January 3, 2017 10:49 PM
To: Yang, Qiming <qiming.yang@intel.com>
Cc: dev@dpdk.org; Horton, Remy <remy.horton@intel.com>; Thomas Monjalon <thomas.monjalon@6wind.com>
Subject: Re: [PATCH v3 1/4] ethdev: add firmware information get

On 1/3/2017 9:05 AM, Yang, Qiming wrote:
> Hi, Ferruh
> Please see the question below. In my opinion, etrack_id is just a name used to define the ID of one NIC.
> In kernel version ethtool, it will print this ID in the line of firmware verison. 
> I know what is etrack_id mean, but I really don't know why this named etrack_id.

Hi Qiming,

I suggested the API based on fields you already used in your patch.

So, this API is to get FW version, is etrack_id something that defines (part of) firmware version?

Thanks,
ferruh


> Can you explain this question?
>  
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, January 3, 2017 4:40 PM
> To: Yang, Qiming <qiming.yang@intel.com>
> Subject: Re: [PATCH v3 1/4] ethdev: add firmware information get
> 
> Please reply below the question and on the mailing list.
> You'll have to explain why this name etrack_id.
> 
> 2017-01-03 03:28, Yang, Qiming:
>> Hi, Thomas
>> etrack_id is not a terminology, it's decided by me.
>> Which is store the unique number of the firmware.
>> firmware-version: 5.04 0x800024ca
>> 800024ca is the etrack_id of this NIC.
>>
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>> Sent: Monday, January 2, 2017 11:39 PM
>> To: Yang, Qiming <qiming.yang@intel.com>
>> Cc: dev@dpdk.org; Horton, Remy <remy.horton@intel.com>; Yigit, Ferruh 
>> <ferruh.yigit@intel.com>
>> Subject: Re: [PATCH v3 1/4] ethdev: add firmware information get
>>
>> 2016-12-27 20:30, Qiming Yang:
>>>  /**
>>> + * Retrieve the firmware version of a device.
>>> + *
>>> + * @param port_id
>>> + *   The port identifier of the device.
>>> + * @param fw_major
>>> + *   A array pointer to store the major firmware version of a device.
>>> + * @param fw_minor
>>> + *   A array pointer to store the minor firmware version of a device.
>>> + * @param fw_patch
>>> + *   A array pointer to store the firmware patch number of a device.
>>> + * @param etrack_id
>>> + *   A array pointer to store the nvm version of a device.
>>> + */
>>> +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 have a reserve about the naming etrack_id.
>> Please could you point to a document explaining this ID?
>> Is it known outside of Intel?
> 
>
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yang, Qiming
> Sent: Wednesday, January 4, 2017 11:33 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Horton, Remy <remy.horton@intel.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add firmware information get
> 
> Yes, in my opinion it is. And I use this name already exist in the share code from
> ND team.
> 
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, January 3, 2017 10:49 PM
> To: Yang, Qiming <qiming.yang@intel.com>
> Cc: dev@dpdk.org; Horton, Remy <remy.horton@intel.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>
> Subject: Re: [PATCH v3 1/4] ethdev: add firmware information get
> 
> On 1/3/2017 9:05 AM, Yang, Qiming wrote:
> > Hi, Ferruh
> > Please see the question below. In my opinion, etrack_id is just a name used to
> define the ID of one NIC.
> > In kernel version ethtool, it will print this ID in the line of firmware verison.
> > I know what is etrack_id mean, but I really don't know why this named
> etrack_id.
> 
> Hi Qiming,
> 
> I suggested the API based on fields you already used in your patch.
> 
> So, this API is to get FW version, is etrack_id something that defines (part of)
> firmware version?
> 
> Thanks,
> ferruh
> 
> 
Different HW may have different version format, so it is better to use string.

And I prefer the API definition in your v2 patch like

rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int fw_length);

Thanks
Jingjing
  

On 1/4/2017 7:48 AM, Wu, Jingjing wrote:
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yang, Qiming
>> Sent: Wednesday, January 4, 2017 11:33 AM
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; Horton, Remy <remy.horton@intel.com>; Thomas Monjalon
>> <thomas.monjalon@6wind.com>
>> Subject: Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add firmware information get
>>
>> Yes, in my opinion it is. And I use this name already exist in the share code from
>> ND team.
>>
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, January 3, 2017 10:49 PM
>> To: Yang, Qiming <qiming.yang@intel.com>
>> Cc: dev@dpdk.org; Horton, Remy <remy.horton@intel.com>; Thomas Monjalon
>> <thomas.monjalon@6wind.com>
>> Subject: Re: [PATCH v3 1/4] ethdev: add firmware information get
>>
>> On 1/3/2017 9:05 AM, Yang, Qiming wrote:
>>> Hi, Ferruh
>>> Please see the question below. In my opinion, etrack_id is just a name used to
>> define the ID of one NIC.
>>> In kernel version ethtool, it will print this ID in the line of firmware verison.
>>> I know what is etrack_id mean, but I really don't know why this named
>> etrack_id.
>>
>> Hi Qiming,
>>
>> I suggested the API based on fields you already used in your patch.
>>
>> So, this API is to get FW version, is etrack_id something that defines (part of)
>> firmware version?
>>
>> Thanks,
>> ferruh
>>
>>
> Different HW may have different version format, so it is better to use string.
> 
> And I prefer the API definition in your v2 patch like
> 
> rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int fw_length);

The problem with this is the format and content of the string is not
defined, as you said different HW has different version format. This is
no problem if you will only print the string.

But this is a public API, if an application wants to call this API and
do something useful according the FW version information, it will need
to parse the string, and it will not able to parse it because format of
the string is not defined. By making API fill some defined variables,
app won't need to parse them, and API output won't be HW dependent.

Thanks,
ferruh

> 
> Thanks
> Jingjing
>
  

> > Different HW may have different version format, so it is better to use string.
> >
> > And I prefer the API definition in your v2 patch like
> >
> > rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int
> > fw_length);
> 
> The problem with this is the format and content of the string is not defined, as
> you said different HW has different version format. This is no problem if you will
> only print the string.
> 
> But this is a public API, if an application wants to call this API and do something
> useful according the FW version information, it will need to parse the string, and
> it will not able to parse it because format of the string is not defined. By making
> API fill some defined variables, app won't need to parse them, and API output
> won't be HW dependent.
> 

As my understand, the firmware version is specific things to each HW. The format
cannot be generic at all, or at least we have no standard to follow. I think
the API should not be HW dependent, but about the output, application should know
What the string's meaning. Otherwise why we need to provide the firmware info?

Thanks
Jingjing