LinuxLists.cc - [PATCH v3 00/18] Add basic Minidump kernel driver support

2023-05-03 17:05:33

Subject: [PATCH v3 00/18] Add basic Minidump kernel driver support

2023-05-03 17:05:47

Subject: [PATCH v3 06/18] soc: qcom: minidump: Add update region support

Add support to update client's region physical/virtual addresses,
which is useful for dynamic loadable modules, dynamic address
changing clients like if we want to collect current stack
information for each core and the current stack is changing on
each sched_switch event, So here virtual/physical address of
the current stack is changing. So, to cover such use cases
add the update region support in minidump driver.

Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/soc/qcom/qcom_minidump.c | 57 ++++++++++++++++++++++++++++++++++++++++
include/soc/qcom/qcom_minidump.h | 7 +++++
2 files changed, 64 insertions(+)

diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
index 6d29371..853bdda 100644
--- a/drivers/soc/qcom/qcom_minidump.c
+++ b/drivers/soc/qcom/qcom_minidump.c
@@ -561,6 +561,63 @@ int qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region
}
EXPORT_SYMBOL_GPL(qcom_apss_minidump_region_unregister);

+/**
+ * qcom_apss_minidump_update_region() - Update region in APSS minidump table.
+ * @region: minidump region.
+ *
+ * Return: On success, it returns 0 and negative error value on failure.
+ */
+int qcom_apss_minidump_update_region(const struct qcom_apss_minidump_region *region)
+{
+ struct minidump_pending_region *md_pregion;
+ struct qcom_apss_minidump_region *tmp;
+ struct minidump_region *mdr;
+ struct elfhdr *ehdr;
+ struct elf_shdr *shdr;
+ struct elf_phdr *phdr;
+ int idx;
+ int ret = 0;
+
+ if (!qcom_apss_minidump_valid_region(region))
+ return -EINVAL;
+
+ mutex_lock(&minidump_lock);
+ if (!__md) {
+ md_pregion = get_qcom_apss_pending_region(region);
+ if (!md_pregion) {
+ ret = -ENOENT;
+ goto unlock;
+ }
+ tmp = &md_pregion->region;
+ tmp->phys_addr = region->phys_addr;
+ tmp->virt_addr = region->virt_addr;
+ goto unlock;
+ }
+
+ idx = get_apss_minidump_region_index(region);
+ if (idx < 0) {
+ pr_err("%s region is not present\n", region->name);
+ ret = idx;
+ goto unlock;
+ }
+
+ mdr = &__md->md_regions[idx];
+ mdr->address = cpu_to_le64(region->phys_addr);
+
+ /* Skip predefined shdr/phdr header entry at the start */
+ ehdr = __md->elf.ehdr;
+ shdr = elf_shdr_entry_addr(ehdr, idx + 4);
+ phdr = elf_phdr_entry_addr(ehdr, idx + 1);
+
+ shdr->sh_addr = (elf_addr_t)region->virt_addr;
+ phdr->p_vaddr = (elf_addr_t)region->virt_addr;
+ phdr->p_paddr = region->phys_addr;
+unlock:
+ mutex_unlock(&minidump_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_apss_minidump_update_region);
+
static int qcom_minidump_init_apss_subsystem(struct minidump *md)
{
struct minidump_subsystem *apsstoc;
diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
index 1872668..0c8cc2d 100644
--- a/include/soc/qcom/qcom_minidump.h
+++ b/include/soc/qcom/qcom_minidump.h
@@ -102,6 +102,8 @@ extern int
qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region);
extern int
qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region);
+extern int
+qcom_apss_minidump_update_region(const struct qcom_apss_minidump_region *region);
#else
static inline
struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index)
@@ -119,5 +121,10 @@ qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *reg
{
return 0;
}
+statuc inline int
+int qcom_apss_minidump_update_region(const struct qcom_apss_minidump_region *region)
+{
+ return 0;
+}
#endif
#endif /* _QCOM_MINIDUMP_H_ */
--
2.7.4

2023-05-03 17:06:05

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 07/18] arm64: defconfig: Enable Qualcomm minidump driver

Previous patches add the Qualcomm minidump driver support, so
lets enable minidump config so that it can be used by kernel
clients.

Signed-off-by: Mukesh Ojha <[email protected]>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index a24609e..831c942 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1250,6 +1250,7 @@ CONFIG_QCOM_STATS=m
CONFIG_QCOM_WCNSS_CTRL=m
CONFIG_QCOM_APR=m
CONFIG_QCOM_ICC_BWMON=m
+CONFIG_QCOM_MINIDUMP=y
CONFIG_ARCH_R8A77995=y
CONFIG_ARCH_R8A77990=y
CONFIG_ARCH_R8A77951=y
--
2.7.4

2023-05-03 17:06:08

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 05/18] soc: qcom: minidump: Add pending region registration support

Pending regions are those apss minidump regions which came for
registration before minidump was initialized or ready to do
register the region.

We can add regions to pending region list and register them from
apss minidump driver probe in one go.

Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/soc/qcom/qcom_minidump.c | 114 ++++++++++++++++++++++++++++++++++-----
1 file changed, 100 insertions(+), 14 deletions(-)

diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
index d107a86..6d29371 100644
--- a/drivers/soc/qcom/qcom_minidump.c
+++ b/drivers/soc/qcom/qcom_minidump.c
@@ -49,6 +49,25 @@ struct minidump {
struct device *dev;
};

+/**
+ * struct minidump_pending_region - Minidump pending region
+ * @list : Pending region list pointer
+ * @region : APSS minidump client region
+ */
+struct minidump_pending_region {
+ struct list_head list;
+ struct qcom_apss_minidump_region region;
+};
+
+/**
+ * struct minidump_pending_region_list - Minidump pending region list
+ * @pregion_list : List of pending region to be registered
+ * @pregion_cnt : Count of the pending region to be registered
+ */
+struct minidump_pending_region_list {
+ struct list_head pregion_list;
+ int pregion_cnt;
+};
/*
* In some of the Old Qualcomm devices, boot firmware statically allocates 300
* as total number of supported region (including all co-processors) in
@@ -62,6 +81,10 @@ struct minidump {

static struct minidump *__md;
static DEFINE_MUTEX(minidump_lock);
+static struct minidump_pending_region_list md_pregion_list = {
+ .pregion_list = LIST_HEAD_INIT(md_pregion_list.pregion_list),
+ .pregion_cnt = 0,
+};

static struct elf_shdr *elf_shdr_entry_addr(struct elfhdr *ehdr, int idx)
{
@@ -312,6 +335,26 @@ struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_in
}
EXPORT_SYMBOL_GPL(qcom_minidump_subsystem_desc);

+static struct minidump_pending_region *
+get_qcom_apss_pending_region(const struct qcom_apss_minidump_region *region)
+{
+ struct minidump_pending_region *md_pregion;
+ struct minidump_pending_region *tmp;
+ bool found = false;
+
+ list_for_each_entry_safe(md_pregion, tmp, &md_pregion_list.pregion_list, list) {
+ struct qcom_apss_minidump_region *md_region;
+
+ md_region = &md_pregion->region;
+ if (!strcmp(md_region->name, region->name)) {
+ found = true;
+ break;
+ }
+ }
+
+ return found ? md_pregion : NULL;
+}
+
/**
* qcom_apss_minidump_region_register() - Register a region in Minidump table.
* @region: minidump region.
@@ -320,34 +363,58 @@ EXPORT_SYMBOL_GPL(qcom_minidump_subsystem_desc);
*/
int qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region)
{
+ struct minidump_pending_region *md_pregion;
unsigned int num_region;
- int ret;
-
- if (!__md)
- return -EPROBE_DEFER;
+ int ret = 0;

if (!qcom_apss_minidump_valid_region(region))
return -EINVAL;

mutex_lock(&minidump_lock);
- ret = get_apss_minidump_region_index(region);
- if (ret >= 0) {
- dev_info(__md->dev, "%s region is already registered\n", region->name);
+ if (!__md) {
+ md_pregion = get_qcom_apss_pending_region(region);
+ if (md_pregion) {
+ pr_info("%s region is already registered\n", region->name);
+ ret = -EEXIST;
+ goto unlock;
+ }
+ /*
+ * Maintain a list of client regions which came before
+ * minidump driver was ready and once it is ready,
+ * register them in one go from minidump probe function.
+ */
+ md_pregion = kzalloc(sizeof(*md_pregion), GFP_KERNEL);
+ if (!md_pregion) {
+ ret = -ENOMEM;
+ goto unlock;
+ }
+ md_pregion->region = *region;
+ list_add_tail(&md_pregion->list, &md_pregion_list.pregion_list);
+ md_pregion_list.pregion_cnt++;
+ goto unlock;
+ }
+
+ if (get_apss_minidump_region_index(region) >= 0) {
+ pr_info("%s region is already registered\n", region->name);
ret = -EEXIST;
goto unlock;
}

- /* Check if there is a room for a new entry */
+ /*
+ * Check if there is a room for a new region.
+ * Also, here the check for pregion_cnt is against one less
+ * than MAX_NUM_ENTRIES as we need one entry for ELF header.
+ */
num_region = le32_to_cpu(__md->md_apss_toc->region_count);
- if (num_region >= MAX_NUM_ENTRIES) {
- dev_err(__md->dev, "maximum region limit %u reached\n", num_region);
+ if (md_pregion_list.pregion_cnt >= (MAX_NUM_ENTRIES - 1) ||
+ num_region >= MAX_NUM_ENTRIES) {
+ pr_err("maximum region limit %u reached\n", num_region);
ret = -ENOSPC;
goto unlock;
}

qcom_apss_minidump_add_region(region);
qcom_apss_minidump_update_elf_header(region);
- ret = 0;
unlock:
mutex_unlock(&minidump_lock);
return ret;
@@ -443,17 +510,23 @@ qcom_apss_minidump_clear_header(const struct qcom_apss_minidump_region *region)
*/
int qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region)
{
+ struct minidump_pending_region *md_pregion;
struct minidump_region *mdr;
unsigned int num_region;
unsigned int idx;
- int ret;
+ int ret = 0;

- if (!region)
+ if (!qcom_apss_minidump_valid_region(region))
return -EINVAL;

mutex_lock(&minidump_lock);
if (!__md) {
- ret = -EPROBE_DEFER;
+ md_pregion = get_qcom_apss_pending_region(region);
+ if (!md_pregion)
+ goto unlock;
+ list_del(&md_pregion->list);
+ kfree(md_pregion);
+ md_pregion_list.pregion_cnt--;
goto unlock;
}

@@ -513,6 +586,8 @@ static int qcom_minidump_init_apss_subsystem(struct minidump *md)

static int qcom_minidump_probe(struct platform_device *pdev)
{
+ struct minidump_pending_region *md_pregion;
+ struct minidump_pending_region *tmp;
struct minidump_global_toc *mdgtoc;
struct minidump *md;
size_t size;
@@ -551,8 +626,19 @@ static int qcom_minidump_probe(struct platform_device *pdev)
dev_err(&pdev->dev, "Failed to add elf header: %d\n", ret);
memset(md->md_apss_toc, 0, sizeof(struct minidump_subsystem));
__md = NULL;
+ goto unlock;
}

+ list_for_each_entry_safe(md_pregion, tmp, &md_pregion_list.pregion_list, list) {
+ struct qcom_apss_minidump_region *region;
+
+ region = &md_pregion->region;
+ qcom_apss_minidump_add_region(region);
+ qcom_apss_minidump_update_elf_header(region);
+ list_del(&md_pregion->list);
+ kfree(md_pregion);
+ md_pregion_list.pregion_cnt--;
+ }
unlock:
mutex_unlock(&minidump_lock);
return ret;
--
2.7.4

2023-05-03 17:06:10

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 02/18] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

Move minidump specific data types and macros to a separate internal
header(qcom_minidump.h) so that it can be shared among different
Qualcomm drivers.

There is no change in functional behavior after this.

Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/remoteproc/qcom_common.c | 56 +---------------------------------
include/soc/qcom/qcom_minidump.h | 66 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 67 insertions(+), 55 deletions(-)
create mode 100644 include/soc/qcom/qcom_minidump.h

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 805e525..88fc984 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -18,6 +18,7 @@
#include <linux/slab.h>
#include <linux/soc/qcom/mdt_loader.h>
#include <linux/soc/qcom/smem.h>
+#include <soc/qcom/qcom_minidump.h>

#include "remoteproc_internal.h"
#include "qcom_common.h"
@@ -26,61 +27,6 @@
#define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev)
#define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev)

-#define MAX_NUM_OF_SS 10
-#define MAX_REGION_NAME_LENGTH 16
-#define SBL_MINIDUMP_SMEM_ID 602
-#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
-#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
-#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
-
-/**
- * struct minidump_region - Minidump region
- * @name : Name of the region to be dumped
- * @seq_num: : Use to differentiate regions with same name.
- * @valid : This entry to be dumped (if set to 1)
- * @address : Physical address of region to be dumped
- * @size : Size of the region
- */
-struct minidump_region {
- char name[MAX_REGION_NAME_LENGTH];
- __le32 seq_num;
- __le32 valid;
- __le64 address;
- __le64 size;
-};
-
-/**
- * struct minidump_subsystem - Subsystem's SMEM Table of content
- * @status : Subsystem toc init status
- * @enabled : 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
- * @region_count : Number of regions added in this subsystem toc
- * @regions_baseptr : regions base pointer of the subsystem
- */
-struct minidump_subsystem {
- __le32 status;
- __le32 enabled;
- __le32 encryption_status;
- __le32 encryption_required;
- __le32 region_count;
- __le64 regions_baseptr;
-};
-
-/**
- * struct minidump_global_toc - Global Table of Content
- * @status : Global Minidump init status
- * @md_revision : Minidump revision
- * @enabled : Minidump enable status
- * @subsystems : Array of subsystems toc
- */
-struct minidump_global_toc {
- __le32 status;
- __le32 md_revision;
- __le32 enabled;
- struct minidump_subsystem subsystems[MAX_NUM_OF_SS];
-};
-
struct qcom_ssr_subsystem {
const char *name;
struct srcu_notifier_head notifier_list;
diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
new file mode 100644
index 0000000..84c8605
--- /dev/null
+++ b/include/soc/qcom/qcom_minidump.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Qualcomm minidump shared data structures and macros
+ *
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. 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 MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
+#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
+#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+
+/**
+ * struct minidump_region - Minidump region
+ * @name : Name of the region to be dumped
+ * @seq_num: : Use to differentiate regions with same name.
+ * @valid : This entry to be dumped (if set to 1)
+ * @address : Physical address of region to be dumped
+ * @size : Size of the region
+ */
+struct minidump_region {
+ char name[MAX_REGION_NAME_LENGTH];
+ __le32 seq_num;
+ __le32 valid;
+ __le64 address;
+ __le64 size;
+};
+
+/**
+ * struct minidump_subsystem - Subsystem's SMEM Table of content
+ * @status : Subsystem toc init status
+ * @enabled : 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
+ * @region_count : Number of regions added in this subsystem toc
+ * @regions_baseptr : regions base pointer of the subsystem
+ */
+struct minidump_subsystem {
+ __le32 status;
+ __le32 enabled;
+ __le32 encryption_status;
+ __le32 encryption_required;
+ __le32 region_count;
+ __le64 regions_baseptr;
+};
+
+/**
+ * struct minidump_global_toc - Global Table of Content
+ * @status : Global Minidump init status
+ * @md_revision : Minidump revision
+ * @enabled : Minidump enable status
+ * @subsystems : Array of subsystems toc
+ */
+struct minidump_global_toc {
+ __le32 status;
+ __le32 md_revision;
+ __le32 enabled;
+ struct minidump_subsystem subsystems[MAX_NUM_OF_SS];
+};
+
+#endif /* _QCOM_MINIDUMP_H_ */
--
2.7.4

2023-05-03 17:06:18

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 04/18] soc: qcom: Add Qualcomm minidump kernel driver

Minidump is a best effort mechanism to collect useful and predefined
data for first level of debugging on end user devices running on
Qualcomm SoCs. It is built on the premise that System on Chip (SoC)
or subsystem part of SoC crashes, due to a range of hardware and
software bugs. Hence, the ability to collect accurate data is only
a best-effort. The data collected could be invalid or corrupted,
data collection itself could fail, and so on.

Qualcomm devices in engineering mode provides a mechanism for
generating full system ramdumps for post mortem debugging. But in some
cases it's however not feasible to capture the entire content of RAM.
The minidump mechanism provides the means for selecting region should
be included in the ramdump. The solution supports extracting the
ramdump/minidump produced either over USB or stored to an attached
storage device.

The core of minidump feature is part of Qualcomm's boot firmware code.
It initializes shared memory(SMEM), which is a part of DDR and
allocates a small section of it to minidump table i.e also called
global table of content (G-ToC). Each subsystem (APSS, ADSP, ...) has
their own table of segments to be included in the minidump, all
references from a descriptor in SMEM (G-ToC). Each segment/region has
some details like name, physical address and it's size etc. and it
could be anywhere scattered in the DDR.

Minidump kernel driver adds the capability to add linux region to be
dumped as part of ram dump collection. It provides appropriate symbol
to check its enablement and register client regions.

To simplify post mortem debugging, it creates and maintain an ELF
header as first region that gets updated upon registration
of a new region.

Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/soc/qcom/Kconfig | 14 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qcom_minidump.c | 581 +++++++++++++++++++++++++++++++++++++++
drivers/soc/qcom/smem.c | 8 +
include/soc/qcom/qcom_minidump.h | 61 +++-
5 files changed, 663 insertions(+), 2 deletions(-)
create mode 100644 drivers/soc/qcom/qcom_minidump.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index a491718..15c931e 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -279,4 +279,18 @@ config QCOM_INLINE_CRYPTO_ENGINE
tristate
select QCOM_SCM

+config QCOM_MINIDUMP
+ tristate "QCOM Minidump Support"
+ depends on ARCH_QCOM || COMPILE_TEST
+ select QCOM_SMEM
+ help
+ Enablement of core minidump feature is controlled from boot firmware
+ side, and this config allow linux to query and manages APPS minidump
+ table.
+
+ Client drivers can register their internal data structures and debug
+ messages as part of the minidump region and when the SoC is crashed,
+ these selective regions will be dumped instead of the entire DDR.
+ This saves significant amount of time and/or storage space.
+
endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 0f43a88..1ebe081 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -33,3 +33,4 @@ obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += ice.o
+obj-$(CONFIG_QCOM_MINIDUMP) += qcom_minidump.o
diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
new file mode 100644
index 0000000..d107a86
--- /dev/null
+++ b/drivers/soc/qcom/qcom_minidump.c
@@ -0,0 +1,581 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/elf.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/string.h>
+#include <linux/soc/qcom/smem.h>
+#include <soc/qcom/qcom_minidump.h>
+
+/**
+ * struct minidump_elfhdr - Minidump table elf header
+ * @ehdr: Elf main header
+ * @shdr: Section header
+ * @phdr: Program header
+ * @elf_offset: Section offset in elf
+ * @strtable_idx: String table current index position
+ */
+struct minidump_elfhdr {
+ struct elfhdr *ehdr;
+ struct elf_shdr *shdr;
+ struct elf_phdr *phdr;
+ size_t elf_offset;
+ size_t strtable_idx;
+};
+
+/**
+ * struct minidump - Minidump driver private data
+ * @md_gbl_toc : Global TOC pointer
+ * @md_apss_toc : Application Subsystem TOC pointer
+ * @md_regions : High level OS region base pointer
+ * @elf : Minidump elf header
+ * @dev : Minidump device
+ */
+struct minidump {
+ struct minidump_global_toc *md_gbl_toc;
+ struct minidump_subsystem *md_apss_toc;
+ struct minidump_region *md_regions;
+ struct minidump_elfhdr elf;
+ struct device *dev;
+};
+
+/*
+ * In some of the Old Qualcomm devices, boot firmware statically allocates 300
+ * as total number of supported region (including all co-processors) in
+ * minidump table out of which linux was using 201. In future, this limitation
+ * from boot firmware might get removed by allocating the region dynamically.
+ * So, keep it compatible with older devices, we can keep the current limit for
+ * Linux to 201.
+ */
+#define MAX_NUM_ENTRIES 201
+#define MAX_STRTBL_SIZE (MAX_NUM_ENTRIES * MAX_REGION_NAME_LENGTH)
+
+static struct minidump *__md;
+static DEFINE_MUTEX(minidump_lock);
+
+static struct elf_shdr *elf_shdr_entry_addr(struct elfhdr *ehdr, int idx)
+{
+ struct elf_shdr *eshdr = (struct elf_shdr *)((size_t)ehdr + ehdr->e_shoff);
+
+ return &eshdr[idx];
+}
+
+static struct elf_phdr *elf_phdr_entry_addr(struct elfhdr *ehdr, int idx)
+{
+ struct elf_phdr *ephdr = (struct elf_phdr *)((size_t)ehdr + ehdr->e_phoff);
+
+ return &ephdr[idx];
+}
+
+static char *elf_str_table_start(struct elfhdr *ehdr)
+{
+ struct elf_shdr *eshdr;
+
+ if (ehdr->e_shstrndx == SHN_UNDEF)
+ return NULL;
+
+ eshdr = elf_shdr_entry_addr(ehdr, ehdr->e_shstrndx);
+ return (char *)ehdr + eshdr->sh_offset;
+}
+
+static char *elf_lookup_string(struct elfhdr *ehdr, int offset)
+{
+ char *strtab = elf_str_table_start(ehdr);
+
+ if (!strtab || (__md->elf.strtable_idx < offset))
+ return NULL;
+
+ return strtab + offset;
+}
+
+static unsigned int append_str_to_strtable(const char *name)
+{
+ char *strtab = elf_str_table_start(__md->elf.ehdr);
+ unsigned int old_idx = __md->elf.strtable_idx;
+ unsigned int ret;
+
+ if (!strtab || !name)
+ return 0;
+
+ ret = old_idx;
+ old_idx += strscpy((strtab + old_idx), name, MAX_REGION_NAME_LENGTH);
+ __md->elf.strtable_idx = old_idx + 1;
+ return ret;
+}
+
+static int
+get_apss_minidump_region_index(const struct qcom_apss_minidump_region *region)
+{
+ struct minidump_region *mdr;
+ unsigned int i;
+ unsigned int count;
+
+ count = le32_to_cpu(__md->md_apss_toc->region_count);
+ for (i = 0; i < count; i++) {
+ mdr = &__md->md_regions[i];
+ if (!strcmp(mdr->name, region->name))
+ return i;
+ }
+ return -ENOENT;
+}
+
+static void
+qcom_apss_minidump_update_elf_header(const struct qcom_apss_minidump_region *region)
+{
+ struct elfhdr *ehdr = __md->elf.ehdr;
+ struct elf_shdr *shdr;
+ struct elf_phdr *phdr;
+
+ shdr = elf_shdr_entry_addr(ehdr, ehdr->e_shnum++);
+ phdr = elf_phdr_entry_addr(ehdr, ehdr->e_phnum++);
+
+ shdr->sh_type = SHT_PROGBITS;
+ shdr->sh_name = append_str_to_strtable(region->name);
+ shdr->sh_addr = (elf_addr_t)region->virt_addr;
+ shdr->sh_size = region->size;
+ shdr->sh_flags = SHF_WRITE;
+ shdr->sh_offset = __md->elf.elf_offset;
+ shdr->sh_entsize = 0;
+
+ phdr->p_type = PT_LOAD;
+ phdr->p_offset = __md->elf.elf_offset;
+ phdr->p_vaddr = (elf_addr_t)region->virt_addr;
+ phdr->p_paddr = region->phys_addr;
+ phdr->p_filesz = phdr->p_memsz = region->size;
+ phdr->p_flags = PF_R | PF_W;
+ __md->elf.elf_offset += shdr->sh_size;
+}
+
+static void
+qcom_apss_minidump_add_region(const struct qcom_apss_minidump_region *region)
+{
+ struct minidump_region *mdr;
+ unsigned int region_cnt = le32_to_cpu(__md->md_apss_toc->region_count);
+
+ mdr = &__md->md_regions[region_cnt];
+ strscpy(mdr->name, region->name, sizeof(mdr->name));
+ mdr->address = cpu_to_le64(region->phys_addr);
+ mdr->size = cpu_to_le64(region->size);
+ mdr->valid = cpu_to_le32(MINIDUMP_REGION_VALID);
+ region_cnt++;
+ __md->md_apss_toc->region_count = cpu_to_le32(region_cnt);
+}
+
+static bool
+qcom_apss_minidump_valid_region(const struct qcom_apss_minidump_region *region)
+{
+ return region &&
+ strnlen(region->name, MAX_NAME_LENGTH) < MAX_NAME_LENGTH &&
+ region->virt_addr &&
+ region->size &&
+ IS_ALIGNED(region->size, 4);
+}
+
+static int qcom_apss_minidump_add_elf_header(void)
+{
+ struct qcom_apss_minidump_region elfregion;
+ struct elfhdr *ehdr;
+ struct elf_shdr *shdr;
+ struct elf_phdr *phdr;
+ unsigned int elfh_size;
+ unsigned int strtbl_off;
+ unsigned int phdr_off;
+ char *banner;
+ unsigned int banner_len;
+
+ banner_len = strlen(linux_banner);
+ /*
+ * Header buffer contains:
+ * ELF header, (MAX_NUM_ENTRIES + 4) of Section and Program ELF headers,
+ * where, 4 additional entries, one for empty header, one for string table
+ * one for minidump table and one for linux banner.
+ *
+ * Linux banner is stored in minidump to aid post mortem tools to determine
+ * the kernel version.
+ */
+ elfh_size = sizeof(*ehdr);
+ elfh_size += MAX_STRTBL_SIZE;
+ elfh_size += banner_len + 1;
+ elfh_size += ((sizeof(*shdr) + sizeof(*phdr)) * (MAX_NUM_ENTRIES + 4));
+ elfh_size = ALIGN(elfh_size, 4);
+
+ __md->elf.ehdr = kzalloc(elfh_size, GFP_KERNEL);
+ if (!__md->elf.ehdr)
+ return -ENOMEM;
+
+ /* Register ELF header as first region */
+ strscpy(elfregion.name, "KELF_HEADER", sizeof(elfregion.name));
+ elfregion.virt_addr = __md->elf.ehdr;
+ elfregion.phys_addr = virt_to_phys(__md->elf.ehdr);
+ elfregion.size = elfh_size;
+ qcom_apss_minidump_add_region(&elfregion);
+
+ ehdr = __md->elf.ehdr;
+ /* Assign Section/Program headers offset */
+ __md->elf.shdr = shdr = (struct elf_shdr *)(ehdr + 1);
+ __md->elf.phdr = phdr = (struct elf_phdr *)(shdr + MAX_NUM_ENTRIES);
+ phdr_off = sizeof(*ehdr) + (sizeof(*shdr) * MAX_NUM_ENTRIES);
+
+ memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
+ ehdr->e_ident[EI_CLASS] = ELF_CLASS;
+ ehdr->e_ident[EI_DATA] = ELF_DATA;
+ ehdr->e_ident[EI_VERSION] = EV_CURRENT;
+ ehdr->e_ident[EI_OSABI] = ELF_OSABI;
+ ehdr->e_type = ET_CORE;
+ ehdr->e_machine = ELF_ARCH;
+ ehdr->e_version = EV_CURRENT;
+ ehdr->e_ehsize = sizeof(*ehdr);
+ ehdr->e_phoff = phdr_off;
+ ehdr->e_phentsize = sizeof(*phdr);
+ ehdr->e_shoff = sizeof(*ehdr);
+ ehdr->e_shentsize = sizeof(*shdr);
+ ehdr->e_shstrndx = 1;
+
+ __md->elf.elf_offset = elfh_size;
+
+ /*
+ * The zeroth index of the section header is reserved and is rarely used.
+ * Set the section header as null (SHN_UNDEF) and move to the next one.
+ * 2nd Section is String table.
+ */
+ __md->elf.strtable_idx = 1;
+ strtbl_off = sizeof(*ehdr) + ((sizeof(*phdr) + sizeof(*shdr)) * MAX_NUM_ENTRIES);
+ shdr++;
+ shdr->sh_type = SHT_STRTAB;
+ shdr->sh_offset = (elf_addr_t)strtbl_off;
+ shdr->sh_size = MAX_STRTBL_SIZE;
+ shdr->sh_entsize = 0;
+ shdr->sh_flags = 0;
+ shdr->sh_name = append_str_to_strtable("STR_TBL");
+ shdr++;
+
+ /* 3rd Section is Linux banner */
+ banner = (char *)ehdr + strtbl_off + MAX_STRTBL_SIZE;
+ memcpy(banner, linux_banner, banner_len);
+
+ shdr->sh_type = SHT_PROGBITS;
+ shdr->sh_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
+ shdr->sh_size = banner_len + 1;
+ shdr->sh_addr = (elf_addr_t)linux_banner;
+ shdr->sh_entsize = 0;
+ shdr->sh_flags = SHF_WRITE;
+ shdr->sh_name = append_str_to_strtable("linux_banner");
+
+ phdr->p_type = PT_LOAD;
+ phdr->p_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
+ phdr->p_vaddr = (elf_addr_t)linux_banner;
+ phdr->p_paddr = virt_to_phys(linux_banner);
+ phdr->p_filesz = phdr->p_memsz = banner_len + 1;
+ phdr->p_flags = PF_R | PF_W;
+
+ /*
+ * Above are some prdefined sections/program header used
+ * for debug, update their count here.
+ */
+ ehdr->e_phnum = 1;
+ ehdr->e_shnum = 3;
+
+ return 0;
+}
+
+/**
+ * qcom_minidump_subsystem_desc() - Get minidump subsystem descriptor.
+ * @minidump_index: minidump index for a subsystem in minidump table
+ *
+ * Return: minidump subsystem descriptor address on success and error
+ * on failure
+ */
+struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index)
+{
+ struct minidump_subsystem *md_ss_toc;
+
+ mutex_lock(&minidump_lock);
+ if (!__md) {
+ md_ss_toc = ERR_PTR(-EPROBE_DEFER);
+ goto unlock;
+ }
+
+ md_ss_toc = &__md->md_gbl_toc->subsystems[minidump_index];
+unlock:
+ mutex_unlock(&minidump_lock);
+ return md_ss_toc;
+}
+EXPORT_SYMBOL_GPL(qcom_minidump_subsystem_desc);
+
+/**
+ * qcom_apss_minidump_region_register() - Register a region in Minidump table.
+ * @region: minidump region.
+ *
+ * Return: On success, it returns 0, otherwise a negative error value on failure.
+ */
+int qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region)
+{
+ unsigned int num_region;
+ int ret;
+
+ if (!__md)
+ return -EPROBE_DEFER;
+
+ if (!qcom_apss_minidump_valid_region(region))
+ return -EINVAL;
+
+ mutex_lock(&minidump_lock);
+ ret = get_apss_minidump_region_index(region);
+ if (ret >= 0) {
+ dev_info(__md->dev, "%s region is already registered\n", region->name);
+ ret = -EEXIST;
+ goto unlock;
+ }
+
+ /* Check if there is a room for a new entry */
+ num_region = le32_to_cpu(__md->md_apss_toc->region_count);
+ if (num_region >= MAX_NUM_ENTRIES) {
+ dev_err(__md->dev, "maximum region limit %u reached\n", num_region);
+ ret = -ENOSPC;
+ goto unlock;
+ }
+
+ qcom_apss_minidump_add_region(region);
+ qcom_apss_minidump_update_elf_header(region);
+ ret = 0;
+unlock:
+ mutex_unlock(&minidump_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_apss_minidump_region_register);
+
+static int
+qcom_apss_minidump_clear_header(const struct qcom_apss_minidump_region *region)
+{
+ struct elfhdr *ehdr = __md->elf.ehdr;
+ struct elf_shdr *shdr;
+ struct elf_shdr *tmp_shdr;
+ struct elf_phdr *phdr;
+ struct elf_phdr *tmp_phdr;
+ unsigned int phidx;
+ unsigned int shidx;
+ unsigned int len;
+ unsigned int i;
+ char *shname;
+
+ for (i = 0; i < ehdr->e_phnum; i++) {
+ phdr = elf_phdr_entry_addr(ehdr, i);
+ if (phdr->p_paddr == region->phys_addr &&
+ phdr->p_memsz == region->size)
+ break;
+ }
+
+ if (i == ehdr->e_phnum) {
+ dev_err(__md->dev, "Cannot find program header entry in elf\n");
+ return -EINVAL;
+ }
+
+ phidx = i;
+ for (i = 0; i < ehdr->e_shnum; i++) {
+ shdr = elf_shdr_entry_addr(ehdr, i);
+ shname = elf_lookup_string(ehdr, shdr->sh_name);
+ if (shname && !strcmp(shname, region->name) &&
+ shdr->sh_addr == (elf_addr_t)region->virt_addr &&
+ shdr->sh_size == region->size)
+ break;
+ }
+
+ if (i == ehdr->e_shnum) {
+ dev_err(__md->dev, "Cannot find section header entry in elf\n");
+ return -EINVAL;
+ }
+
+ shidx = i;
+ if (shdr->sh_offset != phdr->p_offset) {
+ dev_err(__md->dev, "Invalid entry details for region: %s\n", region->name);
+ return -EINVAL;
+ }
+
+ /* Clear name in string table */
+ len = strlen(shname) + 1;
+ memmove(shname, shname + len,
+ __md->elf.strtable_idx - shdr->sh_name - len);
+ __md->elf.strtable_idx -= len;
+
+ /* Clear program header */
+ tmp_phdr = elf_phdr_entry_addr(ehdr, phidx);
+ for (i = phidx; i < ehdr->e_phnum - 1; i++) {
+ tmp_phdr = elf_phdr_entry_addr(ehdr, i + 1);
+ phdr = elf_phdr_entry_addr(ehdr, i);
+ memcpy(phdr, tmp_phdr, sizeof(struct elf_phdr));
+ phdr->p_offset = phdr->p_offset - region->size;
+ }
+ memset(tmp_phdr, 0, sizeof(struct elf_phdr));
+ ehdr->e_phnum--;
+
+ /* Clear section header */
+ tmp_shdr = elf_shdr_entry_addr(ehdr, shidx);
+ for (i = shidx; i < ehdr->e_shnum - 1; i++) {
+ tmp_shdr = elf_shdr_entry_addr(ehdr, i + 1);
+ shdr = elf_shdr_entry_addr(ehdr, i);
+ memcpy(shdr, tmp_shdr, sizeof(struct elf_shdr));
+ shdr->sh_offset -= region->size;
+ shdr->sh_name -= len;
+ }
+
+ memset(tmp_shdr, 0, sizeof(struct elf_shdr));
+ ehdr->e_shnum--;
+ __md->elf.elf_offset -= region->size;
+
+ return 0;
+}
+
+/**
+ * qcom_apss_minidump_region_unregister() - Unregister region from Minidump table.
+ * @region: minidump region.
+ *
+ * Return: On success, it returns 0 and negative error value on failure.
+ */
+int qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region)
+{
+ struct minidump_region *mdr;
+ unsigned int num_region;
+ unsigned int idx;
+ int ret;
+
+ if (!region)
+ return -EINVAL;
+
+ mutex_lock(&minidump_lock);
+ if (!__md) {
+ ret = -EPROBE_DEFER;
+ goto unlock;
+ }
+
+ idx = get_apss_minidump_region_index(region);
+ if (idx < 0) {
+ dev_err(__md->dev, "%s region is not present\n", region->name);
+ ret = idx;
+ goto unlock;
+ }
+
+ mdr = &__md->md_regions[0];
+ num_region = le32_to_cpu(__md->md_apss_toc->region_count);
+ /*
+ * Left shift all the regions exist after this removed region
+ * index by 1 to fill the gap and zero out the last region
+ * present at the end.
+ */
+ memmove(&mdr[idx], &mdr[idx + 1],
+ (num_region - idx - 1) * sizeof(struct minidump_region));
+ memset(&mdr[num_region - 1], 0, sizeof(struct minidump_region));
+ ret = qcom_apss_minidump_clear_header(region);
+ if (ret) {
+ dev_err(__md->dev, "Failed to remove region: %s\n", region->name);
+ goto unlock;
+ }
+
+ num_region--;
+ __md->md_apss_toc->region_count = cpu_to_le32(num_region);
+unlock:
+ mutex_unlock(&minidump_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_apss_minidump_region_unregister);
+
+static int qcom_minidump_init_apss_subsystem(struct minidump *md)
+{
+ struct minidump_subsystem *apsstoc;
+
+ apsstoc = &md->md_gbl_toc->subsystems[MINIDUMP_APSS_DESC];
+ md->md_regions = devm_kcalloc(md->dev, MAX_NUM_ENTRIES,
+ sizeof(struct minidump_region), GFP_KERNEL);
+ if (!md->md_regions)
+ return -ENOMEM;
+
+ md->md_apss_toc = apsstoc;
+ apsstoc->regions_baseptr = cpu_to_le64(virt_to_phys(md->md_regions));
+ apsstoc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
+ apsstoc->status = cpu_to_le32(1);
+ apsstoc->region_count = cpu_to_le32(0);
+
+ /* Tell bootloader not to encrypt the regions of this subsystem */
+ apsstoc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
+ apsstoc->encryption_required = cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
+
+ return 0;
+}
+
+static int qcom_minidump_probe(struct platform_device *pdev)
+{
+ struct minidump_global_toc *mdgtoc;
+ struct minidump *md;
+ size_t size;
+ int ret;
+
+ md = devm_kzalloc(&pdev->dev, sizeof(*md), GFP_KERNEL);
+ if (!md)
+ return -ENOMEM;
+
+ mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size);
+ if (IS_ERR(mdgtoc)) {
+ ret = PTR_ERR(mdgtoc);
+ dev_err(&pdev->dev, "Couldn't find minidump smem item: %d\n", ret);
+ return ret;
+ }
+
+ if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
+ ret = -EINVAL;
+ dev_err(&pdev->dev, "minidump table is not initialized: %d\n", ret);
+ return ret;
+ }
+
+ mutex_lock(&minidump_lock);
+ md->dev = &pdev->dev;
+ md->md_gbl_toc = mdgtoc;
+ ret = qcom_minidump_init_apss_subsystem(md);
+ if (ret) {
+ dev_err(&pdev->dev, "apss minidump initialization failed: %d\n", ret);
+ goto unlock;
+ }
+
+ __md = md;
+ /* First entry would be ELF header */
+ ret = qcom_apss_minidump_add_elf_header();
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to add elf header: %d\n", ret);
+ memset(md->md_apss_toc, 0, sizeof(struct minidump_subsystem));
+ __md = NULL;
+ }
+
+unlock:
+ mutex_unlock(&minidump_lock);
+ return ret;
+}
+
+static int qcom_minidump_remove(struct platform_device *pdev)
+{
+ memset(__md->md_apss_toc, 0, sizeof(struct minidump_subsystem));
+ __md = NULL;
+
+ return 0;
+}
+
+static struct platform_driver qcom_minidump_driver = {
+ .probe = qcom_minidump_probe,
+ .remove = qcom_minidump_remove,
+ .driver = {
+ .name = "qcom-minidump",
+ },
+};
+
+module_platform_driver(qcom_minidump_driver);
+
+MODULE_DESCRIPTION("Qualcomm APSS minidump driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:qcom-minidump");
diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 6be7ea9..d459656 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -279,6 +279,7 @@ struct qcom_smem {

u32 item_count;
struct platform_device *socinfo;
+ struct platform_device *minidump;
struct smem_ptable *ptable;
struct smem_partition global_partition;
struct smem_partition partitions[SMEM_HOST_COUNT];
@@ -1151,12 +1152,19 @@ static int qcom_smem_probe(struct platform_device *pdev)
if (IS_ERR(smem->socinfo))
dev_dbg(&pdev->dev, "failed to register socinfo device\n");

+ smem->minidump = platform_device_register_data(&pdev->dev, "qcom-minidump",
+ PLATFORM_DEVID_NONE, NULL,
+ 0);
+ if (IS_ERR(smem->minidump))
+ dev_dbg(&pdev->dev, "failed to register minidump device\n");
+
return 0;
}

