[PATCH v4 1/3, RESEND] media: camss: vfe: Use trace_printk for debugging only

[PATCH v4 1/3, RESEND] media: camss: vfe: Use trace_printk for debugging only

[Nicolas Boichat]

trace_printk should not be used in production code. Since
tracing interrupts is presumably latency sensitive, pr_dbg is
not appropriate, so guard the call with a preprocessor symbol
that can be defined for debugging purpose.

Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
---

(resending this patch as part of the whole series, since we need a new
patch 3/3 now).

 drivers/media/platform/qcom/camss/camss-vfe-4-1.c | 2 ++
 drivers/media/platform/qcom/camss/camss-vfe-4-7.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
index 174a36be6f5d866..0c57171fae4f9e9 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
@@ -936,8 +936,10 @@ static irqreturn_t vfe_isr(int irq, void *dev)
 
 	vfe->ops->isr_read(vfe, &value0, &value1);
 
+#ifdef CAMSS_VFE_TRACE_IRQ
 	trace_printk("VFE: status0 = 0x%08x, status1 = 0x%08x\n",
 		     value0, value1);
+#endif
 
 	if (value0 & VFE_0_IRQ_STATUS_0_RESET_ACK)
 		vfe->isr_ops.reset_ack(vfe);
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
index 0dca8bf9281e774..307675925e5c779 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
@@ -1058,8 +1058,10 @@ static irqreturn_t vfe_isr(int irq, void *dev)
 
 	vfe->ops->isr_read(vfe, &value0, &value1);
 
+#ifdef CAMSS_VFE_TRACE_IRQ
 	trace_printk("VFE: status0 = 0x%08x, status1 = 0x%08x\n",
 		     value0, value1);
+#endif
 
 	if (value0 & VFE_0_IRQ_STATUS_0_RESET_ACK)
 		vfe->isr_ops.reset_ack(vfe);
-- 
2.28.0.220.ged08abb693-goog

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

[Nicolas Boichat]

We added a config option CONFIG_TRACING_ALLOW_PRINTK to make sure
that no extra trace_printk gets added to the kernel, let's use
that in this driver to guard the trace_printk call.

Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
---

Technically, we could only initialize the trace_printk buffers
when the print env is switched, to avoid the build error and
unconditional boot-time warning, but I assume this printing
framework will eventually get removed when the driver moves out
of staging?

 drivers/staging/media/atomisp/pci/atomisp_compat_css20.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
index 1b2b2c68025b4cc..020519dca1324ab 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
@@ -165,11 +165,13 @@ static int atomisp_css2_dbg_print(const char *fmt, va_list args)
 	return 0;
 }
 
+#ifdef CONFIG_TRACING_ALLOW_PRINTK
 static int atomisp_css2_dbg_ftrace_print(const char *fmt, va_list args)
 {
 	ftrace_vprintk(fmt, args);
 	return 0;
 }
