[RFC,0/4] Drop custom logging

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

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(-)
  

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(-)
>
  

On Wed, Jan 18, 2023 at 06:54:56PM +0100, Stefan Wahren 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
> 

Kernel logging is actually has a bunch of features.  You can turn them
on for just a module or function or enable a specific message.  See
Documentation/admin-guide/dynamic-debug-howto.rst
for more info.

This vchiq logging is a re-implementation of a subset of the features
that normal kernel logging infrastructure provides.  Moving to normal
logging will make it cleaner but also more flexible and powerful.  It's
better in every way.

The broadcom stuff is different and more complicated than what this
module is trying to do.  They are sorting out their logging according to
various components.  I understand the motivation, but they would
probably have been better just use standard logging like everyone else.

regards,
dan carpenter
  

On Wed, Jan 18, 2023 at 05:28:06PM +0530, Umang Jain wrote:
> Drop custom logging from the vchiq interface.
> Mostly of them are replaced with dev_dbg and friends
> and/or pr_info and friends. 

Everything should be dev_*() calls, there should never be a pr_* call in
a driver.

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

Do not have any "custom" debugging controls in a driver, they are not
special and should follow the whole rest of the kernel.  Just remove
them all and rely on the existing dev_*() calls instead please.

thanks,

greg k-h
  

Hi all,


On Wed, 18 Jan 2023 at 17:55, Stefan Wahren <stefan.wahren@i2se.com> 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
  

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@i2se.com> 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?

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

On Thu, 19 Jan 2023 at 13:37, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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@i2se.com> 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
  

On Thu, Jan 19, 2023 at 01:47:44PM +0000, Phil Elwell wrote:
> > > 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?

Yeah.  You will be still able to do that and more besides after the
transition.  Cleaning this up makes the code better in every way.

Documentation/admin-guide/dynamic-debug-howto.rst

regards,
dan carpenter
  

On Thu, 19 Jan 2023 at 14:25, Dan Carpenter <error27@gmail.com> wrote:
>
> On Thu, Jan 19, 2023 at 01:47:44PM +0000, Phil Elwell wrote:
> > > > 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?
>
> Yeah.  You will be still able to do that and more besides after the
> transition.  Cleaning this up makes the code better in every way.
>
> Documentation/admin-guide/dynamic-debug-howto.rst

Are you saying this patch set gets us to that point?
  

On Thu, Jan 19, 2023 at 02:31:50PM +0000, Phil Elwell wrote:
> On Thu, 19 Jan 2023 at 14:25, Dan Carpenter <error27@gmail.com> wrote:
> >
> > On Thu, Jan 19, 2023 at 01:47:44PM +0000, Phil Elwell wrote:
> > > > > 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?
> >
> > Yeah.  You will be still able to do that and more besides after the
> > transition.  Cleaning this up makes the code better in every way.
> >
> > Documentation/admin-guide/dynamic-debug-howto.rst
> 
> Are you saying this patch set gets us to that point?

Yes.  The patch has some issues, but yes.

regards,
dan carpenter
  

On Thu, 19 Jan 2023 at 14:37, Dan Carpenter <error27@gmail.com> wrote:
>
> On Thu, Jan 19, 2023 at 02:31:50PM +0000, Phil Elwell wrote:
> > On Thu, 19 Jan 2023 at 14:25, Dan Carpenter <error27@gmail.com> wrote:
> > >
> > > On Thu, Jan 19, 2023 at 01:47:44PM +0000, Phil Elwell wrote:
> > > > > > 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?
> > >
> > > Yeah.  You will be still able to do that and more besides after the
> > > transition.  Cleaning this up makes the code better in every way.
> > >
> > > Documentation/admin-guide/dynamic-debug-howto.rst
> >
> > Are you saying this patch set gets us to that point?
>
> Yes.  The patch has some issues, but yes.

OK, thanks. I'll be watching how this plays out with interest.

Phil
  

On Thu, Jan 19, 2023 at 05:37:48PM +0300, Dan Carpenter wrote:
> On Thu, Jan 19, 2023 at 02:31:50PM +0000, Phil Elwell wrote:
> > On Thu, 19 Jan 2023 at 14:25, Dan Carpenter wrote:
> > > On Thu, Jan 19, 2023 at 01:47:44PM +0000, Phil Elwell wrote:
> > > > > > 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?
> > >
> > > Yeah.  You will be still able to do that and more besides after the
> > > transition.  Cleaning this up makes the code better in every way.
> > >
> > > Documentation/admin-guide/dynamic-debug-howto.rst
> > 
> > Are you saying this patch set gets us to that point?
> 
> Yes.  The patch has some issues, but yes.

I think I'm missing something too then. Dynamic debug provides the
ability to easily switch dev_dbg() messages on and off at runtime, but
it doesn't provide, as far as I'm aware, log levels or log categories.

Log levels are currently used by the vchiq code to suppress messages
below a certain level. Kernel log levels are not an exact replacement,
as the messages still end up in the kernel log (except for debug
messages).

Log categories are used to group messages in categories and control
their log level per category. As far as I know, dynamic debug doesn't
provide any such feature.
  