static int qcom_smem_remove(struct platform_device *pdev)
{
platform_device_unregister(__smem->socinfo);
+ platform_device_unregister(__smem->minidump);

hwspin_lock_free(__smem->hwlock);
__smem = NULL;
diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
index 84c8605..1872668 100644
--- a/include/soc/qcom/qcom_minidump.h
+++ b/include/soc/qcom/qcom_minidump.h
@@ -1,6 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
- * Qualcomm minidump shared data structures and macros
+ * This file contain Qualcomm minidump data structures and macros shared with
+ * boot firmware and also apss minidump client's data structure
*
* Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
*/
@@ -9,12 +10,27 @@
#define _QCOM_MINIDUMP_H_

#define MAX_NUM_OF_SS 10
+#define MAX_NAME_LENGTH 12
#define MAX_REGION_NAME_LENGTH 16
+
+#define MINIDUMP_REVISION 1
#define SBL_MINIDUMP_SMEM_ID 602
+
+/* Application processor minidump descriptor */
+#define MINIDUMP_APSS_DESC 0
+#define SMEM_ENTRY_SIZE 40
+
#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
+#define MINIDUMP_REGION_INVALID ('I' << 24 | 'N' << 16 | 'V' << 8 | 'A' << 0)
+#define MINIDUMP_REGION_INIT ('I' << 24 | 'N' << 16 | 'I' << 8 | 'T' << 0)
+#define MINIDUMP_REGION_NOINIT 0
+
+#define MINIDUMP_SS_ENCR_REQ (0 << 24 | 'Y' << 16 | 'E' << 8 | 'S' << 0)
+#define MINIDUMP_SS_ENCR_NOTREQ (0 << 24 | 0 << 16 | 'N' << 8 | 'R' << 0)
+#define MINIDUMP_SS_ENCR_NONE ('N' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
+#define MINIDUMP_SS_ENCR_START ('S' << 24 | 'T' << 16 | 'R' << 8 | 'T' << 0)
#define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
-
/**
* struct minidump_region - Minidump region
* @name : Name of the region to be dumped
@@ -63,4 +79,45 @@ struct minidump_global_toc {
struct minidump_subsystem subsystems[MAX_NUM_OF_SS];
};

+/**
+ * struct qcom_apss_minidump_region - APSS Minidump region information
+ *
+ * @name: Entry name, Minidump will dump binary with this name.
+ * @virt_addr: Virtual address of the entry.
+ * @phys_addr: Physical address of the entry to dump.
+ * @size: Number of byte to dump from @address location,
+ * and it should be 4 byte aligned.
+ */
+struct qcom_apss_minidump_region {
+ char name[MAX_NAME_LENGTH];
+ void *virt_addr;
+ phys_addr_t phys_addr;
+ size_t size;
+};
+
+#if IS_ENABLED(CONFIG_QCOM_MINIDUMP)
+extern struct minidump_subsystem *
+qcom_minidump_subsystem_desc(unsigned int minidump_index);
+extern int
+qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region);
+extern int
+qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region);
+#else
+static inline
+struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index)
+{
+ return NULL;
+}
+static inline int
+qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region)
+{
+ /* Return quietly, if minidump is not enabled */
+ return 0;
+}
+static inline int
+qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region)
+{
+ return 0;
+}
+#endif
#endif /* _QCOM_MINIDUMP_H_ */
--
2.7.4

2023-05-03 17:06:38

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 13/18] arm64: defconfig: Enable Qualcomm pstore minidump client driver

As we have enabled qualcomm minidump driver, so lets enable one client
driver which captures the already existing ramoops region like record,
console, ftrace, pmsg through qualcomm minidump infrastructure.

Signed-off-by: Mukesh Ojha <[email protected]>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 831c942..1ccae8b 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1251,6 +1251,7 @@ CONFIG_QCOM_WCNSS_CTRL=m
CONFIG_QCOM_APR=m
CONFIG_QCOM_ICC_BWMON=m
CONFIG_QCOM_MINIDUMP=y
+CONFIG_QCOM_PSTORE_MINIDUMP=y
CONFIG_ARCH_R8A77995=y
CONFIG_ARCH_R8A77990=y
CONFIG_ARCH_R8A77951=y
--
2.7.4

2023-05-03 17:06:57

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 11/18] arm64: dts: qcom: sm8450: Add Qualcomm ramoops minidump node

This enable dynamic reserve memory for Qualcomm ramoops device,
Which will used to save ramoops frontend data and this region
gets dumped on crash via Qualcomm's minidump infrastructure.
qcom_pstore_minidump is the associated driver for this node.

Signed-off-by: Mukesh Ojha <[email protected]>
---
arch/arm64/boot/dts/qcom/sm8450.dtsi | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 595533a..92d023f 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -614,6 +614,17 @@
reg = <0x0 0xed900000 0x0 0x3b00000>;
no-map;
};
+
+ qcom_ramoops_md_region:qcom_ramoops_md {
+ alloc-ranges = <0x0 0x00000000 0xffffffff 0xffffffff>;
+ size = <0x0 0x200000>;
+ no-map;
+ };
+ };
+
+ qcom_ramoops_md {
+ compatible = "qcom,sm8450-ramoops-minidump", "qcom,ramoops-minidump";
+ memory-region = <&qcom_ramoops_md_region>;
};

smp2p-adsp {
--
2.7.4

2023-05-03 17:06:57

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 10/18] dt-bindings: reserved-memory: Add qcom,ramoops-minidump binding

Qualcomm ramoops minidump logger provide a means of storing the ramoops data
to some dynamically reserved memory instead of traditionally implemented
ramoops where the region should be statically fixed ram region.

Add qcom,ramoops-minidump binding under "/reserved-memory", and is named
"qcom,ramoops-minidump" and the reason of naming like this is because
as it is going to contain ramoops frontend data and this content will
be collected via Qualcomm minidump infrastructure provided from the
boot firmware.

Signed-off-by: Mukesh Ojha <[email protected]>
---
.../reserved-memory/qcom,ramoops-minidump.yaml | 69 ++++++++++++++++++++++
1 file changed, 69 insertions(+)
create mode 100644 Documentation/devicetree/bindings/reserved-memory/qcom,ramoops-minidump.yaml

diff --git a/Documentation/devicetree/bindings/reserved-memory/qcom,ramoops-minidump.yaml b/Documentation/devicetree/bindings/reserved-memory/qcom,ramoops-minidump.yaml
new file mode 100644
index 0000000..a308db0
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/qcom,ramoops-minidump.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/reserved-memory/qcom,ramoops-minidump.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Qualcomm Ramoops minidump logger
+
+description: |
+ Qualcomm ramoops minidump logger provide a means of storing the ramoops data
+ to some dynamically reserved memory instead of traditionally implemented
+ ramoops where the region should be statically fixed ram region.
+
+ This is a child-node of "/reserved-memory", and is named "qcom_ramoops_md_region"
+ and the reason of naming like this is because as it is going to contain ramoops
+ frontend data and this content will be collected via Qualcomm minidump
+ infrastructure provided from the boot firmware.
+
+maintainers:
+ - Mukesh Ojha <[email protected]>
+
+allOf:
+ - $ref: "reserved-memory.yaml"
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - qcom,sm8450-ramoops-minidump
+ - const: qcom,ramoops-minidump
+
+ memory-region:
+ maxItems: 1
+ items:
+ - description: handle to memory reservation for qcom,ramoops-minidump region.
+
+ no-map: true
+
+unevaluatedProperties: false
+
+required:
+ - compatible
+ - memory-region
+
+examples:
+ - |
+ / {
+ compatible = "foo";
+ model = "foo";
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ reserved-memory {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ qcom_ramoops_md_region@qcom_ramoops_md {
+ alloc-ranges = <0x0 0x00000000 0xffffffff 0xffffffff>;
+ size = <0x0 0x200000>;
+ no-map
+ };
+ };
+
+ qcom_ramoops_md {
+ compatible = "qcom,sm8450-ramoops-minidump", "qcom,ramoops-minidump";
+ memory-region = <&qcom_ramoops_md_region>;
+ };
+ };
--
2.7.4

2023-05-03 17:07:03

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 08/18] remoterproc: qcom: refactor to leverage exported minidump symbol

qcom_minidump driver provides qcom_minidump_subsystem_desc()
exported API which other driver can use it query subsystem
descriptor. Refactor qcom_minidump() to use this symbol.

Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/remoteproc/qcom_common.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 88fc984..240e9f7 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, unsigned int minidump_id,
{
int ret;
struct minidump_subsystem *subsystem;
- struct minidump_global_toc *toc;

- /* Get Global minidump ToC*/
- toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, NULL);
-
- /* check if global table pointer exists and init is set */
- if (IS_ERR(toc) || !toc->status) {
- dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
+ subsystem = qcom_minidump_subsystem_desc(minidump_id);
+ if (IS_ERR(subsystem))
return;
- }
-
- /* Get subsystem table of contents using the minidump id */
- subsystem = &toc->subsystems[minidump_id];

/**
* Collect minidump if SS ToC is valid and segment table
--
2.7.4

2023-05-03 17:07:10

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 14/18] firmware: qcom_scm: provide a read-modify-write function

It was realized by Srinivas K. that there is a need of
read-modify-write scm exported function so that it can
be used by multiple clients.

Let's introduce qcom_scm_io_update_field() which masks
out the bits and write the passed value to that
bit-offset. Subsequent patch will use this function.

Suggested-by: Srinivas Kandagatla <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/firmware/qcom_scm.c | 15 +++++++++++++++
include/linux/firmware/qcom/qcom_scm.h | 2 ++
2 files changed, 17 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index fde33acd..003cbcb 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -407,6 +407,21 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
}
EXPORT_SYMBOL(qcom_scm_set_remote_state);

+int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, unsigned int val)
+{
+ unsigned int old, new;
+ int ret;
+
+ ret = qcom_scm_io_readl(addr, &old);
+ if (ret)
+ return ret;
+
+ new = (old & ~mask) | val << (ffs(mask) - 1);
+
+ return qcom_scm_io_writel(addr, new);
+}
+EXPORT_SYMBOL(qcom_scm_io_update_field);
+
static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
{
struct qcom_scm_desc desc = {
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index 250ea4e..ca41e4e 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);

extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
+extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask,
+ unsigned int val);

extern bool qcom_scm_restore_sec_cfg_available(void);
extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
--
2.7.4

2023-05-03 17:07:10

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 09/18] soc: qcom: Add qcom's pstore minidump driver support

This driver was inspired from the fact pstore ram region should be
fixed and boot firmware need to have awarness about this region,
so that it will be persistent across boot. But, there are many
QCOM SoC which does not support warm boot from hardware but they
have minidump support from the software, and for them, there is
no need of this pstore ram region to be fixed, but at the same
time have interest in the pstore frontends. So, this driver
get the dynamic reserved region from the ram and register the
ramoops platform device.

+---------+ +---------+ +--------+ +---------+
| console | | pmsg | | ftrace | | dmesg |
+---------+ +---------+ +--------+ +---------+
| | | |
| | | |
+------------------------------------------+
|
\ /
+----------------+
(1) |pstore frontends|
+----------------+
|
\ /
+------------------- +
(2) | pstore backend(ram)|
+--------------------+
|
\ /
+--------------------+
(3) |qcom_pstore_minidump|
+--------------------+
|
\ /
+---------------+
(4) | qcom_minidump |
+---------------+

This driver will route all the pstore front data to the stored
in qcom pstore reserved region and the reason of showing an
arrow from (3) to (4) as qcom_pstore_minidump driver will register
all the available frontends region with qcom minidump driver
in upcoming patch.

Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/soc/qcom/Kconfig | 11 +++
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qcom_pstore_minidump.c | 116 ++++++++++++++++++++++++++++++++
3 files changed, 128 insertions(+)
create mode 100644 drivers/soc/qcom/qcom_pstore_minidump.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 15c931e..afdc634 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -293,4 +293,15 @@ config QCOM_MINIDUMP
these selective regions will be dumped instead of the entire DDR.
This saves significant amount of time and/or storage space.

+config QCOM_PSTORE_MINIDUMP
+ tristate "Pstore support for QCOM Minidump"
+ depends on ARCH_QCOM
+ depends on PSTORE_RAM
+ depends on QCOM_MINIDUMP
+ help
+ Enablement of this driver ensures that ramoops region can be anywhere
+ reserved in ram instead of being fixed address which needs boot firmware
+ awareness. So, this driver creates plaform device and registers available
+ frontend region with the Qualcomm's minidump driver.
+
endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 1ebe081..02d30d7 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -34,3 +34,4 @@ obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += ice.o
obj-$(CONFIG_QCOM_MINIDUMP) += qcom_minidump.o
+obj-$(CONFIG_QCOM_PSTORE_MINIDUMP) += qcom_pstore_minidump.o
diff --git a/drivers/soc/qcom/qcom_pstore_minidump.c b/drivers/soc/qcom/qcom_pstore_minidump.c
new file mode 100644
index 0000000..8d58500
--- /dev/null
+++ b/drivers/soc/qcom/qcom_pstore_minidump.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/pstore_ram.h>
+#include <soc/qcom/qcom_minidump.h>
+
+struct qcom_ramoops_config {
+ unsigned long record_size;
+ unsigned long console_size;
+ unsigned long ftrace_size;
+ unsigned long pmsg_size;
+ unsigned int mem_type;
+ unsigned int flags;
+ int max_reason;
+};
+
+struct qcom_ramoops_dd {
+ struct ramoops_platform_data qcom_ramoops_pdata;
+ struct platform_device *ramoops_pdev;
+};
+
+static struct qcom_ramoops_config default_ramoops_config = {
+ .mem_type = 2,
+ .record_size = 0x0,
+ .console_size = 0x200000,
+ .ftrace_size = 0x0,
+ .pmsg_size = 0x0,
+};
+
+static struct qcom_ramoops_dd *qcom_rdd;
+static int qcom_ramoops_probe(struct platform_device *pdev)
+{
+ struct device_node *of_node = pdev->dev.of_node;
+ struct device_node *node;
+ const struct qcom_ramoops_config *cfg;
+ struct ramoops_platform_data *pdata;
+ struct reserved_mem *rmem;
+ long ret;
+
+ node = of_parse_phandle(of_node, "memory-region", 0);
+ if (!node)
+ return -ENODEV;
+
+ rmem = of_reserved_mem_lookup(node);
+ of_node_put(node);
+ if (!rmem) {
+ dev_err(&pdev->dev, "failed to locate DT /reserved-memory resource\n");
+ return -EINVAL;
+ }
+
+ qcom_rdd = devm_kzalloc(&pdev->dev, sizeof(*qcom_rdd), GFP_KERNEL);
+ if (!qcom_rdd)
+ return -ENOMEM;
+
+ cfg = of_device_get_match_data(&pdev->dev);
+ if (!cfg) {
+ dev_err(&pdev->dev, "failed to get supported matched data\n");
+ return -ENOENT;
+ }
+
+ pdata = &qcom_rdd->qcom_ramoops_pdata;
+ pdata->mem_size = rmem->size;
+ pdata->mem_address = rmem->base;
+ pdata->mem_type = cfg->mem_type;
+ pdata->record_size = cfg->record_size;
+ pdata->console_size = cfg->console_size;
+ pdata->ftrace_size = cfg->ftrace_size;
+ pdata->pmsg_size = cfg->pmsg_size;
+ pdata->max_reason = KMSG_DUMP_PANIC;
+
+ qcom_rdd->ramoops_pdev = platform_device_register_data(NULL, "ramoops", -1,
+ pdata, sizeof(*pdata));
+ if (IS_ERR(qcom_rdd->ramoops_pdev)) {
+ ret = PTR_ERR(qcom_rdd->ramoops_pdev);
+ dev_err(&pdev->dev, "could not create platform device: %ld\n", ret);
+ qcom_rdd->ramoops_pdev = NULL;
+ }
+
+ return ret;
+}
+
+static int qcom_ramoops_remove(struct platform_device *pdev)
+{
+ platform_device_unregister(qcom_rdd->ramoops_pdev);
+ qcom_rdd->ramoops_pdev = NULL;
+
+ return 0;
+}
+
+static const struct of_device_id qcom_ramoops_of_match[] = {
+ { .compatible = "qcom,sm8450-ramoops-minidump", .data = &default_ramoops_config },
+ { .compatible = "qcom,ramoops-minidump", .data = &default_ramoops_config },
+ {}
+};
+
+MODULE_DEVICE_TABLE(of, qcom_ramoops_of_match);
+static struct platform_driver qcom_ramoops_drv = {
+ .driver = {
+ .name = "qcom,ramoops-minidump",
+ .of_match_table = qcom_ramoops_of_match,
+ },
+ .probe = qcom_ramoops_probe,
+ .remove = qcom_ramoops_remove,
+};
+
+module_platform_driver(qcom_ramoops_drv);
+
+MODULE_DESCRIPTION("Qualcomm minidump pstore driver");
+MODULE_LICENSE("GPL");
--
2.7.4

2023-05-03 17:07:17

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 15/18] pinctrl: qcom: Use qcom_scm_io_update_field()

Use qcom_scm_io_update_field() exported function introduced
in last commit.

Acked-by: Linus Walleij <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 4515f37..856ebbf 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1042,6 +1042,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
unsigned long flags;
bool was_enabled;
u32 val;
+ u32 mask;

if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
set_bit(d->hwirq, pctrl->dual_edge_irqs);
@@ -1075,23 +1076,19 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
* With intr_target_use_scm interrupts are routed to
* application cpu using scm calls.
*/
+ mask = (7 << g->intr_target_bit);
if (pctrl->intr_target_use_scm) {
u32 addr = pctrl->phys_base[0] + g->intr_target_reg;
int ret;

- qcom_scm_io_readl(addr, &val);
-
- val &= ~(7 << g->intr_target_bit);
- val |= g->intr_target_kpss_val << g->intr_target_bit;
-
- ret = qcom_scm_io_writel(addr, val);
+ ret = qcom_scm_io_update_field(addr, mask, g->intr_target_kpss_val);
if (ret)
dev_err(pctrl->dev,
"Failed routing %lu interrupt to Apps proc",
d->hwirq);
} else {
val = msm_readl_intr_target(pctrl, g);
- val &= ~(7 << g->intr_target_bit);
+ val &= ~mask;
val |= g->intr_target_kpss_val << g->intr_target_bit;
msm_writel_intr_target(val, pctrl, g);
}
--
2.7.4

2023-05-03 17:07:26

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 17/18] firmware: qcom_scm: Refactor code to support multiple download mode

Currently on Qualcomm SoC, download_mode is enabled if
CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT is selected.

Refactor the code such that it supports multiple download
modes and drop CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT config
instead, give interface to set the download mode from
module parameter.

Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/firmware/Kconfig | 11 ---------
drivers/firmware/qcom_scm.c | 60 +++++++++++++++++++++++++++++++++++++++------
2 files changed, 52 insertions(+), 19 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b59e304..ff7e9f3 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -215,17 +215,6 @@ config MTK_ADSP_IPC
config QCOM_SCM
tristate

-config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
- bool "Qualcomm download mode enabled by default"
- depends on QCOM_SCM
- help
- A device with "download mode" enabled will upon an unexpected
- warm-restart enter a special debug mode that allows the user to
- "download" memory content over USB for offline postmortem analysis.
- The feature can be enabled/disabled on the kernel command line.
-
- Say Y here to enable "download mode" by default.
-
config SYSFB
bool
select BOOT_VESA_SUPPORT
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 775ac68..4e8fd4e 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -20,11 +20,11 @@
#include <linux/clk.h>
#include <linux/reset-controller.h>
#include <linux/arm-smccc.h>
+#include <linux/kstrtox.h>

#include "qcom_scm.h"

-static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
-module_param(download_mode, bool, 0);
+static u32 download_mode;

#define SCM_HAS_CORE_CLK BIT(0)
#define SCM_HAS_IFACE_CLK BIT(1)
@@ -32,6 +32,7 @@ module_param(download_mode, bool, 0);

#define QCOM_DOWNLOAD_MODE_MASK 0x30
#define QCOM_DOWNLOAD_FULLDUMP 0x1
+#define QCOM_DOWNLOAD_NODUMP 0x0

struct qcom_scm {
struct device *dev;
@@ -440,8 +441,9 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
}

-static void qcom_scm_set_download_mode(bool enable)
+static void qcom_scm_set_download_mode(u32 download_mode)
{
+ bool enable = !!download_mode;
bool avail;
int ret = 0;

@@ -452,8 +454,7 @@ static void qcom_scm_set_download_mode(bool enable)
ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
} else if (__scm->dload_mode_addr) {
ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
- QCOM_DOWNLOAD_MODE_MASK,
- enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
+ QCOM_DOWNLOAD_MODE_MASK, download_mode);
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");
@@ -1421,6 +1422,49 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
return IRQ_HANDLED;
}

+
+static int get_download_mode(char *buffer, const struct kernel_param *kp)
+{
+ int len = 0;
+
+ if (download_mode == QCOM_DOWNLOAD_FULLDUMP)
+ len = sysfs_emit(buffer, "full\n");
+ else if (download_mode == QCOM_DOWNLOAD_NODUMP)
+ len = sysfs_emit(buffer, "off\n");
+
+ return len;
+}
+
+static int set_download_mode(const char *val, const struct kernel_param *kp)
+{
+ u32 old = download_mode;
+
+ if (sysfs_streq(val, "full")) {
+ download_mode = QCOM_DOWNLOAD_FULLDUMP;
+ } else if (sysfs_streq(val, "off")) {
+ download_mode = QCOM_DOWNLOAD_NODUMP;
+ } else if (kstrtouint(val, 0, &download_mode) ||
+ !(download_mode == 0 || download_mode == 1)) {
+ download_mode = old;
+ pr_err("qcom_scm: unknown download mode: %s\n", val);
+ return -EINVAL;
+ }
+
+ if (__scm)
+ qcom_scm_set_download_mode(download_mode);
+
+ return 0;
+}
+
+static const struct kernel_param_ops download_mode_param_ops = {
+ .get = get_download_mode,
+ .set = set_download_mode,
+};
+
+module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644);
+MODULE_PARM_DESC(download_mode,
+ "Download mode: off/full or 0/1 for existing users");
+
static int qcom_scm_probe(struct platform_device *pdev)
{
struct qcom_scm *scm;
@@ -1514,12 +1558,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
__get_convention();

/*
- * If requested enable "download mode", from this point on warmboot
+ * If "download mode" is requested, from this point on warmboot
* will cause the boot stages to enter download mode, unless
* disabled below by a clean shutdown/reboot.
*/
if (download_mode)
- qcom_scm_set_download_mode(true);
+ qcom_scm_set_download_mode(download_mode);

return 0;
}
@@ -1527,7 +1571,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
static void qcom_scm_shutdown(struct platform_device *pdev)
{
/* Clean shutdown, disable download mode to allow normal restart */
- qcom_scm_set_download_mode(false);
+ qcom_scm_set_download_mode(QCOM_DOWNLOAD_NODUMP);
}

static const struct of_device_id qcom_scm_dt_match[] = {
--
2.7.4

2023-05-03 17:07:57

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 16/18] firmware: scm: Modify only the download bits in TCSR register

CrashDump collection is based on the DLOAD bit of TCSR register.
To retain other bits, we read the register and modify only the
DLOAD bit as the other bits have their own significance.

Signed-off-by: Poovendhan Selvaraj <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/firmware/qcom_scm.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 003cbcb..775ac68 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -30,6 +30,9 @@ module_param(download_mode, bool, 0);
#define SCM_HAS_IFACE_CLK BIT(1)
#define SCM_HAS_BUS_CLK BIT(2)