+#endif
 
 static int atomisp_css2_err_print(const char *fmt, va_list args)
 {
@@ -865,9 +867,11 @@ static inline int __set_css_print_env(struct atomisp_device *isp, int opt)
 
 	if (opt == 0)
 		isp->css_env.isp_css_env.print_env.debug_print = NULL;
+#ifdef CONFIG_TRACING_ALLOW_PRINTK
 	else if (opt == 1)
 		isp->css_env.isp_css_env.print_env.debug_print =
 		    atomisp_css2_dbg_ftrace_print;
+#endif
 	else if (opt == 2)
 		isp->css_env.isp_css_env.print_env.debug_print =
 		    atomisp_css2_dbg_print;
-- 
2.28.0.220.ged08abb693-goog

Re: [PATCH v4 1/3, RESEND] media: camss: vfe: Use trace_printk for debugging only

[Steven Rostedt]

On Thu, 20 Aug 2020 17:14:10 +0800
Nicolas Boichat <drinkcat@chromium.org> wrote:

> trace_printk should not be used in production code. Since
> tracing interrupts is presumably latency sensitive, pr_dbg is
> not appropriate, so guard the call with a preprocessor symbol
> that can be defined for debugging purpose.
> 
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> ---
> 
> (resending this patch as part of the whole series, since we need a new
> patch 3/3 now).
> 
>  drivers/media/platform/qcom/camss/camss-vfe-4-1.c | 2 ++
>  drivers/media/platform/qcom/camss/camss-vfe-4-7.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
> index 174a36be6f5d866..0c57171fae4f9e9 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
> @@ -936,8 +936,10 @@ static irqreturn_t vfe_isr(int irq, void *dev)
>  
>  	vfe->ops->isr_read(vfe, &value0, &value1);
>  
> +#ifdef CAMSS_VFE_TRACE_IRQ
>  	trace_printk("VFE: status0 = 0x%08x, status1 = 0x%08x\n",
>  		     value0, value1);

Why are these not trace events?

The reason I have that ugly banner is to keep people from doing EXACTLY THIS!

trace_printk() is really easy to add, but it also is not configurable.
When a trace_printk() is in the code, it is always enabled (well, you
can turn trace_printk off, but that's an all or nothing. That is, by
turning trace_printk off, you turn off *all* trace_printks).

Instead, people should add trace events. This here is a perfect place
to have a trace event. You don't need to add #ifdef around trace events
because when not enabled, they are simply a nop. When enabled, the nop
is turned into a jump to the tracing code. It should not affect
performance. And as a trace event, you get a bunch of features with it
(filtering, histograms, etc).

-- Steve


> +#endif
>  
>  	if (value0 & VFE_0_IRQ_STATUS_0_RESET_ACK)
>  		vfe->isr_ops.reset_ack(vfe);
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
> index 0dca8bf9281e774..307675925e5c779 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
> @@ -1058,8 +1058,10 @@ static irqreturn_t vfe_isr(int irq, void *dev)
>  
>  	vfe->ops->isr_read(vfe, &value0, &value1);
>  
> +#ifdef CAMSS_VFE_TRACE_IRQ
>  	trace_printk("VFE: status0 = 0x%08x, status1 = 0x%08x\n",
>  		     value0, value1);
> +#endif
>  
>  	if (value0 & VFE_0_IRQ_STATUS_0_RESET_ACK)
>  		vfe->isr_ops.reset_ack(vfe);

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

[Steven Rostedt]

On Thu, 20 Aug 2020 17:14:12 +0800
Nicolas Boichat <drinkcat@chromium.org> wrote:

> Technically, we could only initialize the trace_printk buffers
> when the print env is switched, to avoid the build error and
> unconditional boot-time warning, but I assume this printing
> framework will eventually get removed when the driver moves out
> of staging?

Perhaps this should be converting into a trace event. Look at what bpf
did for their bpf_trace_printk().

The more I think about it, the less I like this series.

-- Steve

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

[Nicolas Boichat]

On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 20 Aug 2020 17:14:12 +0800
> Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> > Technically, we could only initialize the trace_printk buffers
> > when the print env is switched, to avoid the build error and
> > unconditional boot-time warning, but I assume this printing
> > framework will eventually get removed when the driver moves out
> > of staging?
>
> Perhaps this should be converting into a trace event. Look at what bpf
> did for their bpf_trace_printk().
>
> The more I think about it, the less I like this series.

To make it clear, the primary goal of this series is to get rid of
trace_printk sprinkled in the kernel by making sure some randconfig
builds fail. Since my v2, there already has been one more added (the
one that this patch removes), so I'd like to land 2/3 ASAP to prevent
even more from being added.

Looking at your reply on 1/3, I think we are aligned on that goal? Is
there some other approach you'd recommend?

Now, I'm not pretending my fixes are the best possible ones, but I
would much rather have the burden of converting to trace events on the
respective driver maintainers. (btw is there a short
documentation/tutorial that I could link to in these patches, to help
developers understand what is the recommended way now?)

Thanks,

>
> -- Steve

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

[Steven Rostedt]

On Fri, 21 Aug 2020 08:13:00 +0800
Nicolas Boichat <drinkcat@chromium.org> wrote:

> On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Thu, 20 Aug 2020 17:14:12 +0800
> > Nicolas Boichat <drinkcat@chromium.org> wrote:
> >  
> > > Technically, we could only initialize the trace_printk buffers
> > > when the print env is switched, to avoid the build error and
> > > unconditional boot-time warning, but I assume this printing
> > > framework will eventually get removed when the driver moves out
> > > of staging?  
> >
> > Perhaps this should be converting into a trace event. Look at what bpf
> > did for their bpf_trace_printk().
> >
> > The more I think about it, the less I like this series.  
> 
> To make it clear, the primary goal of this series is to get rid of
> trace_printk sprinkled in the kernel by making sure some randconfig
> builds fail. Since my v2, there already has been one more added (the
> one that this patch removes), so I'd like to land 2/3 ASAP to prevent
> even more from being added.
> 
> Looking at your reply on 1/3, I think we are aligned on that goal? Is
> there some other approach you'd recommend?
> 
> Now, I'm not pretending my fixes are the best possible ones, but I
> would much rather have the burden of converting to trace events on the
> respective driver maintainers. (btw is there a short
> documentation/tutorial that I could link to in these patches, to help
> developers understand what is the recommended way now?)
>

I like the goal, but I guess I never articulated the problem I have
with the methodology.

trace_printk() is meant to be a debugging tool. Something that people
can and do sprinkle all over the kernel to help them find a bug in
areas that are called quite often (where printk() is way too slow).

The last thing I want them to deal with is adding a trace_printk() with
their distro's config (or a config from someone that triggered the bug)
only to have the build to fail, because they also need to add a config
value.

I add to the Cc a few developers I know that use trace_printk() in this
fashion. I'd like to hear their view on having to add a config option
to make trace_printk work before they test a config that is sent to
them.

-- Steve

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

[Nicolas Boichat]

On Fri, Aug 21, 2020 at 8:36 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 21 Aug 2020 08:13:00 +0800
> Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> > On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > On Thu, 20 Aug 2020 17:14:12 +0800
> > > Nicolas Boichat <drinkcat@chromium.org> wrote:
> > >
> > > > Technically, we could only initialize the trace_printk buffers
> > > > when the print env is switched, to avoid the build error and
> > > > unconditional boot-time warning, but I assume this printing
> > > > framework will eventually get removed when the driver moves out
> > > > of staging?
> > >
> > > Perhaps this should be converting into a trace event. Look at what bpf
> > > did for their bpf_trace_printk().
> > >
> > > The more I think about it, the less I like this series.
> >
> > To make it clear, the primary goal of this series is to get rid of
> > trace_printk sprinkled in the kernel by making sure some randconfig
> > builds fail. Since my v2, there already has been one more added (the
> > one that this patch removes), so I'd like to land 2/3 ASAP to prevent
> > even more from being added.
> >
> > Looking at your reply on 1/3, I think we are aligned on that goal? Is
> > there some other approach you'd recommend?
> >
> > Now, I'm not pretending my fixes are the best possible ones, but I
> > would much rather have the burden of converting to trace events on the
> > respective driver maintainers. (btw is there a short
> > documentation/tutorial that I could link to in these patches, to help
> > developers understand what is the recommended way now?)
> >
>
> I like the goal, but I guess I never articulated the problem I have
> with the methodology.
>
> trace_printk() is meant to be a debugging tool. Something that people
> can and do sprinkle all over the kernel to help them find a bug in
> areas that are called quite often (where printk() is way too slow).
>
> The last thing I want them to deal with is adding a trace_printk() with
> their distro's config (or a config from someone that triggered the bug)
> only to have the build to fail, because they also need to add a config
> value.
>
> I add to the Cc a few developers I know that use trace_printk() in this
> fashion. I'd like to hear their view on having to add a config option
> to make trace_printk work before they test a config that is sent to
> them.

Gotcha, thanks. I have also used trace_printk in the past, as
uncommitted changes (and understand the usefulness ,-)). And in Chrome
OS team here, developers have also raised this concern: how do we make
the developer flow convenient so that we can add trace_printk to our
code for debugging, without having to flip back that config option,
and _yet_ make sure that no trace_printk ever makes it into our
production kernels. We have creative ways of making that work (portage
USE flags and stuff). But I'm not sure about other flows, and your
concern is totally valid...

