Message-ID: <16fbb0fe-0578-4058-5106-76dbf2a6458e@intel.com>
Date: Mon, 21 Aug 2023 16:20:56 -0700
From: Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>, Leon Romanovsky
	<leon@...nel.org>
CC: Tony Nguyen <anthony.l.nguyen@...el.com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<netdev@...r.kernel.org>, <jacob.e.keller@...el.com>, <horms@...nel.org>,
	Pucha Himasekhar Reddy <himasekharx.reddy.pucha@...el.com>
Subject: Re: [PATCH net-next v3 2/5] ice: configure FW logging

On 8/18/2023 5:31 AM, Przemek Kitszel wrote:
> On 8/18/23 13:10, Leon Romanovsky wrote:
>> On Thu, Aug 17, 2023 at 02:25:34PM -0700, Paul M Stillwell Jr wrote:
>>> On 8/15/2023 11:38 AM, Leon Romanovsky wrote:
>>>> On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
>>>>> From: Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com>
>>>>>
>>>>> 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
>>>
>>>> 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.
>>
>>>
>>> 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
>>>
>>>>>
>>>>> 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 <paul.m.stillwell.jr@...el.com>
>>>>> Tested-by: Pucha Himasekhar Reddy 
>>>>> <himasekharx.reddy.pucha@...el.com> (A Contingent worker at Intel)
>>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.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?

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

Powered by blists - more mailing lists