+#define QCOM_DOWNLOAD_MODE_MASK 0x30
+#define QCOM_DOWNLOAD_FULLDUMP 0x1
+
struct qcom_scm {
struct device *dev;
struct clk *core_clk;
@@ -448,8 +451,9 @@ static void qcom_scm_set_download_mode(bool enable)
if (avail) {
ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
} else if (__scm->dload_mode_addr) {
- ret = qcom_scm_io_writel(__scm->dload_mode_addr,
- enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
+ ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
+ QCOM_DOWNLOAD_MODE_MASK,
+ enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");
--
2.7.4

2023-05-03 17:34:42

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 18/18] firmware: qcom_scm: Add multiple download mode support

Currently, scm driver only supports full dump when download
mode is selected. Add support to enable minidump as well as
enable it along with fulldump.

Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/firmware/qcom_scm.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 4e8fd4e..be7adc6 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -32,6 +32,8 @@ static u32 download_mode;

#define QCOM_DOWNLOAD_MODE_MASK 0x30
#define QCOM_DOWNLOAD_FULLDUMP 0x1
+#define QCOM_DOWNLOAD_MINIDUMP 0x2
+#define QCOM_DOWNLOAD_BOTHDUMP (QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP)
#define QCOM_DOWNLOAD_NODUMP 0x0

struct qcom_scm {
@@ -1422,13 +1424,16 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
return IRQ_HANDLED;
}

-
static int get_download_mode(char *buffer, const struct kernel_param *kp)
{
int len = 0;

if (download_mode == QCOM_DOWNLOAD_FULLDUMP)
len = sysfs_emit(buffer, "full\n");
+ else if (download_mode == QCOM_DOWNLOAD_MINIDUMP)
+ len = sysfs_emit(buffer, "mini\n");
+ else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP)
+ len = sysfs_emit(buffer, "full,mini\n");
else if (download_mode == QCOM_DOWNLOAD_NODUMP)
len = sysfs_emit(buffer, "off\n");

@@ -1439,8 +1444,12 @@ static int set_download_mode(const char *val, const struct kernel_param *kp)
{
u32 old = download_mode;

- if (sysfs_streq(val, "full")) {
+ if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) {
+ download_mode = QCOM_DOWNLOAD_BOTHDUMP;
+ } else if (sysfs_streq(val, "full")) {
download_mode = QCOM_DOWNLOAD_FULLDUMP;
+ } else if (sysfs_streq(val, "mini")) {
+ download_mode = QCOM_DOWNLOAD_MINIDUMP;
} else if (sysfs_streq(val, "off")) {
download_mode = QCOM_DOWNLOAD_NODUMP;
} else if (kstrtouint(val, 0, &download_mode) ||
@@ -1463,7 +1472,7 @@ static const struct kernel_param_ops download_mode_param_ops = {

module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644);
MODULE_PARM_DESC(download_mode,
- "Download mode: off/full or 0/1 for existing users");
+ "download mode: off/full/mini/full,mini or mini,full and 0/1 for existing users");

static int qcom_scm_probe(struct platform_device *pdev)
{
--
2.7.4

2023-05-03 17:50:04

by Mukesh Ojha

[permalink] [raw]

Subject: [PATCH v3 12/18] soc: qcom: Register pstore frontend region with minidump

Since qcom_pstore_minidump driver creates platform device
for qualcomm devices, so it knows the physical addresses
of the frontend region now. Let's register the regions
with qcom_minidump driver.

Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/soc/qcom/qcom_pstore_minidump.c | 80 ++++++++++++++++++++++++++++++++-
1 file changed, 79 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/qcom_pstore_minidump.c b/drivers/soc/qcom/qcom_pstore_minidump.c
index 8d58500..c2bba4e 100644
--- a/drivers/soc/qcom/qcom_pstore_minidump.c
+++ b/drivers/soc/qcom/qcom_pstore_minidump.c
@@ -11,6 +11,8 @@
#include <linux/pstore_ram.h>
#include <soc/qcom/qcom_minidump.h>

+#define QCOM_PSTORE_TYPE_MAX 4
+
struct qcom_ramoops_config {
unsigned long record_size;
unsigned long console_size;
@@ -24,6 +26,11 @@ struct qcom_ramoops_config {
struct qcom_ramoops_dd {
struct ramoops_platform_data qcom_ramoops_pdata;
struct platform_device *ramoops_pdev;
+ struct device *dev;
+ struct qcom_apss_minidump_region *record_region;
+ struct qcom_apss_minidump_region *console_region;
+ struct qcom_apss_minidump_region *pmsg_region;
+ struct qcom_apss_minidump_region *ftrace_region;
};

static struct qcom_ramoops_config default_ramoops_config = {
@@ -35,6 +42,64 @@ static struct qcom_ramoops_config default_ramoops_config = {
};

static struct qcom_ramoops_dd *qcom_rdd;
+
+static int
+__qcom_ramoops_minidump_region_register(struct qcom_apss_minidump_region *md_region,
+ const char *name, phys_addr_t phys_addr,
+ unsigned long size)
+{
+ int ret;
+
+ if (!size)
+ return 0;
+
+ md_region = devm_kzalloc(qcom_rdd->dev, sizeof(*md_region), GFP_KERNEL);
+ if (!md_region)
+ return -ENOMEM;
+
+ strlcpy(md_region->name, name, sizeof(md_region->name));
+ md_region->phys_addr = phys_addr;
+ md_region->virt_addr = phys_to_virt(phys_addr);
+ md_region->size = size;
+ ret = qcom_apss_minidump_region_register(md_region);
+ if (ret)
+ dev_err(qcom_rdd->dev,
+ "failed to add %s in minidump: err: %d\n", name, ret);
+
+ return ret;
+}
+
+static int
+qcom_ramoops_minidump_region_register(struct ramoops_platform_data *qcom_ramoops_data)
+{
+ phys_addr_t phys_addr;
+ int ret = 0;
+
+ phys_addr = qcom_ramoops_data->mem_address;
+ ret = __qcom_ramoops_minidump_region_register(qcom_rdd->record_region,
+ "KDMESG", phys_addr, qcom_ramoops_data->record_size);
+ if (ret)
+ return ret;
+
+ phys_addr += qcom_ramoops_data->record_size;
+ ret = __qcom_ramoops_minidump_region_register(qcom_rdd->console_region,
+ "KCONSOLE", phys_addr, qcom_ramoops_data->console_size);
+ if (ret)
+ return ret;
+
+ phys_addr += qcom_ramoops_data->console_size;
+ ret = __qcom_ramoops_minidump_region_register(qcom_rdd->pmsg_region,
+ "KPMSG", phys_addr, qcom_ramoops_data->pmsg_size);
+ if (ret)
+ return ret;
+
+ phys_addr += qcom_ramoops_data->pmsg_size;
+ ret = __qcom_ramoops_minidump_region_register(qcom_rdd->ftrace_region,
+ "KFTRACE", phys_addr, qcom_ramoops_data->ftrace_size);
+
+ return ret;
+}
+
static int qcom_ramoops_probe(struct platform_device *pdev)
{
struct device_node *of_node = pdev->dev.of_node;
@@ -59,6 +124,7 @@ static int qcom_ramoops_probe(struct platform_device *pdev)
if (!qcom_rdd)
return -ENOMEM;

+ qcom_rdd->dev = &pdev->dev;
cfg = of_device_get_match_data(&pdev->dev);
if (!cfg) {
dev_err(&pdev->dev, "failed to get supported matched data\n");
@@ -81,13 +147,25 @@ static int qcom_ramoops_probe(struct platform_device *pdev)
ret = PTR_ERR(qcom_rdd->ramoops_pdev);
dev_err(&pdev->dev, "could not create platform device: %ld\n", ret);
qcom_rdd->ramoops_pdev = NULL;
+ return ret;
}

- return ret;
+ return qcom_ramoops_minidump_region_register(pdata);
}

static int qcom_ramoops_remove(struct platform_device *pdev)
{
+ struct ramoops_platform_data *pdata;
+
+ pdata = &qcom_rdd->qcom_ramoops_pdata;
+ if (pdata->record_size)
+ qcom_apss_minidump_region_unregister(qcom_rdd->record_region);
+ if (pdata->console_size)
+ qcom_apss_minidump_region_unregister(qcom_rdd->console_region);
+ if (pdata->pmsg_size)
+ qcom_apss_minidump_region_unregister(qcom_rdd->pmsg_region);
+ if (pdata->ftrace_size)
+ qcom_apss_minidump_region_unregister(qcom_rdd->ftrace_region);
platform_device_unregister(qcom_rdd->ramoops_pdev);
qcom_rdd->ramoops_pdev = NULL;

--
2.7.4

2023-05-04 07:34:51

by Konrad Dybcio

[permalink] [raw]

Subject: Re: [PATCH v3 11/18] arm64: dts: qcom: sm8450: Add Qualcomm ramoops minidump node

On 3.05.2023 19:02, Mukesh Ojha wrote:
> This enable dynamic reserve memory for Qualcomm ramoops device,
> Which will used to save ramoops frontend data and this region
> gets dumped on crash via Qualcomm's minidump infrastructure.
> qcom_pstore_minidump is the associated driver for this node.
>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sm8450.dtsi | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 595533a..92d023f 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -614,6 +614,17 @@
> reg = <0x0 0xed900000 0x0 0x3b00000>;
> no-map;
> };
> +
> + qcom_ramoops_md_region:qcom_ramoops_md {
Missing space after ':'

node names should not contain underscores

minidump {

or

ramoops {

would probably be better names for this node
> + alloc-ranges = <0x0 0x00000000 0xffffffff 0xffffffff>;
> + size = <0x0 0x200000>;
> + no-map;
> + };
> + };
> +
> + qcom_ramoops_md {
Node names should be generic (e.g. ramdump or something) and should
not contain underscores.

Konrad
> + compatible = "qcom,sm8450-ramoops-minidump", "qcom,ramoops-minidump";
> + memory-region = <&qcom_ramoops_md_region>;
> };
>
> smp2p-adsp {

2023-05-04 11:25:23

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 10/18] dt-bindings: reserved-memory: Add qcom,ramoops-minidump binding

On 03/05/2023 19:02, Mukesh Ojha wrote:
> Qualcomm ramoops minidump logger provide a means of storing the ramoops data
> to some dynamically reserved memory instead of traditionally implemented
> ramoops where the region should be statically fixed ram region.
>
> Add qcom,ramoops-minidump binding under "/reserved-memory", and is named
> "qcom,ramoops-minidump" and the reason of naming like this is because
> as it is going to contain ramoops frontend data and this content will
> be collected via Qualcomm minidump infrastructure provided from the
> boot firmware.
>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested.
Please resend and include all necessary entries.

Without testing that's a NAK. :(

Best regards,
Krzysztof

2023-05-04 11:25:39

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 07/18] arm64: defconfig: Enable Qualcomm minidump driver

On 03/05/2023 19:02, Mukesh Ojha wrote:
> Previous patches add the Qualcomm minidump driver support, so
> lets enable minidump config so that it can be used by kernel
> clients.
>
> Signed-off-by: Mukesh Ojha <[email protected]>

This patchset is split too much. Defconfig change is one change. Not two
or three.

> ---
> arch/arm64/configs/defconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index a24609e..831c942 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -1250,6 +1250,7 @@ CONFIG_QCOM_STATS=m
> CONFIG_QCOM_WCNSS_CTRL=m
> CONFIG_QCOM_APR=m
> CONFIG_QCOM_ICC_BWMON=m
> +CONFIG_QCOM_MINIDUMP=y

This must be a module.

Best regards,
Krzysztof

2023-05-04 11:26:01

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 13/18] arm64: defconfig: Enable Qualcomm pstore minidump client driver

On 03/05/2023 19:02, Mukesh Ojha wrote:
> As we have enabled qualcomm minidump driver, so lets enable one client
> driver which captures the already existing ramoops region like record,
> console, ftrace, pmsg through qualcomm minidump infrastructure.
>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> arch/arm64/configs/defconfig | 1 +
> 1 file changed, 1 insertion(+)

Squash with other change.

>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 831c942..1ccae8b 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -1251,6 +1251,7 @@ CONFIG_QCOM_WCNSS_CTRL=m
> CONFIG_QCOM_APR=m
> CONFIG_QCOM_ICC_BWMON=m
> CONFIG_QCOM_MINIDUMP=y
> +CONFIG_QCOM_PSTORE_MINIDUMP=y

This must be a module.

Best regards,
Krzysztof

2023-05-04 11:31:52

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 11/18] arm64: dts: qcom: sm8450: Add Qualcomm ramoops minidump node

On 03/05/2023 19:02, Mukesh Ojha wrote:
> This enable dynamic reserve memory for Qualcomm ramoops device,
> Which will used to save ramoops frontend data and this region
> gets dumped on crash via Qualcomm's minidump infrastructure.
> qcom_pstore_minidump is the associated driver for this node.
>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sm8450.dtsi | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 595533a..92d023f 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -614,6 +614,17 @@
> reg = <0x0 0xed900000 0x0 0x3b00000>;
> no-map;
> };
> +
> + qcom_ramoops_md_region:qcom_ramoops_md {

No underscores in node names. Didn't we talk about this?

Best regards,
Krzysztof

2023-05-04 11:33:42

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 00/18] Add basic Minidump kernel driver support

On 03/05/2023 19:02, Mukesh Ojha wrote:
> Minidump is a best effort mechanism to collect useful and predefined data
> for first level of debugging on end user devices running on Qualcomm SoCs.
> It is built on the premise that System on Chip (SoC) or subsystem part of
> SoC crashes, due to a range of hardware and software bugs. Hence, the
> ability to collect accurate data is only a best-effort. The data collected
> could be invalid or corrupted, data collection itself could fail, and so on.

You organized the patch in a way making it very hard for us to review. I
see mixed remoteproc, then soc, then defconfig (!!!), then remote proc,
then soc, then bindings (! they must be before usage...), then dts
(which should be the last), then soc then dts then... You see the point.

Bindings, docs, changes organized by subsystem. Then DTS as separate
patchset with a link to this one. If you have bisectability issues then
it's a hint something is wrongly organized or done and must be fixed anyway.

Best regards,
Krzysztof

2023-05-04 11:45:35

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 02/18] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

On 03/05/2023 19:02, Mukesh Ojha wrote:
> Move minidump specific data types and macros to a separate internal
> header(qcom_minidump.h) so that it can be shared among different
> Qualcomm drivers.

No, this is not internal header. You moved it to global header.

There is no reason driver internals should be exposed to other unrelated
subsystems.

>
> There is no change in functional behavior after this.

It is. You made all these internal symbols available to others.

>

This comes without justification why other drivers needs to access
private and internal data. It does not look correct design. NAK.

Best regards,
Krzysztof

2023-05-04 11:46:11

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 04/18] soc: qcom: Add Qualcomm minidump kernel driver

On 03/05/2023 19:02, Mukesh Ojha wrote:
> Minidump is a best effort mechanism to collect useful and predefined
> data for first level of debugging on end user devices running on
> Qualcomm SoCs. It is built on the premise that System on Chip (SoC)
> or subsystem part of SoC crashes, due to a range of hardware and
> software bugs. Hence, the ability to collect accurate data is only
> a best-effort. The data collected could be invalid or corrupted,
> data collection itself could fail, and so on.
>
> Qualcomm devices in engineering mode provides a mechanism for
> generating full system ramdumps for post mortem debugging. But in some
> cases it's however not feasible to capture the entire content of RAM.
> The minidump mechanism provides the means for selecting region should
> be included in the ramdump. The solution supports extracting the
> ramdump/minidump produced either over USB or stored to an attached
> storage device.
>
> The core of minidump feature is part of Qualcomm's boot firmware code.
> It initializes shared memory(SMEM), which is a part of DDR and
> allocates a small section of it to minidump table i.e also called
> global table of content (G-ToC). Each subsystem (APSS, ADSP, ...) has
> their own table of segments to be included in the minidump, all
> references from a descriptor in SMEM (G-ToC). Each segment/region has
> some details like name, physical address and it's size etc. and it
> could be anywhere scattered in the DDR.
>
> Minidump kernel driver adds the capability to add linux region to be
> dumped as part of ram dump collection. It provides appropriate symbol
> to check its enablement and register client regions.
>
> To simplify post mortem debugging, it creates and maintain an ELF
> header as first region that gets updated upon registration
> of a new region.
>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> drivers/soc/qcom/Kconfig | 14 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom_minidump.c | 581 +++++++++++++++++++++++++++++++++++++++
> drivers/soc/qcom/smem.c | 8 +
> include/soc/qcom/qcom_minidump.h | 61 +++-
> 5 files changed, 663 insertions(+), 2 deletions(-)
> create mode 100644 drivers/soc/qcom/qcom_minidump.c
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a491718..15c931e 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -279,4 +279,18 @@ config QCOM_INLINE_CRYPTO_ENGINE
> tristate
> select QCOM_SCM
>
> +config QCOM_MINIDUMP
> + tristate "QCOM Minidump Support"
> + depends on ARCH_QCOM || COMPILE_TEST
> + select QCOM_SMEM
> + help
> + Enablement of core minidump feature is controlled from boot firmware
> + side, and this config allow linux to query and manages APPS minidump
> + table.
> +
> + Client drivers can register their internal data structures and debug
> + messages as part of the minidump region and when the SoC is crashed,
> + these selective regions will be dumped instead of the entire DDR.
> + This saves significant amount of time and/or storage space.
> +
> endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 0f43a88..1ebe081 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -33,3 +33,4 @@ obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += ice.o
> +obj-$(CONFIG_QCOM_MINIDUMP) += qcom_minidump.o
> diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
> new file mode 100644
> index 0000000..d107a86
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom_minidump.c
> @@ -0,0 +1,581 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/elf.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/string.h>
> +#include <linux/soc/qcom/smem.h>
> +#include <soc/qcom/qcom_minidump.h>
> +
> +/**
> + * struct minidump_elfhdr - Minidump table elf header
> + * @ehdr: Elf main header
> + * @shdr: Section header
> + * @phdr: Program header
> + * @elf_offset: Section offset in elf
> + * @strtable_idx: String table current index position
> + */
> +struct minidump_elfhdr {
> + struct elfhdr *ehdr;
> + struct elf_shdr *shdr;
> + struct elf_phdr *phdr;
> + size_t elf_offset;
> + size_t strtable_idx;
> +};
> +
> +/**
> + * struct minidump - Minidump driver private data
> + * @md_gbl_toc : Global TOC pointer
> + * @md_apss_toc : Application Subsystem TOC pointer
> + * @md_regions : High level OS region base pointer
> + * @elf : Minidump elf header
> + * @dev : Minidump device
> + */
> +struct minidump {
> + struct minidump_global_toc *md_gbl_toc;
> + struct minidump_subsystem *md_apss_toc;
> + struct minidump_region *md_regions;
> + struct minidump_elfhdr elf;
> + struct device *dev;
> +};
> +
> +/*
> + * In some of the Old Qualcomm devices, boot firmware statically allocates 300
> + * as total number of supported region (including all co-processors) in
> + * minidump table out of which linux was using 201. In future, this limitation
> + * from boot firmware might get removed by allocating the region dynamically.
> + * So, keep it compatible with older devices, we can keep the current limit for
> + * Linux to 201.
> + */
> +#define MAX_NUM_ENTRIES 201
> +#define MAX_STRTBL_SIZE (MAX_NUM_ENTRIES * MAX_REGION_NAME_LENGTH)
> +
> +static struct minidump *__md;

No, no file scope or global scope statics.

> +static DEFINE_MUTEX(minidump_lock);

Neither this.

Also you need to clearly express what is protected by newn lock.

> +
> +static struct elf_shdr *elf_shdr_entry_addr(struct elfhdr *ehdr, int idx)
> +{
> + struct elf_shdr *eshdr = (struct elf_shdr *)((size_t)ehdr + ehdr->e_shoff);
> +
> + return &eshdr[idx];
> +}
> +
> +static struct elf_phdr *elf_phdr_entry_addr(struct elfhdr *ehdr, int idx)
> +{
> + struct elf_phdr *ephdr = (struct elf_phdr *)((size_t)ehdr + ehdr->e_phoff);
> +
> + return &ephdr[idx];
> +}
> +
> +static char *elf_str_table_start(struct elfhdr *ehdr)
> +{
> + struct elf_shdr *eshdr;
> +
> + if (ehdr->e_shstrndx == SHN_UNDEF)
> + return NULL;
> +
> + eshdr = elf_shdr_entry_addr(ehdr, ehdr->e_shstrndx);
> + return (char *)ehdr + eshdr->sh_offset;
> +}
> +
> +static char *elf_lookup_string(struct elfhdr *ehdr, int offset)
> +{
> + char *strtab = elf_str_table_start(ehdr);
> +
> + if (!strtab || (__md->elf.strtable_idx < offset))
> + return NULL;
> +
> + return strtab + offset;
> +}
> +
> +static unsigned int append_str_to_strtable(const char *name)
> +{
> + char *strtab = elf_str_table_start(__md->elf.ehdr);
> + unsigned int old_idx = __md->elf.strtable_idx;
> + unsigned int ret;
> +
> + if (!strtab || !name)
> + return 0;
> +
> + ret = old_idx;
> + old_idx += strscpy((strtab + old_idx), name, MAX_REGION_NAME_LENGTH);
> + __md->elf.strtable_idx = old_idx + 1;
> + return ret;
> +}
> +
> +static int
> +get_apss_minidump_region_index(const struct qcom_apss_minidump_region *region)
> +{
> + struct minidump_region *mdr;
> + unsigned int i;
> + unsigned int count;
> +
> + count = le32_to_cpu(__md->md_apss_toc->region_count);
> + for (i = 0; i < count; i++) {
> + mdr = &__md->md_regions[i];
> + if (!strcmp(mdr->name, region->name))
> + return i;
> + }
> + return -ENOENT;
> +}
> +
> +static void
> +qcom_apss_minidump_update_elf_header(const struct qcom_apss_minidump_region *region)

You need to name everything shorter. Neither functions nor structs are
making it easy to follow.

> +{
> + struct elfhdr *ehdr = __md->elf.ehdr;
> + struct elf_shdr *shdr;
> + struct elf_phdr *phdr;
> +
> + shdr = elf_shdr_entry_addr(ehdr, ehdr->e_shnum++);
> + phdr = elf_phdr_entry_addr(ehdr, ehdr->e_phnum++);
> +
> + shdr->sh_type = SHT_PROGBITS;
> + shdr->sh_name = append_str_to_strtable(region->name);
> + shdr->sh_addr = (elf_addr_t)region->virt_addr;
> + shdr->sh_size = region->size;
> + shdr->sh_flags = SHF_WRITE;
> + shdr->sh_offset = __md->elf.elf_offset;
> + shdr->sh_entsize = 0;
> +
> + phdr->p_type = PT_LOAD;
> + phdr->p_offset = __md->elf.elf_offset;
> + phdr->p_vaddr = (elf_addr_t)region->virt_addr;
> + phdr->p_paddr = region->phys_addr;
> + phdr->p_filesz = phdr->p_memsz = region->size;
> + phdr->p_flags = PF_R | PF_W;
> + __md->elf.elf_offset += shdr->sh_size;
> +}
> +
> +static void
> +qcom_apss_minidump_add_region(const struct qcom_apss_minidump_region *region)
> +{
> + struct minidump_region *mdr;
> + unsigned int region_cnt = le32_to_cpu(__md->md_apss_toc->region_count);
> +
> + mdr = &__md->md_regions[region_cnt];
> + strscpy(mdr->name, region->name, sizeof(mdr->name));
> + mdr->address = cpu_to_le64(region->phys_addr);
> + mdr->size = cpu_to_le64(region->size);
> + mdr->valid = cpu_to_le32(MINIDUMP_REGION_VALID);
> + region_cnt++;
> + __md->md_apss_toc->region_count = cpu_to_le32(region_cnt);
> +}
> +
> +static bool
> +qcom_apss_minidump_valid_region(const struct qcom_apss_minidump_region *region)
> +{
> + return region &&
> + strnlen(region->name, MAX_NAME_LENGTH) < MAX_NAME_LENGTH &&
> + region->virt_addr &&
> + region->size &&
> + IS_ALIGNED(region->size, 4);
> +}
> +
> +static int qcom_apss_minidump_add_elf_header(void)
> +{
> + struct qcom_apss_minidump_region elfregion;
> + struct elfhdr *ehdr;
> + struct elf_shdr *shdr;
> + struct elf_phdr *phdr;
> + unsigned int elfh_size;
> + unsigned int strtbl_off;
> + unsigned int phdr_off;
> + char *banner;
> + unsigned int banner_len;
> +
> + banner_len = strlen(linux_banner);
> + /*
> + * Header buffer contains:
> + * ELF header, (MAX_NUM_ENTRIES + 4) of Section and Program ELF headers,
> + * where, 4 additional entries, one for empty header, one for string table
> + * one for minidump table and one for linux banner.
> + *
> + * Linux banner is stored in minidump to aid post mortem tools to determine
> + * the kernel version.
> + */
> + elfh_size = sizeof(*ehdr);
> + elfh_size += MAX_STRTBL_SIZE;
> + elfh_size += banner_len + 1;
> + elfh_size += ((sizeof(*shdr) + sizeof(*phdr)) * (MAX_NUM_ENTRIES + 4));
> + elfh_size = ALIGN(elfh_size, 4);
> +
> + __md->elf.ehdr = kzalloc(elfh_size, GFP_KERNEL);
> + if (!__md->elf.ehdr)
> + return -ENOMEM;
> +
> + /* Register ELF header as first region */
> + strscpy(elfregion.name, "KELF_HEADER", sizeof(elfregion.name));
> + elfregion.virt_addr = __md->elf.ehdr;
> + elfregion.phys_addr = virt_to_phys(__md->elf.ehdr);
> + elfregion.size = elfh_size;
> + qcom_apss_minidump_add_region(&elfregion);
> +
> + ehdr = __md->elf.ehdr;
> + /* Assign Section/Program headers offset */
> + __md->elf.shdr = shdr = (struct elf_shdr *)(ehdr + 1);
> + __md->elf.phdr = phdr = (struct elf_phdr *)(shdr + MAX_NUM_ENTRIES);
> + phdr_off = sizeof(*ehdr) + (sizeof(*shdr) * MAX_NUM_ENTRIES);
> +
> + memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
> + ehdr->e_ident[EI_CLASS] = ELF_CLASS;
> + ehdr->e_ident[EI_DATA] = ELF_DATA;
> + ehdr->e_ident[EI_VERSION] = EV_CURRENT;
> + ehdr->e_ident[EI_OSABI] = ELF_OSABI;
> + ehdr->e_type = ET_CORE;
> + ehdr->e_machine = ELF_ARCH;
> + ehdr->e_version = EV_CURRENT;
> + ehdr->e_ehsize = sizeof(*ehdr);
> + ehdr->e_phoff = phdr_off;
> + ehdr->e_phentsize = sizeof(*phdr);
> + ehdr->e_shoff = sizeof(*ehdr);
> + ehdr->e_shentsize = sizeof(*shdr);
> + ehdr->e_shstrndx = 1;
> +
> + __md->elf.elf_offset = elfh_size;
> +
> + /*
> + * The zeroth index of the section header is reserved and is rarely used.
> + * Set the section header as null (SHN_UNDEF) and move to the next one.
> + * 2nd Section is String table.
> + */
> + __md->elf.strtable_idx = 1;
> + strtbl_off = sizeof(*ehdr) + ((sizeof(*phdr) + sizeof(*shdr)) * MAX_NUM_ENTRIES);
> + shdr++;
> + shdr->sh_type = SHT_STRTAB;
> + shdr->sh_offset = (elf_addr_t)strtbl_off;
> + shdr->sh_size = MAX_STRTBL_SIZE;
> + shdr->sh_entsize = 0;
> + shdr->sh_flags = 0;
> + shdr->sh_name = append_str_to_strtable("STR_TBL");
> + shdr++;
> +
> + /* 3rd Section is Linux banner */
> + banner = (char *)ehdr + strtbl_off + MAX_STRTBL_SIZE;
> + memcpy(banner, linux_banner, banner_len);
> +
> + shdr->sh_type = SHT_PROGBITS;
> + shdr->sh_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
> + shdr->sh_size = banner_len + 1;
> + shdr->sh_addr = (elf_addr_t)linux_banner;
> + shdr->sh_entsize = 0;
> + shdr->sh_flags = SHF_WRITE;
> + shdr->sh_name = append_str_to_strtable("linux_banner");
> +
> + phdr->p_type = PT_LOAD;
> + phdr->p_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
> + phdr->p_vaddr = (elf_addr_t)linux_banner;
> + phdr->p_paddr = virt_to_phys(linux_banner);
> + phdr->p_filesz = phdr->p_memsz = banner_len + 1;
> + phdr->p_flags = PF_R | PF_W;
> +
> + /*
> + * Above are some prdefined sections/program header used
> + * for debug, update their count here.
> + */
> + ehdr->e_phnum = 1;
> + ehdr->e_shnum = 3;
> +
> + return 0;
> +}
> +
> +/**
> + * qcom_minidump_subsystem_desc() - Get minidump subsystem descriptor.
> + * @minidump_index: minidump index for a subsystem in minidump table
> + *
> + * Return: minidump subsystem descriptor address on success and error
> + * on failure
> + */
> +struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index)
> +{
> + struct minidump_subsystem *md_ss_toc;
> +
> + mutex_lock(&minidump_lock);
> + if (!__md) {
> + md_ss_toc = ERR_PTR(-EPROBE_DEFER);
> + goto unlock;
> + }
> +
> + md_ss_toc = &__md->md_gbl_toc->subsystems[minidump_index];
> +unlock:
> + mutex_unlock(&minidump_lock);
> + return md_ss_toc;
> +}
> +EXPORT_SYMBOL_GPL(qcom_minidump_subsystem_desc);
> +
> +/**
> + * qcom_apss_minidump_region_register() - Register a region in Minidump table.
> + * @region: minidump region.
> + *
> + * Return: On success, it returns 0, otherwise a negative error value on failure.
> + */
> +int qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region)
> +{
> + unsigned int num_region;
> + int ret;
> +
> + if (!__md)
> + return -EPROBE_DEFER;
> +
> + if (!qcom_apss_minidump_valid_region(region))
> + return -EINVAL;
> +
> + mutex_lock(&minidump_lock);
> + ret = get_apss_minidump_region_index(region);
> + if (ret >= 0) {
> + dev_info(__md->dev, "%s region is already registered\n", region->name);
> + ret = -EEXIST;
> + goto unlock;
> + }
> +
> + /* Check if there is a room for a new entry */
> + num_region = le32_to_cpu(__md->md_apss_toc->region_count);
> + if (num_region >= MAX_NUM_ENTRIES) {
> + dev_err(__md->dev, "maximum region limit %u reached\n", num_region);
> + ret = -ENOSPC;
> + goto unlock;
> + }
> +
> + qcom_apss_minidump_add_region(region);
> + qcom_apss_minidump_update_elf_header(region);
> + ret = 0;
> +unlock:
> + mutex_unlock(&minidump_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_apss_minidump_region_register);
> +
> +static int
> +qcom_apss_minidump_clear_header(const struct qcom_apss_minidump_region *region)
> +{
> + struct elfhdr *ehdr = __md->elf.ehdr;
> + struct elf_shdr *shdr;
> + struct elf_shdr *tmp_shdr;
> + struct elf_phdr *phdr;
> + struct elf_phdr *tmp_phdr;
> + unsigned int phidx;
> + unsigned int shidx;
> + unsigned int len;
> + unsigned int i;
> + char *shname;
> +
> + for (i = 0; i < ehdr->e_phnum; i++) {
> + phdr = elf_phdr_entry_addr(ehdr, i);
> + if (phdr->p_paddr == region->phys_addr &&
> + phdr->p_memsz == region->size)
> + break;
> + }
> +
> + if (i == ehdr->e_phnum) {
> + dev_err(__md->dev, "Cannot find program header entry in elf\n");
> + return -EINVAL;
> + }
> +
> + phidx = i;
> + for (i = 0; i < ehdr->e_shnum; i++) {
> + shdr = elf_shdr_entry_addr(ehdr, i);
> + shname = elf_lookup_string(ehdr, shdr->sh_name);
> + if (shname && !strcmp(shname, region->name) &&
> + shdr->sh_addr == (elf_addr_t)region->virt_addr &&
> + shdr->sh_size == region->size)
> + break;
> + }
> +
> + if (i == ehdr->e_shnum) {
> + dev_err(__md->dev, "Cannot find section header entry in elf\n");
> + return -EINVAL;
> + }
> +
> + shidx = i;
> + if (shdr->sh_offset != phdr->p_offset) {
> + dev_err(__md->dev, "Invalid entry details for region: %s\n", region->name);
> + return -EINVAL;
> + }
> +
> + /* Clear name in string table */
> + len = strlen(shname) + 1;
> + memmove(shname, shname + len,
> + __md->elf.strtable_idx - shdr->sh_name - len);
> + __md->elf.strtable_idx -= len;
> +
> + /* Clear program header */
> + tmp_phdr = elf_phdr_entry_addr(ehdr, phidx);
> + for (i = phidx; i < ehdr->e_phnum - 1; i++) {
> + tmp_phdr = elf_phdr_entry_addr(ehdr, i + 1);
> + phdr = elf_phdr_entry_addr(ehdr, i);
> + memcpy(phdr, tmp_phdr, sizeof(struct elf_phdr));
> + phdr->p_offset = phdr->p_offset - region->size;
> + }
> + memset(tmp_phdr, 0, sizeof(struct elf_phdr));
> + ehdr->e_phnum--;
> +
> + /* Clear section header */
> + tmp_shdr = elf_shdr_entry_addr(ehdr, shidx);
> + for (i = shidx; i < ehdr->e_shnum - 1; i++) {
> + tmp_shdr = elf_shdr_entry_addr(ehdr, i + 1);
> + shdr = elf_shdr_entry_addr(ehdr, i);
> + memcpy(shdr, tmp_shdr, sizeof(struct elf_shdr));
> + shdr->sh_offset -= region->size;
> + shdr->sh_name -= len;
> + }
> +
> + memset(tmp_shdr, 0, sizeof(struct elf_shdr));
> + ehdr->e_shnum--;
> + __md->elf.elf_offset -= region->size;
> +
> + return 0;
> +}
> +
> +/**
> + * qcom_apss_minidump_region_unregister() - Unregister region from Minidump table.
> + * @region: minidump region.
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +int qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region)
> +{
> + struct minidump_region *mdr;
> + unsigned int num_region;
> + unsigned int idx;
> + int ret;
> +
> + if (!region)
> + return -EINVAL;
> +
> + mutex_lock(&minidump_lock);
> + if (!__md) {
> + ret = -EPROBE_DEFER;
> + goto unlock;
> + }
> +
> + idx = get_apss_minidump_region_index(region);
> + if (idx < 0) {
> + dev_err(__md->dev, "%s region is not present\n", region->name);
> + ret = idx;
> + goto unlock;
> + }
> +
> + mdr = &__md->md_regions[0];
> + num_region = le32_to_cpu(__md->md_apss_toc->region_count);
> + /*
> + * Left shift all the regions exist after this removed region
> + * index by 1 to fill the gap and zero out the last region
> + * present at the end.
> + */
> + memmove(&mdr[idx], &mdr[idx + 1],
> + (num_region - idx - 1) * sizeof(struct minidump_region));
> + memset(&mdr[num_region - 1], 0, sizeof(struct minidump_region));
> + ret = qcom_apss_minidump_clear_header(region);
> + if (ret) {
> + dev_err(__md->dev, "Failed to remove region: %s\n", region->name);
> + goto unlock;
> + }
> +
> + num_region--;
> + __md->md_apss_toc->region_count = cpu_to_le32(num_region);
> +unlock:
> + mutex_unlock(&minidump_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_apss_minidump_region_unregister);
> +
> +static int qcom_minidump_init_apss_subsystem(struct minidump *md)
> +{
> + struct minidump_subsystem *apsstoc;
> +
> + apsstoc = &md->md_gbl_toc->subsystems[MINIDUMP_APSS_DESC];
> + md->md_regions = devm_kcalloc(md->dev, MAX_NUM_ENTRIES,
> + sizeof(struct minidump_region), GFP_KERNEL);
> + if (!md->md_regions)
> + return -ENOMEM;
> +
> + md->md_apss_toc = apsstoc;
> + apsstoc->regions_baseptr = cpu_to_le64(virt_to_phys(md->md_regions));
> + apsstoc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
> + apsstoc->status = cpu_to_le32(1);
> + apsstoc->region_count = cpu_to_le32(0);
> +
> + /* Tell bootloader not to encrypt the regions of this subsystem */
> + apsstoc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
> + apsstoc->encryption_required = cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
> +
> + return 0;
> +}
> +
> +static int qcom_minidump_probe(struct platform_device *pdev)
> +{
> + struct minidump_global_toc *mdgtoc;
> + struct minidump *md;
> + size_t size;
> + int ret;
> +
> + md = devm_kzalloc(&pdev->dev, sizeof(*md), GFP_KERNEL);
> + if (!md)
> + return -ENOMEM;
> +
> + mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size);
> + if (IS_ERR(mdgtoc)) {
> + ret = PTR_ERR(mdgtoc);
> + dev_err(&pdev->dev, "Couldn't find minidump smem item: %d\n", ret);
> + return ret;
> + }
> +
> + if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
> + ret = -EINVAL;
> + dev_err(&pdev->dev, "minidump table is not initialized: %d\n", ret);
> + return ret;
> + }
> +
> + mutex_lock(&minidump_lock);
> + md->dev = &pdev->dev;
> + md->md_gbl_toc = mdgtoc;

