[Intel-wired-lan] [PATCH net-next 0/5] add v2 FW logging for ice driver

All of lore.kernel.org
 help / color / mirror / Atom feed

* [Intel-wired-lan] [PATCH net-next 0/5] add v2 FW logging for ice driver
@ 2022-11-28 21:47 Paul M Stillwell Jr
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 1/5] ice: remove FW logging code Paul M Stillwell Jr
                   ` (5 more replies)
  0 siblings, 6 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2022-11-28 21:47 UTC (permalink / raw)
  To: intel-wired-lan

FW log support was added to the ice driver, but that version is no
longer supported. There is a newer version of FW logging (v2) that
adds more controls knobs to get the exact data out of the FW
for debugging.

Additionally, instead of dumping the FW log output to syslog,
dump it to debugfs. The FW log data is really just binary
data that the FW log team decodes to determine what happens so the
translation from binary to some text output just slows things down
and results in potential dropped data. The structure for the debugfs
entry is: /sys/kernel/debug/ice/<pci device>/fwlog

Once enabled the FW log data is received as ARQ events that the driver
processes.

The FW logging is across all the PFs on the device, so restrict the
commands to only PF0.

The following new device parameters are added:
- fwlog_supported (read-only): does the FW support FW logging
- fwlog_enabled (read/write): is FW logging currently running
- fwlog_level (read/write): the log level enabled, valid values are
    Each level includes the messages from the previous/lower level
	0 - no logging
	1 - error logging
	2 - warning logging
	3 - normal logging
	4 - verbose logging
- fwlog_resolution (read/write): the number of log messages to included
  in a single ARQ event. The range is 1-128 (1 means push every log
  message, 128 means push only when the max AQ command buffer is full).
  The suggested value is 10.

This patch series adds the following set of devlink commands:

devlink dev param set <pci dev> name fwlog_enabled value <true/false> cmode runtime
devlink dev param set <pci dev> name fwlog_level value <0-4> cmode runtime
devlink dev param set <pci dev> name fwlog_resolution value <1-128> cmode runtime

Paul M Stillwell Jr (5):
  ice: remove FW logging code
  ice: enable devlink to check FW logging status
  ice: add ability to query/set FW log level and resolution
  ice: disable FW logging on driver unload
  ice: use debugfs to output FW log data

 drivers/net/ethernet/intel/ice/Makefile       |   4 +-
 drivers/net/ethernet/intel/ice/ice.h          |  24 ++
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   | 164 ++++----
 drivers/net/ethernet/intel/ice/ice_common.c   | 218 +---------
 drivers/net/ethernet/intel/ice/ice_debugfs.c  | 109 +++++
 drivers/net/ethernet/intel/ice/ice_devlink.c  | 205 ++++++++-
 drivers/net/ethernet/intel/ice/ice_fwlog.c    | 394 ++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_fwlog.h    |  56 +++
 drivers/net/ethernet/intel/ice/ice_main.c     |  99 ++++-
 drivers/net/ethernet/intel/ice/ice_type.h     |  23 +-
 10 files changed, 980 insertions(+), 316 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/ice/ice_debugfs.c
 create mode 100644 drivers/net/ethernet/intel/ice/ice_fwlog.c
 create mode 100644 drivers/net/ethernet/intel/ice/ice_fwlog.h

-- 
2.35.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Intel-wired-lan] [PATCH net-next 1/5] ice: remove FW logging code
  2022-11-28 21:47 [Intel-wired-lan] [PATCH net-next 0/5] add v2 FW logging for ice driver Paul M Stillwell Jr
@ 2022-11-28 21:47 ` Paul M Stillwell Jr
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 2/5] ice: enable devlink to check FW logging status Paul M Stillwell Jr
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2022-11-28 21:47 UTC (permalink / raw)
  To: intel-wired-lan

The FW logging code doesn't work because there is no way to set
cq_ena or uart_ena so remove the code. This code is the original
(v1) way of FW logging so it should be replaced with the v2 way.

Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
---
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  78 -------
 drivers/net/ethernet/intel/ice/ice_common.c   | 218 ------------------
 drivers/net/ethernet/intel/ice/ice_main.c     |   3 -
 drivers/net/ethernet/intel/ice/ice_type.h     |  20 --
 4 files changed, 319 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 54c87ced4fad..5cac5a73c66b 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1980,78 +1980,6 @@ struct ice_aqc_add_rdma_qset_data {
 	struct ice_aqc_add_tx_rdma_qset_entry rdma_qsets[];
 };
 
-/* Configure Firmware Logging Command (indirect 0xFF09)
- * Logging Information Read Response (indirect 0xFF10)
- * Note: The 0xFF10 command has no input parameters.
- */
-struct ice_aqc_fw_logging {
-	u8 log_ctrl;
-#define ICE_AQC_FW_LOG_AQ_EN		BIT(0)
-#define ICE_AQC_FW_LOG_UART_EN		BIT(1)
-	u8 rsvd0;
-	u8 log_ctrl_valid; /* Not used by 0xFF10 Response */
-#define ICE_AQC_FW_LOG_AQ_VALID		BIT(0)
-#define ICE_AQC_FW_LOG_UART_VALID	BIT(1)
-	u8 rsvd1[5];
-	__le32 addr_high;
-	__le32 addr_low;
-};
-
-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_NETPROXY,
-	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_MAX,
-};
-
-/* Defines for both above FW logging command/response buffers */
-#define ICE_AQC_FW_LOG_ID_S		0
-#define ICE_AQC_FW_LOG_ID_M		(0xFFF << ICE_AQC_FW_LOG_ID_S)
-
-#define ICE_AQC_FW_LOG_CONF_SUCCESS	0	/* Used by response */
-#define ICE_AQC_FW_LOG_CONF_BAD_INDX	BIT(12)	/* Used by response */
-
-#define ICE_AQC_FW_LOG_EN_S		12
-#define ICE_AQC_FW_LOG_EN_M		(0xF << ICE_AQC_FW_LOG_EN_S)
-#define ICE_AQC_FW_LOG_INFO_EN		BIT(12)	/* Used by command */
-#define ICE_AQC_FW_LOG_INIT_EN		BIT(13)	/* Used by command */
-#define ICE_AQC_FW_LOG_FLOW_EN		BIT(14)	/* Used by command */
-#define ICE_AQC_FW_LOG_ERR_EN		BIT(15)	/* Used by command */
-
-/* Get/Clear FW Log (indirect 0xFF11) */
-struct ice_aqc_get_clear_fw_log {
-	u8 flags;
-#define ICE_AQC_FW_LOG_CLEAR		BIT(0)
-#define ICE_AQC_FW_LOG_MORE_DATA_AVAIL	BIT(1)
-	u8 rsvd1[7];
-	__le32 addr_high;
-	__le32 addr_low;
-};
-
 /* Download Package (indirect 0x0C40) */
 /* Also used for Update Package (indirect 0x0C41 and 0x0C42) */
 struct ice_aqc_download_pkg {
@@ -2211,8 +2139,6 @@ struct ice_aq_desc {
 		struct ice_aqc_add_rdma_qset add_rdma_qset;
 		struct ice_aqc_add_get_update_free_vsi vsi_cmd;
 		struct ice_aqc_add_update_free_vsi_resp add_update_free_vsi_res;
-		struct ice_aqc_fw_logging fw_logging;
-		struct ice_aqc_get_clear_fw_log get_clear_fw_log;
 		struct ice_aqc_download_pkg download_pkg;
 		struct ice_aqc_driver_shared_params drv_shared_params;
 		struct ice_aqc_set_mac_lb set_mac_lb;
@@ -2399,10 +2325,6 @@ enum ice_adminq_opc {
 
 	/* Standalone Commands/Events */
 	ice_aqc_opc_event_lan_overflow			= 0x1001,
-
-	/* debug commands */
-	ice_aqc_opc_fw_logging				= 0xFF09,
-	ice_aqc_opc_fw_logging_info			= 0xFF10,
 };
 
 #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 0e9584e50d82..b781655ee805 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -822,217 +822,6 @@ static void ice_cleanup_fltr_mgmt_struct(struct ice_hw *hw)
 	devm_kfree(ice_hw_to_dev(hw), sw);
 }
 
-/**
- * ice_get_fw_log_cfg - get FW logging configuration
- * @hw: pointer to the HW struct
- */
-static int ice_get_fw_log_cfg(struct ice_hw *hw)
-{
-	struct ice_aq_desc desc;
-	__le16 *config;
-	int status;
-	u16 size;
-
-	size = sizeof(*config) * ICE_AQC_FW_LOG_ID_MAX;
-	config = devm_kzalloc(ice_hw_to_dev(hw), size, GFP_KERNEL);
-	if (!config)
-		return -ENOMEM;
-
-	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_fw_logging_info);
-
-	status = ice_aq_send_cmd(hw, &desc, config, size, NULL);
-	if (!status) {
-		u16 i;
-
-		/* Save FW logging information into the HW structure */
-		for (i = 0; i < ICE_AQC_FW_LOG_ID_MAX; i++) {
-			u16 v, m, flgs;
-
-			v = le16_to_cpu(config[i]);
-			m = (v & ICE_AQC_FW_LOG_ID_M) >> ICE_AQC_FW_LOG_ID_S;
-			flgs = (v & ICE_AQC_FW_LOG_EN_M) >> ICE_AQC_FW_LOG_EN_S;
-
-			if (m < ICE_AQC_FW_LOG_ID_MAX)
-				hw->fw_log.evnts[m].cur = flgs;
-		}
-	}
-
-	devm_kfree(ice_hw_to_dev(hw), config);
-
-	return status;
-}
-
-/**
- * ice_cfg_fw_log - configure FW logging
- * @hw: pointer to the HW struct
- * @enable: enable certain FW logging events if true, disable all if false
- *
- * This function enables/disables the FW logging via Rx CQ events and a UART
- * port based on predetermined configurations. FW logging via the Rx CQ can be
- * enabled/disabled for individual PF's. However, FW logging via the UART can
- * only be enabled/disabled for all PFs on the same device.
- *
- * To enable overall FW logging, the "cq_en" and "uart_en" enable bits in
- * hw->fw_log need to be set accordingly, e.g. based on user-provided input,
- * before initializing the device.
- *
- * When re/configuring FW logging, callers need to update the "cfg" elements of
- * the hw->fw_log.evnts array with the desired logging event configurations for
- * modules of interest. When disabling FW logging completely, the callers can
- * just pass false in the "enable" parameter. On completion, the function will
- * update the "cur" element of the hw->fw_log.evnts array with the resulting
- * logging event configurations of the modules that are being re/configured. FW
- * logging modules that are not part of a reconfiguration operation retain their
- * previous states.
- *
- * Before resetting the device, it is recommended that the driver disables FW
- * logging before shutting down the control queue. When disabling FW logging
- * ("enable" = false), the latest configurations of FW logging events stored in
- * hw->fw_log.evnts[] are not overridden to allow them to be reconfigured after
- * a device reset.
- *
- * When enabling FW logging to emit log messages via the Rx CQ during the
- * device's initialization phase, a mechanism alternative to interrupt handlers
- * needs to be used to extract FW log messages from the Rx CQ periodically and
- * to prevent the Rx CQ from being full and stalling other types of control
- * messages from FW to SW. Interrupts are typically disabled during the device's
- * initialization phase.
- */
-static int ice_cfg_fw_log(struct ice_hw *hw, bool enable)
-{
-	struct ice_aqc_fw_logging *cmd;
-	u16 i, chgs = 0, len = 0;
-	struct ice_aq_desc desc;
-	__le16 *data = NULL;
-	u8 actv_evnts = 0;
-	void *buf = NULL;
-	int status = 0;
-
-	if (!hw->fw_log.cq_en && !hw->fw_log.uart_en)
-		return 0;
-
-	/* Disable FW logging only when the control queue is still responsive */
-	if (!enable &&
-	    (!hw->fw_log.actv_evnts || !ice_check_sq_alive(hw, &hw->adminq)))
-		return 0;
-
-	/* Get current FW log settings */
-	status = ice_get_fw_log_cfg(hw);
-	if (status)
-		return status;
-
-	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_fw_logging);
-	cmd = &desc.params.fw_logging;
-
-	/* Indicate which controls are valid */
-	if (hw->fw_log.cq_en)
-		cmd->log_ctrl_valid |= ICE_AQC_FW_LOG_AQ_VALID;
-
-	if (hw->fw_log.uart_en)
-		cmd->log_ctrl_valid |= ICE_AQC_FW_LOG_UART_VALID;
-
-	if (enable) {
-		/* Fill in an array of entries with FW logging modules and
-		 * logging events being reconfigured.
-		 */
-		for (i = 0; i < ICE_AQC_FW_LOG_ID_MAX; i++) {
-			u16 val;
-
-			/* Keep track of enabled event types */
-			actv_evnts |= hw->fw_log.evnts[i].cfg;
-
-			if (hw->fw_log.evnts[i].cfg == hw->fw_log.evnts[i].cur)
-				continue;
-
-			if (!data) {
-				data = devm_kcalloc(ice_hw_to_dev(hw),
-						    ICE_AQC_FW_LOG_ID_MAX,
-						    sizeof(*data),
-						    GFP_KERNEL);
-				if (!data)
-					return -ENOMEM;
-			}
-
-			val = i << ICE_AQC_FW_LOG_ID_S;
-			val |= hw->fw_log.evnts[i].cfg << ICE_AQC_FW_LOG_EN_S;
-			data[chgs++] = cpu_to_le16(val);
-		}
-
-		/* Only enable FW logging if at least one module is specified.
-		 * If FW logging is currently enabled but all modules are not
-		 * enabled to emit log messages, disable FW logging altogether.
-		 */
-		if (actv_evnts) {
-			/* Leave if there is effectively no change */
-			if (!chgs)
-				goto out;
-
-			if (hw->fw_log.cq_en)
-				cmd->log_ctrl |= ICE_AQC_FW_LOG_AQ_EN;
-
-			if (hw->fw_log.uart_en)
-				cmd->log_ctrl |= ICE_AQC_FW_LOG_UART_EN;
-
-			buf = data;
-			len = sizeof(*data) * chgs;
-			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
-		}
-	}
-
-	status = ice_aq_send_cmd(hw, &desc, buf, len, NULL);
-	if (!status) {
-		/* Update the current configuration to reflect events enabled.
-		 * hw->fw_log.cq_en and hw->fw_log.uart_en indicate if the FW
-		 * logging mode is enabled for the device. They do not reflect
-		 * actual modules being enabled to emit log messages. So, their
-		 * values remain unchanged even when all modules are disabled.
-		 */
-		u16 cnt = enable ? chgs : (u16)ICE_AQC_FW_LOG_ID_MAX;
-
-		hw->fw_log.actv_evnts = actv_evnts;
-		for (i = 0; i < cnt; i++) {
-			u16 v, m;
-
-			if (!enable) {
-				/* When disabling all FW logging events as part
-				 * of device's de-initialization, the original
-				 * configurations are retained, and can be used
-				 * to reconfigure FW logging later if the device
-				 * is re-initialized.
-				 */
-				hw->fw_log.evnts[i].cur = 0;
-				continue;
-			}
-
-			v = le16_to_cpu(data[i]);
-			m = (v & ICE_AQC_FW_LOG_ID_M) >> ICE_AQC_FW_LOG_ID_S;
-			hw->fw_log.evnts[m].cur = hw->fw_log.evnts[m].cfg;
-		}
-	}
-
-out:
-	if (data)
-		devm_kfree(ice_hw_to_dev(hw), data);
-
-	return status;
-}
-
-/**
- * ice_output_fw_log
- * @hw: pointer to the HW struct
- * @desc: pointer to the AQ message descriptor
- * @buf: pointer to the buffer accompanying the AQ message
- *
- * Formats a FW Log message and outputs it via the standard driver logs.
- */
-void ice_output_fw_log(struct ice_hw *hw, struct ice_aq_desc *desc, void *buf)
-{
-	ice_debug(hw, ICE_DBG_FW_LOG, "[ FW Log Msg Start ]\n");
-	ice_debug_array(hw, ICE_DBG_FW_LOG, 16, 1, (u8 *)buf,
-			le16_to_cpu(desc->datalen));
-	ice_debug(hw, ICE_DBG_FW_LOG, "[ FW Log Msg End ]\n");
-}
-
 /**
  * ice_get_itr_intrl_gran
  * @hw: pointer to the HW struct
@@ -1090,11 +879,6 @@ int ice_init_hw(struct ice_hw *hw)
 	if (status)
 		goto err_unroll_cqinit;
 
-	/* Enable FW logging. Not fatal if this fails. */
-	status = ice_cfg_fw_log(hw, true);
-	if (status)
-		ice_debug(hw, ICE_DBG_INIT, "Failed to enable FW logging.\n");
-
 	status = ice_clear_pf_cfg(hw);
 	if (status)
 		goto err_unroll_cqinit;
@@ -1247,8 +1031,6 @@ void ice_deinit_hw(struct ice_hw *hw)
 		hw->port_info = NULL;
 	}
 
-	/* Attempt to disable FW logging before shutting down control queues */
-	ice_cfg_fw_log(hw, false);
 	ice_destroy_all_ctrlq(hw);
 
 	/* Clear VSI contexts if not already cleared */
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 69984fea7fce..517702af327c 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -1486,9 +1486,6 @@ static int __ice_clean_ctrlq(struct ice_pf *pf, enum ice_ctl_q q_type)
 			if (!ice_is_malicious_vf(pf, &event, i, pending))
 				ice_vc_process_vf_msg(pf, &event);
 			break;
-		case ice_aqc_opc_fw_logging:
-			ice_output_fw_log(hw, &event.desc, event.msg_buf);
-			break;
 		case ice_aqc_opc_lldp_set_mib_change:
 			ice_dcb_process_lldp_set_mib_change(pf, &event);
 			break;
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index f77992cbfa41..dd064b297d77 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -728,24 +728,6 @@ struct ice_switch_info {
 	DECLARE_BITMAP(prof_res_bm[ICE_MAX_NUM_PROFILES], ICE_MAX_FV_WORDS);
 };
 
-/* FW logging configuration */
-struct ice_fw_log_evnt {
-	u8 cfg : 4;	/* New event enables to configure */
-	u8 cur : 4;	/* Current/active event enables */
-};
-
-struct ice_fw_log_cfg {
-	u8 cq_en : 1;    /* FW logging is enabled via the control queue */
-	u8 uart_en : 1;  /* FW logging is enabled via UART for all PFs */
-	u8 actv_evnts;   /* Cumulation of currently enabled log events */
-
-#define ICE_FW_LOG_EVNT_INFO	(ICE_AQC_FW_LOG_INFO_EN >> ICE_AQC_FW_LOG_EN_S)
-#define ICE_FW_LOG_EVNT_INIT	(ICE_AQC_FW_LOG_INIT_EN >> ICE_AQC_FW_LOG_EN_S)
-#define ICE_FW_LOG_EVNT_FLOW	(ICE_AQC_FW_LOG_FLOW_EN >> ICE_AQC_FW_LOG_EN_S)
-#define ICE_FW_LOG_EVNT_ERR	(ICE_AQC_FW_LOG_ERR_EN >> ICE_AQC_FW_LOG_EN_S)
-	struct ice_fw_log_evnt evnts[ICE_AQC_FW_LOG_ID_MAX];
-};
-
 /* Enum defining the different states of the mailbox snapshot in the
  * PF-VF mailbox overflow detection algorithm. The snapshot can be in
  * states:
@@ -878,8 +860,6 @@ struct ice_hw {
 	u8 fw_patch;		/* firmware patch version */
 	u32 fw_build;		/* firmware build number */
 
-	struct ice_fw_log_cfg fw_log;
-
 /* 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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Intel-wired-lan] [PATCH net-next 2/5] ice: enable devlink to check FW logging status
  2022-11-28 21:47 [Intel-wired-lan] [PATCH net-next 0/5] add v2 FW logging for ice driver Paul M Stillwell Jr
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 1/5] ice: remove FW logging code Paul M Stillwell Jr
@ 2022-11-28 21:47 ` Paul M Stillwell Jr
  2022-12-01  0:39   ` Tony Nguyen
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 3/5] ice: add ability to query/set FW log level and resolution Paul M Stillwell Jr
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 21+ messages in thread
From: Paul M Stillwell Jr @ 2022-11-28 21:47 UTC (permalink / raw)
  To: intel-wired-lan

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)
+ */
+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)
+	u8 rsp_flag;
+#define ICE_AQC_FW_LOG_MORE_DATA	BIT(1)
+	__le16 fw_rt_msb;
+	union {
+		struct {
+			__le32 fw_rt_lsb;
+		} sync;
+		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;
+	/* 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"
+#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? */
+	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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Intel-wired-lan] [PATCH net-next 3/5] ice: add ability to query/set FW log level and resolution
  2022-11-28 21:47 [Intel-wired-lan] [PATCH net-next 0/5] add v2 FW logging for ice driver Paul M Stillwell Jr
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 1/5] ice: remove FW logging code Paul M Stillwell Jr
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 2/5] ice: enable devlink to check FW logging status Paul M Stillwell Jr
@ 2022-11-28 21:47 ` Paul M Stillwell Jr
  2022-11-29 11:48   ` Wilczynski, Michal
                     ` (2 more replies)
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 4/5] ice: disable FW logging on driver unload Paul M Stillwell Jr
                   ` (2 subsequent siblings)
  5 siblings, 3 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2022-11-28 21:47 UTC (permalink / raw)
  To: intel-wired-lan

