[1/2] drm: Only #define DEBUG if CONFIG_DYNAMIC_DEBUG is disabled

Commit Message

Michel Dänzer Dec. 5, 2018, 4:56 p.m. UTC

From: Michel Dänzer <[e-mail]>

The following cases are possible for pr_debug():

1. CONFIG_DYNAMIC_DEBUG disabled
   a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
      it never generates any output.
   b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
      i.e. it generates output which doesn't appear in dmesg by default,
      can be enabled dynamically.

2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
   dynamic_pr_debug()
   a) DEBUG not defined: dynamic_pr_debug() generates no output by
      default, can be enabled dynamically.
   b) DEBUG defined: dynamic_pr_debug() generates output by default,
      can be disabled dynamically.

The intention for drm_debug_printer() is to generate output which
doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.

Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
Signed-off-by: Michel Dänzer <[e-mail]>
---
 drivers/gpu/drm/drm_print.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Zhang, Jerry(Junwei) Dec. 6, 2018, 2:40 a.m. UTC | #1

On 12/6/18 12:56 AM, Michel Dänzer wrote:
> From: Michel Dänzer <[e-mail]>
>
> The following cases are possible for pr_debug():
>
> 1. CONFIG_DYNAMIC_DEBUG disabled
>     a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
>        it never generates any output.
>     b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
>        i.e. it generates output which doesn't appear in dmesg by default,
>        can be enabled dynamically.
>
> 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
>     dynamic_pr_debug()
>     a) DEBUG not defined: dynamic_pr_debug() generates no output by
>        default, can be enabled dynamically.
>     b) DEBUG defined: dynamic_pr_debug() generates output by default,
>        can be disabled dynamically.
>
> The intention for drm_debug_printer() is to generate output which
> doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
> cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
> instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
>
> Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
> Signed-off-by: Michel Dänzer <[e-mail]>
Reviewed-by: Junwei Zhang <[e-mail]>

> ---
>   drivers/gpu/drm/drm_print.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index 0e7fc3e7dfb4..ee56e4a1b343 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -23,11 +23,13 @@
>    * Rob Clark <[e-mail]>
>    */
>   
> -#define DEBUG /* for pr_debug() */
> -
>   #include <stdarg.h>
>   #include <linux/seq_file.h>
>   #include <drm/drmP.h>
> +
> +#ifndef CONFIG_DYNAMIC_DEBUG
> +#define DEBUG /* for pr_debug() */
> +#endif
>   #include <drm/drm_print.h>
>   
>   void __drm_puts_coredump(struct drm_printer *p, const char *str)

Joe Perches Dec. 6, 2018, 2:51 a.m. UTC | #2

On Thu, 2018-12-06 at 10:40 +0800, Zhang, Jerry(Junwei) wrote:
> On 12/6/18 12:56 AM, Michel Dänzer wrote:
> > From: Michel Dänzer <[e-mail]>
> > 
> > The following cases are possible for pr_debug():
> > 
> > 1. CONFIG_DYNAMIC_DEBUG disabled
> >     a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
> >        it never generates any output.
> >     b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
> >        i.e. it generates output which doesn't appear in dmesg by default,
> >        can be enabled dynamically.
> > 
> > 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
> >     dynamic_pr_debug()
> >     a) DEBUG not defined: dynamic_pr_debug() generates no output by
> >        default, can be enabled dynamically.
> >     b) DEBUG defined: dynamic_pr_debug() generates output by default,
> >        can be disabled dynamically.
> > 
> > The intention for drm_debug_printer() is to generate output which
> > doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
> > cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
> > instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
> > 
> > Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")

I very much doubt this is a fix.

Did you read the commit log for this commit?

It says "make sure it will always produce output"

And why didn't you cc Chris Wilson, the author of that patch?

