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

[Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>]

On 11/30/2022 4:40 PM, Tony Nguyen wrote:
> 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?
> 

I'll look and see what things look like and see if I can move it

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

Will remove

>>   };
>>   #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()?
> 

The reason I called it ice_pf_fwlog_init() is because it's going to get 
called for every PF. In my mind the implication of calling it 
ice_fwlog_init() is that it's only called once, which wouldn't be true.

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

Same answer as above

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

I don't think so. In ice_module_exit() we will remove the debugfs entry 
points

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