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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 13 Apr 2023 14:04:33 +0000
From:   "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To:     Leon Romanovsky <leon@...nel.org>
CC:     "jiri@...nulli.us" <jiri@...nulli.us>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "corbet@....net" <corbet@....net>,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "richardcochran@...il.com" <richardcochran@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
Subject: RE: [RFC PATCH v1] ice: add CGU info to devlink info callback

>From: Leon Romanovsky <leon@...nel.org>
>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@...el.com>
>> ---
>>  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