Some other approaches/ideas:
 1. Filter all lkml messages that contain trace_printk. Already found
1 instance, and I can easily reply to those with a semi-canned answer,
if I remember to check that filter regularly (not sustainable in the
long run...).
 2. Integration into some kernel test robot? (I will not roll my own
for this ,-)) It may be a bit difficult as some debug config options
do enable trace_printk, and that's ok.
 3. In Chromium OS, I can add a unit test (i.e. something outside of
the normal kernel build system), but that'll only catch regressions
downstream (or when we happen to backport patches).

Down the line, #3 catches what I care about the most (Chromium OS
issues: we had production kernels for a few days/weeks showing that
splat on boot), but it'd be nice to have something upstream that
benefits everyone.

Thanks,

>
> -- Steve

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

[Steven Rostedt]

On Fri, 21 Aug 2020 09:39:19 +0800
Nicolas Boichat <drinkcat@chromium.org> wrote:

> On Fri, Aug 21, 2020 at 8:36 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Fri, 21 Aug 2020 08:13:00 +0800
> > Nicolas Boichat <drinkcat@chromium.org> wrote:
> >  
> > > On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt <rostedt@goodmis.org> wrote:  
> > > >
> > > > On Thu, 20 Aug 2020 17:14:12 +0800
> > > > Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > >  
> > > > > Technically, we could only initialize the trace_printk buffers
> > > > > when the print env is switched, to avoid the build error and
> > > > > unconditional boot-time warning, but I assume this printing
> > > > > framework will eventually get removed when the driver moves out
> > > > > of staging?  
> > > >
> > > > Perhaps this should be converting into a trace event. Look at what bpf
> > > > did for their bpf_trace_printk().
> > > >
> > > > The more I think about it, the less I like this series.  
> > >
> > > To make it clear, the primary goal of this series is to get rid of
> > > trace_printk sprinkled in the kernel by making sure some randconfig
> > > builds fail. Since my v2, there already has been one more added (the
> > > one that this patch removes), so I'd like to land 2/3 ASAP to prevent
> > > even more from being added.
> > >
> > > Looking at your reply on 1/3, I think we are aligned on that goal? Is
> > > there some other approach you'd recommend?
> > >
> > > Now, I'm not pretending my fixes are the best possible ones, but I
> > > would much rather have the burden of converting to trace events on the
> > > respective driver maintainers. (btw is there a short
> > > documentation/tutorial that I could link to in these patches, to help
> > > developers understand what is the recommended way now?)
> > >  
> >
> > I like the goal, but I guess I never articulated the problem I have
> > with the methodology.
> >
> > trace_printk() is meant to be a debugging tool. Something that people
> > can and do sprinkle all over the kernel to help them find a bug in
> > areas that are called quite often (where printk() is way too slow).
> >
> > The last thing I want them to deal with is adding a trace_printk() with
> > their distro's config (or a config from someone that triggered the bug)
> > only to have the build to fail, because they also need to add a config
> > value.
> >
> > I add to the Cc a few developers I know that use trace_printk() in this
> > fashion. I'd like to hear their view on having to add a config option
> > to make trace_printk work before they test a config that is sent to
> > them.  
> 
> Gotcha, thanks. I have also used trace_printk in the past, as
> uncommitted changes (and understand the usefulness ,-)). And in Chrome
> OS team here, developers have also raised this concern: how do we make
> the developer flow convenient so that we can add trace_printk to our
> code for debugging, without having to flip back that config option,
> and _yet_ make sure that no trace_printk ever makes it into our
> production kernels. We have creative ways of making that work (portage
> USE flags and stuff). But I'm not sure about other flows, and your
> concern is totally valid...
> 
> Some other approaches/ideas:
>  1. Filter all lkml messages that contain trace_printk. Already found
> 1 instance, and I can easily reply to those with a semi-canned answer,
> if I remember to check that filter regularly (not sustainable in the
> long run...).

Added Joe Perches to the thread.

We can update checkpatch.pl to complain about a trace_printk() that it
finds in the added code.

>  2. Integration into some kernel test robot? (I will not roll my own
> for this ,-)) It may be a bit difficult as some debug config options
> do enable trace_printk, and that's ok.
>  3. In Chromium OS, I can add a unit test (i.e. something outside of
> the normal kernel build system), but that'll only catch regressions
> downstream (or when we happen to backport patches).
> 
> Down the line, #3 catches what I care about the most (Chromium OS
> issues: we had production kernels for a few days/weeks showing that
> splat on boot), but it'd be nice to have something upstream that
> benefits everyone.
> 

What about an opposite config. That is, not have a config to enable it.
But one to disable it. If it is disabled and a trace_printk is found,
it will fail the build. This way your builds will not allow your kernel
to get out the door with one.

#ifdef CONFIG_DISABLE_TRACE_PRINTK
#define trace_printk	__this_function_is_disabled
#endif

?

-- Steve

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

[Joe Perches]

On Thu, 2020-08-20 at 21:57 -0400, Steven Rostedt wrote:
> On Fri, 21 Aug 2020 09:39:19 +0800
> Nicolas Boichat <drinkcat@chromium.org> wrote:
[]
> > Some other approaches/ideas:
> >  1. Filter all lkml messages that contain trace_printk. Already found
> > 1 instance, and I can easily reply to those with a semi-canned answer,
> > if I remember to check that filter regularly (not sustainable in the
> > long run...).
> 
> Added Joe Perches to the thread.
> 
> We can update checkpatch.pl to complain about a trace_printk() that it
> finds in the added code.

