Date: Tue, 22 Aug 2023 14:16:38 -0700
From: Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, Leon Romanovsky <leon@...nel.org>
CC: "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "davem@...emloft.net"
	<davem@...emloft.net>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"horms@...nel.org" <horms@...nel.org>, "Pucha, HimasekharX Reddy"
	<himasekharx.reddy.pucha@...el.com>
Subject: Re: [PATCH net-next v3 2/5] ice: configure FW logging

On 8/22/2023 1:58 PM, Paul M Stillwell Jr wrote:
> On 8/22/2023 1:44 PM, Keller, Jacob E wrote:
>>
>>
>>> -----Original Message-----
>>> From: Stillwell Jr, Paul M <paul.m.stillwell.jr@...el.com>
>>> Sent: Monday, August 21, 2023 4:21 PM
>>> To: Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com>; Leon Romanovsky
>>> <leon@...nel.org>
>>> Cc: Nguyen, Anthony L <anthony.l.nguyen@...el.com>; davem@...emloft.net;
>>> kuba@...nel.org; pabeni@...hat.com; edumazet@...gle.com;
>>> netdev@...r.kernel.org; Keller, Jacob E <jacob.e.keller@...el.com>;
>>> horms@...nel.org; Pucha, HimasekharX 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?
>>>
>>
>> These stubs are for functions we have in ice_debugfs.o?
> 
> All of the subs except for 1 (which could be moved) are in ice_debugfs.o
> 
>> Is there an ice_debugfs.h? We should implement stubs for those 
>> functions there so we don't have to check CONFIG_DEBUG_FS.
> 
> There isn't an ice_debugfs.h, but moving the stubs there would have the 
> same issue correct? You would have to wrap them with CONFIG_DEBUG_FS no 
> matter where they are, right?
> 

Never mind all of this. I looked around some more and as Leon said 
earlier, we don't need the stubs and we should not be using the Makefile 
to remove the code if CONFIG_DEBUG_FS is not set. We should always 
include ice_debugfs.o when building the driver and the calls to 
debugfs_<whatever> will fail if CONFIG_DEBUG_FS is not set.

I'll change the code to reflect what I just said (and what Leon 
originally said). Sorry for the noise.

>> Or, if they don’t' really belong there move them into another file 
>> that isn't stripped out with CONFIG_DEBUG_FS=n.
>>
>>
> 

Powered by blists - more mailing lists