The E8xx has the ability to change the FW log level and
the granularity at which the logs get output. Enable
the ability to see what the current values are and to
change them.

Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
---
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   4 +
 drivers/net/ethernet/intel/ice/ice_common.c   |   4 +-
 drivers/net/ethernet/intel/ice/ice_devlink.c  | 142 ++++++++-
 drivers/net/ethernet/intel/ice/ice_fwlog.c    | 277 ++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_fwlog.h    |   5 +
 drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
 6 files changed, 429 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index d0026161a2b4..8fa18bc5d441 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -2406,7 +2406,11 @@ enum ice_adminq_opc {
 
 	/* Standalone Commands/Events */
 	ice_aqc_opc_event_lan_overflow			= 0x1001,
+	/* FW Logging Commands */
+	ice_aqc_opc_fw_logs_config			= 0xFF30,
+	ice_aqc_opc_fw_logs_register			= 0xFF31,
 	ice_aqc_opc_fw_logs_query			= 0xFF32,
+	ice_aqc_opc_fw_logs_event			= 0xFF33,
 };
 
 #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 ecdc1ebb45d5..0b3adac13c66 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -879,7 +879,9 @@ int ice_init_hw(struct ice_hw *hw)
 	if (status)
 		goto err_unroll_cqinit;
 
-	ice_fwlog_set_support_ena(hw);
+	status = ice_fwlog_init(hw);
+	if (status)
+		ice_debug(hw, ICE_DBG_FW_LOG, "Error initializing FW logging: %d\n", status);
 
 	status = ice_clear_pf_cfg(hw);
 	if (status)
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 8843ff492f7f..ca67f2741f83 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -1488,6 +1488,8 @@ 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,
+	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
+	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
 };
 
 static int
@@ -1530,8 +1532,121 @@ 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;
+	struct ice_pf *pf = devlink_priv(devlink);
+	struct ice_hw *hw = &pf->hw;
+	int status;
+
+	/* only support fw log commands on PF 0 */
+	if (hw->bus.func)
+		return -EOPNOTSUPP;
+
+	if (hw->fwlog_ena == ctx->val.vbool)
+		return 0;
+
+	hw->fwlog_ena = ctx->val.vbool;
+
+	if (hw->fwlog_ena)
+		hw->fwlog_cfg.options |= ICE_FWLOG_OPTION_ARQ_ENA;
+	else
+		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
+
+	/* send the cfg to FW and register for events */
+	status = ice_fwlog_set(hw, &hw->fwlog_cfg);
+	if (status)
+		return status;
+
+	if (hw->fwlog_ena) {
+		status = ice_fwlog_register(hw);
+		if (status)
+			return status;
+	} else {
+		status = ice_fwlog_unregister(hw);
+		if (status)
+			return status;
+	}
+
+	return 0;
+}
+
+static int
+ice_devlink_fwlog_level_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;
+
+	/* all the log levels are the same so pick the first one */
+	ctx->val.vu8 = pf->hw.fwlog_cfg.module_entries[0].log_level;
+
+	return 0;
+}
+
+static int
+ice_devlink_fwlog_level_set(struct devlink *devlink, u32 id,
+			    struct devlink_param_gset_ctx *ctx)
+{
+	struct ice_pf *pf = devlink_priv(devlink);
+	struct ice_fwlog_cfg *cfg;
+	u8 i;
+
+	if (pf->hw.bus.func)
+		return -EOPNOTSUPP;
+
+	if (ctx->val.vu8 >= ICE_FWLOG_LEVEL_INVALID) {
+		dev_err(ice_pf_to_dev(pf), "Log level is greater than allowed! %d\n",
+			ctx->val.vu8);
+		return -EINVAL;
+	}
+
+	cfg = &pf->hw.fwlog_cfg;
+
+	/* update the log level for all modules to the same thing. this gets
+	 * written to the FW when the user says enable logging
+	 */
+	for (i = 0; i < ICE_AQC_FW_LOG_ID_MAX; i++)
+		cfg->module_entries[i].log_level = ctx->val.vu8;
+
+	return 0;
+}
+
+static int
+ice_devlink_fwlog_resolution_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.vu8 = pf->hw.fwlog_cfg.log_resolution;
+
+	return 0;
+}
+
+static int
+ice_devlink_fwlog_resolution_set(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;
+
+	if (ctx->val.vu8 < ICE_AQC_FW_LOG_MIN_RESOLUTION ||
+	    ctx->val.vu8 > ICE_AQC_FW_LOG_MAX_RESOLUTION) {
+		dev_err(ice_pf_to_dev(pf), "Log resolution is not allowed! Should be between 1 - 128: %d\n",
+			ctx->val.vu8);
+		return -EINVAL;
+	}
+
+	pf->hw.fwlog_cfg.log_resolution = ctx->val.vu8;
+
+	return 0;
 }
 
 static const struct devlink_param ice_devlink_params[] = {
@@ -1562,6 +1677,18 @@ static const struct devlink_param ice_devlink_params[] = {
 			     ice_devlink_fwlog_enabled_get,
 			     ice_devlink_fwlog_enabled_set,
 			     NULL),
+	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
+			     "fwlog_level", DEVLINK_PARAM_TYPE_U8,
+			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			     ice_devlink_fwlog_level_get,
+			     ice_devlink_fwlog_level_set,
+			     NULL),
+	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
+			     "fwlog_resolution", DEVLINK_PARAM_TYPE_U8,
+			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			     ice_devlink_fwlog_resolution_get,
+			     ice_devlink_fwlog_resolution_set,
+			     NULL),
 };
 
 static void ice_devlink_free(void *devlink_ptr)
@@ -1662,11 +1789,20 @@ int ice_devlink_register_params(struct ice_pf *pf)
 					   ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED,
 					   value);
 
-	value.vbool = false;
 	devlink_param_driverinit_value_set(devlink,
 					   ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED,
 					   value);
 
+	value.vu8 = ICE_FWLOG_LEVEL_NORMAL;
+	devlink_param_driverinit_value_set(devlink,
+					   ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
+					   value);
+
+	/* set the default value for the FW to pack 10 messages per AQ event */
+	value.vu8 = 10;
+	devlink_param_driverinit_value_set(devlink,
+					   ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
+					   value);
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.c b/drivers/net/ethernet/intel/ice/ice_fwlog.c
index 67e670c62091..1174fd889307 100644
--- a/drivers/net/ethernet/intel/ice/ice_fwlog.c
+++ b/drivers/net/ethernet/intel/ice/ice_fwlog.c
@@ -4,6 +4,165 @@
 #include "ice_common.h"
 #include "ice_fwlog.h"
 
+/**
+ * cache_cfg - Cache FW logging config
+ * @hw: pointer to the HW structure
+ * @cfg: config to cache
+ */
+static void cache_cfg(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
+{
+	hw->fwlog_cfg = *cfg;
+}
+
+/**
+ * valid_module_entries - validate all the module entry IDs and log levels
+ * @hw: pointer to the HW structure
+ * @entries: entries to validate
+ * @num_entries: number of entries to validate
+ */
+static bool
+valid_module_entries(struct ice_hw *hw, struct ice_fwlog_module_entry *entries,
+		     u16 num_entries)
+{
+	u16 i;
+
+	if (!entries) {
+		ice_debug(hw, ICE_DBG_FW_LOG, "Null ice_fwlog_module_entry array\n");
+		return false;
+	}
+
+	if (!num_entries) {
+		ice_debug(hw, ICE_DBG_FW_LOG, "num_entries must be non-zero\n");
+		return false;
+	}
+
+	for (i = 0; i < num_entries; i++) {
+		struct ice_fwlog_module_entry *entry = &entries[i];
+
+		if (entry->module_id >= ICE_AQC_FW_LOG_ID_MAX) {
+			ice_debug(hw, ICE_DBG_FW_LOG, "Invalid module_id %u, max valid module_id is %u\n",
+				  entry->module_id, ICE_AQC_FW_LOG_ID_MAX - 1);
+			return false;
+		}
+
+		if (entry->log_level >= ICE_FWLOG_LEVEL_INVALID) {
+			ice_debug(hw, ICE_DBG_FW_LOG, "Invalid log_level %u, max valid log_level is %u\n",
+				  entry->log_level,
+				  ICE_AQC_FW_LOG_ID_MAX - 1);
+			return false;
+		}
+	}
+
+	return true;
+}
+
+/**
+ * valid_cfg - validate entire configuration
+ * @hw: pointer to the HW structure
+ * @cfg: config to validate
+ */
+static bool valid_cfg(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
+{
+	if (!cfg) {
+		ice_debug(hw, ICE_DBG_FW_LOG, "Null ice_fwlog_cfg\n");
+		return false;
+	}
+
+	if (cfg->log_resolution < ICE_AQC_FW_LOG_MIN_RESOLUTION ||
+	    cfg->log_resolution > ICE_AQC_FW_LOG_MAX_RESOLUTION) {
+		ice_debug(hw, ICE_DBG_FW_LOG, "Unsupported log_resolution %u, must be between %u and %u\n",
+			  cfg->log_resolution, ICE_AQC_FW_LOG_MIN_RESOLUTION,
+			  ICE_AQC_FW_LOG_MAX_RESOLUTION);
+		return false;
+	}
+
+	if (!valid_module_entries(hw, cfg->module_entries,
+				  ICE_AQC_FW_LOG_ID_MAX))
+		return false;
+
+	return true;
+}
+
+/**
+ * ice_fwlog_init - Initialize FW logging variables
+ * @hw: pointer to the HW structure
+ *
+ * This function should be called on driver initialization during
+ * ice_init_hw().
+ */
+int ice_fwlog_init(struct ice_hw *hw)
+{
+	int status;
+
+	ice_fwlog_set_support_ena(hw);
+
+	if (ice_fwlog_supported(hw)) {
+		struct ice_fwlog_cfg *cfg;
+
+		cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
+		if (!cfg)
+			return -ENOMEM;
+
+		/* read the current config from the FW and store it */
+		status = ice_fwlog_get(hw, cfg);
+		if (status)
+			return status;
+	}
+
+	return 0;
+}
+
+/**
+ * ice_aq_fwlog_set - Set FW logging configuration AQ command (0xFF30)
+ * @hw: pointer to the HW structure
+ * @entries: entries to configure
+ * @num_entries: number of @entries
+ * @options: options from ice_fwlog_cfg->options structure
+ * @log_resolution: logging resolution
+ */
+static int
+ice_aq_fwlog_set(struct ice_hw *hw, struct ice_fwlog_module_entry *entries,
+		 u16 num_entries, u16 options, u16 log_resolution)
+{
+	struct ice_aqc_fw_log_cfg_resp *fw_modules;
+	struct ice_aqc_fw_log *cmd;
+	struct ice_aq_desc desc;
+	int status;
+	u16 i;
+
+	fw_modules = kcalloc(num_entries, sizeof(*fw_modules), GFP_KERNEL);
+	if (!fw_modules)
+		return -ENOMEM;
+
+	for (i = 0; i < num_entries; i++) {
+		fw_modules[i].module_identifier =
+			cpu_to_le16(entries[i].module_id);
+		fw_modules[i].log_level = entries[i].log_level;
+	}
+
+	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_fw_logs_config);
+	desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
+
+	cmd = &desc.params.fw_log;
+
+	cmd->cmd_flags = ICE_AQC_FW_LOG_CONF_SET_VALID;
+	cmd->ops.cfg.log_resolution = cpu_to_le16(log_resolution);
+	cmd->ops.cfg.mdl_cnt = cpu_to_le16(num_entries);
+
+	if (options & ICE_FWLOG_OPTION_ARQ_ENA)
+		cmd->cmd_flags |= ICE_AQC_FW_LOG_CONF_AQ_EN;
+	if (options & ICE_FWLOG_OPTION_UART_ENA)
+		cmd->cmd_flags |= ICE_AQC_FW_LOG_CONF_UART_EN;
+
+	status = ice_aq_send_cmd(hw, &desc, fw_modules,
+				 sizeof(*fw_modules) * num_entries,
+				 NULL);
+
+	kfree(fw_modules);
+
+	return status;
+}
+
 /**
  * ice_fwlog_supported - Cached for whether FW supports FW logging or not
  * @hw: pointer to the HW structure
@@ -16,6 +175,99 @@ bool ice_fwlog_supported(struct ice_hw *hw)
 	return hw->fwlog_support_ena;
 }
 
+/**
+ * ice_fwlog_set - Set the firmware logging settings
+ * @hw: pointer to the HW structure
+ * @cfg: config used to set firmware logging
+ *
+ * This function should be called whenever the driver needs to set the firmware
+ * logging configuration. It can be called on initialization, reset, or during
+ * runtime.
+ *
+ * If the PF wishes to receive FW logging then it must register via
+ * ice_fwlog_register. Note, that ice_fwlog_register does not need to be called
+ * for init.
+ */
+int
+ice_fwlog_set(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
+{
+	int status;
+
+	if (!ice_fwlog_supported(hw))
+		return -EOPNOTSUPP;
+
+	if (!valid_cfg(hw, cfg))
+		return -EINVAL;
+
+	status = ice_aq_fwlog_set(hw, cfg->module_entries,
+				  ICE_AQC_FW_LOG_ID_MAX, cfg->options,
+				  cfg->log_resolution);
+	if (!status)
+		cache_cfg(hw, cfg);
+
+	return status;
+}
+
+/**
+ * ice_aq_fwlog_register - Register PF for firmware logging events (0xFF31)
+ * @hw: pointer to the HW structure
+ * @reg: true to register and false to unregister
+ */
+static int ice_aq_fwlog_register(struct ice_hw *hw, bool reg)
+{
+	struct ice_aq_desc desc;
+
+	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_fw_logs_register);
+
+	if (reg)
+		desc.params.fw_log.cmd_flags = ICE_AQC_FW_LOG_AQ_REGISTER;
+
+	return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
+}
+
+/**
+ * ice_fwlog_register - Register the PF for firmware logging
+ * @hw: pointer to the HW structure
+ *
+ * After this call the PF will start to receive firmware logging based on the
+ * configuration set in ice_fwlog_set.
+ */
+int ice_fwlog_register(struct ice_hw *hw)
+{
+	int status;
+
+	if (!ice_fwlog_supported(hw))
+		return -EOPNOTSUPP;
+
+	status = ice_aq_fwlog_register(hw, true);
+	if (status)
+		ice_debug(hw, ICE_DBG_FW_LOG, "Failed to register for firmware logging events over ARQ\n");
+	else
+		hw->fwlog_cfg.options |= ICE_FWLOG_OPTION_IS_REGISTERED;
+
+	return status;
+}
+
+/**
+ * ice_fwlog_unregister - Unregister the PF from firmware logging
+ * @hw: pointer to the HW structure
+ */
+int ice_fwlog_unregister(struct ice_hw *hw)
+{
+	int status;
+
+	if (!ice_fwlog_supported(hw))
+		return -EOPNOTSUPP;
+
+	status = ice_aq_fwlog_register(hw, false);
+	if (status)
+		ice_debug(hw, ICE_DBG_FW_LOG, "Failed to unregister from firmware logging events over ARQ\n");
+	else
+		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_IS_REGISTERED;
+
+	return status;
+}
+
 /**
  * ice_aq_fwlog_get - Get the current firmware logging configuration (0xFF32)
  * @hw: pointer to the HW structure
@@ -115,3 +367,28 @@ void ice_fwlog_set_support_ena(struct ice_hw *hw)
 
 	kfree(cfg);
 }
+
+/**
+ * ice_fwlog_get - Get the firmware logging settings
+ * @hw: pointer to the HW structure
+ * @cfg: config to populate based on current firmware logging settings
+ */
+int
+ice_fwlog_get(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
+{
+	int status;
+
+	if (!ice_fwlog_supported(hw))
+		return -EOPNOTSUPP;
+
+	if (!cfg)
+		return -EINVAL;
+
+	status = ice_aq_fwlog_get(hw, cfg);
+	if (status)
+		return status;
+
+	cache_cfg(hw, cfg);
+
+	return 0;
+}
diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.h b/drivers/net/ethernet/intel/ice/ice_fwlog.h
index d7371253b8e5..66648c5ba92c 100644
--- a/drivers/net/ethernet/intel/ice/ice_fwlog.h
+++ b/drivers/net/ethernet/intel/ice/ice_fwlog.h
@@ -48,4 +48,9 @@ struct ice_fwlog_cfg {
 
 void ice_fwlog_set_support_ena(struct ice_hw *hw);
 bool ice_fwlog_supported(struct ice_hw *hw);
+int ice_fwlog_init(struct ice_hw *hw);
+int ice_fwlog_set(struct ice_hw *hw, struct ice_fwlog_cfg *cfg);
+int ice_fwlog_get(struct ice_hw *hw, struct ice_fwlog_cfg *cfg);
+int ice_fwlog_register(struct ice_hw *hw);
+int ice_fwlog_unregister(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 10b78faf0a32..c524179e79f0 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -861,6 +861,7 @@ struct ice_hw {
 	u8 fw_patch;		/* firmware patch version */
 	u32 fw_build;		/* firmware build number */
 
+	struct ice_fwlog_cfg fwlog_cfg;
 	bool fwlog_support_ena; /* does hardware support FW logging? */
 	bool fwlog_ena; /* currently logging? */
 
-- 
2.35.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Intel-wired-lan] [PATCH net-next 4/5] ice: disable FW logging on driver unload
  2022-11-28 21:47 [Intel-wired-lan] [PATCH net-next 0/5] add v2 FW logging for ice driver Paul M Stillwell Jr
                   ` (2 preceding siblings ...)
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 3/5] ice: add ability to query/set FW log level and resolution Paul M Stillwell Jr
@ 2022-11-28 21:47 ` Paul M Stillwell Jr
  2022-11-29 12:05   ` Wilczynski, Michal
  2022-12-01  0:40   ` Tony Nguyen
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 5/5] ice: use debugfs to output FW log data Paul M Stillwell Jr
  2022-12-01  0:39 ` [Intel-wired-lan] [PATCH net-next 0/5] add v2 FW logging for ice driver Tony Nguyen
  5 siblings, 2 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2022-11-28 21:47 UTC (permalink / raw)
  To: intel-wired-lan

The FW is running in it's own context irregardless of what the driver
is doing. In this case, if the driver previously registered for FW
log events and then the driver unloads without informing the FW to
unregister for FW log events then the FW still has a timer running to
output FW logs.

The next time the driver loads and tries to register for FW log events
then the FW returns an error, but still enables the continued
outputting of FW logs. This causes an IO error to devlink which isn't
intuitive since the logs are still being output.

Fix this by disabling FW logging when the driver is being unloaded.

Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_devlink.c | 32 +++++++++++++++-----
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index ca67f2741f83..923556e6ae79 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -378,6 +378,10 @@ static int ice_devlink_info_get(struct devlink *devlink,
 enum ice_devlink_param_id {
 	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
 	ICE_DEVLINK_PARAM_ID_TX_BALANCE,
+	ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED,
+	ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED,
+	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
+	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
 };
 
 /**
@@ -1484,14 +1488,6 @@ 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,
-	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
-	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
-};
-
 static int
 ice_devlink_fwlog_supported_get(struct devlink *devlink, u32 id,
 				struct devlink_param_gset_ctx *ctx)
@@ -1743,6 +1739,26 @@ void ice_devlink_register(struct ice_pf *pf)
  */
 void ice_devlink_unregister(struct ice_pf *pf)
 {
+	struct ice_hw *hw = &pf->hw;
+
+	/* make sure FW logging is disabled to not put the FW in a weird state
+	 * for the next driver load
+	 */
+	if (hw->fwlog_ena) {
+		int status;
+
+		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
+		status = ice_fwlog_set(hw, &hw->fwlog_cfg);
+		if (status)
+			dev_warn(ice_pf_to_dev(pf), "Unable to turn off FW logging, status: %d\n",
+				 status);
+
+		status = ice_fwlog_unregister(hw);
+		if (status)
+			dev_warn(ice_pf_to_dev(pf), "Unable to unregister FW logging, status: %d\n",
+				 status);
+	}
+
 	devlink_unregister(priv_to_devlink(pf));
 }
 
-- 
2.35.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Intel-wired-lan] [PATCH net-next 5/5] ice: use debugfs to output FW log data
  2022-11-28 21:47 [Intel-wired-lan] [PATCH net-next 0/5] add v2 FW logging for ice driver Paul M Stillwell Jr
                   ` (3 preceding siblings ...)
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 4/5] ice: disable FW logging on driver unload Paul M Stillwell Jr
@ 2022-11-28 21:47 ` Paul M Stillwell Jr
  2022-11-29 13:53   ` Paul Menzel
  2022-12-01  0:40   ` Tony Nguyen
  2022-12-01  0:39 ` [Intel-wired-lan] [PATCH net-next 0/5] add v2 FW logging for ice driver Tony Nguyen
  5 siblings, 2 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2022-11-28 21:47 UTC (permalink / raw)
  To: intel-wired-lan

The FW log data can be quite large so we don't want to use syslog.
Instead take advantage of debugfs to write the data to.

Also, a previous patch addded codde to disable FW logging when
the driver was unloaded. The code was added to
ice_devlink_unregister(), which didn't seem correct so move the
disabling of FW logging on driver unload to a better place.

Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
---
 drivers/net/ethernet/intel/ice/Makefile       |   3 +-
 drivers/net/ethernet/intel/ice/ice.h          |  24 ++++
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   2 +
 drivers/net/ethernet/intel/ice/ice_debugfs.c  | 109 ++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_devlink.c  |  20 ----
 drivers/net/ethernet/intel/ice/ice_main.c     |  98 ++++++++++++++++
 6 files changed, 235 insertions(+), 21 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/ice/ice_debugfs.c

diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
index 750fed7e07d7..5e0013330c46 100644
--- a/drivers/net/ethernet/intel/ice/Makefile
+++ b/drivers/net/ethernet/intel/ice/Makefile
@@ -33,7 +33,8 @@ ice-y := ice_main.o	\
 	 ice_ethtool.o  \
 	 ice_repr.o	\
 	 ice_tc_lib.o	\
-	 ice_fwlog.o
+	 ice_fwlog.o	\
+	 ice_debugfs.o
 ice-$(CONFIG_PCI_IOV) +=	\
 	ice_sriov.o		\
 	ice_virtchnl.o		\
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index ea64bcff108a..c8f1f34b8fbc 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -552,6 +552,9 @@ struct ice_pf {
 	struct ice_vsi_stats **vsi_stats;
 	struct ice_sw *first_sw;	/* first switch created by firmware */
 	u16 eswitch_mode;		/* current mode of eswitch */
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *ice_debugfs_pf;
+#endif /* CONFIG_DEBUG_FS */
 	struct ice_vfs vfs;
 	DECLARE_BITMAP(features, ICE_F_MAX);
 	DECLARE_BITMAP(state, ICE_STATE_NBITS);
@@ -634,6 +637,8 @@ struct ice_pf {
 #define ICE_VF_AGG_NODE_ID_START	65
 #define ICE_MAX_VF_AGG_NODES		32
 	struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES];
+	struct list_head fwlog_data_list;
+	u8 fwlog_list_count;
 };
 
 struct ice_netdev_priv {
@@ -648,6 +653,15 @@ struct ice_netdev_priv {
 	struct list_head tc_indr_block_priv_list;
 };
 
+struct ice_fwlog_data {
+	struct list_head list;
+	u16 data_size;
+	u8 *data;
+};
+
+/* define the maximum number of items that can be in the list */
+#define ICE_FWLOG_MAX_SIZE	64
+
 /**
  * ice_vector_ch_enabled
  * @qv: pointer to q_vector, can be NULL
@@ -872,6 +886,16 @@ static inline bool ice_is_adq_active(struct ice_pf *pf)
 	return false;
 }
 
+#ifdef CONFIG_DEBUG_FS
+void ice_debugfs_fwlog_init(struct ice_pf *pf);
+void ice_debugfs_init(void);
+void ice_debugfs_exit(void);
+#else
+static inline void ice_debugfs_fwlog_init(struct ice_pf *pf) { }
+static inline void ice_debugfs_init(void) { }
+static inline void ice_debugfs_exit(void) { }
+#endif /* CONFIG_DEBUG_FS */
+
 bool netif_is_ice(struct net_device *dev);
 int ice_vsi_setup_tx_rings(struct ice_vsi *vsi);
 int ice_vsi_setup_rx_rings(struct ice_vsi *vsi);
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 8fa18bc5d441..d17bcc778bdd 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -2406,11 +2406,13 @@ enum ice_adminq_opc {
 
 	/* Standalone Commands/Events */
 	ice_aqc_opc_event_lan_overflow			= 0x1001,
+
 	/* FW Logging Commands */
 	ice_aqc_opc_fw_logs_config			= 0xFF30,
 	ice_aqc_opc_fw_logs_register			= 0xFF31,
 	ice_aqc_opc_fw_logs_query			= 0xFF32,
 	ice_aqc_opc_fw_logs_event			= 0xFF33,
+	ice_aqc_opc_fw_logs_get				= 0xFF34,
 };
 
 #endif /* _ICE_ADMINQ_CMD_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_debugfs.c b/drivers/net/ethernet/intel/ice/ice_debugfs.c
new file mode 100644
index 000000000000..767df937f56a
--- /dev/null
+++ b/drivers/net/ethernet/intel/ice/ice_debugfs.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, Intel Corporation. */
+
+#include <linux/fs.h>
+#include <linux/debugfs.h>
+#include <linux/random.h>
+#include "ice.h"
+
+static struct dentry *ice_debugfs_root;
+
+/**
+ * ice_debugfs_command_read - read from command datum
+ * @filp: the opened file
+ * @buffer: where to write the data for the user to read
+ * @count: the size of the user's buffer
+ * @ppos: file position offset
+ */
+static ssize_t ice_debugfs_command_read(struct file *filp, char __user *buffer,
+					size_t count, loff_t *ppos)
+{
+	struct ice_pf *pf = filp->private_data;
+	struct device *dev = ice_pf_to_dev(pf);
+	struct ice_fwlog_data *log, *tmp_log;
+	int data_copied = 0;
+
+	if (list_empty(&pf->fwlog_data_list)) {
+		dev_info(dev, "FW log is empty\n");
+		return 0;
+	}
+
+	list_for_each_entry_safe(log, tmp_log, &pf->fwlog_data_list, list) {
+		u16 cur_buf_len = log->data_size;
+		int retval;
+
+		if (cur_buf_len > count)
+			break;
+
+		retval = copy_to_user(buffer, log->data, cur_buf_len);
+		if (retval)
+			return -EFAULT;
+
+		data_copied += cur_buf_len;
+		buffer += cur_buf_len;
+		count -= cur_buf_len;
+		*ppos += cur_buf_len;
+
+		/* don't delete the list element until we know it got copied */
+		kfree(log->data);
+		list_del(&log->list);
+		kfree(log);
+		pf->fwlog_list_count--;
+	}
+
+	return data_copied;
+}
+
+static const struct file_operations ice_debugfs_command_fops = {
+	.owner = THIS_MODULE,
+	.open  = simple_open,
+	.read = ice_debugfs_command_read,
+};
+
+/**
+ * ice_debugfs_fwlog_init - setup the debugfs directory
+ * @pf: the ice that is starting up
+ */
+void ice_debugfs_fwlog_init(struct ice_pf *pf)
+{
+	const char *name = pci_name(pf->pdev);
+	struct dentry *pfile;
+
+	/* only support fw log commands on PF 0 */
+	if (pf->hw.bus.func)
+		return;
+
+	pf->ice_debugfs_pf = debugfs_create_dir(name, ice_debugfs_root);
+	if (IS_ERR(pf->ice_debugfs_pf))
+		return;
+
+	pfile = debugfs_create_file("fwlog", 0400, pf->ice_debugfs_pf, pf,
+				    &ice_debugfs_command_fops);
+	if (!pfile)
+		goto create_failed;
+
+	return;
+
+create_failed:
+	dev_err(ice_pf_to_dev(pf), "debugfs dir/file for %s failed\n", name);
+	debugfs_remove_recursive(pf->ice_debugfs_pf);
+}
+
+/**
+ * ice_debugfs_init - create root directory for debugfs entries
+ */
+void ice_debugfs_init(void)
+{
+	ice_debugfs_root = debugfs_create_dir(KBUILD_MODNAME, NULL);
+	if (IS_ERR(ice_debugfs_root))
+		pr_info("init of debugfs failed\n");
+}
+
+/**
+ * ice_debugfs_exit - remove debugfs entries
+ */
+void ice_debugfs_exit(void)
+{
+	debugfs_remove_recursive(ice_debugfs_root);
+	ice_debugfs_root = NULL;
+}
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 923556e6ae79..148902b515bf 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -1739,26 +1739,6 @@ void ice_devlink_register(struct ice_pf *pf)
  */
 void ice_devlink_unregister(struct ice_pf *pf)
 {
-	struct ice_hw *hw = &pf->hw;
-
-	/* make sure FW logging is disabled to not put the FW in a weird state
-	 * for the next driver load
-	 */
-	if (hw->fwlog_ena) {
-		int status;
-
-		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
-		status = ice_fwlog_set(hw, &hw->fwlog_cfg);
-		if (status)
-			dev_warn(ice_pf_to_dev(pf), "Unable to turn off FW logging, status: %d\n",
-				 status);
-
-		status = ice_fwlog_unregister(hw);
-		if (status)
-			dev_warn(ice_pf_to_dev(pf), "Unable to unregister FW logging, status: %d\n",
-				 status);
-	}
-
 	devlink_unregister(priv_to_devlink(pf));
 }
 
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 517702af327c..01cf4cb78009 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -1213,6 +1213,45 @@ ice_handle_link_event(struct ice_pf *pf, struct ice_rq_event_info *event)
 	return status;
 }
 
+/**
+ * ice_get_fwlog_data - copy the FW log data from ARQ event
+ * @pf: PF that the FW log event is associated with
+ * @event: event structure containing FW log data
+ */
+static void
+ice_get_fwlog_data(struct ice_pf *pf, struct ice_rq_event_info *event)
+{
+	struct device *dev = ice_pf_to_dev(pf);
+	struct ice_fwlog_data *fwlog;
+
+	if (pf->fwlog_list_count >= ICE_FWLOG_MAX_SIZE) {
+		dev_info(dev, "Reached max list size for fwlog list!\n");
+		return;
+	}
+
+	fwlog = kzalloc(sizeof(*fwlog), GFP_KERNEL);
+	if (!fwlog) {
+		dev_warn(dev, "Couldn't allocate memory for FWlog element\n");
+		return;
+	}
+
+	INIT_LIST_HEAD(&fwlog->list);
+
+	fwlog->data_size = le16_to_cpu(event->desc.datalen);
+	fwlog->data = kzalloc(fwlog->data_size, GFP_KERNEL);
+	if (!fwlog->data) {
+		dev_warn(dev, "Couldn't allocate memory for FWlog data\n");
+		kfree(fwlog);
+		return;
+	}
+
+	memcpy(fwlog->data, event->msg_buf, fwlog->data_size);
+
+	list_add_tail(&fwlog->list, &pf->fwlog_data_list);
+
+	pf->fwlog_list_count++;
+}
+
 enum ice_aq_task_state {
 	ICE_AQ_TASK_WAITING = 0,
 	ICE_AQ_TASK_COMPLETE,
@@ -1486,6 +1525,9 @@ static int __ice_clean_ctrlq(struct ice_pf *pf, enum ice_ctl_q q_type)
 			if (!ice_is_malicious_vf(pf, &event, i, pending))
 				ice_vc_process_vf_msg(pf, &event);
 			break;
+		case ice_aqc_opc_fw_logs_event:
+			ice_get_fwlog_data(pf, &event);
+			break;
 		case ice_aqc_opc_lldp_set_mib_change:
 			ice_dcb_process_lldp_set_mib_change(pf, &event);
 			break;
@@ -4688,6 +4730,52 @@ static int ice_register_netdev(struct ice_pf *pf)
 	return err;
 }
 
+/**
+ * ice_pf_fwlog_init - initialize FW logging on device init
+ * @pf: pointer to the PF struct
+ *
+ * This should always be called after ice_hw_init().
+ */
+static void
+ice_pf_fwlog_init(struct ice_pf *pf)
+{
+	INIT_LIST_HEAD(&pf->fwlog_data_list);
+}
+
+/**
+ * ice_pf_fwlog_deinit - clear FW logging metadata on device exit
+ * @pf: pointer to the PF struct
+ */
+static void
+ice_pf_fwlog_deinit(struct ice_pf *pf)
+{
+	struct ice_fwlog_data *fwlog, *fwlog_tmp;
+	struct ice_hw *hw = &pf->hw;
+
+	/* make sure FW logging is disabled to not put the FW in a weird state
+	 * for the next driver load
+	 */
+	if (hw->fwlog_ena) {
+		int status;
+
+		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
+		status = ice_fwlog_set(hw, &hw->fwlog_cfg);
+		if (status)
+			dev_warn(ice_pf_to_dev(pf), "Unable to turn off FW logging, status: %d\n",
+				 status);
+
+		status = ice_fwlog_unregister(hw);
+		if (status)
+			dev_warn(ice_pf_to_dev(pf), "Unable to unregister FW logging, status: %d\n",
+				 status);
+	}
+
+	list_for_each_entry_safe(fwlog, fwlog_tmp, &pf->fwlog_data_list, list) {
+		kfree(fwlog->data);
+		kfree(fwlog);
+	}
+}
+
 /**
  * ice_probe - Device initialization routine
  * @pdev: PCI device information struct
@@ -4773,8 +4861,12 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 		goto err_exit_unroll;
 	}
 
+	ice_pf_fwlog_init(pf);
+
 	ice_init_feature_support(pf);
 
+	ice_debugfs_fwlog_init(pf);
+
 	err = ice_init_ddp_config(hw, pf);
 
 	/* during topology change ice_init_hw may fail */
@@ -5139,6 +5231,8 @@ static void ice_remove(struct pci_dev *pdev)
 		msleep(100);
 	}
 
+	ice_pf_fwlog_deinit(pf);
+
 	ice_tc_indir_block_remove(pf);
 
 	if (test_bit(ICE_FLAG_SRIOV_ENA, pf->flags)) {
@@ -5620,10 +5714,13 @@ static int __init ice_module_init(void)
 		return -ENOMEM;
 	}
 
+	ice_debugfs_init();
+
 	status = pci_register_driver(&ice_driver);
 	if (status) {
 		pr_err("failed to register PCI driver, err %d\n", status);
 		destroy_workqueue(ice_wq);
+		ice_debugfs_exit();
 	}
 
 	return status;
@@ -5640,6 +5737,7 @@ static void __exit ice_module_exit(void)
 {
 	pci_unregister_driver(&ice_driver);
 	destroy_workqueue(ice_wq);
+	ice_debugfs_exit();
 	pr_info("module unloaded\n");
 }
 module_exit(ice_module_exit);
-- 
2.35.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 3/5] ice: add ability to query/set FW log level and resolution
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 3/5] ice: add ability to query/set FW log level and resolution Paul M Stillwell Jr
@ 2022-11-29 11:48   ` Wilczynski, Michal
  2022-11-29 13:30     ` Alexander Lobakin
  2022-12-01  0:39   ` Tony Nguyen
  2022-12-01  7:36   ` Paul Menzel
  2 siblings, 1 reply; 21+ messages in thread
