Re: [PATCH v5 2/3] remoteproc: qcom: Add capability to collect minidumps
Hi Siddharth,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.9-rc7 next-20200929]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Siddharth-Gupta/Introduce-mini-dump-support-for-remoteproc/20200925-075245
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
171d4ff79f965c1f164705ef0aaea102a6ad238b
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/0day-ci/linux/commit/cc292eb63f7fe2d3007889428362b160e8854e9e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Siddharth-Gupta/Introduce-mini-dump-support-for-remoteproc/20200925-075245
git checkout cc292eb63f7fe2d3007889428362b160e8854e9e
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
drivers/remoteproc/qcom_q6v5_pas.c: In function 'adsp_add_minidump_segments':
>> drivers/remoteproc/qcom_q6v5_pas.c:151:9: error: implicit declaration of
>> function '__raw_readq'; did you mean '__raw_readl'?
>> [-Werror=implicit-function-declaration]
151 |da = __raw_readq(_info->region_base_address);
| ^~~
| __raw_readl
cc1: some warnings being treated as errors
vim +151 drivers/remoteproc/qcom_q6v5_pas.c
123
124 static void adsp_add_minidump_segments(struct rproc *rproc, struct
md_ss_toc *minidump_ss)
125 {
126 struct md_ss_region __iomem *region_info;
127 int seg_cnt = 0, i;
128 void __iomem *ptr;
129 dma_addr_t da;
130 size_t size;
131 char *name;
132
133 seg_cnt = minidump_ss->ss_region_count;
134 ptr = ioremap((unsigned
long)minidump_ss->md_ss_smem_regions_baseptr,
135seg_cnt * sizeof(struct md_ss_region));
136
137 if (!ptr)
138 return;
139
140 region_info = ptr;
141
142 if (!list_empty(>dump_segments)) {
143 dev_info(>dev, "dump segment list already
populated\n");
144 goto unmap_iomem;
145 }
146
147 for (i = 0; i < seg_cnt; i++) {
148 if (__raw_readl(_info->md_valid) ==
MD_REGION_VALID) {
149 name = kmalloc(MAX_REGION_NAME_LENGTH,
GFP_KERNEL);
150 memcpy_fromio(name, region_info->name,
sizeof(region_info->name));
> 151 da =
> __raw_readq(_info->region_base_address);
152 size = __raw_readq(_info->region_size);
153 rproc_coredump_add_custom_segment(rproc, da,
size, NULL, name);
154 }
155 region_info++;
156 }
157
158 unmap_iomem:
159 iounmap(ptr);
160 }
161
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH v5 2/3] remoteproc: qcom: Add capability to collect minidumps
On 9/25/2020 9:10 PM, Bjorn Andersson wrote:
On Thu 24 Sep 16:51 PDT 2020, Siddharth Gupta wrote:
This patch adds support for collecting minidump in the event of remoteproc
crash. Parse the minidump table based on remoteproc's unique minidump-id,
read all memory regions from the remoteproc's minidump table entry and
expose the memory to userspace. The remoteproc platform driver can choose
to collect a full/mini dump by specifying the coredump op.
Signed-off-by: Rishabh Bhatnagar
Signed-off-by: Gurbir Arora
Signed-off-by: Siddharth Gupta
If the three of you authored this patch you should add Co-developed-by
for Rishabh and Gurbir as well.
Sure
---
drivers/remoteproc/qcom_minidump.h | 64 +
drivers/remoteproc/qcom_q6v5_pas.c | 106 -
drivers/remoteproc/remoteproc_coredump.c| 138
drivers/remoteproc/remoteproc_elf_helpers.h | 27 ++
include/linux/remoteproc.h | 1 +
5 files changed, 334 insertions(+), 2 deletions(-)
create mode 100644 drivers/remoteproc/qcom_minidump.h
diff --git a/drivers/remoteproc/qcom_minidump.h
b/drivers/remoteproc/qcom_minidump.h
new file mode 100644
index 000..437e030
--- /dev/null
+++ b/drivers/remoteproc/qcom_minidump.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __QCOM_MINIDUMP_H
+#define __QCOM_MINIDUMP_H
+
+#define MAX_NUM_OF_SS 10
+#define MAX_REGION_NAME_LENGTH 16
+#define SBL_MINIDUMP_SMEM_ID 602
+#define MD_REGION_VALID('V' << 24 | 'A' << 16 | 'L' << 8 | 'I'
<< 0)
+#define MD_SS_ENCR_DONE('D' << 24 | 'O' << 16 | 'N' << 8 | 'E'
<< 0)
+#define MD_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+
+/**
+ * md_ss_region - Minidump region
Valid kerneldoc should be:
* struct md_ss_region - Minidump region
+ * @name : Name of the region to be dumped
+ * @seq_num: : Use to differentiate regions with same name.
+ * @md_valid : This entry to be dumped (if set to 1)
+ * @region_base_address: Physical address of region to be dumped
+ * @region_size: Size of the region
+ */
+struct md_ss_region {
But why don't you call this struct minidump_region?
+ charname[MAX_REGION_NAME_LENGTH];
+ u32 seq_num;
Is this __le32 or __be32?
It is __le32, in that case should I update the structure to have all
__leX values (__le32 seq_num,
__le64 address, etc.) or do a conversion using leX_to_cpu while reading
the iomem region?
+ u32 md_valid;
"valid" would be sufficient
+ u64 region_base_address;
"address"
+ u64 region_size;
"size"
+};
+
+/**
+ * md_ss_toc: Subsystem's SMEM Table of content
+ * @md_ss_toc_init : Subsystem toc init status
+ * @md_ss_enable_status : if set to 1, this region would be copied during
coredump
+ * @encryption_status: Encryption status for this subsystem
+ * @encryption_required : Decides to encrypt the subsystem regions or not
+ * @ss_region_count : Number of regions added in this subsystem toc
+ * @md_ss_smem_regions_baseptr : regions base pointer of the subsystem
+ */
+struct md_ss_toc {
minidump_subsystem_toc
+ u32 md_ss_toc_init;
"status"
+ u32 md_ss_enable_status;
"enabled"
+ u32 encryption_status;
+ u32 encryption_required;
+ u32 ss_region_count;
+ u64 md_ss_smem_regions_baseptr;
+};
+
+/**
+ * md_global_toc: Global Table of Content
+ * @md_toc_init : Global Minidump init status
+ * @md_revision : Minidump revision
+ * @md_enable_status : Minidump enable status
+ * @md_ss_toc : Array of subsystems toc
+ */
+struct md_global_toc {
minidump_global_toc
Sure, I will update the structs accordingly.
+ u32 md_toc_init;
+ u32 md_revision;
+ u32 md_enable_status;
+ struct md_ss_tocmd_ss_toc[MAX_NUM_OF_SS];
+};
+
+#endif
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c
b/drivers/remoteproc/qcom_q6v5_pas.c
index 3837f23..752c862 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -28,11 +28,13 @@
#include "qcom_pil_info.h"
#include "qcom_q6v5.h"
#include "remoteproc_internal.h"
+#include "qcom_minidump.h"
struct adsp_data {
int crash_reason_smem;
const char *firmware_name;
int pas_id;
+ unsigned int minidump_id;
bool has_aggre2_clk;
bool auto_boot;
@@ -63,6 +65,7 @@ struct qcom_adsp {
int proxy_pd_count;
int pas_id;
+ unsigned int minidump_id;
int crash_reason_smem;
bool has_aggre2_clk;
const char *info_name;
@@ -81,6 +84,8 @@ struct qcom_adsp {
Re: [PATCH v5 2/3] remoteproc: qcom: Add capability to collect minidumps
On Thu 24 Sep 16:51 PDT 2020, Siddharth Gupta wrote:
> This patch adds support for collecting minidump in the event of remoteproc
> crash. Parse the minidump table based on remoteproc's unique minidump-id,
> read all memory regions from the remoteproc's minidump table entry and
> expose the memory to userspace. The remoteproc platform driver can choose
> to collect a full/mini dump by specifying the coredump op.
>
> Signed-off-by: Rishabh Bhatnagar
> Signed-off-by: Gurbir Arora
> Signed-off-by: Siddharth Gupta
If the three of you authored this patch you should add Co-developed-by
for Rishabh and Gurbir as well.
> ---
> drivers/remoteproc/qcom_minidump.h | 64 +
> drivers/remoteproc/qcom_q6v5_pas.c | 106 -
> drivers/remoteproc/remoteproc_coredump.c| 138
>
> drivers/remoteproc/remoteproc_elf_helpers.h | 27 ++
> include/linux/remoteproc.h | 1 +
> 5 files changed, 334 insertions(+), 2 deletions(-)
> create mode 100644 drivers/remoteproc/qcom_minidump.h
>
> diff --git a/drivers/remoteproc/qcom_minidump.h
> b/drivers/remoteproc/qcom_minidump.h
> new file mode 100644
> index 000..437e030
> --- /dev/null
> +++ b/drivers/remoteproc/qcom_minidump.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef __QCOM_MINIDUMP_H
> +#define __QCOM_MINIDUMP_H
> +
> +#define MAX_NUM_OF_SS 10
> +#define MAX_REGION_NAME_LENGTH 16
> +#define SBL_MINIDUMP_SMEM_ID 602
> +#define MD_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I'
> << 0)
> +#define MD_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E'
> << 0)
> +#define MD_SS_ENABLED('E' << 24 | 'N' << 16 | 'B' << 8 | 'L'
> << 0)
> +
> +/**
> + * md_ss_region - Minidump region
Valid kerneldoc should be:
* struct md_ss_region - Minidump region
> + * @name : Name of the region to be dumped
> + * @seq_num: : Use to differentiate regions with same name.
> + * @md_valid : This entry to be dumped (if set to 1)
> + * @region_base_address : Physical address of region to be dumped
> + * @region_size : Size of the region
> + */
> +struct md_ss_region {
But why don't you call this struct minidump_region?
> + charname[MAX_REGION_NAME_LENGTH];
> + u32 seq_num;
Is this __le32 or __be32?
> + u32 md_valid;
"valid" would be sufficient
> + u64 region_base_address;
"address"
> + u64 region_size;
"size"
> +};
> +
> +/**
> + * md_ss_toc: Subsystem's SMEM Table of content
> + * @md_ss_toc_init : Subsystem toc init status
> + * @md_ss_enable_status : if set to 1, this region would be copied during
> coredump
> + * @encryption_status: Encryption status for this subsystem
> + * @encryption_required : Decides to encrypt the subsystem regions or not
> + * @ss_region_count : Number of regions added in this subsystem toc
> + * @md_ss_smem_regions_baseptr : regions base pointer of the subsystem
> + */
> +struct md_ss_toc {
minidump_subsystem_toc
> + u32 md_ss_toc_init;
"status"
> + u32 md_ss_enable_status;
"enabled"
> + u32 encryption_status;
> + u32 encryption_required;
> + u32 ss_region_count;
> + u64 md_ss_smem_regions_baseptr;
> +};
> +
> +/**
> + * md_global_toc: Global Table of Content
> + * @md_toc_init : Global Minidump init status
> + * @md_revision : Minidump revision
> + * @md_enable_status : Minidump enable status
> + * @md_ss_toc : Array of subsystems toc
> + */
> +struct md_global_toc {
minidump_global_toc
> + u32 md_toc_init;
> + u32 md_revision;
> + u32 md_enable_status;
> + struct md_ss_tocmd_ss_toc[MAX_NUM_OF_SS];
> +};
> +
> +#endif
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c
> b/drivers/remoteproc/qcom_q6v5_pas.c
> index 3837f23..752c862 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -28,11 +28,13 @@
> #include "qcom_pil_info.h"
> #include "qcom_q6v5.h"
> #include "remoteproc_internal.h"
> +#include "qcom_minidump.h"
>
> struct adsp_data {
> int crash_reason_smem;
> const char *firmware_name;
> int pas_id;
> + unsigned int minidump_id;
> bool has_aggre2_clk;
> bool auto_boot;
>
> @@ -63,6 +65,7 @@ struct qcom_adsp {
> int proxy_pd_count;
>
> int pas_id;
> + unsigned int minidump_id;
> int crash_reason_smem;
> bool has_aggre2_clk;
> const char *info_name;
> @@ -81,6 +84,8 @@ struct qcom_adsp {
> struct qcom_sysmon *sysmon;
> };
>
> +static struct md_global_toc *g_md_toc;
"minidump_toc", but I would prefer that you just resolve