On Fri, Jan 20, 2023 at 02:53:17AM +0200, Laurent Pinchart wrote:
> On Thu, Jan 19, 2023 at 05:37:48PM +0300, Dan Carpenter wrote:
> > On Thu, Jan 19, 2023 at 02:31:50PM +0000, Phil Elwell wrote:
> > > On Thu, 19 Jan 2023 at 14:25, Dan Carpenter wrote:
> > > > On Thu, Jan 19, 2023 at 01:47:44PM +0000, Phil Elwell wrote:
> > > > > > > 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?
> > > >
> > > > Yeah.  You will be still able to do that and more besides after the
> > > > transition.  Cleaning this up makes the code better in every way.
> > > >
> > > > Documentation/admin-guide/dynamic-debug-howto.rst
> > > 
> > > Are you saying this patch set gets us to that point?
> > 
> > Yes.  The patch has some issues, but yes.
> 
> I think I'm missing something too then. Dynamic debug provides the
> ability to easily switch dev_dbg() messages on and off at runtime, but
> it doesn't provide, as far as I'm aware, log levels or log categories.
> 
> Log levels are currently used by the vchiq code to suppress messages
> below a certain level. Kernel log levels are not an exact replacement,
> as the messages still end up in the kernel log (except for debug
> messages).
> 
> Log categories are used to group messages in categories and control
> their log level per category. As far as I know, dynamic debug doesn't
> provide any such feature.

After a bit more research (which I should have done before replying,
sorry), it looks like dynamic debug has support for classes, which are
used by, for instance, the DRM logging infrastructure (see
include/drm/drm_print.h). I don't see that being wired up to dev_*()
print macros though, am I missing something, or would vchiq need to keep
using custom logging macros (with dynamic debug used as a backend,
replacing the current custom implementation) to make use of this feature
?
  

Hi Umang,

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

at first i want to thank you for the work on vchiq so far.

There is something which is not directly related to this series, but it 
is also about debugging. The driver has a buffer which is accessed by 
it's own DEBUG_* macros. The content of this debug buffer can be dumped 
via the /dev/vchiq which is also used by ioctl. I would appreciate to 
move this dump feature into a new debugfs entry.

Best regards
  

Hi Stefan,

Quoting Stefan Wahren (2023-01-22 14:21:05)
> Hi Umang,
> 
> 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.
> > """
> 
> at first i want to thank you for the work on vchiq so far.
> 
> There is something which is not directly related to this series, but it 
> is also about debugging. The driver has a buffer which is accessed by 
> it's own DEBUG_* macros. The content of this debug buffer can be dumped 
> via the /dev/vchiq which is also used by ioctl. I would appreciate to 
> move this dump feature into a new debugfs entry.

Do you have a full list of the tasks you'd like to see completed ?
(including/or above drivers/staging/vc04_services/interface/TODO)

It would help to have a clear picture of tasks needed to get this driver
destaged, so that we can support the ISP upstream.

Regards
--
Kieran
  

Hi Kieran,

Am 22.01.23 um 17:26 schrieb Kieran Bingham:
> Hi Stefan,
>
> Quoting Stefan Wahren (2023-01-22 14:21:05)
>> Hi Umang,
>>
>> 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.
>>> """
>> at first i want to thank you for the work on vchiq so far.
>>
>> There is something which is not directly related to this series, but it
>> is also about debugging. The driver has a buffer which is accessed by
>> it's own DEBUG_* macros. The content of this debug buffer can be dumped
>> via the /dev/vchiq which is also used by ioctl. I would appreciate to
>> move this dump feature into a new debugfs entry.
> Do you have a full list of the tasks you'd like to see completed ?
> (including/or above drivers/staging/vc04_services/interface/TODO)

i consider every point except of point 1 (importing new drivers) as 
necessary to leave staging.

Additionally there is the additional point (i can add them to the TODO) 
above. Unfortunately i don't have a complete insight, how vchiq should 
be to be acceptable. Sorry, if i can't help you further with possible 
resource planning.

Are some points on the TODO list unclear?

Thanks

>
> It would help to have a clear picture of tasks needed to get this driver
> destaged, so that we can support the ISP upstream.
>
> Regards
> --
> Kieran
  

On Fri, Jan 20, 2023 at 02:53:15AM +0200, Laurent Pinchart wrote:
> Log categories are used to group messages in categories and control
> their log level per category. As far as I know, dynamic debug doesn't
> provide any such feature.

One idea is you could put the category in the format string.  Then you
could just grep /proc/dynamic_debug/control for it and enable the printks
like that.

regards,
dan carpenter
  

Hi Stefan,

Quoting Stefan Wahren (2023-01-22 18:07:03)
> Hi Kieran,
> 
> Am 22.01.23 um 17:26 schrieb Kieran Bingham:
> > Hi Stefan,
> >
> > Quoting Stefan Wahren (2023-01-22 14:21:05)
> >> Hi Umang,
> >>
> >> 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.
> >>> """
> >> at first i want to thank you for the work on vchiq so far.
> >>
> >> There is something which is not directly related to this series, but it
> >> is also about debugging. The driver has a buffer which is accessed by
> >> it's own DEBUG_* macros. The content of this debug buffer can be dumped
> >> via the /dev/vchiq which is also used by ioctl. I would appreciate to
> >> move this dump feature into a new debugfs entry.
> > Do you have a full list of the tasks you'd like to see completed ?
> > (including/or above drivers/staging/vc04_services/interface/TODO)
> 
> i consider every point except of point 1 (importing new drivers) as 
> necessary to leave staging.

Thanks

> Additionally there is the additional point (i can add them to the TODO) 
> above. Unfortunately i don't have a complete insight, how vchiq should 
> be to be acceptable. Sorry, if i can't help you further with possible 
> resource planning.
> 
> Are some points on the TODO list unclear?

I believe the list is fine, but I was enquiring if there were anymore
additional tasks above the scope listed in
drivers/staging/vc04_services/interface/TODO which are required. I don't
think you need to send a patch for the task above - unless there are a
lot more tasks required, or it becomes too much to do now.

The goal is to get the ISP upstream to support libcamera, and it would
help to know how deep the rabbit hole really is ;-)

--
Kieran