Why?

I don't see much value in a trace_printk checkpatch warning.
tracing is still dependent on CONFIG_TRACING otherwise
trace_printk is an if (0)

ELI5 please.

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

[Nicolas Boichat]

On Fri, Aug 21, 2020 at 9:57 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 21 Aug 2020 09:39:19 +0800
> Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> > On Fri, Aug 21, 2020 at 8:36 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > On Fri, 21 Aug 2020 08:13:00 +0800
> > > Nicolas Boichat <drinkcat@chromium.org> wrote:
> > >
> > > > On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > > > >
> > > > > On Thu, 20 Aug 2020 17:14:12 +0800
> > > > > Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > > >
> > > > > > Technically, we could only initialize the trace_printk buffers
> > > > > > when the print env is switched, to avoid the build error and
> > > > > > unconditional boot-time warning, but I assume this printing
> > > > > > framework will eventually get removed when the driver moves out
> > > > > > of staging?
> > > > >
> > > > > Perhaps this should be converting into a trace event. Look at what bpf
> > > > > did for their bpf_trace_printk().
> > > > >
> > > > > The more I think about it, the less I like this series.
> > > >
> > > > To make it clear, the primary goal of this series is to get rid of
> > > > trace_printk sprinkled in the kernel by making sure some randconfig
> > > > builds fail. Since my v2, there already has been one more added (the
> > > > one that this patch removes), so I'd like to land 2/3 ASAP to prevent
> > > > even more from being added.
> > > >
> > > > Looking at your reply on 1/3, I think we are aligned on that goal? Is
> > > > there some other approach you'd recommend?
> > > >
> > > > Now, I'm not pretending my fixes are the best possible ones, but I
> > > > would much rather have the burden of converting to trace events on the
> > > > respective driver maintainers. (btw is there a short
> > > > documentation/tutorial that I could link to in these patches, to help
> > > > developers understand what is the recommended way now?)
> > > >
> > >
> > > I like the goal, but I guess I never articulated the problem I have
> > > with the methodology.
> > >
> > > trace_printk() is meant to be a debugging tool. Something that people
> > > can and do sprinkle all over the kernel to help them find a bug in
> > > areas that are called quite often (where printk() is way too slow).
> > >
> > > The last thing I want them to deal with is adding a trace_printk() with
> > > their distro's config (or a config from someone that triggered the bug)
> > > only to have the build to fail, because they also need to add a config
> > > value.
> > >
> > > I add to the Cc a few developers I know that use trace_printk() in this
> > > fashion. I'd like to hear their view on having to add a config option
> > > to make trace_printk work before they test a config that is sent to
> > > them.
> >
> > Gotcha, thanks. I have also used trace_printk in the past, as
> > uncommitted changes (and understand the usefulness ,-)). And in Chrome
> > OS team here, developers have also raised this concern: how do we make
> > the developer flow convenient so that we can add trace_printk to our
> > code for debugging, without having to flip back that config option,
> > and _yet_ make sure that no trace_printk ever makes it into our
> > production kernels. We have creative ways of making that work (portage
> > USE flags and stuff). But I'm not sure about other flows, and your
> > concern is totally valid...
> >
> > Some other approaches/ideas:
> >  1. Filter all lkml messages that contain trace_printk. Already found
> > 1 instance, and I can easily reply to those with a semi-canned answer,
> > if I remember to check that filter regularly (not sustainable in the
> > long run...).
>
> Added Joe Perches to the thread.
>
> We can update checkpatch.pl to complain about a trace_printk() that it
> finds in the added code.

Oh, that's a good and simple idea.

>
> >  2. Integration into some kernel test robot? (I will not roll my own
> > for this ,-)) It may be a bit difficult as some debug config options
> > do enable trace_printk, and that's ok.
> >  3. In Chromium OS, I can add a unit test (i.e. something outside of
> > the normal kernel build system), but that'll only catch regressions
> > downstream (or when we happen to backport patches).
> >
> > Down the line, #3 catches what I care about the most (Chromium OS
> > issues: we had production kernels for a few days/weeks showing that
> > splat on boot), but it'd be nice to have something upstream that
> > benefits everyone.
> >
>
> What about an opposite config. That is, not have a config to enable it.
> But one to disable it. If it is disabled and a trace_printk is found,
> it will fail the build. This way your builds will not allow your kernel
> to get out the door with one.
>
> #ifdef CONFIG_DISABLE_TRACE_PRINTK
> #define trace_printk    __this_function_is_disabled
> #endif

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?

>
> ?
>
> -- Steve

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

[Nicolas Boichat]

On Fri, Aug 21, 2020 at 10:36 AM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2020-08-20 at 21:57 -0400, Steven Rostedt wrote:
> > On Fri, 21 Aug 2020 09:39:19 +0800
> > Nicolas Boichat <drinkcat@chromium.org> wrote:
> []
> > > Some other approaches/ideas:
> > >  1. Filter all lkml messages that contain trace_printk. Already found
> > > 1 instance, and I can easily reply to those with a semi-canned answer,
> > > if I remember to check that filter regularly (not sustainable in the
> > > long run...).
> >
> > Added Joe Perches to the thread.
> >
> > We can update checkpatch.pl to complain about a trace_printk() that it
> > finds in the added code.
>
> Why?
>
> I don't see much value in a trace_printk checkpatch warning.
> tracing is still dependent on CONFIG_TRACING otherwise
> trace_printk is an if (0)
>
> ELI5 please.

This is my "new" canned answer to this:

Please do not use trace_printk in production code [1,2], it is only
meant for debug use. Consider using trace events, or dev_dbg.
[1] https://elixir.bootlin.com/linux/v5.8/source/kernel/trace/trace.c#L3158
[2] https://elixir.bootlin.com/linux/v5.8/source/include/linux/kernel.h#L766

