[Intel-wired-lan] [PATCH net-next v9 2/5] ice: enable debugfs to check FW logging status
Paul M Stillwell Jr
paul.m.stillwell.jr at intel.com
Mon Mar 6 22:00:30 UTC 2023
On 3/3/2023 3:21 PM, Tony Nguyen wrote:
> On 3/2/2023 1:51 PM, Paul M Stillwell Jr wrote:
>
> [...]
>
>> +#ifdef CONFIG_DEBUG_FS
>> +void ice_debugfs_fwlog_init(struct ice_pf *pf);
>> +void ice_debugfs_init(void);
>> +void ice_debugfs_exit(void);
>> +#else
>> +static inline void ice_debugfs_fwlog_init(struct ice_pf *pf) { }
>> +static inline void ice_debugfs_init(void) { }
>> +static inline void ice_debugfs_exit(void) { }
>> +#endif /* CONFIG_DEBUG_FS */
>> +
>> bool netif_is_ice(struct net_device *dev);
>> int ice_vsi_setup_tx_rings(struct ice_vsi *vsi);
>> int ice_vsi_setup_rx_rings(struct ice_vsi *vsi);
>> @@ -934,6 +945,18 @@ int ice_aq_wait_for_event(struct ice_pf *pf, u16
>> opcode, unsigned long timeout,
>> int ice_open(struct net_device *netdev);
>> int ice_open_internal(struct net_device *netdev);
>> int ice_stop(struct net_device *netdev);
>> +#ifdef CONFIG_DEBUG_FS
>> +int
>> +ice_pf_fwlog_update_modules(struct ice_pf *pf, u8 log_level,
>> + unsigned long events);
>> +#else
>> +static int
>> +ice_pf_fwlog_update_modules(struct ice_pf *pf, u8 log_level,
>> + unsigned long events)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +#endif /* CONFIG_DEBUG_FS */
>
> This could be put with the ifdef above
>
Done
>> void ice_service_task_schedule(struct ice_pf *pf);
>> int ice_load(struct ice_pf *pf);
>> void ice_unload(struct ice_pf *pf);
>
> [...]
>
>> +/* 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_REGISTER BIT(0)
>> +#define ICE_AQC_FW_LOG_AQ_QUERY BIT(2)
>> +#define ICE_AQC_FW_LOG_PERSISTENT BIT(0)
>> + u8 rsp_flag;
>
> Please add a newline between the member and the defines that relate to
> it. Please check this for other instances/needs as well.
>
Done
> [...]
>
>> +#include <linux/vmalloc.h>
>> +
>
> Any particular reason this isn't with everything else (and alphabetized)?
>
This isn't needed in this patch so remove it here and add it back in the
correct patch.
>> +#include <linux/fs.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/random.h>
>> +#include "ice.h"
>> +
>> +static struct dentry *ice_debugfs_root;
>> +
>> +static const char *module_id_to_name(u16 module_id)
>> +{
>> + switch (module_id) {
>> + case ICE_AQC_FW_LOG_ID_GENERAL:
>> + return "General";
>> + case ICE_AQC_FW_LOG_ID_CTRL:
>> + return "Control (Resets + Autoload)";
>> + case ICE_AQC_FW_LOG_ID_LINK:
>> + return "Link Management";
>> + case ICE_AQC_FW_LOG_ID_LINK_TOPO:
>> + return "Link Topology Detection";
>> + case ICE_AQC_FW_LOG_ID_DNL:
>> + return "DNL";
>> + case ICE_AQC_FW_LOG_ID_I2C:
>> + return "I2C";
>> + case ICE_AQC_FW_LOG_ID_SDP:
>> + return "SDP";
>> + case ICE_AQC_FW_LOG_ID_MDIO:
>> + return "MDIO";
>> + case ICE_AQC_FW_LOG_ID_ADMINQ:
>> + return "Admin Queue";
>> + case ICE_AQC_FW_LOG_ID_HDMA:
>> + return "HDMA";
>> + case ICE_AQC_FW_LOG_ID_LLDP:
>> + return "LLDP";
>> + case ICE_AQC_FW_LOG_ID_DCBX:
>> + return "DCBX";
>> + case ICE_AQC_FW_LOG_ID_DCB:
>> + return "DCB";
>> + case ICE_AQC_FW_LOG_ID_XLR:
>> + return "XLR";
>> + case ICE_AQC_FW_LOG_ID_NVM:
>> + return "NVM";
>> + case ICE_AQC_FW_LOG_ID_AUTH:
>> + return "Authentication";
>> + case ICE_AQC_FW_LOG_ID_VPD:
>> + return "VPD";
>> + case ICE_AQC_FW_LOG_ID_IOSF:
>> + return "IOSF";
>> + case ICE_AQC_FW_LOG_ID_PARSER:
>> + return "Parser";
>> + case ICE_AQC_FW_LOG_ID_SW:
>> + return "Switch";
>> + case ICE_AQC_FW_LOG_ID_SCHEDULER:
>> + return "Scheduler";
>> + case ICE_AQC_FW_LOG_ID_TXQ:
>> + return "Tx Queue Management";
>> + case ICE_AQC_FW_LOG_ID_POST:
>> + return "Post";
>> + case ICE_AQC_FW_LOG_ID_WATCHDOG:
>> + return "Watchdog";
>> + case ICE_AQC_FW_LOG_ID_TASK_DISPATCH:
>> + return "Task Dispatcher";
>> + case ICE_AQC_FW_LOG_ID_MNG:
>> + return "Manageability";
>> + case ICE_AQC_FW_LOG_ID_SYNCE:
>> + return "Synce";
>> + case ICE_AQC_FW_LOG_ID_HEALTH:
>> + return "Health";
>> + case ICE_AQC_FW_LOG_ID_TSDRV:
>> + return "Time Sync";
>> + case ICE_AQC_FW_LOG_ID_PFREG:
>> + return "PF Registration";
>> + case ICE_AQC_FW_LOG_ID_MDLVER:
>> + return "Module Version";
>> + default:
>> + return "Unsupported";
>> + }
>> +}
>> +
>> +static const char *log_level_to_name(u8 log_level)
>> +{
>> + switch (log_level) {
>> + case ICE_FWLOG_LEVEL_NONE:
>> + return "None";
>> + case ICE_FWLOG_LEVEL_ERROR:
>> + return "Error";
>> + case ICE_FWLOG_LEVEL_WARNING:
>> + return "Warning";
>> + case ICE_FWLOG_LEVEL_NORMAL:
>> + return "Normal";
>> + case ICE_FWLOG_LEVEL_VERBOSE:
>> + return "Verbose";
>> + default:
>> + return "Unsupported";
>> + }
>> +}
>> +
>> +static void ice_print_fwlog_config(struct ice_hw *hw, struct
>> ice_fwlog_cfg *cfg)
>> +{
>> + struct device *dev = ice_pf_to_dev((struct ice_pf *)(hw->back));
>
> I don't believe this casting is needed.
>
Changed this to ice_hw_to_dev() since that makes more sense
>> + u16 i;
>> +
>> + dev_info(dev, "Log_resolution: %d\n", cfg->log_resolution);
>> + dev_info(dev, "Options: 0x%04x\n", cfg->options);
>> + dev_info(dev, "\tarq_ena: %s\n",
>> + (cfg->options &
>> + ICE_FWLOG_OPTION_ARQ_ENA) ? "true" : "false");
>> + dev_info(dev, "\tuart_ena: %s\n",
>> + (cfg->options &
>> + ICE_FWLOG_OPTION_UART_ENA) ? "true" : "false");
>> + dev_info(dev, "\trunning: %s\n",
>> + (cfg->options &
>> + ICE_FWLOG_OPTION_IS_REGISTERED) ? "true" : "false");
>> +
>> + dev_info(dev, "Module Entries:\n");
>> + for (i = 0; i < ICE_AQC_FW_LOG_ID_MAX; i++) {
>> + struct ice_fwlog_module_entry *entry =
>> + &cfg->module_entries[i];
>> +
>> + dev_info(dev, "\tModule ID %d (%s) Log Level %d (%s)\n",
>> + entry->module_id, module_id_to_name(entry->module_id),
>> + entry->log_level, log_level_to_name(entry->log_level));
>> + }
>> +}
>> +
>> +/**
>> + * ice_fwlog_dump_cfg - Dump current FW logging configuration
>> + * @hw: pointer to the HW structure
>> + */
>> +static void ice_fwlog_dump_cfg(struct ice_hw *hw)
>> +{
>> + struct device *dev = ice_pf_to_dev((struct ice_pf *)(hw->back));
>
> same here
>
Same as above
>> + struct ice_fwlog_cfg *cfg;
>> + int status;
>> +
>> + cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
>> + if (!cfg)
>> + return;
>> +
>> + status = ice_fwlog_get(hw, cfg);
>> + if (status) {
>> + kfree(cfg);
>> + return;
>> + }
>> +
>> + dev_info(dev, "Running FWLOG Configuration:\n");
>> + ice_print_fwlog_config(hw, cfg);
>> +
>> + kfree(cfg);
>> +}
>
> [...]
>
>> +
>> +/**
>> + * ice_fwlog_get - Get the firmware logging settings
>> + * @hw: pointer to the HW structure
>> + * @cfg: config to populate based on current firmware logging settings
>> + */
>> +int ice_fwlog_get(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
>> +{
>> + int status;
>> +
>> + if (!ice_fwlog_supported(hw))
>> + return -EOPNOTSUPP;
>> +
>> + if (!cfg)
>> + return -EINVAL;
>> +
>> + status = ice_aq_fwlog_get(hw, cfg);
>> + if (status)
>> + return status;
>> +
>> + return 0;
>
> This can be 'return ice_aq_fwlog_get(hw, cfg);'
Done
More information about the Intel-wired-lan
mailing list