What are you protecting here? It's not possible to have concurrent
access to md, is it?

> + ret = qcom_minidump_init_apss_subsystem(md);
> + if (ret) {
> + dev_err(&pdev->dev, "apss minidump initialization failed: %d\n", ret);
> + goto unlock;
> + }
> +
> + __md = md;

No. This is a platform device, so it can have multiple instances.

> + /* First entry would be ELF header */
> + ret = qcom_apss_minidump_add_elf_header();
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to add elf header: %d\n", ret);
> + memset(md->md_apss_toc, 0, sizeof(struct minidump_subsystem));
> + __md = NULL;
> + }
> +
> +unlock:
> + mutex_unlock(&minidump_lock);
> + return ret;
> +}
> +
> +static int qcom_minidump_remove(struct platform_device *pdev)
> +{
> + memset(__md->md_apss_toc, 0, sizeof(struct minidump_subsystem));
> + __md = NULL;

Don't use __ in variable names. Drop it everywhere.

> +
> + return 0;
> +}
> +
> +static struct platform_driver qcom_minidump_driver = {
> + .probe = qcom_minidump_probe,
> + .remove = qcom_minidump_remove,
> + .driver = {
> + .name = "qcom-minidump",
> + },
> +};
> +
> +module_platform_driver(qcom_minidump_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm APSS minidump driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom-minidump");
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 6be7ea9..d459656 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -279,6 +279,7 @@ struct qcom_smem {
>
> u32 item_count;
> struct platform_device *socinfo;
> + struct platform_device *minidump;
> struct smem_ptable *ptable;
> struct smem_partition global_partition;
> struct smem_partition partitions[SMEM_HOST_COUNT];
> @@ -1151,12 +1152,19 @@ static int qcom_smem_probe(struct platform_device *pdev)
> if (IS_ERR(smem->socinfo))
> dev_dbg(&pdev->dev, "failed to register socinfo device\n");
>
> + smem->minidump = platform_device_register_data(&pdev->dev, "qcom-minidump",
> + PLATFORM_DEVID_NONE, NULL,
> + 0);
> + if (IS_ERR(smem->minidump))
> + dev_dbg(&pdev->dev, "failed to register minidump device\n");
> +
> return 0;
> }
>
> static int qcom_smem_remove(struct platform_device *pdev)
> {
> platform_device_unregister(__smem->socinfo);
> + platform_device_unregister(__smem->minidump);

Wrong order. You registered first socinfo, right?

>
> hwspin_lock_free(__smem->hwlock);
> __smem = NULL;
> diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
> index 84c8605..1872668 100644
> --- a/include/soc/qcom/qcom_minidump.h
> +++ b/include/soc/qcom/qcom_minidump.h
> @@ -1,6 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> /*
> - * Qualcomm minidump shared data structures and macros
> + * This file contain Qualcomm minidump data structures and macros shared with
> + * boot firmware and also apss minidump client's data structure
> *
> * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> */
> @@ -9,12 +10,27 @@
> #define _QCOM_MINIDUMP_H_
>
> #define MAX_NUM_OF_SS 10
> +#define MAX_NAME_LENGTH 12
> #define MAX_REGION_NAME_LENGTH 16
> +
> +#define MINIDUMP_REVISION 1
> #define SBL_MINIDUMP_SMEM_ID 602
> +
> +/* Application processor minidump descriptor */
> +#define MINIDUMP_APSS_DESC 0
> +#define SMEM_ENTRY_SIZE 40
> +
> #define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
> +#define MINIDUMP_REGION_INVALID ('I' << 24 | 'N' << 16 | 'V' << 8 | 'A' << 0)
> +#define MINIDUMP_REGION_INIT ('I' << 24 | 'N' << 16 | 'I' << 8 | 'T' << 0)
> +#define MINIDUMP_REGION_NOINIT 0
> +
> +#define MINIDUMP_SS_ENCR_REQ (0 << 24 | 'Y' << 16 | 'E' << 8 | 'S' << 0)
> +#define MINIDUMP_SS_ENCR_NOTREQ (0 << 24 | 0 << 16 | 'N' << 8 | 'R' << 0)
> +#define MINIDUMP_SS_ENCR_NONE ('N' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
> #define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
> +#define MINIDUMP_SS_ENCR_START ('S' << 24 | 'T' << 16 | 'R' << 8 | 'T' << 0)
> #define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
> -

Why removing this?

> /**
> * struct minidump_region - Minidump region
> * @name : Name of the region to be dumped
> @@ -63,4 +79,45 @@ struct minidump_global_toc {
> struct minidump_subsystem subsystems[MAX_NUM_OF_SS];
> };
>
> +/**
> + * struct qcom_apss_minidump_region - APSS Minidump region information
> + *
> + * @name: Entry name, Minidump will dump binary with this name.
> + * @virt_addr: Virtual address of the entry.
> + * @phys_addr: Physical address of the entry to dump.
> + * @size: Number of byte to dump from @address location,
> + * and it should be 4 byte aligned.
> + */
> +struct qcom_apss_minidump_region {
> + char name[MAX_NAME_LENGTH];
> + void *virt_addr;
> + phys_addr_t phys_addr;
> + size_t size;
> +};

You expose way too much internals in global header.

> +
> +#if IS_ENABLED(CONFIG_QCOM_MINIDUMP)
> +extern struct minidump_subsystem *

No externs.

The header is unreadable.

> +qcom_minidump_subsystem_desc(unsigned int minidump_index);
> +extern int
> +qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region);
> +extern int
> +qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region);

Blank line

> +#else

Blank line

> +static inline
> +struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index)
> +{
> + return NULL;
> +}

Blank line

> +static inline int
> +qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region)
> +{
> + /* Return quietly, if minidump is not enabled */
> + return 0;
> +}

> +static inline int
> +qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region)
> +{
> + return 0;
> +}

> +#endif

/* CONFIG_QCOM_MINIDUMP */

> #endif /* _QCOM_MINIDUMP_H_ */

Best regards,
Krzysztof

2023-05-04 11:46:54

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 06/18] soc: qcom: minidump: Add update region support

On 03/05/2023 19:02, Mukesh Ojha wrote:
> Add support to update client's region physical/virtual addresses,
> which is useful for dynamic loadable modules, dynamic address
> changing clients like if we want to collect current stack
> information for each core and the current stack is changing on
> each sched_switch event, So here virtual/physical address of
> the current stack is changing. So, to cover such use cases
> add the update region support in minidump driver.
>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> drivers/soc/qcom/qcom_minidump.c | 57 ++++++++++++++++++++++++++++++++++++++++
> include/soc/qcom/qcom_minidump.h | 7 +++++
> 2 files changed, 64 insertions(+)
>
> diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
> index 6d29371..853bdda 100644
> --- a/drivers/soc/qcom/qcom_minidump.c
> +++ b/drivers/soc/qcom/qcom_minidump.c
> @@ -561,6 +561,63 @@ int qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region
> }
> EXPORT_SYMBOL_GPL(qcom_apss_minidump_region_unregister);
>
> +/**
> + * qcom_apss_minidump_update_region() - Update region in APSS minidump table.

Description of the function is exact copy of function name. This is a
hint for two things:
1. Your function has wrong name.
2. Your description is not helping.

Best regards,
Krzysztof

2023-05-04 11:52:12

by Mukesh Ojha

[permalink] [raw]

Subject: Re: [PATCH v3 07/18] arm64: defconfig: Enable Qualcomm minidump driver

On 5/4/2023 4:53 PM, Krzysztof Kozlowski wrote:
> On 03/05/2023 19:02, Mukesh Ojha wrote:
>> Previous patches add the Qualcomm minidump driver support, so
>> lets enable minidump config so that it can be used by kernel
>> clients.
>>
>> Signed-off-by: Mukesh Ojha <[email protected]>
>
> This patchset is split too much. Defconfig change is one change. Not two
> or three.
>
>> ---
>> arch/arm64/configs/defconfig | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>> index a24609e..831c942 100644
>> --- a/arch/arm64/configs/defconfig
>> +++ b/arch/arm64/configs/defconfig
>> @@ -1250,6 +1250,7 @@ CONFIG_QCOM_STATS=m
>> CONFIG_QCOM_WCNSS_CTRL=m
>> CONFIG_QCOM_APR=m
>> CONFIG_QCOM_ICC_BWMON=m
>> +CONFIG_QCOM_MINIDUMP=y
>
> This must be a module.

Why do you think this should be a module ?

Is it because, it is lying here among others '=m' ?

Or you have some other reasoning ? like it is for qcom specific
soc and can not be used outside ? but that is not true for
all configs mentioned here.

The reason behind making it as '=y' was, to collect information from
core kernel data structure as well as the information like percpu data,
run queue, irq stat kind of information on kernel crash on a target
running some perf configuration(android phone).

-- Mukesh

>
> Best regards,
> Krzysztof
>

2023-05-04 12:04:21

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 02/18] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

On 04/05/2023 13:58, Mukesh Ojha wrote:
>
>
> On 5/4/2023 5:08 PM, Krzysztof Kozlowski wrote:
>> On 03/05/2023 19:02, Mukesh Ojha wrote:
>>> Move minidump specific data types and macros to a separate internal
>>> header(qcom_minidump.h) so that it can be shared among different
>>> Qualcomm drivers.
>>
>> No, this is not internal header. You moved it to global header.
>>
>> There is no reason driver internals should be exposed to other unrelated
>> subsystems.
>>
>>>
>>> There is no change in functional behavior after this.
>>
>> It is. You made all these internal symbols available to others.
>>
>>>
>>
>> This comes without justification why other drivers needs to access
>> private and internal data. It does not look correct design. NAK.
>
> Thanks for catching outdated commit text, will fix the commit with
> more descriptive reasoning.
>
> It has to be global so that co-processor minidump and apss minidump can
> share data structure and they are lying in different directory.
>

Then you should not share all the internals of memory layout but only
few pieces necessary to talk with minidump driver. The minidump driver
should organize everything how it wants.

Best regards,
Krzysztof

2023-05-04 12:10:10

by Mukesh Ojha

[permalink] [raw]

Subject: Re: [PATCH v3 02/18] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

On 5/4/2023 5:08 PM, Krzysztof Kozlowski wrote:
> On 03/05/2023 19:02, Mukesh Ojha wrote:
>> Move minidump specific data types and macros to a separate internal
>> header(qcom_minidump.h) so that it can be shared among different
>> Qualcomm drivers.
>
> No, this is not internal header. You moved it to global header.
>
> There is no reason driver internals should be exposed to other unrelated
> subsystems.
>
>>
>> There is no change in functional behavior after this.
>
> It is. You made all these internal symbols available to others.
>
>>
>
> This comes without justification why other drivers needs to access
> private and internal data. It does not look correct design. NAK.

Thanks for catching outdated commit text, will fix the commit with
more descriptive reasoning.

It has to be global so that co-processor minidump and apss minidump can
share data structure and they are lying in different directory.

-Mukesh

>
> Best regards,
> Krzysztof
>

2023-05-04 12:37:04

by Mukesh Ojha

[permalink] [raw]

Subject: Re: [PATCH v3 02/18] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

On 5/4/2023 5:33 PM, Krzysztof Kozlowski wrote:
> On 04/05/2023 13:58, Mukesh Ojha wrote:
>>
>>
>> On 5/4/2023 5:08 PM, Krzysztof Kozlowski wrote:
>>> On 03/05/2023 19:02, Mukesh Ojha wrote:
>>>> Move minidump specific data types and macros to a separate internal
>>>> header(qcom_minidump.h) so that it can be shared among different
>>>> Qualcomm drivers.
>>>
>>> No, this is not internal header. You moved it to global header.
>>>
>>> There is no reason driver internals should be exposed to other unrelated
>>> subsystems.
>>>
>>>>
>>>> There is no change in functional behavior after this.
>>>
>>> It is. You made all these internal symbols available to others.
>>>
>>>>
>>>
>>> This comes without justification why other drivers needs to access
>>> private and internal data. It does not look correct design. NAK.
>>
>> Thanks for catching outdated commit text, will fix the commit with
>> more descriptive reasoning.
>>
>> It has to be global so that co-processor minidump and apss minidump can
>> share data structure and they are lying in different directory.
>>
>
> Then you should not share all the internals of memory layout but only
> few pieces necessary to talk with minidump driver. The minidump driver
> should organize everything how it wants.

These are core data structure which is shared with boot firmware and the
one's are moved here all are required by minidump driver .

If you follow here[1], i raised by concern to make this particular one's
as private and later to avoid confusion went with single header.
But if others agree, I will keep the one that get shared with minidump
as separate one or if relative path of headers are allowed that can make
it private between these drivers(which i don't think, will be allowed or
recommended).

[1]
https://lore.kernel.org/lkml/[email protected]/

-- Mukesh
>
> Best regards,
> Krzysztof
>

2023-05-04 12:37:44

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 07/18] arm64: defconfig: Enable Qualcomm minidump driver

On 04/05/2023 13:45, Mukesh Ojha wrote:
>
>
> On 5/4/2023 4:53 PM, Krzysztof Kozlowski wrote:
>> On 03/05/2023 19:02, Mukesh Ojha wrote:
>>> Previous patches add the Qualcomm minidump driver support, so
>>> lets enable minidump config so that it can be used by kernel
>>> clients.
>>>
>>> Signed-off-by: Mukesh Ojha <[email protected]>
>>
>> This patchset is split too much. Defconfig change is one change. Not two
>> or three.
>>
>>> ---
>>> arch/arm64/configs/defconfig | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>>> index a24609e..831c942 100644
>>> --- a/arch/arm64/configs/defconfig
>>> +++ b/arch/arm64/configs/defconfig
>>> @@ -1250,6 +1250,7 @@ CONFIG_QCOM_STATS=m
>>> CONFIG_QCOM_WCNSS_CTRL=m
>>> CONFIG_QCOM_APR=m
>>> CONFIG_QCOM_ICC_BWMON=m
>>> +CONFIG_QCOM_MINIDUMP=y
>>
>> This must be a module.
>
> Why do you think this should be a module ?
>
> Is it because, it is lying here among others '=m' ?

Because we want and insist on everything being a module. That's the
generic rule. There are exceptions, so if this justifies being an
exception, please bring appropriate arguments.

>
> Or you have some other reasoning ? like it is for qcom specific
> soc and can not be used outside ? but that is not true for
> all configs mentioned here.
>
> The reason behind making it as '=y' was, to collect information from
> core kernel data structure as well as the information like percpu data,
> run queue, irq stat kind of information on kernel crash on a target
> running some perf configuration(android phone).

I don't understand why =m stops you from all that. What's more, I don't
understand why do you refer to the Android here. This is a development
and debugging Linux defconfig, not Android reference config for vendors...

Best regards,
Krzysztof

2023-05-04 12:39:27

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 02/18] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

On 04/05/2023 14:26, Mukesh Ojha wrote:
>
>
> On 5/4/2023 5:33 PM, Krzysztof Kozlowski wrote:
>> On 04/05/2023 13:58, Mukesh Ojha wrote:
>>>
>>>
>>> On 5/4/2023 5:08 PM, Krzysztof Kozlowski wrote:
>>>> On 03/05/2023 19:02, Mukesh Ojha wrote:
>>>>> Move minidump specific data types and macros to a separate internal
>>>>> header(qcom_minidump.h) so that it can be shared among different
>>>>> Qualcomm drivers.
>>>>
>>>> No, this is not internal header. You moved it to global header.
>>>>
>>>> There is no reason driver internals should be exposed to other unrelated
>>>> subsystems.
>>>>
>>>>>
>>>>> There is no change in functional behavior after this.
>>>>
>>>> It is. You made all these internal symbols available to others.
>>>>
>>>>>
>>>>
>>>> This comes without justification why other drivers needs to access
>>>> private and internal data. It does not look correct design. NAK.
>>>
>>> Thanks for catching outdated commit text, will fix the commit with
>>> more descriptive reasoning.
>>>
>>> It has to be global so that co-processor minidump and apss minidump can
>>> share data structure and they are lying in different directory.
>>>
>>
>> Then you should not share all the internals of memory layout but only
>> few pieces necessary to talk with minidump driver. The minidump driver
>> should organize everything how it wants.
>
> These are core data structure which is shared with boot firmware and the
> one's are moved here all are required by minidump driver .

I am not sure if I understand correctly. If they are all required by
minidump driver, then this must not be in include, but stay with
minidump. Remoteproc then should not touch it.

I don't understand why internals of minidump should be important for
remoteproc. If they are, means you broken encapsulation.

>
> If you follow here[1], i raised by concern to make this particular one's
> as private and later to avoid confusion went with single header.
> But if others agree, I will keep the one that get shared with minidump
> as separate one or if relative path of headers are allowed that can make
> it private between these drivers(which i don't think, will be allowed or
> recommended).

Let's be specific: why MD_REGION_VALID must be available for remoteproc
or any other driver after introducing qcom minidump driver?

Best regards,
Krzysztof

2023-05-04 12:49:45

by Mukesh Ojha

[permalink] [raw]

Subject: Re: [PATCH v3 04/18] soc: qcom: Add Qualcomm minidump kernel driver

On 5/4/2023 5:06 PM, Krzysztof Kozlowski wrote:
> On 03/05/2023 19:02, Mukesh Ojha wrote:
>> Minidump is a best effort mechanism to collect useful and predefined
>> data for first level of debugging on end user devices running on
>> Qualcomm SoCs. It is built on the premise that System on Chip (SoC)
>> or subsystem part of SoC crashes, due to a range of hardware and
>> software bugs. Hence, the ability to collect accurate data is only
>> a best-effort. The data collected could be invalid or corrupted,
>> data collection itself could fail, and so on.
>>
>> Qualcomm devices in engineering mode provides a mechanism for
>> generating full system ramdumps for post mortem debugging. But in some
>> cases it's however not feasible to capture the entire content of RAM.
>> The minidump mechanism provides the means for selecting region should
>> be included in the ramdump. The solution supports extracting the
>> ramdump/minidump produced either over USB or stored to an attached
>> storage device.
>>
>> The core of minidump feature is part of Qualcomm's boot firmware code.
>> It initializes shared memory(SMEM), which is a part of DDR and
>> allocates a small section of it to minidump table i.e also called
>> global table of content (G-ToC). Each subsystem (APSS, ADSP, ...) has
>> their own table of segments to be included in the minidump, all
>> references from a descriptor in SMEM (G-ToC). Each segment/region has
>> some details like name, physical address and it's size etc. and it
>> could be anywhere scattered in the DDR.
>>
>> Minidump kernel driver adds the capability to add linux region to be
>> dumped as part of ram dump collection. It provides appropriate symbol
>> to check its enablement and register client regions.
>>
>> To simplify post mortem debugging, it creates and maintain an ELF
>> header as first region that gets updated upon registration
>> of a new region.
>>
>> Signed-off-by: Mukesh Ojha <[email protected]>
>> ---
>> drivers/soc/qcom/Kconfig | 14 +
>> drivers/soc/qcom/Makefile | 1 +
>> drivers/soc/qcom/qcom_minidump.c | 581 +++++++++++++++++++++++++++++++++++++++
>> drivers/soc/qcom/smem.c | 8 +
>> include/soc/qcom/qcom_minidump.h | 61 +++-
>> 5 files changed, 663 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/soc/qcom/qcom_minidump.c
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index a491718..15c931e 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -279,4 +279,18 @@ config QCOM_INLINE_CRYPTO_ENGINE
>> tristate
>> select QCOM_SCM
>>
>> +config QCOM_MINIDUMP
>> + tristate "QCOM Minidump Support"
>> + depends on ARCH_QCOM || COMPILE_TEST
>> + select QCOM_SMEM
>> + help
>> + Enablement of core minidump feature is controlled from boot firmware
>> + side, and this config allow linux to query and manages APPS minidump
>> + table.
>> +
>> + Client drivers can register their internal data structures and debug
>> + messages as part of the minidump region and when the SoC is crashed,
>> + these selective regions will be dumped instead of the entire DDR.
>> + This saves significant amount of time and/or storage space.
>> +
>> endmenu
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 0f43a88..1ebe081 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -33,3 +33,4 @@ obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
>> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
>> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += ice.o
>> +obj-$(CONFIG_QCOM_MINIDUMP) += qcom_minidump.o
>> diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
>> new file mode 100644
>> index 0000000..d107a86
>> --- /dev/null
>> +++ b/drivers/soc/qcom/qcom_minidump.c
>> @@ -0,0 +1,581 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +/*
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/elf.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/export.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/string.h>
>> +#include <linux/soc/qcom/smem.h>
>> +#include <soc/qcom/qcom_minidump.h>
>> +
>> +/**
>> + * struct minidump_elfhdr - Minidump table elf header
>> + * @ehdr: Elf main header
>> + * @shdr: Section header
>> + * @phdr: Program header
>> + * @elf_offset: Section offset in elf
>> + * @strtable_idx: String table current index position
>> + */
>> +struct minidump_elfhdr {
>> + struct elfhdr *ehdr;
>> + struct elf_shdr *shdr;
>> + struct elf_phdr *phdr;
>> + size_t elf_offset;
>> + size_t strtable_idx;
>> +};
>> +
>> +/**
>> + * struct minidump - Minidump driver private data
>> + * @md_gbl_toc : Global TOC pointer
>> + * @md_apss_toc : Application Subsystem TOC pointer
>> + * @md_regions : High level OS region base pointer
>> + * @elf : Minidump elf header
>> + * @dev : Minidump device
>> + */
>> +struct minidump {
>> + struct minidump_global_toc *md_gbl_toc;
>> + struct minidump_subsystem *md_apss_toc;
>> + struct minidump_region *md_regions;
>> + struct minidump_elfhdr elf;
>> + struct device *dev;
>> +};
>> +
>> +/*
>> + * In some of the Old Qualcomm devices, boot firmware statically allocates 300
>> + * as total number of supported region (including all co-processors) in
>> + * minidump table out of which linux was using 201. In future, this limitation
>> + * from boot firmware might get removed by allocating the region dynamically.
>> + * So, keep it compatible with older devices, we can keep the current limit for
>> + * Linux to 201.
>> + */
>> +#define MAX_NUM_ENTRIES 201
>> +#define MAX_STRTBL_SIZE (MAX_NUM_ENTRIES * MAX_REGION_NAME_LENGTH)
>> +
>> +static struct minidump *__md;
>
> No, no file scope or global scope statics.

Sorry, this is done as per recommendation given here [1] and this
matches both driver/firmware/qcom_scm.c and driver/soc/qcom/smem.c
implementations.

[1]
https://lore.kernel.org/lkml/[email protected]/

>
>> +static DEFINE_MUTEX(minidump_lock);
>
> Neither this.
>
> Also you need to clearly express what is protected by newn lock.

Sure, will keep proper reasoning why global lock reasoning.

"
Since it is possible to call this file's exported function before
this driver's probe get called or it's probe is happening in parallel
so it needs global lock to protect this case.

"

>
>> +
>> +static struct elf_shdr *elf_shdr_entry_addr(struct elfhdr *ehdr, int idx)
>> +{
>> + struct elf_shdr *eshdr = (struct elf_shdr *)((size_t)ehdr + ehdr->e_shoff);
>> +
>> + return &eshdr[idx];
>> +}
>> +
>> +static struct elf_phdr *elf_phdr_entry_addr(struct elfhdr *ehdr, int idx)
>> +{
>> + struct elf_phdr *ephdr = (struct elf_phdr *)((size_t)ehdr + ehdr->e_phoff);
>> +
>> + return &ephdr[idx];
>> +}
>> +
>> +static char *elf_str_table_start(struct elfhdr *ehdr)
>> +{
>> + struct elf_shdr *eshdr;
>> +
>> + if (ehdr->e_shstrndx == SHN_UNDEF)
>> + return NULL;
>> +
>> + eshdr = elf_shdr_entry_addr(ehdr, ehdr->e_shstrndx);
>> + return (char *)ehdr + eshdr->sh_offset;
>> +}
>> +
>> +static char *elf_lookup_string(struct elfhdr *ehdr, int offset)
>> +{
>> + char *strtab = elf_str_table_start(ehdr);
>> +
>> + if (!strtab || (__md->elf.strtable_idx < offset))
>> + return NULL;
>> +
>> + return strtab + offset;
>> +}
>> +
>> +static unsigned int append_str_to_strtable(const char *name)
>> +{
>> + char *strtab = elf_str_table_start(__md->elf.ehdr);
>> + unsigned int old_idx = __md->elf.strtable_idx;
>> + unsigned int ret;
>> +
>> + if (!strtab || !name)
>> + return 0;
>> +
>> + ret = old_idx;
>> + old_idx += strscpy((strtab + old_idx), name, MAX_REGION_NAME_LENGTH);
>> + __md->elf.strtable_idx = old_idx + 1;
>> + return ret;
>> +}
>> +
>> +static int
>> +get_apss_minidump_region_index(const struct qcom_apss_minidump_region *region)
>> +{
>> + struct minidump_region *mdr;
>> + unsigned int i;
>> + unsigned int count;
>> +
>> + count = le32_to_cpu(__md->md_apss_toc->region_count);
>> + for (i = 0; i < count; i++) {
>> + mdr = &__md->md_regions[i];
>> + if (!strcmp(mdr->name, region->name))
>> + return i;
>> + }
>> + return -ENOENT;
>> +}
>> +
>> +static void
>> +qcom_apss_minidump_update_elf_header(const struct qcom_apss_minidump_region *region)
>
> You need to name everything shorter. Neither functions nor structs are
> making it easy to follow.

Sure !!

Thought of being very much descriptive here :-)

