Tue, Feb 11, 2025 at 01:12:12PM +0100, [email protected] wrote:
>From: Jiri Pirko <[email protected]>
>Sent: Monday, February 10, 2025 5:26 PM
>>Mon, Feb 10, 2025 at 02:56:28PM +0100, [email protected] 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
>>>
>>>
