netdev - RE: [PATCH net-next 10/15] ixgbe: extend .info

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]

Message-ID: <DS0PR11MB778531031053B86BC5301CBBF0B42@DS0PR11MB7785.namprd11.prod.outlook.com>
Date: Wed, 9 Apr 2025 10:50:34 +0000
From: "Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>
CC: "davem@...emloft.net" <davem@...emloft.net>, "pabeni@...hat.com"
	<pabeni@...hat.com>, "Dumazet, Eric" <edumazet@...gle.com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, "jiri@...nulli.us" <jiri@...nulli.us>,
	"horms@...nel.org" <horms@...nel.org>, "corbet@....net" <corbet@....net>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>, "Polchlopek,
 Mateusz" <mateusz.polchlopek@...el.com>, "R, Bharath" <bharath.r@...el.com>,
	"Mrozowicz, SlawomirX" <slawomirx.mrozowicz@...el.com>, "Kwapulinski, Piotr"
	<piotr.kwapulinski@...el.com>
Subject: RE: [PATCH net-next 10/15] ixgbe: extend .info_get with() stored
 versions

From: Jakub Kicinski <kuba@...nel.org> 
Sent: Wednesday, April 9, 2025 4:31 AM

>On Mon,  7 Apr 2025 14:51:14 -0700 Tony Nguyen wrote:
>> From: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
>> 
>> Add functions reading inactive versions from the inactive flash
>> bank.
>
>Just to be crystal clear -- could you share the outputs for dev info:
> - before update
> - after update, before reboot/reload
> - after update, after activation (/reboot/reload)
>?

OK so this looks this way:

[osdadmin@...delivery ~]$ devlink dev info pci/0000:63:00.0
pci/0000:63:00.0:
  driver ixgbe
  serial_number 00-00-00-ff-ff-00-01-00
  versions:
      fixed:
        board.id N43235-000
      running:
        fw.undi 0.12552.1
        fw.bundle_id 0x8000c459
        fw.mgmt.api 1.7.11
        fw.mgmt.build 0x3a458c89
        fw.mgmt.srev 1
        fw.undi.srev 1
        fw.psid.api 1.00
        fw.netlist 0.0.0-0.22.0
        fw.netlist.build 0xe1aaca1d
[osdadmin@...delivery ~]$
[root@...delivery lanconf]# devlink dev flash pci/0000:63:00.0 file nvm/file.bin
Preparing to flash

[...]

[root@...delivery lanconf]# devlink dev info pci/0000:63:00.0
pci/0000:63:00.0:
  driver ixgbe
  serial_number 00-00-00-ff-ff-00-01-00
  versions:
      fixed:
        board.id N43235-000
      running:
        fw.undi 0.12552.1
        fw.bundle_id 0x8000c459
        fw.mgmt.api 1.7.11
        fw.mgmt.build 0x3a458c89
        fw.mgmt.srev 1
        fw.undi.srev 1
        fw.psid.api 1.00
        fw.netlist 0.0.0-0.22.0
        fw.netlist.build 0xe1aaca1d
      stored:
        fw.mgmt.srev 1
        fw.bundle_id 0x8000d993
        fw.psid.api 1.01
        fw.undi 0.12552.1
        fw.undi.srev 1
[root@...delivery lanconf]#
[root@...delivery lanconf]# devlink dev reload pci/0000:63:00.0 action fw_activate
reload_actions_performed:
    fw_activate
[root@...delivery lanconf]# devlink dev info pci/0000:63:00.0
pci/0000:63:00.0:
  driver ixgbe
  serial_number 00-00-00-ff-ff-00-01-00
  versions:
      fixed:
        board.id N43235-000
      running:
        fw.undi 0.12552.1
        fw.bundle_id 0x8000d993
        fw.mgmt.api 1.7.11
        fw.mgmt.build 0xbc86d939
        fw.mgmt.srev 1
        fw.undi.srev 1
        fw.psid.api 1.01
        fw.netlist 0.0.0-0.22.0
        fw.netlist.build 0xe1aaca1d
[root@...delivery lanconf]#

>
>AFAICT the code is fine but talking about the "inactive versions"
>is not exactly in line with the uAPI expectations. 
>
>> +static int ixgbe_set_ctx_dev_caps(struct ixgbe_hw *hw,
>> +				  struct ixgbe_info_ctx *ctx,
>> +				  struct netlink_ext_ack *extack)
>> +{
>> +	int err = ixgbe_discover_dev_caps(hw, &ctx->dev_caps);
>> +
>> +	if (err) {
>
>Don't call functions which need error checking as part of variable init.
>
>> +		NL_SET_ERR_MSG_MOD(extack,
>> +				   "Unable to discover device capabilities");
>> +		return err;
>> +	}
>> +
>> +	if (ctx->dev_caps.common_cap.nvm_update_pending_orom) {
>> +		err = ixgbe_get_inactive_orom_ver(hw, &ctx->pending_orom);
>> +		if (err)
>> +			ctx->dev_caps.common_cap.nvm_update_pending_orom =
>> +				false;
>
>This function would benefit from having ctx->dev_caps.common_cap
>stored to a local variable with a shorter name :S

yeah, agree

>
>> +	}
>> +
>> +	if (ctx->dev_caps.common_cap.nvm_update_pending_nvm) {
>> +		err = ixgbe_get_inactive_nvm_ver(hw, &ctx->pending_nvm);
>> +		if (err)
>> +			ctx->dev_caps.common_cap.nvm_update_pending_nvm = false;
>> +	}
>> +
>> +	if (ctx->dev_caps.common_cap.nvm_update_pending_netlist) {
>> +		err = ixgbe_get_inactive_netlist_ver(hw, &ctx->pending_netlist);
>> +		if (err)
>> +			ctx->dev_caps.common_cap.nvm_update_pending_netlist =
>> +				false;
>> +	}
>-- 
>pw-bot: cr