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: <e6eff1f0-97a2-5581-1a86-22d863578c1b@intel.com> 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