I also had arguments in patch 2/3 notes:

There's at least 3 reasons that I can come up with:
 1. trace_printk introduces some overhead. [some users, e.g.
Android/Chrome OS, want CONFIG_TRACING but _not_ that extra overhead]
 2. If the kernel keeps adding always-enabled trace_printk, it will be
much harder for developers to make use of trace_printk for debugging.
 3. People may assume that trace_printk is for debugging only, and may
accidentally output sensitive data (theoretical at this stage).

(we'll need to summarize that somehow if we want to add to checkpatch.pl)

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

[Steven Rostedt]

On Thu, 20 Aug 2020 19:36:19 -0700
Joe Perches <joe@perches.com> wrote:

> On Thu, 2020-08-20 at 21:57 -0400, Steven Rostedt wrote:
> > On Fri, 21 Aug 2020 09:39:19 +0800
> > Nicolas Boichat <drinkcat@chromium.org> wrote:  
> []
> > > Some other approaches/ideas:
> > >  1. Filter all lkml messages that contain trace_printk. Already found
> > > 1 instance, and I can easily reply to those with a semi-canned answer,
> > > if I remember to check that filter regularly (not sustainable in the
> > > long run...).  
> > 
> > Added Joe Perches to the thread.
> > 
> > We can update checkpatch.pl to complain about a trace_printk() that it
> > finds in the added code.  
> 
> Why?
> 
> I don't see much value in a trace_printk checkpatch warning.
> tracing is still dependent on CONFIG_TRACING otherwise
> trace_printk is an if (0)
> 
> ELI5 please.
> 

Because no production code should contain trace_printk(). It should be
deleted before going to Linus. If you have trace_printk() in your code,
you will be greeted by the following banner in your dmesg:

 **********************************************************
 **   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **
 **                                                      **
 ** trace_printk() being used. Allocating extra memory.  **
 **                                                      **
 ** This means that this is a DEBUG kernel and it is     **
 ** unsafe for production use.                           **
 **                                                      **
 ** If you see this message and you are not debugging    **
 ** the kernel, report this immediately to your vendor!  **
 **                                                      **
 **   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **
 **********************************************************

-- Steve

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

[Joe Perches]

On Fri, 2020-08-21 at 10:42 +0800, Nicolas Boichat wrote:
> On Fri, Aug 21, 2020 at 10:36 AM Joe Perches <joe@perches.com> wrote:
> > On Thu, 2020-08-20 at 21:57 -0400, Steven Rostedt wrote:
> > > On Fri, 21 Aug 2020 09:39:19 +0800
> > > Nicolas Boichat <drinkcat@chromium.org> wrote:
> > []
> > > > Some other approaches/ideas:
> > > >  1. Filter all lkml messages that contain trace_printk. Already found
> > > > 1 instance, and I can easily reply to those with a semi-canned answer,
> > > > if I remember to check that filter regularly (not sustainable in the
> > > > long run...).
> > > 
> > > Added Joe Perches to the thread.
> > > 
> > > We can update checkpatch.pl to complain about a trace_printk() that it
> > > finds in the added code.
> > 
> > Why?
> > 
> > I don't see much value in a trace_printk checkpatch warning.
> > tracing is still dependent on CONFIG_TRACING otherwise
> > trace_printk is an if (0)
> > 
> > ELI5 please.
> 
> This is my "new" canned answer to this:
> 
> Please do not use trace_printk in production code [1,2], it is only
> meant for debug use. Consider using trace events, or dev_dbg.
> [1] https://elixir.bootlin.com/linux/v5.8/source/kernel/trace/trace.c#L3158
> [2] https://elixir.bootlin.com/linux/v5.8/source/include/linux/kernel.h#L766
> 
> I also had arguments in patch 2/3 notes:
> 
> There's at least 3 reasons that I can come up with:
>  1. trace_printk introduces some overhead. [some users, e.g.
> Android/Chrome OS, want CONFIG_TRACING but _not_ that extra overhead]
>  2. If the kernel keeps adding always-enabled trace_printk, it will be
> much harder for developers to make use of trace_printk for debugging.
>  3. People may assume that trace_printk is for debugging only, and may
> accidentally output sensitive data (theoretical at this stage).

Perhaps make trace_printk dependent on #define DEBUG?

Something like:
---
 include/linux/kernel.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 500def620d8f..6ca8f958df73 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -717,6 +717,7 @@ do {									\
  * let gcc optimize the rest.
  */
 
+#ifdef DEBUG
 #define trace_printk(fmt, ...)				\
 do {							\
 	char _______STR[] = __stringify((__VA_ARGS__));	\
@@ -725,6 +726,12 @@ do {							\
 	else						\
 		trace_puts(fmt);			\
 } while (0)
+#else
+#define trace_printk(fmt, ...)						\
+do {									\
+	__trace_printk_check_format(fmt, ##args);			\
+} while (0)
+#endif
 
 #define do_trace_printk(fmt, args...)					\
 do {									\

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

[Steven Rostedt]

On Fri, 21 Aug 2020 10:39:02 +0800
Nicolas Boichat <drinkcat@chromium.org> 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.

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

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

[Steven Rostedt]

On Thu, 20 Aug 2020 19:49:59 -0700
Joe Perches <joe@perches.com> wrote:

> Perhaps make trace_printk dependent on #define DEBUG?

This is basically what Nicolas's patch series does in this very patch!

And no, I hate it. We are currently discussing ways of not having to
modify the config in order to allow trace_printk() to be used.

We don't want to burden the developer to take a config, add a bunch of
trace_printks() and find that it's compiled out!

Thus, this is a NAK.

-- Steve


> 
> Something like:
> ---
>  include/linux/kernel.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 500def620d8f..6ca8f958df73 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -717,6 +717,7 @@ do {									\
>   * let gcc optimize the rest.
>   */
>  
> +#ifdef DEBUG
>  #define trace_printk(fmt, ...)				\
>  do {							\
>  	char _______STR[] = __stringify((__VA_ARGS__));	\
> @@ -725,6 +726,12 @@ do {							\
>  	else						\
>  		trace_puts(fmt);			\
>  } while (0)
> +#else
> +#define trace_printk(fmt, ...)						\
> +do {									\
> +	__trace_printk_check_format(fmt, ##args);			\
> +} while (0)
> +#endif
>  
>  #define do_trace_printk(fmt, args...)					\
>  do {									\
> 

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

[Steven Rostedt]

On Thu, 20 Aug 2020 23:04:59 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 20 Aug 2020 19:49:59 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > Perhaps make trace_printk dependent on #define DEBUG?  
> 
> This is basically what Nicolas's patch series does in this very patch!
> 
> And no, I hate it. We are currently discussing ways of not having to
> modify the config in order to allow trace_printk() to be used.
> 
> We don't want to burden the developer to take a config, add a bunch of
> trace_printks() and find that it's compiled out!
>

This also breaks another use case. You may be working on a module for a
production kernel. It is fine to include trace_printk() in your module,
and load it on the production kernel. You will get that banner when you
load your module, but that's OK because it is still under development.

But something like this change will prevent that from happening.

-- Steve

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

[David Laight]

From: Steven Rostedt
> Sent: 21 August 2020 01:36
> On Fri, 21 Aug 2020 08:13:00 +0800
> Nicolas Boichat <drinkcat@chromium.org> wrote:
> 
> > On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > On Thu, 20 Aug 2020 17:14:12 +0800
> > > Nicolas Boichat <drinkcat@chromium.org> wrote:
> > >
> > > > Technically, we could only initialize the trace_printk buffers
> > > > when the print env is switched, to avoid the build error and
> > > > unconditional boot-time warning, but I assume this printing
> > > > framework will eventually get removed when the driver moves out
> > > > of staging?
> > >
> > > Perhaps this should be converting into a trace event. Look at what bpf
> > > did for their bpf_trace_printk().
> > >
> > > The more I think about it, the less I like this series.
> >
> > To make it clear, the primary goal of this series is to get rid of
> > trace_printk sprinkled in the kernel by making sure some randconfig
> > builds fail. Since my v2, there already has been one more added (the
> > one that this patch removes), so I'd like to land 2/3 ASAP to prevent
> > even more from being added.
> >
> > Looking at your reply on 1/3, I think we are aligned on that goal? Is
> > there some other approach you'd recommend?
> >
> > Now, I'm not pretending my fixes are the best possible ones, but I
> > would much rather have the burden of converting to trace events on the
> > respective driver maintainers. (btw is there a short
> > documentation/tutorial that I could link to in these patches, to help
> > developers understand what is the recommended way now?)
> >
> 
> I like the goal, but I guess I never articulated the problem I have
> with the methodology.
> 
> trace_printk() is meant to be a debugging tool. Something that people
> can and do sprinkle all over the kernel to help them find a bug in
> areas that are called quite often (where printk() is way too slow).
> 
> The last thing I want them to deal with is adding a trace_printk() with
> their distro's config (or a config from someone that triggered the bug)
> only to have the build to fail, because they also need to add a config
> value.
> 
> I add to the Cc a few developers I know that use trace_printk() in this
> fashion. I'd like to hear their view on having to add a config option
> to make trace_printk work before they test a config that is sent to
> them.

Is it worth having three compile-time options:
1) trace_printk() ignored.
2) trace_printk() enabled.
3) trace_printk() generates a compile time error.

Normal kernel builds would ignore calls.
Either a config option or a module option (etc) would enable it.
A config option that 'rand-config' builds would turn on would
generate compile-time errors.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Nicolas Boichat]

On Fri, Aug 21, 2020 at 4:48 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: Steven Rostedt
> > Sent: 21 August 2020 01:36
> > On Fri, 21 Aug 2020 08:13:00 +0800
> > Nicolas Boichat <drinkcat@chromium.org> wrote:
> >
> > > On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > > >
> > > > On Thu, 20 Aug 2020 17:14:12 +0800
> > > > Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > >
> > > > > Technically, we could only initialize the trace_printk buffers
> > > > > when the print env is switched, to avoid the build error and
> > > > > unconditional boot-time warning, but I assume this printing
> > > > > framework will eventually get removed when the driver moves out
> > > > > of staging?
> > > >
> > > > Perhaps this should be converting into a trace event. Look at what bpf
> > > > did for their bpf_trace_printk().
> > > >
> > > > The more I think about it, the less I like this series.
> > >
> > > To make it clear, the primary goal of this series is to get rid of
> > > trace_printk sprinkled in the kernel by making sure some randconfig
> > > builds fail. Since my v2, there already has been one more added (the
> > > one that this patch removes), so I'd like to land 2/3 ASAP to prevent
> > > even more from being added.
> > >
> > > Looking at your reply on 1/3, I think we are aligned on that goal? Is
> > > there some other approach you'd recommend?
> > >
> > > Now, I'm not pretending my fixes are the best possible ones, but I
> > > would much rather have the burden of converting to trace events on the
> > > respective driver maintainers. (btw is there a short
> > > documentation/tutorial that I could link to in these patches, to help
> > > developers understand what is the recommended way now?)
> > >
> >
> > I like the goal, but I guess I never articulated the problem I have
> > with the methodology.
> >
> > trace_printk() is meant to be a debugging tool. Something that people
> > can and do sprinkle all over the kernel to help them find a bug in
> > areas that are called quite often (where printk() is way too slow).
> >
> > The last thing I want them to deal with is adding a trace_printk() with
> > their distro's config (or a config from someone that triggered the bug)
> > only to have the build to fail, because they also need to add a config
> > value.
> >
> > I add to the Cc a few developers I know that use trace_printk() in this
> > fashion. I'd like to hear their view on having to add a config option
> > to make trace_printk work before they test a config that is sent to
> > them.
>
> Is it worth having three compile-time options:
> 1) trace_printk() ignored.

CONFIG_TRACE=n (now)

> 2) trace_printk() enabled.

CONFIG_TRACE=y (now)

> 3) trace_printk() generates a compile time error.

CONFIG_TRACE=y and CONFIG_TRACING_ALLOW_PRINTK=n (my patch)

>
> Normal kernel builds would ignore calls.
> Either a config option or a module option (etc) would enable it.
> A config option that 'rand-config' builds would turn on would
> generate compile-time errors.

Yes, a config option is exactly what my patch 2/2 does. And see
Steven's argument.


>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>

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

[David Laight]

From: Nicolas Boichat
> Sent: 21 August 2020 11:28
> 
> On Fri, Aug 21, 2020 at 4:48 PM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Steven Rostedt
> > > Sent: 21 August 2020 01:36
> > > On Fri, 21 Aug 2020 08:13:00 +0800
> > > Nicolas Boichat <drinkcat@chromium.org> wrote:
> > >
> > > > On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > > > >
> > > > > On Thu, 20 Aug 2020 17:14:12 +0800
> > > > > Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > > >
> > > > > > Technically, we could only initialize the trace_printk buffers
> > > > > > when the print env is switched, to avoid the build error and
> > > > > > unconditional boot-time warning, but I assume this printing
> > > > > > framework will eventually get removed when the driver moves out
> > > > > > of staging?
> > > > >
> > > > > Perhaps this should be converting into a trace event. Look at what bpf
> > > > > did for their bpf_trace_printk().
> > > > >
> > > > > The more I think about it, the less I like this series.
> > > >
> > > > To make it clear, the primary goal of this series is to get rid of
> > > > trace_printk sprinkled in the kernel by making sure some randconfig
> > > > builds fail. Since my v2, there already has been one more added (the
> > > > one that this patch removes), so I'd like to land 2/3 ASAP to prevent
> > > > even more from being added.
> > > >
> > > > Looking at your reply on 1/3, I think we are aligned on that goal? Is
> > > > there some other approach you'd recommend?
> > > >
> > > > Now, I'm not pretending my fixes are the best possible ones, but I
> > > > would much rather have the burden of converting to trace events on the
> > > > respective driver maintainers. (btw is there a short
> > > > documentation/tutorial that I could link to in these patches, to help
> > > > developers understand what is the recommended way now?)
> > > >
> > >
> > > I like the goal, but I guess I never articulated the problem I have
> > > with the methodology.
> > >
> > > trace_printk() is meant to be a debugging tool. Something that people
> > > can and do sprinkle all over the kernel to help them find a bug in
> > > areas that are called quite often (where printk() is way too slow).
> > >
> > > The last thing I want them to deal with is adding a trace_printk() with
> > > their distro's config (or a config from someone that triggered the bug)
> > > only to have the build to fail, because they also need to add a config
> > > value.
> > >
> > > I add to the Cc a few developers I know that use trace_printk() in this
> > > fashion. I'd like to hear their view on having to add a config option
> > > to make trace_printk work before they test a config that is sent to
> > > them.
> >
> > Is it worth having three compile-time options:
> > 1) trace_printk() ignored.
> 
> CONFIG_TRACE=n (now)
> 
> > 2) trace_printk() enabled.
> 
> CONFIG_TRACE=y (now)
> 
> > 3) trace_printk() generates a compile time error.
> 
> CONFIG_TRACE=y and CONFIG_TRACING_ALLOW_PRINTK=n (my patch)
> 
> >
> > Normal kernel builds would ignore calls.
> > Either a config option or a module option (etc) would enable it.
> > A config option that 'rand-config' builds would turn on would
> > generate compile-time errors.
> 
> Yes, a config option is exactly what my patch 2/2 does. And see
> Steven's argument.