From: Wilczynski, Michal @ 2022-11-29 11:48 UTC (permalink / raw)
  To: Paul M Stillwell Jr, intel-wired-lan



On 11/28/2022 10:47 PM, Paul M Stillwell Jr wrote:
> The E8xx has the ability to change the FW log level and
> the granularity at which the logs get output. Enable
> the ability to see what the current values are and to
> change them.
>
> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> ---
>  .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   4 +
>  drivers/net/ethernet/intel/ice/ice_common.c   |   4 +-
>  drivers/net/ethernet/intel/ice/ice_devlink.c  | 142 ++++++++-
>  drivers/net/ethernet/intel/ice/ice_fwlog.c    | 277 ++++++++++++++++++
>  drivers/net/ethernet/intel/ice/ice_fwlog.h    |   5 +
>  drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
>  6 files changed, 429 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index d0026161a2b4..8fa18bc5d441 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -2406,7 +2406,11 @@ enum ice_adminq_opc {
>  
>  	/* Standalone Commands/Events */
>  	ice_aqc_opc_event_lan_overflow			= 0x1001,
> +	/* FW Logging Commands */
> +	ice_aqc_opc_fw_logs_config			= 0xFF30,
> +	ice_aqc_opc_fw_logs_register			= 0xFF31,
>  	ice_aqc_opc_fw_logs_query			= 0xFF32,
> +	ice_aqc_opc_fw_logs_event			= 0xFF33,
>  };
>  
>  #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 ecdc1ebb45d5..0b3adac13c66 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -879,7 +879,9 @@ int ice_init_hw(struct ice_hw *hw)
>  	if (status)
>  		goto err_unroll_cqinit;
>  
> -	ice_fwlog_set_support_ena(hw);
> +	status = ice_fwlog_init(hw);
> +	if (status)
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Error initializing FW logging: %d\n", status);
>  
>  	status = ice_clear_pf_cfg(hw);
>  	if (status)
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index 8843ff492f7f..ca67f2741f83 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -1488,6 +1488,8 @@ 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,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
>  };
>  
>  static int
> @@ -1530,8 +1532,121 @@ 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;
> +	struct ice_pf *pf = devlink_priv(devlink);
> +	struct ice_hw *hw = &pf->hw;
> +	int status;
> +
> +	/* only support fw log commands on PF 0 */
> +	if (hw->bus.func)
> +		return -EOPNOTSUPP;
> +
> +	if (hw->fwlog_ena == ctx->val.vbool)
> +		return 0;
> +
> +	hw->fwlog_ena = ctx->val.vbool;
> +
> +	if (hw->fwlog_ena)
> +		hw->fwlog_cfg.options |= ICE_FWLOG_OPTION_ARQ_ENA;
> +	else
> +		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
> +
> +	/* send the cfg to FW and register for events */
> +	status = ice_fwlog_set(hw, &hw->fwlog_cfg);
> +	if (status)
> +		return status;
> +
> +	if (hw->fwlog_ena) {
> +		status = ice_fwlog_register(hw);
> +		if (status)
> +			return status;
> +	} else {
> +		status = ice_fwlog_unregister(hw);
> +		if (status)
> +			return status;
> +	}

This could be simplified i.e

