Re: [PATCH 14/17] media: atomisp: pci: Remove remaining instance of call to trace_printk

Tsuchiya Yuto
2021-11-01T14:23:04Z

On Thu, 2021-10-28 at 12:42 +0100, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Oct 2021 18:34:47 +0900
> Tsuchiya Yuto <kitakar@gmail.com> escreveu:
>
> > <Adding back people/list to Cc>
> >
> > On Tue, 2021-10-26 at 09:32 +0100, Mauro Carvalho Chehab wrote:
> > > Em Mon, 18 Oct 2021 01:19:54 +0900
> > > Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> > >
> > > > (patch based on intel-aero kernel patch:
> > > > https://github.com/intel-aero/meta-intel-aero-base/commit/26fc9fe5030b63bc9dcf0b5f32981948911ca272)
> > > >
> > > > Here is the original commit message from the aforementioned patch:
> > > >
> > > > From 26fc9fe5030b63bc9dcf0b5f32981948911ca272 Mon Sep 17 00:00:00 2001
> > > > From: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > Date: Fri, 7 Jul 2017 14:23:53 -0700
> > > > Subject: [PATCH] linux-yocto: Remove remaining instance of call to
> > > > trace_printk
> > > >
> > > > It's not sufficient to leave trace_printk() out of "normal call chains" since
> > > > the way trace infrastructure works is that it will allocate the trace_printk
> > > > buffers if the symbol is there (by using a separate section for the function
> > > > and checking if __start_* and __stop_* symbols are different.
> > > >
> > > > Therefore, even if the default value for the param tells the module to use
> > > > printk(), just the fact that it can be changed to trace_printk() means the
> > > > initialization code will be called.
> > > >
> > > > The trace_printk() was replaced by pr_info() on commit 3d81099c75a6
> > > > ("media: atomisp: Replace trace_printk by pr_info") for the upstreamed
> > > > atomisp, too. However, as the aforementioned commit message says, there
> > > > is still a remaining instance. This causes the "trace_printk() being
> > > > used" kernel warning message to still appear on the first driver load.
> > > >
> > > > Based on the aforementioned patch, this patch removes the call to
> > > > ftrace_vprintk(). This removes that kernel warning.
> > > >
> > > > In addition to this, this patch also removes the following now unused
> > > > things:
> > > >
> > > > - now empty atomisp_css2_dbg_ftrace_print()
> > > > - trace_printk option from dbg_func kernel parameter
> > >
> > > This patch is incomplete. I mean, if the idea is to drop support for
> > > trace, dbg_func parameter becomes obsolete, and a lot of code can be
> > > dropped/cleaned.
> >
> > I left __set_css_print_env() and dbg_func module parameter intentionally
> > because there is still the option "non", which means do not use any
> > function to log. Yes, I don't know if it's really useful since the module
> > parameter dbg_level=0 (this is the default value) means no debug output.
> >
> > However, they (those who originally wrote atomisp) added the option "non"
> > anyway. So, I thought there is a use case for this.
> >
> > If everyone thinks it's useless, I can additionally remove them. In this
> > case, indeed a lot of code may be additionally removed (I haven't checked
> > yet).
> >
> > > That's said, I don't see a good reason to drop it, at least while
> > > this driver is not 100%.
> >
> > This driver is not 100% yet, but at least it can capture images now
> > (at least on ISP2401). So, I thought this is the right time to remove
> > it when I wrote this patch.
> >
> > But yes, we don't necessarily have to drop it. The other idea is hide
> > it by default using a macro, and still can be enabled by manual edit.
> > Here is the example from drivers/usb/dwc2/core.h:
> >
> > 49-/*
> > 50- * Suggested defines for tracers:
> > 51- * - no_printk: Disable tracing
> > 52- * - pr_info: Print this info to the console
> > 53- * - trace_printk: Print this info to trace buffer (good for verbose logging)
> > 54- */
> > 55-
> > 56:#define DWC2_TRACE_SCHEDULER no_printk
> > 57:#define DWC2_TRACE_SCHEDULER_VB no_printk
> > 58-
> > 59-/* Detailed scheduler tracing, but won't overwhelm console */
> > 60-#define dwc2_sch_dbg(hsotg, fmt, ...) \
> > 61: DWC2_TRACE_SCHEDULER(pr_fmt("%s: SCH: " fmt), \
> > 62- dev_name(hsotg->dev), ##__VA_ARGS__)
> > 63-
> > 64-/* Verbose scheduler tracing */
> > 65-#define dwc2_sch_vdbg(hsotg, fmt, ...) \
> > 66: DWC2_TRACE_SCHEDULER_VB(pr_fmt("%s: SCH: " fmt), \
> > 67- dev_name(hsotg->dev), ##__VA_ARGS__)
> >
> > with using pr_info by default for atomisp (the parameter dbg_level is
> > 0 by default, so still no output by default). If this sounds OK, I'd
> > like to try this way.
>
> Let's just postpone this change. It sounds a little early to start
> cleaning up debug stuff. This can be done any time later, but let's
> first test it on our devices and ensure that it is stable enough.

Got it. First, let's focus on make atomisp work. But I still want to
address this a little earlier after we made it work. That kernel warning
message "trace_printk() being used" looks kinda scary...