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