From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f178.google.com (mail-pf1-f178.google.com [209.85.210.178]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C465972 for ; Thu, 28 Oct 2021 09:34:52 +0000 (UTC) Received: by mail-pf1-f178.google.com with SMTP id a26so5379809pfr.11 for ; Thu, 28 Oct 2021 02:34:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:subject:from:to:cc:date:in-reply-to:references :user-agent:mime-version:content-transfer-encoding; bh=NlNBOEtYToMvrTOUWg8aHj5KqGcd+olGDxMZcfpJiAA=; b=i3R15UgKwQolUrkgvcNE1Cm3gglyU/ZbrW8oOst2Wyfja6SJD05M7nzLWQoSKt9+Ib ILkrDBS11ZnwlTvgqpVH3OUF/Zj2dSvChuvRHRJm4pSzecdF45gv/Y+QEQk/K7eN4iRy 12F0Nz5ZHIlH5822ZQ+ApgCl9ExzHj6EegM5OleVToUzvLmfixdTXEb/sQwIz45u2K9c dbhtKLNHtai/QOkwrb4VMnP8mjBvy3WqTg/hdv3W/hLCHP9hATEW/8fiS5K04nHhi9IL GYB9Q+r26c2KMrGTz9lG/9N37izXAuCsE9fXXZrh+U1h10Udj5Y2ZpWnJ7ZuB3JHYHp1 I5ZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=NlNBOEtYToMvrTOUWg8aHj5KqGcd+olGDxMZcfpJiAA=; b=hbTwpmM2k0aEa/xVcevaVI0yfzZzA/ziX1EbB53GuTXD5WPL5jyyuhrznQph99X9FJ +SlBmA4LesPu4juuwYGrvBqXzKve/DMNNCHIhpDNuiBqwBV3SUVFj2fkta5ooQB4e93c rLcE5O7pntLlPgRsI66dpVEFK3Wlu6KWOmWpBbSt/YyfqqK764U0SmLnJT4Z2iwuE9uZ tsFiD/SaE0fLxYsUvfO6s64vAI1Dl2uF4O4LBNzfC8x9agKuf5RFtz4V4uXwtH9QBAyM v3liug+I3RE/CKOhwrYbUEne7XrOxB2kUVLntmMUdyB/fg1OF86xPK3mCJ7z/8yR9Gt2 WYsQ== X-Gm-Message-State: AOAM532ti3nwo0Qkq8cKSa29B1cftLAgSeprQYYMWqGOx1bDJrDSVI1L 2e7I33RueCRFp1I/8YLlZog= X-Google-Smtp-Source: ABdhPJzTUen2QmDRKdci9q3U8FprwcO4z5set3+OWzjXKVUKI8l4iowgQ3wdGxPbZ7ileGp6yN4UKQ== X-Received: by 2002:a05:6a00:2343:b0:44d:2e13:3edf with SMTP id j3-20020a056a00234300b0044d2e133edfmr3241071pfj.72.1635413692135; Thu, 28 Oct 2021 02:34:52 -0700 (PDT) Received: from ?IPv6:2400:4052:6980:3800:dba7:2b1f:3f26:a5ec? ([2400:4052:6980:3800:dba7:2b1f:3f26:a5ec]) by smtp.gmail.com with ESMTPSA id nn2sm7285796pjb.34.2021.10.28.02.34.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 28 Oct 2021 02:34:51 -0700 (PDT) Message-ID: Subject: Re: [PATCH 14/17] media: atomisp: pci: Remove remaining instance of call to trace_printk From: Tsuchiya Yuto To: Mauro Carvalho Chehab Cc: Mauro Carvalho Chehab , Sakari Ailus , Greg Kroah-Hartman , linux-media@vger.kernel.org, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org, Nable , Andy Shevchenko , Fabio Aiuto , "andrey.i.trufanov" , Patrik Gfeller Date: Thu, 28 Oct 2021 18:34:47 +0900 In-Reply-To: <20211026093224.6c7f7fbf@sal.lan> References: <20211017161958.44351-1-kitakar@gmail.com> <20211017161958.44351-15-kitakar@gmail.com> <20211026093224.6c7f7fbf@sal.lan> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.40.4 Precedence: bulk X-Mailing-List: linux-staging@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 7bit On Tue, 2021-10-26 at 09:32 +0100, Mauro Carvalho Chehab wrote: > Em Mon, 18 Oct 2021 01:19:54 +0900 > Tsuchiya Yuto 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 > > 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 > > --- > > .../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);