> > Signed-off-by: Michel Dänzer <[e-mail]>
> Reviewed-by: Junwei Zhang <[e-mail]>
> 
> > ---
> >   drivers/gpu/drm/drm_print.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> > index 0e7fc3e7dfb4..ee56e4a1b343 100644
> > --- a/drivers/gpu/drm/drm_print.c
> > +++ b/drivers/gpu/drm/drm_print.c
> > @@ -23,11 +23,13 @@
> >    * Rob Clark <[e-mail]>
> >    */
> >   
> > -#define DEBUG /* for pr_debug() */
> > -
> >   #include <stdarg.h>
> >   #include <linux/seq_file.h>
> >   #include <drm/drmP.h>
> > +
> > +#ifndef CONFIG_DYNAMIC_DEBUG
> > +#define DEBUG /* for pr_debug() */
> > +#endif
> >   #include <drm/drm_print.h>
> >   
> >   void __drm_puts_coredump(struct drm_printer *p, const char *str)

Chris Wilson Dec. 6, 2018, 9:12 a.m. UTC | #3

Quoting Zhang, Jerry(Junwei) (2018-12-06 02:40:42)
> On 12/6/18 12:56 AM, Michel Dänzer wrote:
> > From: Michel Dänzer <[e-mail]>
> >
> > The following cases are possible for pr_debug():
> >
> > 1. CONFIG_DYNAMIC_DEBUG disabled
> >     a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
> >        it never generates any output.
> >     b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
> >        i.e. it generates output which doesn't appear in dmesg by default,
> >        can be enabled dynamically.
> >
> > 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
> >     dynamic_pr_debug()
> >     a) DEBUG not defined: dynamic_pr_debug() generates no output by
> >        default, can be enabled dynamically.
> >     b) DEBUG defined: dynamic_pr_debug() generates output by default,
> >        can be disabled dynamically.
> >
> > The intention for drm_debug_printer() is to generate output which
> > doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
> > cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
> > instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
> >
> > Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
> > Signed-off-by: Michel Dänzer <[e-mail]>
> Reviewed-by: Junwei Zhang <[e-mail]>

At the cost of 1a? Nah.
-Chris

Michel Dänzer Dec. 6, 2018, 9:21 a.m. UTC | #4

On 2018-12-06 10:12 a.m., Chris Wilson wrote:
> Quoting Zhang, Jerry(Junwei) (2018-12-06 02:40:42)
>> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>>> From: Michel Dänzer <[e-mail]>
>>>
>>> The following cases are possible for pr_debug():
>>>
>>> 1. CONFIG_DYNAMIC_DEBUG disabled
>>>     a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
>>>        it never generates any output.
>>>     b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
>>>        i.e. it generates output which doesn't appear in dmesg by default,
>>>        can be enabled dynamically.
>>>
>>> 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
>>>     dynamic_pr_debug()
>>>     a) DEBUG not defined: dynamic_pr_debug() generates no output by
>>>        default, can be enabled dynamically.
>>>     b) DEBUG defined: dynamic_pr_debug() generates output by default,
>>>        can be disabled dynamically.
>>>
>>> The intention for drm_debug_printer() is to generate output which
>>> doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
>>> cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
>>> instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
>>>
>>> Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
>>> Signed-off-by: Michel Dänzer <[e-mail]>
>> Reviewed-by: Junwei Zhang <[e-mail]>
> 
> At the cost of 1a? Nah.

We still #define DEBUG if CONFIG_DYNAMIC_DEBUG isn't defined, so it's
still 1b), not 1a).

Michel Dänzer Dec. 6, 2018, 9:23 a.m. UTC | #5

On 2018-12-06 3:51 a.m., Joe Perches wrote:
> On Thu, 2018-12-06 at 10:40 +0800, Zhang, Jerry(Junwei) wrote:
>> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>>> From: Michel Dänzer <[e-mail]>
>>>
>>> The following cases are possible for pr_debug():
>>>
>>> 1. CONFIG_DYNAMIC_DEBUG disabled
>>>     a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
>>>        it never generates any output.
>>>     b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
>>>        i.e. it generates output which doesn't appear in dmesg by default,
>>>        can be enabled dynamically.
>>>
>>> 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
>>>     dynamic_pr_debug()
>>>     a) DEBUG not defined: dynamic_pr_debug() generates no output by
>>>        default, can be enabled dynamically.
>>>     b) DEBUG defined: dynamic_pr_debug() generates output by default,
>>>        can be disabled dynamically.
>>>
>>> The intention for drm_debug_printer() is to generate output which
>>> doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
>>> cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
>>> instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
>>>
>>> Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
> 
> I very much doubt this is a fix.
> 
> Did you read the commit log for this commit?
> 
> It says "make sure it will always produce output"

