| Message ID | 20230118115810.21979-1-umang.jain@ideasonboard.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Drop custom logging | expand |
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