RE: [RFC PATCH v1] ice: add CGU info to devlink info callback

[Kubalewski, Arkadiusz]

>From: Leon Romanovsky <leon@xxxxxxxxxx>
>Sent: Thursday, April 13, 2023 3:17 PM
>
>On Wed, Apr 12, 2023 at 03:38:11PM +0200, Arkadiusz Kubalewski wrote:
>> If Clock Generation Unit and dplls are present on NIC board user shall
>> know its details.
>> Provide the devlink info callback with a new:
>> - fixed type object `cgu.id` - hardware variant of onboard CGU
>> - running type object `fw.cgu` - CGU firmware version
>> - running type object `fw.cgu.build` - CGU configuration build version
>>
>> These information shall be known for debugging purposes.
>>
>> Test (on NIC board with CGU)
>> $ devlink dev info <bus_name>/<dev_name> | grep cgu
>>         cgu.id 8032
>>         fw.cgu 6021
>>         fw.cgu.build 0x1030001
>>
>> Test (on NIC board without CGU)
>> $ devlink dev info <bus_name>/<dev_name> | grep cgu -c
>> 0
>>
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>
>> ---
>>  Documentation/networking/devlink/ice.rst     | 14 +++++++++
>>  drivers/net/ethernet/intel/ice/ice_devlink.c | 30 ++++++++++++++++++++
>>  drivers/net/ethernet/intel/ice/ice_main.c    |  5 +++-
>>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c  | 12 ++++----
>>  drivers/net/ethernet/intel/ice/ice_type.h    |  9 +++++-
>>  5 files changed, 62 insertions(+), 8 deletions(-)
>
><...>
>
>>  Flash Update
>>  ============
>> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c
>b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> index bc44cc220818..06fe895739af 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> @@ -193,6 +193,33 @@ ice_info_pending_netlist_build(struct ice_pf
>>__always_unused *pf,
>>  		snprintf(ctx->buf, sizeof(ctx->buf), "0x%08x", netlist->hash);
>>  }
>>
>> +static void ice_info_cgu_id(struct ice_pf *pf, struct ice_info_ctx *ctx)
>> +{
>> +	if (ice_is_feature_supported(pf, ICE_F_CGU)) {
>> +		struct ice_hw *hw = &pf->hw;
>> +
>> +		snprintf(ctx->buf, sizeof(ctx->buf), "%u", hw->cgu.id);
>> +	}
>
>Please use kernel coding style - success oriented flow
>
>struct ice_hw *hw = &pf->hw;
>
>if (!ice_is_feature_supported(pf, ICE_F_CGU))
>  return;
>
>snprintf(ctx->buf, sizeof(ctx->buf), "%u", hw->cgu.id);
>
>
>However, it will be nice to have these callbacks only if CGU is
>supported, in such way you won't need any of ice_is_feature_supported()
>checks.
>
>Thanks

Sure, I will fix as suggested in the next version.
Although most important is to achieve common understanding and agreement if
This way is the right one. Maybe those devlink id's shall be defined as a
part of "include/net/devlink.h", so other vendors could use it?
Also in such case probably naming might need to be unified.

Thank you!
Arkadiusz