Re: [Intel-wired-lan] [PATCH net-next v6 0/5] add v2 FW logging for ice driver

[Jacob Keller <jacob.e.keller@intel.com>]

On 1/13/2023 2:23 PM, Paul M Stillwell Jr wrote:
> FW log support was added to the ice driver, but that version is no
> longer supported. There is a newer version of FW logging (v2) that
> adds more control knobs to get the exact data out of the FW
> for debugging.
> 
> Additionally, instead of dumping the FW log output to syslog,
> dump it to debugfs. The FW log data is really just binary
> data that the FW log team decodes to determine what happens so the
> translation from binary to some text output just slows things down
> and results in potential dropped data. The structure for the debugfs
> entry is: /sys/kernel/debug/ice/<pci device>/fwlog
> 
> Once enabled the FW log data is received as ARQ events that the driver
> processes.
> 
> The FW logging is across all the PFs on the device, so restrict the
> commands to only PF0.
> 
> The following new device parameters are added:
> - fwlog_supported (read-only): does the FW support FW logging
> - fwlog_enabled (read/write): is FW logging currently running
> - fwlog_level (read/write): the log level enabled, valid values are
>     Each level includes the messages from the previous/lower level
> 	0 - no logging
> 	1 - error logging
> 	2 - warning logging
> 	3 - normal logging
> 	4 - verbose logging
> - fwlog_resolution (read/write): the number of log messages to included
>   in a single ARQ event. The range is 1-128 (1 means push every log
>   message, 128 means push only when the max AQ command buffer is full).
>   The suggested value is 10.
> 
> This patch series adds the following set of devlink commands:
> 
> devlink dev param set <pci dev> name fwlog_enabled value <true/false> cmode runtime
> devlink dev param set <pci dev> name fwlog_level value <0-4> cmode runtime
> devlink dev param set <pci dev> name fwlog_resolution value <1-128> cmode runtime
> ---

Overall this looks good. I had a few questions and suggestions you're
welcome to take.

I'm sure we'll get feedback about using debugfs and the nature of the
logs being binary. I think its better to send this and get feedback than
it is to do nothing though.

Thanks,
Jake

> v6:
> - removed cache_cfg() based on feedback
> - a couple of other minor changes based on feedback
> v5:
> - handle devlink reload path correctly so debugfs directories don't get
>   added twice
> - fix issue where code wrapped with CONFIG_DEBUG_FS was causing sparc
>   compile issues with multiple defines
> v4:
> - actually changed the modes in ice.rst for new params
> v3:
> - fixed ice.rst to have proper mode for new params and fixed formatting 
> v2:
> - removed some unused admin queue commands
> - updated copyright in ice_fwlog.[ch] to 2022
> - moved defines in structures under the variables and added blank line
> - removed a couple of unused defines
> - changed fwlog_support_ena to fwlog_supported to be clearer
> - consolidated ice_devlink_param_id enum together
> - changed ice_fwlog_set_support_ena() to ice_fwlog_set_supported()
> - consolidated return status logic in ice_devlink_fwlog_enabled_set()
> - pull up functions in ice_fwlog.c where appropriate
> - add newline for FW Logging Commands comment
> - changed any new u[8/16] loop variables to int
> - moved ice_pf_fwlog_deinit() from patch 5 to patch 4
> - changed error message to be clearer
> - updated Documentation/networking/devlink/ice.rst
> - updated commit messages with examples of devlink commands and using
>   debugfs to get log files
> 
> Paul M Stillwell Jr (5):
>   ice: remove FW logging code
>   ice: enable devlink to check FW logging status
>   ice: add ability to query/set FW log level and resolution
>   ice: disable FW logging on driver unload
>   ice: use debugfs to output FW log data
> 
>  Documentation/networking/devlink/ice.rst      |  39 ++
>  drivers/net/ethernet/intel/ice/Makefile       |   5 +-
>  drivers/net/ethernet/intel/ice/ice.h          |  22 ++
>  .../net/ethernet/intel/ice/ice_adminq_cmd.h   | 160 ++++----
>  drivers/net/ethernet/intel/ice/ice_common.c   | 218 +----------
>  drivers/net/ethernet/intel/ice/ice_common.h   |   1 -
>  drivers/net/ethernet/intel/ice/ice_debugfs.c  | 109 ++++++
>  drivers/net/ethernet/intel/ice/ice_devlink.c  | 200 +++++++++-
>  drivers/net/ethernet/intel/ice/ice_fwlog.c    | 367 ++++++++++++++++++
>  drivers/net/ethernet/intel/ice/ice_fwlog.h    |  57 +++
>  drivers/net/ethernet/intel/ice/ice_main.c     | 100 ++++-
>  drivers/net/ethernet/intel/ice/ice_type.h     |  23 +-
>  12 files changed, 984 insertions(+), 317 deletions(-)
>  create mode 100644 drivers/net/ethernet/intel/ice/ice_debugfs.c
>  create mode 100644 drivers/net/ethernet/intel/ice/ice_fwlog.c
>  create mode 100644 drivers/net/ethernet/intel/ice/ice_fwlog.h
> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan