Re: [RFC PATCH 0/4] Drop custom logging

[Phil Elwell]

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, 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.

Phil