lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <c865cde7-fe13-c158-673a-a0acd200b667@intel.com> Date: Thu, 17 Aug 2023 14:25:34 -0700 From: Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com> To: Leon Romanovsky <leon@...nel.org>, Tony Nguyen <anthony.l.nguyen@...el.com> CC: <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/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 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? >> +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