netdev - RE: [PATCH iwl-next v2 02/13] ixgbe: add handler for devlink .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: <DS0PR11MB7785B1EF702ED5536D4B4CCBF0FD2@DS0PR11MB7785.namprd11.prod.outlook.com>
Date: Tue, 11 Feb 2025 12:12:12 +0000
From: "Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "horms@...nel.org" <horms@...nel.org>, "Polchlopek,
 Mateusz" <mateusz.polchlopek@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>
Subject: RE: [PATCH iwl-next v2 02/13] ixgbe: add handler for devlink
 .info_get()

From: Jiri Pirko <jiri@...nulli.us> 
Sent: Monday, February 10, 2025 5:26 PM
>Mon, Feb 10, 2025 at 02:56:28PM +0100, jedrzej.jagielski@...el.com wrote:
>
>[...]
>
>>+enum ixgbe_devlink_version_type {
>>+	IXGBE_DL_VERSION_FIXED,
>>+	IXGBE_DL_VERSION_RUNNING,
>>+};
>>+
>>+static int ixgbe_devlink_info_put(struct devlink_info_req *req,
>>+				  enum ixgbe_devlink_version_type type,
>>+				  const char *key, const char *value)
>
>I may be missing something, but what's the benefit of having this helper
>instead of calling directly devlink_info_version_*_put()?

ixgbe devlink .info_get() supports various adapters across ixgbe portfolio which
have various sets of version types - some version types are not applicable
for some of the adapters - so we want just to check if it's *not empty.*

If so then we don't want to create such entry at all so avoid calling
devlink_info_version_*_put() in this case.
Putting value check prior each calling of devlink_info_version_*_put()
would provide quite a code redundancy and would look not so good imho.

Me and Przemek are not fully convinced by adding such additional
layer of abstraction but we defineltly need this value check to not
print empty type or get error and return from the function.

Another solution would be to add such check to devlink function.

>
>
>
>>+{
>>+	if (!*value)
>>+		return 0;
>>+
>>+	switch (type) {
>>+	case IXGBE_DL_VERSION_FIXED:
>>+		return devlink_info_version_fixed_put(req, key, value);
>>+	case IXGBE_DL_VERSION_RUNNING:
>>+		return devlink_info_version_running_put(req, key, value);
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>
>[...]
>
>
>>+static int ixgbe_devlink_info_get(struct devlink *devlink,
>>+				  struct devlink_info_req *req,
>>+				  struct netlink_ext_ack *extack)
>>+{
>>+	struct ixgbe_devlink_priv *devlink_private = devlink_priv(devlink);
>>+	struct ixgbe_adapter *adapter = devlink_private->adapter;
>>+	struct ixgbe_hw *hw = &adapter->hw;
>>+	struct ixgbe_info_ctx *ctx;
>>+	int err;
>>+
>>+	ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
>>+	if (!ctx)
>>+		return -ENOMEM;
>>+
>>+	ixgbe_info_get_dsn(adapter, ctx);
>>+	err = devlink_info_serial_number_put(req, ctx->buf);
>>+	if (err)
>>+		goto free_ctx;
>>+
>>+	ixgbe_info_nvm_ver(adapter, ctx);
>>+	err = ixgbe_devlink_info_put(req, IXGBE_DL_VERSION_RUNNING,
>>+				     DEVLINK_INFO_VERSION_GENERIC_FW_UNDI,
>>+				     ctx->buf);
>>+	if (err)
>>+		goto free_ctx;
>>+
>>+	ixgbe_info_eetrack(adapter, ctx);
>>+	err = ixgbe_devlink_info_put(req, IXGBE_DL_VERSION_RUNNING,
>>+				     DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID,
>>+				     ctx->buf);
>>+	if (err)
>>+		goto free_ctx;
>>+
>>+	err = ixgbe_read_pba_string_generic(hw, ctx->buf, sizeof(ctx->buf));
>>+	if (err)
>>+		goto free_ctx;
>>+
>>+	err = ixgbe_devlink_info_put(req, IXGBE_DL_VERSION_FIXED,
>>+				     DEVLINK_INFO_VERSION_GENERIC_BOARD_ID,
>>+				     ctx->buf);
>>+free_ctx:
>>+	kfree(ctx);
>>+	return err;
>>+}
>>+
>> static const struct devlink_ops ixgbe_devlink_ops = {
>>+	.info_get = ixgbe_devlink_info_get,
>> };
>> 
>> /**
>>-- 
>>2.31.1
>>
>>