if (hw->fwlog_ena) {
	status = ice_fwlog_register(hw);
else 
        status = ice_fwlog_unregister(hw);

	return status;


> +
> +	return 0;
> +}
> +
> +static int
> +ice_devlink_fwlog_level_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;
> +
> +	/* all the log levels are the same so pick the first one */
> +	ctx->val.vu8 = pf->hw.fwlog_cfg.module_entries[0].log_level;
> +
> +	return 0;
> +}
> +
> +static int
> +ice_devlink_fwlog_level_set(struct devlink *devlink, u32 id,
> +			    struct devlink_param_gset_ctx *ctx)
> +{
> +	struct ice_pf *pf = devlink_priv(devlink);
> +	struct ice_fwlog_cfg *cfg;
> +	u8 i;
> +
> +	if (pf->hw.bus.func)
> +		return -EOPNOTSUPP;
> +
> +	if (ctx->val.vu8 >= ICE_FWLOG_LEVEL_INVALID) {
> +		dev_err(ice_pf_to_dev(pf), "Log level is greater than allowed! %d\n",
> +			ctx->val.vu8);
> +		return -EINVAL;
> +	}
> +
> +	cfg = &pf->hw.fwlog_cfg;
> +
> +	/* update the log level for all modules to the same thing. this gets
> +	 * written to the FW when the user says enable logging
> +	 */
> +	for (i = 0; i < ICE_AQC_FW_LOG_ID_MAX; i++)
> +		cfg->module_entries[i].log_level = ctx->val.vu8;
> +
> +	return 0;
> +}
> +
> +static int
> +ice_devlink_fwlog_resolution_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.vu8 = pf->hw.fwlog_cfg.log_resolution;
> +
> +	return 0;
> +}
> +
> +static int
> +ice_devlink_fwlog_resolution_set(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;
> +
> +	if (ctx->val.vu8 < ICE_AQC_FW_LOG_MIN_RESOLUTION ||
> +	    ctx->val.vu8 > ICE_AQC_FW_LOG_MAX_RESOLUTION) {
> +		dev_err(ice_pf_to_dev(pf), "Log resolution is not allowed! Should be between 1 - 128: %d\n",

This message might be a bit confusing, maybe something like 'Log resolution out of range" ?

> +			ctx->val.vu8);
> +		return -EINVAL;
> +	}
> +
> +	pf->hw.fwlog_cfg.log_resolution = ctx->val.vu8;
> +
> +	return 0;
>  }
>  
>  static const struct devlink_param ice_devlink_params[] = {
> @@ -1562,6 +1677,18 @@ static const struct devlink_param ice_devlink_params[] = {
>  			     ice_devlink_fwlog_enabled_get,
>  			     ice_devlink_fwlog_enabled_set,
>  			     NULL),
> +	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
> +			     "fwlog_level", DEVLINK_PARAM_TYPE_U8,
> +			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
> +			     ice_devlink_fwlog_level_get,
> +			     ice_devlink_fwlog_level_set,
> +			     NULL),
> +	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
> +			     "fwlog_resolution", DEVLINK_PARAM_TYPE_U8,
> +			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
> +			     ice_devlink_fwlog_resolution_get,
> +			     ice_devlink_fwlog_resolution_set,
> +			     NULL),
>  };
>  
>  static void ice_devlink_free(void *devlink_ptr)
> @@ -1662,11 +1789,20 @@ int ice_devlink_register_params(struct ice_pf *pf)
>  					   ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED,
>  					   value);
>  
> -	value.vbool = false;
>  	devlink_param_driverinit_value_set(devlink,
>  					   ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED,
>  					   value);
>  
> +	value.vu8 = ICE_FWLOG_LEVEL_NORMAL;
> +	devlink_param_driverinit_value_set(devlink,
> +					   ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
> +					   value);
> +
> +	/* set the default value for the FW to pack 10 messages per AQ event */
> +	value.vu8 = 10;
> +	devlink_param_driverinit_value_set(devlink,
> +					   ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
> +					   value);
>  	return 0;
>  }
>  
> diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.c b/drivers/net/ethernet/intel/ice/ice_fwlog.c
> index 67e670c62091..1174fd889307 100644
> --- a/drivers/net/ethernet/intel/ice/ice_fwlog.c
> +++ b/drivers/net/ethernet/intel/ice/ice_fwlog.c
> @@ -4,6 +4,165 @@
>  #include "ice_common.h"
>  #include "ice_fwlog.h"
>  
> +/**
> + * cache_cfg - Cache FW logging config
> + * @hw: pointer to the HW structure
> + * @cfg: config to cache
> + */
> +static void cache_cfg(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
> +{
> +	hw->fwlog_cfg = *cfg;
> +}
> +
> +/**
> + * valid_module_entries - validate all the module entry IDs and log levels
> + * @hw: pointer to the HW structure
> + * @entries: entries to validate
> + * @num_entries: number of entries to validate
> + */
> +static bool
> +valid_module_entries(struct ice_hw *hw, struct ice_fwlog_module_entry *entries,
> +		     u16 num_entries)
> +{
> +	u16 i;
> +
> +	if (!entries) {
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Null ice_fwlog_module_entry array\n");
> +		return false;
> +	}
> +
> +	if (!num_entries) {
> +		ice_debug(hw, ICE_DBG_FW_LOG, "num_entries must be non-zero\n");
> +		return false;
> +	}
> +
> +	for (i = 0; i < num_entries; i++) {
> +		struct ice_fwlog_module_entry *entry = &entries[i];
> +
> +		if (entry->module_id >= ICE_AQC_FW_LOG_ID_MAX) {
> +			ice_debug(hw, ICE_DBG_FW_LOG, "Invalid module_id %u, max valid module_id is %u\n",
> +				  entry->module_id, ICE_AQC_FW_LOG_ID_MAX - 1);
> +			return false;
> +		}
> +
> +		if (entry->log_level >= ICE_FWLOG_LEVEL_INVALID) {
> +			ice_debug(hw, ICE_DBG_FW_LOG, "Invalid log_level %u, max valid log_level is %u\n",
> +				  entry->log_level,
> +				  ICE_AQC_FW_LOG_ID_MAX - 1);
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +/**
> + * valid_cfg - validate entire configuration
> + * @hw: pointer to the HW structure
> + * @cfg: config to validate
> + */
> +static bool valid_cfg(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
> +{
> +	if (!cfg) {
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Null ice_fwlog_cfg\n");
> +		return false;
> +	}
> +
> +	if (cfg->log_resolution < ICE_AQC_FW_LOG_MIN_RESOLUTION ||
> +	    cfg->log_resolution > ICE_AQC_FW_LOG_MAX_RESOLUTION) {
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Unsupported log_resolution %u, must be between %u and %u\n",
> +			  cfg->log_resolution, ICE_AQC_FW_LOG_MIN_RESOLUTION,
> +			  ICE_AQC_FW_LOG_MAX_RESOLUTION);
> +		return false;
> +	}
> +
> +	if (!valid_module_entries(hw, cfg->module_entries,
> +				  ICE_AQC_FW_LOG_ID_MAX))
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * ice_fwlog_init - Initialize FW logging variables
> + * @hw: pointer to the HW structure
> + *
> + * This function should be called on driver initialization during
> + * ice_init_hw().
> + */
> +int ice_fwlog_init(struct ice_hw *hw)
> +{
> +	int status;
> +
> +	ice_fwlog_set_support_ena(hw);
> +
> +	if (ice_fwlog_supported(hw)) {
> +		struct ice_fwlog_cfg *cfg;
> +
> +		cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
> +		if (!cfg)
> +			return -ENOMEM;
> +
> +		/* read the current config from the FW and store it */
> +		status = ice_fwlog_get(hw, cfg);
> +		if (status)
> +			return status;

Maybe initialize status with zero, and just return status at the end ?

> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * ice_aq_fwlog_set - Set FW logging configuration AQ command (0xFF30)
> + * @hw: pointer to the HW structure
> + * @entries: entries to configure
> + * @num_entries: number of @entries
> + * @options: options from ice_fwlog_cfg->options structure
> + * @log_resolution: logging resolution
> + */
> +static int
> +ice_aq_fwlog_set(struct ice_hw *hw, struct ice_fwlog_module_entry *entries,
> +		 u16 num_entries, u16 options, u16 log_resolution)
> +{
> +	struct ice_aqc_fw_log_cfg_resp *fw_modules;
> +	struct ice_aqc_fw_log *cmd;
> +	struct ice_aq_desc desc;
> +	int status;
> +	u16 i;
> +
> +	fw_modules = kcalloc(num_entries, sizeof(*fw_modules), GFP_KERNEL);
> +	if (!fw_modules)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_entries; i++) {
> +		fw_modules[i].module_identifier =
> +			cpu_to_le16(entries[i].module_id);
> +		fw_modules[i].log_level = entries[i].log_level;
> +	}
> +
> +	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_fw_logs_config);
> +	desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> +
> +	cmd = &desc.params.fw_log;
> +
> +	cmd->cmd_flags = ICE_AQC_FW_LOG_CONF_SET_VALID;
> +	cmd->ops.cfg.log_resolution = cpu_to_le16(log_resolution);
> +	cmd->ops.cfg.mdl_cnt = cpu_to_le16(num_entries);
> +
> +	if (options & ICE_FWLOG_OPTION_ARQ_ENA)
> +		cmd->cmd_flags |= ICE_AQC_FW_LOG_CONF_AQ_EN;
> +	if (options & ICE_FWLOG_OPTION_UART_ENA)
> +		cmd->cmd_flags |= ICE_AQC_FW_LOG_CONF_UART_EN;
> +
> +	status = ice_aq_send_cmd(hw, &desc, fw_modules,
> +				 sizeof(*fw_modules) * num_entries,
> +				 NULL);
> +
> +	kfree(fw_modules);
> +
> +	return status;
> +}
> +
>  /**
>   * ice_fwlog_supported - Cached for whether FW supports FW logging or not
>   * @hw: pointer to the HW structure
> @@ -16,6 +175,99 @@ bool ice_fwlog_supported(struct ice_hw *hw)
>  	return hw->fwlog_support_ena;
>  }
>  
> +/**
> + * ice_fwlog_set - Set the firmware logging settings
> + * @hw: pointer to the HW structure
> + * @cfg: config used to set firmware logging
> + *
> + * This function should be called whenever the driver needs to set the firmware
> + * logging configuration. It can be called on initialization, reset, or during
> + * runtime.
> + *
> + * If the PF wishes to receive FW logging then it must register via
> + * ice_fwlog_register. Note, that ice_fwlog_register does not need to be called
> + * for init.
> + */
> +int
> +ice_fwlog_set(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
> +{
> +	int status;
> +
> +	if (!ice_fwlog_supported(hw))
> +		return -EOPNOTSUPP;
> +
> +	if (!valid_cfg(hw, cfg))
> +		return -EINVAL;
> +
> +	status = ice_aq_fwlog_set(hw, cfg->module_entries,
> +				  ICE_AQC_FW_LOG_ID_MAX, cfg->options,
> +				  cfg->log_resolution);
> +	if (!status)
> +		cache_cfg(hw, cfg);
> +
> +	return status;
> +}
> +
> +/**
> + * ice_aq_fwlog_register - Register PF for firmware logging events (0xFF31)
> + * @hw: pointer to the HW structure
> + * @reg: true to register and false to unregister
> + */
> +static int ice_aq_fwlog_register(struct ice_hw *hw, bool reg)
> +{
> +	struct ice_aq_desc desc;
> +
> +	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_fw_logs_register);
> +
> +	if (reg)
> +		desc.params.fw_log.cmd_flags = ICE_AQC_FW_LOG_AQ_REGISTER;
> +
> +	return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
> +}
> +
> +/**
> + * ice_fwlog_register - Register the PF for firmware logging
> + * @hw: pointer to the HW structure
> + *
> + * After this call the PF will start to receive firmware logging based on the
> + * configuration set in ice_fwlog_set.
> + */
> +int ice_fwlog_register(struct ice_hw *hw)
> +{
> +	int status;
> +
> +	if (!ice_fwlog_supported(hw))
> +		return -EOPNOTSUPP;
> +
> +	status = ice_aq_fwlog_register(hw, true);
> +	if (status)
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Failed to register for firmware logging events over ARQ\n");
> +	else
> +		hw->fwlog_cfg.options |= ICE_FWLOG_OPTION_IS_REGISTERED;
> +
> +	return status;
> +}
> +
> +/**
> + * ice_fwlog_unregister - Unregister the PF from firmware logging
> + * @hw: pointer to the HW structure
> + */
> +int ice_fwlog_unregister(struct ice_hw *hw)
> +{
> +	int status;
> +
> +	if (!ice_fwlog_supported(hw))
> +		return -EOPNOTSUPP;
> +
> +	status = ice_aq_fwlog_register(hw, false);
> +	if (status)
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Failed to unregister from firmware logging events over ARQ\n");
> +	else
> +		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_IS_REGISTERED;
> +
> +	return status;
> +}
> +
>  /**
>   * ice_aq_fwlog_get - Get the current firmware logging configuration (0xFF32)
>   * @hw: pointer to the HW structure
> @@ -115,3 +367,28 @@ void ice_fwlog_set_support_ena(struct ice_hw *hw)
>  
>  	kfree(cfg);
>  }
> +
> +/**
> + * ice_fwlog_get - Get the firmware logging settings
> + * @hw: pointer to the HW structure
> + * @cfg: config to populate based on current firmware logging settings
> + */
> +int
> +ice_fwlog_get(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
> +{
> +	int status;
> +
> +	if (!ice_fwlog_supported(hw))
> +		return -EOPNOTSUPP;
> +
> +	if (!cfg)
> +		return -EINVAL;
> +
> +	status = ice_aq_fwlog_get(hw, cfg);
> +	if (status)
> +		return status;
> +
> +	cache_cfg(hw, cfg);
> +
> +	return 0;
> +}
> diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.h b/drivers/net/ethernet/intel/ice/ice_fwlog.h
> index d7371253b8e5..66648c5ba92c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_fwlog.h
> +++ b/drivers/net/ethernet/intel/ice/ice_fwlog.h
> @@ -48,4 +48,9 @@ struct ice_fwlog_cfg {
>  
>  void ice_fwlog_set_support_ena(struct ice_hw *hw);
>  bool ice_fwlog_supported(struct ice_hw *hw);
> +int ice_fwlog_init(struct ice_hw *hw);
> +int ice_fwlog_set(struct ice_hw *hw, struct ice_fwlog_cfg *cfg);
> +int ice_fwlog_get(struct ice_hw *hw, struct ice_fwlog_cfg *cfg);
> +int ice_fwlog_register(struct ice_hw *hw);
> +int ice_fwlog_unregister(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 10b78faf0a32..c524179e79f0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_type.h
> +++ b/drivers/net/ethernet/intel/ice/ice_type.h
> @@ -861,6 +861,7 @@ struct ice_hw {
>  	u8 fw_patch;		/* firmware patch version */
>  	u32 fw_build;		/* firmware build number */
>  
> +	struct ice_fwlog_cfg fwlog_cfg;
>  	bool fwlog_support_ena; /* does hardware support FW logging? */
>  	bool fwlog_ena; /* currently logging? */
>  

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 4/5] ice: disable FW logging on driver unload
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 4/5] ice: disable FW logging on driver unload Paul M Stillwell Jr
@ 2022-11-29 12:05   ` Wilczynski, Michal
  2022-11-29 21:31     ` Paul M Stillwell Jr
  2022-12-01  0:40   ` Tony Nguyen
  1 sibling, 1 reply; 21+ messages in thread
From: Wilczynski, Michal @ 2022-11-29 12:05 UTC (permalink / raw)
  To: Paul M Stillwell Jr, intel-wired-lan



On 11/28/2022 10:47 PM, Paul M Stillwell Jr wrote:
> The FW is running in it's own context irregardless of what the driver
> is doing. In this case, if the driver previously registered for FW
> log events and then the driver unloads without informing the FW to
> unregister for FW log events then the FW still has a timer running to
> output FW logs.
>
> The next time the driver loads and tries to register for FW log events
> then the FW returns an error, but still enables the continued
> outputting of FW logs. This causes an IO error to devlink which isn't
> intuitive since the logs are still being output.
>
> Fix this by disabling FW logging when the driver is being unloaded.
>
> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_devlink.c | 32 +++++++++++++++-----
>  1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index ca67f2741f83..923556e6ae79 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -378,6 +378,10 @@ static int ice_devlink_info_get(struct devlink *devlink,
>  enum ice_devlink_param_id {
>  	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
>  	ICE_DEVLINK_PARAM_ID_TX_BALANCE,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,

My understanding was the the community doesn't like private devlink params.
Was some investigation done whether the FW logging can be done using some
generic API ? I believe we had a discussion about this a long time ago. Maybe
I'm a bit out of a loop at this point :)

>  };
>  
>  /**
> @@ -1484,14 +1488,6 @@ 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,
> -	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
> -	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
> -};
> -
>  static int
>  ice_devlink_fwlog_supported_get(struct devlink *devlink, u32 id,
>  				struct devlink_param_gset_ctx *ctx)
> @@ -1743,6 +1739,26 @@ void ice_devlink_register(struct ice_pf *pf)
>   */
>  void ice_devlink_unregister(struct ice_pf *pf)
>  {
> +	struct ice_hw *hw = &pf->hw;
> +
> +	/* make sure FW logging is disabled to not put the FW in a weird state
> +	 * for the next driver load
> +	 */
> +	if (hw->fwlog_ena) {
> +		int status;
> +
> +		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
> +		status = ice_fwlog_set(hw, &hw->fwlog_cfg);
> +		if (status)
> +			dev_warn(ice_pf_to_dev(pf), "Unable to turn off FW logging, status: %d\n",
> +				 status);
> +
> +		status = ice_fwlog_unregister(hw);
> +		if (status)
> +			dev_warn(ice_pf_to_dev(pf), "Unable to unregister FW logging, status: %d\n",
> +				 status);
> +	}
> +
>  	devlink_unregister(priv_to_devlink(pf));
>  }
>  

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 3/5] ice: add ability to query/set FW log level and resolution
  2022-11-29 11:48   ` Wilczynski, Michal
@ 2022-11-29 13:30     ` Alexander Lobakin
  2022-11-29 21:31       ` Paul M Stillwell Jr
  0 siblings, 1 reply; 21+ messages in thread
From: Alexander Lobakin @ 2022-11-29 13:30 UTC (permalink / raw)
  To: Wilczynski, Michal; +Cc: intel-wired-lan

From: "Wilczynski, Michal" <michal.wilczynski@intel.com>
Date: Tue, 29 Nov 2022 12:48:25 +0100

> On 11/28/2022 10:47 PM, Paul M Stillwell Jr wrote:
> > The E8xx has the ability to change the FW log level and
> > the granularity at which the logs get output. Enable
> > the ability to see what the current values are and to
> > change them.
> >
> > Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>

[...]

> > +	/* send the cfg to FW and register for events */
> > +	status = ice_fwlog_set(hw, &hw->fwlog_cfg);
> > +	if (status)
> > +		return status;
> > +
> > +	if (hw->fwlog_ena) {
> > +		status = ice_fwlog_register(hw);
> > +		if (status)
> > +			return status;
> > +	} else {
> > +		status = ice_fwlog_unregister(hw);
> > +		if (status)
> > +			return status;
> > +	}
> 
> This could be simplified i.e
> 
> if (hw->fwlog_ena) {
> 	status = ice_fwlog_register(hw);
> else 
>         status = ice_fwlog_unregister(hw);
> 
> 	return status;

	return hw->fwlog_ena ? ice_fwlog_register(hw) : ice_fwlog_unregister(hw);
> 
> 
> > +
> > +	return 0;
> > +}

[...]

Thanks,
Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 5/5] ice: use debugfs to output FW log data
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 5/5] ice: use debugfs to output FW log data Paul M Stillwell Jr
@ 2022-11-29 13:53   ` Paul Menzel
  2022-11-29 21:28     ` Paul M Stillwell Jr
  2022-12-01  0:40   ` Tony Nguyen
  1 sibling, 1 reply; 21+ messages in thread
From: Paul Menzel @ 2022-11-29 13:53 UTC (permalink / raw)
  To: Paul M Stillwell Jr; +Cc: intel-wired-lan

Dear Paul,


Am 28.11.22 um 22:47 schrieb Paul M Stillwell Jr:
> The FW log data can be quite large so we don't want to use syslog.
> Instead take advantage of debugfs to write the data to.
> 
> Also, a previous patch addded codde to disable FW logging when

added code

> the driver was unloaded. The code was added to
> ice_devlink_unregister(), which didn't seem correct so move the
> disabling of FW logging on driver unload to a better place.

It’d be great if you added examples, how to get the log from debugfs, 
and decode(?) it.


Kind regards,

Paul


> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/Makefile       |   3 +-
>   drivers/net/ethernet/intel/ice/ice.h          |  24 ++++
>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   2 +
>   drivers/net/ethernet/intel/ice/ice_debugfs.c  | 109 ++++++++++++++++++
>   drivers/net/ethernet/intel/ice/ice_devlink.c  |  20 ----
>   drivers/net/ethernet/intel/ice/ice_main.c     |  98 ++++++++++++++++
>   6 files changed, 235 insertions(+), 21 deletions(-)
>   create mode 100644 drivers/net/ethernet/intel/ice/ice_debugfs.c

[…]
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 5/5] ice: use debugfs to output FW log data
  2022-11-29 13:53   ` Paul Menzel
@ 2022-11-29 21:28     ` Paul M Stillwell Jr
  0 siblings, 0 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2022-11-29 21:28 UTC (permalink / raw)
  To: Paul Menzel; +Cc: intel-wired-lan

On 11/29/2022 5:53 AM, Paul Menzel wrote:
> Dear Paul,
> 
> 
> Am 28.11.22 um 22:47 schrieb Paul M Stillwell Jr:
>> The FW log data can be quite large so we don't want to use syslog.
>> Instead take advantage of debugfs to write the data to.
>>
>> Also, a previous patch addded codde to disable FW logging when
> 
> added code
> 

Good catch, will fix.

>> the driver was unloaded. The code was added to
>> ice_devlink_unregister(), which didn't seem correct so move the
>> disabling of FW logging on driver unload to a better place.
> 
> It’d be great if you added examples, how to get the log from debugfs, 
> and decode(?) it.
> 

Will do

> 
> Kind regards,
> 
> Paul
> 
> 
>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/Makefile       |   3 +-
>>   drivers/net/ethernet/intel/ice/ice.h          |  24 ++++
>>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   2 +
>>   drivers/net/ethernet/intel/ice/ice_debugfs.c  | 109 ++++++++++++++++++
>>   drivers/net/ethernet/intel/ice/ice_devlink.c  |  20 ----
>>   drivers/net/ethernet/intel/ice/ice_main.c     |  98 ++++++++++++++++
>>   6 files changed, 235 insertions(+), 21 deletions(-)
>>   create mode 100644 drivers/net/ethernet/intel/ice/ice_debugfs.c
> 
> […]

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 4/5] ice: disable FW logging on driver unload
  2022-11-29 12:05   ` Wilczynski, Michal
@ 2022-11-29 21:31     ` Paul M Stillwell Jr
  0 siblings, 0 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2022-11-29 21:31 UTC (permalink / raw)
  To: Wilczynski, Michal, intel-wired-lan

On 11/29/2022 4:05 AM, Wilczynski, Michal wrote:
> 
> 
> On 11/28/2022 10:47 PM, Paul M Stillwell Jr wrote:
>> The FW is running in it's own context irregardless of what the driver
>> is doing. In this case, if the driver previously registered for FW
>> log events and then the driver unloads without informing the FW to
>> unregister for FW log events then the FW still has a timer running to
>> output FW logs.
>>
>> The next time the driver loads and tries to register for FW log events
>> then the FW returns an error, but still enables the continued
>> outputting of FW logs. This causes an IO error to devlink which isn't
>> intuitive since the logs are still being output.
>>
>> Fix this by disabling FW logging when the driver is being unloaded.
>>
>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_devlink.c | 32 +++++++++++++++-----
>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> index ca67f2741f83..923556e6ae79 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> @@ -378,6 +378,10 @@ static int ice_devlink_info_get(struct devlink *devlink,
>>   enum ice_devlink_param_id {
>>   	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
>>   	ICE_DEVLINK_PARAM_ID_TX_BALANCE,
>> +	ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED,
>> +	ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED,
>> +	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
>> +	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
> 
> My understanding was the the community doesn't like private devlink params.
> Was some investigation done whether the FW logging can be done using some
> generic API ? I believe we had a discussion about this a long time ago. Maybe
> I'm a bit out of a loop at this point :)
> 

Yeah, I looked at all the other devlink interfaces and couldn't find one 
that really worked. I considered adding a brand new interface, but 
didn't want to go that route unless we have to because I'm guessing that 
every device that can get FW logs has a slightly different way to do it.

If you have a suggestion then I'm all ears :).

>>   };
>>   
>>   /**
>> @@ -1484,14 +1488,6 @@ 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,
>> -	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
>> -	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
>> -};
>> -
>>   static int
>>   ice_devlink_fwlog_supported_get(struct devlink *devlink, u32 id,
>>   				struct devlink_param_gset_ctx *ctx)
>> @@ -1743,6 +1739,26 @@ void ice_devlink_register(struct ice_pf *pf)
>>    */
>>   void ice_devlink_unregister(struct ice_pf *pf)
>>   {
>> +	struct ice_hw *hw = &pf->hw;
>> +
>> +	/* make sure FW logging is disabled to not put the FW in a weird state
>> +	 * for the next driver load
>> +	 */
>> +	if (hw->fwlog_ena) {
>> +		int status;
>> +
>> +		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
>> +		status = ice_fwlog_set(hw, &hw->fwlog_cfg);
>> +		if (status)
>> +			dev_warn(ice_pf_to_dev(pf), "Unable to turn off FW logging, status: %d\n",
>> +				 status);
>> +
>> +		status = ice_fwlog_unregister(hw);
>> +		if (status)
>> +			dev_warn(ice_pf_to_dev(pf), "Unable to unregister FW logging, status: %d\n",
>> +				 status);
>> +	}
>> +
>>   	devlink_unregister(priv_to_devlink(pf));
>>   }
>>   
> 

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 3/5] ice: add ability to query/set FW log level and resolution
  2022-11-29 13:30     ` Alexander Lobakin
@ 2022-11-29 21:31       ` Paul M Stillwell Jr
  0 siblings, 0 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2022-11-29 21:31 UTC (permalink / raw)
  To: Alexander Lobakin, Wilczynski, Michal; +Cc: intel-wired-lan

On 11/29/2022 5:30 AM, Alexander Lobakin wrote:
> From: "Wilczynski, Michal" <michal.wilczynski@intel.com>
> Date: Tue, 29 Nov 2022 12:48:25 +0100
> 
>> On 11/28/2022 10:47 PM, Paul M Stillwell Jr wrote:
>>> The E8xx has the ability to change the FW log level and
>>> the granularity at which the logs get output. Enable
>>> the ability to see what the current values are and to
>>> change them.
>>>
>>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> 
> [...]
> 
>>> +	/* send the cfg to FW and register for events */
>>> +	status = ice_fwlog_set(hw, &hw->fwlog_cfg);
>>> +	if (status)
>>> +		return status;
>>> +
>>> +	if (hw->fwlog_ena) {
>>> +		status = ice_fwlog_register(hw);
>>> +		if (status)
>>> +			return status;
>>> +	} else {
>>> +		status = ice_fwlog_unregister(hw);
>>> +		if (status)
>>> +			return status;
>>> +	}
>>
>> This could be simplified i.e
>>
>> if (hw->fwlog_ena) {
>> 	status = ice_fwlog_register(hw);
>> else
>>          status = ice_fwlog_unregister(hw);
>>
>> 	return status;
> 
> 	return hw->fwlog_ena ? ice_fwlog_register(hw) : ice_fwlog_unregister(hw);

Will change.

>>
>>
>>> +
>>> +	return 0;
>>> +}
> 
> [...]
> 
> Thanks,
> Olek

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 0/5] add v2 FW logging for ice driver
  2022-11-28 21:47 [Intel-wired-lan] [PATCH net-next 0/5] add v2 FW logging for ice driver Paul M Stillwell Jr
                   ` (4 preceding siblings ...)
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 5/5] ice: use debugfs to output FW log data Paul M Stillwell Jr
@ 2022-12-01  0:39 ` Tony Nguyen
  5 siblings, 0 replies; 21+ messages in thread
From: Tony Nguyen @ 2022-12-01  0:39 UTC (permalink / raw)
  To: Paul M Stillwell Jr, intel-wired-lan

On 11/28/2022 1:47 PM, Paul M Stillwell Jr wrote:
> FW log support was added to the ice driver, but that version is no
> longer supported. There is a newer version of FW logging (v2) that
> adds more controls knobs to get the exact data out of the FW
> for debugging.
> 
> Additionally, instead of dumping the FW log output to syslog,
> dump it to debugfs. The FW log data is really just binary
> data that the FW log team decodes to determine what happens so the
> translation from binary to some text output just slows things down
> and results in potential dropped data. The structure for the debugfs
> entry is: /sys/kernel/debug/ice/<pci device>/fwlog
> 
> Once enabled the FW log data is received as ARQ events that the driver
> processes.
> 
> The FW logging is across all the PFs on the device, so restrict the
> commands to only PF0.
> 
> The following new device parameters are added:
> - fwlog_supported (read-only): does the FW support FW logging
> - fwlog_enabled (read/write): is FW logging currently running
> - fwlog_level (read/write): the log level enabled, valid values are
>      Each level includes the messages from the previous/lower level
> 	0 - no logging
> 	1 - error logging
> 	2 - warning logging
> 	3 - normal logging
> 	4 - verbose logging
> - fwlog_resolution (read/write): the number of log messages to included
>    in a single ARQ event. The range is 1-128 (1 means push every log
>    message, 128 means push only when the max AQ command buffer is full).
>    The suggested value is 10.
> 
> This patch series adds the following set of devlink commands:
> 
> devlink dev param set <pci dev> name fwlog_enabled value <true/false> cmode runtime
> devlink dev param set <pci dev> name fwlog_level value <0-4> cmode runtime
> devlink dev param set <pci dev> name fwlog_resolution value <1-128> cmode runtime
> 
> Paul M Stillwell Jr (5):
>    ice: remove FW logging code
>    ice: enable devlink to check FW logging status
>    ice: add ability to query/set FW log level and resolution
>    ice: disable FW logging on driver unload
>    ice: use debugfs to output FW log data
> 
>   drivers/net/ethernet/intel/ice/Makefile       |   4 +-
>   drivers/net/ethernet/intel/ice/ice.h          |  24 ++
>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   | 164 ++++----
>   drivers/net/ethernet/intel/ice/ice_common.c   | 218 +---------
>   drivers/net/ethernet/intel/ice/ice_debugfs.c  | 109 +++++
>   drivers/net/ethernet/intel/ice/ice_devlink.c  | 205 ++++++++-
>   drivers/net/ethernet/intel/ice/ice_fwlog.c    | 394 ++++++++++++++++++
>   drivers/net/ethernet/intel/ice/ice_fwlog.h    |  56 +++
>   drivers/net/ethernet/intel/ice/ice_main.c     |  99 ++++-
>   drivers/net/ethernet/intel/ice/ice_type.h     |  23 +-

Please add Documentation for this

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 2/5] ice: enable devlink to check FW logging status
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 2/5] ice: enable devlink to check FW logging status Paul M Stillwell Jr
@ 2022-12-01  0:39   ` Tony Nguyen
  0 siblings, 0 replies; 21+ messages in thread
From: Tony Nguyen @ 2022-12-01  0:39 UTC (permalink / raw)
  To: Paul M Stillwell Jr, intel-wired-lan

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 3/5] ice: add ability to query/set FW log level and resolution
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 3/5] ice: add ability to query/set FW log level and resolution Paul M Stillwell Jr
  2022-11-29 11:48   ` Wilczynski, Michal
@ 2022-12-01  0:39   ` Tony Nguyen
  2022-12-09  0:00     ` Paul M Stillwell Jr
  2022-12-01  7:36   ` Paul Menzel
  2 siblings, 1 reply; 21+ messages in thread
From: Tony Nguyen @ 2022-12-01  0:39 UTC (permalink / raw)
  To: Paul M Stillwell Jr, intel-wired-lan

On 11/28/2022 1:47 PM, Paul M Stillwell Jr wrote:
> The E8xx has the ability to change the FW log level and
> the granularity at which the logs get output. Enable
> the ability to see what the current values are and to
> change them.
> 
> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> ---
>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   4 +
>   drivers/net/ethernet/intel/ice/ice_common.c   |   4 +-
>   drivers/net/ethernet/intel/ice/ice_devlink.c  | 142 ++++++++-
>   drivers/net/ethernet/intel/ice/ice_fwlog.c    | 277 ++++++++++++++++++
>   drivers/net/ethernet/intel/ice/ice_fwlog.h    |   5 +
>   drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
>   6 files changed, 429 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index d0026161a2b4..8fa18bc5d441 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -2406,7 +2406,11 @@ enum ice_adminq_opc {
>   
>   	/* Standalone Commands/Events */
>   	ice_aqc_opc_event_lan_overflow			= 0x1001,
> +	/* FW Logging Commands */

nit: As this seperator was removed from patch 1 and the first command in 
patch 2. This comment seems to fit better there. Also retaining the new 
line between the op and the FW logging group would be nice.

> +	ice_aqc_opc_fw_logs_config			= 0xFF30,
> +	ice_aqc_opc_fw_logs_register			= 0xFF31,
>   	ice_aqc_opc_fw_logs_query			= 0xFF32,
> +	ice_aqc_opc_fw_logs_event			= 0xFF33,
>   };
>   
>   #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 ecdc1ebb45d5..0b3adac13c66 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -879,7 +879,9 @@ int ice_init_hw(struct ice_hw *hw)
>   	if (status)
>   		goto err_unroll_cqinit;
>   
> -	ice_fwlog_set_support_ena(hw);
> +	status = ice_fwlog_init(hw);
> +	if (status)
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Error initializing FW logging: %d\n", status);
>   
>   	status = ice_clear_pf_cfg(hw);
>   	if (status)
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index 8843ff492f7f..ca67f2741f83 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -1488,6 +1488,8 @@ 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,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
>   };
>   
>   static int
> @@ -1530,8 +1532,121 @@ 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;
> +	struct ice_pf *pf = devlink_priv(devlink);
> +	struct ice_hw *hw = &pf->hw;
> +	int status;
> +
> +	/* only support fw log commands on PF 0 */
> +	if (hw->bus.func)
> +		return -EOPNOTSUPP;
> +
> +	if (hw->fwlog_ena == ctx->val.vbool)
> +		return 0;
> +
> +	hw->fwlog_ena = ctx->val.vbool;
> +
> +	if (hw->fwlog_ena)
> +		hw->fwlog_cfg.options |= ICE_FWLOG_OPTION_ARQ_ENA;
> +	else
> +		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
> +
> +	/* send the cfg to FW and register for events */
> +	status = ice_fwlog_set(hw, &hw->fwlog_cfg);
> +	if (status)
> +		return status;
> +
> +	if (hw->fwlog_ena) {
> +		status = ice_fwlog_register(hw);
> +		if (status)
> +			return status;
> +	} else {
> +		status = ice_fwlog_unregister(hw);
> +		if (status)
> +			return status;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +ice_devlink_fwlog_level_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;
> +
> +	/* all the log levels are the same so pick the first one */
> +	ctx->val.vu8 = pf->hw.fwlog_cfg.module_entries[0].log_level;

Since they are all the same, would it make sense to store it in the 
config and pull the value from there instead of duplicating it for each 
entry?

> +
> +	return 0;
> +}
> +
> +static int
> +ice_devlink_fwlog_level_set(struct devlink *devlink, u32 id,
> +			    struct devlink_param_gset_ctx *ctx)
> +{
> +	struct ice_pf *pf = devlink_priv(devlink);
> +	struct ice_fwlog_cfg *cfg;
> +	u8 i;
> +
> +	if (pf->hw.bus.func)
> +		return -EOPNOTSUPP;
> +
> +	if (ctx->val.vu8 >= ICE_FWLOG_LEVEL_INVALID) {
> +		dev_err(ice_pf_to_dev(pf), "Log level is greater than allowed! %d\n",
> +			ctx->val.vu8);
> +		return -EINVAL;
> +	}
> +
> +	cfg = &pf->hw.fwlog_cfg;
> +
> +	/* update the log level for all modules to the same thing. this gets
> +	 * written to the FW when the user says enable logging
> +	 */
> +	for (i = 0; i < ICE_AQC_FW_LOG_ID_MAX; i++)
> +		cfg->module_entries[i].log_level = ctx->val.vu8;
> +
> +	return 0;
> +}
> +
> +static int
> +ice_devlink_fwlog_resolution_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.vu8 = pf->hw.fwlog_cfg.log_resolution;
> +
> +	return 0;
> +}
> +
> +static int
> +ice_devlink_fwlog_resolution_set(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;
> +
> +	if (ctx->val.vu8 < ICE_AQC_FW_LOG_MIN_RESOLUTION ||
> +	    ctx->val.vu8 > ICE_AQC_FW_LOG_MAX_RESOLUTION) {
> +		dev_err(ice_pf_to_dev(pf), "Log resolution is not allowed! Should be between 1 - 128: %d\n",
> +			ctx->val.vu8);
> +		return -EINVAL;
> +	}
> +
> +	pf->hw.fwlog_cfg.log_resolution = ctx->val.vu8;
> +
> +	return 0;
>   }
>   
>   static const struct devlink_param ice_devlink_params[] = {
> @@ -1562,6 +1677,18 @@ static const struct devlink_param ice_devlink_params[] = {
>   			     ice_devlink_fwlog_enabled_get,
>   			     ice_devlink_fwlog_enabled_set,
>   			     NULL),
> +	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
> +			     "fwlog_level", DEVLINK_PARAM_TYPE_U8,
> +			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
> +			     ice_devlink_fwlog_level_get,
> +			     ice_devlink_fwlog_level_set,
> +			     NULL),
> +	DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
> +			     "fwlog_resolution", DEVLINK_PARAM_TYPE_U8,
> +			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
> +			     ice_devlink_fwlog_resolution_get,
> +			     ice_devlink_fwlog_resolution_set,
> +			     NULL),
>   };
>   
>   static void ice_devlink_free(void *devlink_ptr)
> @@ -1662,11 +1789,20 @@ int ice_devlink_register_params(struct ice_pf *pf)
>   					   ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED,
>   					   value);
>   
> -	value.vbool = false;

