On 8/18/23 13:10, Leon Romanovsky wrote:

On Thu, Aug 17, 2023 at 02:25:34PM -0700, Paul M Stillwell Jr wrote:

quoted

On 8/15/2023 11:38 AM, Leon Romanovsky wrote:

quoted

On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:

quoted

From: Paul M Stillwell Jr <redacted>

Users want the ability to debug FW issues by retrieving the
FW logs from the E8xx devices. Use debugfs to allow the user to
read/write the FW log configuration by adding a 'fwlog/modules' file.
Reading the file will show either the currently running configuration or
the next configuration (if the user has changed the configuration).
Writing to the file will update the configuration, but NOT enable the
configuration (that is a separate command).

To see the status of FW logging then read the 'fwlog/modules' file like
this:

cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules

To change the configuration of FW logging then write to the 'fwlog/modules'
file like this:

echo DCB NORMAL > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules

The format to change the configuration is

echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device

This line is truncated, it is not clear where you are writing.

Good catch, I don't know how I missed this... Will fix

quoted

And more general question, a long time ago, netdev had a policy of
not-allowing writing to debugfs, was it changed?

I had this same thought and it seems like there were 2 concerns in the past

Maybe, I'm not enough time in netdev world to know the history.

quoted

1. Having a single file that was read/write with lots of commands going
through it
2. Having code in the driver to parse the text from the commands that was
error/security prone

We have addressed this in 2 ways:
1. Split the commands into multiple files that have a single purpose
2. Use kernel parsing functions for anything where we *have* to pass text to
decode

quoted

quoted

where

* fwlog_level is a name as described below. Each level includes the
    messages from the previous/lower level

        * NONE
        *	ERROR
        *	WARNING
        *	NORMAL
        *	VERBOSE

* fwlog_event is a name that represents the module to receive events for.
    The module names are

        *	GENERAL
        *	CTRL
        *	LINK
        *	LINK_TOPO
        *	DNL
        *	I2C
        *	SDP
        *	MDIO
        *	ADMINQ
        *	HDMA
        *	LLDP
        *	DCBX
        *	DCB
        *	XLR
        *	NVM
        *	AUTH
        *	VPD
        *	IOSF
        *	PARSER
        *	SW
        *	SCHEDULER
        *	TXQ
        *	RSVD
        *	POST
        *	WATCHDOG
        *	TASK_DISPATCH
        *	MNG
        *	SYNCE
        *	HEALTH
        *	TSDRV
        *	PFREG
        *	MDLVER
        *	ALL

The name ALL is special and specifies setting all of the modules to the
specified fwlog_level.

If the NVM supports FW logging then the file 'fwlog' will be created
under the PCI device ID for the ice driver. If the file does not exist
then either the NVM doesn't support FW logging or debugfs is not enabled
on the system.

In addition to configuring the modules, the user can also configure the
number of log messages (resolution) to include in a single Admin Receive
Queue (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.

To see/change the resolution the user can read/write the
'fwlog/resolution' file. An example changing the value to 50 is

echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution

Signed-off-by: Paul M Stillwell Jr <redacted>
Tested-by: Pucha Himasekhar Reddy <redacted> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
   drivers/net/ethernet/intel/ice/Makefile       |   4 +-
   drivers/net/ethernet/intel/ice/ice.h          |  18 +
   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  80 ++++
   drivers/net/ethernet/intel/ice/ice_common.c   |   5 +
   drivers/net/ethernet/intel/ice/ice_debugfs.c  | 450 ++++++++++++++++++
   drivers/net/ethernet/intel/ice/ice_fwlog.c    | 231 +++++++++
   drivers/net/ethernet/intel/ice/ice_fwlog.h    |  55 +++
   drivers/net/ethernet/intel/ice/ice_main.c     |  21 +
   drivers/net/ethernet/intel/ice/ice_type.h     |   4 +
   9 files changed, 867 insertions(+), 1 deletion(-)
   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

diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
index 960277d78e09..d43a59e5f8ee 100644
--- a/drivers/net/ethernet/intel/ice/Makefile
+++ b/drivers/net/ethernet/intel/ice/Makefile

@@ -34,7 +34,8 @@ ice-y := ice_main.o	\
   	 ice_lag.o	\
   	 ice_ethtool.o  \
   	 ice_repr.o	\
-	 ice_tc_lib.o
+	 ice_tc_lib.o	\
+	 ice_fwlog.o
   ice-$(CONFIG_PCI_IOV) +=	\
   	ice_sriov.o		\
   	ice_virtchnl.o		\

@@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
   ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
   ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
   ice-$(CONFIG_GNSS) += ice_gnss.o
+ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 5ac0ad12f9f1..e6dd9f6f9eee 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h

@@ -556,6 +556,8 @@ 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 */
+	struct dentry *ice_debugfs_pf;
+	struct dentry *ice_debugfs_pf_fwlog;
   	struct ice_vfs vfs;
   	DECLARE_BITMAP(features, ICE_F_MAX);
   	DECLARE_BITMAP(state, ICE_STATE_NBITS);

@@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct ice_pf *pf)
   	return false;
   }
+#ifdef CONFIG_DEBUG_FS

There is no need in this CONFIG_DEBUG_FS and code should be written
without debugfs stubs.

I don't understand this comment... If the kernel is configured *without*
debugfs, won't the kernel fail to compile due to missing functions if we
don't do this?

It will work fine, see include/linux/debugfs.h.

Nice, as-is impl of ice_debugfs_fwlog_init() would just fail on first 
debugfs API call.

quoted

quoted

quoted

+void ice_debugfs_fwlog_init(struct ice_pf *pf);
+void ice_debugfs_init(void);
+void ice_debugfs_exit(void);
+void ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module);
+#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) { }
+static void
+ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module)
+{
+	return -EOPNOTSUPP;
+}
+#endif /* CONFIG_DEBUG_FS */

Thanks