Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed

[Nicolas Boichat]

On Fri, Aug 21, 2020 at 11:01 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Fri, 21 Aug 2020 10:39:02 +0800
> Nicolas Boichat <drinkcat@xxxxxxxxxxxx> wrote:
>
> > I'm not sure how that helps? I mean, the use case you have in mind is
> > somebody reusing a distro/random config and not being able to use
> > trace_printk, right? If that config has CONFIG_DISABLE_TRACE_PRINTK=y,
> > then the developer will still need to flip that back.
> >
> > Note that the option I'm added has default=y (_allow_ trace_printk),
> > so I don't think default y or default n really matters?
>
> Ideally, the production system doesn't have it set. It only sets it to
> make sure that it's clean before sending out. But then it can add it
> back before production. Yeah, it's pretty much cutting hairs between
> the two. I don't like either one.
>
> Really, if you are worried about this, just add your patch to your
> local tree. I'm not sure this is something that can be fixed upstream.
>
> Another idea is to add something like below, and build with:
>
>  make CHECK_TRACE_PRINT=1
>
> This way it is a build command line option that causes the build to
> fail if trace_printk() is added.
>
> This way production systems can add this to make sure their kernels are
> free of trace_printk() but it doesn't affect the config that is used.

(for some reason I missed this reply in the thread ,-P)

Thanks, that's quite a nice idea, I'll try it out (not sure if we
still want that build_assert trick). We'd lose the automatic detection
of issues through randconfig kernel test robot, but hopefully if
enough distros enable it they'll start filing reports about issues.

And maybe we can use that in combination with a checkpatch.pl check.

(it turns out I'm having a hard time finding a good spot for this test
in our Chrome OS build infra, so an upstream-friendly solution would
be much better ,-P)

>
> -- Steve
>
> [ Not even compiled tested! ]
>
> diff --git a/Makefile b/Makefile
> index 2057c92a6205..5714a738879d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -91,6 +91,13 @@ else
>    Q = @
>  endif
>
> +ifeq ("$(origin CHECK_TRACE_PRINTK)", "command line")
> +  KBUILD_NO_TRACE_PRINTK = $(NO_TRACE_PRINTK)
> +endif
> +ifndef KBUILD_NO_TRACE_PRINTK
> +  KBUILD_NO_TRACE_PRINTK = 0
> +endif
> +
>  # If the user is running make -s (silent mode), suppress echoing of
>  # commands
>
> @@ -839,6 +846,10 @@ KBUILD_AFLAGS      += -gz=zlib
>  KBUILD_LDFLAGS += --compress-debug-sections=zlib
>  endif
>
> +ifeq ($(KBUILD_NO_TRACE_PRINTK),1)
> +KBUILD_CFLAGS += -DNO_TRACE_PRINTK
> +endif
> +
>  KBUILD_CFLAGS += $(DEBUG_CFLAGS)
>  export DEBUG_CFLAGS
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 500def620d8f..bee432547d26 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -680,11 +680,14 @@ extern void tracing_stop(void);
>  static inline __printf(1, 2)
>  void ____trace_printk_check_format(const char *fmt, ...)
>  {
> +#ifdef NO_TRACE_PRINTK
> +       extern void __no_trace_printk_on_build(void);
> +       __no_trace_printk_on_build();
> +#endif
>  }
>  #define __trace_printk_check_format(fmt, args...)                      \
>  do {                                                                   \
> -       if (0)                                                          \
> -               ____trace_printk_check_format(fmt, ##args);             \
> +       ____trace_printk_check_format(fmt, ##args);                     \
>  } while (0)
>
>  /**