>
>> +{
>> + struct elfhdr *ehdr = __md->elf.ehdr;
>> + struct elf_shdr *shdr;
>> + struct elf_phdr *phdr;
>> +
>> + shdr = elf_shdr_entry_addr(ehdr, ehdr->e_shnum++);
>> + phdr = elf_phdr_entry_addr(ehdr, ehdr->e_phnum++);
>> +
>> + shdr->sh_type = SHT_PROGBITS;
>> + shdr->sh_name = append_str_to_strtable(region->name);
>> + shdr->sh_addr = (elf_addr_t)region->virt_addr;
>> + shdr->sh_size = region->size;
>> + shdr->sh_flags = SHF_WRITE;
>> + shdr->sh_offset = __md->elf.elf_offset;
>> + shdr->sh_entsize = 0;
>> +
>> + phdr->p_type = PT_LOAD;
>> + phdr->p_offset = __md->elf.elf_offset;
>> + phdr->p_vaddr = (elf_addr_t)region->virt_addr;
>> + phdr->p_paddr = region->phys_addr;
>> + phdr->p_filesz = phdr->p_memsz = region->size;
>> + phdr->p_flags = PF_R | PF_W;
>> + __md->elf.elf_offset += shdr->sh_size;
>> +}
>> +
>> +static void
>> +qcom_apss_minidump_add_region(const struct qcom_apss_minidump_region *region)
>> +{
>> + struct minidump_region *mdr;
>> + unsigned int region_cnt = le32_to_cpu(__md->md_apss_toc->region_count);
>> +
>> + mdr = &__md->md_regions[region_cnt];
>> + strscpy(mdr->name, region->name, sizeof(mdr->name));
>> + mdr->address = cpu_to_le64(region->phys_addr);
>> + mdr->size = cpu_to_le64(region->size);
>> + mdr->valid = cpu_to_le32(MINIDUMP_REGION_VALID);
>> + region_cnt++;
>> + __md->md_apss_toc->region_count = cpu_to_le32(region_cnt);
>> +}
>> +
>> +static bool
>> +qcom_apss_minidump_valid_region(const struct qcom_apss_minidump_region *region)
>> +{
>> + return region &&
>> + strnlen(region->name, MAX_NAME_LENGTH) < MAX_NAME_LENGTH &&
>> + region->virt_addr &&
>> + region->size &&
>> + IS_ALIGNED(region->size, 4);
>> +}
>> +
>> +static int qcom_apss_minidump_add_elf_header(void)
>> +{
>> + struct qcom_apss_minidump_region elfregion;
>> + struct elfhdr *ehdr;
>> + struct elf_shdr *shdr;
>> + struct elf_phdr *phdr;
>> + unsigned int elfh_size;
>> + unsigned int strtbl_off;
>> + unsigned int phdr_off;
>> + char *banner;
>> + unsigned int banner_len;
>> +
>> + banner_len = strlen(linux_banner);
>> + /*
>> + * Header buffer contains:
>> + * ELF header, (MAX_NUM_ENTRIES + 4) of Section and Program ELF headers,
>> + * where, 4 additional entries, one for empty header, one for string table
>> + * one for minidump table and one for linux banner.
>> + *
>> + * Linux banner is stored in minidump to aid post mortem tools to determine
>> + * the kernel version.
>> + */
>> + elfh_size = sizeof(*ehdr);
>> + elfh_size += MAX_STRTBL_SIZE;
>> + elfh_size += banner_len + 1;
>> + elfh_size += ((sizeof(*shdr) + sizeof(*phdr)) * (MAX_NUM_ENTRIES + 4));
>> + elfh_size = ALIGN(elfh_size, 4);
>> +
>> + __md->elf.ehdr = kzalloc(elfh_size, GFP_KERNEL);
>> + if (!__md->elf.ehdr)
>> + return -ENOMEM;
>> +
>> + /* Register ELF header as first region */
>> + strscpy(elfregion.name, "KELF_HEADER", sizeof(elfregion.name));
>> + elfregion.virt_addr = __md->elf.ehdr;
>> + elfregion.phys_addr = virt_to_phys(__md->elf.ehdr);
>> + elfregion.size = elfh_size;
>> + qcom_apss_minidump_add_region(&elfregion);
>> +
>> + ehdr = __md->elf.ehdr;
>> + /* Assign Section/Program headers offset */
>> + __md->elf.shdr = shdr = (struct elf_shdr *)(ehdr + 1);
>> + __md->elf.phdr = phdr = (struct elf_phdr *)(shdr + MAX_NUM_ENTRIES);
>> + phdr_off = sizeof(*ehdr) + (sizeof(*shdr) * MAX_NUM_ENTRIES);
>> +
>> + memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
>> + ehdr->e_ident[EI_CLASS] = ELF_CLASS;
>> + ehdr->e_ident[EI_DATA] = ELF_DATA;
>> + ehdr->e_ident[EI_VERSION] = EV_CURRENT;
>> + ehdr->e_ident[EI_OSABI] = ELF_OSABI;
>> + ehdr->e_type = ET_CORE;
>> + ehdr->e_machine = ELF_ARCH;
>> + ehdr->e_version = EV_CURRENT;
>> + ehdr->e_ehsize = sizeof(*ehdr);
>> + ehdr->e_phoff = phdr_off;
>> + ehdr->e_phentsize = sizeof(*phdr);
>> + ehdr->e_shoff = sizeof(*ehdr);
>> + ehdr->e_shentsize = sizeof(*shdr);
>> + ehdr->e_shstrndx = 1;
>> +
>> + __md->elf.elf_offset = elfh_size;
>> +
>> + /*
>> + * The zeroth index of the section header is reserved and is rarely used.
>> + * Set the section header as null (SHN_UNDEF) and move to the next one.
>> + * 2nd Section is String table.
>> + */
>> + __md->elf.strtable_idx = 1;
>> + strtbl_off = sizeof(*ehdr) + ((sizeof(*phdr) + sizeof(*shdr)) * MAX_NUM_ENTRIES);
>> + shdr++;
>> + shdr->sh_type = SHT_STRTAB;
>> + shdr->sh_offset = (elf_addr_t)strtbl_off;
>> + shdr->sh_size = MAX_STRTBL_SIZE;
>> + shdr->sh_entsize = 0;
>> + shdr->sh_flags = 0;
>> + shdr->sh_name = append_str_to_strtable("STR_TBL");
>> + shdr++;
>> +
>> + /* 3rd Section is Linux banner */
>> + banner = (char *)ehdr + strtbl_off + MAX_STRTBL_SIZE;
>> + memcpy(banner, linux_banner, banner_len);
>> +
>> + shdr->sh_type = SHT_PROGBITS;
>> + shdr->sh_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
>> + shdr->sh_size = banner_len + 1;
>> + shdr->sh_addr = (elf_addr_t)linux_banner;
>> + shdr->sh_entsize = 0;
>> + shdr->sh_flags = SHF_WRITE;
>> + shdr->sh_name = append_str_to_strtable("linux_banner");
>> +
>> + phdr->p_type = PT_LOAD;
>> + phdr->p_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
>> + phdr->p_vaddr = (elf_addr_t)linux_banner;
>> + phdr->p_paddr = virt_to_phys(linux_banner);
>> + phdr->p_filesz = phdr->p_memsz = banner_len + 1;
>> + phdr->p_flags = PF_R | PF_W;
>> +
>> + /*
>> + * Above are some prdefined sections/program header used
>> + * for debug, update their count here.
>> + */
>> + ehdr->e_phnum = 1;
>> + ehdr->e_shnum = 3;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * qcom_minidump_subsystem_desc() - Get minidump subsystem descriptor.
>> + * @minidump_index: minidump index for a subsystem in minidump table
>> + *
>> + * Return: minidump subsystem descriptor address on success and error
>> + * on failure
>> + */
>> +struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index)
>> +{
>> + struct minidump_subsystem *md_ss_toc;
>> +
>> + mutex_lock(&minidump_lock);
>> + if (!__md) {
>> + md_ss_toc = ERR_PTR(-EPROBE_DEFER);
>> + goto unlock;
>> + }
>> +
>> + md_ss_toc = &__md->md_gbl_toc->subsystems[minidump_index];
>> +unlock:
>> + mutex_unlock(&minidump_lock);
>> + return md_ss_toc;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_minidump_subsystem_desc);
>> +
>> +/**
>> + * qcom_apss_minidump_region_register() - Register a region in Minidump table.
>> + * @region: minidump region.
>> + *
>> + * Return: On success, it returns 0, otherwise a negative error value on failure.
>> + */
>> +int qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region)
>> +{
>> + unsigned int num_region;
>> + int ret;
>> +
>> + if (!__md)
>> + return -EPROBE_DEFER;
>> +
>> + if (!qcom_apss_minidump_valid_region(region))
>> + return -EINVAL;
>> +
>> + mutex_lock(&minidump_lock);
>> + ret = get_apss_minidump_region_index(region);
>> + if (ret >= 0) {
>> + dev_info(__md->dev, "%s region is already registered\n", region->name);
>> + ret = -EEXIST;
>> + goto unlock;
>> + }
>> +
>> + /* Check if there is a room for a new entry */
>> + num_region = le32_to_cpu(__md->md_apss_toc->region_count);
>> + if (num_region >= MAX_NUM_ENTRIES) {
>> + dev_err(__md->dev, "maximum region limit %u reached\n", num_region);
>> + ret = -ENOSPC;
>> + goto unlock;
>> + }
>> +
>> + qcom_apss_minidump_add_region(region);
>> + qcom_apss_minidump_update_elf_header(region);
>> + ret = 0;
>> +unlock:
>> + mutex_unlock(&minidump_lock);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_apss_minidump_region_register);
>> +
>> +static int
>> +qcom_apss_minidump_clear_header(const struct qcom_apss_minidump_region *region)
>> +{
>> + struct elfhdr *ehdr = __md->elf.ehdr;
>> + struct elf_shdr *shdr;
>> + struct elf_shdr *tmp_shdr;
>> + struct elf_phdr *phdr;
>> + struct elf_phdr *tmp_phdr;
>> + unsigned int phidx;
>> + unsigned int shidx;
>> + unsigned int len;
>> + unsigned int i;
>> + char *shname;
>> +
>> + for (i = 0; i < ehdr->e_phnum; i++) {
>> + phdr = elf_phdr_entry_addr(ehdr, i);
>> + if (phdr->p_paddr == region->phys_addr &&
>> + phdr->p_memsz == region->size)
>> + break;
>> + }
>> +
>> + if (i == ehdr->e_phnum) {
>> + dev_err(__md->dev, "Cannot find program header entry in elf\n");
>> + return -EINVAL;
>> + }
>> +
>> + phidx = i;
>> + for (i = 0; i < ehdr->e_shnum; i++) {
>> + shdr = elf_shdr_entry_addr(ehdr, i);
>> + shname = elf_lookup_string(ehdr, shdr->sh_name);
>> + if (shname && !strcmp(shname, region->name) &&
>> + shdr->sh_addr == (elf_addr_t)region->virt_addr &&
>> + shdr->sh_size == region->size)
>> + break;
>> + }
>> +
>> + if (i == ehdr->e_shnum) {
>> + dev_err(__md->dev, "Cannot find section header entry in elf\n");
>> + return -EINVAL;
>> + }
>> +
>> + shidx = i;
>> + if (shdr->sh_offset != phdr->p_offset) {
>> + dev_err(__md->dev, "Invalid entry details for region: %s\n", region->name);
>> + return -EINVAL;
>> + }
>> +
>> + /* Clear name in string table */
>> + len = strlen(shname) + 1;
>> + memmove(shname, shname + len,
>> + __md->elf.strtable_idx - shdr->sh_name - len);
>> + __md->elf.strtable_idx -= len;
>> +
>> + /* Clear program header */
>> + tmp_phdr = elf_phdr_entry_addr(ehdr, phidx);
>> + for (i = phidx; i < ehdr->e_phnum - 1; i++) {
>> + tmp_phdr = elf_phdr_entry_addr(ehdr, i + 1);
>> + phdr = elf_phdr_entry_addr(ehdr, i);
>> + memcpy(phdr, tmp_phdr, sizeof(struct elf_phdr));
>> + phdr->p_offset = phdr->p_offset - region->size;
>> + }
>> + memset(tmp_phdr, 0, sizeof(struct elf_phdr));
>> + ehdr->e_phnum--;
>> +
>> + /* Clear section header */
>> + tmp_shdr = elf_shdr_entry_addr(ehdr, shidx);
>> + for (i = shidx; i < ehdr->e_shnum - 1; i++) {
>> + tmp_shdr = elf_shdr_entry_addr(ehdr, i + 1);
>> + shdr = elf_shdr_entry_addr(ehdr, i);
>> + memcpy(shdr, tmp_shdr, sizeof(struct elf_shdr));
>> + shdr->sh_offset -= region->size;
>> + shdr->sh_name -= len;
>> + }
>> +
>> + memset(tmp_shdr, 0, sizeof(struct elf_shdr));
>> + ehdr->e_shnum--;
>> + __md->elf.elf_offset -= region->size;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * qcom_apss_minidump_region_unregister() - Unregister region from Minidump table.
>> + * @region: minidump region.
>> + *
>> + * Return: On success, it returns 0 and negative error value on failure.
>> + */
>> +int qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region)
>> +{
>> + struct minidump_region *mdr;
>> + unsigned int num_region;
>> + unsigned int idx;
>> + int ret;
>> +
>> + if (!region)
>> + return -EINVAL;
>> +
>> + mutex_lock(&minidump_lock);
>> + if (!__md) {
>> + ret = -EPROBE_DEFER;
>> + goto unlock;
>> + }
>> +
>> + idx = get_apss_minidump_region_index(region);
>> + if (idx < 0) {
>> + dev_err(__md->dev, "%s region is not present\n", region->name);
>> + ret = idx;
>> + goto unlock;
>> + }
>> +
>> + mdr = &__md->md_regions[0];
>> + num_region = le32_to_cpu(__md->md_apss_toc->region_count);
>> + /*
>> + * Left shift all the regions exist after this removed region
>> + * index by 1 to fill the gap and zero out the last region
>> + * present at the end.
>> + */
>> + memmove(&mdr[idx], &mdr[idx + 1],
>> + (num_region - idx - 1) * sizeof(struct minidump_region));
>> + memset(&mdr[num_region - 1], 0, sizeof(struct minidump_region));
>> + ret = qcom_apss_minidump_clear_header(region);
>> + if (ret) {
>> + dev_err(__md->dev, "Failed to remove region: %s\n", region->name);
>> + goto unlock;
>> + }
>> +
>> + num_region--;
>> + __md->md_apss_toc->region_count = cpu_to_le32(num_region);
>> +unlock:
>> + mutex_unlock(&minidump_lock);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_apss_minidump_region_unregister);
>> +
>> +static int qcom_minidump_init_apss_subsystem(struct minidump *md)
>> +{
>> + struct minidump_subsystem *apsstoc;
>> +
>> + apsstoc = &md->md_gbl_toc->subsystems[MINIDUMP_APSS_DESC];
>> + md->md_regions = devm_kcalloc(md->dev, MAX_NUM_ENTRIES,
>> + sizeof(struct minidump_region), GFP_KERNEL);
>> + if (!md->md_regions)
>> + return -ENOMEM;
>> +
>> + md->md_apss_toc = apsstoc;
>> + apsstoc->regions_baseptr = cpu_to_le64(virt_to_phys(md->md_regions));
>> + apsstoc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
>> + apsstoc->status = cpu_to_le32(1);
>> + apsstoc->region_count = cpu_to_le32(0);
>> +
>> + /* Tell bootloader not to encrypt the regions of this subsystem */
>> + apsstoc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
>> + apsstoc->encryption_required = cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_minidump_probe(struct platform_device *pdev)
>> +{
>> + struct minidump_global_toc *mdgtoc;
>> + struct minidump *md;
>> + size_t size;
>> + int ret;
>> +
>> + md = devm_kzalloc(&pdev->dev, sizeof(*md), GFP_KERNEL);
>> + if (!md)
>> + return -ENOMEM;
>> +
>> + mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size);
>> + if (IS_ERR(mdgtoc)) {
>> + ret = PTR_ERR(mdgtoc);
>> + dev_err(&pdev->dev, "Couldn't find minidump smem item: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
>> + ret = -EINVAL;
>> + dev_err(&pdev->dev, "minidump table is not initialized: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + mutex_lock(&minidump_lock);
>> + md->dev = &pdev->dev;
>> + md->md_gbl_toc = mdgtoc;
>
> What are you protecting here? It's not possible to have concurrent
> access to md, is it?

Check qcom_apss_minidump_region_{register/unregister} and it is possible
that these API gets called parallel to this probe.

I agree, i made a mistake in not protecting __md in {register} API
but did it unregister API in this patch, which i have fixed in later patch.

>
>> + ret = qcom_minidump_init_apss_subsystem(md);
>> + if (ret) {
>> + dev_err(&pdev->dev, "apss minidump initialization failed: %d\n", ret);
>> + goto unlock;
>> + }
>> +
>> + __md = md;
>
> No. This is a platform device, so it can have multiple instances.

It can have only one instance that is created from SMEM driver probe.

>
>> + /* First entry would be ELF header */
>> + ret = qcom_apss_minidump_add_elf_header();
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to add elf header: %d\n", ret);
>> + memset(md->md_apss_toc, 0, sizeof(struct minidump_subsystem));
>> + __md = NULL;
>> + }
>> +
>> +unlock:
>> + mutex_unlock(&minidump_lock);
>> + return ret;
>> +}
>> +
>> +static int qcom_minidump_remove(struct platform_device *pdev)
>> +{
>> + memset(__md->md_apss_toc, 0, sizeof(struct minidump_subsystem));
>> + __md = NULL;
>
> Don't use __ in variable names. Drop it everywhere.

As i said above, this is being followed in other drivers, so followed
it here as per recommendation.

Let @srini comeback on this.

>
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver qcom_minidump_driver = {
>> + .probe = qcom_minidump_probe,
>> + .remove = qcom_minidump_remove,
>> + .driver = {
>> + .name = "qcom-minidump",
>> + },
>> +};
>> +
>> +module_platform_driver(qcom_minidump_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm APSS minidump driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:qcom-minidump");
>> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
>> index 6be7ea9..d459656 100644
>> --- a/drivers/soc/qcom/smem.c
>> +++ b/drivers/soc/qcom/smem.c
>> @@ -279,6 +279,7 @@ struct qcom_smem {
>>
>> u32 item_count;
>> struct platform_device *socinfo;
>> + struct platform_device *minidump;
>> struct smem_ptable *ptable;
>> struct smem_partition global_partition;
>> struct smem_partition partitions[SMEM_HOST_COUNT];
>> @@ -1151,12 +1152,19 @@ static int qcom_smem_probe(struct platform_device *pdev)
>> if (IS_ERR(smem->socinfo))
>> dev_dbg(&pdev->dev, "failed to register socinfo device\n");
>>
>> + smem->minidump = platform_device_register_data(&pdev->dev, "qcom-minidump",
>> + PLATFORM_DEVID_NONE, NULL,
>> + 0);
>> + if (IS_ERR(smem->minidump))
>> + dev_dbg(&pdev->dev, "failed to register minidump device\n");
>> +
>> return 0;
>> }
>>
>> static int qcom_smem_remove(struct platform_device *pdev)
>> {
>> platform_device_unregister(__smem->socinfo);
>> + platform_device_unregister(__smem->minidump);
>
> Wrong order. You registered first socinfo, right?

Any order is fine here, they are not dependent.
But, will fix this.

>
>>
>> hwspin_lock_free(__smem->hwlock);
>> __smem = NULL;
>> diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
>> index 84c8605..1872668 100644
>> --- a/include/soc/qcom/qcom_minidump.h
>> +++ b/include/soc/qcom/qcom_minidump.h
>> @@ -1,6 +1,7 @@
>> /* SPDX-License-Identifier: GPL-2.0-only */
>> /*
>> - * Qualcomm minidump shared data structures and macros
>> + * This file contain Qualcomm minidump data structures and macros shared with
>> + * boot firmware and also apss minidump client's data structure
>> *
>> * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>> */
>> @@ -9,12 +10,27 @@
>> #define _QCOM_MINIDUMP_H_
>>
>> #define MAX_NUM_OF_SS 10
>> +#define MAX_NAME_LENGTH 12
>> #define MAX_REGION_NAME_LENGTH 16
>> +
>> +#define MINIDUMP_REVISION 1
>> #define SBL_MINIDUMP_SMEM_ID 602
>> +
>> +/* Application processor minidump descriptor */
>> +#define MINIDUMP_APSS_DESC 0
>> +#define SMEM_ENTRY_SIZE 40
>> +
>> #define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
>> +#define MINIDUMP_REGION_INVALID ('I' << 24 | 'N' << 16 | 'V' << 8 | 'A' << 0)
>> +#define MINIDUMP_REGION_INIT ('I' << 24 | 'N' << 16 | 'I' << 8 | 'T' << 0)
>> +#define MINIDUMP_REGION_NOINIT 0
>> +
>> +#define MINIDUMP_SS_ENCR_REQ (0 << 24 | 'Y' << 16 | 'E' << 8 | 'S' << 0)
>> +#define MINIDUMP_SS_ENCR_NOTREQ (0 << 24 | 0 << 16 | 'N' << 8 | 'R' << 0)
>> +#define MINIDUMP_SS_ENCR_NONE ('N' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
>> #define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
>> +#define MINIDUMP_SS_ENCR_START ('S' << 24 | 'T' << 16 | 'R' << 8 | 'T' << 0)
>> #define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
>> -
>
> Why removing this?

Ok, will keep it.

>
>> /**
>> * struct minidump_region - Minidump region
>> * @name : Name of the region to be dumped
>> @@ -63,4 +79,45 @@ struct minidump_global_toc {
>> struct minidump_subsystem subsystems[MAX_NUM_OF_SS];
>> };
>>
>> +/**
>> + * struct qcom_apss_minidump_region - APSS Minidump region information
>> + *
>> + * @name: Entry name, Minidump will dump binary with this name.
>> + * @virt_addr: Virtual address of the entry.
>> + * @phys_addr: Physical address of the entry to dump.
>> + * @size: Number of byte to dump from @address location,
>> + * and it should be 4 byte aligned.
>> + */
>> +struct qcom_apss_minidump_region {
>> + char name[MAX_NAME_LENGTH];
>> + void *virt_addr;
>> + phys_addr_t phys_addr;
>> + size_t size;
>> +};
>
> You expose way too much internals in global header.

This suppose to shared with all minidump clients.

As already discussed in earlier patch, will keep the one which get
shared with remoteproc as differnet header if the people who have
reviewed till now agree to this common stuff.

>
>> +
>> +#if IS_ENABLED(CONFIG_QCOM_MINIDUMP)
>> +extern struct minidump_subsystem *
>
> No externs.

ok.

> The header is unreadable.
>
>> +qcom_minidump_subsystem_desc(unsigned int minidump_index);
>> +extern int
>> +qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region);
>> +extern int
>> +qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region);
>
> Blank line
>
>> +#else
>
> Blank line
>
>> +static inline
>> +struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index)
>> +{
>> + return NULL;
>> +}
>
> Blank line
>
>> +static inline int
>> +qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region)
>> +{
>> + /* Return quietly, if minidump is not enabled */
>> + return 0;
>> +}
>
>
>> +static inline int
>> +qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region)
>> +{
>> + return 0;
>> +}
>
>> +#endif
>
> /* CONFIG_QCOM_MINIDUMP */
>
>> #endif /* _QCOM_MINIDUMP_H_ */

Noted, thanks.

>
> Best regards,
> Krzysztof
>

2023-05-04 13:13:19

by Mukesh Ojha

[permalink] [raw]

Subject: Re: [PATCH v3 02/18] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

On 5/4/2023 6:06 PM, Krzysztof Kozlowski wrote:
> On 04/05/2023 14:26, Mukesh Ojha wrote:
>>
>>
>> On 5/4/2023 5:33 PM, Krzysztof Kozlowski wrote:
>>> On 04/05/2023 13:58, Mukesh Ojha wrote:
>>>>
>>>>
>>>> On 5/4/2023 5:08 PM, Krzysztof Kozlowski wrote:
>>>>> On 03/05/2023 19:02, Mukesh Ojha wrote:
>>>>>> Move minidump specific data types and macros to a separate internal
>>>>>> header(qcom_minidump.h) so that it can be shared among different
>>>>>> Qualcomm drivers.
>>>>>
>>>>> No, this is not internal header. You moved it to global header.
>>>>>
>>>>> There is no reason driver internals should be exposed to other unrelated
>>>>> subsystems.
>>>>>
>>>>>>
>>>>>> There is no change in functional behavior after this.
>>>>>
>>>>> It is. You made all these internal symbols available to others.
>>>>>
>>>>>>
>>>>>
>>>>> This comes without justification why other drivers needs to access
>>>>> private and internal data. It does not look correct design. NAK.
>>>>
>>>> Thanks for catching outdated commit text, will fix the commit with
>>>> more descriptive reasoning.
>>>>
>>>> It has to be global so that co-processor minidump and apss minidump can
>>>> share data structure and they are lying in different directory.
>>>>
>>>
>>> Then you should not share all the internals of memory layout but only
>>> few pieces necessary to talk with minidump driver. The minidump driver
>>> should organize everything how it wants.
>>
>> These are core data structure which is shared with boot firmware and the
>> one's are moved here all are required by minidump driver .
>
> I am not sure if I understand correctly. If they are all required by
> minidump driver, then this must not be in include, but stay with
> minidump. Remoteproc then should not touch it.
>
> I don't understand why internals of minidump should be important for
> remoteproc. If they are, means you broken encapsulation.
>
>>
>> If you follow here[1], i raised by concern to make this particular one's
>> as private and later to avoid confusion went with single header.
>> But if others agree, I will keep the one that get shared with minidump
>> as separate one or if relative path of headers are allowed that can make
>> it private between these drivers(which i don't think, will be allowed or
>> recommended).
>
> Let's be specific: why MD_REGION_VALID must be available for remoteproc
> or any other driver after introducing qcom minidump driver?

Forget about this driver for a moment.

I am not sure how much you know about existing qcom_minidump()
implementation and why is it there in first place in remoteproc
code in driver/remoteproc/qcom_common.c

The idea is, remoteproc co-processor like adsp/cdsp etc. may have their
static predefined region (segments) to be collected on their crash which
is what exactly existing qcom_minidump() is doing.

Now, after this minidump series, APSS (linux) will have it's
own of collecting linux client region independent of whether
remoteproc minidump collection.

I think, are you hinting to move all minidump related code from
remoteproc to qcom_minidump driver, is this what are you trying
to say ?

-- Mukesh
>
>
> Best regards,
> Krzysztof
>

2023-05-04 14:49:43

by Mukesh Ojha

[permalink] [raw]

Subject: Re: [PATCH v3 07/18] arm64: defconfig: Enable Qualcomm minidump driver

On 5/4/2023 6:02 PM, Krzysztof Kozlowski wrote:
> On 04/05/2023 13:45, Mukesh Ojha wrote:
>>
>>
>> On 5/4/2023 4:53 PM, Krzysztof Kozlowski wrote:
>>> On 03/05/2023 19:02, Mukesh Ojha wrote:
>>>> Previous patches add the Qualcomm minidump driver support, so
>>>> lets enable minidump config so that it can be used by kernel
>>>> clients.
>>>>
>>>> Signed-off-by: Mukesh Ojha <[email protected]>
>>>
>>> This patchset is split too much. Defconfig change is one change. Not two
>>> or three.
>>>
>>>> ---
>>>> arch/arm64/configs/defconfig | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>>>> index a24609e..831c942 100644
>>>> --- a/arch/arm64/configs/defconfig
>>>> +++ b/arch/arm64/configs/defconfig
>>>> @@ -1250,6 +1250,7 @@ CONFIG_QCOM_STATS=m
>>>> CONFIG_QCOM_WCNSS_CTRL=m
>>>> CONFIG_QCOM_APR=m
>>>> CONFIG_QCOM_ICC_BWMON=m
>>>> +CONFIG_QCOM_MINIDUMP=y
>>>
>>> This must be a module.
>>
>> Why do you think this should be a module ?
>>
>> Is it because, it is lying here among others '=m' ?
>
> Because we want and insist on everything being a module. That's the
> generic rule. There are exceptions, so if this justifies being an
> exception, please bring appropriate arguments.
>
>>
>> Or you have some other reasoning ? like it is for qcom specific
>> soc and can not be used outside ? but that is not true for
>> all configs mentioned here.
>>
>> The reason behind making it as '=y' was, to collect information from
>> core kernel data structure as well as the information like percpu data,
>> run queue, irq stat kind of information on kernel crash on a target
>> running some perf configuration(android phone).
>
> I don't understand why =m stops you from all that.

How do i get kernel symbol address from a modules
can we use kallsyms_lookup_name from modules ?

--Mukesh

What's more, I don't
> understand why do you refer to the Android here. This is a development
> and debugging Linux defconfig, not Android reference config for vendors...
>
> Best regards,
> Krzysztof
>

2023-05-04 15:27:50

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 02/18] remoteproc: qcom: Move minidump specific data to qcom_minidump.h

On 04/05/2023 14:57, Mukesh Ojha wrote:
>
>
> On 5/4/2023 6:06 PM, Krzysztof Kozlowski wrote:
>> On 04/05/2023 14:26, Mukesh Ojha wrote:
>>>
>>>
>>> On 5/4/2023 5:33 PM, Krzysztof Kozlowski wrote:
>>>> On 04/05/2023 13:58, Mukesh Ojha wrote:
>>>>>
>>>>>
>>>>> On 5/4/2023 5:08 PM, Krzysztof Kozlowski wrote:
>>>>>> On 03/05/2023 19:02, Mukesh Ojha wrote:
>>>>>>> Move minidump specific data types and macros to a separate internal
>>>>>>> header(qcom_minidump.h) so that it can be shared among different
>>>>>>> Qualcomm drivers.
>>>>>>
>>>>>> No, this is not internal header. You moved it to global header.
>>>>>>
>>>>>> There is no reason driver internals should be exposed to other unrelated
>>>>>> subsystems.
>>>>>>
>>>>>>>
>>>>>>> There is no change in functional behavior after this.
>>>>>>
>>>>>> It is. You made all these internal symbols available to others.
>>>>>>
>>>>>>>
>>>>>>
>>>>>> This comes without justification why other drivers needs to access
>>>>>> private and internal data. It does not look correct design. NAK.
>>>>>
>>>>> Thanks for catching outdated commit text, will fix the commit with
>>>>> more descriptive reasoning.
>>>>>
>>>>> It has to be global so that co-processor minidump and apss minidump can
>>>>> share data structure and they are lying in different directory.
>>>>>
>>>>
>>>> Then you should not share all the internals of memory layout but only
>>>> few pieces necessary to talk with minidump driver. The minidump driver
>>>> should organize everything how it wants.
>>>
>>> These are core data structure which is shared with boot firmware and the
>>> one's are moved here all are required by minidump driver .
>>
>> I am not sure if I understand correctly. If they are all required by
>> minidump driver, then this must not be in include, but stay with
>> minidump. Remoteproc then should not touch it.
>>
>> I don't understand why internals of minidump should be important for
>> remoteproc. If they are, means you broken encapsulation.
>>
>>>
>>> If you follow here[1], i raised by concern to make this particular one's
>>> as private and later to avoid confusion went with single header.
>>> But if others agree, I will keep the one that get shared with minidump
>>> as separate one or if relative path of headers are allowed that can make
>>> it private between these drivers(which i don't think, will be allowed or
>>> recommended).
>>
>> Let's be specific: why MD_REGION_VALID must be available for remoteproc
>> or any other driver after introducing qcom minidump driver?
>
> Forget about this driver for a moment.
>
> I am not sure how much you know about existing qcom_minidump()
> implementation and why is it there in first place in remoteproc
> code in driver/remoteproc/qcom_common.c
>
> The idea is, remoteproc co-processor like adsp/cdsp etc. may have their
> static predefined region (segments) to be collected on their crash which
> is what exactly existing qcom_minidump() is doing.
>
> Now, after this minidump series, APSS (linux) will have it's
> own of collecting linux client region independent of whether
> remoteproc minidump collection.
>
> I think, are you hinting to move all minidump related code from
> remoteproc to qcom_minidump driver, is this what are you trying
> to say ?

Close, not all but the ones not necessary to identify the
regions/storage/layout. If some variable about this
region/storage/layout is the same everywhere, it means it's basically a
property of qcom minidump and you have just exposed it to consumers
breaking encapsulation.

Best regards,
Krzysztof

2023-05-04 15:37:35

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 07/18] arm64: defconfig: Enable Qualcomm minidump driver