I thought the commit log covered this, suggestions for improvement welcome.

Chris' change addressed case 1a), but also took us from 2a) to 2b). But
we want 2a).

I suspect Chris missed that pr_debug()'s output is visible by default if
CONFIG_DYNAMIC_DEBUG and DEBUG are both defined.


> And why didn't you cc Chris Wilson, the author of that patch?

I used the get_maintainer.pl script. Thanks for adding Chris.


P.S. FYI, your e-mail had a very aggressive tone to me, not sure what for.


>>> Signed-off-by: Michel Dänzer <[e-mail]>
>> Reviewed-by: Junwei Zhang <[e-mail]>
>>
>>> ---
>>>   drivers/gpu/drm/drm_print.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
>>> index 0e7fc3e7dfb4..ee56e4a1b343 100644
>>> --- a/drivers/gpu/drm/drm_print.c
>>> +++ b/drivers/gpu/drm/drm_print.c
>>> @@ -23,11 +23,13 @@
>>>    * Rob Clark <[e-mail]>
>>>    */
>>>   
>>> -#define DEBUG /* for pr_debug() */
>>> -
>>>   #include <stdarg.h>
>>>   #include <linux/seq_file.h>
>>>   #include <drm/drmP.h>
>>> +
>>> +#ifndef CONFIG_DYNAMIC_DEBUG
>>> +#define DEBUG /* for pr_debug() */
>>> +#endif
>>>   #include <drm/drm_print.h>
>>>   
>>>   void __drm_puts_coredump(struct drm_printer *p, const char *str)
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Chris Wilson Dec. 6, 2018, 9:28 a.m. UTC | #6

Quoting Michel Dänzer (2018-12-06 09:21:40)
> On 2018-12-06 10:12 a.m., Chris Wilson wrote:
> > Quoting Zhang, Jerry(Junwei) (2018-12-06 02:40:42)
> >> On 12/6/18 12:56 AM, Michel Dänzer wrote:
> >>> From: Michel Dänzer <[e-mail]>
> >>>
> >>> The following cases are possible for pr_debug():
> >>>
> >>> 1. CONFIG_DYNAMIC_DEBUG disabled
> >>>     a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
> >>>        it never generates any output.
> >>>     b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
> >>>        i.e. it generates output which doesn't appear in dmesg by default,
> >>>        can be enabled dynamically.
> >>>
> >>> 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
> >>>     dynamic_pr_debug()
> >>>     a) DEBUG not defined: dynamic_pr_debug() generates no output by
> >>>        default, can be enabled dynamically.
> >>>     b) DEBUG defined: dynamic_pr_debug() generates output by default,
> >>>        can be disabled dynamically.
> >>>
> >>> The intention for drm_debug_printer() is to generate output which
> >>> doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
> >>> cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
> >>> instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
> >>>
> >>> Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
> >>> Signed-off-by: Michel Dänzer <[e-mail]>
> >> Reviewed-by: Junwei Zhang <[e-mail]>
> > 
> > At the cost of 1a? Nah.
> 
> We still #define DEBUG if CONFIG_DYNAMIC_DEBUG isn't defined, so it's
> still 1b), not 1a).

I completely fluffed my reading of ifndef.

#if !IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
-Chris

Joe Perches Dec. 6, 2018, 11:41 a.m. UTC | #7

