2020-11-10 01:22:43

by Rishabh Bhatnagar

[permalink] [raw]
Subject: [PATCH 1/2] soc: qcom: Add tracepoints to mdt loader

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 <[email protected]>
---
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);
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);
+
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


2020-11-10 03:38:54

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 1/2] soc: qcom: Add tracepoints to mdt loader

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 <[email protected]>
> ---
> 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
>