linux-kernel - Re: [PATCH v4 1/3, RESEND] media: camss: vfe: Use trace

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200820102131.3cd3c08e@oasis.local.home>
Date:   Thu, 20 Aug 2020 10:21:31 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Nicolas Boichat <drinkcat@...omium.org>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Todor Tomov <todor.too@...il.com>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org
Subject: Re: [PATCH v4 1/3, RESEND] media: camss: vfe: Use trace_printk for
 debugging only

On Thu, 20 Aug 2020 17:14:10 +0800
Nicolas Boichat <drinkcat@...omium.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@...omium.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);