Is this on purpose?

>   	devlink_param_driverinit_value_set(devlink,
>   					   ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED,
>   					   value);
>   
> +	value.vu8 = ICE_FWLOG_LEVEL_NORMAL;
> +	devlink_param_driverinit_value_set(devlink,
> +					   ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
> +					   value);
> +
> +	/* set the default value for the FW to pack 10 messages per AQ event */
> +	value.vu8 = 10;
> +	devlink_param_driverinit_value_set(devlink,
> +					   ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
> +					   value);
>   	return 0;
>   }
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.c b/drivers/net/ethernet/intel/ice/ice_fwlog.c
> index 67e670c62091..1174fd889307 100644
> --- a/drivers/net/ethernet/intel/ice/ice_fwlog.c
> +++ b/drivers/net/ethernet/intel/ice/ice_fwlog.c
> @@ -4,6 +4,165 @@
>   #include "ice_common.h"
>   #include "ice_fwlog.h"
>   
> +/**
> + * cache_cfg - Cache FW logging config
> + * @hw: pointer to the HW structure
> + * @cfg: config to cache
> + */
> +static void cache_cfg(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
> +{
> +	hw->fwlog_cfg = *cfg;
> +}
> +
> +/**
> + * valid_module_entries - validate all the module entry IDs and log levels
> + * @hw: pointer to the HW structure
> + * @entries: entries to validate
> + * @num_entries: number of entries to validate
> + */
> +static bool
> +valid_module_entries(struct ice_hw *hw, struct ice_fwlog_module_entry *entries,
> +		     u16 num_entries)
> +{
> +	u16 i;
> +
> +	if (!entries) {
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Null ice_fwlog_module_entry array\n");
> +		return false;
> +	}
> +
> +	if (!num_entries) {
> +		ice_debug(hw, ICE_DBG_FW_LOG, "num_entries must be non-zero\n");
> +		return false;
> +	}
> +
> +	for (i = 0; i < num_entries; i++) {
> +		struct ice_fwlog_module_entry *entry = &entries[i];
> +
> +		if (entry->module_id >= ICE_AQC_FW_LOG_ID_MAX) {
> +			ice_debug(hw, ICE_DBG_FW_LOG, "Invalid module_id %u, max valid module_id is %u\n",
> +				  entry->module_id, ICE_AQC_FW_LOG_ID_MAX - 1);
> +			return false;
> +		}
> +
> +		if (entry->log_level >= ICE_FWLOG_LEVEL_INVALID) {
> +			ice_debug(hw, ICE_DBG_FW_LOG, "Invalid log_level %u, max valid log_level is %u\n",
> +				  entry->log_level,
> +				  ICE_AQC_FW_LOG_ID_MAX - 1);
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +/**
> + * valid_cfg - validate entire configuration
> + * @hw: pointer to the HW structure
> + * @cfg: config to validate
> + */
> +static bool valid_cfg(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
> +{
> +	if (!cfg) {
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Null ice_fwlog_cfg\n");
> +		return false;
> +	}
> +
> +	if (cfg->log_resolution < ICE_AQC_FW_LOG_MIN_RESOLUTION ||
> +	    cfg->log_resolution > ICE_AQC_FW_LOG_MAX_RESOLUTION) {
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Unsupported log_resolution %u, must be between %u and %u\n",
> +			  cfg->log_resolution, ICE_AQC_FW_LOG_MIN_RESOLUTION,
> +			  ICE_AQC_FW_LOG_MAX_RESOLUTION);
> +		return false;
> +	}
> +
> +	if (!valid_module_entries(hw, cfg->module_entries,
> +				  ICE_AQC_FW_LOG_ID_MAX))
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * ice_fwlog_init - Initialize FW logging variables
> + * @hw: pointer to the HW structure
> + *
> + * This function should be called on driver initialization during
> + * ice_init_hw().
> + */
> +int ice_fwlog_init(struct ice_hw *hw)
> +{
> +	int status;
> +
> +	ice_fwlog_set_support_ena(hw);
> +
> +	if (ice_fwlog_supported(hw)) {
> +		struct ice_fwlog_cfg *cfg;
> +
> +		cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
> +		if (!cfg)
> +			return -ENOMEM;
> +
> +		/* read the current config from the FW and store it */
> +		status = ice_fwlog_get(hw, cfg);
> +		if (status)
> +			return status;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * ice_aq_fwlog_set - Set FW logging configuration AQ command (0xFF30)
> + * @hw: pointer to the HW structure
> + * @entries: entries to configure
> + * @num_entries: number of @entries
> + * @options: options from ice_fwlog_cfg->options structure
> + * @log_resolution: logging resolution
> + */
> +static int
> +ice_aq_fwlog_set(struct ice_hw *hw, struct ice_fwlog_module_entry *entries,
> +		 u16 num_entries, u16 options, u16 log_resolution)
> +{
> +	struct ice_aqc_fw_log_cfg_resp *fw_modules;
> +	struct ice_aqc_fw_log *cmd;
> +	struct ice_aq_desc desc;
> +	int status;
> +	u16 i;
> +
> +	fw_modules = kcalloc(num_entries, sizeof(*fw_modules), GFP_KERNEL);
> +	if (!fw_modules)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_entries; i++) {
> +		fw_modules[i].module_identifier =
> +			cpu_to_le16(entries[i].module_id);
> +		fw_modules[i].log_level = entries[i].log_level;
> +	}
> +
> +	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_fw_logs_config);
> +	desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> +
> +	cmd = &desc.params.fw_log;
> +
> +	cmd->cmd_flags = ICE_AQC_FW_LOG_CONF_SET_VALID;
> +	cmd->ops.cfg.log_resolution = cpu_to_le16(log_resolution);
> +	cmd->ops.cfg.mdl_cnt = cpu_to_le16(num_entries);
> +
> +	if (options & ICE_FWLOG_OPTION_ARQ_ENA)
> +		cmd->cmd_flags |= ICE_AQC_FW_LOG_CONF_AQ_EN;
> +	if (options & ICE_FWLOG_OPTION_UART_ENA)
> +		cmd->cmd_flags |= ICE_AQC_FW_LOG_CONF_UART_EN;
> +
> +	status = ice_aq_send_cmd(hw, &desc, fw_modules,
> +				 sizeof(*fw_modules) * num_entries,
> +				 NULL);
> +
> +	kfree(fw_modules);
> +
> +	return status;
> +}
> +
>   /**
>    * ice_fwlog_supported - Cached for whether FW supports FW logging or not
>    * @hw: pointer to the HW structure
> @@ -16,6 +175,99 @@ bool ice_fwlog_supported(struct ice_hw *hw)
>   	return hw->fwlog_support_ena;
>   }
>   
> +/**
> + * ice_fwlog_set - Set the firmware logging settings
> + * @hw: pointer to the HW structure
> + * @cfg: config used to set firmware logging
> + *
> + * This function should be called whenever the driver needs to set the firmware
> + * logging configuration. It can be called on initialization, reset, or during
> + * runtime.
> + *
> + * If the PF wishes to receive FW logging then it must register via
> + * ice_fwlog_register. Note, that ice_fwlog_register does not need to be called
> + * for init.
> + */
> +int
> +ice_fwlog_set(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)

This can fit on one line...

> +{
> +	int status;
> +
> +	if (!ice_fwlog_supported(hw))
> +		return -EOPNOTSUPP;
> +
> +	if (!valid_cfg(hw, cfg))
> +		return -EINVAL;
> +
> +	status = ice_aq_fwlog_set(hw, cfg->module_entries,
> +				  ICE_AQC_FW_LOG_ID_MAX, cfg->options,
> +				  cfg->log_resolution);
> +	if (!status)
> +		cache_cfg(hw, cfg);
> +
> +	return status;
> +}
> +
> +/**
> + * ice_aq_fwlog_register - Register PF for firmware logging events (0xFF31)
> + * @hw: pointer to the HW structure
> + * @reg: true to register and false to unregister
> + */
> +static int ice_aq_fwlog_register(struct ice_hw *hw, bool reg)
> +{
> +	struct ice_aq_desc desc;
> +
> +	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_fw_logs_register);
> +
> +	if (reg)
> +		desc.params.fw_log.cmd_flags = ICE_AQC_FW_LOG_AQ_REGISTER;
> +
> +	return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
> +}
> +
> +/**
> + * ice_fwlog_register - Register the PF for firmware logging
> + * @hw: pointer to the HW structure
> + *
> + * After this call the PF will start to receive firmware logging based on the
> + * configuration set in ice_fwlog_set.
> + */
> +int ice_fwlog_register(struct ice_hw *hw)
> +{
> +	int status;
> +
> +	if (!ice_fwlog_supported(hw))
> +		return -EOPNOTSUPP;
> +
> +	status = ice_aq_fwlog_register(hw, true);
> +	if (status)
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Failed to register for firmware logging events over ARQ\n");
> +	else
> +		hw->fwlog_cfg.options |= ICE_FWLOG_OPTION_IS_REGISTERED;
> +
> +	return status;
> +}
> +
> +/**
> + * ice_fwlog_unregister - Unregister the PF from firmware logging
> + * @hw: pointer to the HW structure
> + */
> +int ice_fwlog_unregister(struct ice_hw *hw)
> +{
> +	int status;
> +
> +	if (!ice_fwlog_supported(hw))
> +		return -EOPNOTSUPP;
> +
> +	status = ice_aq_fwlog_register(hw, false);
> +	if (status)
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Failed to unregister from firmware logging events over ARQ\n");
> +	else
> +		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_IS_REGISTERED;
> +
> +	return status;
> +}
> +
>   /**
>    * ice_aq_fwlog_get - Get the current firmware logging configuration (0xFF32)
>    * @hw: pointer to the HW structure
> @@ -115,3 +367,28 @@ void ice_fwlog_set_support_ena(struct ice_hw *hw)
>   
>   	kfree(cfg);
>   }
> +
> +/**
> + * ice_fwlog_get - Get the firmware logging settings
> + * @hw: pointer to the HW structure
> + * @cfg: config to populate based on current firmware logging settings
> + */
> +int
> +ice_fwlog_get(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)

and this one. Can you check that all the others are pulled up properly 
as well?

> +{
> +	int status;
> +
> +	if (!ice_fwlog_supported(hw))
> +		return -EOPNOTSUPP;
> +
> +	if (!cfg)
> +		return -EINVAL;
> +
> +	status = ice_aq_fwlog_get(hw, cfg);
> +	if (status)
> +		return status;
> +
> +	cache_cfg(hw, cfg);
> +
> +	return 0;
> +}
> diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.h b/drivers/net/ethernet/intel/ice/ice_fwlog.h
> index d7371253b8e5..66648c5ba92c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_fwlog.h
> +++ b/drivers/net/ethernet/intel/ice/ice_fwlog.h
> @@ -48,4 +48,9 @@ struct ice_fwlog_cfg {
>   
>   void ice_fwlog_set_support_ena(struct ice_hw *hw);
>   bool ice_fwlog_supported(struct ice_hw *hw);
> +int ice_fwlog_init(struct ice_hw *hw);
> +int ice_fwlog_set(struct ice_hw *hw, struct ice_fwlog_cfg *cfg);
> +int ice_fwlog_get(struct ice_hw *hw, struct ice_fwlog_cfg *cfg);
> +int ice_fwlog_register(struct ice_hw *hw);
> +int ice_fwlog_unregister(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 10b78faf0a32..c524179e79f0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_type.h
> +++ b/drivers/net/ethernet/intel/ice/ice_type.h
> @@ -861,6 +861,7 @@ struct ice_hw {
>   	u8 fw_patch;		/* firmware patch version */
>   	u32 fw_build;		/* firmware build number */
>   
> +	struct ice_fwlog_cfg fwlog_cfg;
>   	bool fwlog_support_ena; /* does hardware support FW logging? */
>   	bool fwlog_ena; /* currently logging? */
>   
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 4/5] ice: disable FW logging on driver unload
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 4/5] ice: disable FW logging on driver unload Paul M Stillwell Jr
  2022-11-29 12:05   ` Wilczynski, Michal
@ 2022-12-01  0:40   ` Tony Nguyen
  1 sibling, 0 replies; 21+ messages in thread
From: Tony Nguyen @ 2022-12-01  0:40 UTC (permalink / raw)
  To: Paul M Stillwell Jr, intel-wired-lan

On 11/28/2022 1:47 PM, Paul M Stillwell Jr wrote:
> The FW is running in it's own context irregardless of what the driver
> is doing. In this case, if the driver previously registered for FW
> log events and then the driver unloads without informing the FW to
> unregister for FW log events then the FW still has a timer running to
> output FW logs.
> 
> The next time the driver loads and tries to register for FW log events
> then the FW returns an error, but still enables the continued
> outputting of FW logs. This causes an IO error to devlink which isn't
> intuitive since the logs are still being output.
> 
> Fix this by disabling FW logging when the driver is being unloaded.
> 
> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_devlink.c | 32 +++++++++++++++-----
>   1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index ca67f2741f83..923556e6ae79 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -378,6 +378,10 @@ static int ice_devlink_info_get(struct devlink *devlink,
>   enum ice_devlink_param_id {
>   	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
>   	ICE_DEVLINK_PARAM_ID_TX_BALANCE,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
>   };
>   
>   /**
> @@ -1484,14 +1488,6 @@ 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,
> -	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
> -	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
> -};

Is there a reason you didn't add to the original enum in the previous 
patches and are it combining now?

> -
>   static int
>   ice_devlink_fwlog_supported_get(struct devlink *devlink, u32 id,
>   				struct devlink_param_gset_ctx *ctx)
> @@ -1743,6 +1739,26 @@ void ice_devlink_register(struct ice_pf *pf)
>    */
>   void ice_devlink_unregister(struct ice_pf *pf)
>   {
> +	struct ice_hw *hw = &pf->hw;
> +
> +	/* make sure FW logging is disabled to not put the FW in a weird state
> +	 * for the next driver load
> +	 */
> +	if (hw->fwlog_ena) {
> +		int status;
> +
> +		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
> +		status = ice_fwlog_set(hw, &hw->fwlog_cfg);
> +		if (status)
> +			dev_warn(ice_pf_to_dev(pf), "Unable to turn off FW logging, status: %d\n",
> +				 status);
> +
> +		status = ice_fwlog_unregister(hw);
> +		if (status)
> +			dev_warn(ice_pf_to_dev(pf), "Unable to unregister FW logging, status: %d\n",
> +				 status);
> +	}
> +
>   	devlink_unregister(priv_to_devlink(pf));
>   }
>   
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 5/5] ice: use debugfs to output FW log data
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 5/5] ice: use debugfs to output FW log data Paul M Stillwell Jr
  2022-11-29 13:53   ` Paul Menzel
@ 2022-12-01  0:40   ` Tony Nguyen
  2022-12-09 18:32     ` Paul M Stillwell Jr
  1 sibling, 1 reply; 21+ messages in thread
From: Tony Nguyen @ 2022-12-01  0:40 UTC (permalink / raw)
  To: Paul M Stillwell Jr, intel-wired-lan

On 11/28/2022 1:47 PM, Paul M Stillwell Jr wrote:
> The FW log data can be quite large so we don't want to use syslog.
> Instead take advantage of debugfs to write the data to.
> 
> Also, a previous patch addded codde to disable FW logging when
> the driver was unloaded. The code was added to
> ice_devlink_unregister(), which didn't seem correct so move the
> disabling of FW logging on driver unload to a better place.

Since this was done in the patch directly preceding this, why don't you 
just modify the previous patch to put it in the proper place?

> 
> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/Makefile       |   3 +-
>   drivers/net/ethernet/intel/ice/ice.h          |  24 ++++
>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   2 +
>   drivers/net/ethernet/intel/ice/ice_debugfs.c  | 109 ++++++++++++++++++
>   drivers/net/ethernet/intel/ice/ice_devlink.c  |  20 ----
>   drivers/net/ethernet/intel/ice/ice_main.c     |  98 ++++++++++++++++
>   6 files changed, 235 insertions(+), 21 deletions(-)
>   create mode 100644 drivers/net/ethernet/intel/ice/ice_debugfs.c
> 
> diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
> index 750fed7e07d7..5e0013330c46 100644
> --- a/drivers/net/ethernet/intel/ice/Makefile
> +++ b/drivers/net/ethernet/intel/ice/Makefile
> @@ -33,7 +33,8 @@ ice-y := ice_main.o	\
>   	 ice_ethtool.o  \
>   	 ice_repr.o	\
>   	 ice_tc_lib.o	\
> -	 ice_fwlog.o
> +	 ice_fwlog.o	\
> +	 ice_debugfs.o
>   ice-$(CONFIG_PCI_IOV) +=	\
>   	ice_sriov.o		\
>   	ice_virtchnl.o		\
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index ea64bcff108a..c8f1f34b8fbc 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -552,6 +552,9 @@ struct ice_pf {
>   	struct ice_vsi_stats **vsi_stats;
>   	struct ice_sw *first_sw;	/* first switch created by firmware */
>   	u16 eswitch_mode;		/* current mode of eswitch */
> +#ifdef CONFIG_DEBUG_FS
> +	struct dentry *ice_debugfs_pf;
> +#endif /* CONFIG_DEBUG_FS */
>   	struct ice_vfs vfs;
>   	DECLARE_BITMAP(features, ICE_F_MAX);
>   	DECLARE_BITMAP(state, ICE_STATE_NBITS);
> @@ -634,6 +637,8 @@ struct ice_pf {
>   #define ICE_VF_AGG_NODE_ID_START	65
>   #define ICE_MAX_VF_AGG_NODES		32
>   	struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES];
> +	struct list_head fwlog_data_list;
> +	u8 fwlog_list_count;
>   };
>   
>   struct ice_netdev_priv {
> @@ -648,6 +653,15 @@ struct ice_netdev_priv {
>   	struct list_head tc_indr_block_priv_list;
>   };
>   
> +struct ice_fwlog_data {
> +	struct list_head list;
> +	u16 data_size;
> +	u8 *data;
> +};
> +
> +/* define the maximum number of items that can be in the list */
> +#define ICE_FWLOG_MAX_SIZE	64
> +
>   /**
>    * ice_vector_ch_enabled
>    * @qv: pointer to q_vector, can be NULL
> @@ -872,6 +886,16 @@ static inline bool ice_is_adq_active(struct ice_pf *pf)
>   	return false;
>   }
>   
> +#ifdef CONFIG_DEBUG_FS
> +void ice_debugfs_fwlog_init(struct ice_pf *pf);
> +void ice_debugfs_init(void);
> +void ice_debugfs_exit(void);
> +#else
> +static inline void ice_debugfs_fwlog_init(struct ice_pf *pf) { }
> +static inline void ice_debugfs_init(void) { }
> +static inline void ice_debugfs_exit(void) { }
> +#endif /* CONFIG_DEBUG_FS */
> +
>   bool netif_is_ice(struct net_device *dev);
>   int ice_vsi_setup_tx_rings(struct ice_vsi *vsi);
>   int ice_vsi_setup_rx_rings(struct ice_vsi *vsi);
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index 8fa18bc5d441..d17bcc778bdd 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -2406,11 +2406,13 @@ enum ice_adminq_opc {
>   
>   	/* Standalone Commands/Events */
>   	ice_aqc_opc_event_lan_overflow			= 0x1001,
> +
>   	/* FW Logging Commands */
>   	ice_aqc_opc_fw_logs_config			= 0xFF30,
>   	ice_aqc_opc_fw_logs_register			= 0xFF31,
>   	ice_aqc_opc_fw_logs_query			= 0xFF32,
>   	ice_aqc_opc_fw_logs_event			= 0xFF33,
> +	ice_aqc_opc_fw_logs_get				= 0xFF34,

I don't think I see this being used.

>   };
>   
>   #endif /* _ICE_ADMINQ_CMD_H_ */
> diff --git a/drivers/net/ethernet/intel/ice/ice_debugfs.c b/drivers/net/ethernet/intel/ice/ice_debugfs.c
> new file mode 100644
> index 000000000000..767df937f56a
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ice/ice_debugfs.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2018, Intel Corporation. */
> +
> +#include <linux/fs.h>
> +#include <linux/debugfs.h>
> +#include <linux/random.h>
> +#include "ice.h"
> +
> +static struct dentry *ice_debugfs_root;
> +
> +/**
> + * ice_debugfs_command_read - read from command datum
> + * @filp: the opened file
> + * @buffer: where to write the data for the user to read
> + * @count: the size of the user's buffer
> + * @ppos: file position offset
> + */
> +static ssize_t ice_debugfs_command_read(struct file *filp, char __user *buffer,
> +					size_t count, loff_t *ppos)
> +{
> +	struct ice_pf *pf = filp->private_data;
> +	struct device *dev = ice_pf_to_dev(pf);
> +	struct ice_fwlog_data *log, *tmp_log;
> +	int data_copied = 0;
> +
> +	if (list_empty(&pf->fwlog_data_list)) {
> +		dev_info(dev, "FW log is empty\n");
> +		return 0;
> +	}
> +
> +	list_for_each_entry_safe(log, tmp_log, &pf->fwlog_data_list, list) {
> +		u16 cur_buf_len = log->data_size;
> +		int retval;
> +
> +		if (cur_buf_len > count)
> +			break;
> +
> +		retval = copy_to_user(buffer, log->data, cur_buf_len);
> +		if (retval)
> +			return -EFAULT;
> +
> +		data_copied += cur_buf_len;
> +		buffer += cur_buf_len;
> +		count -= cur_buf_len;
> +		*ppos += cur_buf_len;
> +
> +		/* don't delete the list element until we know it got copied */
> +		kfree(log->data);
> +		list_del(&log->list);
> +		kfree(log);
> +		pf->fwlog_list_count--;
> +	}
> +
> +	return data_copied;
> +}
> +
> +static const struct file_operations ice_debugfs_command_fops = {
> +	.owner = THIS_MODULE,
> +	.open  = simple_open,
> +	.read = ice_debugfs_command_read,
> +};
> +
> +/**
> + * ice_debugfs_fwlog_init - setup the debugfs directory
> + * @pf: the ice that is starting up
> + */
> +void ice_debugfs_fwlog_init(struct ice_pf *pf)
> +{
> +	const char *name = pci_name(pf->pdev);
> +	struct dentry *pfile;
> +
> +	/* only support fw log commands on PF 0 */
> +	if (pf->hw.bus.func)
> +		return;
> +
> +	pf->ice_debugfs_pf = debugfs_create_dir(name, ice_debugfs_root);
> +	if (IS_ERR(pf->ice_debugfs_pf))
> +		return;
> +
> +	pfile = debugfs_create_file("fwlog", 0400, pf->ice_debugfs_pf, pf,
> +				    &ice_debugfs_command_fops);
> +	if (!pfile)
> +		goto create_failed;
> +
> +	return;
> +
> +create_failed:
> +	dev_err(ice_pf_to_dev(pf), "debugfs dir/file for %s failed\n", name);
> +	debugfs_remove_recursive(pf->ice_debugfs_pf);
> +}
> +
> +/**
> + * ice_debugfs_init - create root directory for debugfs entries
> + */
> +void ice_debugfs_init(void)
> +{
> +	ice_debugfs_root = debugfs_create_dir(KBUILD_MODNAME, NULL);
> +	if (IS_ERR(ice_debugfs_root))
> +		pr_info("init of debugfs failed\n");
> +}
> +
> +/**
> + * ice_debugfs_exit - remove debugfs entries
> + */
> +void ice_debugfs_exit(void)
> +{
> +	debugfs_remove_recursive(ice_debugfs_root);
> +	ice_debugfs_root = NULL;
> +}
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index 923556e6ae79..148902b515bf 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -1739,26 +1739,6 @@ void ice_devlink_register(struct ice_pf *pf)
>    */
>   void ice_devlink_unregister(struct ice_pf *pf)
>   {
> -	struct ice_hw *hw = &pf->hw;
> -
> -	/* make sure FW logging is disabled to not put the FW in a weird state
> -	 * for the next driver load
> -	 */
> -	if (hw->fwlog_ena) {
> -		int status;
> -
> -		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
> -		status = ice_fwlog_set(hw, &hw->fwlog_cfg);
> -		if (status)
> -			dev_warn(ice_pf_to_dev(pf), "Unable to turn off FW logging, status: %d\n",
> -				 status);
> -
> -		status = ice_fwlog_unregister(hw);
> -		if (status)
> -			dev_warn(ice_pf_to_dev(pf), "Unable to unregister FW logging, status: %d\n",
> -				 status);
> -	}
> -
>   	devlink_unregister(priv_to_devlink(pf));
>   }
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 517702af327c..01cf4cb78009 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -1213,6 +1213,45 @@ ice_handle_link_event(struct ice_pf *pf, struct ice_rq_event_info *event)
>   	return status;
>   }
>   
> +/**
> + * ice_get_fwlog_data - copy the FW log data from ARQ event
> + * @pf: PF that the FW log event is associated with
> + * @event: event structure containing FW log data
> + */
> +static void
> +ice_get_fwlog_data(struct ice_pf *pf, struct ice_rq_event_info *event)
> +{
> +	struct device *dev = ice_pf_to_dev(pf);
> +	struct ice_fwlog_data *fwlog;
> +
> +	if (pf->fwlog_list_count >= ICE_FWLOG_MAX_SIZE) {
> +		dev_info(dev, "Reached max list size for fwlog list!\n");
> +		return;
> +	}
> +
> +	fwlog = kzalloc(sizeof(*fwlog), GFP_KERNEL);
> +	if (!fwlog) {
> +		dev_warn(dev, "Couldn't allocate memory for FWlog element\n");
> +		return;
> +	}
> +
> +	INIT_LIST_HEAD(&fwlog->list);
> +
> +	fwlog->data_size = le16_to_cpu(event->desc.datalen);
> +	fwlog->data = kzalloc(fwlog->data_size, GFP_KERNEL);
> +	if (!fwlog->data) {
> +		dev_warn(dev, "Couldn't allocate memory for FWlog data\n");
> +		kfree(fwlog);
> +		return;
> +	}
> +
> +	memcpy(fwlog->data, event->msg_buf, fwlog->data_size);
> +
> +	list_add_tail(&fwlog->list, &pf->fwlog_data_list);
> +
> +	pf->fwlog_list_count++;
> +}
> +
>   enum ice_aq_task_state {
>   	ICE_AQ_TASK_WAITING = 0,
>   	ICE_AQ_TASK_COMPLETE,
> @@ -1486,6 +1525,9 @@ static int __ice_clean_ctrlq(struct ice_pf *pf, enum ice_ctl_q q_type)
>   			if (!ice_is_malicious_vf(pf, &event, i, pending))
>   				ice_vc_process_vf_msg(pf, &event);
>   			break;
> +		case ice_aqc_opc_fw_logs_event:
> +			ice_get_fwlog_data(pf, &event);
> +			break;
>   		case ice_aqc_opc_lldp_set_mib_change:
>   			ice_dcb_process_lldp_set_mib_change(pf, &event);
>   			break;
> @@ -4688,6 +4730,52 @@ static int ice_register_netdev(struct ice_pf *pf)
>   	return err;
>   }
>   
> +/**
> + * ice_pf_fwlog_init - initialize FW logging on device init
> + * @pf: pointer to the PF struct
> + *
> + * This should always be called after ice_hw_init().
> + */
> +static void
> +ice_pf_fwlog_init(struct ice_pf *pf)

Is this going to expand to other things or why not just call it 
ice_fwlog_init()?

> +{
> +	INIT_LIST_HEAD(&pf->fwlog_data_list);
> +}
> +
> +/**
> + * ice_pf_fwlog_deinit - clear FW logging metadata on device exit
> + * @pf: pointer to the PF struct
> + */
> +static void
> +ice_pf_fwlog_deinit(struct ice_pf *pf)

same with this one

> +{
> +	struct ice_fwlog_data *fwlog, *fwlog_tmp;
> +	struct ice_hw *hw = &pf->hw;
> +
> +	/* make sure FW logging is disabled to not put the FW in a weird state
> +	 * for the next driver load
> +	 */
> +	if (hw->fwlog_ena) {
> +		int status;
> +
> +		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
> +		status = ice_fwlog_set(hw, &hw->fwlog_cfg);
> +		if (status)
> +			dev_warn(ice_pf_to_dev(pf), "Unable to turn off FW logging, status: %d\n",
> +				 status);
> +
> +		status = ice_fwlog_unregister(hw);
> +		if (status)
> +			dev_warn(ice_pf_to_dev(pf), "Unable to unregister FW logging, status: %d\n",
> +				 status);
> +	}
> +
> +	list_for_each_entry_safe(fwlog, fwlog_tmp, &pf->fwlog_data_list, list) {
> +		kfree(fwlog->data);
> +		kfree(fwlog);
> +	}
> +}
> +
>   /**
>    * ice_probe - Device initialization routine
>    * @pdev: PCI device information struct
> @@ -4773,8 +4861,12 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>   		goto err_exit_unroll;
>   	}
>   
> +	ice_pf_fwlog_init(pf);
> +
>   	ice_init_feature_support(pf);
>   
> +	ice_debugfs_fwlog_init(pf);
> +

Do these need to be unrolled for error path?

>   	err = ice_init_ddp_config(hw, pf);
>   
>   	/* during topology change ice_init_hw may fail */
> @@ -5139,6 +5231,8 @@ static void ice_remove(struct pci_dev *pdev)
>   		msleep(100);
>   	}
>   
> +	ice_pf_fwlog_deinit(pf);
> +
>   	ice_tc_indir_block_remove(pf);
>   
>   	if (test_bit(ICE_FLAG_SRIOV_ENA, pf->flags)) {
> @@ -5620,10 +5714,13 @@ static int __init ice_module_init(void)
>   		return -ENOMEM;
>   	}
>   
> +	ice_debugfs_init();
> +
>   	status = pci_register_driver(&ice_driver);
>   	if (status) {
>   		pr_err("failed to register PCI driver, err %d\n", status);
>   		destroy_workqueue(ice_wq);
> +		ice_debugfs_exit();
>   	}
>   
>   	return status;
> @@ -5640,6 +5737,7 @@ static void __exit ice_module_exit(void)
>   {
>   	pci_unregister_driver(&ice_driver);
>   	destroy_workqueue(ice_wq);
> +	ice_debugfs_exit();
>   	pr_info("module unloaded\n");
>   }
>   module_exit(ice_module_exit);
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 3/5] ice: add ability to query/set FW log level and resolution
  2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 3/5] ice: add ability to query/set FW log level and resolution Paul M Stillwell Jr
  2022-11-29 11:48   ` Wilczynski, Michal
  2022-12-01  0:39   ` Tony Nguyen
@ 2022-12-01  7:36   ` Paul Menzel
  2 siblings, 0 replies; 21+ messages in thread
From: Paul Menzel @ 2022-12-01  7:36 UTC (permalink / raw)
  To: Paul M Stillwell Jr; +Cc: intel-wired-lan

Dear Paul,


Thank you for the patch.

Am 28.11.22 um 22:47 schrieb Paul M Stillwell Jr:
> The E8xx has the ability to change the FW log level and
> the granularity at which the logs get output. Enable
> the ability to see what the current values are and to
> change them.

Please reflow the message for 75 characters per line.

Please add the examples, how to list and change the current values.

A small comment regarding the implementation would also be nice, and 
maybe a reference to the used datasheet (name, revision).

> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> ---
>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   4 +
>   drivers/net/ethernet/intel/ice/ice_common.c   |   4 +-
>   drivers/net/ethernet/intel/ice/ice_devlink.c  | 142 ++++++++-
>   drivers/net/ethernet/intel/ice/ice_fwlog.c    | 277 ++++++++++++++++++
>   drivers/net/ethernet/intel/ice/ice_fwlog.h    |   5 +
>   drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
>   6 files changed, 429 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index d0026161a2b4..8fa18bc5d441 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -2406,7 +2406,11 @@ enum ice_adminq_opc {
>   
>   	/* Standalone Commands/Events */
>   	ice_aqc_opc_event_lan_overflow			= 0x1001,
> +	/* FW Logging Commands */
> +	ice_aqc_opc_fw_logs_config			= 0xFF30,
> +	ice_aqc_opc_fw_logs_register			= 0xFF31,
>   	ice_aqc_opc_fw_logs_query			= 0xFF32,
> +	ice_aqc_opc_fw_logs_event			= 0xFF33,
>   };
>   
>   #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 ecdc1ebb45d5..0b3adac13c66 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -879,7 +879,9 @@ int ice_init_hw(struct ice_hw *hw)
>   	if (status)
>   		goto err_unroll_cqinit;
>   
> -	ice_fwlog_set_support_ena(hw);
> +	status = ice_fwlog_init(hw);
> +	if (status)
> +		ice_debug(hw, ICE_DBG_FW_LOG, "Error initializing FW logging: %d\n", status);
>   
>   	status = ice_clear_pf_cfg(hw);
>   	if (status)
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index 8843ff492f7f..ca67f2741f83 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -1488,6 +1488,8 @@ 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,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
>   };
>   
>   static int
> @@ -1530,8 +1532,121 @@ 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;
> +	struct ice_pf *pf = devlink_priv(devlink);
> +	struct ice_hw *hw = &pf->hw;
> +	int status;
> +
> +	/* only support fw log commands on PF 0 */
> +	if (hw->bus.func)
> +		return -EOPNOTSUPP;
> +
> +	if (hw->fwlog_ena == ctx->val.vbool)
> +		return 0;
> +
> +	hw->fwlog_ena = ctx->val.vbool;
> +
> +	if (hw->fwlog_ena)
> +		hw->fwlog_cfg.options |= ICE_FWLOG_OPTION_ARQ_ENA;
> +	else
> +		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
> +
> +	/* send the cfg to FW and register for events */
> +	status = ice_fwlog_set(hw, &hw->fwlog_cfg);
> +	if (status)
> +		return status;
> +
> +	if (hw->fwlog_ena) {
> +		status = ice_fwlog_register(hw);
> +		if (status)
> +			return status;
> +	} else {
> +		status = ice_fwlog_unregister(hw);
> +		if (status)
> +			return status;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +ice_devlink_fwlog_level_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;
> +
> +	/* all the log levels are the same so pick the first one */
> +	ctx->val.vu8 = pf->hw.fwlog_cfg.module_entries[0].log_level;
> +
> +	return 0;
> +}
> +
> +static int
> +ice_devlink_fwlog_level_set(struct devlink *devlink, u32 id,
> +			    struct devlink_param_gset_ctx *ctx)
> +{
> +	struct ice_pf *pf = devlink_priv(devlink);
> +	struct ice_fwlog_cfg *cfg;
> +	u8 i;

Please use native types. Limiting the width actually generates more code 
[1]. (You can also check that with `scripts/bloat-o-meter`.)

(Please also fix that for the rest of the added code.)

[…]


Kind regards,

Paul


[1]: https://notabs.org/coding/smallIntsBigPenalty.htm
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 3/5] ice: add ability to query/set FW log level and resolution
  2022-12-01  0:39   ` Tony Nguyen
@ 2022-12-09  0:00     ` Paul M Stillwell Jr
  0 siblings, 0 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2022-12-09  0:00 UTC (permalink / raw)
  To: Tony Nguyen, intel-wired-lan

On 11/30/2022 4:39 PM, Tony Nguyen wrote:
> On 11/28/2022 1:47 PM, Paul M Stillwell Jr wrote:
>> The E8xx has the ability to change the FW log level and
>> the granularity at which the logs get output. Enable
>> the ability to see what the current values are and to
>> change them.
>>
>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>> ---
>>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   4 +
>>   drivers/net/ethernet/intel/ice/ice_common.c   |   4 +-
>>   drivers/net/ethernet/intel/ice/ice_devlink.c  | 142 ++++++++-
>>   drivers/net/ethernet/intel/ice/ice_fwlog.c    | 277 ++++++++++++++++++
>>   drivers/net/ethernet/intel/ice/ice_fwlog.h    |   5 +
>>   drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
>>   6 files changed, 429 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h 
>> b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> index d0026161a2b4..8fa18bc5d441 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> @@ -2406,7 +2406,11 @@ enum ice_adminq_opc {
>>       /* Standalone Commands/Events */
>>       ice_aqc_opc_event_lan_overflow            = 0x1001,
>> +    /* FW Logging Commands */
> 
> nit: As this seperator was removed from patch 1 and the first command in 
> patch 2. This comment seems to fit better there. Also retaining the new 
> line between the op and the FW logging group would be nice.
> 
>> +    ice_aqc_opc_fw_logs_config            = 0xFF30,
>> +    ice_aqc_opc_fw_logs_register            = 0xFF31,
>>       ice_aqc_opc_fw_logs_query            = 0xFF32,
>> +    ice_aqc_opc_fw_logs_event            = 0xFF33,
>>   };
>>   #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 ecdc1ebb45d5..0b3adac13c66 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_common.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
>> @@ -879,7 +879,9 @@ int ice_init_hw(struct ice_hw *hw)
>>       if (status)
>>           goto err_unroll_cqinit;
>> -    ice_fwlog_set_support_ena(hw);
>> +    status = ice_fwlog_init(hw);
>> +    if (status)
>> +        ice_debug(hw, ICE_DBG_FW_LOG, "Error initializing FW logging: 
>> %d\n", status);
>>       status = ice_clear_pf_cfg(hw);
>>       if (status)
>> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c 
>> b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> index 8843ff492f7f..ca67f2741f83 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> @@ -1488,6 +1488,8 @@ 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,
>> +    ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
>> +    ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
>>   };
>>   static int
>> @@ -1530,8 +1532,121 @@ 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;
>> +    struct ice_pf *pf = devlink_priv(devlink);
>> +    struct ice_hw *hw = &pf->hw;
>> +    int status;
>> +
>> +    /* only support fw log commands on PF 0 */
>> +    if (hw->bus.func)
>> +        return -EOPNOTSUPP;
>> +
>> +    if (hw->fwlog_ena == ctx->val.vbool)
>> +        return 0;
>> +
>> +    hw->fwlog_ena = ctx->val.vbool;
>> +
>> +    if (hw->fwlog_ena)
>> +        hw->fwlog_cfg.options |= ICE_FWLOG_OPTION_ARQ_ENA;
>> +    else
>> +        hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
>> +
>> +    /* send the cfg to FW and register for events */
>> +    status = ice_fwlog_set(hw, &hw->fwlog_cfg);
>> +    if (status)
>> +        return status;
>> +
>> +    if (hw->fwlog_ena) {
>> +        status = ice_fwlog_register(hw);
>> +        if (status)
>> +            return status;
>> +    } else {
>> +        status = ice_fwlog_unregister(hw);
>> +        if (status)
>> +            return status;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int
>> +ice_devlink_fwlog_level_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;
>> +
>> +    /* all the log levels are the same so pick the first one */
>> +    ctx->val.vu8 = pf->hw.fwlog_cfg.module_entries[0].log_level;
> 
> Since they are all the same, would it make sense to store it in the 
> config and pull the value from there instead of duplicating it for each 
> entry?
> 

I don't think so. They have to all be stored in the config because you 
have to write them all out when you write the config. It makes sense to 
me to keep them all in the config and read one of them instead of having 
2 places with the value and possibly having a bug because they are out 
of sync.

If I'm misundersstanding your comment then let me know.

>> +
>> +    return 0;
>> +}
>> +
>> +static int
>> +ice_devlink_fwlog_level_set(struct devlink *devlink, u32 id,
>> +                struct devlink_param_gset_ctx *ctx)
>> +{
>> +    struct ice_pf *pf = devlink_priv(devlink);
>> +    struct ice_fwlog_cfg *cfg;
>> +    u8 i;
>> +
>> +    if (pf->hw.bus.func)
>> +        return -EOPNOTSUPP;
>> +
>> +    if (ctx->val.vu8 >= ICE_FWLOG_LEVEL_INVALID) {
>> +        dev_err(ice_pf_to_dev(pf), "Log level is greater than 
>> allowed! %d\n",
>> +            ctx->val.vu8);
>> +        return -EINVAL;
>> +    }
>> +
>> +    cfg = &pf->hw.fwlog_cfg;
>> +
>> +    /* update the log level for all modules to the same thing. this gets
>> +     * written to the FW when the user says enable logging
>> +     */
>> +    for (i = 0; i < ICE_AQC_FW_LOG_ID_MAX; i++)
>> +        cfg->module_entries[i].log_level = ctx->val.vu8;
>> +
>> +    return 0;
>> +}
>> +
>> +static int
>> +ice_devlink_fwlog_resolution_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.vu8 = pf->hw.fwlog_cfg.log_resolution;
>> +
>> +    return 0;
>> +}
>> +
>> +static int
>> +ice_devlink_fwlog_resolution_set(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;
>> +
>> +    if (ctx->val.vu8 < ICE_AQC_FW_LOG_MIN_RESOLUTION ||
>> +        ctx->val.vu8 > ICE_AQC_FW_LOG_MAX_RESOLUTION) {
>> +        dev_err(ice_pf_to_dev(pf), "Log resolution is not allowed! 
>> Should be between 1 - 128: %d\n",
>> +            ctx->val.vu8);
>> +        return -EINVAL;
>> +    }
>> +
>> +    pf->hw.fwlog_cfg.log_resolution = ctx->val.vu8;
>> +
>> +    return 0;
>>   }
>>   static const struct devlink_param ice_devlink_params[] = {
>> @@ -1562,6 +1677,18 @@ static const struct devlink_param 
>> ice_devlink_params[] = {
>>                    ice_devlink_fwlog_enabled_get,
>>                    ice_devlink_fwlog_enabled_set,
>>                    NULL),
>> +    DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
>> +                 "fwlog_level", DEVLINK_PARAM_TYPE_U8,
>> +                 BIT(DEVLINK_PARAM_CMODE_RUNTIME),
>> +                 ice_devlink_fwlog_level_get,
>> +                 ice_devlink_fwlog_level_set,
>> +                 NULL),
>> +    DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
>> +                 "fwlog_resolution", DEVLINK_PARAM_TYPE_U8,
>> +                 BIT(DEVLINK_PARAM_CMODE_RUNTIME),
>> +                 ice_devlink_fwlog_resolution_get,
>> +                 ice_devlink_fwlog_resolution_set,
>> +                 NULL),
>>   };
>>   static void ice_devlink_free(void *devlink_ptr)
>> @@ -1662,11 +1789,20 @@ int ice_devlink_register_params(struct ice_pf 
>> *pf)
>>                          ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED,
>>                          value);
>> -    value.vbool = false;
> 
> Is this on purpose?
> 

