* [PATCH net-next v3 1/5] ice: remove FW logging code
2023-08-15 16:57 [PATCH net-next v3 0/5][pull request] add v2 FW logging for ice driver Tony Nguyen
@ 2023-08-15 16:57 ` Tony Nguyen
2023-08-15 18:34 ` Leon Romanovsky
2023-08-15 16:57 ` [PATCH net-next v3 2/5] ice: configure FW logging Tony Nguyen
` (3 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Tony Nguyen @ 2023-08-15 16:57 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, netdev
Cc: Paul M Stillwell Jr, anthony.l.nguyen, jacob.e.keller, horms,
Pucha Himasekhar Reddy
From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
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>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
.../net/ethernet/intel/ice/ice_adminq_cmd.h | 78 -------
drivers/net/ethernet/intel/ice/ice_common.c | 217 ------------------
drivers/net/ethernet/intel/ice/ice_common.h | 1 -
drivers/net/ethernet/intel/ice/ice_main.c | 3 -
drivers/net/ethernet/intel/ice/ice_type.h | 20 --
5 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 29f7a9852aec..45822cb2dd90 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1999,78 +1999,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 {
@@ -2231,8 +2159,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;
@@ -2417,10 +2343,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 a86255b529a0..8feff73bf8be 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -822,216 +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 = kzalloc(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;
- }
- }
-
- kfree(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:
- 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
@@ -1089,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;
@@ -1243,8 +1028,6 @@ void ice_deinit_hw(struct ice_hw *hw)
ice_free_hw_tbls(hw);
mutex_destroy(&hw->tnl_lock);
- /* 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_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index 71b82cdf4a6d..1c95ac1a31cb 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -193,7 +193,6 @@ ice_aq_cfg_lan_txq(struct ice_hw *hw, struct ice_aqc_cfg_txqs_buf *buf,
struct ice_sq_cd *cd);
int ice_replay_vsi(struct ice_hw *hw, u16 vsi_handle);
void ice_replay_post(struct ice_hw *hw);
-void ice_output_fw_log(struct ice_hw *hw, struct ice_aq_desc *desc, void *buf);
struct ice_q_ctx *
ice_get_lan_q_ctx(struct ice_hw *hw, u16 vsi_handle, u8 tc, u16 q_handle);
int ice_sbq_rw_reg(struct ice_hw *hw, struct ice_sbq_msg_input *in);
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 0f04347eda39..19fa5a76c253 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -1530,9 +1530,6 @@ static int __ice_clean_ctrlq(struct ice_pf *pf, enum ice_ctl_q q_type)
ice_vc_process_vf_msg(pf, &event, &data);
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 5e353b0cbe6f..5b82c5d7a291 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -729,24 +729,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:
@@ -880,8 +862,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.38.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v3 1/5] ice: remove FW logging code
2023-08-15 16:57 ` [PATCH net-next v3 1/5] ice: remove FW logging code Tony Nguyen
@ 2023-08-15 18:34 ` Leon Romanovsky
0 siblings, 0 replies; 21+ messages in thread
From: Leon Romanovsky @ 2023-08-15 18:34 UTC (permalink / raw)
To: Tony Nguyen
Cc: davem, kuba, pabeni, edumazet, netdev, Paul M Stillwell Jr,
jacob.e.keller, horms, Pucha Himasekhar Reddy
On Tue, Aug 15, 2023 at 09:57:46AM -0700, Tony Nguyen wrote:
> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>
> 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>
> Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 78 -------
> drivers/net/ethernet/intel/ice/ice_common.c | 217 ------------------
> drivers/net/ethernet/intel/ice/ice_common.h | 1 -
> drivers/net/ethernet/intel/ice/ice_main.c | 3 -
> drivers/net/ethernet/intel/ice/ice_type.h | 20 --
> 5 files changed, 319 deletions(-)
>
Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v3 2/5] ice: configure FW logging
2023-08-15 16:57 [PATCH net-next v3 0/5][pull request] add v2 FW logging for ice driver Tony Nguyen
2023-08-15 16:57 ` [PATCH net-next v3 1/5] ice: remove FW logging code Tony Nguyen
@ 2023-08-15 16:57 ` Tony Nguyen
2023-08-15 18:38 ` Leon Romanovsky
2023-08-15 16:57 ` [PATCH net-next v3 3/5] ice: enable " Tony Nguyen
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Tony Nguyen @ 2023-08-15 16:57 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, netdev
Cc: Paul M Stillwell Jr, anthony.l.nguyen, jacob.e.keller, horms,
Pucha Himasekhar Reddy
From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
Users want the ability to debug FW issues by retrieving the
FW logs from the E8xx devices. Use debugfs to allow the user to
read/write the FW log configuration by adding a 'fwlog/modules' file.
Reading the file will show either the currently running configuration or
the next configuration (if the user has changed the configuration).
Writing to the file will update the configuration, but NOT enable the
configuration (that is a separate command).
To see the status of FW logging then read the 'fwlog/modules' file like
this:
cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
To change the configuration of FW logging then write to the 'fwlog/modules'
file like this:
echo DCB NORMAL > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
The format to change the configuration is
echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device
where
* fwlog_level is a name as described below. Each level includes the
messages from the previous/lower level
* NONE
* ERROR
* WARNING
* NORMAL
* VERBOSE
* fwlog_event is a name that represents the module to receive events for.
The module names are
* GENERAL
* CTRL
* LINK
* LINK_TOPO
* DNL
* I2C
* SDP
* MDIO
* ADMINQ
* HDMA
* LLDP
* DCBX
* DCB
* XLR
* NVM
* AUTH
* VPD
* IOSF
* PARSER
* SW
* SCHEDULER
* TXQ
* RSVD
* POST
* WATCHDOG
* TASK_DISPATCH
* MNG
* SYNCE
* HEALTH
* TSDRV
* PFREG
* MDLVER
* ALL
The name ALL is special and specifies setting all of the modules to the
specified fwlog_level.
If the NVM supports FW logging then the file 'fwlog' will be created
under the PCI device ID for the ice driver. If the file does not exist
then either the NVM doesn't support FW logging or debugfs is not enabled
on the system.
In addition to configuring the modules, the user can also configure the
number of log messages (resolution) to include in a single Admin Receive
Queue (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.
To see/change the resolution the user can read/write the
'fwlog/resolution' file. An example changing the value to 50 is
echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/Makefile | 4 +-
drivers/net/ethernet/intel/ice/ice.h | 18 +
.../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
drivers/net/ethernet/intel/ice/ice_common.c | 5 +
drivers/net/ethernet/intel/ice/ice_debugfs.c | 450 ++++++++++++++++++
drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
drivers/net/ethernet/intel/ice/ice_main.c | 21 +
drivers/net/ethernet/intel/ice/ice_type.h | 4 +
9 files changed, 867 insertions(+), 1 deletion(-)
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
diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
index 960277d78e09..d43a59e5f8ee 100644
--- a/drivers/net/ethernet/intel/ice/Makefile
+++ b/drivers/net/ethernet/intel/ice/Makefile
@@ -34,7 +34,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 \
@@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
ice-$(CONFIG_GNSS) += ice_gnss.o
+ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 5ac0ad12f9f1..e6dd9f6f9eee 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -556,6 +556,8 @@ 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 */
+ struct dentry *ice_debugfs_pf;
+ struct dentry *ice_debugfs_pf_fwlog;
struct ice_vfs vfs;
DECLARE_BITMAP(features, ICE_F_MAX);
DECLARE_BITMAP(state, ICE_STATE_NBITS);
@@ -861,6 +863,22 @@ 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);
+void ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module);
+#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) { }
+static void
+ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module)
+{
+ return -EOPNOTSUPP;
+}
+#endif /* CONFIG_DEBUG_FS */
+
bool netif_is_ice(const 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 45822cb2dd90..facd662a2768 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -2084,6 +2084,81 @@ 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)
+ * Query FW Logging (indirect 0xFF32)
+ */
+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_QUERY BIT(2)
+
+ u8 rsp_flag;
+ __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
@@ -2161,6 +2236,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;
@@ -2343,6 +2419,10 @@ 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_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 8feff73bf8be..7bfd965b7eca 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -879,6 +879,11 @@ int ice_init_hw(struct ice_hw *hw)
if (status)
goto err_unroll_cqinit;
+ 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)
goto err_unroll_cqinit;
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..e354c7287ff6
--- /dev/null
+++ b/drivers/net/ethernet/intel/ice/ice_debugfs.c
@@ -0,0 +1,450 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022, Intel Corporation. */
+
+#include <linux/fs.h>
+#include <linux/debugfs.h>
+#include <linux/random.h>
+#include <linux/vmalloc.h>
+#include "ice.h"
+
+static struct dentry *ice_debugfs_root;
+
+/* the ordering in this array is important. it matches the ordering of the
+ * values in the FW so the index is the same value as in ice_aqc_fw_logging_mod
+ */
+static const char * const ice_fwlog_module_string[] = {
+ "GENERAL",
+ "CTRL",
+ "LINK",
+ "LINK_TOPO",
+ "DNL",
+ "I2C",
+ "SDP",
+ "MDIO",
+ "ADMINQ",
+ "HDMA",
+ "LLDP",
+ "DCBX",
+ "DCB",
+ "XLR",
+ "NVM",
+ "AUTH",
+ "VPD",
+ "IOSF",
+ "PARSER",
+ "SW",
+ "SCHEDULER",
+ "TXQ",
+ "RSVD",
+ "POST",
+ "WATCHDOG",
+ "TASK_DISPATCH",
+ "MNG",
+ "SYNCE",
+ "HEALTH",
+ "TSDRV",
+ "PFREG",
+ "MDLVER",
+ "ALL",
+};
+
+/* the ordering in this array is important. it matches the ordering of the
+ * values in the FW so the index is the same value as in ice_fwlog_level
+ */
+static const char * const ice_fwlog_level_string[] = {
+ "NONE",
+ "ERROR",
+ "WARNING",
+ "NORMAL",
+ "VERBOSE",
+};
+
+static void ice_print_fwlog_config(struct ice_hw *hw, struct ice_fwlog_cfg *cfg,
+ char **buff, int *size)
+{
+ char *tmp = *buff;
+ int used = *size;
+ u16 i, len;
+
+ len = snprintf(tmp, used, "Log_resolution: %d\n", cfg->log_resolution);
+ tmp = tmp + len;
+ used -= len;
+ len = snprintf(tmp, used, "Options: 0x%04x\n", cfg->options);
+ tmp = tmp + len;
+ used -= len;
+ len = snprintf(tmp, used, "\tarq_ena: %s\n",
+ (cfg->options &
+ ICE_FWLOG_OPTION_ARQ_ENA) ? "true" : "false");
+ tmp = tmp + len;
+ used -= len;
+ len = snprintf(tmp, used, "\tuart_ena: %s\n",
+ (cfg->options &
+ ICE_FWLOG_OPTION_UART_ENA) ? "true" : "false");
+ tmp = tmp + len;
+ used -= len;
+ len = snprintf(tmp, used, "\trunning: %s\n",
+ (cfg->options &
+ ICE_FWLOG_OPTION_IS_REGISTERED) ? "true" : "false");
+ tmp = tmp + len;
+ used -= len;
+ len = snprintf(tmp, used, "Module Entries:\n");
+ tmp = tmp + len;
+ used -= len;
+
+ for (i = 0; i < ICE_AQC_FW_LOG_ID_MAX; i++) {
+ struct ice_fwlog_module_entry *entry =
+ &cfg->module_entries[i];
+
+ len = snprintf(tmp, used, "\tModule: %s, Log Level: %s\n",
+ ice_fwlog_module_string[entry->module_id],
+ ice_fwlog_level_string[entry->log_level]);
+ tmp = tmp + len;
+ used -= len;
+ }
+
+ len = snprintf(tmp, used, "Valid log levels:\n");
+ tmp = tmp + len;
+ used -= len;
+
+ for (i = 0; i < ICE_FWLOG_LEVEL_INVALID; i++) {
+ len = snprintf(tmp, used, "\t%s\n", ice_fwlog_level_string[i]);
+ tmp = tmp + len;
+ used -= len;
+ }
+
+ *buff = tmp;
+ *size = used;
+}
+
+/**
+ * ice_fwlog_dump_cfg - Dump current FW logging configuration
+ * @hw: pointer to the HW structure
+ * @buff: pointer to a buffer to hold the config strings
+ * @buff_size: size of the buffer in bytes
+ */
+static void ice_fwlog_dump_cfg(struct ice_hw *hw, char *buff, int buff_size)
+{
+ int len;
+
+ len = snprintf(buff, buff_size, "FWLOG Configuration:\n");
+ buff = buff + len;
+ buff_size -= len;
+
+ ice_print_fwlog_config(hw, &hw->fwlog_cfg, &buff, &buff_size);
+}
+
+/**
+ * ice_debugfs_parse_cmd_line - Parse the command line that was passed in
+ * @src: pointer to a buffer holding the command line
+ * @len: size of the buffer in bytes
+ * @argv: pointer to store the command line items
+ * @argc: pointer to store the number of command line items
+ */
+static ssize_t ice_debugfs_parse_cmd_line(const char __user *src, size_t len,
+ char ***argv, int *argc)
+{
+ char *cmd_buf, *cmd_buf_tmp;
+
+ cmd_buf = memdup_user(src, len + 1);
+ if (IS_ERR(cmd_buf))
+ return PTR_ERR(cmd_buf);
+ cmd_buf[len] = '\0';
+
+ /* the cmd_buf has a newline at the end of the command so
+ * remove it
+ */
+ cmd_buf_tmp = strchr(cmd_buf, '\n');
+ if (cmd_buf_tmp) {
+ *cmd_buf_tmp = '\0';
+ len = (size_t)cmd_buf_tmp - (size_t)cmd_buf + 1;
+ }
+
+ *argv = argv_split(GFP_KERNEL, cmd_buf, argc);
+ if (!*argv)
+ return -ENOMEM;
+
+ kfree(cmd_buf);
+ return 0;
+}
+
+/**
+ * ice_debugfs_module_read - read from 'module' file
+ * @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_module_read(struct file *filp, char __user *buffer,
+ size_t count, loff_t *ppos)
+{
+ struct ice_pf *pf = filp->private_data;
+ char *data = NULL;
+ int status;
+
+ /* don't allow commands if the FW doesn't support it */
+ if (!ice_fwlog_supported(&pf->hw))
+ return -EOPNOTSUPP;
+
+ data = vzalloc(ICE_AQ_MAX_BUF_LEN);
+ if (!data) {
+ dev_warn(ice_pf_to_dev(pf), "Unable to allocate memory for FW configuration!\n");
+ return -ENOMEM;
+ }
+
+ ice_fwlog_dump_cfg(&pf->hw, data, ICE_AQ_MAX_BUF_LEN);
+
+ if (count < strlen(data))
+ return -ENOSPC;
+
+ status = simple_read_from_buffer(buffer, count, ppos, data,
+ strlen(data));
+ vfree(data);
+
+ return status;
+}
+
+/**
+ * ice_debugfs_module_write - write into 'module' file
+ * @filp: the opened file
+ * @buf: where to find the user's data
+ * @count: the length of the user's data
+ * @ppos: file position offset
+ */
+static ssize_t
+ice_debugfs_module_write(struct file *filp, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct ice_pf *pf = filp->private_data;
+ struct device *dev = ice_pf_to_dev(pf);
+ ssize_t ret;
+ char **argv;
+ int argc;
+
+ /* don't allow commands if the FW doesn't support it */
+ if (!ice_fwlog_supported(&pf->hw))
+ return -EOPNOTSUPP;
+
+ /* don't allow partial writes */
+ if (*ppos != 0)
+ return 0;
+
+ ret = ice_debugfs_parse_cmd_line(buf, count, &argv, &argc);
+ if (ret)
+ goto err_copy_from_user;
+
+ if (argc == 2) {
+ int module, log_level;
+
+ module = sysfs_match_string(ice_fwlog_module_string, argv[0]);
+ if (module < 0) {
+ dev_info(dev, "unknown module '%s'\n", argv[0]);
+ ret = -EINVAL;
+ goto module_write_error;
+ }
+
+ log_level = sysfs_match_string(ice_fwlog_level_string, argv[1]);
+ if (log_level < 0) {
+ dev_info(dev, "unknown log level '%s'\n", argv[1]);
+ ret = -EINVAL;
+ goto module_write_error;
+ }
+
+ /* module is valid because it was range checked using
+ * sysfs_match_string()
+ */
+ if (module != ICE_AQC_FW_LOG_ID_MAX) {
+ ice_pf_fwlog_update_module(pf, log_level, module);
+ } else {
+ /* the module 'ALL' is a shortcut so that we can set
+ * all of the modules to the same level quickly
+ */
+ int i;
+
+ for (i = 0; i < ICE_AQC_FW_LOG_ID_MAX; i++)
+ ice_pf_fwlog_update_module(pf, log_level, i);
+ }
+ } else {
+ dev_info(dev, "unknown or invalid command '%s'\n", argv[0]);
+ ret = -EINVAL;
+ goto module_write_error;
+ }
+
+ /* if we get here, nothing went wrong; return bytes copied */
+ ret = (ssize_t)count;
+
+module_write_error:
+ argv_free(argv);
+err_copy_from_user:
+ /* This function always consumes all of the written input, or produces
+ * an error. Check and enforce this. Otherwise, the write operation
+ * won't complete properly.
+ */
+ if (WARN_ON(ret != (ssize_t)count && ret >= 0))
+ ret = -EIO;
+
+ return ret;
+}
+
+static const struct file_operations ice_debugfs_module_fops = {
+ .owner = THIS_MODULE,
+ .open = simple_open,
+ .read = ice_debugfs_module_read,
+ .write = ice_debugfs_module_write,
+};
+
+/**
+ * ice_debugfs_resolution_read - read from 'resolution' file
+ * @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_resolution_read(struct file *filp,
+ char __user *buffer, size_t count,
+ loff_t *ppos)
+{
+ struct ice_pf *pf = filp->private_data;
+ struct ice_hw *hw = &pf->hw;
+ char buff[32] = {};
+ int status;
+
+ /* don't allow commands if the FW doesn't support it */
+ if (!ice_fwlog_supported(&pf->hw))
+ return -EOPNOTSUPP;
+
+ snprintf(buff, sizeof(buff), "%d\n",
+ hw->fwlog_cfg.log_resolution);
+
+ status = simple_read_from_buffer(buffer, count, ppos, buff,
+ strlen(buff));
+
+ return status;
+}
+
+/**
+ * ice_debugfs_resolution_write - write into 'resolution' file
+ * @filp: the opened file
+ * @buf: where to find the user's data
+ * @count: the length of the user's data
+ * @ppos: file position offset
+ */
+static ssize_t
+ice_debugfs_resolution_write(struct file *filp, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct ice_pf *pf = filp->private_data;
+ struct device *dev = ice_pf_to_dev(pf);
+ struct ice_hw *hw = &pf->hw;
+ ssize_t ret;
+ char **argv;
+ int argc;
+
+ /* don't allow commands if the FW doesn't support it */
+ if (!ice_fwlog_supported(hw))
+ return -EOPNOTSUPP;
+
+ /* don't allow partial writes */
+ if (*ppos != 0)
+ return 0;
+
+ ret = ice_debugfs_parse_cmd_line(buf, count, &argv, &argc);
+ if (ret)
+ goto err_copy_from_user;
+
+ if (argc == 1) {
+ s16 resolution;
+
+ ret = kstrtos16(argv[0], 0, &resolution);
+ if (ret)
+ goto resolution_write_error;
+
+ if (resolution < ICE_AQC_FW_LOG_MIN_RESOLUTION ||
+ resolution > ICE_AQC_FW_LOG_MAX_RESOLUTION) {
+ dev_err(dev, "Invalid FW log resolution %d, value must be between %d - %d\n",
+ resolution, ICE_AQC_FW_LOG_MIN_RESOLUTION,
+ ICE_AQC_FW_LOG_MAX_RESOLUTION);
+ ret = -EINVAL;
+ goto resolution_write_error;
+ }
+
+ hw->fwlog_cfg.log_resolution = resolution;
+ } else {
+ dev_info(dev, "unknown or invalid command '%s'\n", argv[0]);
+ ret = -EINVAL;
+ goto resolution_write_error;
+ }
+
+ /* if we get here, nothing went wrong; return bytes copied */
+ ret = (ssize_t)count;
+
+resolution_write_error:
+ argv_free(argv);
+err_copy_from_user:
+ /* This function always consumes all of the written input, or produces
+ * an error. Check and enforce this. Otherwise, the write operation
+ * won't complete properly.
+ */
+ if (WARN_ON(ret != (ssize_t)count && ret >= 0))
+ ret = -EIO;
+
+ return ret;
+}
+
+static const struct file_operations ice_debugfs_resolution_fops = {
+ .owner = THIS_MODULE,
+ .open = simple_open,
+ .read = ice_debugfs_resolution_read,
+ .write = ice_debugfs_resolution_write,
+};
+
+/**
+ * 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);
+
+ /* 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)) {
+ pr_info("init of debugfs PCI dir failed\n");
+ return;
+ }
+
+ pf->ice_debugfs_pf_fwlog = debugfs_create_dir("fwlog",
+ pf->ice_debugfs_pf);
+ if (IS_ERR(pf->ice_debugfs_pf)) {
+ pr_info("init of debugfs fwlog dir failed\n");
+ return;
+ }
+
+ debugfs_create_file("modules", 0600, pf->ice_debugfs_pf_fwlog,
+ pf, &ice_debugfs_module_fops);
+
+ debugfs_create_file("resolution", 0600,
+ pf->ice_debugfs_pf_fwlog, pf,
+ &ice_debugfs_resolution_fops);
+}
+
+/**
+ * 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_fwlog.c b/drivers/net/ethernet/intel/ice/ice_fwlog.c
new file mode 100644
index 000000000000..1f4b474dcc97
--- /dev/null
+++ b/drivers/net/ethernet/intel/ice/ice_fwlog.c
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022, Intel Corporation. */
+
+#include "ice.h"
+#include "ice_common.h"
+#include "ice_fwlog.h"
+
+/**
+ * ice_fwlog_init - Initialize FW logging configuration
+ * @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_supported(hw);
+
+ if (ice_fwlog_supported(hw)) {
+ /* read the current config from the FW and store it */
+ status = ice_fwlog_get(hw, &hw->fwlog_cfg);
+ if (status)
+ return status;
+
+ ice_debugfs_fwlog_init(hw->back);
+ } else {
+ dev_warn(ice_hw_to_dev(hw), "FW logging is not supported in this NVM image. Please update the NVM to get FW log support\n");
+ }
+
+ return 0;
+}
+
+/**
+ * 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_supported;
+}
+
+/**
+ * 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;
+ int 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_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)
+{
+ if (!ice_fwlog_supported(hw))
+ return -EOPNOTSUPP;
+
+ return ice_aq_fwlog_set(hw, cfg->module_entries,
+ ICE_AQC_FW_LOG_ID_MAX, cfg->options,
+ cfg->log_resolution);
+}
+
+/**
+ * ice_aq_fwlog_get - Get the current firmware logging configuration (0xFF32)
+ * @hw: pointer to the HW structure
+ * @cfg: firmware logging configuration to populate
+ */
+static int ice_aq_fwlog_get(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
+{
+ struct ice_aqc_fw_log_cfg_resp *fw_modules;
+ struct ice_aqc_fw_log *cmd;
+ struct ice_aq_desc desc;
+ u16 module_id_cnt;
+ int status;
+ void *buf;
+ int i;
+
+ memset(cfg, 0, sizeof(*cfg));
+
+ buf = kzalloc(ICE_AQ_MAX_BUF_LEN, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_fw_logs_query);
+ cmd = &desc.params.fw_log;
+
+ cmd->cmd_flags = ICE_AQC_FW_LOG_AQ_QUERY;
+
+ status = ice_aq_send_cmd(hw, &desc, buf, ICE_AQ_MAX_BUF_LEN, NULL);
+ if (status) {
+ ice_debug(hw, ICE_DBG_FW_LOG, "Failed to get FW log configuration\n");
+ goto status_out;
+ }
+
+ module_id_cnt = le16_to_cpu(cmd->ops.cfg.mdl_cnt);
+ if (module_id_cnt < ICE_AQC_FW_LOG_ID_MAX) {
+ ice_debug(hw, ICE_DBG_FW_LOG, "FW returned less than the expected number of FW log module IDs\n");
+ } else if (module_id_cnt > ICE_AQC_FW_LOG_ID_MAX) {
+ ice_debug(hw, ICE_DBG_FW_LOG, "FW returned more than expected number of FW log module IDs, setting module_id_cnt to software expected max %u\n",
+ ICE_AQC_FW_LOG_ID_MAX);
+ module_id_cnt = ICE_AQC_FW_LOG_ID_MAX;
+ }
+
+ 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;
+
+ 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_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)
+{
+ if (!ice_fwlog_supported(hw))
+ return -EOPNOTSUPP;
+
+ if (!cfg)
+ return -EINVAL;
+
+ return ice_aq_fwlog_get(hw, cfg);
+}
+
+/**
+ * ice_fwlog_set_supported - Set if FW logging is supported by FW
+ * @hw: pointer to the HW struct
+ *
+ * If FW returns success to the ice_aq_fwlog_get call then it supports FW
+ * logging, else it doesn't. Set the fwlog_supported flag accordingly.
+ *
+ * This function is only meant to be called during driver init to determine if
+ * the FW support FW logging.
+ */
+void ice_fwlog_set_supported(struct ice_hw *hw)
+{
+ struct ice_fwlog_cfg *cfg;
+ int status;
+
+ hw->fwlog_supported = false;
+
+ cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
+ if (!cfg)
+ return;
+
+ /* don't call ice_fwlog_get() because that would check to see if FW
+ * logging is supported which is what the driver is determining now
+ */
+ status = ice_aq_fwlog_get(hw, cfg);
+ if (status)
+ ice_debug(hw, ICE_DBG_FW_LOG, "ice_aq_fwlog_get failed, FW logging is not supported on this version of FW, status %d\n",
+ status);
+ else
+ hw->fwlog_supported = true;
+
+ kfree(cfg);
+}
diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.h b/drivers/net/ethernet/intel/ice/ice_fwlog.h
new file mode 100644
index 000000000000..5a4194527cf9
--- /dev/null
+++ b/drivers/net/ethernet/intel/ice/ice_fwlog.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2022, Intel Corporation. */
+
+#ifndef _ICE_FWLOG_H_
+#define _ICE_FWLOG_H_
+#include "ice_adminq_cmd.h"
+
+struct ice_hw;
+
+/* Only a single log level should be set and all log levels under the set value
+ * are enabled, e.g. if log level is set to ICE_FW_LOG_LEVEL_VERBOSE, then all
+ * other log levels are included (except ICE_FW_LOG_LEVEL_NONE)
+ */
+enum ice_fwlog_level {
+ ICE_FWLOG_LEVEL_NONE = 0,
+ ICE_FWLOG_LEVEL_ERROR = 1,
+ ICE_FWLOG_LEVEL_WARNING = 2,
+ ICE_FWLOG_LEVEL_NORMAL = 3,
+ ICE_FWLOG_LEVEL_VERBOSE = 4,
+ ICE_FWLOG_LEVEL_INVALID, /* all values >= this entry are invalid */
+};
+
+struct ice_fwlog_module_entry {
+ /* module ID for the corresponding firmware logging event */
+ u16 module_id;
+ /* verbosity level for the module_id */
+ u8 log_level;
+};
+
+struct ice_fwlog_cfg {
+ /* list of modules for configuring log level */
+ struct ice_fwlog_module_entry module_entries[ICE_AQC_FW_LOG_ID_MAX];
+ /* options used to configure firmware logging */
+ u16 options;
+#define ICE_FWLOG_OPTION_ARQ_ENA BIT(0)
+#define ICE_FWLOG_OPTION_UART_ENA BIT(1)
+ /* set before calling ice_fwlog_init() so the PF registers for firmware
+ * logging on initialization
+ */
+#define ICE_FWLOG_OPTION_REGISTER_ON_INIT BIT(2)
+ /* set in the ice_fwlog_get() response if the PF is registered for FW
+ * logging events over ARQ
+ */
+#define ICE_FWLOG_OPTION_IS_REGISTERED BIT(3)
+
+ /* minimum number of log events sent per Admin Receive Queue event */
+ u16 log_resolution;
+};
+
+void ice_fwlog_set_supported(struct ice_hw *hw);
+bool ice_fwlog_supported(struct ice_hw *hw);
+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);
+#endif /* _ICE_FWLOG_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 19fa5a76c253..73c7dcfa8108 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4354,6 +4354,22 @@ static void ice_print_wake_reason(struct ice_pf *pf)
dev_info(ice_pf_to_dev(pf), "Wake reason: %s", wake_str);
}
+/**
+ * ice_pf_fwlog_update_module - update 1 module
+ * @pf: pointer to the PF struct
+ * @log_level: log_level to use for the @module
+ * @module: module to update
+ */
+void ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module)
+{
+ struct ice_fwlog_module_entry *entries;
+ struct ice_hw *hw = &pf->hw;
+
+ entries = (struct ice_fwlog_module_entry *)hw->fwlog_cfg.module_entries;
+
+ entries[module].log_level = log_level;
+}
+
/**
* ice_register_netdev - register netdev
* @vsi: pointer to the VSI struct
@@ -5174,6 +5190,8 @@ static void ice_remove(struct pci_dev *pdev)
msleep(100);
}
+ ice_debugfs_exit();
+
if (test_bit(ICE_FLAG_SRIOV_ENA, pf->flags)) {
set_bit(ICE_VF_RESETS_DISABLED, pf->state);
ice_free_vfs(pf);
@@ -5635,6 +5653,8 @@ static int __init ice_module_init(void)
goto err_dest_wq;
}
+ ice_debugfs_init();
+
status = pci_register_driver(&ice_driver);
if (status) {
pr_err("failed to register PCI driver, err %d\n", status);
@@ -5644,6 +5664,7 @@ static int __init ice_module_init(void)
return 0;
err_dest_lag_wq:
+ ice_debugfs_exit();
destroy_workqueue(ice_lag_wq);
err_dest_wq:
destroy_workqueue(ice_wq);
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 5b82c5d7a291..16904cb8a25b 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)
{
@@ -862,6 +863,9 @@ struct ice_hw {
u8 fw_patch; /* firmware patch version */
u32 fw_build; /* firmware build number */
+ struct ice_fwlog_cfg fwlog_cfg;
+ bool fwlog_supported; /* does hardware support FW logging? */
+
/* Device max aggregate bandwidths corresponding to the GL_PWR_MODE_CTL
* register. Used for determining the ITR/INTRL granularity during
* initialization.
--
2.38.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v3 2/5] ice: configure FW logging
2023-08-15 16:57 ` [PATCH net-next v3 2/5] ice: configure FW logging Tony Nguyen
@ 2023-08-15 18:38 ` Leon Romanovsky
2023-08-17 21:25 ` Paul M Stillwell Jr
2023-08-23 22:23 ` Paul M Stillwell Jr
0 siblings, 2 replies; 21+ messages in thread
From: Leon Romanovsky @ 2023-08-15 18:38 UTC (permalink / raw)
To: Tony Nguyen
Cc: davem, kuba, pabeni, edumazet, netdev, Paul M Stillwell Jr,
jacob.e.keller, horms, Pucha Himasekhar Reddy
On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>
> Users want the ability to debug FW issues by retrieving the
> FW logs from the E8xx devices. Use debugfs to allow the user to
> read/write the FW log configuration by adding a 'fwlog/modules' file.
> Reading the file will show either the currently running configuration or
> the next configuration (if the user has changed the configuration).
> Writing to the file will update the configuration, but NOT enable the
> configuration (that is a separate command).
>
> To see the status of FW logging then read the 'fwlog/modules' file like
> this:
>
> cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>
> To change the configuration of FW logging then write to the 'fwlog/modules'
> file like this:
>
> echo DCB NORMAL > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>
> The format to change the configuration is
>
> echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device
This line is truncated, it is not clear where you are writing.
And more general question, a long time ago, netdev had a policy of
not-allowing writing to debugfs, was it changed?
>
> where
>
> * fwlog_level is a name as described below. Each level includes the
> messages from the previous/lower level
>
> * NONE
> * ERROR
> * WARNING
> * NORMAL
> * VERBOSE
>
> * fwlog_event is a name that represents the module to receive events for.
> The module names are
>
> * GENERAL
> * CTRL
> * LINK
> * LINK_TOPO
> * DNL
> * I2C
> * SDP
> * MDIO
> * ADMINQ
> * HDMA
> * LLDP
> * DCBX
> * DCB
> * XLR
> * NVM
> * AUTH
> * VPD
> * IOSF
> * PARSER
> * SW
> * SCHEDULER
> * TXQ
> * RSVD
> * POST
> * WATCHDOG
> * TASK_DISPATCH
> * MNG
> * SYNCE
> * HEALTH
> * TSDRV
> * PFREG
> * MDLVER
> * ALL
>
> The name ALL is special and specifies setting all of the modules to the
> specified fwlog_level.
>
> If the NVM supports FW logging then the file 'fwlog' will be created
> under the PCI device ID for the ice driver. If the file does not exist
> then either the NVM doesn't support FW logging or debugfs is not enabled
> on the system.
>
> In addition to configuring the modules, the user can also configure the
> number of log messages (resolution) to include in a single Admin Receive
> Queue (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.
>
> To see/change the resolution the user can read/write the
> 'fwlog/resolution' file. An example changing the value to 50 is
>
> echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
>
> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
> drivers/net/ethernet/intel/ice/Makefile | 4 +-
> drivers/net/ethernet/intel/ice/ice.h | 18 +
> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
> drivers/net/ethernet/intel/ice/ice_common.c | 5 +
> drivers/net/ethernet/intel/ice/ice_debugfs.c | 450 ++++++++++++++++++
> drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
> drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
> drivers/net/ethernet/intel/ice/ice_main.c | 21 +
> drivers/net/ethernet/intel/ice/ice_type.h | 4 +
> 9 files changed, 867 insertions(+), 1 deletion(-)
> 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
>
> diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
> index 960277d78e09..d43a59e5f8ee 100644
> --- a/drivers/net/ethernet/intel/ice/Makefile
> +++ b/drivers/net/ethernet/intel/ice/Makefile
> @@ -34,7 +34,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 \
> @@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
> ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
> ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
> ice-$(CONFIG_GNSS) += ice_gnss.o
> +ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 5ac0ad12f9f1..e6dd9f6f9eee 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -556,6 +556,8 @@ 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 */
> + struct dentry *ice_debugfs_pf;
> + struct dentry *ice_debugfs_pf_fwlog;
> struct ice_vfs vfs;
> DECLARE_BITMAP(features, ICE_F_MAX);
> DECLARE_BITMAP(state, ICE_STATE_NBITS);
> @@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct ice_pf *pf)
> return false;
> }
>
> +#ifdef CONFIG_DEBUG_FS
There is no need in this CONFIG_DEBUG_FS and code should be written
without debugfs stubs.
> +void ice_debugfs_fwlog_init(struct ice_pf *pf);
> +void ice_debugfs_init(void);
> +void ice_debugfs_exit(void);
> +void ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module);
> +#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) { }
> +static void
> +ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module)
> +{
> + return -EOPNOTSUPP;
> +}
> +#endif /* CONFIG_DEBUG_FS */
Thanks
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v3 2/5] ice: configure FW logging
2023-08-15 18:38 ` Leon Romanovsky
@ 2023-08-17 21:25 ` Paul M Stillwell Jr
2023-08-18 11:10 ` Leon Romanovsky
2023-08-23 22:23 ` Paul M Stillwell Jr
1 sibling, 1 reply; 21+ messages in thread
From: Paul M Stillwell Jr @ 2023-08-17 21:25 UTC (permalink / raw)
To: Leon Romanovsky, Tony Nguyen
Cc: davem, kuba, pabeni, edumazet, netdev, jacob.e.keller, horms,
Pucha Himasekhar Reddy
On 8/15/2023 11:38 AM, Leon Romanovsky wrote:
> On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
>> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>
>> Users want the ability to debug FW issues by retrieving the
>> FW logs from the E8xx devices. Use debugfs to allow the user to
>> read/write the FW log configuration by adding a 'fwlog/modules' file.
>> Reading the file will show either the currently running configuration or
>> the next configuration (if the user has changed the configuration).
>> Writing to the file will update the configuration, but NOT enable the
>> configuration (that is a separate command).
>>
>> To see the status of FW logging then read the 'fwlog/modules' file like
>> this:
>>
>> cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>
>> To change the configuration of FW logging then write to the 'fwlog/modules'
>> file like this:
>>
>> echo DCB NORMAL > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>
>> The format to change the configuration is
>>
>> echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device
>
> This line is truncated, it is not clear where you are writing.
Good catch, I don't know how I missed this... Will fix
> And more general question, a long time ago, netdev had a policy of
> not-allowing writing to debugfs, was it changed?
>
I had this same thought and it seems like there were 2 concerns in the past
1. Having a single file that was read/write with lots of commands going
through it
2. Having code in the driver to parse the text from the commands that
was error/security prone
We have addressed this in 2 ways:
1. Split the commands into multiple files that have a single purpose
2. Use kernel parsing functions for anything where we *have* to pass
text to decode
>>
>> where
>>
>> * fwlog_level is a name as described below. Each level includes the
>> messages from the previous/lower level
>>
>> * NONE
>> * ERROR
>> * WARNING
>> * NORMAL
>> * VERBOSE
>>
>> * fwlog_event is a name that represents the module to receive events for.
>> The module names are
>>
>> * GENERAL
>> * CTRL
>> * LINK
>> * LINK_TOPO
>> * DNL
>> * I2C
>> * SDP
>> * MDIO
>> * ADMINQ
>> * HDMA
>> * LLDP
>> * DCBX
>> * DCB
>> * XLR
>> * NVM
>> * AUTH
>> * VPD
>> * IOSF
>> * PARSER
>> * SW
>> * SCHEDULER
>> * TXQ
>> * RSVD
>> * POST
>> * WATCHDOG
>> * TASK_DISPATCH
>> * MNG
>> * SYNCE
>> * HEALTH
>> * TSDRV
>> * PFREG
>> * MDLVER
>> * ALL
>>
>> The name ALL is special and specifies setting all of the modules to the
>> specified fwlog_level.
>>
>> If the NVM supports FW logging then the file 'fwlog' will be created
>> under the PCI device ID for the ice driver. If the file does not exist
>> then either the NVM doesn't support FW logging or debugfs is not enabled
>> on the system.
>>
>> In addition to configuring the modules, the user can also configure the
>> number of log messages (resolution) to include in a single Admin Receive
>> Queue (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.
>>
>> To see/change the resolution the user can read/write the
>> 'fwlog/resolution' file. An example changing the value to 50 is
>>
>> echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
>>
>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>> Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>> ---
>> drivers/net/ethernet/intel/ice/Makefile | 4 +-
>> drivers/net/ethernet/intel/ice/ice.h | 18 +
>> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
>> drivers/net/ethernet/intel/ice/ice_common.c | 5 +
>> drivers/net/ethernet/intel/ice/ice_debugfs.c | 450 ++++++++++++++++++
>> drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
>> drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
>> drivers/net/ethernet/intel/ice/ice_main.c | 21 +
>> drivers/net/ethernet/intel/ice/ice_type.h | 4 +
>> 9 files changed, 867 insertions(+), 1 deletion(-)
>> 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
>>
>> diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
>> index 960277d78e09..d43a59e5f8ee 100644
>> --- a/drivers/net/ethernet/intel/ice/Makefile
>> +++ b/drivers/net/ethernet/intel/ice/Makefile
>> @@ -34,7 +34,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 \
>> @@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
>> ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
>> ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
>> ice-$(CONFIG_GNSS) += ice_gnss.o
>> +ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
>> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
>> index 5ac0ad12f9f1..e6dd9f6f9eee 100644
>> --- a/drivers/net/ethernet/intel/ice/ice.h
>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>> @@ -556,6 +556,8 @@ 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 */
>> + struct dentry *ice_debugfs_pf;
>> + struct dentry *ice_debugfs_pf_fwlog;
>> struct ice_vfs vfs;
>> DECLARE_BITMAP(features, ICE_F_MAX);
>> DECLARE_BITMAP(state, ICE_STATE_NBITS);
>> @@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct ice_pf *pf)
>> return false;
>> }
>>
>> +#ifdef CONFIG_DEBUG_FS
>
> There is no need in this CONFIG_DEBUG_FS and code should be written
> without debugfs stubs.
>
I don't understand this comment... If the kernel is configured *without*
debugfs, won't the kernel fail to compile due to missing functions if we
don't do this?
>> +void ice_debugfs_fwlog_init(struct ice_pf *pf);
>> +void ice_debugfs_init(void);
>> +void ice_debugfs_exit(void);
>> +void ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module);
>> +#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) { }
>> +static void
>> +ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +#endif /* CONFIG_DEBUG_FS */
>
> Thanks
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v3 2/5] ice: configure FW logging
2023-08-17 21:25 ` Paul M Stillwell Jr
@ 2023-08-18 11:10 ` Leon Romanovsky
2023-08-18 12:31 ` Przemek Kitszel
0 siblings, 1 reply; 21+ messages in thread
From: Leon Romanovsky @ 2023-08-18 11:10 UTC (permalink / raw)
To: Paul M Stillwell Jr
Cc: Tony Nguyen, davem, kuba, pabeni, edumazet, netdev,
jacob.e.keller, horms, Pucha Himasekhar Reddy
On Thu, Aug 17, 2023 at 02:25:34PM -0700, Paul M Stillwell Jr wrote:
> On 8/15/2023 11:38 AM, Leon Romanovsky wrote:
> > On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
> > > From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> > >
> > > Users want the ability to debug FW issues by retrieving the
> > > FW logs from the E8xx devices. Use debugfs to allow the user to
> > > read/write the FW log configuration by adding a 'fwlog/modules' file.
> > > Reading the file will show either the currently running configuration or
> > > the next configuration (if the user has changed the configuration).
> > > Writing to the file will update the configuration, but NOT enable the
> > > configuration (that is a separate command).
> > >
> > > To see the status of FW logging then read the 'fwlog/modules' file like
> > > this:
> > >
> > > cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
> > >
> > > To change the configuration of FW logging then write to the 'fwlog/modules'
> > > file like this:
> > >
> > > echo DCB NORMAL > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
> > >
> > > The format to change the configuration is
> > >
> > > echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device
> >
> > This line is truncated, it is not clear where you are writing.
>
> Good catch, I don't know how I missed this... Will fix
>
> > And more general question, a long time ago, netdev had a policy of
> > not-allowing writing to debugfs, was it changed?
> >
>
> I had this same thought and it seems like there were 2 concerns in the past
Maybe, I'm not enough time in netdev world to know the history.
>
> 1. Having a single file that was read/write with lots of commands going
> through it
> 2. Having code in the driver to parse the text from the commands that was
> error/security prone
>
> We have addressed this in 2 ways:
> 1. Split the commands into multiple files that have a single purpose
> 2. Use kernel parsing functions for anything where we *have* to pass text to
> decode
>
> > >
> > > where
> > >
> > > * fwlog_level is a name as described below. Each level includes the
> > > messages from the previous/lower level
> > >
> > > * NONE
> > > * ERROR
> > > * WARNING
> > > * NORMAL
> > > * VERBOSE
> > >
> > > * fwlog_event is a name that represents the module to receive events for.
> > > The module names are
> > >
> > > * GENERAL
> > > * CTRL
> > > * LINK
> > > * LINK_TOPO
> > > * DNL
> > > * I2C
> > > * SDP
> > > * MDIO
> > > * ADMINQ
> > > * HDMA
> > > * LLDP
> > > * DCBX
> > > * DCB
> > > * XLR
> > > * NVM
> > > * AUTH
> > > * VPD
> > > * IOSF
> > > * PARSER
> > > * SW
> > > * SCHEDULER
> > > * TXQ
> > > * RSVD
> > > * POST
> > > * WATCHDOG
> > > * TASK_DISPATCH
> > > * MNG
> > > * SYNCE
> > > * HEALTH
> > > * TSDRV
> > > * PFREG
> > > * MDLVER
> > > * ALL
> > >
> > > The name ALL is special and specifies setting all of the modules to the
> > > specified fwlog_level.
> > >
> > > If the NVM supports FW logging then the file 'fwlog' will be created
> > > under the PCI device ID for the ice driver. If the file does not exist
> > > then either the NVM doesn't support FW logging or debugfs is not enabled
> > > on the system.
> > >
> > > In addition to configuring the modules, the user can also configure the
> > > number of log messages (resolution) to include in a single Admin Receive
> > > Queue (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.
> > >
> > > To see/change the resolution the user can read/write the
> > > 'fwlog/resolution' file. An example changing the value to 50 is
> > >
> > > echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
> > >
> > > Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> > > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
> > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > > ---
> > > drivers/net/ethernet/intel/ice/Makefile | 4 +-
> > > drivers/net/ethernet/intel/ice/ice.h | 18 +
> > > .../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
> > > drivers/net/ethernet/intel/ice/ice_common.c | 5 +
> > > drivers/net/ethernet/intel/ice/ice_debugfs.c | 450 ++++++++++++++++++
> > > drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
> > > drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
> > > drivers/net/ethernet/intel/ice/ice_main.c | 21 +
> > > drivers/net/ethernet/intel/ice/ice_type.h | 4 +
> > > 9 files changed, 867 insertions(+), 1 deletion(-)
> > > 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
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
> > > index 960277d78e09..d43a59e5f8ee 100644
> > > --- a/drivers/net/ethernet/intel/ice/Makefile
> > > +++ b/drivers/net/ethernet/intel/ice/Makefile
> > > @@ -34,7 +34,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 \
> > > @@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
> > > ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
> > > ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
> > > ice-$(CONFIG_GNSS) += ice_gnss.o
> > > +ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
> > > diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> > > index 5ac0ad12f9f1..e6dd9f6f9eee 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice.h
> > > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > > @@ -556,6 +556,8 @@ 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 */
> > > + struct dentry *ice_debugfs_pf;
> > > + struct dentry *ice_debugfs_pf_fwlog;
> > > struct ice_vfs vfs;
> > > DECLARE_BITMAP(features, ICE_F_MAX);
> > > DECLARE_BITMAP(state, ICE_STATE_NBITS);
> > > @@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct ice_pf *pf)
> > > return false;
> > > }
> > > +#ifdef CONFIG_DEBUG_FS
> >
> > There is no need in this CONFIG_DEBUG_FS and code should be written
> > without debugfs stubs.
> >
>
> I don't understand this comment... If the kernel is configured *without*
> debugfs, won't the kernel fail to compile due to missing functions if we
> don't do this?
It will work fine, see include/linux/debugfs.h.
>
> > > +void ice_debugfs_fwlog_init(struct ice_pf *pf);
> > > +void ice_debugfs_init(void);
> > > +void ice_debugfs_exit(void);
> > > +void ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module);
> > > +#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) { }
> > > +static void
> > > +ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module)
> > > +{
> > > + return -EOPNOTSUPP;
> > > +}
> > > +#endif /* CONFIG_DEBUG_FS */
> >
> > Thanks
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v3 2/5] ice: configure FW logging
2023-08-18 11:10 ` Leon Romanovsky
@ 2023-08-18 12:31 ` Przemek Kitszel
2023-08-21 23:20 ` Paul M Stillwell Jr
0 siblings, 1 reply; 21+ messages in thread
From: Przemek Kitszel @ 2023-08-18 12:31 UTC (permalink / raw)
To: Leon Romanovsky, Paul M Stillwell Jr
Cc: Tony Nguyen, davem, kuba, pabeni, edumazet, netdev,
jacob.e.keller, horms, Pucha Himasekhar Reddy
On 8/18/23 13:10, Leon Romanovsky wrote:
> On Thu, Aug 17, 2023 at 02:25:34PM -0700, Paul M Stillwell Jr wrote:
>> On 8/15/2023 11:38 AM, Leon Romanovsky wrote:
>>> On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
>>>> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>>>
>>>> Users want the ability to debug FW issues by retrieving the
>>>> FW logs from the E8xx devices. Use debugfs to allow the user to
>>>> read/write the FW log configuration by adding a 'fwlog/modules' file.
>>>> Reading the file will show either the currently running configuration or
>>>> the next configuration (if the user has changed the configuration).
>>>> Writing to the file will update the configuration, but NOT enable the
>>>> configuration (that is a separate command).
>>>>
>>>> To see the status of FW logging then read the 'fwlog/modules' file like
>>>> this:
>>>>
>>>> cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>>>
>>>> To change the configuration of FW logging then write to the 'fwlog/modules'
>>>> file like this:
>>>>
>>>> echo DCB NORMAL > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>>>
>>>> The format to change the configuration is
>>>>
>>>> echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device
>>>
>>> This line is truncated, it is not clear where you are writing.
>>
>> Good catch, I don't know how I missed this... Will fix
>>
>>> And more general question, a long time ago, netdev had a policy of
>>> not-allowing writing to debugfs, was it changed?
>>>
>>
>> I had this same thought and it seems like there were 2 concerns in the past
>
> Maybe, I'm not enough time in netdev world to know the history.
>
>>
>> 1. Having a single file that was read/write with lots of commands going
>> through it
>> 2. Having code in the driver to parse the text from the commands that was
>> error/security prone
>>
>> We have addressed this in 2 ways:
>> 1. Split the commands into multiple files that have a single purpose
>> 2. Use kernel parsing functions for anything where we *have* to pass text to
>> decode
>>
>>>>
>>>> where
>>>>
>>>> * fwlog_level is a name as described below. Each level includes the
>>>> messages from the previous/lower level
>>>>
>>>> * NONE
>>>> * ERROR
>>>> * WARNING
>>>> * NORMAL
>>>> * VERBOSE
>>>>
>>>> * fwlog_event is a name that represents the module to receive events for.
>>>> The module names are
>>>>
>>>> * GENERAL
>>>> * CTRL
>>>> * LINK
>>>> * LINK_TOPO
>>>> * DNL
>>>> * I2C
>>>> * SDP
>>>> * MDIO
>>>> * ADMINQ
>>>> * HDMA
>>>> * LLDP
>>>> * DCBX
>>>> * DCB
>>>> * XLR
>>>> * NVM
>>>> * AUTH
>>>> * VPD
>>>> * IOSF
>>>> * PARSER
>>>> * SW
>>>> * SCHEDULER
>>>> * TXQ
>>>> * RSVD
>>>> * POST
>>>> * WATCHDOG
>>>> * TASK_DISPATCH
>>>> * MNG
>>>> * SYNCE
>>>> * HEALTH
>>>> * TSDRV
>>>> * PFREG
>>>> * MDLVER
>>>> * ALL
>>>>
>>>> The name ALL is special and specifies setting all of the modules to the
>>>> specified fwlog_level.
>>>>
>>>> If the NVM supports FW logging then the file 'fwlog' will be created
>>>> under the PCI device ID for the ice driver. If the file does not exist
>>>> then either the NVM doesn't support FW logging or debugfs is not enabled
>>>> on the system.
>>>>
>>>> In addition to configuring the modules, the user can also configure the
>>>> number of log messages (resolution) to include in a single Admin Receive
>>>> Queue (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.
>>>>
>>>> To see/change the resolution the user can read/write the
>>>> 'fwlog/resolution' file. An example changing the value to 50 is
>>>>
>>>> echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
>>>>
>>>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>>> Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>>> ---
>>>> drivers/net/ethernet/intel/ice/Makefile | 4 +-
>>>> drivers/net/ethernet/intel/ice/ice.h | 18 +
>>>> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
>>>> drivers/net/ethernet/intel/ice/ice_common.c | 5 +
>>>> drivers/net/ethernet/intel/ice/ice_debugfs.c | 450 ++++++++++++++++++
>>>> drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
>>>> drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
>>>> drivers/net/ethernet/intel/ice/ice_main.c | 21 +
>>>> drivers/net/ethernet/intel/ice/ice_type.h | 4 +
>>>> 9 files changed, 867 insertions(+), 1 deletion(-)
>>>> 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
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
>>>> index 960277d78e09..d43a59e5f8ee 100644
>>>> --- a/drivers/net/ethernet/intel/ice/Makefile
>>>> +++ b/drivers/net/ethernet/intel/ice/Makefile
>>>> @@ -34,7 +34,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 \
>>>> @@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
>>>> ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
>>>> ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
>>>> ice-$(CONFIG_GNSS) += ice_gnss.o
>>>> +ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
>>>> index 5ac0ad12f9f1..e6dd9f6f9eee 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice.h
>>>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>>>> @@ -556,6 +556,8 @@ 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 */
>>>> + struct dentry *ice_debugfs_pf;
>>>> + struct dentry *ice_debugfs_pf_fwlog;
>>>> struct ice_vfs vfs;
>>>> DECLARE_BITMAP(features, ICE_F_MAX);
>>>> DECLARE_BITMAP(state, ICE_STATE_NBITS);
>>>> @@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct ice_pf *pf)
>>>> return false;
>>>> }
>>>> +#ifdef CONFIG_DEBUG_FS
>>>
>>> There is no need in this CONFIG_DEBUG_FS and code should be written
>>> without debugfs stubs.
>>>
>>
>> I don't understand this comment... If the kernel is configured *without*
>> debugfs, won't the kernel fail to compile due to missing functions if we
>> don't do this?
>
> It will work fine, see include/linux/debugfs.h.
Nice, as-is impl of ice_debugfs_fwlog_init() would just fail on first
debugfs API call.
>
>>
>>>> +void ice_debugfs_fwlog_init(struct ice_pf *pf);
>>>> +void ice_debugfs_init(void);
>>>> +void ice_debugfs_exit(void);
>>>> +void ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module);
>>>> +#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) { }
>>>> +static void
>>>> +ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module)
>>>> +{
>>>> + return -EOPNOTSUPP;
>>>> +}
>>>> +#endif /* CONFIG_DEBUG_FS */
>>>
>>> Thanks
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v3 2/5] ice: configure FW logging
2023-08-18 12:31 ` Przemek Kitszel
@ 2023-08-21 23:20 ` Paul M Stillwell Jr
2023-08-22 7:33 ` Przemek Kitszel
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2023-08-21 23:20 UTC (permalink / raw)
To: Przemek Kitszel, Leon Romanovsky
Cc: Tony Nguyen, davem, kuba, pabeni, edumazet, netdev,
jacob.e.keller, horms, Pucha Himasekhar Reddy
On 8/18/2023 5:31 AM, Przemek Kitszel wrote:
> On 8/18/23 13:10, Leon Romanovsky wrote:
>> On Thu, Aug 17, 2023 at 02:25:34PM -0700, Paul M Stillwell Jr wrote:
>>> On 8/15/2023 11:38 AM, Leon Romanovsky wrote:
>>>> On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
>>>>> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>>>>
>>>>> Users want the ability to debug FW issues by retrieving the
>>>>> FW logs from the E8xx devices. Use debugfs to allow the user to
>>>>> read/write the FW log configuration by adding a 'fwlog/modules' file.
>>>>> Reading the file will show either the currently running
>>>>> configuration or
>>>>> the next configuration (if the user has changed the configuration).
>>>>> Writing to the file will update the configuration, but NOT enable the
>>>>> configuration (that is a separate command).
>>>>>
>>>>> To see the status of FW logging then read the 'fwlog/modules' file
>>>>> like
>>>>> this:
>>>>>
>>>>> cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>>>>
>>>>> To change the configuration of FW logging then write to the
>>>>> 'fwlog/modules'
>>>>> file like this:
>>>>>
>>>>> echo DCB NORMAL > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>>>>
>>>>> The format to change the configuration is
>>>>>
>>>>> echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device
>>>>
>>>> This line is truncated, it is not clear where you are writing.
>>>
>>> Good catch, I don't know how I missed this... Will fix
>>>
>>>> And more general question, a long time ago, netdev had a policy of
>>>> not-allowing writing to debugfs, was it changed?
>>>>
>>>
>>> I had this same thought and it seems like there were 2 concerns in
>>> the past
>>
>> Maybe, I'm not enough time in netdev world to know the history.
>>
>>>
>>> 1. Having a single file that was read/write with lots of commands going
>>> through it
>>> 2. Having code in the driver to parse the text from the commands that
>>> was
>>> error/security prone
>>>
>>> We have addressed this in 2 ways:
>>> 1. Split the commands into multiple files that have a single purpose
>>> 2. Use kernel parsing functions for anything where we *have* to pass
>>> text to
>>> decode
>>>
>>>>>
>>>>> where
>>>>>
>>>>> * fwlog_level is a name as described below. Each level includes the
>>>>> messages from the previous/lower level
>>>>>
>>>>> * NONE
>>>>> * ERROR
>>>>> * WARNING
>>>>> * NORMAL
>>>>> * VERBOSE
>>>>>
>>>>> * fwlog_event is a name that represents the module to receive
>>>>> events for.
>>>>> The module names are
>>>>>
>>>>> * GENERAL
>>>>> * CTRL
>>>>> * LINK
>>>>> * LINK_TOPO
>>>>> * DNL
>>>>> * I2C
>>>>> * SDP
>>>>> * MDIO
>>>>> * ADMINQ
>>>>> * HDMA
>>>>> * LLDP
>>>>> * DCBX
>>>>> * DCB
>>>>> * XLR
>>>>> * NVM
>>>>> * AUTH
>>>>> * VPD
>>>>> * IOSF
>>>>> * PARSER
>>>>> * SW
>>>>> * SCHEDULER
>>>>> * TXQ
>>>>> * RSVD
>>>>> * POST
>>>>> * WATCHDOG
>>>>> * TASK_DISPATCH
>>>>> * MNG
>>>>> * SYNCE
>>>>> * HEALTH
>>>>> * TSDRV
>>>>> * PFREG
>>>>> * MDLVER
>>>>> * ALL
>>>>>
>>>>> The name ALL is special and specifies setting all of the modules to
>>>>> the
>>>>> specified fwlog_level.
>>>>>
>>>>> If the NVM supports FW logging then the file 'fwlog' will be created
>>>>> under the PCI device ID for the ice driver. If the file does not exist
>>>>> then either the NVM doesn't support FW logging or debugfs is not
>>>>> enabled
>>>>> on the system.
>>>>>
>>>>> In addition to configuring the modules, the user can also configure
>>>>> the
>>>>> number of log messages (resolution) to include in a single Admin
>>>>> Receive
>>>>> Queue (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.
>>>>>
>>>>> To see/change the resolution the user can read/write the
>>>>> 'fwlog/resolution' file. An example changing the value to 50 is
>>>>>
>>>>> echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
>>>>>
>>>>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>>>> Tested-by: Pucha Himasekhar Reddy
>>>>> <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
>>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>>>> ---
>>>>> drivers/net/ethernet/intel/ice/Makefile | 4 +-
>>>>> drivers/net/ethernet/intel/ice/ice.h | 18 +
>>>>> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
>>>>> drivers/net/ethernet/intel/ice/ice_common.c | 5 +
>>>>> drivers/net/ethernet/intel/ice/ice_debugfs.c | 450
>>>>> ++++++++++++++++++
>>>>> drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
>>>>> drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
>>>>> drivers/net/ethernet/intel/ice/ice_main.c | 21 +
>>>>> drivers/net/ethernet/intel/ice/ice_type.h | 4 +
>>>>> 9 files changed, 867 insertions(+), 1 deletion(-)
>>>>> 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
>>>>>
>>>>> diff --git a/drivers/net/ethernet/intel/ice/Makefile
>>>>> b/drivers/net/ethernet/intel/ice/Makefile
>>>>> index 960277d78e09..d43a59e5f8ee 100644
>>>>> --- a/drivers/net/ethernet/intel/ice/Makefile
>>>>> +++ b/drivers/net/ethernet/intel/ice/Makefile
>>>>> @@ -34,7 +34,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 \
>>>>> @@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
>>>>> ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
>>>>> ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
>>>>> ice-$(CONFIG_GNSS) += ice_gnss.o
>>>>> +ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
>>>>> diff --git a/drivers/net/ethernet/intel/ice/ice.h
>>>>> b/drivers/net/ethernet/intel/ice/ice.h
>>>>> index 5ac0ad12f9f1..e6dd9f6f9eee 100644
>>>>> --- a/drivers/net/ethernet/intel/ice/ice.h
>>>>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>>>>> @@ -556,6 +556,8 @@ 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 */
>>>>> + struct dentry *ice_debugfs_pf;
>>>>> + struct dentry *ice_debugfs_pf_fwlog;
>>>>> struct ice_vfs vfs;
>>>>> DECLARE_BITMAP(features, ICE_F_MAX);
>>>>> DECLARE_BITMAP(state, ICE_STATE_NBITS);
>>>>> @@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct
>>>>> ice_pf *pf)
>>>>> return false;
>>>>> }
>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>
>>>> There is no need in this CONFIG_DEBUG_FS and code should be written
>>>> without debugfs stubs.
>>>>
>>>
>>> I don't understand this comment... If the kernel is configured *without*
>>> debugfs, won't the kernel fail to compile due to missing functions if we
>>> don't do this?
>>
>> It will work fine, see include/linux/debugfs.h.
>
> Nice, as-is impl of ice_debugfs_fwlog_init() would just fail on first
> debugfs API call.
>
I've thought about this some more and I am confused what to do. In the
Makefile there is a bit that removes ice_debugfs.o if CONFIG_DEBUG_FS is
not set. This would result in the stubs being needed (since the
functions are called from ice_fwlog.c). In this case the code would not
compile (since the functions would be missing). Should I remove the code
from the Makefile that deals with ice_debugfs.o (which doesn't make
sense since then there will be code in the driver that doesn't get used)
or do I leave the stubs in?
>>
>>>
>>>>> +void ice_debugfs_fwlog_init(struct ice_pf *pf);
>>>>> +void ice_debugfs_init(void);
>>>>> +void ice_debugfs_exit(void);
>>>>> +void ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level,
>>>>> int module);
>>>>> +#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) { }
>>>>> +static void
>>>>> +ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int
>>>>> module)
>>>>> +{
>>>>> + return -EOPNOTSUPP;
>>>>> +}
>>>>> +#endif /* CONFIG_DEBUG_FS */
>>>>
>>>> Thanks
>>>
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v3 2/5] ice: configure FW logging
2023-08-21 23:20 ` Paul M Stillwell Jr
@ 2023-08-22 7:33 ` Przemek Kitszel
2023-08-22 20:44 ` Keller, Jacob E
2023-08-22 20:45 ` Keller, Jacob E
2 siblings, 0 replies; 21+ messages in thread
From: Przemek Kitszel @ 2023-08-22 7:33 UTC (permalink / raw)
To: Paul M Stillwell Jr, Leon Romanovsky
Cc: Tony Nguyen, davem, kuba, pabeni, edumazet, netdev,
jacob.e.keller, horms, Pucha Himasekhar Reddy
On 8/22/23 01:20, Paul M Stillwell Jr wrote:
> On 8/18/2023 5:31 AM, Przemek Kitszel wrote:
>> On 8/18/23 13:10, Leon Romanovsky wrote:
>>> On Thu, Aug 17, 2023 at 02:25:34PM -0700, Paul M Stillwell Jr wrote:
>>>> On 8/15/2023 11:38 AM, Leon Romanovsky wrote:
>>>>> On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
>>>>>> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>>>>>
>>>>>> Users want the ability to debug FW issues by retrieving the
>>>>>> FW logs from the E8xx devices. Use debugfs to allow the user to
>>>>>> read/write the FW log configuration by adding a 'fwlog/modules' file.
>>>>>> Reading the file will show either the currently running
>>>>>> configuration or
>>>>>> the next configuration (if the user has changed the configuration).
>>>>>> Writing to the file will update the configuration, but NOT enable the
>>>>>> configuration (that is a separate command).
>>>>>>
>>>>>> To see the status of FW logging then read the 'fwlog/modules' file
>>>>>> like
>>>>>> this:
>>>>>>
>>>>>> cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>>>>>
>>>>>> To change the configuration of FW logging then write to the
>>>>>> 'fwlog/modules'
>>>>>> file like this:
>>>>>>
>>>>>> echo DCB NORMAL > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>>>>>
>>>>>> The format to change the configuration is
>>>>>>
>>>>>> echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device
>>>>>
>>>>> This line is truncated, it is not clear where you are writing.
>>>>
>>>> Good catch, I don't know how I missed this... Will fix
>>>>
>>>>> And more general question, a long time ago, netdev had a policy of
>>>>> not-allowing writing to debugfs, was it changed?
>>>>>
>>>>
>>>> I had this same thought and it seems like there were 2 concerns in
>>>> the past
>>>
>>> Maybe, I'm not enough time in netdev world to know the history.
>>>
>>>>
>>>> 1. Having a single file that was read/write with lots of commands going
>>>> through it
>>>> 2. Having code in the driver to parse the text from the commands
>>>> that was
>>>> error/security prone
>>>>
>>>> We have addressed this in 2 ways:
>>>> 1. Split the commands into multiple files that have a single purpose
>>>> 2. Use kernel parsing functions for anything where we *have* to pass
>>>> text to
>>>> decode
>>>>
>>>>>>
>>>>>> where
>>>>>>
>>>>>> * fwlog_level is a name as described below. Each level includes the
>>>>>> messages from the previous/lower level
>>>>>>
>>>>>> * NONE
>>>>>> * ERROR
>>>>>> * WARNING
>>>>>> * NORMAL
>>>>>> * VERBOSE
>>>>>>
>>>>>> * fwlog_event is a name that represents the module to receive
>>>>>> events for.
>>>>>> The module names are
>>>>>>
>>>>>> * GENERAL
>>>>>> * CTRL
>>>>>> * LINK
>>>>>> * LINK_TOPO
>>>>>> * DNL
>>>>>> * I2C
>>>>>> * SDP
>>>>>> * MDIO
>>>>>> * ADMINQ
>>>>>> * HDMA
>>>>>> * LLDP
>>>>>> * DCBX
>>>>>> * DCB
>>>>>> * XLR
>>>>>> * NVM
>>>>>> * AUTH
>>>>>> * VPD
>>>>>> * IOSF
>>>>>> * PARSER
>>>>>> * SW
>>>>>> * SCHEDULER
>>>>>> * TXQ
>>>>>> * RSVD
>>>>>> * POST
>>>>>> * WATCHDOG
>>>>>> * TASK_DISPATCH
>>>>>> * MNG
>>>>>> * SYNCE
>>>>>> * HEALTH
>>>>>> * TSDRV
>>>>>> * PFREG
>>>>>> * MDLVER
>>>>>> * ALL
>>>>>>
>>>>>> The name ALL is special and specifies setting all of the modules
>>>>>> to the
>>>>>> specified fwlog_level.
>>>>>>
>>>>>> If the NVM supports FW logging then the file 'fwlog' will be created
>>>>>> under the PCI device ID for the ice driver. If the file does not
>>>>>> exist
>>>>>> then either the NVM doesn't support FW logging or debugfs is not
>>>>>> enabled
>>>>>> on the system.
>>>>>>
>>>>>> In addition to configuring the modules, the user can also
>>>>>> configure the
>>>>>> number of log messages (resolution) to include in a single Admin
>>>>>> Receive
>>>>>> Queue (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.
>>>>>>
>>>>>> To see/change the resolution the user can read/write the
>>>>>> 'fwlog/resolution' file. An example changing the value to 50 is
>>>>>>
>>>>>> echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
>>>>>>
>>>>>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>>>>> Tested-by: Pucha Himasekhar Reddy
>>>>>> <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
>>>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>>>>> ---
>>>>>> drivers/net/ethernet/intel/ice/Makefile | 4 +-
>>>>>> drivers/net/ethernet/intel/ice/ice.h | 18 +
>>>>>> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
>>>>>> drivers/net/ethernet/intel/ice/ice_common.c | 5 +
>>>>>> drivers/net/ethernet/intel/ice/ice_debugfs.c | 450
>>>>>> ++++++++++++++++++
>>>>>> drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
>>>>>> drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
>>>>>> drivers/net/ethernet/intel/ice/ice_main.c | 21 +
>>>>>> drivers/net/ethernet/intel/ice/ice_type.h | 4 +
>>>>>> 9 files changed, 867 insertions(+), 1 deletion(-)
>>>>>> 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
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/intel/ice/Makefile
>>>>>> b/drivers/net/ethernet/intel/ice/Makefile
>>>>>> index 960277d78e09..d43a59e5f8ee 100644
>>>>>> --- a/drivers/net/ethernet/intel/ice/Makefile
>>>>>> +++ b/drivers/net/ethernet/intel/ice/Makefile
>>>>>> @@ -34,7 +34,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 \
>>>>>> @@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
>>>>>> ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
>>>>>> ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
>>>>>> ice-$(CONFIG_GNSS) += ice_gnss.o
>>>>>> +ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
>>>>>> diff --git a/drivers/net/ethernet/intel/ice/ice.h
>>>>>> b/drivers/net/ethernet/intel/ice/ice.h
>>>>>> index 5ac0ad12f9f1..e6dd9f6f9eee 100644
>>>>>> --- a/drivers/net/ethernet/intel/ice/ice.h
>>>>>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>>>>>> @@ -556,6 +556,8 @@ 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 */
>>>>>> + struct dentry *ice_debugfs_pf;
>>>>>> + struct dentry *ice_debugfs_pf_fwlog;
>>>>>> struct ice_vfs vfs;
>>>>>> DECLARE_BITMAP(features, ICE_F_MAX);
>>>>>> DECLARE_BITMAP(state, ICE_STATE_NBITS);
>>>>>> @@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct
>>>>>> ice_pf *pf)
>>>>>> return false;
>>>>>> }
>>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>>
>>>>> There is no need in this CONFIG_DEBUG_FS and code should be written
>>>>> without debugfs stubs.
>>>>>
>>>>
>>>> I don't understand this comment... If the kernel is configured
>>>> *without*
>>>> debugfs, won't the kernel fail to compile due to missing functions
>>>> if we
>>>> don't do this?
>>>
>>> It will work fine, see include/linux/debugfs.h.
>>
>> Nice, as-is impl of ice_debugfs_fwlog_init() would just fail on first
>> debugfs API call.
>>
>
> I've thought about this some more and I am confused what to do. > In the Makefile there is a bit that removes ice_debugfs.o if
CONFIG_DEBUG_FS is
> not set.
That's true, and it is to prevent 450 lines of code, and some includes,
so makes sense.
> This would result in the stubs being needed (since the
> functions are called from ice_fwlog.c). In this case the code would not
> compile (since the functions would be missing). Should I remove the code
> from the Makefile that deals with ice_debugfs.o (which doesn't make
> sense since then there will be code in the driver that doesn't get used)
> or do I leave the stubs in?
other option is to have those few (that would be stubs otherwise)
functions outside of ice_debugfs.c, I didn't checked if there will be
any dependencies though
>
>>>
>>>>
>>>>>> +void ice_debugfs_fwlog_init(struct ice_pf *pf);
>>>>>> +void ice_debugfs_init(void);
>>>>>> +void ice_debugfs_exit(void);
>>>>>> +void ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level,
>>>>>> int module);
>>>>>> +#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) { }
>>>>>> +static void
>>>>>> +ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int
>>>>>> module)
>>>>>> +{
>>>>>> + return -EOPNOTSUPP;
>>>>>> +}
>>>>>> +#endif /* CONFIG_DEBUG_FS */
>>>>>
>>>>> Thanks
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [PATCH net-next v3 2/5] ice: configure FW logging
2023-08-21 23:20 ` Paul M Stillwell Jr
2023-08-22 7:33 ` Przemek Kitszel
@ 2023-08-22 20:44 ` Keller, Jacob E
2023-08-22 20:58 ` Paul M Stillwell Jr
2023-08-22 20:45 ` Keller, Jacob E
2 siblings, 1 reply; 21+ messages in thread
From: Keller, Jacob E @ 2023-08-22 20:44 UTC (permalink / raw)
To: Stillwell Jr, Paul M, Kitszel, Przemyslaw, Leon Romanovsky
Cc: Nguyen, Anthony L, davem, kuba, pabeni, edumazet, netdev, horms,
Pucha, HimasekharX Reddy
> -----Original Message-----
> From: Stillwell Jr, Paul M <paul.m.stillwell.jr@intel.com>
> Sent: Monday, August 21, 2023 4:21 PM
> To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Leon Romanovsky
> <leon@kernel.org>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net;
> kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
> netdev@vger.kernel.org; Keller, Jacob E <jacob.e.keller@intel.com>;
> horms@kernel.org; Pucha, HimasekharX Reddy
> <himasekharx.reddy.pucha@intel.com>
> Subject: Re: [PATCH net-next v3 2/5] ice: configure FW logging
>
> On 8/18/2023 5:31 AM, Przemek Kitszel wrote:
> > On 8/18/23 13:10, Leon Romanovsky wrote:
> >> On Thu, Aug 17, 2023 at 02:25:34PM -0700, Paul M Stillwell Jr wrote:
> >>> On 8/15/2023 11:38 AM, Leon Romanovsky wrote:
> >>>> On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
> >>>>> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> >>>>>
> >>>>> Users want the ability to debug FW issues by retrieving the
> >>>>> FW logs from the E8xx devices. Use debugfs to allow the user to
> >>>>> read/write the FW log configuration by adding a 'fwlog/modules' file.
> >>>>> Reading the file will show either the currently running
> >>>>> configuration or
> >>>>> the next configuration (if the user has changed the configuration).
> >>>>> Writing to the file will update the configuration, but NOT enable the
> >>>>> configuration (that is a separate command).
> >>>>>
> >>>>> To see the status of FW logging then read the 'fwlog/modules' file
> >>>>> like
> >>>>> this:
> >>>>>
> >>>>> cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
> >>>>>
> >>>>> To change the configuration of FW logging then write to the
> >>>>> 'fwlog/modules'
> >>>>> file like this:
> >>>>>
> >>>>> echo DCB NORMAL >
> /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
> >>>>>
> >>>>> The format to change the configuration is
> >>>>>
> >>>>> echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device
> >>>>
> >>>> This line is truncated, it is not clear where you are writing.
> >>>
> >>> Good catch, I don't know how I missed this... Will fix
> >>>
> >>>> And more general question, a long time ago, netdev had a policy of
> >>>> not-allowing writing to debugfs, was it changed?
> >>>>
> >>>
> >>> I had this same thought and it seems like there were 2 concerns in
> >>> the past
> >>
> >> Maybe, I'm not enough time in netdev world to know the history.
> >>
> >>>
> >>> 1. Having a single file that was read/write with lots of commands going
> >>> through it
> >>> 2. Having code in the driver to parse the text from the commands that
> >>> was
> >>> error/security prone
> >>>
> >>> We have addressed this in 2 ways:
> >>> 1. Split the commands into multiple files that have a single purpose
> >>> 2. Use kernel parsing functions for anything where we *have* to pass
> >>> text to
> >>> decode
> >>>
> >>>>>
> >>>>> where
> >>>>>
> >>>>> * fwlog_level is a name as described below. Each level includes the
> >>>>> messages from the previous/lower level
> >>>>>
> >>>>> * NONE
> >>>>> * ERROR
> >>>>> * WARNING
> >>>>> * NORMAL
> >>>>> * VERBOSE
> >>>>>
> >>>>> * fwlog_event is a name that represents the module to receive
> >>>>> events for.
> >>>>> The module names are
> >>>>>
> >>>>> * GENERAL
> >>>>> * CTRL
> >>>>> * LINK
> >>>>> * LINK_TOPO
> >>>>> * DNL
> >>>>> * I2C
> >>>>> * SDP
> >>>>> * MDIO
> >>>>> * ADMINQ
> >>>>> * HDMA
> >>>>> * LLDP
> >>>>> * DCBX
> >>>>> * DCB
> >>>>> * XLR
> >>>>> * NVM
> >>>>> * AUTH
> >>>>> * VPD
> >>>>> * IOSF
> >>>>> * PARSER
> >>>>> * SW
> >>>>> * SCHEDULER
> >>>>> * TXQ
> >>>>> * RSVD
> >>>>> * POST
> >>>>> * WATCHDOG
> >>>>> * TASK_DISPATCH
> >>>>> * MNG
> >>>>> * SYNCE
> >>>>> * HEALTH
> >>>>> * TSDRV
> >>>>> * PFREG
> >>>>> * MDLVER
> >>>>> * ALL
> >>>>>
> >>>>> The name ALL is special and specifies setting all of the modules to
> >>>>> the
> >>>>> specified fwlog_level.
> >>>>>
> >>>>> If the NVM supports FW logging then the file 'fwlog' will be created
> >>>>> under the PCI device ID for the ice driver. If the file does not exist
> >>>>> then either the NVM doesn't support FW logging or debugfs is not
> >>>>> enabled
> >>>>> on the system.
> >>>>>
> >>>>> In addition to configuring the modules, the user can also configure
> >>>>> the
> >>>>> number of log messages (resolution) to include in a single Admin
> >>>>> Receive
> >>>>> Queue (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.
> >>>>>
> >>>>> To see/change the resolution the user can read/write the
> >>>>> 'fwlog/resolution' file. An example changing the value to 50 is
> >>>>>
> >>>>> echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
> >>>>>
> >>>>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> >>>>> Tested-by: Pucha Himasekhar Reddy
> >>>>> <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
> >>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> >>>>> ---
> >>>>> drivers/net/ethernet/intel/ice/Makefile | 4 +-
> >>>>> drivers/net/ethernet/intel/ice/ice.h | 18 +
> >>>>> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
> >>>>> drivers/net/ethernet/intel/ice/ice_common.c | 5 +
> >>>>> drivers/net/ethernet/intel/ice/ice_debugfs.c | 450
> >>>>> ++++++++++++++++++
> >>>>> drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
> >>>>> drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
> >>>>> drivers/net/ethernet/intel/ice/ice_main.c | 21 +
> >>>>> drivers/net/ethernet/intel/ice/ice_type.h | 4 +
> >>>>> 9 files changed, 867 insertions(+), 1 deletion(-)
> >>>>> 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
> >>>>>
> >>>>> diff --git a/drivers/net/ethernet/intel/ice/Makefile
> >>>>> b/drivers/net/ethernet/intel/ice/Makefile
> >>>>> index 960277d78e09..d43a59e5f8ee 100644
> >>>>> --- a/drivers/net/ethernet/intel/ice/Makefile
> >>>>> +++ b/drivers/net/ethernet/intel/ice/Makefile
> >>>>> @@ -34,7 +34,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 \
> >>>>> @@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
> >>>>> ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
> >>>>> ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
> >>>>> ice-$(CONFIG_GNSS) += ice_gnss.o
> >>>>> +ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
> >>>>> diff --git a/drivers/net/ethernet/intel/ice/ice.h
> >>>>> b/drivers/net/ethernet/intel/ice/ice.h
> >>>>> index 5ac0ad12f9f1..e6dd9f6f9eee 100644
> >>>>> --- a/drivers/net/ethernet/intel/ice/ice.h
> >>>>> +++ b/drivers/net/ethernet/intel/ice/ice.h
> >>>>> @@ -556,6 +556,8 @@ 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 */
> >>>>> + struct dentry *ice_debugfs_pf;
> >>>>> + struct dentry *ice_debugfs_pf_fwlog;
> >>>>> struct ice_vfs vfs;
> >>>>> DECLARE_BITMAP(features, ICE_F_MAX);
> >>>>> DECLARE_BITMAP(state, ICE_STATE_NBITS);
> >>>>> @@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct
> >>>>> ice_pf *pf)
> >>>>> return false;
> >>>>> }
> >>>>> +#ifdef CONFIG_DEBUG_FS
> >>>>
> >>>> There is no need in this CONFIG_DEBUG_FS and code should be written
> >>>> without debugfs stubs.
> >>>>
> >>>
> >>> I don't understand this comment... If the kernel is configured *without*
> >>> debugfs, won't the kernel fail to compile due to missing functions if we
> >>> don't do this?
> >>
> >> It will work fine, see include/linux/debugfs.h.
> >
> > Nice, as-is impl of ice_debugfs_fwlog_init() would just fail on first
> > debugfs API call.
> >
>
> I've thought about this some more and I am confused what to do. In the
> Makefile there is a bit that removes ice_debugfs.o if CONFIG_DEBUG_FS is
> not set. This would result in the stubs being needed (since the
> functions are called from ice_fwlog.c). In this case the code would not
> compile (since the functions would be missing). Should I remove the code
> from the Makefile that deals with ice_debugfs.o (which doesn't make
> sense since then there will be code in the driver that doesn't get used)
> or do I leave the stubs in?
>
These stubs are for functions we have in ice_debugfs.o? Is there an ice_debugfs.h? We should implement stubs for those functions there so we don't have to check CONFIG_DEBUG_FS. Or, if they don’t' really belong there move them into another file that isn't stripped out with CONFIG_DEBUG_FS=n.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v3 2/5] ice: configure FW logging
2023-08-22 20:44 ` Keller, Jacob E
@ 2023-08-22 20:58 ` Paul M Stillwell Jr
2023-08-22 21:16 ` Paul M Stillwell Jr
0 siblings, 1 reply; 21+ messages in thread
From: Paul M Stillwell Jr @ 2023-08-22 20:58 UTC (permalink / raw)
To: Keller, Jacob E, Kitszel, Przemyslaw, Leon Romanovsky
Cc: Nguyen, Anthony L, davem, kuba, pabeni, edumazet, netdev, horms,
Pucha, HimasekharX Reddy
On 8/22/2023 1:44 PM, Keller, Jacob E wrote:
>
>
>> -----Original Message-----
>> From: Stillwell Jr, Paul M <paul.m.stillwell.jr@intel.com>
>> Sent: Monday, August 21, 2023 4:21 PM
>> To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Leon Romanovsky
>> <leon@kernel.org>
>> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net;
>> kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
>> netdev@vger.kernel.org; Keller, Jacob E <jacob.e.keller@intel.com>;
>> horms@kernel.org; Pucha, HimasekharX Reddy
>> <himasekharx.reddy.pucha@intel.com>
>> Subject: Re: [PATCH net-next v3 2/5] ice: configure FW logging
>>
>> On 8/18/2023 5:31 AM, Przemek Kitszel wrote:
>>> On 8/18/23 13:10, Leon Romanovsky wrote:
>>>> On Thu, Aug 17, 2023 at 02:25:34PM -0700, Paul M Stillwell Jr wrote:
>>>>> On 8/15/2023 11:38 AM, Leon Romanovsky wrote:
>>>>>> On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
>>>>>>> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>>>>>>
>>>>>>> Users want the ability to debug FW issues by retrieving the
>>>>>>> FW logs from the E8xx devices. Use debugfs to allow the user to
>>>>>>> read/write the FW log configuration by adding a 'fwlog/modules' file.
>>>>>>> Reading the file will show either the currently running
>>>>>>> configuration or
>>>>>>> the next configuration (if the user has changed the configuration).
>>>>>>> Writing to the file will update the configuration, but NOT enable the
>>>>>>> configuration (that is a separate command).
>>>>>>>
>>>>>>> To see the status of FW logging then read the 'fwlog/modules' file
>>>>>>> like
>>>>>>> this:
>>>>>>>
>>>>>>> cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>>>>>>
>>>>>>> To change the configuration of FW logging then write to the
>>>>>>> 'fwlog/modules'
>>>>>>> file like this:
>>>>>>>
>>>>>>> echo DCB NORMAL >
>> /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>>>>>>
>>>>>>> The format to change the configuration is
>>>>>>>
>>>>>>> echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device
>>>>>>
>>>>>> This line is truncated, it is not clear where you are writing.
>>>>>
>>>>> Good catch, I don't know how I missed this... Will fix
>>>>>
>>>>>> And more general question, a long time ago, netdev had a policy of
>>>>>> not-allowing writing to debugfs, was it changed?
>>>>>>
>>>>>
>>>>> I had this same thought and it seems like there were 2 concerns in
>>>>> the past
>>>>
>>>> Maybe, I'm not enough time in netdev world to know the history.
>>>>
>>>>>
>>>>> 1. Having a single file that was read/write with lots of commands going
>>>>> through it
>>>>> 2. Having code in the driver to parse the text from the commands that
>>>>> was
>>>>> error/security prone
>>>>>
>>>>> We have addressed this in 2 ways:
>>>>> 1. Split the commands into multiple files that have a single purpose
>>>>> 2. Use kernel parsing functions for anything where we *have* to pass
>>>>> text to
>>>>> decode
>>>>>
>>>>>>>
>>>>>>> where
>>>>>>>
>>>>>>> * fwlog_level is a name as described below. Each level includes the
>>>>>>> messages from the previous/lower level
>>>>>>>
>>>>>>> * NONE
>>>>>>> * ERROR
>>>>>>> * WARNING
>>>>>>> * NORMAL
>>>>>>> * VERBOSE
>>>>>>>
>>>>>>> * fwlog_event is a name that represents the module to receive
>>>>>>> events for.
>>>>>>> The module names are
>>>>>>>
>>>>>>> * GENERAL
>>>>>>> * CTRL
>>>>>>> * LINK
>>>>>>> * LINK_TOPO
>>>>>>> * DNL
>>>>>>> * I2C
>>>>>>> * SDP
>>>>>>> * MDIO
>>>>>>> * ADMINQ
>>>>>>> * HDMA
>>>>>>> * LLDP
>>>>>>> * DCBX
>>>>>>> * DCB
>>>>>>> * XLR
>>>>>>> * NVM
>>>>>>> * AUTH
>>>>>>> * VPD
>>>>>>> * IOSF
>>>>>>> * PARSER
>>>>>>> * SW
>>>>>>> * SCHEDULER
>>>>>>> * TXQ
>>>>>>> * RSVD
>>>>>>> * POST
>>>>>>> * WATCHDOG
>>>>>>> * TASK_DISPATCH
>>>>>>> * MNG
>>>>>>> * SYNCE
>>>>>>> * HEALTH
>>>>>>> * TSDRV
>>>>>>> * PFREG
>>>>>>> * MDLVER
>>>>>>> * ALL
>>>>>>>
>>>>>>> The name ALL is special and specifies setting all of the modules to
>>>>>>> the
>>>>>>> specified fwlog_level.
>>>>>>>
>>>>>>> If the NVM supports FW logging then the file 'fwlog' will be created
>>>>>>> under the PCI device ID for the ice driver. If the file does not exist
>>>>>>> then either the NVM doesn't support FW logging or debugfs is not
>>>>>>> enabled
>>>>>>> on the system.
>>>>>>>
>>>>>>> In addition to configuring the modules, the user can also configure
>>>>>>> the
>>>>>>> number of log messages (resolution) to include in a single Admin
>>>>>>> Receive
>>>>>>> Queue (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.
>>>>>>>
>>>>>>> To see/change the resolution the user can read/write the
>>>>>>> 'fwlog/resolution' file. An example changing the value to 50 is
>>>>>>>
>>>>>>> echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
>>>>>>>
>>>>>>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>>>>>> Tested-by: Pucha Himasekhar Reddy
>>>>>>> <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
>>>>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>>>>>> ---
>>>>>>> drivers/net/ethernet/intel/ice/Makefile | 4 +-
>>>>>>> drivers/net/ethernet/intel/ice/ice.h | 18 +
>>>>>>> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
>>>>>>> drivers/net/ethernet/intel/ice/ice_common.c | 5 +
>>>>>>> drivers/net/ethernet/intel/ice/ice_debugfs.c | 450
>>>>>>> ++++++++++++++++++
>>>>>>> drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
>>>>>>> drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
>>>>>>> drivers/net/ethernet/intel/ice/ice_main.c | 21 +
>>>>>>> drivers/net/ethernet/intel/ice/ice_type.h | 4 +
>>>>>>> 9 files changed, 867 insertions(+), 1 deletion(-)
>>>>>>> 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
>>>>>>>
>>>>>>> diff --git a/drivers/net/ethernet/intel/ice/Makefile
>>>>>>> b/drivers/net/ethernet/intel/ice/Makefile
>>>>>>> index 960277d78e09..d43a59e5f8ee 100644
>>>>>>> --- a/drivers/net/ethernet/intel/ice/Makefile
>>>>>>> +++ b/drivers/net/ethernet/intel/ice/Makefile
>>>>>>> @@ -34,7 +34,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 \
>>>>>>> @@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
>>>>>>> ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
>>>>>>> ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
>>>>>>> ice-$(CONFIG_GNSS) += ice_gnss.o
>>>>>>> +ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
>>>>>>> diff --git a/drivers/net/ethernet/intel/ice/ice.h
>>>>>>> b/drivers/net/ethernet/intel/ice/ice.h
>>>>>>> index 5ac0ad12f9f1..e6dd9f6f9eee 100644
>>>>>>> --- a/drivers/net/ethernet/intel/ice/ice.h
>>>>>>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>>>>>>> @@ -556,6 +556,8 @@ 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 */
>>>>>>> + struct dentry *ice_debugfs_pf;
>>>>>>> + struct dentry *ice_debugfs_pf_fwlog;
>>>>>>> struct ice_vfs vfs;
>>>>>>> DECLARE_BITMAP(features, ICE_F_MAX);
>>>>>>> DECLARE_BITMAP(state, ICE_STATE_NBITS);
>>>>>>> @@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct
>>>>>>> ice_pf *pf)
>>>>>>> return false;
>>>>>>> }
>>>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>>>
>>>>>> There is no need in this CONFIG_DEBUG_FS and code should be written
>>>>>> without debugfs stubs.
>>>>>>
>>>>>
>>>>> I don't understand this comment... If the kernel is configured *without*
>>>>> debugfs, won't the kernel fail to compile due to missing functions if we
>>>>> don't do this?
>>>>
>>>> It will work fine, see include/linux/debugfs.h.
>>>
>>> Nice, as-is impl of ice_debugfs_fwlog_init() would just fail on first
>>> debugfs API call.
>>>
>>
>> I've thought about this some more and I am confused what to do. In the
>> Makefile there is a bit that removes ice_debugfs.o if CONFIG_DEBUG_FS is
>> not set. This would result in the stubs being needed (since the
>> functions are called from ice_fwlog.c). In this case the code would not
>> compile (since the functions would be missing). Should I remove the code
>> from the Makefile that deals with ice_debugfs.o (which doesn't make
>> sense since then there will be code in the driver that doesn't get used)
>> or do I leave the stubs in?
>>
>
> These stubs are for functions we have in ice_debugfs.o?
All of the subs except for 1 (which could be moved) are in ice_debugfs.o
> Is there an ice_debugfs.h? We should implement stubs for those functions there so we don't have to check CONFIG_DEBUG_FS.
There isn't an ice_debugfs.h, but moving the stubs there would have the
same issue correct? You would have to wrap them with CONFIG_DEBUG_FS no
matter where they are, right?
> Or, if they don’t' really belong there move them into another file that isn't stripped out with CONFIG_DEBUG_FS=n.
>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v3 2/5] ice: configure FW logging
2023-08-22 20:58 ` Paul M Stillwell Jr
@ 2023-08-22 21:16 ` Paul M Stillwell Jr
0 siblings, 0 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2023-08-22 21:16 UTC (permalink / raw)
To: Keller, Jacob E, Kitszel, Przemyslaw, Leon Romanovsky
Cc: Nguyen, Anthony L, davem, kuba, pabeni, edumazet, netdev, horms,
Pucha, HimasekharX Reddy
On 8/22/2023 1:58 PM, Paul M Stillwell Jr wrote:
> On 8/22/2023 1:44 PM, Keller, Jacob E wrote:
>>
>>
>>> -----Original Message-----
>>> From: Stillwell Jr, Paul M <paul.m.stillwell.jr@intel.com>
>>> Sent: Monday, August 21, 2023 4:21 PM
>>> To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Leon Romanovsky
>>> <leon@kernel.org>
>>> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net;
>>> kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
>>> netdev@vger.kernel.org; Keller, Jacob E <jacob.e.keller@intel.com>;
>>> horms@kernel.org; Pucha, HimasekharX Reddy
>>> <himasekharx.reddy.pucha@intel.com>
>>> Subject: Re: [PATCH net-next v3 2/5] ice: configure FW logging
>>>
>>> On 8/18/2023 5:31 AM, Przemek Kitszel wrote:
>>>> On 8/18/23 13:10, Leon Romanovsky wrote:
>>>>> On Thu, Aug 17, 2023 at 02:25:34PM -0700, Paul M Stillwell Jr wrote:
>>>>>> On 8/15/2023 11:38 AM, Leon Romanovsky wrote:
>>>>>>> On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
>>>>>>>> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>>>>>>>
>>>>>>>> Users want the ability to debug FW issues by retrieving the
>>>>>>>> FW logs from the E8xx devices. Use debugfs to allow the user to
>>>>>>>> read/write the FW log configuration by adding a 'fwlog/modules'
>>>>>>>> file.
>>>>>>>> Reading the file will show either the currently running
>>>>>>>> configuration or
>>>>>>>> the next configuration (if the user has changed the configuration).
>>>>>>>> Writing to the file will update the configuration, but NOT
>>>>>>>> enable the
>>>>>>>> configuration (that is a separate command).
>>>>>>>>
>>>>>>>> To see the status of FW logging then read the 'fwlog/modules' file
>>>>>>>> like
>>>>>>>> this:
>>>>>>>>
>>>>>>>> cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>>>>>>>
>>>>>>>> To change the configuration of FW logging then write to the
>>>>>>>> 'fwlog/modules'
>>>>>>>> file like this:
>>>>>>>>
>>>>>>>> echo DCB NORMAL >
>>> /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>>>>>>>
>>>>>>>> The format to change the configuration is
>>>>>>>>
>>>>>>>> echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci
>>>>>>>> device
>>>>>>>
>>>>>>> This line is truncated, it is not clear where you are writing.
>>>>>>
>>>>>> Good catch, I don't know how I missed this... Will fix
>>>>>>
>>>>>>> And more general question, a long time ago, netdev had a policy of
>>>>>>> not-allowing writing to debugfs, was it changed?
>>>>>>>
>>>>>>
>>>>>> I had this same thought and it seems like there were 2 concerns in
>>>>>> the past
>>>>>
>>>>> Maybe, I'm not enough time in netdev world to know the history.
>>>>>
>>>>>>
>>>>>> 1. Having a single file that was read/write with lots of commands
>>>>>> going
>>>>>> through it
>>>>>> 2. Having code in the driver to parse the text from the commands that
>>>>>> was
>>>>>> error/security prone
>>>>>>
>>>>>> We have addressed this in 2 ways:
>>>>>> 1. Split the commands into multiple files that have a single purpose
>>>>>> 2. Use kernel parsing functions for anything where we *have* to pass
>>>>>> text to
>>>>>> decode
>>>>>>
>>>>>>>>
>>>>>>>> where
>>>>>>>>
>>>>>>>> * fwlog_level is a name as described below. Each level includes the
>>>>>>>> messages from the previous/lower level
>>>>>>>>
>>>>>>>> * NONE
>>>>>>>> * ERROR
>>>>>>>> * WARNING
>>>>>>>> * NORMAL
>>>>>>>> * VERBOSE
>>>>>>>>
>>>>>>>> * fwlog_event is a name that represents the module to receive
>>>>>>>> events for.
>>>>>>>> The module names are
>>>>>>>>
>>>>>>>> * GENERAL
>>>>>>>> * CTRL
>>>>>>>> * LINK
>>>>>>>> * LINK_TOPO
>>>>>>>> * DNL
>>>>>>>> * I2C
>>>>>>>> * SDP
>>>>>>>> * MDIO
>>>>>>>> * ADMINQ
>>>>>>>> * HDMA
>>>>>>>> * LLDP
>>>>>>>> * DCBX
>>>>>>>> * DCB
>>>>>>>> * XLR
>>>>>>>> * NVM
>>>>>>>> * AUTH
>>>>>>>> * VPD
>>>>>>>> * IOSF
>>>>>>>> * PARSER
>>>>>>>> * SW
>>>>>>>> * SCHEDULER
>>>>>>>> * TXQ
>>>>>>>> * RSVD
>>>>>>>> * POST
>>>>>>>> * WATCHDOG
>>>>>>>> * TASK_DISPATCH
>>>>>>>> * MNG
>>>>>>>> * SYNCE
>>>>>>>> * HEALTH
>>>>>>>> * TSDRV
>>>>>>>> * PFREG
>>>>>>>> * MDLVER
>>>>>>>> * ALL
>>>>>>>>
>>>>>>>> The name ALL is special and specifies setting all of the modules to
>>>>>>>> the
>>>>>>>> specified fwlog_level.
>>>>>>>>
>>>>>>>> If the NVM supports FW logging then the file 'fwlog' will be
>>>>>>>> created
>>>>>>>> under the PCI device ID for the ice driver. If the file does not
>>>>>>>> exist
>>>>>>>> then either the NVM doesn't support FW logging or debugfs is not
>>>>>>>> enabled
>>>>>>>> on the system.
>>>>>>>>
>>>>>>>> In addition to configuring the modules, the user can also configure
>>>>>>>> the
>>>>>>>> number of log messages (resolution) to include in a single Admin
>>>>>>>> Receive
>>>>>>>> Queue (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.
>>>>>>>>
>>>>>>>> To see/change the resolution the user can read/write the
>>>>>>>> 'fwlog/resolution' file. An example changing the value to 50 is
>>>>>>>>
>>>>>>>> echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
>>>>>>>>
>>>>>>>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>>>>>>> Tested-by: Pucha Himasekhar Reddy
>>>>>>>> <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
>>>>>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>>>>>>> ---
>>>>>>>> drivers/net/ethernet/intel/ice/Makefile | 4 +-
>>>>>>>> drivers/net/ethernet/intel/ice/ice.h | 18 +
>>>>>>>> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
>>>>>>>> drivers/net/ethernet/intel/ice/ice_common.c | 5 +
>>>>>>>> drivers/net/ethernet/intel/ice/ice_debugfs.c | 450
>>>>>>>> ++++++++++++++++++
>>>>>>>> drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
>>>>>>>> drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
>>>>>>>> drivers/net/ethernet/intel/ice/ice_main.c | 21 +
>>>>>>>> drivers/net/ethernet/intel/ice/ice_type.h | 4 +
>>>>>>>> 9 files changed, 867 insertions(+), 1 deletion(-)
>>>>>>>> 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
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/ethernet/intel/ice/Makefile
>>>>>>>> b/drivers/net/ethernet/intel/ice/Makefile
>>>>>>>> index 960277d78e09..d43a59e5f8ee 100644
>>>>>>>> --- a/drivers/net/ethernet/intel/ice/Makefile
>>>>>>>> +++ b/drivers/net/ethernet/intel/ice/Makefile
>>>>>>>> @@ -34,7 +34,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 \
>>>>>>>> @@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
>>>>>>>> ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
>>>>>>>> ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
>>>>>>>> ice-$(CONFIG_GNSS) += ice_gnss.o
>>>>>>>> +ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
>>>>>>>> diff --git a/drivers/net/ethernet/intel/ice/ice.h
>>>>>>>> b/drivers/net/ethernet/intel/ice/ice.h
>>>>>>>> index 5ac0ad12f9f1..e6dd9f6f9eee 100644
>>>>>>>> --- a/drivers/net/ethernet/intel/ice/ice.h
>>>>>>>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>>>>>>>> @@ -556,6 +556,8 @@ 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 */
>>>>>>>> + struct dentry *ice_debugfs_pf;
>>>>>>>> + struct dentry *ice_debugfs_pf_fwlog;
>>>>>>>> struct ice_vfs vfs;
>>>>>>>> DECLARE_BITMAP(features, ICE_F_MAX);
>>>>>>>> DECLARE_BITMAP(state, ICE_STATE_NBITS);
>>>>>>>> @@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct
>>>>>>>> ice_pf *pf)
>>>>>>>> return false;
>>>>>>>> }
>>>>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>>>>
>>>>>>> There is no need in this CONFIG_DEBUG_FS and code should be written
>>>>>>> without debugfs stubs.
>>>>>>>
>>>>>>
>>>>>> I don't understand this comment... If the kernel is configured
>>>>>> *without*
>>>>>> debugfs, won't the kernel fail to compile due to missing functions
>>>>>> if we
>>>>>> don't do this?
>>>>>
>>>>> It will work fine, see include/linux/debugfs.h.
>>>>
>>>> Nice, as-is impl of ice_debugfs_fwlog_init() would just fail on first
>>>> debugfs API call.
>>>>
>>>
>>> I've thought about this some more and I am confused what to do. In the
>>> Makefile there is a bit that removes ice_debugfs.o if CONFIG_DEBUG_FS is
>>> not set. This would result in the stubs being needed (since the
>>> functions are called from ice_fwlog.c). In this case the code would not
>>> compile (since the functions would be missing). Should I remove the code
>>> from the Makefile that deals with ice_debugfs.o (which doesn't make
>>> sense since then there will be code in the driver that doesn't get used)
>>> or do I leave the stubs in?
>>>
>>
>> These stubs are for functions we have in ice_debugfs.o?
>
> All of the subs except for 1 (which could be moved) are in ice_debugfs.o
>
>> Is there an ice_debugfs.h? We should implement stubs for those
>> functions there so we don't have to check CONFIG_DEBUG_FS.
>
> There isn't an ice_debugfs.h, but moving the stubs there would have the
> same issue correct? You would have to wrap them with CONFIG_DEBUG_FS no
> matter where they are, right?
>
Never mind all of this. I looked around some more and as Leon said
earlier, we don't need the stubs and we should not be using the Makefile
to remove the code if CONFIG_DEBUG_FS is not set. We should always
include ice_debugfs.o when building the driver and the calls to
debugfs_<whatever> will fail if CONFIG_DEBUG_FS is not set.
I'll change the code to reflect what I just said (and what Leon
originally said). Sorry for the noise.
>> Or, if they don’t' really belong there move them into another file
>> that isn't stripped out with CONFIG_DEBUG_FS=n.
>>
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH net-next v3 2/5] ice: configure FW logging
2023-08-21 23:20 ` Paul M Stillwell Jr
2023-08-22 7:33 ` Przemek Kitszel
2023-08-22 20:44 ` Keller, Jacob E
@ 2023-08-22 20:45 ` Keller, Jacob E
2023-08-22 20:59 ` Paul M Stillwell Jr
2 siblings, 1 reply; 21+ messages in thread
From: Keller, Jacob E @ 2023-08-22 20:45 UTC (permalink / raw)
To: Stillwell Jr, Paul M, Kitszel, Przemyslaw, Leon Romanovsky
Cc: Nguyen, Anthony L, davem, kuba, pabeni, edumazet, netdev, horms,
Pucha, HimasekharX Reddy
> -----Original Message-----
> From: Stillwell Jr, Paul M <paul.m.stillwell.jr@intel.com>
> Sent: Monday, August 21, 2023 4:21 PM
> To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Leon Romanovsky
> <leon@kernel.org>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net;
> kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
> netdev@vger.kernel.org; Keller, Jacob E <jacob.e.keller@intel.com>;
> horms@kernel.org; Pucha, HimasekharX Reddy
> <himasekharx.reddy.pucha@intel.com>
> Subject: Re: [PATCH net-next v3 2/5] ice: configure FW logging
>
> On 8/18/2023 5:31 AM, Przemek Kitszel wrote:
> > On 8/18/23 13:10, Leon Romanovsky wrote:
> >> On Thu, Aug 17, 2023 at 02:25:34PM -0700, Paul M Stillwell Jr wrote:
> >>> On 8/15/2023 11:38 AM, Leon Romanovsky wrote:
> >>>> On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
> >>>>> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> >>>>>
> >>>>> Users want the ability to debug FW issues by retrieving the
> >>>>> FW logs from the E8xx devices. Use debugfs to allow the user to
> >>>>> read/write the FW log configuration by adding a 'fwlog/modules' file.
> >>>>> Reading the file will show either the currently running
> >>>>> configuration or
> >>>>> the next configuration (if the user has changed the configuration).
> >>>>> Writing to the file will update the configuration, but NOT enable the
> >>>>> configuration (that is a separate command).
> >>>>>
> >>>>> To see the status of FW logging then read the 'fwlog/modules' file
> >>>>> like
> >>>>> this:
> >>>>>
> >>>>> cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
> >>>>>
> >>>>> To change the configuration of FW logging then write to the
> >>>>> 'fwlog/modules'
> >>>>> file like this:
> >>>>>
> >>>>> echo DCB NORMAL >
> /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
> >>>>>
> >>>>> The format to change the configuration is
> >>>>>
> >>>>> echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device
> >>>>
> >>>> This line is truncated, it is not clear where you are writing.
> >>>
> >>> Good catch, I don't know how I missed this... Will fix
> >>>
> >>>> And more general question, a long time ago, netdev had a policy of
> >>>> not-allowing writing to debugfs, was it changed?
> >>>>
> >>>
> >>> I had this same thought and it seems like there were 2 concerns in
> >>> the past
> >>
> >> Maybe, I'm not enough time in netdev world to know the history.
> >>
> >>>
> >>> 1. Having a single file that was read/write with lots of commands going
> >>> through it
> >>> 2. Having code in the driver to parse the text from the commands that
> >>> was
> >>> error/security prone
> >>>
> >>> We have addressed this in 2 ways:
> >>> 1. Split the commands into multiple files that have a single purpose
> >>> 2. Use kernel parsing functions for anything where we *have* to pass
> >>> text to
> >>> decode
> >>>
> >>>>>
> >>>>> where
> >>>>>
> >>>>> * fwlog_level is a name as described below. Each level includes the
> >>>>> messages from the previous/lower level
> >>>>>
> >>>>> * NONE
> >>>>> * ERROR
> >>>>> * WARNING
> >>>>> * NORMAL
> >>>>> * VERBOSE
> >>>>>
> >>>>> * fwlog_event is a name that represents the module to receive
> >>>>> events for.
> >>>>> The module names are
> >>>>>
> >>>>> * GENERAL
> >>>>> * CTRL
> >>>>> * LINK
> >>>>> * LINK_TOPO
> >>>>> * DNL
> >>>>> * I2C
> >>>>> * SDP
> >>>>> * MDIO
> >>>>> * ADMINQ
> >>>>> * HDMA
> >>>>> * LLDP
> >>>>> * DCBX
> >>>>> * DCB
> >>>>> * XLR
> >>>>> * NVM
> >>>>> * AUTH
> >>>>> * VPD
> >>>>> * IOSF
> >>>>> * PARSER
> >>>>> * SW
> >>>>> * SCHEDULER
> >>>>> * TXQ
> >>>>> * RSVD
> >>>>> * POST
> >>>>> * WATCHDOG
> >>>>> * TASK_DISPATCH
> >>>>> * MNG
> >>>>> * SYNCE
> >>>>> * HEALTH
> >>>>> * TSDRV
> >>>>> * PFREG
> >>>>> * MDLVER
> >>>>> * ALL
> >>>>>
> >>>>> The name ALL is special and specifies setting all of the modules to
> >>>>> the
> >>>>> specified fwlog_level.
> >>>>>
> >>>>> If the NVM supports FW logging then the file 'fwlog' will be created
> >>>>> under the PCI device ID for the ice driver. If the file does not exist
> >>>>> then either the NVM doesn't support FW logging or debugfs is not
> >>>>> enabled
> >>>>> on the system.
> >>>>>
> >>>>> In addition to configuring the modules, the user can also configure
> >>>>> the
> >>>>> number of log messages (resolution) to include in a single Admin
> >>>>> Receive
> >>>>> Queue (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.
> >>>>>
> >>>>> To see/change the resolution the user can read/write the
> >>>>> 'fwlog/resolution' file. An example changing the value to 50 is
> >>>>>
> >>>>> echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
> >>>>>
> >>>>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> >>>>> Tested-by: Pucha Himasekhar Reddy
> >>>>> <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
> >>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> >>>>> ---
> >>>>> drivers/net/ethernet/intel/ice/Makefile | 4 +-
> >>>>> drivers/net/ethernet/intel/ice/ice.h | 18 +
> >>>>> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
> >>>>> drivers/net/ethernet/intel/ice/ice_common.c | 5 +
> >>>>> drivers/net/ethernet/intel/ice/ice_debugfs.c | 450
> >>>>> ++++++++++++++++++
> >>>>> drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
> >>>>> drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
> >>>>> drivers/net/ethernet/intel/ice/ice_main.c | 21 +
> >>>>> drivers/net/ethernet/intel/ice/ice_type.h | 4 +
> >>>>> 9 files changed, 867 insertions(+), 1 deletion(-)
> >>>>> 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
> >>>>>
> >>>>> diff --git a/drivers/net/ethernet/intel/ice/Makefile
> >>>>> b/drivers/net/ethernet/intel/ice/Makefile
> >>>>> index 960277d78e09..d43a59e5f8ee 100644
> >>>>> --- a/drivers/net/ethernet/intel/ice/Makefile
> >>>>> +++ b/drivers/net/ethernet/intel/ice/Makefile
> >>>>> @@ -34,7 +34,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 \
> >>>>> @@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
> >>>>> ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
> >>>>> ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
> >>>>> ice-$(CONFIG_GNSS) += ice_gnss.o
> >>>>> +ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
> >>>>> diff --git a/drivers/net/ethernet/intel/ice/ice.h
> >>>>> b/drivers/net/ethernet/intel/ice/ice.h
> >>>>> index 5ac0ad12f9f1..e6dd9f6f9eee 100644
> >>>>> --- a/drivers/net/ethernet/intel/ice/ice.h
> >>>>> +++ b/drivers/net/ethernet/intel/ice/ice.h
> >>>>> @@ -556,6 +556,8 @@ 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 */
> >>>>> + struct dentry *ice_debugfs_pf;
> >>>>> + struct dentry *ice_debugfs_pf_fwlog;
> >>>>> struct ice_vfs vfs;
> >>>>> DECLARE_BITMAP(features, ICE_F_MAX);
> >>>>> DECLARE_BITMAP(state, ICE_STATE_NBITS);
> >>>>> @@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct
> >>>>> ice_pf *pf)
> >>>>> return false;
> >>>>> }
> >>>>> +#ifdef CONFIG_DEBUG_FS
> >>>>
> >>>> There is no need in this CONFIG_DEBUG_FS and code should be written
> >>>> without debugfs stubs.
> >>>>
> >>>
> >>> I don't understand this comment... If the kernel is configured *without*
> >>> debugfs, won't the kernel fail to compile due to missing functions if we
> >>> don't do this?
> >>
> >> It will work fine, see include/linux/debugfs.h.
> >
> > Nice, as-is impl of ice_debugfs_fwlog_init() would just fail on first
> > debugfs API call.
> >
>
> I've thought about this some more and I am confused what to do. In the
> Makefile there is a bit that removes ice_debugfs.o if CONFIG_DEBUG_FS is
> not set. This would result in the stubs being needed (since the
> functions are called from ice_fwlog.c). In this case the code would not
> compile (since the functions would be missing). Should I remove the code
> from the Makefile that deals with ice_debugfs.o (which doesn't make
> sense since then there will be code in the driver that doesn't get used)
> or do I leave the stubs in?
>
Or, since ice_fwlog depends on debugfs support, also strip the fwlog.o object when CONFIG_DEBUGFS=n.
Thanks.
Jake
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v3 2/5] ice: configure FW logging
2023-08-22 20:45 ` Keller, Jacob E
@ 2023-08-22 20:59 ` Paul M Stillwell Jr
0 siblings, 0 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2023-08-22 20:59 UTC (permalink / raw)
To: Keller, Jacob E, Kitszel, Przemyslaw, Leon Romanovsky
Cc: Nguyen, Anthony L, davem, kuba, pabeni, edumazet, netdev, horms,
Pucha, HimasekharX Reddy
On 8/22/2023 1:45 PM, Keller, Jacob E wrote:
>
>
>> -----Original Message-----
>> From: Stillwell Jr, Paul M <paul.m.stillwell.jr@intel.com>
>> Sent: Monday, August 21, 2023 4:21 PM
>> To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Leon Romanovsky
>> <leon@kernel.org>
>> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net;
>> kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
>> netdev@vger.kernel.org; Keller, Jacob E <jacob.e.keller@intel.com>;
>> horms@kernel.org; Pucha, HimasekharX Reddy
>> <himasekharx.reddy.pucha@intel.com>
>> Subject: Re: [PATCH net-next v3 2/5] ice: configure FW logging
>>
>> On 8/18/2023 5:31 AM, Przemek Kitszel wrote:
>>> On 8/18/23 13:10, Leon Romanovsky wrote:
>>>> On Thu, Aug 17, 2023 at 02:25:34PM -0700, Paul M Stillwell Jr wrote:
>>>>> On 8/15/2023 11:38 AM, Leon Romanovsky wrote:
>>>>>> On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
>>>>>>> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>>>>>>
>>>>>>> Users want the ability to debug FW issues by retrieving the
>>>>>>> FW logs from the E8xx devices. Use debugfs to allow the user to
>>>>>>> read/write the FW log configuration by adding a 'fwlog/modules' file.
>>>>>>> Reading the file will show either the currently running
>>>>>>> configuration or
>>>>>>> the next configuration (if the user has changed the configuration).
>>>>>>> Writing to the file will update the configuration, but NOT enable the
>>>>>>> configuration (that is a separate command).
>>>>>>>
>>>>>>> To see the status of FW logging then read the 'fwlog/modules' file
>>>>>>> like
>>>>>>> this:
>>>>>>>
>>>>>>> cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>>>>>>
>>>>>>> To change the configuration of FW logging then write to the
>>>>>>> 'fwlog/modules'
>>>>>>> file like this:
>>>>>>>
>>>>>>> echo DCB NORMAL >
>> /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>>>>>>
>>>>>>> The format to change the configuration is
>>>>>>>
>>>>>>> echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device
>>>>>>
>>>>>> This line is truncated, it is not clear where you are writing.
>>>>>
>>>>> Good catch, I don't know how I missed this... Will fix
>>>>>
>>>>>> And more general question, a long time ago, netdev had a policy of
>>>>>> not-allowing writing to debugfs, was it changed?
>>>>>>
>>>>>
>>>>> I had this same thought and it seems like there were 2 concerns in
>>>>> the past
>>>>
>>>> Maybe, I'm not enough time in netdev world to know the history.
>>>>
>>>>>
>>>>> 1. Having a single file that was read/write with lots of commands going
>>>>> through it
>>>>> 2. Having code in the driver to parse the text from the commands that
>>>>> was
>>>>> error/security prone
>>>>>
>>>>> We have addressed this in 2 ways:
>>>>> 1. Split the commands into multiple files that have a single purpose
>>>>> 2. Use kernel parsing functions for anything where we *have* to pass
>>>>> text to
>>>>> decode
>>>>>
>>>>>>>
>>>>>>> where
>>>>>>>
>>>>>>> * fwlog_level is a name as described below. Each level includes the
>>>>>>> messages from the previous/lower level
>>>>>>>
>>>>>>> * NONE
>>>>>>> * ERROR
>>>>>>> * WARNING
>>>>>>> * NORMAL
>>>>>>> * VERBOSE
>>>>>>>
>>>>>>> * fwlog_event is a name that represents the module to receive
>>>>>>> events for.
>>>>>>> The module names are
>>>>>>>
>>>>>>> * GENERAL
>>>>>>> * CTRL
>>>>>>> * LINK
>>>>>>> * LINK_TOPO
>>>>>>> * DNL
>>>>>>> * I2C
>>>>>>> * SDP
>>>>>>> * MDIO
>>>>>>> * ADMINQ
>>>>>>> * HDMA
>>>>>>> * LLDP
>>>>>>> * DCBX
>>>>>>> * DCB
>>>>>>> * XLR
>>>>>>> * NVM
>>>>>>> * AUTH
>>>>>>> * VPD
>>>>>>> * IOSF
>>>>>>> * PARSER
>>>>>>> * SW
>>>>>>> * SCHEDULER
>>>>>>> * TXQ
>>>>>>> * RSVD
>>>>>>> * POST
>>>>>>> * WATCHDOG
>>>>>>> * TASK_DISPATCH
>>>>>>> * MNG
>>>>>>> * SYNCE
>>>>>>> * HEALTH
>>>>>>> * TSDRV
>>>>>>> * PFREG
>>>>>>> * MDLVER
>>>>>>> * ALL
>>>>>>>
>>>>>>> The name ALL is special and specifies setting all of the modules to
>>>>>>> the
>>>>>>> specified fwlog_level.
>>>>>>>
>>>>>>> If the NVM supports FW logging then the file 'fwlog' will be created
>>>>>>> under the PCI device ID for the ice driver. If the file does not exist
>>>>>>> then either the NVM doesn't support FW logging or debugfs is not
>>>>>>> enabled
>>>>>>> on the system.
>>>>>>>
>>>>>>> In addition to configuring the modules, the user can also configure
>>>>>>> the
>>>>>>> number of log messages (resolution) to include in a single Admin
>>>>>>> Receive
>>>>>>> Queue (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.
>>>>>>>
>>>>>>> To see/change the resolution the user can read/write the
>>>>>>> 'fwlog/resolution' file. An example changing the value to 50 is
>>>>>>>
>>>>>>> echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
>>>>>>>
>>>>>>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>>>>>> Tested-by: Pucha Himasekhar Reddy
>>>>>>> <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
>>>>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>>>>>> ---
>>>>>>> drivers/net/ethernet/intel/ice/Makefile | 4 +-
>>>>>>> drivers/net/ethernet/intel/ice/ice.h | 18 +
>>>>>>> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
>>>>>>> drivers/net/ethernet/intel/ice/ice_common.c | 5 +
>>>>>>> drivers/net/ethernet/intel/ice/ice_debugfs.c | 450
>>>>>>> ++++++++++++++++++
>>>>>>> drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
>>>>>>> drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
>>>>>>> drivers/net/ethernet/intel/ice/ice_main.c | 21 +
>>>>>>> drivers/net/ethernet/intel/ice/ice_type.h | 4 +
>>>>>>> 9 files changed, 867 insertions(+), 1 deletion(-)
>>>>>>> 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
>>>>>>>
>>>>>>> diff --git a/drivers/net/ethernet/intel/ice/Makefile
>>>>>>> b/drivers/net/ethernet/intel/ice/Makefile
>>>>>>> index 960277d78e09..d43a59e5f8ee 100644
>>>>>>> --- a/drivers/net/ethernet/intel/ice/Makefile
>>>>>>> +++ b/drivers/net/ethernet/intel/ice/Makefile
>>>>>>> @@ -34,7 +34,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 \
>>>>>>> @@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
>>>>>>> ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
>>>>>>> ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
>>>>>>> ice-$(CONFIG_GNSS) += ice_gnss.o
>>>>>>> +ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
>>>>>>> diff --git a/drivers/net/ethernet/intel/ice/ice.h
>>>>>>> b/drivers/net/ethernet/intel/ice/ice.h
>>>>>>> index 5ac0ad12f9f1..e6dd9f6f9eee 100644
>>>>>>> --- a/drivers/net/ethernet/intel/ice/ice.h
>>>>>>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>>>>>>> @@ -556,6 +556,8 @@ 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 */
>>>>>>> + struct dentry *ice_debugfs_pf;
>>>>>>> + struct dentry *ice_debugfs_pf_fwlog;
>>>>>>> struct ice_vfs vfs;
>>>>>>> DECLARE_BITMAP(features, ICE_F_MAX);
>>>>>>> DECLARE_BITMAP(state, ICE_STATE_NBITS);
>>>>>>> @@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct
>>>>>>> ice_pf *pf)
>>>>>>> return false;
>>>>>>> }
>>>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>>>
>>>>>> There is no need in this CONFIG_DEBUG_FS and code should be written
>>>>>> without debugfs stubs.
>>>>>>
>>>>>
>>>>> I don't understand this comment... If the kernel is configured *without*
>>>>> debugfs, won't the kernel fail to compile due to missing functions if we
>>>>> don't do this?
>>>>
>>>> It will work fine, see include/linux/debugfs.h.
>>>
>>> Nice, as-is impl of ice_debugfs_fwlog_init() would just fail on first
>>> debugfs API call.
>>>
>>
>> I've thought about this some more and I am confused what to do. In the
>> Makefile there is a bit that removes ice_debugfs.o if CONFIG_DEBUG_FS is
>> not set. This would result in the stubs being needed (since the
>> functions are called from ice_fwlog.c). In this case the code would not
>> compile (since the functions would be missing). Should I remove the code
>> from the Makefile that deals with ice_debugfs.o (which doesn't make
>> sense since then there will be code in the driver that doesn't get used)
>> or do I leave the stubs in?
>>
>
> Or, since ice_fwlog depends on debugfs support, also strip the fwlog.o object when CONFIG_DEBUGFS=n.
>
We should do this as well I think. That makes sense to me, but doesn't
solve all the issues since the code in question gets called from ice_main.c
> Thanks.
> Jake
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net-next v3 2/5] ice: configure FW logging
2023-08-15 18:38 ` Leon Romanovsky
2023-08-17 21:25 ` Paul M Stillwell Jr
@ 2023-08-23 22:23 ` Paul M Stillwell Jr
1 sibling, 0 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2023-08-23 22:23 UTC (permalink / raw)
To: Leon Romanovsky, Tony Nguyen
Cc: davem, kuba, pabeni, edumazet, netdev, jacob.e.keller, horms,
Pucha Himasekhar Reddy
On 8/15/2023 11:38 AM, Leon Romanovsky wrote:
> On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
>> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>
>> Users want the ability to debug FW issues by retrieving the
>> FW logs from the E8xx devices. Use debugfs to allow the user to
>> read/write the FW log configuration by adding a 'fwlog/modules' file.
>> Reading the file will show either the currently running configuration or
>> the next configuration (if the user has changed the configuration).
>> Writing to the file will update the configuration, but NOT enable the
>> configuration (that is a separate command).
>>
>> To see the status of FW logging then read the 'fwlog/modules' file like
>> this:
>>
>> cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>
>> To change the configuration of FW logging then write to the 'fwlog/modules'
>> file like this:
>>
>> echo DCB NORMAL > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>>
>> The format to change the configuration is
>>
>> echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device
>
> This line is truncated, it is not clear where you are writing.
Fixed
> And more general question, a long time ago, netdev had a policy of
> not-allowing writing to debugfs, was it changed?
>
>>
>> where
>>
>> * fwlog_level is a name as described below. Each level includes the
>> messages from the previous/lower level
>>
>> * NONE
>> * ERROR
>> * WARNING
>> * NORMAL
>> * VERBOSE
>>
>> * fwlog_event is a name that represents the module to receive events for.
>> The module names are
>>
>> * GENERAL
>> * CTRL
>> * LINK
>> * LINK_TOPO
>> * DNL
>> * I2C
>> * SDP
>> * MDIO
>> * ADMINQ
>> * HDMA
>> * LLDP
>> * DCBX
>> * DCB
>> * XLR
>> * NVM
>> * AUTH
>> * VPD
>> * IOSF
>> * PARSER
>> * SW
>> * SCHEDULER
>> * TXQ
>> * RSVD
>> * POST
>> * WATCHDOG
>> * TASK_DISPATCH
>> * MNG
>> * SYNCE
>> * HEALTH
>> * TSDRV
>> * PFREG
>> * MDLVER
>> * ALL
>>
>> The name ALL is special and specifies setting all of the modules to the
>> specified fwlog_level.
>>
>> If the NVM supports FW logging then the file 'fwlog' will be created
>> under the PCI device ID for the ice driver. If the file does not exist
>> then either the NVM doesn't support FW logging or debugfs is not enabled
>> on the system.
>>
>> In addition to configuring the modules, the user can also configure the
>> number of log messages (resolution) to include in a single Admin Receive
>> Queue (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.
>>
>> To see/change the resolution the user can read/write the
>> 'fwlog/resolution' file. An example changing the value to 50 is
>>
>> echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
>>
>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>> Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>> ---
>> drivers/net/ethernet/intel/ice/Makefile | 4 +-
>> drivers/net/ethernet/intel/ice/ice.h | 18 +
>> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
>> drivers/net/ethernet/intel/ice/ice_common.c | 5 +
>> drivers/net/ethernet/intel/ice/ice_debugfs.c | 450 ++++++++++++++++++
>> drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
>> drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
>> drivers/net/ethernet/intel/ice/ice_main.c | 21 +
>> drivers/net/ethernet/intel/ice/ice_type.h | 4 +
>> 9 files changed, 867 insertions(+), 1 deletion(-)
>> 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
>>
>> diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
>> index 960277d78e09..d43a59e5f8ee 100644
>> --- a/drivers/net/ethernet/intel/ice/Makefile
>> +++ b/drivers/net/ethernet/intel/ice/Makefile
>> @@ -34,7 +34,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 \
>> @@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
>> ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
>> ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
>> ice-$(CONFIG_GNSS) += ice_gnss.o
>> +ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
>> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
>> index 5ac0ad12f9f1..e6dd9f6f9eee 100644
>> --- a/drivers/net/ethernet/intel/ice/ice.h
>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>> @@ -556,6 +556,8 @@ 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 */
>> + struct dentry *ice_debugfs_pf;
>> + struct dentry *ice_debugfs_pf_fwlog;
>> struct ice_vfs vfs;
>> DECLARE_BITMAP(features, ICE_F_MAX);
>> DECLARE_BITMAP(state, ICE_STATE_NBITS);
>> @@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct ice_pf *pf)
>> return false;
>> }
>>
>> +#ifdef CONFIG_DEBUG_FS
>
> There is no need in this CONFIG_DEBUG_FS and code should be written
> without debugfs stubs.
>
Fixed
>> +void ice_debugfs_fwlog_init(struct ice_pf *pf);
>> +void ice_debugfs_init(void);
>> +void ice_debugfs_exit(void);
>> +void ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module);
>> +#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) { }
>> +static void
>> +ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +#endif /* CONFIG_DEBUG_FS */
>
> Thanks
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v3 3/5] ice: enable FW logging
2023-08-15 16:57 [PATCH net-next v3 0/5][pull request] add v2 FW logging for ice driver Tony Nguyen
2023-08-15 16:57 ` [PATCH net-next v3 1/5] ice: remove FW logging code Tony Nguyen
2023-08-15 16:57 ` [PATCH net-next v3 2/5] ice: configure FW logging Tony Nguyen
@ 2023-08-15 16:57 ` Tony Nguyen
2023-08-15 16:57 ` [PATCH net-next v3 4/5] ice: add ability to read FW log data and configure the number of log buffers Tony Nguyen
2023-08-15 16:57 ` [PATCH net-next v3 5/5] ice: add documentation for FW logging Tony Nguyen
4 siblings, 0 replies; 21+ messages in thread
From: Tony Nguyen @ 2023-08-15 16:57 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, netdev
Cc: Paul M Stillwell Jr, anthony.l.nguyen, jacob.e.keller, horms,
Pucha Himasekhar Reddy
From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
Once users have configured the FW logging then allow them to enable it
by writing to the 'fwlog/enable' file. The file accepts a boolean value
(0 or 1) where 1 means enable FW logging and 0 means disable FW logging.
The user can read the 'fwlog/enable' file to see whether logging is
enabled or not. Reading the actual data is a separate patch.
Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
.../net/ethernet/intel/ice/ice_adminq_cmd.h | 3 +
drivers/net/ethernet/intel/ice/ice_common.c | 1 +
drivers/net/ethernet/intel/ice/ice_debugfs.c | 115 ++++++++++++++++++
drivers/net/ethernet/intel/ice/ice_fwlog.c | 91 ++++++++++++++
drivers/net/ethernet/intel/ice/ice_fwlog.h | 3 +
5 files changed, 213 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index facd662a2768..a8d2f4cab168 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -2121,6 +2121,7 @@ enum ice_aqc_fw_logging_mod {
};
/* Set FW Logging configuration (indirect 0xFF30)
+ * Register for FW Logging (indirect 0xFF31)
* Query FW Logging (indirect 0xFF32)
*/
struct ice_aqc_fw_log {
@@ -2129,6 +2130,7 @@ struct ice_aqc_fw_log {
#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)
u8 rsp_flag;
@@ -2422,6 +2424,7 @@ enum ice_adminq_opc {
/* FW Logging Commands */
ice_aqc_opc_fw_logs_config = 0xFF30,
+ ice_aqc_opc_fw_logs_register = 0xFF31,
ice_aqc_opc_fw_logs_query = 0xFF32,
};
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 7bfd965b7eca..95e57db76557 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1033,6 +1033,7 @@ void ice_deinit_hw(struct ice_hw *hw)
ice_free_hw_tbls(hw);
mutex_destroy(&hw->tnl_lock);
+ ice_fwlog_deinit(hw);
ice_destroy_all_ctrlq(hw);
/* Clear VSI contexts if not already cleared */
diff --git a/drivers/net/ethernet/intel/ice/ice_debugfs.c b/drivers/net/ethernet/intel/ice/ice_debugfs.c
index e354c7287ff6..104ea962adee 100644
--- a/drivers/net/ethernet/intel/ice/ice_debugfs.c
+++ b/drivers/net/ethernet/intel/ice/ice_debugfs.c
@@ -397,6 +397,118 @@ static const struct file_operations ice_debugfs_resolution_fops = {
.write = ice_debugfs_resolution_write,
};
+/**
+ * ice_debugfs_enable_read - read from 'enable' file
+ * @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_enable_read(struct file *filp,
+ char __user *buffer, size_t count,
+ loff_t *ppos)
+{
+ struct ice_pf *pf = filp->private_data;
+ struct ice_hw *hw = &pf->hw;
+ char buff[32] = {};
+ int status;
+
+ /* don't allow commands if the FW doesn't support it */
+ if (!ice_fwlog_supported(&pf->hw))
+ return -EOPNOTSUPP;
+
+ snprintf(buff, sizeof(buff), "%u\n",
+ (u16)(hw->fwlog_cfg.options &
+ ICE_FWLOG_OPTION_IS_REGISTERED) >> 3);
+
+ status = simple_read_from_buffer(buffer, count, ppos, buff,
+ strlen(buff));
+
+ return status;
+}
+
+/**
+ * ice_debugfs_enable_write - write into 'enable' file
+ * @filp: the opened file
+ * @buf: where to find the user's data
+ * @count: the length of the user's data
+ * @ppos: file position offset
+ */
+static ssize_t
+ice_debugfs_enable_write(struct file *filp, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct ice_pf *pf = filp->private_data;
+ struct device *dev = ice_pf_to_dev(pf);
+ struct ice_hw *hw = &pf->hw;
+ ssize_t ret;
+ char **argv;
+ int argc;
+
+ /* don't allow commands if the FW doesn't support it */
+ if (!ice_fwlog_supported(hw))
+ return -EOPNOTSUPP;
+
+ /* don't allow partial writes */
+ if (*ppos != 0)
+ return 0;
+
+ ret = ice_debugfs_parse_cmd_line(buf, count, &argv, &argc);
+ if (ret)
+ goto err_copy_from_user;
+
+ if (argc == 1) {
+ bool enable;
+
+ ret = kstrtobool(argv[0], &enable);
+ if (ret)
+ goto enable_write_error;
+
+ if (enable)
+ hw->fwlog_cfg.options |= ICE_FWLOG_OPTION_ARQ_ENA;
+ else
+ hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
+
+ ret = ice_fwlog_set(hw, &hw->fwlog_cfg);
+ if (ret)
+ goto enable_write_error;
+
+ if (enable)
+ ret = ice_fwlog_register(hw);
+ else
+ ret = ice_fwlog_unregister(hw);
+
+ if (ret)
+ goto enable_write_error;
+ } else {
+ dev_info(dev, "unknown or invalid command '%s'\n", argv[0]);
+ ret = -EINVAL;
+ goto enable_write_error;
+ }
+
+ /* if we get here, nothing went wrong; return bytes copied */
+ ret = (ssize_t)count;
+
+enable_write_error:
+ argv_free(argv);
+err_copy_from_user:
+ /* This function always consumes all of the written input, or produces
+ * an error. Check and enforce this. Otherwise, the write operation
+ * won't complete properly.
+ */
+ if (WARN_ON(ret != (ssize_t)count && ret >= 0))
+ ret = -EIO;
+
+ return ret;
+}
+
+static const struct file_operations ice_debugfs_enable_fops = {
+ .owner = THIS_MODULE,
+ .open = simple_open,
+ .read = ice_debugfs_enable_read,
+ .write = ice_debugfs_enable_write,
+};
+
/**
* ice_debugfs_fwlog_init - setup the debugfs directory
* @pf: the ice that is starting up
@@ -428,6 +540,9 @@ void ice_debugfs_fwlog_init(struct ice_pf *pf)
debugfs_create_file("resolution", 0600,
pf->ice_debugfs_pf_fwlog, pf,
&ice_debugfs_resolution_fops);
+
+ debugfs_create_file("enable", 0600, pf->ice_debugfs_pf_fwlog,
+ pf, &ice_debugfs_enable_fops);
}
/**
diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.c b/drivers/net/ethernet/intel/ice/ice_fwlog.c
index 1f4b474dcc97..ce857ddd4be8 100644
--- a/drivers/net/ethernet/intel/ice/ice_fwlog.c
+++ b/drivers/net/ethernet/intel/ice/ice_fwlog.c
@@ -32,6 +32,35 @@ int ice_fwlog_init(struct ice_hw *hw)
return 0;
}
+/**
+ * ice_fwlog_deinit - unroll FW logging configuration
+ * @hw: pointer to the HW structure
+ *
+ * This function should be called in ice_deinit_hw().
+ */
+void ice_fwlog_deinit(struct ice_hw *hw)
+{
+ int status;
+
+ /* only support fw log commands on PF 0 */
+ if (hw->bus.func)
+ return;
+
+ /* make sure FW logging is disabled to not put the FW in a weird state
+ * for the next driver load
+ */
+ hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
+ status = ice_fwlog_set(hw, &hw->fwlog_cfg);
+ if (status)
+ dev_warn(ice_hw_to_dev(hw), "Unable to turn off FW logging, status: %d\n",
+ status);
+
+ status = ice_fwlog_unregister(hw);
+ if (status)
+ dev_warn(ice_hw_to_dev(hw), "Unable to unregister FW logging, status: %d\n",
+ status);
+}
+
/**
* ice_fwlog_supported - Cached for whether FW supports FW logging or not
* @hw: pointer to the HW structure
@@ -164,6 +193,8 @@ static int ice_aq_fwlog_get(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
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;
@@ -196,6 +227,66 @@ int ice_fwlog_get(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
return ice_aq_fwlog_get(hw, cfg);
}
+/**
+ * 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_fwlog_set_supported - Set if FW logging is supported by FW
* @hw: pointer to the HW struct
diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.h b/drivers/net/ethernet/intel/ice/ice_fwlog.h
index 5a4194527cf9..45865558425d 100644
--- a/drivers/net/ethernet/intel/ice/ice_fwlog.h
+++ b/drivers/net/ethernet/intel/ice/ice_fwlog.h
@@ -50,6 +50,9 @@ struct ice_fwlog_cfg {
void ice_fwlog_set_supported(struct ice_hw *hw);
bool ice_fwlog_supported(struct ice_hw *hw);
int ice_fwlog_init(struct ice_hw *hw);
+void ice_fwlog_deinit(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_ */
--
2.38.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH net-next v3 4/5] ice: add ability to read FW log data and configure the number of log buffers
2023-08-15 16:57 [PATCH net-next v3 0/5][pull request] add v2 FW logging for ice driver Tony Nguyen
` (2 preceding siblings ...)
2023-08-15 16:57 ` [PATCH net-next v3 3/5] ice: enable " Tony Nguyen
@ 2023-08-15 16:57 ` Tony Nguyen
2023-08-15 16:57 ` [PATCH net-next v3 5/5] ice: add documentation for FW logging Tony Nguyen
4 siblings, 0 replies; 21+ messages in thread
From: Tony Nguyen @ 2023-08-15 16:57 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, netdev
Cc: Paul M Stillwell Jr, anthony.l.nguyen, jacob.e.keller, horms,
Pucha Himasekhar Reddy
From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
Once logging is enabled the user should read the data from the 'data'
file. The data is in the form of a binary blob that can be sent to Intel
for decoding. To read the data use a command like:
cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/data > log_data.bin
If the user wants to clear the FW log data that has been stored in the
driver then they can write a 0 to the 'data' file and that will clear
the data.
In addition to being able to read the data the user can configure how
many buffers can be used to store the FW logs within the driver. This
allows the user to increase/decrease the number of buffers used based on
the users situation. The buffers are used as a ring so if the driver
runs out of buffers then it will overwrite data. To change the number of
buffers the user can write to the 'nr_buffs' file like this:
echo 128 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/nr_buffs
The value written to the file must be a power of 2 value between 1 (not
recommended) and 512.
Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
.../net/ethernet/intel/ice/ice_adminq_cmd.h | 2 +
drivers/net/ethernet/intel/ice/ice_debugfs.c | 242 ++++++++++++++++++
drivers/net/ethernet/intel/ice/ice_fwlog.c | 130 +++++++++-
drivers/net/ethernet/intel/ice/ice_fwlog.h | 20 ++
drivers/net/ethernet/intel/ice/ice_main.c | 29 +++
drivers/net/ethernet/intel/ice/ice_type.h | 1 +
6 files changed, 423 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index a8d2f4cab168..8b1d2c4c46e9 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -2123,6 +2123,7 @@ enum ice_aqc_fw_logging_mod {
/* Set FW Logging configuration (indirect 0xFF30)
* Register for FW Logging (indirect 0xFF31)
* Query FW Logging (indirect 0xFF32)
+ * FW Log Event (indirect 0xFF33)
*/
struct ice_aqc_fw_log {
u8 cmd_flags;
@@ -2426,6 +2427,7 @@ enum ice_adminq_opc {
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_debugfs.c b/drivers/net/ethernet/intel/ice/ice_debugfs.c
index 104ea962adee..e3bc1f7a525c 100644
--- a/drivers/net/ethernet/intel/ice/ice_debugfs.c
+++ b/drivers/net/ethernet/intel/ice/ice_debugfs.c
@@ -509,6 +509,240 @@ static const struct file_operations ice_debugfs_enable_fops = {
.write = ice_debugfs_enable_write,
};
+/**
+ * ice_debugfs_nr_buffs_read - read from 'nr_buffs' file
+ * @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_nr_buffs_read(struct file *filp,
+ char __user *buffer, size_t count,
+ loff_t *ppos)
+{
+ struct ice_pf *pf = filp->private_data;
+ struct ice_hw *hw = &pf->hw;
+ char buff[32] = {};
+ int status;
+
+ /* don't allow commands if the FW doesn't support it */
+ if (!ice_fwlog_supported(&pf->hw))
+ return -EOPNOTSUPP;
+
+ snprintf(buff, sizeof(buff), "%d\n", hw->fwlog_ring.size);
+
+ status = simple_read_from_buffer(buffer, count, ppos, buff,
+ strlen(buff));
+
+ return status;
+}
+
+/**
+ * ice_debugfs_nr_buffs_write - write into 'nr_buffs' file
+ * @filp: the opened file
+ * @buf: where to find the user's data
+ * @count: the length of the user's data
+ * @ppos: file position offset
+ */
+static ssize_t
+ice_debugfs_nr_buffs_write(struct file *filp, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct ice_pf *pf = filp->private_data;
+ struct device *dev = ice_pf_to_dev(pf);
+ struct ice_hw *hw = &pf->hw;
+ ssize_t ret;
+ char **argv;
+ int argc;
+
+ /* don't allow commands if the FW doesn't support it */
+ if (!ice_fwlog_supported(hw))
+ return -EOPNOTSUPP;
+
+ /* don't allow partial writes */
+ if (*ppos != 0)
+ return 0;
+
+ ret = ice_debugfs_parse_cmd_line(buf, count, &argv, &argc);
+ if (ret)
+ goto err_copy_from_user;
+
+ if (argc == 1) {
+ s16 nr_buffs;
+
+ ret = kstrtos16(argv[0], 0, &nr_buffs);
+ if (ret)
+ goto nr_buffs_write_error;
+
+ if (nr_buffs <= 0 || nr_buffs > ICE_FWLOG_RING_SIZE_MAX) {
+ dev_info(dev, "nr_buffs '%d' is not within bounds. Please use a value between 1 and %d\n",
+ nr_buffs, ICE_FWLOG_RING_SIZE_MAX);
+ ret = -EINVAL;
+ goto nr_buffs_write_error;
+ } else if (hweight16(nr_buffs) > 1) {
+ dev_info(dev, "nr_buffs '%d' is not a power of 2. Please use a value that is a power of 2.\n",
+ nr_buffs);
+ ret = -EINVAL;
+ goto nr_buffs_write_error;
+ } else if (hw->fwlog_cfg.options &
+ ICE_FWLOG_OPTION_IS_REGISTERED) {
+ dev_info(dev, "FW logging is currently running. Please disable FW logging to change nr_buffs\n");
+ ret = -EINVAL;
+ goto nr_buffs_write_error;
+ }
+
+ /* free all the buffers and the tracking info and resize */
+ ice_fwlog_realloc_rings(hw, nr_buffs);
+ } else {
+ dev_info(dev, "unknown or invalid command '%s'\n", argv[0]);
+ ret = -EINVAL;
+ goto nr_buffs_write_error;
+ }
+
+ /* if we get here, nothing went wrong; return bytes copied */
+ ret = (ssize_t)count;
+
+nr_buffs_write_error:
+ argv_free(argv);
+err_copy_from_user:
+ /* This function always consumes all of the written input, or produces
+ * an error. Check and enforce this. Otherwise, the write operation
+ * won't complete properly.
+ */
+ if (WARN_ON(ret != (ssize_t)count && ret >= 0))
+ ret = -EIO;
+
+ return ret;
+}
+
+static const struct file_operations ice_debugfs_nr_buffs_fops = {
+ .owner = THIS_MODULE,
+ .open = simple_open,
+ .read = ice_debugfs_nr_buffs_read,
+ .write = ice_debugfs_nr_buffs_write,
+};
+
+/**
+ * ice_debugfs_data_read - read from 'data' file
+ * @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_data_read(struct file *filp, char __user *buffer,
+ size_t count, loff_t *ppos)
+{
+ struct ice_pf *pf = filp->private_data;
+ struct ice_hw *hw = &pf->hw;
+ int data_copied = 0;
+ bool done = false;
+
+ /* don't allow commands if the FW doesn't support it */
+ if (!ice_fwlog_supported(&pf->hw))
+ return -EOPNOTSUPP;
+
+ if (ice_fwlog_ring_empty(&hw->fwlog_ring))
+ return 0;
+
+ while (!ice_fwlog_ring_empty(&hw->fwlog_ring) && !done) {
+ struct ice_fwlog_data *log;
+ u16 cur_buf_len;
+
+ log = &hw->fwlog_ring.rings[hw->fwlog_ring.head];
+ cur_buf_len = log->data_size;
+
+ if (cur_buf_len >= count) {
+ done = true;
+ continue;
+ }
+
+ if (copy_to_user(buffer, log->data, cur_buf_len)) {
+ /* if there is an error then bail and return whatever
+ * the driver has copied so far
+ */
+ done = true;
+ continue;
+ }
+
+ data_copied += cur_buf_len;
+ buffer += cur_buf_len;
+ count -= cur_buf_len;
+ *ppos += cur_buf_len;
+ ice_fwlog_ring_increment(&hw->fwlog_ring.head,
+ hw->fwlog_ring.size);
+ }
+
+ return data_copied;
+}
+
+/**
+ * ice_debugfs_data_write - write into 'data' file
+ * @filp: the opened file
+ * @buf: where to find the user's data
+ * @count: the length of the user's data
+ * @ppos: file position offset
+ */
+static ssize_t
+ice_debugfs_data_write(struct file *filp, const char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ struct ice_pf *pf = filp->private_data;
+ struct device *dev = ice_pf_to_dev(pf);
+ struct ice_hw *hw = &pf->hw;
+ ssize_t ret;
+ char **argv;
+ int argc;
+
+ /* don't allow commands if the FW doesn't support it */
+ if (!ice_fwlog_supported(hw))
+ return -EOPNOTSUPP;
+
+ /* don't allow partial writes */
+ if (*ppos != 0)
+ return 0;
+
+ ret = ice_debugfs_parse_cmd_line(buf, count, &argv, &argc);
+ if (ret)
+ goto err_copy_from_user;
+
+ if (argc == 1) {
+ if (!(hw->fwlog_cfg.options & ICE_FWLOG_OPTION_IS_REGISTERED)) {
+ hw->fwlog_ring.head = 0;
+ hw->fwlog_ring.tail = 0;
+ } else {
+ dev_info(dev, "Can't clear FW log data while FW log running\n");
+ ret = -EINVAL;
+ goto nr_buffs_write_error;
+ }
+ } else {
+ dev_info(dev, "unknown or invalid command '%s'\n", argv[0]);
+ ret = -EINVAL;
+ goto nr_buffs_write_error;
+ }
+
+ /* if we get here, nothing went wrong; return bytes copied */
+ ret = (ssize_t)count;
+
+nr_buffs_write_error:
+ argv_free(argv);
+err_copy_from_user:
+ /* This function always consumes all of the written input, or produces
+ * an error. Check and enforce this. Otherwise, the write operation
+ * won't complete properly.
+ */
+ if (WARN_ON(ret != (ssize_t)count && ret >= 0))
+ ret = -EIO;
+
+ return ret;
+}
+
+static const struct file_operations ice_debugfs_data_fops = {
+ .owner = THIS_MODULE,
+ .open = simple_open,
+ .read = ice_debugfs_data_read,
+ .write = ice_debugfs_data_write,
+};
+
/**
* ice_debugfs_fwlog_init - setup the debugfs directory
* @pf: the ice that is starting up
@@ -543,6 +777,14 @@ void ice_debugfs_fwlog_init(struct ice_pf *pf)
debugfs_create_file("enable", 0600, pf->ice_debugfs_pf_fwlog,
pf, &ice_debugfs_enable_fops);
+
+ debugfs_create_file("nr_buffs", 0600, pf->ice_debugfs_pf_fwlog,
+ pf, &ice_debugfs_nr_buffs_fops);
+
+ debugfs_create_file("data", 0600, pf->ice_debugfs_pf_fwlog,
+ pf, &ice_debugfs_data_fops);
+
+ return;
}
/**
diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.c b/drivers/net/ethernet/intel/ice/ice_fwlog.c
index ce857ddd4be8..8cd9fdbaec90 100644
--- a/drivers/net/ethernet/intel/ice/ice_fwlog.c
+++ b/drivers/net/ethernet/intel/ice/ice_fwlog.c
@@ -1,10 +1,111 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2022, Intel Corporation. */
+#include <linux/vmalloc.h>
#include "ice.h"
#include "ice_common.h"
#include "ice_fwlog.h"
+bool ice_fwlog_ring_full(struct ice_fwlog_ring *rings)
+{
+ u16 head, tail;
+
+ head = rings->head;
+ tail = rings->tail;
+
+ if (head < tail && (tail - head == (rings->size - 1)))
+ return true;
+ else if (head > tail && (tail == (head - 1)))
+ return true;
+
+ return false;
+}
+
+bool ice_fwlog_ring_empty(struct ice_fwlog_ring *rings)
+{
+ return rings->head == rings->tail;
+}
+
+void ice_fwlog_ring_increment(u16 *item, u16 size)
+{
+ *item = (*item + 1) & (size - 1);
+}
+
+static int ice_fwlog_alloc_ring_buffs(struct ice_fwlog_ring *rings)
+{
+ int i, ret = 0;
+
+ for (i = 0; i < rings->size; i++) {
+ struct ice_fwlog_data *ring = &rings->rings[i];
+
+ ring->data_size = ICE_AQ_MAX_BUF_LEN;
+ ring->data = vzalloc(ring->data_size);
+ if (!ring->data) {
+ ret = -ENOMEM;
+ break;
+ }
+ }
+
+ return ret;
+}
+
+static void ice_fwlog_free_ring_buffs(struct ice_fwlog_ring *rings)
+{
+ int i;
+
+ for (i = 0; i < rings->size; i++) {
+ struct ice_fwlog_data *ring = &rings->rings[i];
+
+ vfree(ring->data);
+
+ ring->data_size = 0;
+ }
+}
+
+/**
+ * ice_fwlog_realloc_rings - reallocate the FW log rings
+ * @hw: pointer to the HW structure
+ * @ring_size: the new number of rings to allocate
+ *
+ */
+void ice_fwlog_realloc_rings(struct ice_hw *hw, int ring_size)
+{
+ struct ice_fwlog_ring ring;
+ int status;
+
+ if (ring_size == hw->fwlog_ring.size)
+ return;
+
+ /* allocate space for the new rings and buffers then release the
+ * old rings and buffers. that way if we don't have enough
+ * memory then we at least have what we had before
+ */
+ ring.rings = kcalloc(ICE_FWLOG_RING_SIZE_DFLT,
+ sizeof(*ring.rings), GFP_KERNEL);
+ if (!ring.rings) {
+ dev_warn(ice_hw_to_dev(hw), "Unable to allocate memory for FW log ring\n");
+ return;
+ }
+
+ ring.size = ring_size;
+
+ status = ice_fwlog_alloc_ring_buffs(&ring);
+ if (status) {
+ dev_warn(ice_hw_to_dev(hw), "Unable to allocate memory for FW log ring data buffers\n");
+ ice_fwlog_free_ring_buffs(&ring);
+ kfree(ring.rings);
+ return;
+ }
+
+ ice_fwlog_free_ring_buffs(&hw->fwlog_ring);
+ kfree(hw->fwlog_ring.rings);
+
+ hw->fwlog_ring.rings = ring.rings;
+ hw->fwlog_ring.size = ring.size;
+ hw->fwlog_ring.head = 0;
+ hw->fwlog_ring.tail = 0;
+}
+
/**
* ice_fwlog_init - Initialize FW logging configuration
* @hw: pointer to the HW structure
@@ -14,16 +115,38 @@
*/
int ice_fwlog_init(struct ice_hw *hw)
{
- int status;
+ /* only support fw log commands on PF 0 */
+ if (hw->bus.func)
+ return -EINVAL;
ice_fwlog_set_supported(hw);
if (ice_fwlog_supported(hw)) {
+ int status;
+
/* read the current config from the FW and store it */
status = ice_fwlog_get(hw, &hw->fwlog_cfg);
if (status)
return status;
+ hw->fwlog_ring.rings = kcalloc(ICE_FWLOG_RING_SIZE_DFLT,
+ sizeof(*hw->fwlog_ring.rings),
+ GFP_KERNEL);
+ if (!hw->fwlog_ring.rings) {
+ dev_warn(ice_hw_to_dev(hw), "Unable to allocate memory for FW log ring\n");
+ return -ENOMEM;
+ }
+
+ hw->fwlog_ring.size = ICE_FWLOG_RING_SIZE_DFLT;
+
+ status = ice_fwlog_alloc_ring_buffs(&hw->fwlog_ring);
+ if (status) {
+ dev_warn(ice_hw_to_dev(hw), "Unable to allocate memory for FW log ring data buffers\n");
+ ice_fwlog_free_ring_buffs(&hw->fwlog_ring);
+ kfree(hw->fwlog_ring.rings);
+ return status;
+ }
+
ice_debugfs_fwlog_init(hw->back);
} else {
dev_warn(ice_hw_to_dev(hw), "FW logging is not supported in this NVM image. Please update the NVM to get FW log support\n");
@@ -59,6 +182,11 @@ void ice_fwlog_deinit(struct ice_hw *hw)
if (status)
dev_warn(ice_hw_to_dev(hw), "Unable to unregister FW logging, status: %d\n",
status);
+
+ if (hw->fwlog_ring.rings) {
+ ice_fwlog_free_ring_buffs(&hw->fwlog_ring);
+ kfree(hw->fwlog_ring.rings);
+ }
}
/**
diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.h b/drivers/net/ethernet/intel/ice/ice_fwlog.h
index 45865558425d..89969be69970 100644
--- a/drivers/net/ethernet/intel/ice/ice_fwlog.h
+++ b/drivers/net/ethernet/intel/ice/ice_fwlog.h
@@ -47,12 +47,32 @@ struct ice_fwlog_cfg {
u16 log_resolution;
};
+struct ice_fwlog_data {
+ u16 data_size;
+ u8 *data;
+};
+
+struct ice_fwlog_ring {
+ struct ice_fwlog_data *rings;
+ u16 size;
+ u16 head;
+ u16 tail;
+};
+
+#define ICE_FWLOG_RING_SIZE_DFLT 256
+#define ICE_FWLOG_RING_SIZE_MAX 512
+
+bool ice_fwlog_ring_full(struct ice_fwlog_ring *rings);
+bool ice_fwlog_ring_empty(struct ice_fwlog_ring *rings);
+void ice_fwlog_ring_increment(u16 *item, u16 size);
void ice_fwlog_set_supported(struct ice_hw *hw);
bool ice_fwlog_supported(struct ice_hw *hw);
int ice_fwlog_init(struct ice_hw *hw);
void ice_fwlog_deinit(struct ice_hw *hw);
+void ice_fwlog_deinit(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);
+void ice_fwlog_realloc_rings(struct ice_hw *hw, int num_rings);
#endif /* _ICE_FWLOG_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 73c7dcfa8108..087ed73e865e 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -1250,6 +1250,32 @@ 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 ice_fwlog_data *fwlog;
+ struct ice_hw *hw = &pf->hw;
+
+ fwlog = &hw->fwlog_ring.rings[hw->fwlog_ring.tail];
+
+ memset(fwlog->data, 0, PAGE_SIZE);
+ fwlog->data_size = le16_to_cpu(event->desc.datalen);
+
+ memcpy(fwlog->data, event->msg_buf, fwlog->data_size);
+ ice_fwlog_ring_increment(&hw->fwlog_ring.tail, hw->fwlog_ring.size);
+
+ if (ice_fwlog_ring_full(&hw->fwlog_ring)) {
+ /* the rings are full so bump the head to create room */
+ ice_fwlog_ring_increment(&hw->fwlog_ring.head,
+ hw->fwlog_ring.size);
+ }
+}
+
enum ice_aq_task_state {
ICE_AQ_TASK_WAITING = 0,
ICE_AQ_TASK_COMPLETE,
@@ -1530,6 +1556,9 @@ static int __ice_clean_ctrlq(struct ice_pf *pf, enum ice_ctl_q q_type)
ice_vc_process_vf_msg(pf, &event, &data);
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;
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 16904cb8a25b..f1231a8162af 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -865,6 +865,7 @@ struct ice_hw {
struct ice_fwlog_cfg fwlog_cfg;
bool fwlog_supported; /* does hardware support FW logging? */
+ struct ice_fwlog_ring fwlog_ring;
/* Device max aggregate bandwidths corresponding to the GL_PWR_MODE_CTL
* register. Used for determining the ITR/INTRL granularity during
--
2.38.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH net-next v3 5/5] ice: add documentation for FW logging
2023-08-15 16:57 [PATCH net-next v3 0/5][pull request] add v2 FW logging for ice driver Tony Nguyen
` (3 preceding siblings ...)
2023-08-15 16:57 ` [PATCH net-next v3 4/5] ice: add ability to read FW log data and configure the number of log buffers Tony Nguyen
@ 2023-08-15 16:57 ` Tony Nguyen
2023-08-16 22:43 ` Randy Dunlap
4 siblings, 1 reply; 21+ messages in thread
From: Tony Nguyen @ 2023-08-15 16:57 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, netdev
Cc: Paul M Stillwell Jr, anthony.l.nguyen, jacob.e.keller, horms,
corbet, linux-doc
From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
Add documentation for FW logging in
Documentation/networking/device-drivers/ethernet/intel/ice.rst
Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
.../device_drivers/ethernet/intel/ice.rst | 117 ++++++++++++++++++
1 file changed, 117 insertions(+)
diff --git a/Documentation/networking/device_drivers/ethernet/intel/ice.rst b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
index e4d065c55ea8..3ddef911faaa 100644
--- a/Documentation/networking/device_drivers/ethernet/intel/ice.rst
+++ b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
@@ -895,6 +895,123 @@ driver writes raw bytes by the GNSS object to the receiver through i2c. Please
refer to the hardware GNSS module documentation for configuration details.
+Firmware (FW) logging
+---------------------
+The driver supports FW logging via the debugfs interface on PF 0 only. In order
+for FW logging to work, the NVM must support it. The 'fwlog' file will only get
+created in the ice debugfs directory if the NVM supports FW logging.
+
+Module configuration
+~~~~~~~~~~~~~~~~~~~~
+To see the status of FW logging then read the 'fwlog/modules' file like this::
+
+ # cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
+
+To configure FW logging then write to the 'fwlog/modules' file like this::
+
+ # echo <fwlog_event> <fwlog_level> > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
+
+where
+
+* fwlog_level is a name as described below. Each level includes the
+ messages from the previous/lower level
+
+ * NONE
+ * ERROR
+ * WARNING
+ * NORMAL
+ * VERBOSE
+
+* fwlog_event is a name that represents the module to receive events for. The
+ module names are
+
+ * GENERAL
+ * CTRL
+ * LINK
+ * LINK_TOPO
+ * DNL
+ * I2C
+ * SDP
+ * MDIO
+ * ADMINQ
+ * HDMA
+ * LLDP
+ * DCBX
+ * DCB
+ * XLR
+ * NVM
+ * AUTH
+ * VPD
+ * IOSF
+ * PARSER
+ * SW
+ * SCHEDULER
+ * TXQ
+ * RSVD
+ * POST
+ * WATCHDOG
+ * TASK_DISPATCH
+ * MNG
+ * SYNCE
+ * HEALTH
+ * TSDRV
+ * PFREG
+ * MDLVER
+ * ALL
+
+The name ALL is special and specifies setting all of the modules to the
+specified fwlog_level.
+
+Example usage to configure the modules::
+
+ # echo LINK VERBOSE > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
+
+Enabling FW log
+~~~~~~~~~~~~~~~
+Once the desired modules are configured the user will enable the logging. To do
+this the user can write a 1 (enable) or 0 (disable) to 'fwlog/enable'. An
+example is::
+
+ # echo 1 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/enable
+
+Retrieving FW log data
+~~~~~~~~~~~~~~~~~~~~~~
+The FW log data can be retrieved by reading from 'fwlog/data'. The user can
+write to 'fwlog/data' to clear the data. The data can only be cleared when FW
+logging is disabled. The FW log data is a binary file that is sent to Intel and
+used to help debug user issues.
+
+An example to read the data is::
+
+ # cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/data > fwlog.bin
+
+An example to clear the data is::
+
+ # echo 0 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/data
+
+Changing how often the log events are sent to the driver
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+The driver receives FW log data from the Admin Receive Queue (ARQ). The
+frequency that the FW sends the ARQ events can be configured by writing to
+'fwlog/resolution'. 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. The user can see what the value is configured to by reading
+'fwlog/resolution'. An example to set the value is::
+
+ # echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
+
+Configuring the number of buffers used to store FW log data
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+The driver stores FW log data in a ring within the driver. The default size of
+the ring is 256 4K buffers. Some use cases may require more or less data so
+the user can change the number of buffers that are allocated for FW log data.
+To change the number of buffers write to 'fwlog/nr_buffs'. The value must be a
+power of two and between the values 64-512. FW logging must be disabled to
+change the value. An example of changing the value is::
+
+ # echo 128 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/nr_buffs
+
+
Performance Optimization
========================
Driver defaults are meant to fit a wide variety of workloads, but if further
--
2.38.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v3 5/5] ice: add documentation for FW logging
2023-08-15 16:57 ` [PATCH net-next v3 5/5] ice: add documentation for FW logging Tony Nguyen
@ 2023-08-16 22:43 ` Randy Dunlap
2023-08-23 22:25 ` Paul M Stillwell Jr
0 siblings, 1 reply; 21+ messages in thread
From: Randy Dunlap @ 2023-08-16 22:43 UTC (permalink / raw)
To: Tony Nguyen, davem, kuba, pabeni, edumazet, netdev
Cc: Paul M Stillwell Jr, jacob.e.keller, horms, corbet, linux-doc
On 8/15/23 09:57, Tony Nguyen wrote:
> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>
> Add documentation for FW logging in
> Documentation/networking/device-drivers/ethernet/intel/ice.rst
>
> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
> .../device_drivers/ethernet/intel/ice.rst | 117 ++++++++++++++++++
> 1 file changed, 117 insertions(+)
>
> diff --git a/Documentation/networking/device_drivers/ethernet/intel/ice.rst b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
> index e4d065c55ea8..3ddef911faaa 100644
> --- a/Documentation/networking/device_drivers/ethernet/intel/ice.rst
> +++ b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
> @@ -895,6 +895,123 @@ driver writes raw bytes by the GNSS object to the receiver through i2c. Please
> refer to the hardware GNSS module documentation for configuration details.
>
>
> +Firmware (FW) logging
> +---------------------
> +The driver supports FW logging via the debugfs interface on PF 0 only. In order
> +for FW logging to work, the NVM must support it. The 'fwlog' file will only get
> +created in the ice debugfs directory if the NVM supports FW logging.
> +
> +Module configuration
> +~~~~~~~~~~~~~~~~~~~~
> +To see the status of FW logging then read the 'fwlog/modules' file like this::
of FW logging, read
> +
> + # cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
> +
> +To configure FW logging then write to the 'fwlog/modules' file like this::
FW logging, write to
> +
> + # echo <fwlog_event> <fwlog_level> > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
> +
> +where
> +
> +* fwlog_level is a name as described below. Each level includes the
> + messages from the previous/lower level
> +
> + * NONE
Should NONE be aligned with the entries below?
Ah, they are aligned in the source file, but NONE uses a space after the '*'
while the others use a TAB after the '*'.
> + * ERROR
> + * WARNING
> + * NORMAL
> + * VERBOSE
> +
> +* fwlog_event is a name that represents the module to receive events for. The
> + module names are
> +
> + * GENERAL
> + * CTRL
> + * LINK
> + * LINK_TOPO
> + * DNL
> + * I2C
> + * SDP
> + * MDIO
> + * ADMINQ
> + * HDMA
> + * LLDP
> + * DCBX
> + * DCB
> + * XLR
> + * NVM
> + * AUTH
> + * VPD
> + * IOSF
> + * PARSER
> + * SW
> + * SCHEDULER
> + * TXQ
> + * RSVD
> + * POST
> + * WATCHDOG
> + * TASK_DISPATCH
> + * MNG
> + * SYNCE
> + * HEALTH
> + * TSDRV
> + * PFREG
> + * MDLVER
> + * ALL
> +
> +The name ALL is special and specifies setting all of the modules to the
> +specified fwlog_level.
> +
> +Example usage to configure the modules::
> +
> + # echo LINK VERBOSE > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
> +
> +Enabling FW log
> +~~~~~~~~~~~~~~~
> +Once the desired modules are configured the user will enable the logging. To do
the user enables logging. To do
> +this the user can write a 1 (enable) or 0 (disable) to 'fwlog/enable'. An
> +example is::
> +
> + # echo 1 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/enable
> +
> +Retrieving FW log data
> +~~~~~~~~~~~~~~~~~~~~~~
> +The FW log data can be retrieved by reading from 'fwlog/data'. The user can
> +write to 'fwlog/data' to clear the data. The data can only be cleared when FW
> +logging is disabled. The FW log data is a binary file that is sent to Intel and
> +used to help debug user issues.
> +
> +An example to read the data is::
> +
> + # cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/data > fwlog.bin
> +
> +An example to clear the data is::
> +
> + # echo 0 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/data
> +
> +Changing how often the log events are sent to the driver
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +The driver receives FW log data from the Admin Receive Queue (ARQ). The
> +frequency that the FW sends the ARQ events can be configured by writing to
> +'fwlog/resolution'. 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. The user can see what the value is configured to by reading
> +'fwlog/resolution'. An example to set the value is::
> +
> + # echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
> +
> +Configuring the number of buffers used to store FW log data
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +The driver stores FW log data in a ring within the driver. The default size of
> +the ring is 256 4K buffers. Some use cases may require more or less data so
> +the user can change the number of buffers that are allocated for FW log data.
> +To change the number of buffers write to 'fwlog/nr_buffs'. The value must be a
> +power of two and between the values 64-512. FW logging must be disabled to
or
The value must be one of: 64, 128, 256, or 512.
> +change the value. An example of changing the value is::
> +
> + # echo 128 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/nr_buffs
> +
> +
> Performance Optimization
> ========================
> Driver defaults are meant to fit a wide variety of workloads, but if further
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v3 5/5] ice: add documentation for FW logging
2023-08-16 22:43 ` Randy Dunlap
@ 2023-08-23 22:25 ` Paul M Stillwell Jr
0 siblings, 0 replies; 21+ messages in thread
From: Paul M Stillwell Jr @ 2023-08-23 22:25 UTC (permalink / raw)
To: Randy Dunlap, Tony Nguyen, davem, kuba, pabeni, edumazet, netdev
Cc: jacob.e.keller, horms, corbet, linux-doc
On 8/16/2023 3:43 PM, Randy Dunlap wrote:
>
>
> On 8/15/23 09:57, Tony Nguyen wrote:
>> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>>
>> Add documentation for FW logging in
>> Documentation/networking/device-drivers/ethernet/intel/ice.rst
>>
>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>> ---
>> .../device_drivers/ethernet/intel/ice.rst | 117 ++++++++++++++++++
>> 1 file changed, 117 insertions(+)
>>
>> diff --git a/Documentation/networking/device_drivers/ethernet/intel/ice.rst b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
>> index e4d065c55ea8..3ddef911faaa 100644
>> --- a/Documentation/networking/device_drivers/ethernet/intel/ice.rst
>> +++ b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
>> @@ -895,6 +895,123 @@ driver writes raw bytes by the GNSS object to the receiver through i2c. Please
>> refer to the hardware GNSS module documentation for configuration details.
>>
>>
>> +Firmware (FW) logging
>> +---------------------
>> +The driver supports FW logging via the debugfs interface on PF 0 only. In order
>> +for FW logging to work, the NVM must support it. The 'fwlog' file will only get
>> +created in the ice debugfs directory if the NVM supports FW logging.
>> +
>> +Module configuration
>> +~~~~~~~~~~~~~~~~~~~~
>> +To see the status of FW logging then read the 'fwlog/modules' file like this::
>
> of FW logging, read
>
Fixed
>> +
>> + # cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>> +
>> +To configure FW logging then write to the 'fwlog/modules' file like this::
>
> FW logging, write to
>
Fixed
>> +
>> + # echo <fwlog_event> <fwlog_level> > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>> +
>> +where
>> +
>> +* fwlog_level is a name as described below. Each level includes the
>> + messages from the previous/lower level
>> +
>> + * NONE
>
> Should NONE be aligned with the entries below?
> Ah, they are aligned in the source file, but NONE uses a space after the '*'
> while the others use a TAB after the '*'.
>
Good catch, fixed
>> + * ERROR
>> + * WARNING
>> + * NORMAL
>> + * VERBOSE
>> +
>> +* fwlog_event is a name that represents the module to receive events for. The
>> + module names are
>> +
>> + * GENERAL
>> + * CTRL
>> + * LINK
>> + * LINK_TOPO
>> + * DNL
>> + * I2C
>> + * SDP
>> + * MDIO
>> + * ADMINQ
>> + * HDMA
>> + * LLDP
>> + * DCBX
>> + * DCB
>> + * XLR
>> + * NVM
>> + * AUTH
>> + * VPD
>> + * IOSF
>> + * PARSER
>> + * SW
>> + * SCHEDULER
>> + * TXQ
>> + * RSVD
>> + * POST
>> + * WATCHDOG
>> + * TASK_DISPATCH
>> + * MNG
>> + * SYNCE
>> + * HEALTH
>> + * TSDRV
>> + * PFREG
>> + * MDLVER
>> + * ALL
>> +
>> +The name ALL is special and specifies setting all of the modules to the
>> +specified fwlog_level.
>> +
>> +Example usage to configure the modules::
>> +
>> + # echo LINK VERBOSE > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
>> +
>> +Enabling FW log
>> +~~~~~~~~~~~~~~~
>> +Once the desired modules are configured the user will enable the logging. To do
>
> the user enables logging. To do
>
Fixed
>> +this the user can write a 1 (enable) or 0 (disable) to 'fwlog/enable'. An
>> +example is::
>> +
>> + # echo 1 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/enable
>> +
>> +Retrieving FW log data
>> +~~~~~~~~~~~~~~~~~~~~~~
>> +The FW log data can be retrieved by reading from 'fwlog/data'. The user can
>> +write to 'fwlog/data' to clear the data. The data can only be cleared when FW
>> +logging is disabled. The FW log data is a binary file that is sent to Intel and
>> +used to help debug user issues.
>> +
>> +An example to read the data is::
>> +
>> + # cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/data > fwlog.bin
>> +
>> +An example to clear the data is::
>> +
>> + # echo 0 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/data
>> +
>> +Changing how often the log events are sent to the driver
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +The driver receives FW log data from the Admin Receive Queue (ARQ). The
>> +frequency that the FW sends the ARQ events can be configured by writing to
>> +'fwlog/resolution'. 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. The user can see what the value is configured to by reading
>> +'fwlog/resolution'. An example to set the value is::
>> +
>> + # echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
>> +
>> +Configuring the number of buffers used to store FW log data
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +The driver stores FW log data in a ring within the driver. The default size of
>> +the ring is 256 4K buffers. Some use cases may require more or less data so
>> +the user can change the number of buffers that are allocated for FW log data.
>> +To change the number of buffers write to 'fwlog/nr_buffs'. The value must be a
>> +power of two and between the values 64-512. FW logging must be disabled to
>
> or
> The value must be one of: 64, 128, 256, or 512.
>
Changed
>> +change the value. An example of changing the value is::
>> +
>> + # echo 128 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/nr_buffs
>> +
>> +
>> Performance Optimization
>> ========================
>> Driver defaults are meant to fit a wide variety of workloads, but if further
^ permalink raw reply [flat|nested] 21+ messages in thread