Re: [PATCH iwl-next v1 02/13] ixgbe: add handler for devlink .info_get()

[Simon Horman <horms@kernel.org>]

On Mon, Feb 03, 2025 at 04:03:17PM +0100, Jedrzej Jagielski wrote:
> Provide devlink .info_get() callback implementation to allow the
> driver to report detailed version information. The following info
> is reported:
> 
>  "serial_number" -> The PCI DSN of the adapter
>  "fw.bundle_id" -> Unique identifier for the combined flash image
>  "fw.undi" -> Version of the Option ROM containing the UEFI driver
>  "board.id" -> The PBA ID string
> 
> Reviewed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>

...

> diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c

...

> +static void ixgbe_info_nvm_ver(struct ixgbe_adapter *adapter,
> +			       struct ixgbe_info_ctx *ctx)
> +{
> +	struct ixgbe_hw *hw = &adapter->hw;
> +	struct ixgbe_nvm_version nvm_ver;
> +
> +	ixgbe_get_oem_prod_version(hw, &nvm_ver);
> +	if (nvm_ver.oem_valid) {
> +		snprintf(ctx->buf, sizeof(ctx->buf), "%x.%x.%x",
> +			 nvm_ver.oem_major, nvm_ver.oem_minor,
> +			 nvm_ver.oem_release);
> +
> +		return;
> +	}
> +
> +	ixgbe_get_orom_version(hw, &nvm_ver);
> +	if (nvm_ver.or_valid)
> +		snprintf(ctx->buf, sizeof(ctx->buf), "%d.%d.%d",
> +			 nvm_ver.or_major, nvm_ver.or_build, nvm_ver.or_patch);

Hi Jedrzej,

If neither of the conditions above are met then it seems that ctx->buf will
contain whatever string was present when the function was called. Is
something like the following needed here?

	ctx->buf[0] = '\0';

> +}
> +
> +static void ixgbe_info_eetrack(struct ixgbe_adapter *adapter,
> +			       struct ixgbe_info_ctx *ctx)
> +{
> +	struct ixgbe_hw *hw = &adapter->hw;
> +	struct ixgbe_nvm_version nvm_ver;
> +
> +	ixgbe_get_oem_prod_version(hw, &nvm_ver);
> +	/* No ETRACK version for OEM */
> +	if (nvm_ver.oem_valid)
> +		return;

Likewise, here.

> +
> +	ixgbe_get_etk_id(hw, &nvm_ver);
> +	snprintf(ctx->buf, sizeof(ctx->buf), "0x%08x", nvm_ver.etk_id);
> +}
> +
> +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;
> +}

...