| Message ID | 20230913185528.770634-1-umang.jain@ideasonboard.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | staging: vc04: Drop custom logging | expand |
On Thu, Sep 14, 2023 at 12:25:24AM +0530, Umang Jain wrote: > Hello, > > This series attempts to restart the discussion on custom logging used > in VC04. In the last feedback gathered in [1] it seems that the logging > would rather be moved to use dynamic debug. The series tries to move > in that direction. > > The elephant in the room is the ability of turning on/off log levels, > which this series just drops. Compensated by a crude strings > ("error", "warning", "info"... etc) for easier grepping. > > The log category are also just strings (which probably can be transformed > to dynamic debug class names moving forwards?). > > To move forwards, I would like feedback on the broader direction. > There are couple of TODOs in each of the patch (summarised in commit > messages) which require case-by-case discussion. > > Additional high-level questions to move forwards: > 1. Is loss of log levels by moving to dynamic debug, is actually a > concern? Is dynamic debug a valid replacement? Dynamic debug is honestly going to be an improvement. I guess, Greg and I said this back in Jan. > 2. Whether debugfs should be dropped as well, found vestigial in [2] Yes. The "vchiq/log" should be removed. Ideally as part of this patchset so it's easier to understand. > 3. whether vchiq_log_trace() should actually be tracing support for VC04 That can be done later if people want. No need to discuss it now. regards, dan carpenter
Hi, Am 14.09.23 um 08:35 schrieb Dan Carpenter: > On Thu, Sep 14, 2023 at 12:25:24AM +0530, Umang Jain wrote: >> Hello, >> >> This series attempts to restart the discussion on custom logging used >> in VC04. In the last feedback gathered in [1] it seems that the logging >> would rather be moved to use dynamic debug. The series tries to move >> in that direction. >> >> The elephant in the room is the ability of turning on/off log levels, >> which this series just drops. Compensated by a crude strings >> ("error", "warning", "info"... etc) for easier grepping. >> >> The log category are also just strings (which probably can be transformed >> to dynamic debug class names moving forwards?). >> >> To move forwards, I would like feedback on the broader direction. >> There are couple of TODOs in each of the patch (summarised in commit >> messages) which require case-by-case discussion. >> >> Additional high-level questions to move forwards: >> 1. Is loss of log levels by moving to dynamic debug, is actually a >> concern? Is dynamic debug a valid replacement? > > Dynamic debug is honestly going to be an improvement. I guess, Greg and > I said this back in Jan. > >> 2. Whether debugfs should be dropped as well, found vestigial in [2] > > Yes. The "vchiq/log" should be removed. Ideally as part of this > patchset so it's easier to understand. Yes, but please do not remote vchiq_debugfs entirely. I'm working on a patch to move the state dump (debug feature) from the character device /dev/vchiq to debugfs /sys/kernel/debug/vchiq/dump_state. > >> 3. whether vchiq_log_trace() should actually be tracing support for VC04 > > That can be done later if people want. No need to discuss it now. > > regards, > dan carpenter >
Hi Stefan, On 9/17/23 9:06 PM, Stefan Wahren wrote: > Hi, > > Am 14.09.23 um 08:35 schrieb Dan Carpenter: >> On Thu, Sep 14, 2023 at 12:25:24AM +0530, Umang Jain wrote: >>> Hello, >>> >>> This series attempts to restart the discussion on custom logging used >>> in VC04. In the last feedback gathered in [1] it seems that the logging >>> would rather be moved to use dynamic debug. The series tries to move >>> in that direction. >>> >>> The elephant in the room is the ability of turning on/off log levels, >>> which this series just drops. Compensated by a crude strings >>> ("error", "warning", "info"... etc) for easier grepping. >>> >>> The log category are also just strings (which probably can be >>> transformed >>> to dynamic debug class names moving forwards?). >>> >>> To move forwards, I would like feedback on the broader direction. >>> There are couple of TODOs in each of the patch (summarised in commit >>> messages) which require case-by-case discussion. >>> >>> Additional high-level questions to move forwards: >>> 1. Is loss of log levels by moving to dynamic debug, is actually a >>> concern? Is dynamic debug a valid replacement? >> >> Dynamic debug is honestly going to be an improvement. I guess, Greg and >> I said this back in Jan. +1 >> >>> 2. Whether debugfs should be dropped as well, found vestigial in [2] >> >> Yes. The "vchiq/log" should be removed. Ideally as part of this >> patchset so it's easier to understand. > > Yes, but please do not remote vchiq_debugfs entirely. I'm working on a > patch to move the state dump (debug feature) from the character device > /dev/vchiq to debugfs /sys/kernel/debug/vchiq/dump_state. Can't the state dump be printed to dev_dbg() ? Will it pollute the kernel log? Having debugfs for a single dump doesn't seem worthwhile if the state dump can be incorporated to dev_dbg() too. > >> >>> 3. whether vchiq_log_trace() should actually be tracing support for >>> VC04 >> >> That can be done later if people want. No need to discuss it now. Thanks Dan. >> >> regards, >> dan carpenter >>
Hi Umang, Am 23.09.23 um 11:39 schrieb Umang Jain: > Hi Stefan, > > On 9/17/23 9:06 PM, Stefan Wahren wrote: >> Hi, >> >> Am 14.09.23 um 08:35 schrieb Dan Carpenter: >>> On Thu, Sep 14, 2023 at 12:25:24AM +0530, Umang Jain wrote: >>>> Hello, >>>> >>>> This series attempts to restart the discussion on custom logging used >>>> in VC04. In the last feedback gathered in [1] it seems that the logging >>>> would rather be moved to use dynamic debug. The series tries to move >>>> in that direction. >>>> >>>> The elephant in the room is the ability of turning on/off log levels, >>>> which this series just drops. Compensated by a crude strings >>>> ("error", "warning", "info"... etc) for easier grepping. >>>> >>>> The log category are also just strings (which probably can be >>>> transformed >>>> to dynamic debug class names moving forwards?). >>>> >>>> To move forwards, I would like feedback on the broader direction. >>>> There are couple of TODOs in each of the patch (summarised in commit >>>> messages) which require case-by-case discussion. >>>> >>>> Additional high-level questions to move forwards: >>>> 1. Is loss of log levels by moving to dynamic debug, is actually a >>>> concern? Is dynamic debug a valid replacement? >>> >>> Dynamic debug is honestly going to be an improvement. I guess, Greg and >>> I said this back in Jan. > > +1 >>> >>>> 2. Whether debugfs should be dropped as well, found vestigial in [2] >>> >>> Yes. The "vchiq/log" should be removed. Ideally as part of this >>> patchset so it's easier to understand. >> >> Yes, but please do not remote vchiq_debugfs entirely. I'm working on a >> patch to move the state dump (debug feature) from the character device >> /dev/vchiq to debugfs /sys/kernel/debug/vchiq/dump_state. > > Can't the state dump be printed to dev_dbg() ? Will it pollute the > kernel log? Having debugfs for a single dump doesn't seem worthwhile if > the state dump can be incorporated to dev_dbg() too. debugfs was created for a good reason. dev_dbg() is a nice tool, but it isn't useful in every situation. In case of communication the usage of counter is quite popular and removing this ability would make debugging not easier. Try to make "cat /dev/vchiq" you will see you don't want pollute the kernel log with this kind of information. Actually i don't see a problem with have a single dump in debugfs. Let me send a draft of my changes for a better discussion ... Regards Stefan >> >>> >>>> 3. whether vchiq_log_trace() should actually be tracing support for >>>> VC04 >>> >>> That can be done later if people want. No need to discuss it now. > > Thanks Dan. >>> >>> regards, >>> dan carpenter >>> >
Just so it's clear, Stefan and I are on the same page on this. State dumps to debugfs are fine. regards, dan carpenter
Hello, This series attempts to restart the discussion on custom logging used in VC04. In the last feedback gathered in [1] it seems that the logging would rather be moved to use dynamic debug. The series tries to move in that direction. The elephant in the room is the ability of turning on/off log levels, which this series just drops. Compensated by a crude strings ("error", "warning", "info"... etc) for easier grepping. The log category are also just strings (which probably can be transformed to dynamic debug class names moving forwards?). To move forwards, I would like feedback on the broader direction. There are couple of TODOs in each of the patch (summarised in commit messages) which require case-by-case discussion. Additional high-level questions to move forwards: 1. Is loss of log levels by moving to dynamic debug, is actually a concern? Is dynamic debug a valid replacement? 2. Whether debugfs should be dropped as well, found vestigial in [2] 3. whether vchiq_log_trace() should actually be tracing support for VC04 All this is done to elimiate the TODO item in: drivers/staging/vc04_services/interface/TODO * Cleanup logging mechanism Addtional comments/questions are welcome. Thank you. Tested over media-tree master with dynamic debug configs enabled and drivers/staging/vc04_services/interface/TESTING. [1] https://lore.kernel.org/lkml/20230118115810.21979-1-umang.jain@ideasonboard.com/ [2] https://lore.kernel.org/lkml/Y8lHqd9FlxiXTLuW@kroah.com/ Umang Jain (4): staging: vc04: Convert vchiq_log_error() to use dynamic debug staging: vc04: Convert vchiq_log_warning() to use dynamic debug staging: vc04: Convert vchiq_log_info() to use dynamic debug staging: vc04: Convert vchiq_log_trace() to use dynamic debug .../interface/vchiq_arm/vchiq_arm.c | 127 +++++----- .../interface/vchiq_arm/vchiq_connected.c | 6 +- .../interface/vchiq_arm/vchiq_core.c | 225 ++++++++---------- .../interface/vchiq_arm/vchiq_core.h | 46 ++-- .../interface/vchiq_arm/vchiq_dev.c | 41 ++-- 5 files changed, 232 insertions(+), 213 deletions(-) base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d