On 04/05/2023 16:43, Mukesh Ojha wrote:
>
>
> On 5/4/2023 6:02 PM, Krzysztof Kozlowski wrote:
>> On 04/05/2023 13:45, Mukesh Ojha wrote:
>>>
>>>
>>> On 5/4/2023 4:53 PM, Krzysztof Kozlowski wrote:
>>>> On 03/05/2023 19:02, Mukesh Ojha wrote:
>>>>> Previous patches add the Qualcomm minidump driver support, so
>>>>> lets enable minidump config so that it can be used by kernel
>>>>> clients.
>>>>>
>>>>> Signed-off-by: Mukesh Ojha <[email protected]>
>>>>
>>>> This patchset is split too much. Defconfig change is one change. Not two
>>>> or three.
>>>>
>>>>> ---
>>>>> arch/arm64/configs/defconfig | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>>>>> index a24609e..831c942 100644
>>>>> --- a/arch/arm64/configs/defconfig
>>>>> +++ b/arch/arm64/configs/defconfig
>>>>> @@ -1250,6 +1250,7 @@ CONFIG_QCOM_STATS=m
>>>>> CONFIG_QCOM_WCNSS_CTRL=m
>>>>> CONFIG_QCOM_APR=m
>>>>> CONFIG_QCOM_ICC_BWMON=m
>>>>> +CONFIG_QCOM_MINIDUMP=y
>>>>
>>>> This must be a module.
>>>
>>> Why do you think this should be a module ?
>>>
>>> Is it because, it is lying here among others '=m' ?
>>
>> Because we want and insist on everything being a module. That's the
>> generic rule. There are exceptions, so if this justifies being an
>> exception, please bring appropriate arguments.
>>
>>>
>>> Or you have some other reasoning ? like it is for qcom specific
>>> soc and can not be used outside ? but that is not true for
>>> all configs mentioned here.
>>>
>>> The reason behind making it as '=y' was, to collect information from
>>> core kernel data structure as well as the information like percpu data,
>>> run queue, irq stat kind of information on kernel crash on a target
>>> running some perf configuration(android phone).
>>
>> I don't understand why =m stops you from all that.
>
> How do i get kernel symbol address from a modules
> can we use kallsyms_lookup_name from modules ?

You allow it to be a module in patch #4, so I think you solved it,
right? Otherwise it could not be a module?

Anyway, where do you use kallsyms_lookup_name()? I cannot find it in
your patch.

Best regards,
Krzysztof

2023-05-04 15:41:42

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 09/18] soc: qcom: Add qcom's pstore minidump driver support

On 03/05/2023 19:02, Mukesh Ojha wrote:
> This driver was inspired from the fact pstore ram region should be
> fixed and boot firmware need to have awarness about this region,
> so that it will be persistent across boot. But, there are many
> QCOM SoC which does not support warm boot from hardware but they
> have minidump support from the software, and for them, there is
> no need of this pstore ram region to be fixed, but at the same
> time have interest in the pstore frontends. So, this driver
> get the dynamic reserved region from the ram and register the
> ramoops platform device.
>
> +---------+ +---------+ +--------+ +---------+
> | console | | pmsg | | ftrace | | dmesg |
> +---------+ +---------+ +--------+ +---------+
> | | | |
> | | | |
> +------------------------------------------+
> |
> \ /
> +----------------+
> (1) |pstore frontends|
> +----------------+
> |
> \ /
> +------------------- +
> (2) | pstore backend(ram)|
> +--------------------+
> |
> \ /
> +--------------------+
> (3) |qcom_pstore_minidump|
> +--------------------+
> |
> \ /
> +---------------+
> (4) | qcom_minidump |
> +---------------+
>
> This driver will route all the pstore front data to the stored
> in qcom pstore reserved region and the reason of showing an
> arrow from (3) to (4) as qcom_pstore_minidump driver will register
> all the available frontends region with qcom minidump driver
> in upcoming patch.
>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> drivers/soc/qcom/Kconfig | 11 +++
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom_pstore_minidump.c | 116 ++++++++++++++++++++++++++++++++
> 3 files changed, 128 insertions(+)
> create mode 100644 drivers/soc/qcom/qcom_pstore_minidump.c
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 15c931e..afdc634 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -293,4 +293,15 @@ config QCOM_MINIDUMP
> these selective regions will be dumped instead of the entire DDR.
> This saves significant amount of time and/or storage space.
>
> +config QCOM_PSTORE_MINIDUMP
> + tristate "Pstore support for QCOM Minidump"
> + depends on ARCH_QCOM
> + depends on PSTORE_RAM
> + depends on QCOM_MINIDUMP
> + help
> + Enablement of this driver ensures that ramoops region can be anywhere
> + reserved in ram instead of being fixed address which needs boot firmware
> + awareness. So, this driver creates plaform device and registers available
> + frontend region with the Qualcomm's minidump driver.
> +
> endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 1ebe081..02d30d7 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -34,3 +34,4 @@ obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += ice.o
> obj-$(CONFIG_QCOM_MINIDUMP) += qcom_minidump.o
> +obj-$(CONFIG_QCOM_PSTORE_MINIDUMP) += qcom_pstore_minidump.o
> diff --git a/drivers/soc/qcom/qcom_pstore_minidump.c b/drivers/soc/qcom/qcom_pstore_minidump.c
> new file mode 100644
> index 0000000..8d58500
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom_pstore_minidump.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +#include <linux/pstore_ram.h>
> +#include <soc/qcom/qcom_minidump.h>
> +
> +struct qcom_ramoops_config {
> + unsigned long record_size;
> + unsigned long console_size;
> + unsigned long ftrace_size;
> + unsigned long pmsg_size;
> + unsigned int mem_type;
> + unsigned int flags;
> + int max_reason;
> +};
> +
> +struct qcom_ramoops_dd {
> + struct ramoops_platform_data qcom_ramoops_pdata;
> + struct platform_device *ramoops_pdev;
> +};
> +
> +static struct qcom_ramoops_config default_ramoops_config = {

Cannot this be const?

> + .mem_type = 2,
> + .record_size = 0x0,
> + .console_size = 0x200000,
> + .ftrace_size = 0x0,
> + .pmsg_size = 0x0,
> +};
> +
> +static struct qcom_ramoops_dd *qcom_rdd;

Drop file scope variable. It's not even used.

> +static int qcom_ramoops_probe(struct platform_device *pdev)
> +{
> + struct device_node *of_node = pdev->dev.of_node;
> + struct device_node *node;
> + const struct qcom_ramoops_config *cfg;
> + struct ramoops_platform_data *pdata;
> + struct reserved_mem *rmem;
> + long ret;
> +
> + node = of_parse_phandle(of_node, "memory-region", 0);
> + if (!node)
> + return -ENODEV;
> +
> + rmem = of_reserved_mem_lookup(node);
> + of_node_put(node);
> + if (!rmem) {
> + dev_err(&pdev->dev, "failed to locate DT /reserved-memory resource\n");
> + return -EINVAL;
> + }
> +
> + qcom_rdd = devm_kzalloc(&pdev->dev, sizeof(*qcom_rdd), GFP_KERNEL);
> + if (!qcom_rdd)
> + return -ENOMEM;
> +
> + cfg = of_device_get_match_data(&pdev->dev);
> + if (!cfg) {
> + dev_err(&pdev->dev, "failed to get supported matched data\n");
> + return -ENOENT;
> + }
> +
> + pdata = &qcom_rdd->qcom_ramoops_pdata;
> + pdata->mem_size = rmem->size;
> + pdata->mem_address = rmem->base;
> + pdata->mem_type = cfg->mem_type;
> + pdata->record_size = cfg->record_size;
> + pdata->console_size = cfg->console_size;
> + pdata->ftrace_size = cfg->ftrace_size;
> + pdata->pmsg_size = cfg->pmsg_size;
> + pdata->max_reason = KMSG_DUMP_PANIC;
> +
> + qcom_rdd->ramoops_pdev = platform_device_register_data(NULL, "ramoops", -1,
> + pdata, sizeof(*pdata));
> + if (IS_ERR(qcom_rdd->ramoops_pdev)) {
> + ret = PTR_ERR(qcom_rdd->ramoops_pdev);
> + dev_err(&pdev->dev, "could not create platform device: %ld\n", ret);
> + qcom_rdd->ramoops_pdev = NULL;
> + }
> +
> + return ret;
> +}
> +
> +static int qcom_ramoops_remove(struct platform_device *pdev)

Use instead .remove_new callback.

> +{
> + platform_device_unregister(qcom_rdd->ramoops_pdev);
> + qcom_rdd->ramoops_pdev = NULL;
> +
> + return 0;
> +}
> +
> +static const struct of_device_id qcom_ramoops_of_match[] = {
> + { .compatible = "qcom,sm8450-ramoops-minidump", .data = &default_ramoops_config },

You don't need this entry.

> + { .compatible = "qcom,ramoops-minidump", .data = &default_ramoops_config },
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(of, qcom_ramoops_of_match);

Blank line goes after the MODULE_DEVICE_TABLE, not before.

Best regards,
Krzysztof

2023-05-04 16:11:56

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 04/18] soc: qcom: Add Qualcomm minidump kernel driver

On 04/05/2023 14:38, Mukesh Ojha wrote:
>
>
> On 5/4/2023 5:06 PM, Krzysztof Kozlowski wrote:
>> On 03/05/2023 19:02, Mukesh Ojha wrote:
>>> Minidump is a best effort mechanism to collect useful and predefined
>>> data for first level of debugging on end user devices running on
>>> Qualcomm SoCs. It is built on the premise that System on Chip (SoC)
>>> or subsystem part of SoC crashes, due to a range of hardware and
>>> software bugs. Hence, the ability to collect accurate data is only
>>> a best-effort. The data collected could be invalid or corrupted,
>>> data collection itself could fail, and so on.
>>>
>>> Qualcomm devices in engineering mode provides a mechanism for
>>> generating full system ramdumps for post mortem debugging. But in some
>>> cases it's however not feasible to capture the entire content of RAM.
>>> The minidump mechanism provides the means for selecting region should
>>> be included in the ramdump. The solution supports extracting the
>>> ramdump/minidump produced either over USB or stored to an attached
>>> storage device.
>>>
>>> The core of minidump feature is part of Qualcomm's boot firmware code.
>>> It initializes shared memory(SMEM), which is a part of DDR and
>>> allocates a small section of it to minidump table i.e also called
>>> global table of content (G-ToC). Each subsystem (APSS, ADSP, ...) has
>>> their own table of segments to be included in the minidump, all
>>> references from a descriptor in SMEM (G-ToC). Each segment/region has
>>> some details like name, physical address and it's size etc. and it
>>> could be anywhere scattered in the DDR.
>>>
>>> Minidump kernel driver adds the capability to add linux region to be
>>> dumped as part of ram dump collection. It provides appropriate symbol
>>> to check its enablement and register client regions.
>>>
>>> To simplify post mortem debugging, it creates and maintain an ELF
>>> header as first region that gets updated upon registration
>>> of a new region.
>>>
>>> Signed-off-by: Mukesh Ojha <[email protected]>
>>> ---
>>> drivers/soc/qcom/Kconfig | 14 +
>>> drivers/soc/qcom/Makefile | 1 +
>>> drivers/soc/qcom/qcom_minidump.c | 581 +++++++++++++++++++++++++++++++++++++++
>>> drivers/soc/qcom/smem.c | 8 +
>>> include/soc/qcom/qcom_minidump.h | 61 +++-
>>> 5 files changed, 663 insertions(+), 2 deletions(-)
>>> create mode 100644 drivers/soc/qcom/qcom_minidump.c
>>>
>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>> index a491718..15c931e 100644
>>> --- a/drivers/soc/qcom/Kconfig
>>> +++ b/drivers/soc/qcom/Kconfig
>>> @@ -279,4 +279,18 @@ config QCOM_INLINE_CRYPTO_ENGINE
>>> tristate
>>> select QCOM_SCM
>>>
>>> +config QCOM_MINIDUMP
>>> + tristate "QCOM Minidump Support"
>>> + depends on ARCH_QCOM || COMPILE_TEST
>>> + select QCOM_SMEM
>>> + help
>>> + Enablement of core minidump feature is controlled from boot firmware
>>> + side, and this config allow linux to query and manages APPS minidump
>>> + table.
>>> +
>>> + Client drivers can register their internal data structures and debug
>>> + messages as part of the minidump region and when the SoC is crashed,
>>> + these selective regions will be dumped instead of the entire DDR.
>>> + This saves significant amount of time and/or storage space.
>>> +
>>> endmenu
>>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>>> index 0f43a88..1ebe081 100644
>>> --- a/drivers/soc/qcom/Makefile
>>> +++ b/drivers/soc/qcom/Makefile
>>> @@ -33,3 +33,4 @@ obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>>> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
>>> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
>>> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += ice.o
>>> +obj-$(CONFIG_QCOM_MINIDUMP) += qcom_minidump.o
>>> diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
>>> new file mode 100644
>>> index 0000000..d107a86
>>> --- /dev/null
>>> +++ b/drivers/soc/qcom/qcom_minidump.c
>>> @@ -0,0 +1,581 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +/*
>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>>> + */
>>> +
>>> +#include <linux/elf.h>
>>> +#include <linux/err.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/export.h>
>>> +#include <linux/init.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/string.h>
>>> +#include <linux/soc/qcom/smem.h>
>>> +#include <soc/qcom/qcom_minidump.h>
>>> +
>>> +/**
>>> + * struct minidump_elfhdr - Minidump table elf header
>>> + * @ehdr: Elf main header
>>> + * @shdr: Section header
>>> + * @phdr: Program header
>>> + * @elf_offset: Section offset in elf
>>> + * @strtable_idx: String table current index position
>>> + */
>>> +struct minidump_elfhdr {
>>> + struct elfhdr *ehdr;
>>> + struct elf_shdr *shdr;
>>> + struct elf_phdr *phdr;
>>> + size_t elf_offset;
>>> + size_t strtable_idx;
>>> +};
>>> +
>>> +/**
>>> + * struct minidump - Minidump driver private data
>>> + * @md_gbl_toc : Global TOC pointer
>>> + * @md_apss_toc : Application Subsystem TOC pointer
>>> + * @md_regions : High level OS region base pointer
>>> + * @elf : Minidump elf header
>>> + * @dev : Minidump device
>>> + */
>>> +struct minidump {
>>> + struct minidump_global_toc *md_gbl_toc;
>>> + struct minidump_subsystem *md_apss_toc;
>>> + struct minidump_region *md_regions;
>>> + struct minidump_elfhdr elf;
>>> + struct device *dev;
>>> +};
>>> +
>>> +/*
>>> + * In some of the Old Qualcomm devices, boot firmware statically allocates 300
>>> + * as total number of supported region (including all co-processors) in
>>> + * minidump table out of which linux was using 201. In future, this limitation
>>> + * from boot firmware might get removed by allocating the region dynamically.
>>> + * So, keep it compatible with older devices, we can keep the current limit for
>>> + * Linux to 201.
>>> + */
>>> +#define MAX_NUM_ENTRIES 201
>>> +#define MAX_STRTBL_SIZE (MAX_NUM_ENTRIES * MAX_REGION_NAME_LENGTH)
>>> +
>>> +static struct minidump *__md;
>>
>> No, no file scope or global scope statics.
>
> Sorry, this is done as per recommendation given here [1] and this
> matches both driver/firmware/qcom_scm.c and driver/soc/qcom/smem.c
> implementations.
>
> [1]
> https://lore.kernel.org/lkml/[email protected]/

That's not true. You had the static already in v2, before Srini commented.

Look:
https://lore.kernel.org/lkml/[email protected]/

+static struct minidump minidump;
+static DEFINE_MUTEX(minidump_lock);

We do not talk about the names.

>>> +
>>> + if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
>>> + ret = -EINVAL;
>>> + dev_err(&pdev->dev, "minidump table is not initialized: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + mutex_lock(&minidump_lock);
>>> + md->dev = &pdev->dev;
>>> + md->md_gbl_toc = mdgtoc;
>>
>> What are you protecting here? It's not possible to have concurrent
>> access to md, is it?
>
> Check qcom_apss_minidump_region_{register/unregister} and it is possible
> that these API gets called parallel to this probe.

Wait, you say that something can modify local variable md before it is
assigned to __md? How?

>
> I agree, i made a mistake in not protecting __md in {register} API
> but did it unregister API in this patch, which i have fixed in later patch.

No, you are protecting random things. Nothing will concurrently modify
md and &pdev->dev in this moment. mdgtoc is allocated above, so also
cannot by modified.

Otherwise show me the hypothetical scenario.

>
>>
>>> + ret = qcom_minidump_init_apss_subsystem(md);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "apss minidump initialization failed: %d\n", ret);
>>> + goto unlock;
>>> + }
>>> +
>>> + __md = md;
>>
>> No. This is a platform device, so it can have multiple instances.
>
> It can have only one instance that is created from SMEM driver probe.

Anyone can instantiate more of them.... how did you solve it?

