Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 9 Nov 2020 21:35:47 -0600
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Rishabh Bhatnagar <rishabhb@codeaurora.org>
Cc:     linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
        tsoni@codeaurora.org, psodagud@codeaurora.org,
        sidgup@codeaurora.org
Subject: Re: [PATCH 1/2] soc: qcom: Add tracepoints to mdt loader
Message-ID: <20201110033547.GD332990@builder.lan>
References: <1604971241-29000-1-git-send-email-rishabhb@codeaurora.org>
 <1604971241-29000-2-git-send-email-rishabhb@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1604971241-29000-2-git-send-email-rishabhb@codeaurora.org>
Precedence: bulk

On Mon 09 Nov 19:20 CST 2020, Rishabh Bhatnagar wrote:

> Add trace events to the mdt loader driver. These events
> can help us trace the region where we are loading the
> segments and the time it takes to initialize the image
> and setup the memory region.
> 
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
>  drivers/soc/qcom/mdt_loader.c     |  8 ++++++
>  include/trace/events/mdt_loader.h | 57 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>  create mode 100644 include/trace/events/mdt_loader.h
> 
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 24cd193..df69e23 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -17,6 +17,9 @@
>  #include <linux/slab.h>
>  #include <linux/soc/qcom/mdt_loader.h>
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/mdt_loader.h>
> +
>  static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
>  {
>  	if (phdr->p_type != PT_LOAD)
> @@ -169,6 +172,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  			goto out;
>  		}
>  
> +		trace_memory_setup("pas_init_image", fw_name);

I think it would be favourable if you pushed this into the PAS functions
in the scm driver instead.

>  		ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len);
>  
>  		kfree(metadata);
> @@ -196,8 +200,10 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  
>  	if (relocate) {
>  		if (pas_init) {
> +			trace_memory_setup("pas_mem_setup", fw_name);
>  			ret = qcom_scm_pas_mem_setup(pas_id, mem_phys,
>  						     max_addr - min_addr);
> +
>  			if (ret) {
>  				dev_err(dev, "unable to setup relocation\n");
>  				goto out;
> @@ -232,6 +238,8 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  
>  		ptr = mem_region + offset;
>  
> +		trace_regions(ptr, phdr->p_filesz, i);

"regions" is a very generic name for a trace event, perhaps
trace_qcom_mdt_load_segment() ?

I think it would be quite useful with a trace event indicating which
firmware mdt file (and what .bXX files) we're trying to load.

PS. ptr is a virtual address, there's no point in tracing this - we're
interested in "mem_reloc + offset".

Regards,
Bjorn

> +
>  		if (phdr->p_filesz && phdr->p_offset < fw->size) {
>  			/* Firmware is large enough to be non-split */
>  			if (phdr->p_offset + phdr->p_filesz > fw->size) {
> diff --git a/include/trace/events/mdt_loader.h b/include/trace/events/mdt_loader.h
> new file mode 100644
> index 0000000..6299f65
> --- /dev/null
> +++ b/include/trace/events/mdt_loader.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM mdt_loader
> +
> +#if !defined(_TRACE_MDT_LOADER_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_MDT_LOADER_H
> +
> +#include <linux/types.h>
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(memory_setup,
> +
> +	TP_PROTO(const char *event, char *fw_name),
> +
> +	TP_ARGS(event, fw_name),
> +
> +	TP_STRUCT__entry(
> +		__string(event, event)
> +		__string(fw_name, fw_name)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(event, event);
> +		__assign_str(fw_name, fw_name);
> +	),
> +
> +	TP_printk("doing %s for %s", __get_str(event), __get_str(fw_name))
> +);
> +
> +TRACE_EVENT(regions,
> +
> +	TP_PROTO(void *region_start, size_t region_size, int i),
> +
> +	TP_ARGS(region_start, region_size, i),
> +
> +	TP_STRUCT__entry(
> +		__field(void *, region_start)
> +		__field(size_t, region_size)
> +		__field(int, index)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->region_start = region_start;
> +		__entry->region_size = region_size;
> +		__entry->index = i;
> +	),
> +
> +	TP_printk("segment %d: region start=%pK size=%zx", __entry->index,
> +		  __entry->region_start, __entry->region_size)
> +);
> +
> +#endif
> +#include <trace/define_trace.h>
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>