Date: Tue, 22 Aug 2023 09:33:14 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Paul M Stillwell Jr <paul.m.stillwell.jr@...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/22/23 01:20, Paul M Stillwell Jr wrote:
> 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. 

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

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