>
>>
>>> + /* First entry would be ELF header */
>>> + ret = qcom_apss_minidump_add_elf_header();
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Failed to add elf header: %d\n", ret);
>>> + memset(md->md_apss_toc, 0, sizeof(struct minidump_subsystem));
>>> + __md = NULL;
>>> + }
>>> +
>>> +unlock:
>>> + mutex_unlock(&minidump_lock);
>>> + return ret;
>>> +}
>>> +
>>> +static int qcom_minidump_remove(struct platform_device *pdev)
>>> +{
>>> + memset(__md->md_apss_toc, 0, sizeof(struct minidump_subsystem));
>>> + __md = NULL;
>>
>> Don't use __ in variable names. Drop it everywhere.
>
> As i said above, this is being followed in other drivers, so followed
> it here as per recommendation.
>
> Let @srini comeback on this.

Which part of coding style recommends __ for driver code?

>
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static struct platform_driver qcom_minidump_driver = {
>>> + .probe = qcom_minidump_probe,
>>> + .remove = qcom_minidump_remove,
>>> + .driver = {
>>> + .name = "qcom-minidump",
>>> + },
>>> +};
>>> +
>>> +module_platform_driver(qcom_minidump_driver);
>>> +
>>> +MODULE_DESCRIPTION("Qualcomm APSS minidump driver");
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_ALIAS("platform:qcom-minidump");
>>> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
>>> index 6be7ea9..d459656 100644
>>> --- a/drivers/soc/qcom/smem.c
>>> +++ b/drivers/soc/qcom/smem.c
>>> @@ -279,6 +279,7 @@ struct qcom_smem {
>>>
>>> u32 item_count;
>>> struct platform_device *socinfo;
>>> + struct platform_device *minidump;
>>> struct smem_ptable *ptable;
>>> struct smem_partition global_partition;
>>> struct smem_partition partitions[SMEM_HOST_COUNT];
>>> @@ -1151,12 +1152,19 @@ static int qcom_smem_probe(struct platform_device *pdev)
>>> if (IS_ERR(smem->socinfo))
>>> dev_dbg(&pdev->dev, "failed to register socinfo device\n");
>>>
>>> + smem->minidump = platform_device_register_data(&pdev->dev, "qcom-minidump",
>>> + PLATFORM_DEVID_NONE, NULL,
>>> + 0);
>>> + if (IS_ERR(smem->minidump))
>>> + dev_dbg(&pdev->dev, "failed to register minidump device\n");
>>> +
>>> return 0;
>>> }
>>>
>>> static int qcom_smem_remove(struct platform_device *pdev)
>>> {
>>> platform_device_unregister(__smem->socinfo);
>>> + platform_device_unregister(__smem->minidump);
>>
>> Wrong order. You registered first socinfo, right?
>
> Any order is fine here, they are not dependent.
> But, will fix this.

No, the order is always reversed from allocation. It does not matter if
they are dependent or not.

Best regards,
Krzysztof

2023-05-04 17:05:49

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 04/18] soc: qcom: Add Qualcomm minidump kernel driver

On 04/05/2023 17:21, Krzysztof Kozlowski wrote:
>>>
>>>> + ret = qcom_minidump_init_apss_subsystem(md);
>>>> + if (ret) {
>>>> + dev_err(&pdev->dev, "apss minidump initialization failed: %d\n", ret);
>>>> + goto unlock;
>>>> + }
>>>> +
>>>> + __md = md;
>>>
>>> No. This is a platform device, so it can have multiple instances.
>>
>> It can have only one instance that is created from SMEM driver probe.
>
> Anyone can instantiate more of them.... how did you solve it?

To clarify - sprinkling more of singletons makes everything tightly
coupled, difficult to debug and non-portable. You cannot have two
instances, you have to control concurrent initialization by yourself in
each of such singletons.

I understand sometimes they are unavoidable, for example when this does
not map to hardware property. However here you have the parent - smem -
which can return you valid instance. Thus you avoid entire problem of
file-scope variables.

Best regards,
Krzysztof

2023-05-05 05:53:38

by Mukesh Ojha

[permalink] [raw]

Subject: Re: [PATCH v3 04/18] soc: qcom: Add Qualcomm minidump kernel driver

On 5/4/2023 8:51 PM, Krzysztof Kozlowski wrote:
> On 04/05/2023 14:38, Mukesh Ojha wrote:
>>
>>
>> On 5/4/2023 5:06 PM, Krzysztof Kozlowski wrote:
>>> On 03/05/2023 19:02, Mukesh Ojha wrote:
>>>> Minidump is a best effort mechanism to collect useful and predefined
>>>> data for first level of debugging on end user devices running on
>>>> Qualcomm SoCs. It is built on the premise that System on Chip (SoC)
>>>> or subsystem part of SoC crashes, due to a range of hardware and
>>>> software bugs. Hence, the ability to collect accurate data is only
>>>> a best-effort. The data collected could be invalid or corrupted,
>>>> data collection itself could fail, and so on.
>>>>
>>>> Qualcomm devices in engineering mode provides a mechanism for
>>>> generating full system ramdumps for post mortem debugging. But in some
>>>> cases it's however not feasible to capture the entire content of RAM.
>>>> The minidump mechanism provides the means for selecting region should
>>>> be included in the ramdump. The solution supports extracting the
>>>> ramdump/minidump produced either over USB or stored to an attached
>>>> storage device.
>>>>
>>>> The core of minidump feature is part of Qualcomm's boot firmware code.
>>>> It initializes shared memory(SMEM), which is a part of DDR and
>>>> allocates a small section of it to minidump table i.e also called
>>>> global table of content (G-ToC). Each subsystem (APSS, ADSP, ...) has
>>>> their own table of segments to be included in the minidump, all
>>>> references from a descriptor in SMEM (G-ToC). Each segment/region has
>>>> some details like name, physical address and it's size etc. and it
>>>> could be anywhere scattered in the DDR.
>>>>
>>>> Minidump kernel driver adds the capability to add linux region to be
>>>> dumped as part of ram dump collection. It provides appropriate symbol
>>>> to check its enablement and register client regions.
>>>>
>>>> To simplify post mortem debugging, it creates and maintain an ELF
>>>> header as first region that gets updated upon registration
>>>> of a new region.
>>>>
>>>> Signed-off-by: Mukesh Ojha <[email protected]>
>>>> ---
>>>> drivers/soc/qcom/Kconfig | 14 +
>>>> drivers/soc/qcom/Makefile | 1 +
>>>> drivers/soc/qcom/qcom_minidump.c | 581 +++++++++++++++++++++++++++++++++++++++
>>>> drivers/soc/qcom/smem.c | 8 +
>>>> include/soc/qcom/qcom_minidump.h | 61 +++-
>>>> 5 files changed, 663 insertions(+), 2 deletions(-)
>>>> create mode 100644 drivers/soc/qcom/qcom_minidump.c
>>>>
>>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>>> index a491718..15c931e 100644
>>>> --- a/drivers/soc/qcom/Kconfig
>>>> +++ b/drivers/soc/qcom/Kconfig
>>>> @@ -279,4 +279,18 @@ config QCOM_INLINE_CRYPTO_ENGINE
>>>> tristate
>>>> select QCOM_SCM
>>>>
>>>> +config QCOM_MINIDUMP
>>>> + tristate "QCOM Minidump Support"
>>>> + depends on ARCH_QCOM || COMPILE_TEST
>>>> + select QCOM_SMEM
>>>> + help
>>>> + Enablement of core minidump feature is controlled from boot firmware
>>>> + side, and this config allow linux to query and manages APPS minidump
>>>> + table.
>>>> +
>>>> + Client drivers can register their internal data structures and debug
>>>> + messages as part of the minidump region and when the SoC is crashed,
>>>> + these selective regions will be dumped instead of the entire DDR.
>>>> + This saves significant amount of time and/or storage space.
>>>> +
>>>> endmenu
>>>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>>>> index 0f43a88..1ebe081 100644
>>>> --- a/drivers/soc/qcom/Makefile
>>>> +++ b/drivers/soc/qcom/Makefile
>>>> @@ -33,3 +33,4 @@ obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>>>> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
>>>> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
>>>> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += ice.o
>>>> +obj-$(CONFIG_QCOM_MINIDUMP) += qcom_minidump.o
>>>> diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
>>>> new file mode 100644
>>>> index 0000000..d107a86
>>>> --- /dev/null
>>>> +++ b/drivers/soc/qcom/qcom_minidump.c
>>>> @@ -0,0 +1,581 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +
>>>> +/*
>>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>>>> + */
>>>> +
>>>> +#include <linux/elf.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/errno.h>
>>>> +#include <linux/export.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/string.h>
>>>> +#include <linux/soc/qcom/smem.h>
>>>> +#include <soc/qcom/qcom_minidump.h>
>>>> +
>>>> +/**
>>>> + * struct minidump_elfhdr - Minidump table elf header
>>>> + * @ehdr: Elf main header
>>>> + * @shdr: Section header
>>>> + * @phdr: Program header
>>>> + * @elf_offset: Section offset in elf
>>>> + * @strtable_idx: String table current index position
>>>> + */
>>>> +struct minidump_elfhdr {
>>>> + struct elfhdr *ehdr;
>>>> + struct elf_shdr *shdr;
>>>> + struct elf_phdr *phdr;
>>>> + size_t elf_offset;
>>>> + size_t strtable_idx;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct minidump - Minidump driver private data
>>>> + * @md_gbl_toc : Global TOC pointer
>>>> + * @md_apss_toc : Application Subsystem TOC pointer
>>>> + * @md_regions : High level OS region base pointer
>>>> + * @elf : Minidump elf header
>>>> + * @dev : Minidump device
>>>> + */
>>>> +struct minidump {
>>>> + struct minidump_global_toc *md_gbl_toc;
>>>> + struct minidump_subsystem *md_apss_toc;
>>>> + struct minidump_region *md_regions;
>>>> + struct minidump_elfhdr elf;
>>>> + struct device *dev;
>>>> +};
>>>> +
>>>> +/*
>>>> + * In some of the Old Qualcomm devices, boot firmware statically allocates 300
>>>> + * as total number of supported region (including all co-processors) in
>>>> + * minidump table out of which linux was using 201. In future, this limitation
>>>> + * from boot firmware might get removed by allocating the region dynamically.
>>>> + * So, keep it compatible with older devices, we can keep the current limit for
>>>> + * Linux to 201.
>>>> + */
>>>> +#define MAX_NUM_ENTRIES 201
>>>> +#define MAX_STRTBL_SIZE (MAX_NUM_ENTRIES * MAX_REGION_NAME_LENGTH)
>>>> +
>>>> +static struct minidump *__md;
>>>
>>> No, no file scope or global scope statics.
>>
>> Sorry, this is done as per recommendation given here [1] and this
>> matches both driver/firmware/qcom_scm.c and driver/soc/qcom/smem.c
>> implementations.
>>
>> [1]
>> https://lore.kernel.org/lkml/[email protected]/
>
> That's not true. You had the static already in v2, before Srini commented.
>
> Look:
> https://lore.kernel.org/lkml/[email protected]/
>
> +static struct minidump minidump;
> +static DEFINE_MUTEX(minidump_lock);
>
> We do not talk about the names.

I apologize for this.

>
>
>>>> +
>>>> + if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
>>>> + ret = -EINVAL;
>>>> + dev_err(&pdev->dev, "minidump table is not initialized: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + mutex_lock(&minidump_lock);
>>>> + md->dev = &pdev->dev;
>>>> + md->md_gbl_toc = mdgtoc;
>>>
>>> What are you protecting here? It's not possible to have concurrent
>>> access to md, is it?
>>
>> Check qcom_apss_minidump_region_{register/unregister} and it is possible
>> that these API gets called parallel to this probe.
>
> Wait, you say that something can modify local variable md before it is
> assigned to __md? How?

No.

>>
>> I agree, i made a mistake in not protecting __md in {register} API
>> but did it unregister API in this patch, which i have fixed in later patch.
>
> No, you are protecting random things. Nothing will concurrently modify
> md and &pdev->dev in this moment. mdgtoc is allocated above, so also
> cannot by modified.
>
> Otherwise show me the hypothetical scenario.

You are correct, it should just protect the assignment.
__md = md;

Thanks
>
>
>>
>>>
>>>> + ret = qcom_minidump_init_apss_subsystem(md);
>>>> + if (ret) {
>>>> + dev_err(&pdev->dev, "apss minidump initialization failed: %d\n", ret);
>>>> + goto unlock;
>>>> + }
>>>> +
>>>> + __md = md;
>>>
>>> No. This is a platform device, so it can have multiple instances.
>>
>> It can have only one instance that is created from SMEM driver probe.
>
> Anyone can instantiate more of them.... how did you solve it?
>
>
>>
>>>
>>>> + /* First entry would be ELF header */
>>>> + ret = qcom_apss_minidump_add_elf_header();
>>>> + if (ret) {
>>>> + dev_err(&pdev->dev, "Failed to add elf header: %d\n", ret);
>>>> + memset(md->md_apss_toc, 0, sizeof(struct minidump_subsystem));
>>>> + __md = NULL;
>>>> + }
>>>> +
>>>> +unlock:
>>>> + mutex_unlock(&minidump_lock);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int qcom_minidump_remove(struct platform_device *pdev)
>>>> +{
>>>> + memset(__md->md_apss_toc, 0, sizeof(struct minidump_subsystem));
>>>> + __md = NULL;
>>>
>>> Don't use __ in variable names. Drop it everywhere.
>>
>> As i said above, this is being followed in other drivers, so followed
>> it here as per recommendation.
>>
>> Let @srini comeback on this.
>
> Which part of coding style recommends __ for driver code?

Will fix this.

>
>>
>>>
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static struct platform_driver qcom_minidump_driver = {
>>>> + .probe = qcom_minidump_probe,
>>>> + .remove = qcom_minidump_remove,
>>>> + .driver = {
>>>> + .name = "qcom-minidump",
>>>> + },
>>>> +};
>>>> +
>>>> +module_platform_driver(qcom_minidump_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("Qualcomm APSS minidump driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> +MODULE_ALIAS("platform:qcom-minidump");
>>>> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
>>>> index 6be7ea9..d459656 100644
>>>> --- a/drivers/soc/qcom/smem.c
>>>> +++ b/drivers/soc/qcom/smem.c
>>>> @@ -279,6 +279,7 @@ struct qcom_smem {
>>>>
>>>> u32 item_count;
>>>> struct platform_device *socinfo;
>>>> + struct platform_device *minidump;
>>>> struct smem_ptable *ptable;
>>>> struct smem_partition global_partition;
>>>> struct smem_partition partitions[SMEM_HOST_COUNT];
>>>> @@ -1151,12 +1152,19 @@ static int qcom_smem_probe(struct platform_device *pdev)
>>>> if (IS_ERR(smem->socinfo))
>>>> dev_dbg(&pdev->dev, "failed to register socinfo device\n");
>>>>
>>>> + smem->minidump = platform_device_register_data(&pdev->dev, "qcom-minidump",
>>>> + PLATFORM_DEVID_NONE, NULL,
>>>> + 0);
>>>> + if (IS_ERR(smem->minidump))
>>>> + dev_dbg(&pdev->dev, "failed to register minidump device\n");
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>> static int qcom_smem_remove(struct platform_device *pdev)
>>>> {
>>>> platform_device_unregister(__smem->socinfo);
>>>> + platform_device_unregister(__smem->minidump);
>>>
>>> Wrong order. You registered first socinfo, right?
>>
>> Any order is fine here, they are not dependent.
>> But, will fix this.
>
> No, the order is always reversed from allocation. It does not matter if
> they are dependent or not.

Ok

>
> Best regards,
> Krzysztof
>

-- Mukesh

2023-05-08 07:31:02

by Mukesh Ojha

[permalink] [raw]

Subject: Re: [PATCH v3 04/18] soc: qcom: Add Qualcomm minidump kernel driver

On 5/4/2023 10:04 PM, Krzysztof Kozlowski wrote:
> On 04/05/2023 17:21, Krzysztof Kozlowski wrote:
>>>>
>>>>> + ret = qcom_minidump_init_apss_subsystem(md);
>>>>> + if (ret) {
>>>>> + dev_err(&pdev->dev, "apss minidump initialization failed: %d\n", ret);
>>>>> + goto unlock;
>>>>> + }
>>>>> +
>>>>> + __md = md;
>>>>
>>>> No. This is a platform device, so it can have multiple instances.
>>>
>>> It can have only one instance that is created from SMEM driver probe.
>>
>> Anyone can instantiate more of them.... how did you solve it?
>
> To clarify - sprinkling more of singletons makes everything tightly
> coupled, difficult to debug and non-portable. You cannot have two
> instances, you have to control concurrent initialization by yourself in
> each of such singletons.
>
> I understand sometimes they are unavoidable, for example when this does
> not map to hardware property. However here you have the parent - smem -
> which can return you valid instance. Thus you avoid entire problem of
> file-scope variables.

I get your point, why one's should avoid file scope variables.

This is infrastructure driver and will not have multiple instances and
even if it happens could be avoided with with the help of global mutex
and protect below function which i am already doing at the moment and
fail the other probe if it is already initialized with proper logging..e.g

"already initialized..."

ret = qcom_minidump_init_apss_subsystem(md);

And this will be in-lined with

/* Pointer to the one and only smem handle */
static struct qcom_smem *__smem;

Let me know if you still disagree...and have some other way ?

-- Mukesh

>
> Best regards,
> Krzysztof
>

2023-05-09 07:35:42

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 04/18] soc: qcom: Add Qualcomm minidump kernel driver

On 08/05/2023 09:10, Mukesh Ojha wrote:
>
>
> On 5/4/2023 10:04 PM, Krzysztof Kozlowski wrote:
>> On 04/05/2023 17:21, Krzysztof Kozlowski wrote:
>>>>>
>>>>>> + ret = qcom_minidump_init_apss_subsystem(md);
>>>>>> + if (ret) {
>>>>>> + dev_err(&pdev->dev, "apss minidump initialization failed: %d\n", ret);
>>>>>> + goto unlock;
>>>>>> + }
>>>>>> +
>>>>>> + __md = md;
>>>>>
>>>>> No. This is a platform device, so it can have multiple instances.
>>>>
>>>> It can have only one instance that is created from SMEM driver probe.
>>>
>>> Anyone can instantiate more of them.... how did you solve it?
>>
>> To clarify - sprinkling more of singletons makes everything tightly
>> coupled, difficult to debug and non-portable. You cannot have two
>> instances, you have to control concurrent initialization by yourself in
>> each of such singletons.
>>
>> I understand sometimes they are unavoidable, for example when this does
>> not map to hardware property. However here you have the parent - smem -
>> which can return you valid instance. Thus you avoid entire problem of
>> file-scope variables.
>
> I get your point, why one's should avoid file scope variables.
>
>
> This is infrastructure driver and will not have multiple instances and
> even if it happens could be avoided with with the help of global mutex
> and protect below function which i am already doing at the moment and

But we do not want global mutexes... so incorrect design is being
improved by more incorrect design.

> fail the other probe if it is already initialized with proper logging..e.g
>
> "already initialized..."
>
>
> ret = qcom_minidump_init_apss_subsystem(md);
>
>
> And this will be in-lined with
>
> /* Pointer to the one and only smem handle */
> static struct qcom_smem *__smem;
>
> Let me know if you still disagree...and have some other way ?

Why the parent - smem - cannot return every consumer the instance it
has? There will be one smem having only one minidump, so all problems
solved?

Best regards,
Krzysztof

2023-05-09 15:59:51

by Luca Stefani

[permalink] [raw]

Subject: Re: [PATCH v3 12/18] soc: qcom: Register pstore frontend region with minidump

FYI the following comments also apply to the downstream driver, as the
same bogus logic is implemented.

On 03/05/23 19:02, Mukesh Ojha wrote:

> Since qcom_pstore_minidump driver creates platform device
> for qualcomm devices, so it knows the physical addresses
> of the frontend region now. Let's register the regions
> with qcom_minidump driver.
>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> drivers/soc/qcom/qcom_pstore_minidump.c | 80 ++++++++++++++++++++++++++++++++-
> 1 file changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/qcom/qcom_pstore_minidump.c b/drivers/soc/qcom/qcom_pstore_minidump.c
> index 8d58500..c2bba4e 100644
> --- a/drivers/soc/qcom/qcom_pstore_minidump.c
> +++ b/drivers/soc/qcom/qcom_pstore_minidump.c
> @@ -11,6 +11,8 @@
> #include <linux/pstore_ram.h>
> #include <soc/qcom/qcom_minidump.h>
>
> +#define QCOM_PSTORE_TYPE_MAX 4
Unused
> +
> struct qcom_ramoops_config {
> unsigned long record_size;
> unsigned long console_size;
> @@ -24,6 +26,11 @@ struct qcom_ramoops_config {
> struct qcom_ramoops_dd {
> struct ramoops_platform_data qcom_ramoops_pdata;
> struct platform_device *ramoops_pdev;
> + struct device *dev;
> + struct qcom_apss_minidump_region *record_region;

In the pstore driver record_size is used to split the KDMESG region into
different chunks.

There's no "record" region anywhere in RAM that should be preserved, it
should instead be the dmesg_region.

> + struct qcom_apss_minidump_region *console_region;
> + struct qcom_apss_minidump_region *pmsg_region;
> + struct qcom_apss_minidump_region *ftrace_region;
> };
>
> static struct qcom_ramoops_config default_ramoops_config = {
> @@ -35,6 +42,64 @@ static struct qcom_ramoops_config default_ramoops_config = {
> };
>
> static struct qcom_ramoops_dd *qcom_rdd;
> +
> +static int
> +__qcom_ramoops_minidump_region_register(struct qcom_apss_minidump_region *md_region,
> + const char *name, phys_addr_t phys_addr,
> + unsigned long size)
> +{
> + int ret;
> +
> + if (!size)
> + return 0;
> +
> + md_region = devm_kzalloc(qcom_rdd->dev, sizeof(*md_region), GFP_KERNEL);
> + if (!md_region)
> + return -ENOMEM;
> +
> + strlcpy(md_region->name, name, sizeof(md_region->name));
> + md_region->phys_addr = phys_addr;
> + md_region->virt_addr = phys_to_virt(phys_addr);
> + md_region->size = size;
> + ret = qcom_apss_minidump_region_register(md_region);
> + if (ret)
> + dev_err(qcom_rdd->dev,
> + "failed to add %s in minidump: err: %d\n", name, ret);
> +
> + return ret;
> +}
> +
> +static int
> +qcom_ramoops_minidump_region_register(struct ramoops_platform_data *qcom_ramoops_data)
> +{
> + phys_addr_t phys_addr;
> + int ret = 0;
> +
> + phys_addr = qcom_ramoops_data->mem_address;
> + ret = __qcom_ramoops_minidump_region_register(qcom_rdd->record_region,
> + "KDMESG", phys_addr, qcom_ramoops_data->record_size);

You can't use record_size here as it's not the actual size of the dmesg
region.

The size is calculated in fs/pstore/ram.c as mem_size - console_size -
ftrace_size - pmsg_size, where mem_size is the size on the ramoops
memory region in the devicetree.

Since the actual size is never exposed by the pstore driver you have to
re-purpose the same logic in this driver, hoping it never changes.

> + if (ret)
> + return ret;
> +
> + phys_addr += qcom_ramoops_data->record_size;
> + ret = __qcom_ramoops_minidump_region_register(qcom_rdd->console_region,
> + "KCONSOLE", phys_addr, qcom_ramoops_data->console_size);
> + if (ret)
> + return ret;
> +
> + phys_addr += qcom_ramoops_data->console_size;
> + ret = __qcom_ramoops_minidump_region_register(qcom_rdd->pmsg_region,
> + "KPMSG", phys_addr, qcom_ramoops_data->pmsg_size);
> + if (ret)
> + return ret;
> +
> + phys_addr += qcom_ramoops_data->pmsg_size;
> + ret = __qcom_ramoops_minidump_region_register(qcom_rdd->ftrace_region,
> + "KFTRACE", phys_addr, qcom_ramoops_data->ftrace_size);
> +
> + return ret;
> +}
> +
> static int qcom_ramoops_probe(struct platform_device *pdev)
> {
> struct device_node *of_node = pdev->dev.of_node;
> @@ -59,6 +124,7 @@ static int qcom_ramoops_probe(struct platform_device *pdev)
> if (!qcom_rdd)
> return -ENOMEM;
>
> + qcom_rdd->dev = &pdev->dev;
> cfg = of_device_get_match_data(&pdev->dev);
> if (!cfg) {
> dev_err(&pdev->dev, "failed to get supported matched data\n");
> @@ -81,13 +147,25 @@ static int qcom_ramoops_probe(struct platform_device *pdev)
> ret = PTR_ERR(qcom_rdd->ramoops_pdev);
> dev_err(&pdev->dev, "could not create platform device: %ld\n", ret);
> qcom_rdd->ramoops_pdev = NULL;
> + return ret;
> }
>
> - return ret;
> + return qcom_ramoops_minidump_region_register(pdata);
> }
>
> static int qcom_ramoops_remove(struct platform_device *pdev)
> {
> + struct ramoops_platform_data *pdata;
> +
> + pdata = &qcom_rdd->qcom_ramoops_pdata;
> + if (pdata->record_size)
> + qcom_apss_minidump_region_unregister(qcom_rdd->record_region);
> + if (pdata->console_size)
> + qcom_apss_minidump_region_unregister(qcom_rdd->console_region);
> + if (pdata->pmsg_size)
> + qcom_apss_minidump_region_unregister(qcom_rdd->pmsg_region);
> + if (pdata->ftrace_size)
> + qcom_apss_minidump_region_unregister(qcom_rdd->ftrace_region);
> platform_device_unregister(qcom_rdd->ramoops_pdev);
> qcom_rdd->ramoops_pdev = NULL;
>

Regards,

Luca Stefani

2023-05-09 16:16:17

by Luca Stefani

[permalink] [raw]

Subject: Re: [PATCH v3 09/18] soc: qcom: Add qcom's pstore minidump driver support

On 03/05/23 19:02, Mukesh Ojha wrote:
> This driver was inspired from the fact pstore ram region should be
> fixed and boot firmware need to have awarness about this region,
> so that it will be persistent across boot. But, there are many
> QCOM SoC which does not support warm boot from hardware but they
> have minidump support from the software, and for them, there is
> no need of this pstore ram region to be fixed, but at the same
> time have interest in the pstore frontends. So, this driver
> get the dynamic reserved region from the ram and register the
> ramoops platform device.
>
> +---------+ +---------+ +--------+ +---------+
> | console | | pmsg | | ftrace | | dmesg |
> +---------+ +---------+ +--------+ +---------+
> | | | |
> | | | |
> +------------------------------------------+
> |
> \ /
> +----------------+
> (1) |pstore frontends|
> +----------------+
> |
> \ /
> +------------------- +
> (2) | pstore backend(ram)|
> +--------------------+
> |
> \ /
> +--------------------+
> (3) |qcom_pstore_minidump|
> +--------------------+
> |
> \ /
> +---------------+
> (4) | qcom_minidump |
> +---------------+
>
> This driver will route all the pstore front data to the stored
> in qcom pstore reserved region and the reason of showing an
> arrow from (3) to (4) as qcom_pstore_minidump driver will register
> all the available frontends region with qcom minidump driver
> in upcoming patch.
>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> drivers/soc/qcom/Kconfig | 11 +++
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom_pstore_minidump.c | 116 ++++++++++++++++++++++++++++++++
> 3 files changed, 128 insertions(+)
> create mode 100644 drivers/soc/qcom/qcom_pstore_minidump.c
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 15c931e..afdc634 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -293,4 +293,15 @@ config QCOM_MINIDUMP
> these selective regions will be dumped instead of the entire DDR.
> This saves significant amount of time and/or storage space.
>
> +config QCOM_PSTORE_MINIDUMP
> + tristate "Pstore support for QCOM Minidump"
> + depends on ARCH_QCOM
> + depends on PSTORE_RAM
> + depends on QCOM_MINIDUMP
> + help
> + Enablement of this driver ensures that ramoops region can be anywhere
> + reserved in ram instead of being fixed address which needs boot firmware
> + awareness. So, this driver creates plaform device and registers available
> + frontend region with the Qualcomm's minidump driver.
> +
> endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 1ebe081..02d30d7 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -34,3 +34,4 @@ obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += ice.o
> obj-$(CONFIG_QCOM_MINIDUMP) += qcom_minidump.o
> +obj-$(CONFIG_QCOM_PSTORE_MINIDUMP) += qcom_pstore_minidump.o
> diff --git a/drivers/soc/qcom/qcom_pstore_minidump.c b/drivers/soc/qcom/qcom_pstore_minidump.c
> new file mode 100644
> index 0000000..8d58500
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom_pstore_minidump.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +#include <linux/pstore_ram.h>
> +#include <soc/qcom/qcom_minidump.h>
> +
> +struct qcom_ramoops_config {
> + unsigned long record_size;
> + unsigned long console_size;
> + unsigned long ftrace_size;
> + unsigned long pmsg_size;
> + unsigned int mem_type;
> + unsigned int flags;
> + int max_reason;
> +};
> +
> +struct qcom_ramoops_dd {
> + struct ramoops_platform_data qcom_ramoops_pdata;
> + struct platform_device *ramoops_pdev;
> +};
> +
> +static struct qcom_ramoops_config default_ramoops_config = {
> + .mem_type = 2,
> + .record_size = 0x0,
> + .console_size = 0x200000,
> + .ftrace_size = 0x0,
> + .pmsg_size = 0x0,
> +};

This is effectively hard-cording the configuration of ramoops.

Since the memory range is dynamic and by itself doesn't impose any
limitation this should be configurable in the device-tree, like a
standard ramoops entry backed by a memory range.

I think this should provide the same interface/knobs as pstore-ram does,
unless there's some known limitations to minidump, in which case those
should be expressed.

> +
> +static struct qcom_ramoops_dd *qcom_rdd;
> +static int qcom_ramoops_probe(struct platform_device *pdev)
> +{
> + struct device_node *of_node = pdev->dev.of_node;
> + struct device_node *node;
> + const struct qcom_ramoops_config *cfg;
> + struct ramoops_platform_data *pdata;
> + struct reserved_mem *rmem;
> + long ret;
> +
> + node = of_parse_phandle(of_node, "memory-region", 0);
> + if (!node)
> + return -ENODEV;
> +
> + rmem = of_reserved_mem_lookup(node);
> + of_node_put(node);
> + if (!rmem) {
> + dev_err(&pdev->dev, "failed to locate DT /reserved-memory resource\n");
> + return -EINVAL;
> + }
> +
> + qcom_rdd = devm_kzalloc(&pdev->dev, sizeof(*qcom_rdd), GFP_KERNEL);
> + if (!qcom_rdd)
> + return -ENOMEM;
> +
> + cfg = of_device_get_match_data(&pdev->dev);
> + if (!cfg) {
> + dev_err(&pdev->dev, "failed to get supported matched data\n");
> + return -ENOENT;
> + }
> +
> + pdata = &qcom_rdd->qcom_ramoops_pdata;
> + pdata->mem_size = rmem->size;
> + pdata->mem_address = rmem->base;
> + pdata->mem_type = cfg->mem_type;
> + pdata->record_size = cfg->record_size;
> + pdata->console_size = cfg->console_size;
> + pdata->ftrace_size = cfg->ftrace_size;
> + pdata->pmsg_size = cfg->pmsg_size;
> + pdata->max_reason = KMSG_DUMP_PANIC;
> +
> + qcom_rdd->ramoops_pdev = platform_device_register_data(NULL, "ramoops", -1,
> + pdata, sizeof(*pdata));
> + if (IS_ERR(qcom_rdd->ramoops_pdev)) {
> + ret = PTR_ERR(qcom_rdd->ramoops_pdev);
> + dev_err(&pdev->dev, "could not create platform device: %ld\n", ret);
> + qcom_rdd->ramoops_pdev = NULL;
> + }
> +
> + return ret;
> +}
> +
> +static int qcom_ramoops_remove(struct platform_device *pdev)
> +{
> + platform_device_unregister(qcom_rdd->ramoops_pdev);
> + qcom_rdd->ramoops_pdev = NULL;
> +
> + return 0;
> +}
> +
> +static const struct of_device_id qcom_ramoops_of_match[] = {
> + { .compatible = "qcom,sm8450-ramoops-minidump", .data = &default_ramoops_config },
> + { .compatible = "qcom,ramoops-minidump", .data = &default_ramoops_config },
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(of, qcom_ramoops_of_match);
> +static struct platform_driver qcom_ramoops_drv = {
> + .driver = {
> + .name = "qcom,ramoops-minidump",
> + .of_match_table = qcom_ramoops_of_match,
> + },
> + .probe = qcom_ramoops_probe,
> + .remove = qcom_ramoops_remove,
> +};
> +
> +module_platform_driver(qcom_ramoops_drv);
> +
> +MODULE_DESCRIPTION("Qualcomm minidump pstore driver");
> +MODULE_LICENSE("GPL");

2023-05-14 05:50:48

by Trilok Soni

[permalink] [raw]

Subject: Re: [PATCH v3 04/18] soc: qcom: Add Qualcomm minidump kernel driver

On 5/8/2023 12:10 AM, Mukesh Ojha wrote:
>
>
> On 5/4/2023 10:04 PM, Krzysztof Kozlowski wrote:
>> On 04/05/2023 17:21, Krzysztof Kozlowski wrote:
>>>>>
>>>>>> +    ret = qcom_minidump_init_apss_subsystem(md);
>>>>>> +    if (ret) {
>>>>>> +        dev_err(&pdev->dev, "apss minidump initialization failed:
>>>>>> %d\n", ret);
>>>>>> +        goto unlock;
>>>>>> +    }
>>>>>> +
>>>>>> +    __md = md;
>>>>>
>>>>> No. This is a platform device, so it can have multiple instances.
>>>>
>>>> It can have only one instance that is created from SMEM driver probe.
>>>
>>> Anyone can instantiate more of them.... how did you solve it?
>>
>> To clarify - sprinkling more of singletons makes everything tightly
>> coupled, difficult to debug and non-portable. You cannot have two
>> instances, you have to control concurrent initialization by yourself in
>> each of such singletons.
>>
>> I understand sometimes they are unavoidable, for example when this does
>> not map to hardware property. However here you have the parent - smem -
>> which can return you valid instance. Thus you avoid entire problem of
>> file-scope variables.
>
> I get your point, why one's should avoid file scope variables.
>
>
> This is infrastructure driver and will not have multiple instances and
> even if it happens could be avoided with with the help of global mutex
> and protect below function which i am already doing at the moment and
> fail the other probe if it is already initialized with proper logging..e.g

Another way to think here is what if you have chiplets? Two SOCs looks
like one as a product? How does your driver will behave in those cases?

---Trilok Soni

2023-05-16 21:07:15

by Kees Cook

[permalink] [raw]

Subject: Re: [PATCH v3 12/18] soc: qcom: Register pstore frontend region with minidump

On Wed, May 03, 2023 at 10:32:26PM +0530, Mukesh Ojha wrote:
> Since qcom_pstore_minidump driver creates platform device
> for qualcomm devices, so it knows the physical addresses
> of the frontend region now. Let's register the regions
> with qcom_minidump driver.
>
> Signed-off-by: Mukesh Ojha <[email protected]>
> [...]
> + strlcpy(md_region->name, name, sizeof(md_region->name));

nitpick throughout: Please use strscpy()

https://github.com/KSPP/linux/issues/89

-Kees

--
Kees Cook

2023-05-16 21:08:24

by Kees Cook

[permalink] [raw]

Subject: Re: [PATCH v3 09/18] soc: qcom: Add qcom's pstore minidump driver support

On Tue, May 09, 2023 at 06:06:26PM +0200, Luca Stefani wrote:
>
> On 03/05/23 19:02, Mukesh Ojha wrote:
> > This driver was inspired from the fact pstore ram region should be
> > fixed and boot firmware need to have awarness about this region,
> > so that it will be persistent across boot. But, there are many
> > QCOM SoC which does not support warm boot from hardware but they
> > have minidump support from the software, and for them, there is
> > no need of this pstore ram region to be fixed, but at the same
> > time have interest in the pstore frontends. So, this driver
> > get the dynamic reserved region from the ram and register the
> > ramoops platform device.
> >
> > +---------+ +---------+ +--------+ +---------+
> > | console | | pmsg | | ftrace | | dmesg |
> > +---------+ +---------+ +--------+ +---------+
> > | | | |
> > | | | |
> > +------------------------------------------+
> > |
> > \ /
> > +----------------+
> > (1) |pstore frontends|
> > +----------------+
> > |
> > \ /
> > +------------------- +
> > (2) | pstore backend(ram)|
> > +--------------------+
> > |
> > \ /
> > +--------------------+
> > (3) |qcom_pstore_minidump|
> > +--------------------+
> > |
> > \ /
> > +---------------+
> > (4) | qcom_minidump |
> > +---------------+
> >
> > This driver will route all the pstore front data to the stored
> > in qcom pstore reserved region and the reason of showing an
> > arrow from (3) to (4) as qcom_pstore_minidump driver will register
> > all the available frontends region with qcom minidump driver
> > in upcoming patch.
> >
> > Signed-off-by: Mukesh Ojha <[email protected]>
> [...]
> > +static struct qcom_ramoops_config default_ramoops_config = {
> > + .mem_type = 2,
> > + .record_size = 0x0,
> > + .console_size = 0x200000,
> > + .ftrace_size = 0x0,
> > + .pmsg_size = 0x0,
> > +};
>
> This is effectively hard-cording the configuration of ramoops.
>
> Since the memory range is dynamic and by itself doesn't impose any
> limitation this should be configurable in the device-tree, like a standard
> ramoops entry backed by a memory range.
>
> I think this should provide the same interface/knobs as pstore-ram does,
> unless there's some known limitations to minidump, in which case those
> should be expressed.

Yeah, I had the same thought reading this myself. Beyond that, it looks
fine as a way to let pstore know about a new RAM backend.

-Kees

--
Kees Cook

2023-05-18 18:57:36

by Trilok Soni

[permalink] [raw]

Subject: Re: [PATCH v3 14/18] firmware: qcom_scm: provide a read-modify-write function

On 5/3/2023 10:02 AM, Mukesh Ojha wrote:
> It was realized by Srinivas K. that there is a need of
> read-modify-write scm exported function so that it can
> be used by multiple clients.
>
> Let's introduce qcom_scm_io_update_field() which masks
> out the bits and write the passed value to that
> bit-offset. Subsequent patch will use this function.
>
> Suggested-by: Srinivas Kandagatla <[email protected]>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> drivers/firmware/qcom_scm.c | 15 +++++++++++++++
> include/linux/firmware/qcom/qcom_scm.h | 2 ++
> 2 files changed, 17 insertions(+)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index fde33acd..003cbcb 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -407,6 +407,21 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
> }
> EXPORT_SYMBOL(qcom_scm_set_remote_state);
>
> +int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, unsigned int val)
> +{
> + unsigned int old, new;
> + int ret;
> +
> + ret = qcom_scm_io_readl(addr, &old);
> + if (ret)
> + return ret;
> +
> + new = (old & ~mask) | val << (ffs(mask) - 1);
> +
> + return qcom_scm_io_writel(addr, new);
> +}
> +EXPORT_SYMBOL(qcom_scm_io_update_field);

EXPORT_SYMBOL_GPL?

There are other symbols in this driver marked as EXPORT_SYMBOL only. Can
you submit a separate patch to convert them into EXPORT_SYMBOL_GPL. OR I
want to know why we are marking them without _GPL.

checkpatch should catch this as well? By default we should have all the
new additions as EXPORT_SYMBOL_GPL and only few existing APIs w/
exceptions?

---Trilok Soni

2023-05-28 11:41:32

by Mukesh Ojha

[permalink] [raw]

Subject: Re: [PATCH v3 04/18] soc: qcom: Add Qualcomm minidump kernel driver

Hi Krzysztof,

On 5/9/2023 12:41 PM, Krzysztof Kozlowski wrote:
> On 08/05/2023 09:10, Mukesh Ojha wrote:
>>
>>
>> On 5/4/2023 10:04 PM, Krzysztof Kozlowski wrote:
>>> On 04/05/2023 17:21, Krzysztof Kozlowski wrote:
>>>>>>
>>>>>>> + ret = qcom_minidump_init_apss_subsystem(md);
>>>>>>> + if (ret) {
>>>>>>> + dev_err(&pdev->dev, "apss minidump initialization failed: %d\n", ret);
>>>>>>> + goto unlock;
>>>>>>> + }
>>>>>>> +
>>>>>>> + __md = md;
>>>>>>
>>>>>> No. This is a platform device, so it can have multiple instances.
>>>>>
>>>>> It can have only one instance that is created from SMEM driver probe.
>>>>
>>>> Anyone can instantiate more of them.... how did you solve it?
>>>
>>> To clarify - sprinkling more of singletons makes everything tightly
>>> coupled, difficult to debug and non-portable. You cannot have two
>>> instances, you have to control concurrent initialization by yourself in
>>> each of such singletons.
>>>
>>> I understand sometimes they are unavoidable, for example when this does
>>> not map to hardware property. However here you have the parent - smem -
>>> which can return you valid instance. Thus you avoid entire problem of
>>> file-scope variables.
>>
>> I get your point, why one's should avoid file scope variables.
>>
>>
>> This is infrastructure driver and will not have multiple instances and
>> even if it happens could be avoided with with the help of global mutex
>> and protect below function which i am already doing at the moment and
>
> But we do not want global mutexes... so incorrect design is being
> improved by more incorrect design.
>
>> fail the other probe if it is already initialized with proper logging..e.g
>>
>> "already initialized..."
>>
>>
>> ret = qcom_minidump_init_apss_subsystem(md);
>>
>>
>> And this will be in-lined with
>>
>> /* Pointer to the one and only smem handle */
>> static struct qcom_smem *__smem;
>>
>> Let me know if you still disagree...and have some other way ?
>
> Why the parent - smem - cannot return every consumer the instance it
> has? There will be one smem having only one minidump, so all problems
> solved?

Sorry, I am extending this discussion but it is needed to avoid rework
in upcoming patches.

I am inline with the thought of each smem has its own minidump instance,
which is basically one at this moment as SMEM has only instance in DT.
In that way, Client driver calling qcom_apss_minidump_region_register()
will also need to know the instance it need to register with right?

However, I do have a use case [1] where SMEM or similar region
supporting memory mapped region could be virtualized and guest vm does
not have direct access to it, that way it will only have one backend at
a time.But even if they exist together that can be done with below approach.

File scope variable is still needed in minidump core but can be avoided
in backend drivers where each backend register with core and get added
itself in the list and for list protection, list mutex would be needed.

#define SMEM 0;
#define MMIO 1;
or enum may be..

And client can call this to the instance it need to register with..
int qcom_apss_minidump_region_register(region, SMEM);
int qcom_apss_minidump_region_register(region, MMIO);

Do you agree with this approach?

[1]

+----------------+
| |
| client A-Z |
+-----+----------+
|
|
|
|
v
+------------------------+
| | other backends
| minidump core +----------------------------+
| | |
+--+---------------------+ |
| | |
| | |
| | |e.g,
| | |gunyah-rm
+--------v------+ +-----v-----------+ +--+---------+
| | | | | |
|minidump_smem | | minidump_mmio | | ..... |
+---------------+ +-----------------+ +------------+
SMEM backend mmio backend where
smem may be virtualized

-- Mukesh
>
> Best regards,
> Krzysztof
>

2023-06-02 11:22:55

by Mukesh Ojha

[permalink] [raw]

Subject: Re: [PATCH v3 04/18] soc: qcom: Add Qualcomm minidump kernel driver

On 5/4/2023 5:06 PM, Krzysztof Kozlowski wrote:
> On 03/05/2023 19:02, Mukesh Ojha wrote:
>> Minidump is a best effort mechanism to collect useful and predefined
>> data for first level of debugging on end user devices running on
>> Qualcomm SoCs. It is built on the premise that System on Chip (SoC)
>> or subsystem part of SoC crashes, due to a range of hardware and
>> software bugs. Hence, the ability to collect accurate data is only
>> a best-effort. The data collected could be invalid or corrupted,
>> data collection itself could fail, and so on.
>>
>> Qualcomm devices in engineering mode provides a mechanism for
>> generating full system ramdumps for post mortem debugging. But in some
>> cases it's however not feasible to capture the entire content of RAM.
>> The minidump mechanism provides the means for selecting region should
>> be included in the ramdump. The solution supports extracting the
>> ramdump/minidump produced either over USB or stored to an attached
>> storage device.
>>
>> The core of minidump feature is part of Qualcomm's boot firmware code.
>> It initializes shared memory(SMEM), which is a part of DDR and
>> allocates a small section of it to minidump table i.e also called
>> global table of content (G-ToC). Each subsystem (APSS, ADSP, ...) has
>> their own table of segments to be included in the minidump, all
>> references from a descriptor in SMEM (G-ToC). Each segment/region has
>> some details like name, physical address and it's size etc. and it
>> could be anywhere scattered in the DDR.
>>
>> Minidump kernel driver adds the capability to add linux region to be
>> dumped as part of ram dump collection. It provides appropriate symbol
>> to check its enablement and register client regions.
>>
>> To simplify post mortem debugging, it creates and maintain an ELF
>> header as first region that gets updated upon registration
>> of a new region.
>>
>> Signed-off-by: Mukesh Ojha <[email protected]>
>> ---
>> drivers/soc/qcom/Kconfig | 14 +
>> drivers/soc/qcom/Makefile | 1 +
>> drivers/soc/qcom/qcom_minidump.c | 581 +++++++++++++++++++++++++++++++++++++++
>> drivers/soc/qcom/smem.c | 8 +
>> include/soc/qcom/qcom_minidump.h | 61 +++-
>> 5 files changed, 663 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/soc/qcom/qcom_minidump.c
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index a491718..15c931e 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -279,4 +279,18 @@ config QCOM_INLINE_CRYPTO_ENGINE
>> tristate
>> select QCOM_SCM
>>
>> +config QCOM_MINIDUMP
>> + tristate "QCOM Minidump Support"
>> + depends on ARCH_QCOM || COMPILE_TEST
>> + select QCOM_SMEM
>> + help
>> + Enablement of core minidump feature is controlled from boot firmware
>> + side, and this config allow linux to query and manages APPS minidump
>> + table.
>> +
>> + Client drivers can register their internal data structures and debug
>> + messages as part of the minidump region and when the SoC is crashed,
>> + these selective regions will be dumped instead of the entire DDR.
>> + This saves significant amount of time and/or storage space.
>> +
>> endmenu
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 0f43a88..1ebe081 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -33,3 +33,4 @@ obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
>> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
>> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += ice.o
>> +obj-$(CONFIG_QCOM_MINIDUMP) += qcom_minidump.o
>> diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
>> new file mode 100644
>> index 0000000..d107a86
>> --- /dev/null
>> +++ b/drivers/soc/qcom/qcom_minidump.c
>> @@ -0,0 +1,581 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +/*
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/elf.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/export.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/string.h>
>> +#include <linux/soc/qcom/smem.h>
>> +#include <soc/qcom/qcom_minidump.h>
>> +
>> +/**
>> + * struct minidump_elfhdr - Minidump table elf header
>> + * @ehdr: Elf main header
>> + * @shdr: Section header
>> + * @phdr: Program header
>> + * @elf_offset: Section offset in elf
>> + * @strtable_idx: String table current index position
>> + */
>> +struct minidump_elfhdr {
>> + struct elfhdr *ehdr;
>> + struct elf_shdr *shdr;
>> + struct elf_phdr *phdr;
>> + size_t elf_offset;
>> + size_t strtable_idx;
>> +};
>> +
>> +/**
>> + * struct minidump - Minidump driver private data
>> + * @md_gbl_toc : Global TOC pointer
>> + * @md_apss_toc : Application Subsystem TOC pointer
>> + * @md_regions : High level OS region base pointer
>> + * @elf : Minidump elf header
>> + * @dev : Minidump device
>> + */
>> +struct minidump {
>> + struct minidump_global_toc *md_gbl_toc;
>> + struct minidump_subsystem *md_apss_toc;
>> + struct minidump_region *md_regions;
>> + struct minidump_elfhdr elf;
>> + struct device *dev;
>> +};
>> +
>> +/*
>> + * In some of the Old Qualcomm devices, boot firmware statically allocates 300
>> + * as total number of supported region (including all co-processors) in
>> + * minidump table out of which linux was using 201. In future, this limitation
>> + * from boot firmware might get removed by allocating the region dynamically.
>> + * So, keep it compatible with older devices, we can keep the current limit for
>> + * Linux to 201.
>> + */
>> +#define MAX_NUM_ENTRIES 201
>> +#define MAX_STRTBL_SIZE (MAX_NUM_ENTRIES * MAX_REGION_NAME_LENGTH)
>> +
>> +static struct minidump *__md;
>
> No, no file scope or global scope statics.
>
>> +static DEFINE_MUTEX(minidump_lock);
>
> Neither this.
>
> Also you need to clearly express what is protected by newn lock.

Once i divide this driver into two (minidump core + smem backend driver
as described here[1]) so there could more backend drivers could be
attached to minidump core. Some of the file scope variable still be
required.

[1]
https://lore.kernel.org/lkml/[email protected]/#t

>
>> +
>> +static struct elf_shdr *elf_shdr_entry_addr(struct elfhdr *ehdr, int idx)
>> +{
>> + struct elf_shdr *eshdr = (struct elf_shdr *)((size_t)ehdr + ehdr->e_shoff);
>> +
>> + return &eshdr[idx];
>> +}
>> +
>> +static struct elf_phdr *elf_phdr_entry_addr(struct elfhdr *ehdr, int idx)
>> +{
>> + struct elf_phdr *ephdr = (struct elf_phdr *)((size_t)ehdr + ehdr->e_phoff);
>> +
>> + return &ephdr[idx];
>> +}
>> +
>> +static char *elf_str_table_start(struct elfhdr *ehdr)
>> +{
>> + struct elf_shdr *eshdr;
>> +
>> + if (ehdr->e_shstrndx == SHN_UNDEF)
>> + return NULL;
>> +
>> + eshdr = elf_shdr_entry_addr(ehdr, ehdr->e_shstrndx);
>> + return (char *)ehdr + eshdr->sh_offset;
>> +}
>> +
>> +static char *elf_lookup_string(struct elfhdr *ehdr, int offset)
>> +{
>> + char *strtab = elf_str_table_start(ehdr);
>> +
>> + if (!strtab || (__md->elf.strtable_idx < offset))
>> + return NULL;
>> +
>> + return strtab + offset;
>> +}
>> +
>> +static unsigned int append_str_to_strtable(const char *name)
>> +{
>> + char *strtab = elf_str_table_start(__md->elf.ehdr);
>> + unsigned int old_idx = __md->elf.strtable_idx;
>> + unsigned int ret;
>> +
>> + if (!strtab || !name)
>> + return 0;
>> +
>> + ret = old_idx;
>> + old_idx += strscpy((strtab + old_idx), name, MAX_REGION_NAME_LENGTH);
>> + __md->elf.strtable_idx = old_idx + 1;
>> + return ret;
>> +}
>> +
>> +static int
>> +get_apss_minidump_region_index(const struct qcom_apss_minidump_region *region)
>> +{
>> + struct minidump_region *mdr;
>> + unsigned int i;
>> + unsigned int count;
>> +
>> + count = le32_to_cpu(__md->md_apss_toc->region_count);
>> + for (i = 0; i < count; i++) {
>> + mdr = &__md->md_regions[i];
>> + if (!strcmp(mdr->name, region->name))
>> + return i;
>> + }
>> + return -ENOENT;
>> +}
>> +
>> +static void
>> +qcom_apss_minidump_update_elf_header(const struct qcom_apss_minidump_region *region)
>
> You need to name everything shorter. Neither functions nor structs are
> making it easy to follow.

Would drop "apss" from the name;

>
>> +{
>> + struct elfhdr *ehdr = __md->elf.ehdr;
>> + struct elf_shdr *shdr;
>> + struct elf_phdr *phdr;
>> +
>> + shdr = elf_shdr_entry_addr(ehdr, ehdr->e_shnum++);
>> + phdr = elf_phdr_entry_addr(ehdr, ehdr->e_phnum++);
>> +
>> + shdr->sh_type = SHT_PROGBITS;
>> + shdr->sh_name = append_str_to_strtable(region->name);
>> + shdr->sh_addr = (elf_addr_t)region->virt_addr;
>> + shdr->sh_size = region->size;
>> + shdr->sh_flags = SHF_WRITE;
>> + shdr->sh_offset = __md->elf.elf_offset;
>> + shdr->sh_entsize = 0;
>> +
>> + phdr->p_type = PT_LOAD;
>> + phdr->p_offset = __md->elf.elf_offset;
>> + phdr->p_vaddr = (elf_addr_t)region->virt_addr;
>> + phdr->p_paddr = region->phys_addr;
>> + phdr->p_filesz = phdr->p_memsz = region->size;
>> + phdr->p_flags = PF_R | PF_W;
>> + __md->elf.elf_offset += shdr->sh_size;
>> +}
>> +
>> +static void
>> +qcom_apss_minidump_add_region(const struct qcom_apss_minidump_region *region)
>> +{
>> + struct minidump_region *mdr;
>> + unsigned int region_cnt = le32_to_cpu(__md->md_apss_toc->region_count);
>> +
>> + mdr = &__md->md_regions[region_cnt];
>> + strscpy(mdr->name, region->name, sizeof(mdr->name));
>> + mdr->address = cpu_to_le64(region->phys_addr);
>> + mdr->size = cpu_to_le64(region->size);
>> + mdr->valid = cpu_to_le32(MINIDUMP_REGION_VALID);
>> + region_cnt++;
>> + __md->md_apss_toc->region_count = cpu_to_le32(region_cnt);
>> +}
>> +
>> +static bool
>> +qcom_apss_minidump_valid_region(const struct qcom_apss_minidump_region *region)
>> +{
>> + return region &&
>> + strnlen(region->name, MAX_NAME_LENGTH) < MAX_NAME_LENGTH &&
>> + region->virt_addr &&
>> + region->size &&
>> + IS_ALIGNED(region->size, 4);
>> +}
>> +
>> +static int qcom_apss_minidump_add_elf_header(void)
>> +{
>> + struct qcom_apss_minidump_region elfregion;
>> + struct elfhdr *ehdr;
>> + struct elf_shdr *shdr;
>> + struct elf_phdr *phdr;
>> + unsigned int elfh_size;
>> + unsigned int strtbl_off;
>> + unsigned int phdr_off;
>> + char *banner;
>> + unsigned int banner_len;
>> +
>> + banner_len = strlen(linux_banner);
>> + /*
>> + * Header buffer contains:
>> + * ELF header, (MAX_NUM_ENTRIES + 4) of Section and Program ELF headers,
>> + * where, 4 additional entries, one for empty header, one for string table
>> + * one for minidump table and one for linux banner.
>> + *
>> + * Linux banner is stored in minidump to aid post mortem tools to determine
>> + * the kernel version.
>> + */
>> + elfh_size = sizeof(*ehdr);
>> + elfh_size += MAX_STRTBL_SIZE;
>> + elfh_size += banner_len + 1;
>> + elfh_size += ((sizeof(*shdr) + sizeof(*phdr)) * (MAX_NUM_ENTRIES + 4));
>> + elfh_size = ALIGN(elfh_size, 4);
>> +
>> + __md->elf.ehdr = kzalloc(elfh_size, GFP_KERNEL);
>> + if (!__md->elf.ehdr)
>> + return -ENOMEM;
>> +
>> + /* Register ELF header as first region */
>> + strscpy(elfregion.name, "KELF_HEADER", sizeof(elfregion.name));
>> + elfregion.virt_addr = __md->elf.ehdr;
>> + elfregion.phys_addr = virt_to_phys(__md->elf.ehdr);
>> + elfregion.size = elfh_size;
>> + qcom_apss_minidump_add_region(&elfregion);
>> +
>> + ehdr = __md->elf.ehdr;
>> + /* Assign Section/Program headers offset */
>> + __md->elf.shdr = shdr = (struct elf_shdr *)(ehdr + 1);
>> + __md->elf.phdr = phdr = (struct elf_phdr *)(shdr + MAX_NUM_ENTRIES);
>> + phdr_off = sizeof(*ehdr) + (sizeof(*shdr) * MAX_NUM_ENTRIES);
>> +
>> + memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
>> + ehdr->e_ident[EI_CLASS] = ELF_CLASS;
>> + ehdr->e_ident[EI_DATA] = ELF_DATA;
>> + ehdr->e_ident[EI_VERSION] = EV_CURRENT;
>> + ehdr->e_ident[EI_OSABI] = ELF_OSABI;
>> + ehdr->e_type = ET_CORE;
>> + ehdr->e_machine = ELF_ARCH;
>> + ehdr->e_version = EV_CURRENT;
>> + ehdr->e_ehsize = sizeof(*ehdr);
>> + ehdr->e_phoff = phdr_off;
>> + ehdr->e_phentsize = sizeof(*phdr);
>> + ehdr->e_shoff = sizeof(*ehdr);
>> + ehdr->e_shentsize = sizeof(*shdr);
>> + ehdr->e_shstrndx = 1;
>> +
>> + __md->elf.elf_offset = elfh_size;
>> +
>> + /*
>> + * The zeroth index of the section header is reserved and is rarely used.
>> + * Set the section header as null (SHN_UNDEF) and move to the next one.
>> + * 2nd Section is String table.
>> + */
>> + __md->elf.strtable_idx = 1;
>> + strtbl_off = sizeof(*ehdr) + ((sizeof(*phdr) + sizeof(*shdr)) * MAX_NUM_ENTRIES);
>> + shdr++;
>> + shdr->sh_type = SHT_STRTAB;
>> + shdr->sh_offset = (elf_addr_t)strtbl_off;
>> + shdr->sh_size = MAX_STRTBL_SIZE;
>> + shdr->sh_entsize = 0;
>> + shdr->sh_flags = 0;
>> + shdr->sh_name = append_str_to_strtable("STR_TBL");
>> + shdr++;
>> +
>> + /* 3rd Section is Linux banner */
>> + banner = (char *)ehdr + strtbl_off + MAX_STRTBL_SIZE;
>> + memcpy(banner, linux_banner, banner_len);
>> +
>> + shdr->sh_type = SHT_PROGBITS;
>> + shdr->sh_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
>> + shdr->sh_size = banner_len + 1;
>> + shdr->sh_addr = (elf_addr_t)linux_banner;
>> + shdr->sh_entsize = 0;
>> + shdr->sh_flags = SHF_WRITE;
>> + shdr->sh_name = append_str_to_strtable("linux_banner");
>> +
>> + phdr->p_type = PT_LOAD;
>> + phdr->p_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
>> + phdr->p_vaddr = (elf_addr_t)linux_banner;
>> + phdr->p_paddr = virt_to_phys(linux_banner);
>> + phdr->p_filesz = phdr->p_memsz = banner_len + 1;
>> + phdr->p_flags = PF_R | PF_W;
>> +
>> + /*
>> + * Above are some prdefined sections/program header used
>> + * for debug, update their count here.
>> + */
>> + ehdr->e_phnum = 1;
>> + ehdr->e_shnum = 3;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * qcom_minidump_subsystem_desc() - Get minidump subsystem descriptor.
>> + * @minidump_index: minidump index for a subsystem in minidump table
>> + *
>> + * Return: minidump subsystem descriptor address on success and error
>> + * on failure
>> + */
>> +struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index)
>> +{
>> + struct minidump_subsystem *md_ss_toc;
>> +
>> + mutex_lock(&minidump_lock);
>> + if (!__md) {
>> + md_ss_toc = ERR_PTR(-EPROBE_DEFER);
>> + goto unlock;
>> + }
>> +
>> + md_ss_toc = &__md->md_gbl_toc->subsystems[minidump_index];
>> +unlock:
>> + mutex_unlock(&minidump_lock);
>> + return md_ss_toc;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_minidump_subsystem_desc);
>> +
>> +/**
>> + * qcom_apss_minidump_region_register() - Register a region in Minidump table.
>> + * @region: minidump region.
>> + *
>> + * Return: On success, it returns 0, otherwise a negative error value on failure.
>> + */
>> +int qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region)
>> +{
>> + unsigned int num_region;
>> + int ret;
>> +
>> + if (!__md)
>> + return -EPROBE_DEFER;
>> +
>> + if (!qcom_apss_minidump_valid_region(region))
>> + return -EINVAL;
>> +
>> + mutex_lock(&minidump_lock);
>> + ret = get_apss_minidump_region_index(region);
>> + if (ret >= 0) {
>> + dev_info(__md->dev, "%s region is already registered\n", region->name);
>> + ret = -EEXIST;
>> + goto unlock;
>> + }
>> +
>> + /* Check if there is a room for a new entry */
>> + num_region = le32_to_cpu(__md->md_apss_toc->region_count);
>> + if (num_region >= MAX_NUM_ENTRIES) {
>> + dev_err(__md->dev, "maximum region limit %u reached\n", num_region);
>> + ret = -ENOSPC;
>> + goto unlock;
>> + }
>> +
>> + qcom_apss_minidump_add_region(region);
>> + qcom_apss_minidump_update_elf_header(region);
>> + ret = 0;
>> +unlock:
>> + mutex_unlock(&minidump_lock);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_apss_minidump_region_register);
>> +
>> +static int
>> +qcom_apss_minidump_clear_header(const struct qcom_apss_minidump_region *region)
>> +{
>> + struct elfhdr *ehdr = __md->elf.ehdr;
>> + struct elf_shdr *shdr;
>> + struct elf_shdr *tmp_shdr;
>> + struct elf_phdr *phdr;
>> + struct elf_phdr *tmp_phdr;
>> + unsigned int phidx;
>> + unsigned int shidx;
>> + unsigned int len;
>> + unsigned int i;
>> + char *shname;
>> +
>> + for (i = 0; i < ehdr->e_phnum; i++) {
>> + phdr = elf_phdr_entry_addr(ehdr, i);
>> + if (phdr->p_paddr == region->phys_addr &&
>> + phdr->p_memsz == region->size)
>> + break;
>> + }
>> +
>> + if (i == ehdr->e_phnum) {
>> + dev_err(__md->dev, "Cannot find program header entry in elf\n");
>> + return -EINVAL;
>> + }
>> +
>> + phidx = i;
>> + for (i = 0; i < ehdr->e_shnum; i++) {
>> + shdr = elf_shdr_entry_addr(ehdr, i);
>> + shname = elf_lookup_string(ehdr, shdr->sh_name);
>> + if (shname && !strcmp(shname, region->name) &&
>> + shdr->sh_addr == (elf_addr_t)region->virt_addr &&
>> + shdr->sh_size == region->size)
>> + break;
>> + }
>> +
>> + if (i == ehdr->e_shnum) {
>> + dev_err(__md->dev, "Cannot find section header entry in elf\n");
>> + return -EINVAL;
>> + }
>> +
>> + shidx = i;
>> + if (shdr->sh_offset != phdr->p_offset) {
>> + dev_err(__md->dev, "Invalid entry details for region: %s\n", region->name);
>> + return -EINVAL;
>> + }
>> +
>> + /* Clear name in string table */
>> + len = strlen(shname) + 1;
>> + memmove(shname, shname + len,
>> + __md->elf.strtable_idx - shdr->sh_name - len);
>> + __md->elf.strtable_idx -= len;
>> +
>> + /* Clear program header */
>> + tmp_phdr = elf_phdr_entry_addr(ehdr, phidx);
>> + for (i = phidx; i < ehdr->e_phnum - 1; i++) {
>> + tmp_phdr = elf_phdr_entry_addr(ehdr, i + 1);
>> + phdr = elf_phdr_entry_addr(ehdr, i);
>> + memcpy(phdr, tmp_phdr, sizeof(struct elf_phdr));
>> + phdr->p_offset = phdr->p_offset - region->size;
>> + }
>> + memset(tmp_phdr, 0, sizeof(struct elf_phdr));
>> + ehdr->e_phnum--;
>> +
>> + /* Clear section header */
>> + tmp_shdr = elf_shdr_entry_addr(ehdr, shidx);
>> + for (i = shidx; i < ehdr->e_shnum - 1; i++) {
>> + tmp_shdr = elf_shdr_entry_addr(ehdr, i + 1);
>> + shdr = elf_shdr_entry_addr(ehdr, i);
>> + memcpy(shdr, tmp_shdr, sizeof(struct elf_shdr));
>> + shdr->sh_offset -= region->size;
>> + shdr->sh_name -= len;
>> + }
>> +
>> + memset(tmp_shdr, 0, sizeof(struct elf_shdr));
>> + ehdr->e_shnum--;
>> + __md->elf.elf_offset -= region->size;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * qcom_apss_minidump_region_unregister() - Unregister region from Minidump table.
>> + * @region: minidump region.
>> + *
>> + * Return: On success, it returns 0 and negative error value on failure.
>> + */
>> +int qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region)
>> +{
>> + struct minidump_region *mdr;
>> + unsigned int num_region;
>> + unsigned int idx;
>> + int ret;
>> +
>> + if (!region)
>> + return -EINVAL;
>> +
>> + mutex_lock(&minidump_lock);
>> + if (!__md) {
>> + ret = -EPROBE_DEFER;
>> + goto unlock;
>> + }
>> +
>> + idx = get_apss_minidump_region_index(region);
>> + if (idx < 0) {
>> + dev_err(__md->dev, "%s region is not present\n", region->name);
>> + ret = idx;
>> + goto unlock;
>> + }
>> +
>> + mdr = &__md->md_regions[0];
>> + num_region = le32_to_cpu(__md->md_apss_toc->region_count);
>> + /*
>> + * Left shift all the regions exist after this removed region
>> + * index by 1 to fill the gap and zero out the last region
>> + * present at the end.
>> + */
>> + memmove(&mdr[idx], &mdr[idx + 1],
>> + (num_region - idx - 1) * sizeof(struct minidump_region));
>> + memset(&mdr[num_region - 1], 0, sizeof(struct minidump_region));
>> + ret = qcom_apss_minidump_clear_header(region);
>> + if (ret) {
>> + dev_err(__md->dev, "Failed to remove region: %s\n", region->name);
>> + goto unlock;
>> + }
>> +
>> + num_region--;
>> + __md->md_apss_toc->region_count = cpu_to_le32(num_region);
>> +unlock:
>> + mutex_unlock(&minidump_lock);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_apss_minidump_region_unregister);
>> +
>> +static int qcom_minidump_init_apss_subsystem(struct minidump *md)
>> +{
>> + struct minidump_subsystem *apsstoc;
>> +
>> + apsstoc = &md->md_gbl_toc->subsystems[MINIDUMP_APSS_DESC];
>> + md->md_regions = devm_kcalloc(md->dev, MAX_NUM_ENTRIES,
>> + sizeof(struct minidump_region), GFP_KERNEL);
>> + if (!md->md_regions)
>> + return -ENOMEM;
>> +
>> + md->md_apss_toc = apsstoc;
>> + apsstoc->regions_baseptr = cpu_to_le64(virt_to_phys(md->md_regions));
>> + apsstoc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
>> + apsstoc->status = cpu_to_le32(1);
>> + apsstoc->region_count = cpu_to_le32(0);
>> +
>> + /* Tell bootloader not to encrypt the regions of this subsystem */
>> + apsstoc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
>> + apsstoc->encryption_required = cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_minidump_probe(struct platform_device *pdev)
>> +{
>> + struct minidump_global_toc *mdgtoc;
>> + struct minidump *md;
>> + size_t size;
>> + int ret;
>> +
>> + md = devm_kzalloc(&pdev->dev, sizeof(*md), GFP_KERNEL);
>> + if (!md)
>> + return -ENOMEM;
>> +
>> + mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size);
>> + if (IS_ERR(mdgtoc)) {
>> + ret = PTR_ERR(mdgtoc);
>> + dev_err(&pdev->dev, "Couldn't find minidump smem item: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
>> + ret = -EINVAL;
>> + dev_err(&pdev->dev, "minidump table is not initialized: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + mutex_lock(&minidump_lock);
>> + md->dev = &pdev->dev;
>> + md->md_gbl_toc = mdgtoc;
>
> What are you protecting here? It's not possible to have concurrent
> access to md, is it?
>
>> + ret = qcom_minidump_init_apss_subsystem(md);
>> + if (ret) {
>> + dev_err(&pdev->dev, "apss minidump initialization failed: %d\n", ret);
>> + goto unlock;
>> + }
>> +
>> + __md = md;
>
> No. This is a platform device, so it can have multiple instances.
>
>> + /* First entry would be ELF header */
>> + ret = qcom_apss_minidump_add_elf_header();
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to add elf header: %d\n", ret);
>> + memset(md->md_apss_toc, 0, sizeof(struct minidump_subsystem));
>> + __md = NULL;
>> + }
>> +
>> +unlock:
>> + mutex_unlock(&minidump_lock);
>> + return ret;
>> +}
>> +
>> +static int qcom_minidump_remove(struct platform_device *pdev)
>> +{
>> + memset(__md->md_apss_toc, 0, sizeof(struct minidump_subsystem));
>> + __md = NULL;
>
> Don't use __ in variable names. Drop it everywhere.
>
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver qcom_minidump_driver = {
>> + .probe = qcom_minidump_probe,
>> + .remove = qcom_minidump_remove,
>> + .driver = {
>> + .name = "qcom-minidump",
>> + },
>> +};
>> +
>> +module_platform_driver(qcom_minidump_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm APSS minidump driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:qcom-minidump");
>> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
>> index 6be7ea9..d459656 100644
>> --- a/drivers/soc/qcom/smem.c
>> +++ b/drivers/soc/qcom/smem.c
>> @@ -279,6 +279,7 @@ struct qcom_smem {
>>
>> u32 item_count;
>> struct platform_device *socinfo;
>> + struct platform_device *minidump;
>> struct smem_ptable *ptable;
>> struct smem_partition global_partition;
>> struct smem_partition partitions[SMEM_HOST_COUNT];
>> @@ -1151,12 +1152,19 @@ static int qcom_smem_probe(struct platform_device *pdev)
>> if (IS_ERR(smem->socinfo))
>> dev_dbg(&pdev->dev, "failed to register socinfo device\n");
>>
>> + smem->minidump = platform_device_register_data(&pdev->dev, "qcom-minidump",
>> + PLATFORM_DEVID_NONE, NULL,
>> + 0);
>> + if (IS_ERR(smem->minidump))
>> + dev_dbg(&pdev->dev, "failed to register minidump device\n");
>> +
>> return 0;
>> }
>>
>> static int qcom_smem_remove(struct platform_device *pdev)
>> {
>> platform_device_unregister(__smem->socinfo);
>> + platform_device_unregister(__smem->minidump);
>
> Wrong order. You registered first socinfo, right?
>
>>
>> hwspin_lock_free(__smem->hwlock);
>> __smem = NULL;
>> diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
>> index 84c8605..1872668 100644
>> --- a/include/soc/qcom/qcom_minidump.h
>> +++ b/include/soc/qcom/qcom_minidump.h
>> @@ -1,6 +1,7 @@
>> /* SPDX-License-Identifier: GPL-2.0-only */
>> /*
>> - * Qualcomm minidump shared data structures and macros
>> + * This file contain Qualcomm minidump data structures and macros shared with
>> + * boot firmware and also apss minidump client's data structure
>> *
>> * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>> */
>> @@ -9,12 +10,27 @@
>> #define _QCOM_MINIDUMP_H_
>>
>> #define MAX_NUM_OF_SS 10
>> +#define MAX_NAME_LENGTH 12
>> #define MAX_REGION_NAME_LENGTH 16
>> +
>> +#define MINIDUMP_REVISION 1
>> #define SBL_MINIDUMP_SMEM_ID 602
>> +
>> +/* Application processor minidump descriptor */
>> +#define MINIDUMP_APSS_DESC 0
>> +#define SMEM_ENTRY_SIZE 40
>> +
>> #define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
>> +#define MINIDUMP_REGION_INVALID ('I' << 24 | 'N' << 16 | 'V' << 8 | 'A' << 0)
>> +#define MINIDUMP_REGION_INIT ('I' << 24 | 'N' << 16 | 'I' << 8 | 'T' << 0)
>> +#define MINIDUMP_REGION_NOINIT 0
>> +
>> +#define MINIDUMP_SS_ENCR_REQ (0 << 24 | 'Y' << 16 | 'E' << 8 | 'S' << 0)
>> +#define MINIDUMP_SS_ENCR_NOTREQ (0 << 24 | 0 << 16 | 'N' << 8 | 'R' << 0)
>> +#define MINIDUMP_SS_ENCR_NONE ('N' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
>> #define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
>> +#define MINIDUMP_SS_ENCR_START ('S' << 24 | 'T' << 16 | 'R' << 8 | 'T' << 0)
>> #define MINIDUMP_SS_ENABLED ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
>> -
>
> Why removing this?
>
>> /**
>> * struct minidump_region - Minidump region
>> * @name : Name of the region to be dumped
>> @@ -63,4 +79,45 @@ struct minidump_global_toc {
>> struct minidump_subsystem subsystems[MAX_NUM_OF_SS];
>> };
>>
>> +/**
>> + * struct qcom_apss_minidump_region - APSS Minidump region information
>> + *
>> + * @name: Entry name, Minidump will dump binary with this name.
>> + * @virt_addr: Virtual address of the entry.
>> + * @phys_addr: Physical address of the entry to dump.
>> + * @size: Number of byte to dump from @address location,
>> + * and it should be 4 byte aligned.
>> + */
>> +struct qcom_apss_minidump_region {
>> + char name[MAX_NAME_LENGTH];
>> + void *virt_addr;
>> + phys_addr_t phys_addr;
>> + size_t size;
>> +};
>
> You expose way too much internals in global header.
>
>> +
>> +#if IS_ENABLED(CONFIG_QCOM_MINIDUMP)
>> +extern struct minidump_subsystem *
>
> No externs.
>
> The header is unreadable.
>
>> +qcom_minidump_subsystem_desc(unsigned int minidump_index);
>> +extern int
>> +qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region);
>> +extern int
>> +qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region);
>
> Blank line
>
>> +#else
>
> Blank line
>
>> +static inline
>> +struct minidump_subsystem *qcom_minidump_subsystem_desc(unsigned int minidump_index)
>> +{
>> + return NULL;
>> +}
>
> Blank line
>
>> +static inline int
>> +qcom_apss_minidump_region_register(const struct qcom_apss_minidump_region *region)
>> +{
>> + /* Return quietly, if minidump is not enabled */
>> + return 0;
>> +}
>
>
>> +static inline int
>> +qcom_apss_minidump_region_unregister(const struct qcom_apss_minidump_region *region)
>> +{
>> + return 0;
>> +}
>

Will apply the suggestion.

-- Mukesh

>> +#endif
>
> /* CONFIG_QCOM_MINIDUMP */
>
>> #endif /* _QCOM_MINIDUMP_H_ */
>
> Best regards,
> Krzysztof
>

2023-07-15 22:12:48

by Bjorn Andersson

[permalink] [raw]

Subject: Re: (subset) [PATCH v3 00/18] Add basic Minidump kernel driver support

On Wed, 03 May 2023 22:32:14 +0530, Mukesh Ojha wrote:
> Minidump is a best effort mechanism to collect useful and predefined data
> for first level of debugging on end user devices running on Qualcomm SoCs.
> It is built on the premise that System on Chip (SoC) or subsystem part of
> SoC crashes, due to a range of hardware and software bugs. Hence, the
> ability to collect accurate data is only a best-effort. The data collected
> could be invalid or corrupted, data collection itself could fail, and so on.
>
> [...]

Applied, thanks!

[01/18] remoteproc: qcom: Expand MD_* as MINIDUMP_*
commit: 318da1371246fdc1806011a27138175cfb078687

Best regards,
--
Bjorn Andersson <[email protected]>

2023-07-17 01:23:06

by Mathieu Poirier

[permalink] [raw]

Subject: Re: (subset) [PATCH v3 00/18] Add basic Minidump kernel driver support

On Sat, Jul 15, 2023 at 03:13:34PM -0700, Bjorn Andersson wrote:
>
> On Wed, 03 May 2023 22:32:14 +0530, Mukesh Ojha wrote:
> > Minidump is a best effort mechanism to collect useful and predefined data
> > for first level of debugging on end user devices running on Qualcomm SoCs.
> > It is built on the premise that System on Chip (SoC) or subsystem part of
> > SoC crashes, due to a range of hardware and software bugs. Hence, the
> > ability to collect accurate data is only a best-effort. The data collected
> > could be invalid or corrupted, data collection itself could fail, and so on.
> >
> > [...]
>
> Applied, thanks!
>
> [01/18] remoteproc: qcom: Expand MD_* as MINIDUMP_*
> commit: 318da1371246fdc1806011a27138175cfb078687
>

Krzysztof asked for modifications on this patch.

> Best regards,
> --
> Bjorn Andersson <[email protected]>

2023-07-17 08:40:29

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: (subset) [PATCH v3 00/18] Add basic Minidump kernel driver support

On 17/07/2023 03:15, Mathieu Poirier wrote:
> On Sat, Jul 15, 2023 at 03:13:34PM -0700, Bjorn Andersson wrote:
>>
>> On Wed, 03 May 2023 22:32:14 +0530, Mukesh Ojha wrote:
>>> Minidump is a best effort mechanism to collect useful and predefined data
>>> for first level of debugging on end user devices running on Qualcomm SoCs.
>>> It is built on the premise that System on Chip (SoC) or subsystem part of
>>> SoC crashes, due to a range of hardware and software bugs. Hence, the
>>> ability to collect accurate data is only a best-effort. The data collected
>>> could be invalid or corrupted, data collection itself could fail, and so on.
>>>
>>> [...]
>>
>> Applied, thanks!
>>
>> [01/18] remoteproc: qcom: Expand MD_* as MINIDUMP_*
>> commit: 318da1371246fdc1806011a27138175cfb078687
>>
>
> Krzysztof asked for modifications on this patch.

I guess it is fine, no big issue.

Best regards,
Krzysztof

2023-07-17 16:33:54

by Bjorn Andersson

[permalink] [raw]

Subject: Re: (subset) [PATCH v3 00/18] Add basic Minidump kernel driver support

On Sun, Jul 16, 2023 at 07:15:57PM -0600, Mathieu Poirier wrote:
> On Sat, Jul 15, 2023 at 03:13:34PM -0700, Bjorn Andersson wrote:
> >
> > On Wed, 03 May 2023 22:32:14 +0530, Mukesh Ojha wrote:
> > > Minidump is a best effort mechanism to collect useful and predefined data
> > > for first level of debugging on end user devices running on Qualcomm SoCs.
> > > It is built on the premise that System on Chip (SoC) or subsystem part of
> > > SoC crashes, due to a range of hardware and software bugs. Hence, the
> > > ability to collect accurate data is only a best-effort. The data collected
> > > could be invalid or corrupted, data collection itself could fail, and so on.
> > >
> > > [...]
> >
> > Applied, thanks!
> >
> > [01/18] remoteproc: qcom: Expand MD_* as MINIDUMP_*
> > commit: 318da1371246fdc1806011a27138175cfb078687
> >
>
> Krzysztof asked for modifications on this patch.
>

Krzysztof pointed out that there was no reason to trivially modify the
defines and then immediately start moving things around.

I agree with this, but as the consensus was that the rest of the series
needs more work, I find this to be a sensible cleanup, getting rid of
the cryptic "MD_" abbreviation.

Regards,
Bjorn