| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c55212feff3212c27360f2c49f5c4023a95f5b7c.camel@gmail.com>
Date: Thu, 28 Oct 2021 18:34:47 +0900
From: Tsuchiya Yuto <kitakar@...il.com>
To: Mauro Carvalho Chehab <mchehab@...nel.org>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-media@...r.kernel.org, linux-staging@...ts.linux.dev,
linux-kernel@...r.kernel.org,
Nable <nable.maininbox@...glemail.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Fabio Aiuto <fabioaiuto83@...il.com>,
"andrey.i.trufanov" <andrey.i.trufanov@...il.com>,
Patrik Gfeller <patrik.gfeller@...il.com>
Subject: Re: [PATCH 14/17] media: atomisp: pci: Remove remaining instance of
call to trace_printk
<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@...il.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@...el.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.
> >
> > Signed-off-by: Tsuchiya Yuto <kitakar@...il.com>
> > ---
> > .../staging/media/atomisp/pci/atomisp_compat_css20.c | 10 ----------
> > drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 4 ++--
> > 2 files changed, 2 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
> > index 99a632f33d2d..d81d55c6f1fa 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
> > @@ -159,13 +159,6 @@ static void atomisp_css2_hw_load(hrt_address addr, void *to, uint32_t n)
> > spin_unlock_irqrestore(&mmio_lock, flags);
> > }
> >
> > -static int __printf(1, 0) atomisp_css2_dbg_ftrace_print(const char *fmt,
> > - va_list args)
> > -{
> > - ftrace_vprintk(fmt, args);
> > - return 0;
> > -}
> > -
> > static int __printf(1, 0) atomisp_vprintk(const char *fmt, va_list args)
> > {
> > vprintk(fmt, args);
> > @@ -860,9 +853,6 @@ 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;
> > else if (opt == 1)
> > - isp->css_env.isp_css_env.print_env.debug_print =
> > - atomisp_css2_dbg_ftrace_print;
> > - else if (opt == 2)
> > isp->css_env.isp_css_env.print_env.debug_print = atomisp_vprintk;
> > else
> > ret = -EINVAL;
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > index f5362554638e..720963156d24 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> > @@ -87,10 +87,10 @@ module_param(dbg_level, int, 0644);
> > MODULE_PARM_DESC(dbg_level, "debug message level (default:0)");
> >
> > /* log function switch */
> > -int dbg_func = 2;
> > +int dbg_func = 1;
> > module_param(dbg_func, int, 0644);
> > MODULE_PARM_DESC(dbg_func,
> > - "log function switch non/trace_printk/printk (default:printk)");
> > + "log function switch non/printk (default:printk)");
> >
> > int mipicsi_flag;
> > module_param(mipicsi_flag, int, 0644);
Powered by blists - more mailing lists