On 8/22/2023 1:45 PM, Keller, Jacob E wrote:

quoted

-----Original Message-----
From: Stillwell Jr, Paul M <redacted>
Sent: Monday, August 21, 2023 4:21 PM
To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Leon Romanovsky
[off-list ref]
Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net;
kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
netdev@vger.kernel.org; Keller, Jacob E [off-list ref];
horms@kernel.org; Pucha, HimasekharX Reddy
[off-list ref]
Subject: Re: [PATCH net-next v3 2/5] ice: configure FW logging

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

quoted

quoted

quoted

quoted

quoted

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

Or, since ice_fwlog depends on debugfs support, also strip the fwlog.o object when CONFIG_DEBUGFS=n.

We should do this as well I think. That makes sense to me, but doesn't 
solve all the issues since the code in question gets called from ice_main.c

Thanks.
Jake