But you were adding #ifs to you code to enable the traces.
That is just horrid.

What you want is CONFIG_HJHJVLKHCVKIYVKIIYVYKIYVLUCLUCL=y (default n)
that would only ever get set by a 'rand-config' build and would
never be tested in any source code.

You might also want a #define that can set temporarily
to enable traces in a specific file/module even though
CONFIG_TRACE=n.
(But rand-config builds would still fail if they enabled the
'special' option.)

	David.

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Nicolas Boichat]

On Fri, Aug 21, 2020 at 7:32 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: Nicolas Boichat
> > Sent: 21 August 2020 11:28
> >
> > On Fri, Aug 21, 2020 at 4:48 PM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Steven Rostedt
> > > > Sent: 21 August 2020 01:36
> > > > On Fri, 21 Aug 2020 08:13:00 +0800
> > > > Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > >
> > > > > On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > > > > >
> > > > > > On Thu, 20 Aug 2020 17:14:12 +0800
> > > > > > Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > > > >
> > > > > > > Technically, we could only initialize the trace_printk buffers
> > > > > > > when the print env is switched, to avoid the build error and
> > > > > > > unconditional boot-time warning, but I assume this printing
> > > > > > > framework will eventually get removed when the driver moves out
> > > > > > > of staging?
> > > > > >
> > > > > > Perhaps this should be converting into a trace event. Look at what bpf
> > > > > > did for their bpf_trace_printk().
> > > > > >
> > > > > > The more I think about it, the less I like this series.
> > > > >
> > > > > To make it clear, the primary goal of this series is to get rid of
> > > > > trace_printk sprinkled in the kernel by making sure some randconfig
> > > > > builds fail. Since my v2, there already has been one more added (the
> > > > > one that this patch removes), so I'd like to land 2/3 ASAP to prevent
> > > > > even more from being added.
> > > > >
> > > > > Looking at your reply on 1/3, I think we are aligned on that goal? Is
> > > > > there some other approach you'd recommend?
> > > > >
> > > > > Now, I'm not pretending my fixes are the best possible ones, but I
> > > > > would much rather have the burden of converting to trace events on the
> > > > > respective driver maintainers. (btw is there a short
> > > > > documentation/tutorial that I could link to in these patches, to help
> > > > > developers understand what is the recommended way now?)
> > > > >
> > > >
> > > > I like the goal, but I guess I never articulated the problem I have
> > > > with the methodology.
> > > >
> > > > trace_printk() is meant to be a debugging tool. Something that people
> > > > can and do sprinkle all over the kernel to help them find a bug in
> > > > areas that are called quite often (where printk() is way too slow).
> > > >
> > > > The last thing I want them to deal with is adding a trace_printk() with
> > > > their distro's config (or a config from someone that triggered the bug)
> > > > only to have the build to fail, because they also need to add a config
> > > > value.
> > > >
> > > > I add to the Cc a few developers I know that use trace_printk() in this
> > > > fashion. I'd like to hear their view on having to add a config option
> > > > to make trace_printk work before they test a config that is sent to
> > > > them.
> > >
> > > Is it worth having three compile-time options:
> > > 1) trace_printk() ignored.
> >
> > CONFIG_TRACE=n (now)
> >
> > > 2) trace_printk() enabled.
> >
> > CONFIG_TRACE=y (now)
> >
> > > 3) trace_printk() generates a compile time error.
> >
> > CONFIG_TRACE=y and CONFIG_TRACING_ALLOW_PRINTK=n (my patch)
> >
> > >
> > > Normal kernel builds would ignore calls.
> > > Either a config option or a module option (etc) would enable it.
> > > A config option that 'rand-config' builds would turn on would
> > > generate compile-time errors.
> >
> > Yes, a config option is exactly what my patch 2/2 does. And see
> > Steven's argument.
>
> But you were adding #ifs to you code to enable the traces.
> That is just horrid.

