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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
> 
> (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);