On Thu, 2018-12-06 at 10:23 +0100, Michel Dänzer wrote:
> On 2018-12-06 3:51 a.m., Joe Perches wrote:
> > On Thu, 2018-12-06 at 10:40 +0800, Zhang, Jerry(Junwei) wrote:
> > > On 12/6/18 12:56 AM, Michel Dänzer wrote:
> > > > From: Michel Dänzer <[e-mail]>
> > > > 
> > > > The following cases are possible for pr_debug():
> > > > 
> > > > 1. CONFIG_DYNAMIC_DEBUG disabled
> > > >     a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
> > > >        it never generates any output.
> > > >     b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
> > > >        i.e. it generates output which doesn't appear in dmesg by default,
> > > >        can be enabled dynamically.
> > > > 
> > > > 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
> > > >     dynamic_pr_debug()
> > > >     a) DEBUG not defined: dynamic_pr_debug() generates no output by
> > > >        default, can be enabled dynamically.
> > > >     b) DEBUG defined: dynamic_pr_debug() generates output by default,
> > > >        can be disabled dynamically.
> > > > 
> > > > The intention for drm_debug_printer() is to generate output which
> > > > doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
> > > > cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
> > > > instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
> > > > 
> > > > Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
> > 
> > I very much doubt this is a fix.
> > 
> > Did you read the commit log for this commit?
> > 
> > It says "make sure it will always produce output"
> 
> I thought the commit log covered this, suggestions for improvement welcome.
> 
> Chris' change addressed case 1a), but also took us from 2a) to 2b). But
> we want 2a)

> I suspect Chris missed that pr_debug()'s output is visible by default if
> CONFIG_DYNAMIC_DEBUG and DEBUG are both defined.

I believe you and Chris have different goals and that your
point of 2b is
misguided.

I think the language used in the Chris' commit log is
plain ans obvious.  'always produce output'.

> > And why didn't you cc Chris Wilson, the author of that patch?k
> 
> I used the get_maintainer.pl script. Thanks for adding Chris.
> P.S. FYI, your e-mail had a very aggressive tone to me, not sure what for.

No worries.

I think the language I used is also plain and obvious
and you are overreading.

cheers, Joe

Michel Dänzer Dec. 6, 2018, 11:52 a.m. UTC | #8

On 2018-12-06 12:41 p.m., Joe Perches wrote:
> On Thu, 2018-12-06 at 10:23 +0100, Michel Dänzer wrote:
>> On 2018-12-06 3:51 a.m., Joe Perches wrote:
>>> On Thu, 2018-12-06 at 10:40 +0800, Zhang, Jerry(Junwei) wrote:
>>>> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>>>>> From: Michel Dänzer <[e-mail]>
>>>>>
>>>>> The following cases are possible for pr_debug():
>>>>>
>>>>> 1. CONFIG_DYNAMIC_DEBUG disabled
>>>>>     a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
>>>>>        it never generates any output.
>>>>>     b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
>>>>>        i.e. it generates output which doesn't appear in dmesg by default,
>>>>>        can be enabled dynamically.
>>>>>
>>>>> 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
>>>>>     dynamic_pr_debug()
>>>>>     a) DEBUG not defined: dynamic_pr_debug() generates no output by
>>>>>        default, can be enabled dynamically.
>>>>>     b) DEBUG defined: dynamic_pr_debug() generates output by default,
>>>>>        can be disabled dynamically.
>>>>>
>>>>> The intention for drm_debug_printer() is to generate output which
>>>>> doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
>>>>> cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
>>>>> instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
>>>>>
>>>>> Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
>>>
>>> I very much doubt this is a fix.
>>>
>>> Did you read the commit log for this commit?
>>>
>>> It says "make sure it will always produce output"
>>
>> I thought the commit log covered this, suggestions for improvement welcome.
>>
>> Chris' change addressed case 1a), but also took us from 2a) to 2b). But
>> we want 2a)
> 
>> I suspect Chris missed that pr_debug()'s output is visible by default if
>> CONFIG_DYNAMIC_DEBUG and DEBUG are both defined.
> 
> I believe you and Chris have different goals and that your
> point of 2b is
> misguided.
> 
> I think the language used in the Chris' commit log is
> plain ans obvious.  'always produce output'.

That was probably meant as "make sure there always can be output", which
isn't the case with 1a (no_printk suppresses the output at compile time,
the strings might not even exist in the binaries).