Yeah this patch 3/3 is not the best (if I understand what you mean),
1/3 type of #ifdef is actually fairly common in the kernel (an ifdef
that you can enable manually in the same file, _not_ a config option).

Steven's point is that both 1/3 and 3/3 should be replaced by trace
events anyway.

>
> What you want is CONFIG_HJHJVLKHCVKIYVKIIYVYKIYVLUCLUCL=y (default n)
> that would only ever get set by a 'rand-config' build and would
> never be tested in any source code.

What I have now is CONFIG_TRACING_ALLOW_PRINTK=n (default y). That's
the same thing?

Also, I'd want to enable this option on Chromium OS (i.e. set your
CONFIG_HJHJVLKHCVKIYVKIIYVYKIYVLUCLUCL=y, or my
CONFIG_TRACING_ALLOW_PRINTK=n), and it's likely some distros would
make that choice (because they'd also do not want to see that big
trace_printk warning on their production kernels).

And then you end up with Steven's point (developers inconvenience when
trying to add trace_printk using a config that somebody else provided
to them) ,-(

>
> You might also want a #define that can set temporarily
> to enable traces in a specific file/module even though
> CONFIG_TRACE=n.

I don't understand how traces are supposed to work with CONFIG_TRACE=n?


> (But rand-config builds would still fail if they enabled the
> 'special' option.)


>
>         David.
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

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

[David Laight]

From: Nicolas Boichat
> Sent: 21 August 2020 13:07
...
> > You might also want a #define that can set temporarily
> > to enable traces in a specific file/module even though
> > CONFIG_TRACE=n.
> 
> I don't understand how traces are supposed to work with CONFIG_TRACE=n?

Probably because I meant something different :-)

