Message-ID: <87b9788b-bcad-509a-50ef-bf86de2f5c03@intel.com>
Date: Fri, 18 Aug 2023 14:31:50 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Leon Romanovsky <leon@...nel.org>, Paul M Stillwell Jr
	<paul.m.stillwell.jr@...el.com>
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/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.

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