On 8/22/23 01:20, Paul M Stillwell Jr wrote:

On 8/18/2023 5:31 AM, Przemek Kitszel wrote:

quoted

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

quoted

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 
[off-list ref] (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.

I've thought about this some more and I am confused what to do. > In the Makefile there is a bit that removes ice_debugfs.o if 

CONFIG_DEBUG_FS is

not set. 

That's true, and it is to prevent 450 lines of code, and some includes, 
so makes sense.

This would result in the stubs being needed (since the 
functions are called from ice_fwlog.c). In this case the code would not 
compile (since the functions would be missing). Should I remove the code 
from the Makefile that deals with ice_debugfs.o (which doesn't make 
sense since then there will be code in the driver that doesn't get used) 
or do I leave the stubs in?

other option is to have those few (that would be stubs otherwise) 
functions outside of ice_debugfs.c, I didn't checked if there will be 
any dependencies though

quoted

quoted

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