Yes, the value doesn't change between the 2 calls so no need to set it a 
second time.

>>       devlink_param_driverinit_value_set(devlink,
>>                          ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED,
>>                          value);
>> +    value.vu8 = ICE_FWLOG_LEVEL_NORMAL;
>> +    devlink_param_driverinit_value_set(devlink,
>> +                       ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
>> +                       value);
>> +
>> +    /* set the default value for the FW to pack 10 messages per AQ 
>> event */
>> +    value.vu8 = 10;
>> +    devlink_param_driverinit_value_set(devlink,
>> +                       ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
>> +                       value);
>>       return 0;
>>   }
>> diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.c 
>> b/drivers/net/ethernet/intel/ice/ice_fwlog.c
>> index 67e670c62091..1174fd889307 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_fwlog.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_fwlog.c
>> @@ -4,6 +4,165 @@
>>   #include "ice_common.h"
>>   #include "ice_fwlog.h"
>> +/**
>> + * cache_cfg - Cache FW logging config
>> + * @hw: pointer to the HW structure
>> + * @cfg: config to cache
>> + */
>> +static void cache_cfg(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
>> +{
>> +    hw->fwlog_cfg = *cfg;
>> +}
>> +
>> +/**
>> + * valid_module_entries - validate all the module entry IDs and log 
>> levels
>> + * @hw: pointer to the HW structure
>> + * @entries: entries to validate
>> + * @num_entries: number of entries to validate
>> + */
>> +static bool
>> +valid_module_entries(struct ice_hw *hw, struct ice_fwlog_module_entry 
>> *entries,
>> +             u16 num_entries)
>> +{
>> +    u16 i;
>> +
>> +    if (!entries) {
>> +        ice_debug(hw, ICE_DBG_FW_LOG, "Null ice_fwlog_module_entry 
>> array\n");
>> +        return false;
>> +    }
>> +
>> +    if (!num_entries) {
>> +        ice_debug(hw, ICE_DBG_FW_LOG, "num_entries must be non-zero\n");
>> +        return false;
>> +    }
>> +
>> +    for (i = 0; i < num_entries; i++) {
>> +        struct ice_fwlog_module_entry *entry = &entries[i];
>> +
>> +        if (entry->module_id >= ICE_AQC_FW_LOG_ID_MAX) {
>> +            ice_debug(hw, ICE_DBG_FW_LOG, "Invalid module_id %u, max 
>> valid module_id is %u\n",
>> +                  entry->module_id, ICE_AQC_FW_LOG_ID_MAX - 1);
>> +            return false;
>> +        }
>> +
>> +        if (entry->log_level >= ICE_FWLOG_LEVEL_INVALID) {
>> +            ice_debug(hw, ICE_DBG_FW_LOG, "Invalid log_level %u, max 
>> valid log_level is %u\n",
>> +                  entry->log_level,
>> +                  ICE_AQC_FW_LOG_ID_MAX - 1);
>> +            return false;
>> +        }
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +/**
>> + * valid_cfg - validate entire configuration
>> + * @hw: pointer to the HW structure
>> + * @cfg: config to validate
>> + */
>> +static bool valid_cfg(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
>> +{
>> +    if (!cfg) {
>> +        ice_debug(hw, ICE_DBG_FW_LOG, "Null ice_fwlog_cfg\n");
>> +        return false;
>> +    }
>> +
>> +    if (cfg->log_resolution < ICE_AQC_FW_LOG_MIN_RESOLUTION ||
>> +        cfg->log_resolution > ICE_AQC_FW_LOG_MAX_RESOLUTION) {
>> +        ice_debug(hw, ICE_DBG_FW_LOG, "Unsupported log_resolution %u, 
>> must be between %u and %u\n",
>> +              cfg->log_resolution, ICE_AQC_FW_LOG_MIN_RESOLUTION,
>> +              ICE_AQC_FW_LOG_MAX_RESOLUTION);
>> +        return false;
>> +    }
>> +
>> +    if (!valid_module_entries(hw, cfg->module_entries,
>> +                  ICE_AQC_FW_LOG_ID_MAX))
>> +        return false;
>> +
>> +    return true;
>> +}
>> +
>> +/**
>> + * ice_fwlog_init - Initialize FW logging variables
>> + * @hw: pointer to the HW structure
>> + *
>> + * This function should be called on driver initialization during
>> + * ice_init_hw().
>> + */
>> +int ice_fwlog_init(struct ice_hw *hw)
>> +{
>> +    int status;
>> +
>> +    ice_fwlog_set_support_ena(hw);
>> +
>> +    if (ice_fwlog_supported(hw)) {
>> +        struct ice_fwlog_cfg *cfg;
>> +
>> +        cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
>> +        if (!cfg)
>> +            return -ENOMEM;
>> +
>> +        /* read the current config from the FW and store it */
>> +        status = ice_fwlog_get(hw, cfg);
>> +        if (status)
>> +            return status;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/**
>> + * ice_aq_fwlog_set - Set FW logging configuration AQ command (0xFF30)
>> + * @hw: pointer to the HW structure
>> + * @entries: entries to configure
>> + * @num_entries: number of @entries
>> + * @options: options from ice_fwlog_cfg->options structure
>> + * @log_resolution: logging resolution
>> + */
>> +static int
>> +ice_aq_fwlog_set(struct ice_hw *hw, struct ice_fwlog_module_entry 
>> *entries,
>> +         u16 num_entries, u16 options, u16 log_resolution)
>> +{
>> +    struct ice_aqc_fw_log_cfg_resp *fw_modules;
>> +    struct ice_aqc_fw_log *cmd;
>> +    struct ice_aq_desc desc;
>> +    int status;
>> +    u16 i;
>> +
>> +    fw_modules = kcalloc(num_entries, sizeof(*fw_modules), GFP_KERNEL);
>> +    if (!fw_modules)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < num_entries; i++) {
>> +        fw_modules[i].module_identifier =
>> +            cpu_to_le16(entries[i].module_id);
>> +        fw_modules[i].log_level = entries[i].log_level;
>> +    }
>> +
>> +    ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_fw_logs_config);
>> +    desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
>> +
>> +    cmd = &desc.params.fw_log;
>> +
>> +    cmd->cmd_flags = ICE_AQC_FW_LOG_CONF_SET_VALID;
>> +    cmd->ops.cfg.log_resolution = cpu_to_le16(log_resolution);
>> +    cmd->ops.cfg.mdl_cnt = cpu_to_le16(num_entries);
>> +
>> +    if (options & ICE_FWLOG_OPTION_ARQ_ENA)
>> +        cmd->cmd_flags |= ICE_AQC_FW_LOG_CONF_AQ_EN;
>> +    if (options & ICE_FWLOG_OPTION_UART_ENA)
>> +        cmd->cmd_flags |= ICE_AQC_FW_LOG_CONF_UART_EN;
>> +
>> +    status = ice_aq_send_cmd(hw, &desc, fw_modules,
>> +                 sizeof(*fw_modules) * num_entries,
>> +                 NULL);
>> +
>> +    kfree(fw_modules);
>> +
>> +    return status;
>> +}
>> +
>>   /**
>>    * ice_fwlog_supported - Cached for whether FW supports FW logging 
>> or not
>>    * @hw: pointer to the HW structure
>> @@ -16,6 +175,99 @@ bool ice_fwlog_supported(struct ice_hw *hw)
>>       return hw->fwlog_support_ena;
>>   }
>> +/**
>> + * ice_fwlog_set - Set the firmware logging settings
>> + * @hw: pointer to the HW structure
>> + * @cfg: config used to set firmware logging
>> + *
>> + * This function should be called whenever the driver needs to set 
>> the firmware
>> + * logging configuration. It can be called on initialization, reset, 
>> or during
>> + * runtime.
>> + *
>> + * If the PF wishes to receive FW logging then it must register via
>> + * ice_fwlog_register. Note, that ice_fwlog_register does not need to 
>> be called
>> + * for init.
>> + */
>> +int
>> +ice_fwlog_set(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
> 
> This can fit on one line...
> 

I went through the file and pulled all of them up that could be. Nice catch.

>> +{
>> +    int status;
>> +
>> +    if (!ice_fwlog_supported(hw))
>> +        return -EOPNOTSUPP;
>> +
>> +    if (!valid_cfg(hw, cfg))
>> +        return -EINVAL;
>> +
>> +    status = ice_aq_fwlog_set(hw, cfg->module_entries,
>> +                  ICE_AQC_FW_LOG_ID_MAX, cfg->options,
>> +                  cfg->log_resolution);
>> +    if (!status)
>> +        cache_cfg(hw, cfg);
>> +
>> +    return status;
>> +}
>> +
>> +/**
>> + * ice_aq_fwlog_register - Register PF for firmware logging events 
>> (0xFF31)
>> + * @hw: pointer to the HW structure
>> + * @reg: true to register and false to unregister
>> + */
>> +static int ice_aq_fwlog_register(struct ice_hw *hw, bool reg)
>> +{
>> +    struct ice_aq_desc desc;
>> +
>> +    ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_fw_logs_register);
>> +
>> +    if (reg)
>> +        desc.params.fw_log.cmd_flags = ICE_AQC_FW_LOG_AQ_REGISTER;
>> +
>> +    return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
>> +}
>> +
>> +/**
>> + * ice_fwlog_register - Register the PF for firmware logging
>> + * @hw: pointer to the HW structure
>> + *
>> + * After this call the PF will start to receive firmware logging 
>> based on the
>> + * configuration set in ice_fwlog_set.
>> + */
>> +int ice_fwlog_register(struct ice_hw *hw)
>> +{
>> +    int status;
>> +
>> +    if (!ice_fwlog_supported(hw))
>> +        return -EOPNOTSUPP;
>> +
>> +    status = ice_aq_fwlog_register(hw, true);
>> +    if (status)
>> +        ice_debug(hw, ICE_DBG_FW_LOG, "Failed to register for 
>> firmware logging events over ARQ\n");
>> +    else
>> +        hw->fwlog_cfg.options |= ICE_FWLOG_OPTION_IS_REGISTERED;
>> +
>> +    return status;
>> +}
>> +
>> +/**
>> + * ice_fwlog_unregister - Unregister the PF from firmware logging
>> + * @hw: pointer to the HW structure
>> + */
>> +int ice_fwlog_unregister(struct ice_hw *hw)
>> +{
>> +    int status;
>> +
>> +    if (!ice_fwlog_supported(hw))
>> +        return -EOPNOTSUPP;
>> +
>> +    status = ice_aq_fwlog_register(hw, false);
>> +    if (status)
>> +        ice_debug(hw, ICE_DBG_FW_LOG, "Failed to unregister from 
>> firmware logging events over ARQ\n");
>> +    else
>> +        hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_IS_REGISTERED;
>> +
>> +    return status;
>> +}
>> +
>>   /**
>>    * ice_aq_fwlog_get - Get the current firmware logging configuration 
>> (0xFF32)
>>    * @hw: pointer to the HW structure
>> @@ -115,3 +367,28 @@ void ice_fwlog_set_support_ena(struct ice_hw *hw)
>>       kfree(cfg);
>>   }
>> +
>> +/**
>> + * ice_fwlog_get - Get the firmware logging settings
>> + * @hw: pointer to the HW structure
>> + * @cfg: config to populate based on current firmware logging settings
>> + */
>> +int
>> +ice_fwlog_get(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
> 
> and this one. Can you check that all the others are pulled up properly 
> as well?
> 
>> +{
>> +    int status;
>> +
>> +    if (!ice_fwlog_supported(hw))
>> +        return -EOPNOTSUPP;
>> +
>> +    if (!cfg)
>> +        return -EINVAL;
>> +
>> +    status = ice_aq_fwlog_get(hw, cfg);
>> +    if (status)
>> +        return status;
>> +
>> +    cache_cfg(hw, cfg);
>> +
>> +    return 0;
>> +}
>> diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.h 
>> b/drivers/net/ethernet/intel/ice/ice_fwlog.h
>> index d7371253b8e5..66648c5ba92c 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_fwlog.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_fwlog.h
>> @@ -48,4 +48,9 @@ struct ice_fwlog_cfg {
>>   void ice_fwlog_set_support_ena(struct ice_hw *hw);
>>   bool ice_fwlog_supported(struct ice_hw *hw);
>> +int ice_fwlog_init(struct ice_hw *hw);
>> +int ice_fwlog_set(struct ice_hw *hw, struct ice_fwlog_cfg *cfg);
>> +int ice_fwlog_get(struct ice_hw *hw, struct ice_fwlog_cfg *cfg);
>> +int ice_fwlog_register(struct ice_hw *hw);
>> +int ice_fwlog_unregister(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 10b78faf0a32..c524179e79f0 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_type.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_type.h
>> @@ -861,6 +861,7 @@ struct ice_hw {
>>       u8 fw_patch;        /* firmware patch version */
>>       u32 fw_build;        /* firmware build number */
>> +    struct ice_fwlog_cfg fwlog_cfg;
>>       bool fwlog_support_ena; /* does hardware support FW logging? */
>>       bool fwlog_ena; /* currently logging? */

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-wired-lan] [PATCH net-next 5/5] ice: use debugfs to output FW log data
  2022-12-01  0:40   ` Tony Nguyen
@ 2022-12-09 18:32     ` Paul M Stillwell Jr
  0 siblings, 0 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2022-12-09 18:32 UTC (permalink / raw)
  To: Tony Nguyen, intel-wired-lan

On 11/30/2022 4:40 PM, Tony Nguyen wrote:
> On 11/28/2022 1:47 PM, Paul M Stillwell Jr wrote:
>> The FW log data can be quite large so we don't want to use syslog.
>> Instead take advantage of debugfs to write the data to.
>>
>> Also, a previous patch addded codde to disable FW logging when
>> the driver was unloaded. The code was added to
>> ice_devlink_unregister(), which didn't seem correct so move the
>> disabling of FW logging on driver unload to a better place.
> 
> Since this was done in the patch directly preceding this, why don't you 
> just modify the previous patch to put it in the proper place?
> 

I'll look and see what things look like and see if I can move it

>>
>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/Makefile       |   3 +-
>>   drivers/net/ethernet/intel/ice/ice.h          |  24 ++++
>>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   2 +
>>   drivers/net/ethernet/intel/ice/ice_debugfs.c  | 109 ++++++++++++++++++
>>   drivers/net/ethernet/intel/ice/ice_devlink.c  |  20 ----
>>   drivers/net/ethernet/intel/ice/ice_main.c     |  98 ++++++++++++++++
>>   6 files changed, 235 insertions(+), 21 deletions(-)
>>   create mode 100644 drivers/net/ethernet/intel/ice/ice_debugfs.c
>>
>> diff --git a/drivers/net/ethernet/intel/ice/Makefile 
>> b/drivers/net/ethernet/intel/ice/Makefile
>> index 750fed7e07d7..5e0013330c46 100644
>> --- a/drivers/net/ethernet/intel/ice/Makefile
>> +++ b/drivers/net/ethernet/intel/ice/Makefile
>> @@ -33,7 +33,8 @@ ice-y := ice_main.o    \
>>        ice_ethtool.o  \
>>        ice_repr.o    \
>>        ice_tc_lib.o    \
>> -     ice_fwlog.o
>> +     ice_fwlog.o    \
>> +     ice_debugfs.o
>>   ice-$(CONFIG_PCI_IOV) +=    \
>>       ice_sriov.o        \
>>       ice_virtchnl.o        \
>> diff --git a/drivers/net/ethernet/intel/ice/ice.h 
>> b/drivers/net/ethernet/intel/ice/ice.h
>> index ea64bcff108a..c8f1f34b8fbc 100644
>> --- a/drivers/net/ethernet/intel/ice/ice.h
>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>> @@ -552,6 +552,9 @@ struct ice_pf {
>>       struct ice_vsi_stats **vsi_stats;
>>       struct ice_sw *first_sw;    /* first switch created by firmware */
>>       u16 eswitch_mode;        /* current mode of eswitch */
>> +#ifdef CONFIG_DEBUG_FS
>> +    struct dentry *ice_debugfs_pf;
>> +#endif /* CONFIG_DEBUG_FS */
>>       struct ice_vfs vfs;
>>       DECLARE_BITMAP(features, ICE_F_MAX);
>>       DECLARE_BITMAP(state, ICE_STATE_NBITS);
>> @@ -634,6 +637,8 @@ struct ice_pf {
>>   #define ICE_VF_AGG_NODE_ID_START    65
>>   #define ICE_MAX_VF_AGG_NODES        32
>>       struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES];
>> +    struct list_head fwlog_data_list;
>> +    u8 fwlog_list_count;
>>   };
>>   struct ice_netdev_priv {
>> @@ -648,6 +653,15 @@ struct ice_netdev_priv {
>>       struct list_head tc_indr_block_priv_list;
>>   };
>> +struct ice_fwlog_data {
>> +    struct list_head list;
>> +    u16 data_size;
>> +    u8 *data;
>> +};
>> +
>> +/* define the maximum number of items that can be in the list */
>> +#define ICE_FWLOG_MAX_SIZE    64
>> +
>>   /**
>>    * ice_vector_ch_enabled
>>    * @qv: pointer to q_vector, can be NULL
>> @@ -872,6 +886,16 @@ static inline bool ice_is_adq_active(struct 
>> ice_pf *pf)
>>       return false;
>>   }
>> +#ifdef CONFIG_DEBUG_FS
>> +void ice_debugfs_fwlog_init(struct ice_pf *pf);
>> +void ice_debugfs_init(void);
>> +void ice_debugfs_exit(void);
>> +#else
>> +static inline void ice_debugfs_fwlog_init(struct ice_pf *pf) { }
>> +static inline void ice_debugfs_init(void) { }
>> +static inline void ice_debugfs_exit(void) { }
>> +#endif /* CONFIG_DEBUG_FS */
>> +
>>   bool netif_is_ice(struct net_device *dev);
>>   int ice_vsi_setup_tx_rings(struct ice_vsi *vsi);
>>   int ice_vsi_setup_rx_rings(struct ice_vsi *vsi);
>> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h 
>> b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> index 8fa18bc5d441..d17bcc778bdd 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> @@ -2406,11 +2406,13 @@ enum ice_adminq_opc {
>>       /* Standalone Commands/Events */
>>       ice_aqc_opc_event_lan_overflow            = 0x1001,
>> +
>>       /* FW Logging Commands */
>>       ice_aqc_opc_fw_logs_config            = 0xFF30,
>>       ice_aqc_opc_fw_logs_register            = 0xFF31,
>>       ice_aqc_opc_fw_logs_query            = 0xFF32,
>>       ice_aqc_opc_fw_logs_event            = 0xFF33,
>> +    ice_aqc_opc_fw_logs_get                = 0xFF34,
> 
> I don't think I see this being used.
> 

Will remove

>>   };
>>   #endif /* _ICE_ADMINQ_CMD_H_ */
>> diff --git a/drivers/net/ethernet/intel/ice/ice_debugfs.c 
>> b/drivers/net/ethernet/intel/ice/ice_debugfs.c
>> new file mode 100644
>> index 000000000000..767df937f56a
>> --- /dev/null
>> +++ b/drivers/net/ethernet/intel/ice/ice_debugfs.c
>> @@ -0,0 +1,109 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2018, Intel Corporation. */
>> +
>> +#include <linux/fs.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/random.h>
>> +#include "ice.h"
>> +
>> +static struct dentry *ice_debugfs_root;
>> +
>> +/**
>> + * ice_debugfs_command_read - read from command datum
>> + * @filp: the opened file
>> + * @buffer: where to write the data for the user to read
>> + * @count: the size of the user's buffer
>> + * @ppos: file position offset
>> + */
>> +static ssize_t ice_debugfs_command_read(struct file *filp, char 
>> __user *buffer,
>> +                    size_t count, loff_t *ppos)
>> +{
>> +    struct ice_pf *pf = filp->private_data;
>> +    struct device *dev = ice_pf_to_dev(pf);
>> +    struct ice_fwlog_data *log, *tmp_log;
>> +    int data_copied = 0;
>> +
>> +    if (list_empty(&pf->fwlog_data_list)) {
>> +        dev_info(dev, "FW log is empty\n");
>> +        return 0;
>> +    }
>> +
>> +    list_for_each_entry_safe(log, tmp_log, &pf->fwlog_data_list, list) {
>> +        u16 cur_buf_len = log->data_size;
>> +        int retval;
>> +
>> +        if (cur_buf_len > count)
>> +            break;
>> +
>> +        retval = copy_to_user(buffer, log->data, cur_buf_len);
>> +        if (retval)
>> +            return -EFAULT;
>> +
>> +        data_copied += cur_buf_len;
>> +        buffer += cur_buf_len;
>> +        count -= cur_buf_len;
>> +        *ppos += cur_buf_len;
>> +
>> +        /* don't delete the list element until we know it got copied */
>> +        kfree(log->data);
>> +        list_del(&log->list);
>> +        kfree(log);
>> +        pf->fwlog_list_count--;
>> +    }
>> +
>> +    return data_copied;
>> +}
>> +
>> +static const struct file_operations ice_debugfs_command_fops = {
>> +    .owner = THIS_MODULE,
>> +    .open  = simple_open,
>> +    .read = ice_debugfs_command_read,
>> +};
>> +
>> +/**
>> + * ice_debugfs_fwlog_init - setup the debugfs directory
>> + * @pf: the ice that is starting up
>> + */
>> +void ice_debugfs_fwlog_init(struct ice_pf *pf)
>> +{
>> +    const char *name = pci_name(pf->pdev);
>> +    struct dentry *pfile;
>> +
>> +    /* only support fw log commands on PF 0 */
>> +    if (pf->hw.bus.func)
>> +        return;
>> +
>> +    pf->ice_debugfs_pf = debugfs_create_dir(name, ice_debugfs_root);
>> +    if (IS_ERR(pf->ice_debugfs_pf))
>> +        return;
>> +
>> +    pfile = debugfs_create_file("fwlog", 0400, pf->ice_debugfs_pf, pf,
>> +                    &ice_debugfs_command_fops);
>> +    if (!pfile)
>> +        goto create_failed;
>> +
>> +    return;
>> +
>> +create_failed:
>> +    dev_err(ice_pf_to_dev(pf), "debugfs dir/file for %s failed\n", 
>> name);
>> +    debugfs_remove_recursive(pf->ice_debugfs_pf);
>> +}
>> +
>> +/**
>> + * ice_debugfs_init - create root directory for debugfs entries
>> + */
>> +void ice_debugfs_init(void)
>> +{
>> +    ice_debugfs_root = debugfs_create_dir(KBUILD_MODNAME, NULL);
>> +    if (IS_ERR(ice_debugfs_root))
>> +        pr_info("init of debugfs failed\n");
>> +}
>> +
>> +/**
>> + * ice_debugfs_exit - remove debugfs entries
>> + */
>> +void ice_debugfs_exit(void)
>> +{
>> +    debugfs_remove_recursive(ice_debugfs_root);
>> +    ice_debugfs_root = NULL;
>> +}
>> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c 
>> b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> index 923556e6ae79..148902b515bf 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>> @@ -1739,26 +1739,6 @@ void ice_devlink_register(struct ice_pf *pf)
>>    */
>>   void ice_devlink_unregister(struct ice_pf *pf)
>>   {
>> -    struct ice_hw *hw = &pf->hw;
>> -
>> -    /* make sure FW logging is disabled to not put the FW in a weird 
>> state
>> -     * for the next driver load
>> -     */
>> -    if (hw->fwlog_ena) {
>> -        int status;
>> -
>> -        hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
>> -        status = ice_fwlog_set(hw, &hw->fwlog_cfg);
>> -        if (status)
>> -            dev_warn(ice_pf_to_dev(pf), "Unable to turn off FW 
>> logging, status: %d\n",
>> -                 status);
>> -
>> -        status = ice_fwlog_unregister(hw);
>> -        if (status)
>> -            dev_warn(ice_pf_to_dev(pf), "Unable to unregister FW 
>> logging, status: %d\n",
>> -                 status);
>> -    }
>> -
>>       devlink_unregister(priv_to_devlink(pf));
>>   }
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c 
>> b/drivers/net/ethernet/intel/ice/ice_main.c
>> index 517702af327c..01cf4cb78009 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -1213,6 +1213,45 @@ ice_handle_link_event(struct ice_pf *pf, struct 
>> ice_rq_event_info *event)
>>       return status;
>>   }
>> +/**
>> + * ice_get_fwlog_data - copy the FW log data from ARQ event
>> + * @pf: PF that the FW log event is associated with
>> + * @event: event structure containing FW log data
>> + */
>> +static void
>> +ice_get_fwlog_data(struct ice_pf *pf, struct ice_rq_event_info *event)
>> +{
>> +    struct device *dev = ice_pf_to_dev(pf);
>> +    struct ice_fwlog_data *fwlog;
>> +
>> +    if (pf->fwlog_list_count >= ICE_FWLOG_MAX_SIZE) {
>> +        dev_info(dev, "Reached max list size for fwlog list!\n");
>> +        return;
>> +    }
>> +
>> +    fwlog = kzalloc(sizeof(*fwlog), GFP_KERNEL);
>> +    if (!fwlog) {
>> +        dev_warn(dev, "Couldn't allocate memory for FWlog element\n");
>> +        return;
>> +    }
>> +
>> +    INIT_LIST_HEAD(&fwlog->list);
>> +
>> +    fwlog->data_size = le16_to_cpu(event->desc.datalen);
>> +    fwlog->data = kzalloc(fwlog->data_size, GFP_KERNEL);
>> +    if (!fwlog->data) {
>> +        dev_warn(dev, "Couldn't allocate memory for FWlog data\n");
>> +        kfree(fwlog);
>> +        return;
>> +    }
>> +
>> +    memcpy(fwlog->data, event->msg_buf, fwlog->data_size);
>> +
>> +    list_add_tail(&fwlog->list, &pf->fwlog_data_list);
>> +
>> +    pf->fwlog_list_count++;
>> +}
>> +
>>   enum ice_aq_task_state {
>>       ICE_AQ_TASK_WAITING = 0,
>>       ICE_AQ_TASK_COMPLETE,
>> @@ -1486,6 +1525,9 @@ static int __ice_clean_ctrlq(struct ice_pf *pf, 
>> enum ice_ctl_q q_type)
>>               if (!ice_is_malicious_vf(pf, &event, i, pending))
>>                   ice_vc_process_vf_msg(pf, &event);
>>               break;
>> +        case ice_aqc_opc_fw_logs_event:
>> +            ice_get_fwlog_data(pf, &event);
>> +            break;
>>           case ice_aqc_opc_lldp_set_mib_change:
>>               ice_dcb_process_lldp_set_mib_change(pf, &event);
>>               break;
>> @@ -4688,6 +4730,52 @@ static int ice_register_netdev(struct ice_pf *pf)
>>       return err;
>>   }
>> +/**
>> + * ice_pf_fwlog_init - initialize FW logging on device init
>> + * @pf: pointer to the PF struct
>> + *
>> + * This should always be called after ice_hw_init().
>> + */
>> +static void
>> +ice_pf_fwlog_init(struct ice_pf *pf)
> 
> Is this going to expand to other things or why not just call it 
> ice_fwlog_init()?
> 

The reason I called it ice_pf_fwlog_init() is because it's going to get 
called for every PF. In my mind the implication of calling it 
ice_fwlog_init() is that it's only called once, which wouldn't be true.

>> +{
>> +    INIT_LIST_HEAD(&pf->fwlog_data_list);
>> +}
>> +
>> +/**
>> + * ice_pf_fwlog_deinit - clear FW logging metadata on device exit
>> + * @pf: pointer to the PF struct
>> + */
>> +static void
>> +ice_pf_fwlog_deinit(struct ice_pf *pf)
> 
> same with this one
> 

Same answer as above

>> +{
>> +    struct ice_fwlog_data *fwlog, *fwlog_tmp;
>> +    struct ice_hw *hw = &pf->hw;
>> +
>> +    /* make sure FW logging is disabled to not put the FW in a weird 
>> state
>> +     * for the next driver load
>> +     */
>> +    if (hw->fwlog_ena) {
>> +        int status;
>> +
>> +        hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
>> +        status = ice_fwlog_set(hw, &hw->fwlog_cfg);
>> +        if (status)
>> +            dev_warn(ice_pf_to_dev(pf), "Unable to turn off FW 
>> logging, status: %d\n",
>> +                 status);
>> +
>> +        status = ice_fwlog_unregister(hw);
>> +        if (status)
>> +            dev_warn(ice_pf_to_dev(pf), "Unable to unregister FW 
>> logging, status: %d\n",
>> +                 status);
>> +    }
>> +
>> +    list_for_each_entry_safe(fwlog, fwlog_tmp, &pf->fwlog_data_list, 
>> list) {
>> +        kfree(fwlog->data);
>> +        kfree(fwlog);
>> +    }
>> +}
>> +
>>   /**
>>    * ice_probe - Device initialization routine
>>    * @pdev: PCI device information struct
>> @@ -4773,8 +4861,12 @@ ice_probe(struct pci_dev *pdev, const struct 
>> pci_device_id __always_unused *ent)
>>           goto err_exit_unroll;
>>       }
>> +    ice_pf_fwlog_init(pf);
>> +
>>       ice_init_feature_support(pf);
>> +    ice_debugfs_fwlog_init(pf);
>> +
> 
> Do these need to be unrolled for error path?
> 

I don't think so. In ice_module_exit() we will remove the debugfs entry 
points

>>       err = ice_init_ddp_config(hw, pf);
>>       /* during topology change ice_init_hw may fail */
>> @@ -5139,6 +5231,8 @@ static void ice_remove(struct pci_dev *pdev)
>>           msleep(100);
>>       }
>> +    ice_pf_fwlog_deinit(pf);
>> +
>>       ice_tc_indir_block_remove(pf);
>>       if (test_bit(ICE_FLAG_SRIOV_ENA, pf->flags)) {
>> @@ -5620,10 +5714,13 @@ static int __init ice_module_init(void)
>>           return -ENOMEM;
>>       }
>> +    ice_debugfs_init();
>> +
>>       status = pci_register_driver(&ice_driver);
>>       if (status) {
>>           pr_err("failed to register PCI driver, err %d\n", status);
>>           destroy_workqueue(ice_wq);
>> +        ice_debugfs_exit();
>>       }
>>       return status;
>> @@ -5640,6 +5737,7 @@ static void __exit ice_module_exit(void)
>>   {
>>       pci_unregister_driver(&ice_driver);
>>       destroy_workqueue(ice_wq);
>> +    ice_debugfs_exit();
>>       pr_info("module unloaded\n");
>>   }
>>   module_exit(ice_module_exit);

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2022-12-09 18:32 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-28 21:47 [Intel-wired-lan] [PATCH net-next 0/5] add v2 FW logging for ice driver Paul M Stillwell Jr
2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 1/5] ice: remove FW logging code Paul M Stillwell Jr
2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 2/5] ice: enable devlink to check FW logging status Paul M Stillwell Jr
2022-12-01  0:39   ` Tony Nguyen
2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 3/5] ice: add ability to query/set FW log level and resolution Paul M Stillwell Jr
2022-11-29 11:48   ` Wilczynski, Michal
2022-11-29 13:30     ` Alexander Lobakin
2022-11-29 21:31       ` Paul M Stillwell Jr
2022-12-01  0:39   ` Tony Nguyen
2022-12-09  0:00     ` Paul M Stillwell Jr
2022-12-01  7:36   ` Paul Menzel
2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 4/5] ice: disable FW logging on driver unload Paul M Stillwell Jr
2022-11-29 12:05   ` Wilczynski, Michal
2022-11-29 21:31     ` Paul M Stillwell Jr
2022-12-01  0:40   ` Tony Nguyen
2022-11-28 21:47 ` [Intel-wired-lan] [PATCH net-next 5/5] ice: use debugfs to output FW log data Paul M Stillwell Jr
2022-11-29 13:53   ` Paul Menzel
2022-11-29 21:28     ` Paul M Stillwell Jr
2022-12-01  0:40   ` Tony Nguyen
2022-12-09 18:32     ` Paul M Stillwell Jr
2022-12-01  0:39 ` [Intel-wired-lan] [PATCH net-next 0/5] add v2 FW logging for ice driver Tony Nguyen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.