You want the kernel built so that there are no (expanded)
calls to trace_printf() but with support for modules that
contain them.

Then I can load a module into a distro kernel that
contains trace_printf() calls for debug testing.

Which is why I was suggesting a config option that
only rand-config builds would ever set that would
cause the calls to generate compile-time errors.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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@goodmis.org> wrote:
>
> On Fri, 21 Aug 2020 10:39:02 +0800
> Nicolas Boichat <drinkcat@chromium.org> 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)
>
>  /**

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

[Nicolas Boichat]

On Fri, Aug 21, 2020 at 8:18 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: Nicolas Boichat
> > Sent: 21 August 2020 13:07
> ...
> > > You might also want a #define that can set temporarily
> > > to enable traces in a specific file/module even though
> > > CONFIG_TRACE=n.
> >
> > I don't understand how traces are supposed to work with CONFIG_TRACE=n?
>
> Probably because I meant something different :-)
>
> You want the kernel built so that there are no (expanded)
> calls to trace_printf() but with support for modules that
> contain them.
>
> Then I can load a module into a distro kernel that
> contains trace_printf() calls for debug testing.

Gotcha. I think it already works this way ,-)

So if you have CONFIG_TRACE=y, but no trace_printk in your
vmlinux/kernel, no memory is used, and no warning splat
(https://elixir.bootlin.com/linux/v5.8/source/kernel/trace/trace.c#L3160)
is displayed. But then when you load a module with trace_printk, the
buffers are allocated and the warning splat is printed.

The magic is here:
https://elixir.bootlin.com/linux/v5.8/source/kernel/trace/trace_printk.c#L53

My option wouldn't really change that. I mean, if you have
CONFIG_TRACING_ALLOW_PRINTK=n when you compile your module, it'd fail
at build time, but if you set it to =y, your module could happily
build and load (with the big warning splat), no matter how you built
your kernel (I mean, you still need CONFIG_TRACE=y, but
CONFIG_TRACING_ALLOW_PRINTK doesn't matter).

> Which is why I was suggesting a config option that
> only rand-config builds would ever set that would
> cause the calls to generate compile-time errors.

I think I already answered that one above. We'd want that config
option enabled on Chrome OS and we're not a rand-config build (I mean,
we're a very carefully selected random config ,-P).

Thanks,

>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)