Re: [Intel-wired-lan] [PATCH net-next v5 2/5] ice: enable devlink to check FW logging status

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Wed, Jan 11, 2023 at 11:19:03AM -0800, Paul M Stillwell Jr wrote:
> Users want the ability to debug FW issues by retrieving the
> FW logs from the E8xx devices. Enable devlink to query the driver
> to see if the NVM image allows FW logging and to see if FW
> logging is currently running. The set command is not supported
> at this time.
> 
> This is the beginning of the v2 for FW logging.
> 
> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> 
>     pick bfdfb2dc6192 ice: add ability to query/set FW log level and resolution
> ---
>  drivers/net/ethernet/intel/ice/Makefile       |   3 +-
>  .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  81 ++++++++++++
>  drivers/net/ethernet/intel/ice/ice_common.c   |   2 +
>  drivers/net/ethernet/intel/ice/ice_devlink.c  |  73 ++++++++++-
>  drivers/net/ethernet/intel/ice/ice_fwlog.c    | 118 ++++++++++++++++++
>  drivers/net/ethernet/intel/ice/ice_fwlog.h    |  52 ++++++++
>  drivers/net/ethernet/intel/ice/ice_type.h     |   4 +
>  7 files changed, 331 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/net/ethernet/intel/ice/ice_fwlog.c
>  create mode 100644 drivers/net/ethernet/intel/ice/ice_fwlog.h
> 
Hi,

Great changes, does it mean that after applying the patchset we will be
able to for example dump whole switch block?

Looks good, only cosmetic comments.

[...]

