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

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

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

>   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.

[...]

> +#include <linux/vmalloc.h>
> +

Any particular reason this isn't with everything else (and alphabetized)?

> +#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.

> +	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

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