Re: [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@intel.com>]

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


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