In contrast to the 2b case, the pr_debug output isn't visible by default
with 1b, so the latter doesn't fit "always produce output" either. But
Chris' initial follow-up in this thread seems to confirm my assumption
that his change was about 1a/b.

Joe Perches Dec. 6, 2018, 12:23 p.m. UTC | #9

On Thu, 2018-12-06 at 12:52 +0100, Michel Dänzer wrote:
> In contrast to the 2b case, the pr_debug output isn't visible by default
> with 1b, so the latter doesn't fit "always produce output" either.

I think you are mistaken here.

Adding #define DEBUG as Chris did enables pr_debug output
and is your 1b.

Perhaps your default console logging level is set to a
non-default value.

CONSOLE_LOGLEVEL_DEFAULT

lib/Kconfig.debug:config CONSOLE_LOGLEVEL_DEFAULT
lib/Kconfig.debug-      int "Default console loglevel (1-15)"
lib/Kconfig.debug-      range 1 15
lib/Kconfig.debug-      default "7"

Michel Dänzer Dec. 6, 2018, 2:41 p.m. UTC | #10

On 2018-12-06 1:23 p.m., Joe Perches wrote:
> On Thu, 2018-12-06 at 12:52 +0100, Michel Dänzer wrote:
>> In contrast to the 2b case, the pr_debug output isn't visible by default
>> with 1b, so the latter doesn't fit "always produce output" either.
> 
> I think you are mistaken here.

Still puzzled as to what you're hoping to achieve with that kind of
language. None of the confusion about this patch has been on my part. :)


> Adding #define DEBUG as Chris did enables pr_debug output
> and is your 1b.
> 
> Perhaps your default console logging level is set to a
> non-default value.

I have CONFIG_DYNAMIC_DEBUG enabled in my kernels. The problem addressed
by this patch is that messages from drm_debug_printer are visible by
default (case 2b), whereas they shouldn't be (case 2a, like 1b).

Daniel Thompson Dec. 6, 2018, 4:10 p.m. UTC | #11

On Thu, Dec 06, 2018 at 03:41:16PM +0100, Michel Dänzer wrote:
> On 2018-12-06 1:23 p.m., Joe Perches wrote:
> > On Thu, 2018-12-06 at 12:52 +0100, Michel Dänzer wrote:
> >> In contrast to the 2b case, the pr_debug output isn't visible by default
> >> with 1b, so the latter doesn't fit "always produce output" either.
> > 
> > I think you are mistaken here.
> 
> Still puzzled as to what you're hoping to achieve with that kind of
> language. None of the confusion about this patch has been on my part. :)
> 
> 
> > Adding #define DEBUG as Chris did enables pr_debug output
> > and is your 1b.
> > 
> > Perhaps your default console logging level is set to a
> > non-default value.
> 
> I have CONFIG_DYNAMIC_DEBUG enabled in my kernels. The problem addressed
> by this patch is that messages from drm_debug_printer are visible by
> default (case 2b), whereas they shouldn't be (case 2a, like 1b).

When enabled (either dynamically or statically) pr_debug() will emit
output at KERN_DEBUG level regardless of whether CONFIG_DYNAMIC_DEBUG
is defined or not.

Thus unless you change additional settings (either dynamically or
statically) then debug messages should not be shown on the console
because the default settings filter KERN_DEBUG messages. However they
are available via dmesg and system loggers (syslogd, journal, etc).

The patch proposed will change the behaviour of the debug messages
w.r.t. system loggers based on whether the user has enabled
CONFIG_DYNAMIC_DEBUG or not, violating the principle of least surprise.


Daniel.

Michel Dänzer Dec. 6, 2018, 4:14 p.m. UTC | #12

