On Thu, 19 Jan 2023 at 13:37, Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: > > On Thu, Jan 19, 2023 at 01:31:12PM +0000, Phil Elwell wrote: > > Hi all, > > > > > > On Wed, 18 Jan 2023 at 17:55, Stefan Wahren <stefan.wahren@xxxxxxxx> wrote: > > > > > > Hi Umang, > > > > > > [add Phil] > > > > > > Am 18.01.23 um 12:58 schrieb Umang Jain: > > > > Drop custom logging from the vchiq interface. > > > > Mostly of them are replaced with dev_dbg and friends > > > > and/or pr_info and friends. > > > > > > > > The debugfs log levels (in 4/4) are mapped to kernel > > > > logs levels (coming from include/linux/kern_levels.h) > > > > Would like some thoughts on it as I am not sure (hence > > > > marking this is RFC) > > > > > > > > From drivers/staging/vc04_services/interface/TODO: > > > > > > > > """ > > > > * Cleanup logging mechanism > > > > > > > > The driver should probably be using the standard kernel logging mechanisms > > > > such as dev_info, dev_dbg, and friends. > > > > > > i don't have any experience with vchiq logging/debug. So i'm not sure if > > > it's acceptable to lose the second log level dimension (like > > > vchiq_arm_log_level) completely. Complex drivers like brcmfmac have a > > > debug mask to avoid log spamming [1]. Maybe this is a compromise. > > > > > > Btw some loglevel locations has already been messed up during > > > refactoring :-( > > > > > > [1] - drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h > > > > > > > """ > > > > > > > > Umang Jain (4): > > > > staging: vc04_services: vchiq_core: Drop custom logging > > > > staging: vc04_services: vchiq_arm: Drop custom logging > > > > staging: vc04_services: Drop custom logging > > > > staging: vc04_services: Drop remnants of custom logging > > > > > > > > .../interface/vchiq_arm/vchiq_arm.c | 151 +++--- > > > > .../interface/vchiq_arm/vchiq_connected.c | 5 +- > > > > .../interface/vchiq_arm/vchiq_core.c | 479 ++++++++---------- > > > > .../interface/vchiq_arm/vchiq_core.h | 39 -- > > > > .../interface/vchiq_arm/vchiq_debugfs.c | 26 +- > > > > .../interface/vchiq_arm/vchiq_dev.c | 78 ++- > > > > 6 files changed, 329 insertions(+), 449 deletions(-) > > > > > > > > Thanks for the nudge - this patch set hasn't yet made its way through > > the sluggish rpi-kernel moderation. > > > > I understand the desire to remove the custom logging. I don't welcome > > the loss of flexibility that comes with such a strategy > > What "loss of flexibility"? You now have access to the full dynamic > debugging facilities that all of the rest of the kernel has. What is > lacking? Perhaps I've missed something, either in this patch set or the kernel as a whole, but how is one supposed to set different logging levels on different facilities within a driver/module, or even for the module as a whole? I don't consider anything that requires reference to individual lines in the source code to be a suitable replacement, clever though it is. > > , but I'm not > > going to argue about it. What's harder to understand is the state that > > this patchset leaves VCHIQ logging in. From what I can see, the > > per-service logging control has gone, but the code still contains > > macros that hint at something useful. Similarly, the debugfs support > > is completely vestigial, giving the appearance of control while > > actually achieving nothing. > > The debugfs files should also be removed if they don't do anything > anymore. > > thanks, > > greg k-h