> +
> +/**
> + * ice_aq_fwlog_get - Get the current firmware logging configuration (0xFF32)
> + * @hw: pointer to the HW structure
> + * @cfg: firmware logging configuration to populate
> + */
> +static int
> +ice_aq_fwlog_get(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
> +{
> +	struct ice_aqc_fw_log_cfg_resp *fw_modules;
> +	struct ice_aqc_fw_log *cmd;
> +	struct ice_aq_desc desc;
> +	u16 module_id_cnt;
> +	int status;
> +	void *buf;
> +	int i;
> +
> +	memset(cfg, 0, sizeof(*cfg));
> +
> +	buf = kzalloc(ICE_AQ_MAX_BUF_LEN, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_fw_logs_query);
> +	cmd = &desc.params.fw_log;
> +
> +	cmd->cmd_flags = ICE_AQC_FW_LOG_AQ_QUERY;
> +
> +	status = ice_aq_send_cmd(hw, &desc, buf, ICE_AQ_MAX_BUF_LEN, NULL);
> +	if (status) {
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Failed to get FW log configuration\n");
> +		goto status_out;
> +	}
> +
> +	module_id_cnt = le16_to_cpu(cmd->ops.cfg.mdl_cnt);
> +	if (module_id_cnt < ICE_AQC_FW_LOG_ID_MAX) {
> +		ice_debug(hw, ICE_DBG_FW_LOG, "FW returned less than the expected number of FW log module IDs\n");
> +	} else {
> +		if (module_id_cnt > ICE_AQC_FW_LOG_ID_MAX)
> +			ice_debug(hw, ICE_DBG_FW_LOG, "FW returned more than expected number of FW log module IDs, setting module_id_cnt to software expected max %u\n",
> +				  ICE_AQC_FW_LOG_ID_MAX);
> +		module_id_cnt = ICE_AQC_FW_LOG_ID_MAX;
Maybe:
else if (module_id_cnt > ICE_AQC_FW_LOG_ID_MAX) {
	ice_debug();
	module_id_cnt = ICE_AQC_FW_LOG_ID_MAX;
}
We will save one indent, but it is cosmetic, so feel free to ignore.

> +	}
> +
> +	cfg->log_resolution = le16_to_cpu(cmd->ops.cfg.log_resolution);
> +	if (cmd->cmd_flags & ICE_AQC_FW_LOG_CONF_AQ_EN)
> +		cfg->options |= ICE_FWLOG_OPTION_ARQ_ENA;
> +	if (cmd->cmd_flags & ICE_AQC_FW_LOG_CONF_UART_EN)
> +		cfg->options |= ICE_FWLOG_OPTION_UART_ENA;
> +	if (cmd->cmd_flags & ICE_AQC_FW_LOG_QUERY_REGISTERED)
> +		cfg->options |= ICE_FWLOG_OPTION_IS_REGISTERED;
> +
> +	fw_modules = (struct ice_aqc_fw_log_cfg_resp *)buf;
> +
> +	for (i = 0; i < module_id_cnt; i++) {
> +		struct ice_aqc_fw_log_cfg_resp *fw_module = &fw_modules[i];
> +
> +		cfg->module_entries[i].module_id =
> +			le16_to_cpu(fw_module->module_identifier);
> +		cfg->module_entries[i].log_level = fw_module->log_level;
> +	}
> +
> +status_out:
> +	kfree(buf);
> +	return status;
> +}
> +
> +/**
> + * ice_fwlog_set_supported - Set if FW logging is supported by FW
> + * @hw: pointer to the HW struct
> + *
> + * If FW returns success to the ice_aq_fwlog_get call then it supports FW
> + * logging, else it doesn't. Set the fwlog_supported flag accordingly.
> + *
> + * This function is only meant to be called during driver init to determine if
> + * the FW support FW logging.
> + */
> +void ice_fwlog_set_supported(struct ice_hw *hw)
> +{
> +	struct ice_fwlog_cfg *cfg;
> +	int status;
> +
> +	hw->fwlog_supported = false;
> +
> +	cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
> +	if (!cfg)
> +		return;
> +
> +	/* don't call ice_fwlog_get() because that would overwrite the cached
> +	 * configuration from the call to ice_fwlog_init(), which is expected to
> +	 * be called prior to this function
> +	 */
> +	status = ice_aq_fwlog_get(hw, cfg);
> +	if (status)
> +		ice_debug(hw, ICE_DBG_FW_LOG, "ice_aq_fwlog_get failed, FW logging is not supported on this version of FW, status %d\n",
> +			  status);
> +	else
> +		hw->fwlog_supported = true;
> +
> +	kfree(cfg);
> +}
> diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.h b/drivers/net/ethernet/intel/ice/ice_fwlog.h
> new file mode 100644
> index 000000000000..3a2c83502763
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ice/ice_fwlog.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2022, Intel Corporation. */
> +
> +#ifndef _ICE_FWLOG_H_
> +#define _ICE_FWLOG_H_
> +#include "ice_adminq_cmd.h"
> +
> +struct ice_hw;
> +
> +/* Only a single log level should be set and all log levels under the set value
> + * are enabled, e.g. if log level is set to ICE_FW_LOG_LEVEL_VERBOSE, then all
> + * other log levels are included (except ICE_FW_LOG_LEVEL_NONE)
> + */
> +enum ice_fwlog_level {
> +	ICE_FWLOG_LEVEL_NONE = 0,
> +	ICE_FWLOG_LEVEL_ERROR = 1,
> +	ICE_FWLOG_LEVEL_WARNING = 2,
> +	ICE_FWLOG_LEVEL_NORMAL = 3,
> +	ICE_FWLOG_LEVEL_VERBOSE = 4,
> +	ICE_FWLOG_LEVEL_INVALID, /* all values >= this entry are invalid */
> +};
> +
> +struct ice_fwlog_module_entry {
> +	/* module ID for the corresponding firmware logging event */
> +	u16 module_id;
> +	/* verbosity level for the module_id */
> +	u8 log_level;
> +};
> +
> +struct ice_fwlog_cfg {
> +	/* list of modules for configuring log level */
> +	struct ice_fwlog_module_entry module_entries[ICE_AQC_FW_LOG_ID_MAX];
> +	/* options used to configure firmware logging */
> +	u16 options;
> +#define ICE_FWLOG_OPTION_ARQ_ENA		BIT(0)
> +#define ICE_FWLOG_OPTION_UART_ENA		BIT(1)
> +	/* set before calling ice_fwlog_init() so the PF registers for firmware
> +	 * logging on initialization
> +	 */
> +#define ICE_FWLOG_OPTION_REGISTER_ON_INIT	BIT(2)
> +	/* set in the ice_fwlog_get() response if the PF is registered for FW
> +	 * logging events over ARQ
> +	 */
> +#define ICE_FWLOG_OPTION_IS_REGISTERED		BIT(3)
> +
> +	/* minimum number of log events sent per Admin Receive Queue event */
> +	u16 log_resolution;
> +};
> +
> +void ice_fwlog_set_supported(struct ice_hw *hw);
> +bool ice_fwlog_supported(struct ice_hw *hw);
> +#endif /* _ICE_FWLOG_H_ */
> diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
> index 126605b7eb3b..1284fe8d78f2 100644
> --- a/drivers/net/ethernet/intel/ice/ice_type.h
> +++ b/drivers/net/ethernet/intel/ice/ice_type.h
> @@ -17,6 +17,7 @@
>  #include "ice_protocol_type.h"
>  #include "ice_sbq_cmd.h"
>  #include "ice_vlan_mode.h"
> +#include "ice_fwlog.h"
>  
>  static inline bool ice_is_tc_ena(unsigned long bitmap, u8 tc)
>  {
> @@ -859,6 +860,9 @@ struct ice_hw {
>  	u8 fw_patch;		/* firmware patch version */
>  	u32 fw_build;		/* firmware build number */
>  
> +	bool fwlog_supported; /* does hardware support FW logging? */
> +	bool fwlog_ena; /* currently logging? */
> +
>  /* Device max aggregate bandwidths corresponding to the GL_PWR_MODE_CTL
>   * register. Used for determining the ITR/INTRL granularity during
>   * initialization.
> -- 
> 2.35.1
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan