diff mbox series

[v5,11/17] qcom_minidump: Register ramoops region with minidump

Message ID 1694290578-17733-12-git-send-email-quic_mojha@quicinc.com
State Superseded
Headers show
Series Add Qualcomm Minidump kernel driver related support | expand

Commit Message

Mukesh Ojha Sept. 9, 2023, 8:16 p.m. UTC 
Register all the pstore frontend with minidump, so that they can
be dumped as default Linux minidump region to be collected on
SoC where minidump is enabled.

Helper functions is written in separate file and built along with
the minidump driver, since it is client of minidump and also call
it at appropriate place from minidump probe so that they always
get registered.

While at it also rename the out minidump module object name during
build as qcom_apss_minidump which basically depicts as Qualcomm
Application processor subsystem minidump.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/soc/qcom/Kconfig                 |  1 +
 drivers/soc/qcom/Makefile                |  3 +-
 drivers/soc/qcom/qcom_minidump.c         |  4 ++
 drivers/soc/qcom/qcom_ramoops_minidump.c | 88 ++++++++++++++++++++++++++++++++
 drivers/soc/qcom/qcom_ramoops_minidump.h | 10 ++++
 5 files changed, 105 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.c
 create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.h

Comments

Pavan Kondeti Sept. 11, 2023, 6:01 a.m. UTC | #1 
On Sun, Sep 10, 2023 at 01:46:12AM +0530, Mukesh Ojha wrote:
> Register all the pstore frontend with minidump, so that they can
> be dumped as default Linux minidump region to be collected on
> SoC where minidump is enabled.
> 
> Helper functions is written in separate file and built along with
> the minidump driver, since it is client of minidump and also call
> it at appropriate place from minidump probe so that they always
> get registered.
> 
> While at it also rename the out minidump module object name during
> build as qcom_apss_minidump which basically depicts as Qualcomm
> Application processor subsystem minidump.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  drivers/soc/qcom/Kconfig                 |  1 +
>  drivers/soc/qcom/Makefile                |  3 +-
>  drivers/soc/qcom/qcom_minidump.c         |  4 ++
>  drivers/soc/qcom/qcom_ramoops_minidump.c | 88 ++++++++++++++++++++++++++++++++
>  drivers/soc/qcom/qcom_ramoops_minidump.h | 10 ++++
>  5 files changed, 105 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.c
>  create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.h
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 4b36d46807bc..b3977f1687d8 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -305,6 +305,7 @@ config QCOM_MINIDUMP
>  	tristate "QCOM APSS Minidump driver"
>  	depends on ARCH_QCOM || COMPILE_TEST
>  	depends on QCOM_SMEM
> +	depends on PSTORE

Can't we make QC minidump available without PSTORE? PSTORE is another
cllient for minidump, so other clients can still use minidump right?
Where is this hard dependency coming from?

Thanks,
Pavan
Mukesh Ojha Sept. 11, 2023, 7:58 a.m. UTC | #2 
On 9/11/2023 11:31 AM, Pavan Kondeti wrote:
> On Sun, Sep 10, 2023 at 01:46:12AM +0530, Mukesh Ojha wrote:
>> Register all the pstore frontend with minidump, so that they can
>> be dumped as default Linux minidump region to be collected on
>> SoC where minidump is enabled.
>>
>> Helper functions is written in separate file and built along with
>> the minidump driver, since it is client of minidump and also call
>> it at appropriate place from minidump probe so that they always
>> get registered.
>>
>> While at it also rename the out minidump module object name during
>> build as qcom_apss_minidump which basically depicts as Qualcomm
>> Application processor subsystem minidump.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   drivers/soc/qcom/Kconfig                 |  1 +
>>   drivers/soc/qcom/Makefile                |  3 +-
>>   drivers/soc/qcom/qcom_minidump.c         |  4 ++
>>   drivers/soc/qcom/qcom_ramoops_minidump.c | 88 ++++++++++++++++++++++++++++++++
>>   drivers/soc/qcom/qcom_ramoops_minidump.h | 10 ++++
>>   5 files changed, 105 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.c
>>   create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.h
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 4b36d46807bc..b3977f1687d8 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -305,6 +305,7 @@ config QCOM_MINIDUMP
>>   	tristate "QCOM APSS Minidump driver"
>>   	depends on ARCH_QCOM || COMPILE_TEST
>>   	depends on QCOM_SMEM
>> +	depends on PSTORE
> 
> Can't we make QC minidump available without PSTORE? PSTORE is another
> cllient for minidump, so other clients can still use minidump right?
> Where is this hard dependency coming from?

Thanks for asking this question, this was intentionally put here to
continue the discussion forward about minidump existence in kernel tree
and how community want to see it in kernel tree.

Actually, there is no hard dependency of minidump on pstore and  as i 
have said in cover-letter about minidump is going to collect what 
ramoops is offering now or going to offer in future as a default
regions.

So, main minidump driver does not depends on pstore since, i have 
integrated even the minidump client with this
qcom_ramoops_minidump_{register|unregister}()

I may guard this entire qcom_ramoops_minidump_{register|unregister}()
function under CONFIG_PSTORE_RAM, if everyone agree ?

-Mukesh
> 
> Thanks,
> Pavan
Krzysztof Kozlowski Sept. 11, 2023, 11:09 a.m. UTC | #3 
On 09/09/2023 22:16, Mukesh Ojha wrote:
> Register all the pstore frontend with minidump, so that they can
> be dumped as default Linux minidump region to be collected on
> SoC where minidump is enabled.
> 

...

> +
> +	record.type = type;
> +	record.id = 0;
> +	max_dump_cnt = 0;
> +	name = pstore_type_to_name(record.type);
> +	do {
> +		ret = pstore_region_defined(&record, &virt, &phys, &size, &max_dump_cnt);
> +		if (ret < 0)
> +			break;
> +
> +		mdr_list = devm_kzalloc(dev, sizeof(struct md_region_list), GFP_KERNEL);

sizeof(*)

Please fix it everywhere in your code.

> +		if (!mdr_list)
> +			return -ENOMEM;
> +
> +		md_region = &mdr_list->md_region;
> +		scnprintf(md_region->name, sizeof(md_region->name) - 1, "K%s%llu", name, record.id);
> +		md_region->virt_addr = virt;
> +		md_region->phys_addr = phys;
> +		md_region->size = size;
> +		ret = qcom_minidump_region_register(md_region);
> +		if (ret) {
> +			pr_err("failed to register minidump region\n");
> +			break;
> +		}
> +
> +		list_add(&mdr_list->list, &ramoops_region_list);
> +	} while (record.id < max_dump_cnt && ++record.id);
> +
> +	return ret;
> +}


Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 4b36d46807bc..b3977f1687d8 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -305,6 +305,7 @@  config QCOM_MINIDUMP
 	tristate "QCOM APSS Minidump driver"
 	depends on ARCH_QCOM || COMPILE_TEST
 	depends on QCOM_SMEM
+	depends on PSTORE
 	help
 	  This config enables linux core infrastructure for Application
 	  processor subsystem (APSS) minidump collection i.e, it enables
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 9c1a409679ec..fca80df8b5b1 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -36,4 +36,5 @@  obj-$(CONFIG_QCOM_ICC_BWMON)	+= icc-bwmon.o
 qcom_ice-objs			+= ice.o
 obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)	+= qcom_ice.o
 obj-$(CONFIG_QCOM_RPROC_MINIDUMP)	+= qcom_rproc_minidump.o
-obj-$(CONFIG_QCOM_MINIDUMP)		+= qcom_minidump.o
+obj-$(CONFIG_QCOM_MINIDUMP)		+= qcom_apss_minidump.o
+qcom_apss_minidump-objs			+= qcom_minidump.o qcom_ramoops_minidump.o
diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
index 4ce36f154e89..7930a80b9100 100644
--- a/drivers/soc/qcom/qcom_minidump.c
+++ b/drivers/soc/qcom/qcom_minidump.c
@@ -23,6 +23,7 @@ 
 #include <soc/qcom/qcom_minidump.h>
 
 #include "qcom_minidump_internal.h"
+#include "qcom_ramoops_minidump.h"
 
 /**
  * struct minidump_ss_data - Minidump subsystem private data
@@ -688,6 +689,8 @@  static int qcom_apss_minidump_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	qcom_ramoops_minidump_register(md->dev);
+
 	mutex_lock(&md_plist.plock);
 	platform_set_drvdata(pdev, md);
 	qcom_apss_register_pending_regions(md);
@@ -701,6 +704,7 @@  static int qcom_apss_minidump_remove(struct platform_device *pdev)
 	struct minidump *md = platform_get_drvdata(pdev);
 	struct minidump_ss_data *mdss_data;
 
+	qcom_ramoops_minidump_unregister();
 	mdss_data = md->apss_data;
 	memset(mdss_data->md_ss_toc, cpu_to_le32(0), sizeof(struct minidump_subsystem));
 	md = NULL;
diff --git a/drivers/soc/qcom/qcom_ramoops_minidump.c b/drivers/soc/qcom/qcom_ramoops_minidump.c
new file mode 100644
index 000000000000..eb97310e3858
--- /dev/null
+++ b/drivers/soc/qcom/qcom_ramoops_minidump.c
@@ -0,0 +1,88 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pstore.h>
+#include <linux/slab.h>
+#include <soc/qcom/qcom_minidump.h>
+
+#include "qcom_ramoops_minidump.h"
+
+static LIST_HEAD(ramoops_region_list);
+
+struct md_region_list {
+	struct qcom_minidump_region md_region;
+	struct list_head list;
+};
+
+static int qcom_ramoops_region_register(struct device *dev, int type)
+{
+	struct qcom_minidump_region *md_region;
+	struct md_region_list *mdr_list;
+	struct pstore_record record;
+	unsigned int max_dump_cnt;
+	phys_addr_t phys;
+	const char *name;
+	void *virt;
+	size_t size;
+	int ret;
+
+	record.type = type;
+	record.id = 0;
+	max_dump_cnt = 0;
+	name = pstore_type_to_name(record.type);
+	do {
+		ret = pstore_region_defined(&record, &virt, &phys, &size, &max_dump_cnt);
+		if (ret < 0)
+			break;
+
+		mdr_list = devm_kzalloc(dev, sizeof(struct md_region_list), GFP_KERNEL);
+		if (!mdr_list)
+			return -ENOMEM;
+
+		md_region = &mdr_list->md_region;
+		scnprintf(md_region->name, sizeof(md_region->name) - 1, "K%s%llu", name, record.id);
+		md_region->virt_addr = virt;
+		md_region->phys_addr = phys;
+		md_region->size = size;
+		ret = qcom_minidump_region_register(md_region);
+		if (ret) {
+			pr_err("failed to register minidump region\n");
+			break;
+		}
+
+		list_add(&mdr_list->list, &ramoops_region_list);
+	} while (record.id < max_dump_cnt && ++record.id);
+
+	return ret;
+}
+
+void qcom_ramoops_minidump_register(struct device *dev)
+{
+	int type_arr[4] = { PSTORE_TYPE_DMESG, PSTORE_TYPE_CONSOLE,
+			    PSTORE_TYPE_FTRACE, PSTORE_TYPE_PMSG,
+			  };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(type_arr); i++)
+		qcom_ramoops_region_register(dev, type_arr[i]);
+}
+
+void qcom_ramoops_minidump_unregister(void)
+{
+	struct md_region_list *mdr_list, *tmp;
+
+	list_for_each_entry_safe(mdr_list, tmp, &ramoops_region_list, list) {
+		struct qcom_minidump_region *region;
+
+		region = &mdr_list->md_region;
+		qcom_minidump_region_unregister(region);
+		list_del(&mdr_list->list);
+	}
+}
diff --git a/drivers/soc/qcom/qcom_ramoops_minidump.h b/drivers/soc/qcom/qcom_ramoops_minidump.h
new file mode 100644
index 000000000000..6086342cd398
--- /dev/null
+++ b/drivers/soc/qcom/qcom_ramoops_minidump.h
@@ -0,0 +1,10 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __QCOM_RAMOOPS_MINIDUMP_H__
+#define __QCOM_RAMOOPS_MINIDUMP_H__
+
+#include <linux/types.h>
+
+void qcom_ramoops_minidump_register(struct device *dev);
+void qcom_ramoops_minidump_unregister(void);
+
+#endif