On 2018-12-06 5:10 p.m., Daniel Thompson wrote:
> On Thu, Dec 06, 2018 at 03:41:16PM +0100, Michel Dänzer wrote:
>> On 2018-12-06 1:23 p.m., Joe Perches wrote:
>>> On Thu, 2018-12-06 at 12:52 +0100, Michel Dänzer wrote:
>>>> In contrast to the 2b case, the pr_debug output isn't visible by default
>>>> with 1b, so the latter doesn't fit "always produce output" either.
>>>
>>> I think you are mistaken here.
>>
>> Still puzzled as to what you're hoping to achieve with that kind of
>> language. None of the confusion about this patch has been on my part. :)
>>
>>
>>> Adding #define DEBUG as Chris did enables pr_debug output
>>> and is your 1b.
>>>
>>> Perhaps your default console logging level is set to a
>>> non-default value.
>>
>> I have CONFIG_DYNAMIC_DEBUG enabled in my kernels. The problem addressed
>> by this patch is that messages from drm_debug_printer are visible by
>> default (case 2b), whereas they shouldn't be (case 2a, like 1b).
> 
> When enabled (either dynamically or statically) pr_debug() will emit
> output at KERN_DEBUG level regardless of whether CONFIG_DYNAMIC_DEBUG
> is defined or not.
> 
> Thus unless you change additional settings (either dynamically or
> statically) then debug messages should not be shown on the console
> because the default settings filter KERN_DEBUG messages. However they
> are available via dmesg and system loggers (syslogd, journal, etc).
> 
> The patch proposed will change the behaviour of the debug messages
> w.r.t. system loggers based on whether the user has enabled
> CONFIG_DYNAMIC_DEBUG or not, violating the principle of least surprise.

Ah, that makes sense now, thanks.

I'm withdrawing this patch.

Joe Perches Dec. 6, 2018, 4:22 p.m. UTC | #13

On Thu, 2018-12-06 at 15:41 +0100, Michel Dänzer wrote:
> On 2018-12-06 1:23 p.m., Joe Perches wrote:
> > On Thu, 2018-12-06 at 12:52 +0100, Michel Dänzer wrote:
> > > In contrast to the 2b case, the pr_debug output isn't visible by default
> > > with 1b, so the latter doesn't fit "always produce output" either.
> > 
> > I think you are mistaken here.
> 
> Still puzzled as to what you're hoping to achieve with that kind of
> language. None of the confusion about this patch has been on my part. :)

Doubtful.

> > Adding #define DEBUG as Chris did enables pr_debug output
> > and is your 1b.
> > 
> > Perhaps your default console logging level is set to a
> > non-default value.
> 
> I have CONFIG_DYNAMIC_DEBUG enabled in my kernels. The problem addressed
> by this patch is that messages from drm_debug_printer are visible by
> default (case 2b), whereas they shouldn't be (case 2a, like 1b).

The default for config_dynamic_debug when DEBUG is defined is
to output the message equivalent to when DEBUG is defined and
config_dynamic_debug is not.

That's a good thing and has been the general default since 2011.

You want to override that, I suppose that's OK but it's
definitely not a _fix_ as you have written.

It's just a change in behavior.

You added a "Fixes:" line.  I believe that wrong.

A long time ago, at my suggestion:

commit b558c96ffa53f4b3dd52b774e4fb7a52982ab52b
Author: Jim Cromie <[e-mail]>
Date:   Mon Dec 19 17:11:18 2011 -0500

    dynamic_debug: make dynamic-debug supersede DEBUG ccflag
    
    If CONFIG_DYNAMIC_DEBUG is defined, honor it over DEBUG, so that
    pr_debug()s are controllable, instead of always-on.  When DEBUG is
    also defined, change _DPRINTK_FLAGS_DEFAULT to enable printing by
    default.

cheers, Joe

Patch

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 0e7fc3e7dfb4..ee56e4a1b343 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -23,11 +23,13 @@ 
  * Rob Clark <[e-mail]>
  */
 
-#define DEBUG /* for pr_debug() */
-
 #include <stdarg.h>
 #include <linux/seq_file.h>
 #include <drm/drmP.h>
+
+#ifndef CONFIG_DYNAMIC_DEBUG
+#define DEBUG /* for pr_debug() */
+#endif
 #include <drm/drm_print.h>
 
 void __drm_puts_coredump(struct drm_printer *p, const char *str)