Re: [Intel-wired-lan] [PATCH net-next 5/5] ice: use debugfs to output FW log data

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

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

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

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

I don't think I see this being used.

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

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

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

same with this one

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

Do these need to be unrolled for error path?

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