Re: [PATCH v3 2/4] net/e1000: add firmware version get

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

On 1/4/2017 3:14 AM, Yang, Qiming wrote:
> See the reply below.
> 
> -----Original Message-----
> From: Yigit, Ferruh 
> Sent: Tuesday, January 3, 2017 11:03 PM
> To: Yang, Qiming <qiming.yang@intel.com>; dev@dpdk.org; thomas.monjalon@6wind.com
> Cc: Horton, Remy <remy.horton@intel.com>
> Subject: Re: [PATCH v3 2/4] net/e1000: add firmware version get
> 
> On 12/27/2016 12:30 PM, Qiming Yang wrote:
>> This patch adds a new function eth_igb_fw_version_get.
>>
>> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
>> ---
>> v3 changes:
>>  * use eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major,
>>    u32 *fw_minor, u32 *fw_minor, u32 *fw_patch, u32 *etrack_id) instead
>>    of eth_igb_fw_version_get(struct rte_eth_dev *dev, char *fw_version,
>>    int fw_length). Add statusment in /doc/guides/nics/features/igb.ini.
>> ---
>> ---
>>  doc/guides/nics/features/igb.ini |  1 +
>>  drivers/net/e1000/igb_ethdev.c   | 43 ++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 44 insertions(+)
>>
>> diff --git a/doc/guides/nics/features/igb.ini 
>> b/doc/guides/nics/features/igb.ini
>> index 9fafe72..ffd87ba 100644
>> --- a/doc/guides/nics/features/igb.ini
>> +++ b/doc/guides/nics/features/igb.ini
>> @@ -39,6 +39,7 @@ EEPROM dump          = Y
>>  Registers dump       = Y
>>  BSD nic_uio          = Y
>>  Linux UIO            = Y
>> +FW version           = Y
> 
> Please keep same location with default.ini file. Why you are putting this just into middle of the uio and vfio?
> Qiming: It's a clerical error, I want to add this line at the end of this file.
> 
>>  Linux VFIO           = Y
>>  x86-32               = Y
>>  x86-64               = Y
>> diff --git a/drivers/net/e1000/igb_ethdev.c 
>> b/drivers/net/e1000/igb_ethdev.c index 4a15447..25344b7 100644
>> --- a/drivers/net/e1000/igb_ethdev.c
>> +++ b/drivers/net/e1000/igb_ethdev.c
>> @@ -120,6 +120,8 @@ static int eth_igb_xstats_get_names(struct rte_eth_dev *dev,
>>  				    unsigned limit);
>>  static void eth_igb_stats_reset(struct rte_eth_dev *dev);  static 
>> void eth_igb_xstats_reset(struct rte_eth_dev *dev);
>> +static void eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major,
>> +		u32 *fw_minor, u32 *fw_patch, u32 *etrack_id);
> 
> I think you can use a struct as parameter here. But beware, that struct should NOT be a public struct.
> Qiming: I think only add a private struct for igb is unnecessary. Keep the arguments consistent with rte_eth_dev_fw_info_get is better.
> What do you think?

Both are OK.
Normally, I believe using struct is better. But we are not using struct
in public API because of the ABI compatibility issues. Here it is
internal usage, there is no ABI breakage concern, so it may be possible
to use a struct.
But if you prefer to keep the arguments same here with public API, that
is fine.

> 
>>  static void eth_igb_infos_get(struct rte_eth_dev *dev,
>>  			      struct rte_eth_dev_info *dev_info);  static const uint32_t 
>> *eth_igb_supported_ptypes_get(struct rte_eth_dev *dev); @@ -389,6 
>> +391,7 @@ static const struct eth_dev_ops eth_igb_ops = {
>>  	.xstats_get_names     = eth_igb_xstats_get_names,
>>  	.stats_reset          = eth_igb_stats_reset,
>>  	.xstats_reset         = eth_igb_xstats_reset,
>> +	.fw_version_get       = eth_igb_fw_version_get,
>>  	.dev_infos_get        = eth_igb_infos_get,
>>  	.dev_supported_ptypes_get = eth_igb_supported_ptypes_get,
>>  	.mtu_set              = eth_igb_mtu_set,
>> @@ -1981,6 +1984,46 @@ eth_igbvf_stats_reset(struct rte_eth_dev *dev)  
>> }
>>  
> 
> <...>
>