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

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 11/28/2022 1:47 PM, Paul M Stillwell Jr wrote:
> Customers 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   |  82 ++++++++++++
>   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    | 117 ++++++++++++++++++
>   drivers/net/ethernet/intel/ice/ice_fwlog.h    |  51 ++++++++
>   drivers/net/ethernet/intel/ice/ice_type.h     |   4 +
>   7 files changed, 330 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
> 
> diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
> index 060d8f2b4953..750fed7e07d7 100644
> --- a/drivers/net/ethernet/intel/ice/Makefile
> +++ b/drivers/net/ethernet/intel/ice/Makefile
> @@ -32,7 +32,8 @@ ice-y := ice_main.o	\
>   	 ice_lag.o	\
>   	 ice_ethtool.o  \
>   	 ice_repr.o	\
> -	 ice_tc_lib.o
> +	 ice_tc_lib.o	\
> +	 ice_fwlog.o
>   ice-$(CONFIG_PCI_IOV) +=	\
>   	ice_sriov.o		\
>   	ice_virtchnl.o		\
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index 5cac5a73c66b..d0026161a2b4 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -2065,6 +2065,86 @@ struct ice_aqc_event_lan_overflow {
>   	u8 reserved[8];
>   };
>   
> +enum ice_aqc_fw_logging_mod {
> +	ICE_AQC_FW_LOG_ID_GENERAL = 0,
> +	ICE_AQC_FW_LOG_ID_CTRL,
> +	ICE_AQC_FW_LOG_ID_LINK,
> +	ICE_AQC_FW_LOG_ID_LINK_TOPO,
> +	ICE_AQC_FW_LOG_ID_DNL,
> +	ICE_AQC_FW_LOG_ID_I2C,
> +	ICE_AQC_FW_LOG_ID_SDP,
> +	ICE_AQC_FW_LOG_ID_MDIO,
> +	ICE_AQC_FW_LOG_ID_ADMINQ,
> +	ICE_AQC_FW_LOG_ID_HDMA,
> +	ICE_AQC_FW_LOG_ID_LLDP,
> +	ICE_AQC_FW_LOG_ID_DCBX,
> +	ICE_AQC_FW_LOG_ID_DCB,
> +	ICE_AQC_FW_LOG_ID_XLR,
> +	ICE_AQC_FW_LOG_ID_NVM,
> +	ICE_AQC_FW_LOG_ID_AUTH,
> +	ICE_AQC_FW_LOG_ID_VPD,
> +	ICE_AQC_FW_LOG_ID_IOSF,
> +	ICE_AQC_FW_LOG_ID_PARSER,
> +	ICE_AQC_FW_LOG_ID_SW,
> +	ICE_AQC_FW_LOG_ID_SCHEDULER,
> +	ICE_AQC_FW_LOG_ID_TXQ,
> +	ICE_AQC_FW_LOG_ID_RSVD,
> +	ICE_AQC_FW_LOG_ID_POST,
> +	ICE_AQC_FW_LOG_ID_WATCHDOG,
> +	ICE_AQC_FW_LOG_ID_TASK_DISPATCH,
> +	ICE_AQC_FW_LOG_ID_MNG,
> +	ICE_AQC_FW_LOG_ID_SYNCE,
> +	ICE_AQC_FW_LOG_ID_HEALTH,
> +	ICE_AQC_FW_LOG_ID_TSDRV,
> +	ICE_AQC_FW_LOG_ID_PFREG,
> +	ICE_AQC_FW_LOG_ID_MDLVER,
> +	ICE_AQC_FW_LOG_ID_MAX,
> +};
> +
> +/* Set FW Logging configuration (indirect 0xFF30)
> + * Register for FW Logging (indirect 0xFF31)
> + * Query FW Logging (indirect 0xFF32)
> + * FW Log Event (indirect 0xFF33)
> + * Get FW Log (indirect 0xFF34)
> + * Clear FW Log (indirect 0xFF35)

I don't think I saw the 0xFF35 op added anywhere. Also, the 0xFF34 looks 
unused (more on that later in the series)

> + */
> +struct ice_aqc_fw_log {
> +	u8 cmd_flags;
> +#define ICE_AQC_FW_LOG_CONF_UART_EN	BIT(0)
> +#define ICE_AQC_FW_LOG_CONF_AQ_EN	BIT(1)
> +#define ICE_AQC_FW_LOG_QUERY_REGISTERED	BIT(2)
> +#define ICE_AQC_FW_LOG_CONF_SET_VALID	BIT(3)
> +#define ICE_AQC_FW_LOG_AQ_REGISTER	BIT(0)
> +#define ICE_AQC_FW_LOG_AQ_QUERY		BIT(2)
> +#define ICE_AQC_FW_LOG_PERSISTENT	BIT(0)

Please add a newline to separate the defines and the member they are 
associated with.

Also, I don't believe I'm seeing there persistent define being used...

> +	u8 rsp_flag;
> +#define ICE_AQC_FW_LOG_MORE_DATA	BIT(1)

or this one.

> +	__le16 fw_rt_msb;
> +	union {
> +		struct {
> +			__le32 fw_rt_lsb;
> +		} sync;

I don't think I saw sync used either.

> +		struct {
> +			__le16 log_resolution;
> +#define ICE_AQC_FW_LOG_MIN_RESOLUTION		(1)
> +#define ICE_AQC_FW_LOG_MAX_RESOLUTION		(128)
> +			__le16 mdl_cnt;
> +		} cfg;
> +	} ops;
> +	__le32 addr_high;
> +	__le32 addr_low;
> +};
> +
> +/* Response Buffer for:
> + *    Set Firmware Logging Configuration (0xFF30)
> + *    Query FW Logging (0xFF32)
> + */
> +struct ice_aqc_fw_log_cfg_resp {
> +	__le16 module_identifier;
> +	u8 log_level;
> +	u8 rsvd0;
> +};
> +
>   /**
>    * struct ice_aq_desc - Admin Queue (AQ) descriptor
>    * @flags: ICE_AQ_FLAG_* flags
> @@ -2141,6 +2221,7 @@ struct ice_aq_desc {
>   		struct ice_aqc_add_update_free_vsi_resp add_update_free_vsi_res;
>   		struct ice_aqc_download_pkg download_pkg;
>   		struct ice_aqc_driver_shared_params drv_shared_params;
> +		struct ice_aqc_fw_log fw_log;
>   		struct ice_aqc_set_mac_lb set_mac_lb;
>   		struct ice_aqc_alloc_free_res_cmd sw_res_ctrl;
>   		struct ice_aqc_set_mac_cfg set_mac_cfg;
> @@ -2325,6 +2406,7 @@ enum ice_adminq_opc {
>   
>   	/* Standalone Commands/Events */
>   	ice_aqc_opc_event_lan_overflow			= 0x1001,
> +	ice_aqc_opc_fw_logs_query			= 0xFF32,
>   };
>   
>   #endif /* _ICE_ADMINQ_CMD_H_ */
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index b781655ee805..ecdc1ebb45d5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -879,6 +879,8 @@ int ice_init_hw(struct ice_hw *hw)
>   	if (status)
>   		goto err_unroll_cqinit;
>   
> +	ice_fwlog_set_support_ena(hw);
> +
>   	status = ice_clear_pf_cfg(hw);
>   	if (status)
>   		goto err_unroll_cqinit;
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index 5c030e89ac18..8843ff492f7f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -1484,6 +1484,56 @@ ice_devlink_enable_iw_validate(struct devlink *devlink, u32 id,
>   	return 0;
>   }
>   
> +enum ice_devlink_param_id {
> +	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED,
> +};
> +
> +static int
> +ice_devlink_fwlog_supported_get(struct devlink *devlink, u32 id,
> +				struct devlink_param_gset_ctx *ctx)
> +{
> +	struct ice_pf *pf = devlink_priv(devlink);
> +
> +	/* only support fw log commands on PF 0 */
> +	if (pf->hw.bus.func)
> +		return -EOPNOTSUPP;
> +
> +	ctx->val.vbool = ice_fwlog_supported(&pf->hw);
> +	return 0;
> +}
> +
> +static int
> +ice_devlink_fwlog_supported_set(struct devlink *devlink, u32 id,
> +				struct devlink_param_gset_ctx *ctx)
> +{
> +	/* set operation is unsupported */
> +	return -EOPNOTSUPP;
> +}
> +
> +static int
> +ice_devlink_fwlog_enabled_get(struct devlink *devlink, u32 id,
> +			      struct devlink_param_gset_ctx *ctx)
> +{
> +	struct ice_pf *pf = devlink_priv(devlink);
> +
> +	/* only support fw log commands on PF 0 */
> +	if (pf->hw.bus.func)
> +		return -EOPNOTSUPP;
> +
> +	ctx->val.vbool = pf->hw.fwlog_ena;
> +	return 0;
> +}
> +
> +static int
> +ice_devlink_fwlog_enabled_set(struct devlink *devlink, u32 id,
> +			      struct devlink_param_gset_ctx *ctx)
> +{
> +	/* set operation is unsupported at this time */
> +	return -EOPNOTSUPP;
> +}
> +
>   static const struct devlink_param ice_devlink_params[] = {
>   	DEVLINK_PARAM_GENERIC(ENABLE_ROCE, BIT(DEVLINK_PARAM_CMODE_RUNTIME),
>   			      ice_devlink_enable_roce_get,
> @@ -1500,7 +1550,18 @@ static const struct devlink_param ice_devlink_params[] = {
>   			     ice_devlink_txbalance_get,
>   			     ice_devlink_txbalance_set,
>   			     ice_devlink_txbalance_validate),
> -
> +	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED,
> +			     "fwlog_supported", DEVLINK_PARAM_TYPE_BOOL,
> +			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
> +			     ice_devlink_fwlog_supported_get,
> +			     ice_devlink_fwlog_supported_set,
> +			     NULL),
> +	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED,
> +			     "fwlog_enabled", DEVLINK_PARAM_TYPE_BOOL,
> +			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
> +			     ice_devlink_fwlog_enabled_get,
> +			     ice_devlink_fwlog_enabled_set,
> +			     NULL),
>   };
>   
>   static void ice_devlink_free(void *devlink_ptr)
> @@ -1596,6 +1657,16 @@ int ice_devlink_register_params(struct ice_pf *pf)
>   					   DEVLINK_PARAM_GENERIC_ID_ENABLE_ROCE,
>   					   value);
>   
> +	value.vbool = false;
> +	devlink_param_driverinit_value_set(devlink,
> +					   ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED,
> +					   value);
> +
> +	value.vbool = false;
> +	devlink_param_driverinit_value_set(devlink,
> +					   ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED,
> +					   value);
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.c b/drivers/net/ethernet/intel/ice/ice_fwlog.c
> new file mode 100644
> index 000000000000..67e670c62091
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ice/ice_fwlog.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2018, Intel Corporation. */
> +
> +#include "ice_common.h"
> +#include "ice_fwlog.h"
> +
> +/**
> + * ice_fwlog_supported - Cached for whether FW supports FW logging or not
> + * @hw: pointer to the HW structure
> + *
> + * This will always return false if called before ice_init_hw(), so it must be
> + * called after ice_init_hw().
> + */
> +bool ice_fwlog_supported(struct ice_hw *hw)
> +{
> +	return hw->fwlog_support_ena;
> +}
> +
> +/**
> + * 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 i, module_id_cnt;
> +	int status;
> +	void *buf;
> +
> +	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;
> +	}
> +
> +	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_support_ena - 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_support_ena 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_support_ena(struct ice_hw *hw)
> +{
> +	struct ice_fwlog_cfg *cfg;
> +	int status;
> +
> +	hw->fwlog_support_ena = 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_support_ena = 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..d7371253b8e5
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ice/ice_fwlog.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2018-2021, 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];
> +#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)
> +	/* options used to configure firmware logging */
> +	u16 options;

I believe the other structs put the define below the member, this seems 
to put it above.

> +	/* minimum number of log events sent per Admin Receive Queue event */
> +	u16 log_resolution;
> +};
> +
> +void ice_fwlog_set_support_ena(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 dd064b297d77..10b78faf0a32 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"

newline here please

> +#include "ice_fwlog.h"
>   
>   static inline bool ice_is_tc_ena(unsigned long bitmap, u8 tc)
>   {
> @@ -860,6 +861,9 @@ struct ice_hw {
>   	u8 fw_patch;		/* firmware patch version */
>   	u32 fw_build;		/* firmware build number */
>   
> +	bool fwlog_support_ena; /* does hardware support FW logging? */

IMO this name is a bit confusing as the ena makes me think it represents 
enabled not whether hardware supports it. Why not just 'supported' (and 
the other associated *support_ena fields and functions)?

> +	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.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan