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: <qmjitflm2k3zo5yiym74c6okjg5skzhb46evfhn6qpzkwch3uc@epvkzeg37n3f>
Date: Tue, 11 Feb 2025 13:52:07 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: "Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>
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()

Tue, Feb 11, 2025 at 01:12:12PM +0100, jedrzej.jagielski@...el.com wrote:
>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.

That sounds fine to me. Someone else may find this handy as well.


>
>>
>>
>>
>>>+{
>>>+	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
>>>
>>>