* [PATCH 1/3] remoteproc: Make coredump functionality configurable
2020-04-16 18:38 [PATCH 1/3] remoteproc: Make coredump functionality configurable Rishabh Bhatnagar
@ 2020-04-16 18:38 ` Rishabh Bhatnagar
2020-04-16 18:38 ` [PATCH 2/3] remoteproc: Add inline coredump functionality Rishabh Bhatnagar
` (3 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-16 18:38 UTC (permalink / raw)
To: linux-remoteproc, linux-kernel
Cc: bjorn.andersson, ohad, mathieu.poirier, tsoni, psodagud, sidgup,
Rishabh Bhatnagar
Add a new file implementing coredump specific functionality.
This would enable clients to have the option to implement custom
dump functionality. The default coredump functionality remains same
as rproc_coredump function.
Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/remoteproc_core.c | 83 -----------------------
drivers/remoteproc/remoteproc_coredump.c | 111 +++++++++++++++++++++++++++++++
drivers/remoteproc/remoteproc_internal.h | 10 +++
4 files changed, 122 insertions(+), 83 deletions(-)
create mode 100644 drivers/remoteproc/remoteproc_coredump.c
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index e30a1b1..f1d1264 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -9,6 +9,7 @@ remoteproc-y += remoteproc_debugfs.o
remoteproc-y += remoteproc_sysfs.o
remoteproc-y += remoteproc_virtio.o
remoteproc-y += remoteproc_elf_loader.o
+remoteproc-y += remoteproc_coredump.o
obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
obj-$(CONFIG_MTK_SCP) += mtk_scp.o mtk_scp_ipi.o
obj-$(CONFIG_OMAP_REMOTEPROC) += omap_remoteproc.o
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 097f33e..c0e9e5d 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1555,89 +1555,6 @@ int rproc_coredump_add_custom_segment(struct rproc *rproc,
EXPORT_SYMBOL(rproc_coredump_add_custom_segment);
/**
- * rproc_coredump() - perform coredump
- * @rproc: rproc handle
- *
- * This function will generate an ELF header for the registered segments
- * and create a devcoredump device associated with rproc.
- */
-static void rproc_coredump(struct rproc *rproc)
-{
- struct rproc_dump_segment *segment;
- struct elf32_phdr *phdr;
- struct elf32_hdr *ehdr;
- size_t data_size;
- size_t offset;
- void *data;
- void *ptr;
- int phnum = 0;
-
- if (list_empty(&rproc->dump_segments))
- return;
-
- data_size = sizeof(*ehdr);
- list_for_each_entry(segment, &rproc->dump_segments, node) {
- data_size += sizeof(*phdr) + segment->size;
-
- phnum++;
- }
-
- data = vmalloc(data_size);
- if (!data)
- return;
-
- ehdr = data;
-
- memset(ehdr, 0, sizeof(*ehdr));
- memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
- ehdr->e_ident[EI_CLASS] = ELFCLASS32;
- ehdr->e_ident[EI_DATA] = ELFDATA2LSB;
- ehdr->e_ident[EI_VERSION] = EV_CURRENT;
- ehdr->e_ident[EI_OSABI] = ELFOSABI_NONE;
- ehdr->e_type = ET_CORE;
- ehdr->e_machine = EM_NONE;
- ehdr->e_version = EV_CURRENT;
- ehdr->e_entry = rproc->bootaddr;
- ehdr->e_phoff = sizeof(*ehdr);
- ehdr->e_ehsize = sizeof(*ehdr);
- ehdr->e_phentsize = sizeof(*phdr);
- ehdr->e_phnum = phnum;
-
- phdr = data + ehdr->e_phoff;
- offset = ehdr->e_phoff + sizeof(*phdr) * ehdr->e_phnum;
- list_for_each_entry(segment, &rproc->dump_segments, node) {
- memset(phdr, 0, sizeof(*phdr));
- phdr->p_type = PT_LOAD;
- phdr->p_offset = offset;
- phdr->p_vaddr = segment->da;
- phdr->p_paddr = segment->da;
- phdr->p_filesz = segment->size;
- phdr->p_memsz = segment->size;
- phdr->p_flags = PF_R | PF_W | PF_X;
- phdr->p_align = 0;
-
- if (segment->dump) {
- segment->dump(rproc, segment, data + offset);
- } else {
- ptr = rproc_da_to_va(rproc, segment->da, segment->size);
- if (!ptr) {
- dev_err(&rproc->dev,
- "invalid coredump segment (%pad, %zu)\n",
- &segment->da, segment->size);
- memset(data + offset, 0xff, segment->size);
- } else {
- memcpy(data + offset, ptr, segment->size);
- }
- }
-
- offset += phdr->p_filesz;
- phdr++;
- }
-
- dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL);
-}
-
-/**
* rproc_trigger_recovery() - recover a remoteproc
* @rproc: the remote processor
*
diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
new file mode 100644
index 0000000..9de0467
--- /dev/null
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Coredump functionality for Remoteproc framework.
+ *
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/devcoredump.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/remoteproc.h>
+#include "remoteproc_internal.h"
+
+static void create_elf_header(void *data, int phnum, struct rproc *rproc)
+{
+ struct elf32_phdr *phdr;
+ struct elf32_hdr *ehdr;
+ struct rproc_dump_segment *segment;
+ int offset;
+
+ ehdr = data;
+
+ memset(ehdr, 0, sizeof(*ehdr));
+ memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
+ ehdr->e_ident[EI_CLASS] = ELFCLASS32;
+ ehdr->e_ident[EI_DATA] = ELFDATA2LSB;
+ ehdr->e_ident[EI_VERSION] = EV_CURRENT;
+ ehdr->e_ident[EI_OSABI] = ELFOSABI_NONE;
+ ehdr->e_type = ET_CORE;
+ ehdr->e_machine = EM_NONE;
+ ehdr->e_version = EV_CURRENT;
+ ehdr->e_entry = rproc->bootaddr;
+ ehdr->e_phoff = sizeof(*ehdr);
+ ehdr->e_ehsize = sizeof(*ehdr);
+ ehdr->e_phentsize = sizeof(*phdr);
+ ehdr->e_phnum = phnum;
+
+ phdr = data + ehdr->e_phoff;
+ offset = ehdr->e_phoff + sizeof(*phdr) * ehdr->e_phnum;
+ list_for_each_entry(segment, &rproc->dump_segments, node) {
+ memset(phdr, 0, sizeof(*phdr));
+ phdr->p_type = PT_LOAD;
+ phdr->p_offset = offset;
+ phdr->p_vaddr = segment->da;
+ phdr->p_paddr = segment->da;
+ phdr->p_filesz = segment->size;
+ phdr->p_memsz = segment->size;
+ phdr->p_flags = PF_R | PF_W | PF_X;
+ phdr->p_align = 0;
+
+ offset += phdr->p_filesz;
+ phdr++;
+ }
+}
+
+/**
+ * rproc_default_coredump() - perform coredump
+ * @rproc: rproc handle
+ *
+ * This function will generate an ELF header for the registered segments
+ * and create a devcoredump device associated with rproc.
+ */
+void rproc_default_coredump(struct rproc *rproc)
+{
+ struct rproc_dump_segment *segment;
+ struct elf32_phdr *phdr;
+ struct elf32_hdr *ehdr;
+ size_t data_size;
+ void *data, *ptr;
+ int offset, phnum = 0;
+
+ if (list_empty(&rproc->dump_segments))
+ return;
+
+ data_size = sizeof(*ehdr);
+ list_for_each_entry(segment, &rproc->dump_segments, node) {
+ data_size += sizeof(*phdr) + segment->size;
+
+ phnum++;
+ }
+
+ data = vmalloc(data_size);
+ if (!data)
+ return;
+
+ ehdr = data;
+ create_elf_header(data, phnum, rproc);
+
+ phdr = data + ehdr->e_phoff;
+ offset = ehdr->e_phoff + sizeof(*phdr) * ehdr->e_phnum;
+ list_for_each_entry(segment, &rproc->dump_segments, node) {
+ if (segment->dump) {
+ segment->dump(rproc, segment, data + offset);
+ } else {
+ ptr = rproc_da_to_va(rproc, segment->da, segment->size);
+ if (!ptr) {
+ dev_err(&rproc->dev,
+ "invalid coredump segment (%pad, %zu)\n",
+ &segment->da, segment->size);
+ memset(data + offset, 0xff, segment->size);
+ } else {
+ memcpy(data + offset, ptr, segment->size);
+ }
+ }
+ phdr++;
+ }
+
+ dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL);
+}
+EXPORT_SYMBOL(rproc_default_coredump);
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 493ef92..28b6af2 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -47,6 +47,9 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
int rproc_init_sysfs(void);
void rproc_exit_sysfs(void);
+/* from remoteproc_coredump.c */
+void rproc_default_coredump(struct rproc *rproc);
+
void rproc_free_vring(struct rproc_vring *rvring);
int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
@@ -119,4 +122,11 @@ struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
return NULL;
}
+static inline
+void rproc_coredump(struct rproc *rproc)
+{
+ return rproc_default_coredump(rproc);
+
+}
+
#endif /* REMOTEPROC_INTERNAL_H */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-04-16 18:38 [PATCH 1/3] remoteproc: Make coredump functionality configurable Rishabh Bhatnagar
2020-04-16 18:38 ` Rishabh Bhatnagar
@ 2020-04-16 18:38 ` Rishabh Bhatnagar
2020-04-16 18:38 ` Rishabh Bhatnagar
` (4 more replies)
2020-04-16 18:38 ` [PATCH 3/3] remoteproc: Add coredump sysfs attribute Rishabh Bhatnagar
` (2 subsequent siblings)
4 siblings, 5 replies; 27+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-16 18:38 UTC (permalink / raw)
To: linux-remoteproc, linux-kernel
Cc: bjorn.andersson, ohad, mathieu.poirier, tsoni, psodagud, sidgup,
Rishabh Bhatnagar
This patch adds the inline coredump functionality. The current
coredump implementation uses vmalloc area to copy all the segments.
But this might put a lot of strain on low memory targets as the
firmware size sometimes is in ten's of MBs. The situation becomes
worse if there are multiple remote processors undergoing recovery
at the same time. This patch directly copies the device memory to
userspace buffer and avoids extra memory usage. This requires
recovery to be halted until data is read by userspace and free
function is called.
Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
drivers/remoteproc/remoteproc_coredump.c | 130 +++++++++++++++++++++++++++++++
drivers/remoteproc/remoteproc_internal.h | 23 +++++-
include/linux/remoteproc.h | 2 +
3 files changed, 153 insertions(+), 2 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
index 9de0467..888b7dec91 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -12,6 +12,84 @@
#include <linux/remoteproc.h>
#include "remoteproc_internal.h"
+static void rproc_free_dump(void *data)
+{
+ struct rproc_coredump_state *dump_state = data;
+
+ complete(&dump_state->dump_done);
+}
+
+static unsigned long resolve_addr(loff_t user_offset,
+ struct list_head *segments,
+ unsigned long *data_left)
+{
+ struct rproc_dump_segment *segment;
+
+ list_for_each_entry(segment, segments, node) {
+ if (user_offset >= segment->size)
+ user_offset -= segment->size;
+ else
+ break;
+ }
+
+ if (&segment->node == segments) {
+ *data_left = 0;
+ return 0;
+ }
+
+ *data_left = segment->size - user_offset;
+
+ return segment->da + user_offset;
+}
+
+static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t count,
+ void *data, size_t header_size)
+{
+ void *device_mem;
+ size_t data_left, copy_size, bytes_left = count;
+ unsigned long addr;
+ struct rproc_coredump_state *dump_state = data;
+ struct rproc *rproc = dump_state->rproc;
+ void *elfcore = dump_state->header;
+
+ /* Copy the header first */
+ if (offset < header_size) {
+ copy_size = header_size - offset;
+ copy_size = min(copy_size, bytes_left);
+
+ memcpy(buffer, elfcore + offset, copy_size);
+ offset += copy_size;
+ bytes_left -= copy_size;
+ buffer += copy_size;
+ }
+
+ while (bytes_left) {
+ addr = resolve_addr(offset - header_size,
+ &rproc->dump_segments, &data_left);
+ /* EOF check */
+ if (data_left == 0) {
+ pr_info("Ramdump complete %lld bytes read", offset);
+ break;
+ }
+
+ copy_size = min_t(size_t, bytes_left, data_left);
+
+ device_mem = rproc->ops->da_to_va(rproc, addr, copy_size);
+ if (!device_mem) {
+ pr_err("Address:%lx with size %zd out of remoteproc carveout\n",
+ addr, copy_size);
+ return -ENOMEM;
+ }
+ memcpy(buffer, device_mem, copy_size);
+
+ offset += copy_size;
+ buffer += copy_size;
+ bytes_left -= copy_size;
+ }
+
+ return count - bytes_left;
+}
+
static void create_elf_header(void *data, int phnum, struct rproc *rproc)
{
struct elf32_phdr *phdr;
@@ -55,6 +133,58 @@ static void create_elf_header(void *data, int phnum, struct rproc *rproc)
}
/**
+ * rproc_inline_coredump() - perform synchronized coredump
+ * @rproc: rproc handle
+ *
+ * This function will generate an ELF header for the registered segments
+ * and create a devcoredump device associated with rproc. This function
+ * directly copies the segments from device memory to userspace. The
+ * recovery is stalled until the enitire coredump is read. This approach
+ * avoids using extra vmalloc memory(which can be really large).
+ */
+void rproc_inline_coredump(struct rproc *rproc)
+{
+ struct rproc_dump_segment *segment;
+ struct elf32_phdr *phdr;
+ struct elf32_hdr *ehdr;
+ struct rproc_coredump_state *dump_state;
+ size_t header_size;
+ void *data;
+ int phnum = 0;
+
+ if (list_empty(&rproc->dump_segments))
+ return;
+
+ header_size = sizeof(*ehdr);
+ list_for_each_entry(segment, &rproc->dump_segments, node) {
+ header_size += sizeof(*phdr);
+
+ phnum++;
+ }
+
+ data = vmalloc(header_size);
+ if (!data)
+ return;
+
+ ehdr = data;
+ create_elf_header(data, phnum, rproc);
+
+ dump_state = kzalloc(sizeof(*dump_state), GFP_KERNEL);
+ dump_state->rproc = rproc;
+ dump_state->header = data;
+ init_completion(&dump_state->dump_done);
+
+ dev_coredumpm(&rproc->dev, NULL, dump_state, header_size, GFP_KERNEL,
+ rproc_read_dump, rproc_free_dump);
+
+ /* Wait until the dump is read and free is called */
+ wait_for_completion(&dump_state->dump_done);
+
+ kfree(dump_state);
+}
+EXPORT_SYMBOL(rproc_inline_coredump);
+
+/**
* rproc_default_coredump() - perform coredump
* @rproc: rproc handle
*
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 28b6af2..ea6146e 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -24,6 +24,18 @@ struct rproc_debug_trace {
struct rproc_mem_entry trace_mem;
};
+struct rproc_coredump_state {
+ struct rproc *rproc;
+ void *header;
+ struct completion dump_done;
+};
+
+enum rproc_coredump_conf {
+ COREDUMP_DEFAULT,
+ COREDUMP_INLINE,
+ COREDUMP_DISABLED,
+};
+
/* from remoteproc_core.c */
void rproc_release(struct kref *kref);
irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
@@ -49,6 +61,7 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
/* from remoteproc_coredump.c */
void rproc_default_coredump(struct rproc *rproc);
+void rproc_inline_coredump(struct rproc *rproc);
void rproc_free_vring(struct rproc_vring *rvring);
int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
@@ -125,8 +138,14 @@ struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
static inline
void rproc_coredump(struct rproc *rproc)
{
- return rproc_default_coredump(rproc);
-
+ switch (rproc->coredump_conf) {
+ case COREDUMP_DEFAULT:
+ return rproc_default_coredump(rproc);
+ case COREDUMP_INLINE:
+ return rproc_inline_coredump(rproc);
+ default:
+ break;
+ }
}
#endif /* REMOTEPROC_INTERNAL_H */
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 16ad666..23298ce 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -459,6 +459,7 @@ struct rproc_dump_segment {
* @dev: virtual device for refcounting and common remoteproc behavior
* @power: refcount of users who need this rproc powered up
* @state: state of the device
+ * @coredump_conf: Currenlty selected coredump configuration
* @lock: lock which protects concurrent manipulations of the rproc
* @dbg_dir: debugfs directory of this rproc device
* @traces: list of trace buffers
@@ -492,6 +493,7 @@ struct rproc {
struct device dev;
atomic_t power;
unsigned int state;
+ unsigned int coredump_conf;
struct mutex lock;
struct dentry *dbg_dir;
struct list_head traces;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-04-16 18:38 ` [PATCH 2/3] remoteproc: Add inline coredump functionality Rishabh Bhatnagar
@ 2020-04-16 18:38 ` Rishabh Bhatnagar
2020-04-17 7:52 ` Loic PALLARDY
` (3 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-16 18:38 UTC (permalink / raw)
To: linux-remoteproc, linux-kernel
Cc: bjorn.andersson, ohad, mathieu.poirier, tsoni, psodagud, sidgup,
Rishabh Bhatnagar
This patch adds the inline coredump functionality. The current
coredump implementation uses vmalloc area to copy all the segments.
But this might put a lot of strain on low memory targets as the
firmware size sometimes is in ten's of MBs. The situation becomes
worse if there are multiple remote processors undergoing recovery
at the same time. This patch directly copies the device memory to
userspace buffer and avoids extra memory usage. This requires
recovery to be halted until data is read by userspace and free
function is called.
Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
drivers/remoteproc/remoteproc_coredump.c | 130 +++++++++++++++++++++++++++++++
drivers/remoteproc/remoteproc_internal.h | 23 +++++-
include/linux/remoteproc.h | 2 +
3 files changed, 153 insertions(+), 2 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
index 9de0467..888b7dec91 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -12,6 +12,84 @@
#include <linux/remoteproc.h>
#include "remoteproc_internal.h"
+static void rproc_free_dump(void *data)
+{
+ struct rproc_coredump_state *dump_state = data;
+
+ complete(&dump_state->dump_done);
+}
+
+static unsigned long resolve_addr(loff_t user_offset,
+ struct list_head *segments,
+ unsigned long *data_left)
+{
+ struct rproc_dump_segment *segment;
+
+ list_for_each_entry(segment, segments, node) {
+ if (user_offset >= segment->size)
+ user_offset -= segment->size;
+ else
+ break;
+ }
+
+ if (&segment->node == segments) {
+ *data_left = 0;
+ return 0;
+ }
+
+ *data_left = segment->size - user_offset;
+
+ return segment->da + user_offset;
+}
+
+static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t count,
+ void *data, size_t header_size)
+{
+ void *device_mem;
+ size_t data_left, copy_size, bytes_left = count;
+ unsigned long addr;
+ struct rproc_coredump_state *dump_state = data;
+ struct rproc *rproc = dump_state->rproc;
+ void *elfcore = dump_state->header;
+
+ /* Copy the header first */
+ if (offset < header_size) {
+ copy_size = header_size - offset;
+ copy_size = min(copy_size, bytes_left);
+
+ memcpy(buffer, elfcore + offset, copy_size);
+ offset += copy_size;
+ bytes_left -= copy_size;
+ buffer += copy_size;
+ }
+
+ while (bytes_left) {
+ addr = resolve_addr(offset - header_size,
+ &rproc->dump_segments, &data_left);
+ /* EOF check */
+ if (data_left == 0) {
+ pr_info("Ramdump complete %lld bytes read", offset);
+ break;
+ }
+
+ copy_size = min_t(size_t, bytes_left, data_left);
+
+ device_mem = rproc->ops->da_to_va(rproc, addr, copy_size);
+ if (!device_mem) {
+ pr_err("Address:%lx with size %zd out of remoteproc carveout\n",
+ addr, copy_size);
+ return -ENOMEM;
+ }
+ memcpy(buffer, device_mem, copy_size);
+
+ offset += copy_size;
+ buffer += copy_size;
+ bytes_left -= copy_size;
+ }
+
+ return count - bytes_left;
+}
+
static void create_elf_header(void *data, int phnum, struct rproc *rproc)
{
struct elf32_phdr *phdr;
@@ -55,6 +133,58 @@ static void create_elf_header(void *data, int phnum, struct rproc *rproc)
}
/**
+ * rproc_inline_coredump() - perform synchronized coredump
+ * @rproc: rproc handle
+ *
+ * This function will generate an ELF header for the registered segments
+ * and create a devcoredump device associated with rproc. This function
+ * directly copies the segments from device memory to userspace. The
+ * recovery is stalled until the enitire coredump is read. This approach
+ * avoids using extra vmalloc memory(which can be really large).
+ */
+void rproc_inline_coredump(struct rproc *rproc)
+{
+ struct rproc_dump_segment *segment;
+ struct elf32_phdr *phdr;
+ struct elf32_hdr *ehdr;
+ struct rproc_coredump_state *dump_state;
+ size_t header_size;
+ void *data;
+ int phnum = 0;
+
+ if (list_empty(&rproc->dump_segments))
+ return;
+
+ header_size = sizeof(*ehdr);
+ list_for_each_entry(segment, &rproc->dump_segments, node) {
+ header_size += sizeof(*phdr);
+
+ phnum++;
+ }
+
+ data = vmalloc(header_size);
+ if (!data)
+ return;
+
+ ehdr = data;
+ create_elf_header(data, phnum, rproc);
+
+ dump_state = kzalloc(sizeof(*dump_state), GFP_KERNEL);
+ dump_state->rproc = rproc;
+ dump_state->header = data;
+ init_completion(&dump_state->dump_done);
+
+ dev_coredumpm(&rproc->dev, NULL, dump_state, header_size, GFP_KERNEL,
+ rproc_read_dump, rproc_free_dump);
+
+ /* Wait until the dump is read and free is called */
+ wait_for_completion(&dump_state->dump_done);
+
+ kfree(dump_state);
+}
+EXPORT_SYMBOL(rproc_inline_coredump);
+
+/**
* rproc_default_coredump() - perform coredump
* @rproc: rproc handle
*
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 28b6af2..ea6146e 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -24,6 +24,18 @@ struct rproc_debug_trace {
struct rproc_mem_entry trace_mem;
};
+struct rproc_coredump_state {
+ struct rproc *rproc;
+ void *header;
+ struct completion dump_done;
+};
+
+enum rproc_coredump_conf {
+ COREDUMP_DEFAULT,
+ COREDUMP_INLINE,
+ COREDUMP_DISABLED,
+};
+
/* from remoteproc_core.c */
void rproc_release(struct kref *kref);
irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
@@ -49,6 +61,7 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
/* from remoteproc_coredump.c */
void rproc_default_coredump(struct rproc *rproc);
+void rproc_inline_coredump(struct rproc *rproc);
void rproc_free_vring(struct rproc_vring *rvring);
int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
@@ -125,8 +138,14 @@ struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
static inline
void rproc_coredump(struct rproc *rproc)
{
- return rproc_default_coredump(rproc);
-
+ switch (rproc->coredump_conf) {
+ case COREDUMP_DEFAULT:
+ return rproc_default_coredump(rproc);
+ case COREDUMP_INLINE:
+ return rproc_inline_coredump(rproc);
+ default:
+ break;
+ }
}
#endif /* REMOTEPROC_INTERNAL_H */
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 16ad666..23298ce 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -459,6 +459,7 @@ struct rproc_dump_segment {
* @dev: virtual device for refcounting and common remoteproc behavior
* @power: refcount of users who need this rproc powered up
* @state: state of the device
+ * @coredump_conf: Currenlty selected coredump configuration
* @lock: lock which protects concurrent manipulations of the rproc
* @dbg_dir: debugfs directory of this rproc device
* @traces: list of trace buffers
@@ -492,6 +493,7 @@ struct rproc {
struct device dev;
atomic_t power;
unsigned int state;
+ unsigned int coredump_conf;
struct mutex lock;
struct dentry *dbg_dir;
struct list_head traces;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 27+ messages in thread* RE: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-04-16 18:38 ` [PATCH 2/3] remoteproc: Add inline coredump functionality Rishabh Bhatnagar
2020-04-16 18:38 ` Rishabh Bhatnagar
@ 2020-04-17 7:52 ` Loic PALLARDY
2020-04-17 7:52 ` Loic PALLARDY
2020-04-17 17:11 ` Bjorn Andersson
2020-04-20 6:01 ` kbuild test robot
` (2 subsequent siblings)
4 siblings, 2 replies; 27+ messages in thread
From: Loic PALLARDY @ 2020-04-17 7:52 UTC (permalink / raw)
To: Rishabh Bhatnagar, linux-remoteproc, linux-kernel
Cc: bjorn.andersson, ohad, mathieu.poirier, tsoni, psodagud, sidgup
Hi Rishabh,
> -----Original Message-----
> From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
> owner@vger.kernel.org> On Behalf Of Rishabh Bhatnagar
> Sent: jeudi 16 avril 2020 20:39
> To: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: bjorn.andersson@linaro.org; ohad@wizery.com;
> mathieu.poirier@linaro.org; tsoni@codeaurora.org;
> psodagud@codeaurora.org; sidgup@codeaurora.org; Rishabh Bhatnagar
> <rishabhb@codeaurora.org>
> Subject: [PATCH 2/3] remoteproc: Add inline coredump functionality
>
> This patch adds the inline coredump functionality. The current
> coredump implementation uses vmalloc area to copy all the segments.
> But this might put a lot of strain on low memory targets as the
> firmware size sometimes is in ten's of MBs. The situation becomes
> worse if there are multiple remote processors undergoing recovery
> at the same time. This patch directly copies the device memory to
> userspace buffer and avoids extra memory usage. This requires
> recovery to be halted until data is read by userspace and free
> function is called.
>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
> drivers/remoteproc/remoteproc_coredump.c | 130
> +++++++++++++++++++++++++++++++
> drivers/remoteproc/remoteproc_internal.h | 23 +++++-
> include/linux/remoteproc.h | 2 +
> 3 files changed, 153 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_coredump.c
> b/drivers/remoteproc/remoteproc_coredump.c
> index 9de0467..888b7dec91 100644
> --- a/drivers/remoteproc/remoteproc_coredump.c
> +++ b/drivers/remoteproc/remoteproc_coredump.c
> @@ -12,6 +12,84 @@
> #include <linux/remoteproc.h>
> #include "remoteproc_internal.h"
>
> +static void rproc_free_dump(void *data)
> +{
> + struct rproc_coredump_state *dump_state = data;
> +
> + complete(&dump_state->dump_done);
> +}
> +
> +static unsigned long resolve_addr(loff_t user_offset,
> + struct list_head *segments,
> + unsigned long *data_left)
> +{
> + struct rproc_dump_segment *segment;
> +
> + list_for_each_entry(segment, segments, node) {
> + if (user_offset >= segment->size)
> + user_offset -= segment->size;
> + else
> + break;
> + }
> +
> + if (&segment->node == segments) {
> + *data_left = 0;
> + return 0;
> + }
> +
> + *data_left = segment->size - user_offset;
> +
> + return segment->da + user_offset;
> +}
> +
> +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t count,
> + void *data, size_t header_size)
> +{
> + void *device_mem;
> + size_t data_left, copy_size, bytes_left = count;
> + unsigned long addr;
> + struct rproc_coredump_state *dump_state = data;
> + struct rproc *rproc = dump_state->rproc;
> + void *elfcore = dump_state->header;
> +
> + /* Copy the header first */
> + if (offset < header_size) {
> + copy_size = header_size - offset;
> + copy_size = min(copy_size, bytes_left);
> +
> + memcpy(buffer, elfcore + offset, copy_size);
> + offset += copy_size;
> + bytes_left -= copy_size;
> + buffer += copy_size;
> + }
> +
> + while (bytes_left) {
> + addr = resolve_addr(offset - header_size,
> + &rproc->dump_segments, &data_left);
> + /* EOF check */
> + if (data_left == 0) {
> + pr_info("Ramdump complete %lld bytes read",
> offset);
> + break;
> + }
> +
> + copy_size = min_t(size_t, bytes_left, data_left);
> +
> + device_mem = rproc->ops->da_to_va(rproc, addr,
> copy_size);
> + if (!device_mem) {
> + pr_err("Address:%lx with size %zd out of remoteproc
> carveout\n",
> + addr, copy_size);
> + return -ENOMEM;
> + }
> + memcpy(buffer, device_mem, copy_size);
> +
> + offset += copy_size;
> + buffer += copy_size;
> + bytes_left -= copy_size;
> + }
> +
> + return count - bytes_left;
> +}
> +
> static void create_elf_header(void *data, int phnum, struct rproc *rproc)
> {
> struct elf32_phdr *phdr;
> @@ -55,6 +133,58 @@ static void create_elf_header(void *data, int phnum,
> struct rproc *rproc)
> }
>
> /**
> + * rproc_inline_coredump() - perform synchronized coredump
> + * @rproc: rproc handle
> + *
> + * This function will generate an ELF header for the registered segments
> + * and create a devcoredump device associated with rproc. This function
> + * directly copies the segments from device memory to userspace. The
> + * recovery is stalled until the enitire coredump is read. This approach
Typo entire -> entire
> + * avoids using extra vmalloc memory(which can be really large).
> + */
> +void rproc_inline_coredump(struct rproc *rproc)
> +{
> + struct rproc_dump_segment *segment;
> + struct elf32_phdr *phdr;
> + struct elf32_hdr *ehdr;
> + struct rproc_coredump_state *dump_state;
> + size_t header_size;
> + void *data;
> + int phnum = 0;
> +
> + if (list_empty(&rproc->dump_segments))
> + return;
> +
> + header_size = sizeof(*ehdr);
> + list_for_each_entry(segment, &rproc->dump_segments, node) {
> + header_size += sizeof(*phdr);
> +
> + phnum++;
> + }
> +
> + data = vmalloc(header_size);
> + if (!data)
> + return;
> +
> + ehdr = data;
> + create_elf_header(data, phnum, rproc);
> +
> + dump_state = kzalloc(sizeof(*dump_state), GFP_KERNEL);
> + dump_state->rproc = rproc;
> + dump_state->header = data;
> + init_completion(&dump_state->dump_done);
> +
> + dev_coredumpm(&rproc->dev, NULL, dump_state, header_size,
> GFP_KERNEL,
> + rproc_read_dump, rproc_free_dump);
> +
> + /* Wait until the dump is read and free is called */
> + wait_for_completion(&dump_state->dump_done);
Maybe good to add a timeout with value programmable via debugfs?
Regards,
Loic
> +
> + kfree(dump_state);
> +}
> +EXPORT_SYMBOL(rproc_inline_coredump);
> +
> +/**
> * rproc_default_coredump() - perform coredump
> * @rproc: rproc handle
> *
> diff --git a/drivers/remoteproc/remoteproc_internal.h
> b/drivers/remoteproc/remoteproc_internal.h
> index 28b6af2..ea6146e 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -24,6 +24,18 @@ struct rproc_debug_trace {
> struct rproc_mem_entry trace_mem;
> };
>
> +struct rproc_coredump_state {
> + struct rproc *rproc;
> + void *header;
> + struct completion dump_done;
> +};
> +
> +enum rproc_coredump_conf {
> + COREDUMP_DEFAULT,
> + COREDUMP_INLINE,
> + COREDUMP_DISABLED,
> +};
> +
> /* from remoteproc_core.c */
> void rproc_release(struct kref *kref);
> irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> @@ -49,6 +61,7 @@ struct dentry *rproc_create_trace_file(const char
> *name, struct rproc *rproc,
>
> /* from remoteproc_coredump.c */
> void rproc_default_coredump(struct rproc *rproc);
> +void rproc_inline_coredump(struct rproc *rproc);
>
> void rproc_free_vring(struct rproc_vring *rvring);
> int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> @@ -125,8 +138,14 @@ struct resource_table
> *rproc_find_loaded_rsc_table(struct rproc *rproc,
> static inline
> void rproc_coredump(struct rproc *rproc)
> {
> - return rproc_default_coredump(rproc);
> -
> + switch (rproc->coredump_conf) {
> + case COREDUMP_DEFAULT:
> + return rproc_default_coredump(rproc);
> + case COREDUMP_INLINE:
> + return rproc_inline_coredump(rproc);
> + default:
> + break;
> + }
> }
>
> #endif /* REMOTEPROC_INTERNAL_H */
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 16ad666..23298ce 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -459,6 +459,7 @@ struct rproc_dump_segment {
> * @dev: virtual device for refcounting and common remoteproc behavior
> * @power: refcount of users who need this rproc powered up
> * @state: state of the device
> + * @coredump_conf: Currenlty selected coredump configuration
> * @lock: lock which protects concurrent manipulations of the rproc
> * @dbg_dir: debugfs directory of this rproc device
> * @traces: list of trace buffers
> @@ -492,6 +493,7 @@ struct rproc {
> struct device dev;
> atomic_t power;
> unsigned int state;
> + unsigned int coredump_conf;
> struct mutex lock;
> struct dentry *dbg_dir;
> struct list_head traces;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread* RE: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-04-17 7:52 ` Loic PALLARDY
@ 2020-04-17 7:52 ` Loic PALLARDY
2020-04-17 17:11 ` Bjorn Andersson
1 sibling, 0 replies; 27+ messages in thread
From: Loic PALLARDY @ 2020-04-17 7:52 UTC (permalink / raw)
To: Rishabh Bhatnagar, linux-remoteproc, linux-kernel
Cc: bjorn.andersson, ohad, mathieu.poirier, tsoni, psodagud, sidgup
Hi Rishabh,
> -----Original Message-----
> From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
> owner@vger.kernel.org> On Behalf Of Rishabh Bhatnagar
> Sent: jeudi 16 avril 2020 20:39
> To: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: bjorn.andersson@linaro.org; ohad@wizery.com;
> mathieu.poirier@linaro.org; tsoni@codeaurora.org;
> psodagud@codeaurora.org; sidgup@codeaurora.org; Rishabh Bhatnagar
> <rishabhb@codeaurora.org>
> Subject: [PATCH 2/3] remoteproc: Add inline coredump functionality
>
> This patch adds the inline coredump functionality. The current
> coredump implementation uses vmalloc area to copy all the segments.
> But this might put a lot of strain on low memory targets as the
> firmware size sometimes is in ten's of MBs. The situation becomes
> worse if there are multiple remote processors undergoing recovery
> at the same time. This patch directly copies the device memory to
> userspace buffer and avoids extra memory usage. This requires
> recovery to be halted until data is read by userspace and free
> function is called.
>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
> drivers/remoteproc/remoteproc_coredump.c | 130
> +++++++++++++++++++++++++++++++
> drivers/remoteproc/remoteproc_internal.h | 23 +++++-
> include/linux/remoteproc.h | 2 +
> 3 files changed, 153 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_coredump.c
> b/drivers/remoteproc/remoteproc_coredump.c
> index 9de0467..888b7dec91 100644
> --- a/drivers/remoteproc/remoteproc_coredump.c
> +++ b/drivers/remoteproc/remoteproc_coredump.c
> @@ -12,6 +12,84 @@
> #include <linux/remoteproc.h>
> #include "remoteproc_internal.h"
>
> +static void rproc_free_dump(void *data)
> +{
> + struct rproc_coredump_state *dump_state = data;
> +
> + complete(&dump_state->dump_done);
> +}
> +
> +static unsigned long resolve_addr(loff_t user_offset,
> + struct list_head *segments,
> + unsigned long *data_left)
> +{
> + struct rproc_dump_segment *segment;
> +
> + list_for_each_entry(segment, segments, node) {
> + if (user_offset >= segment->size)
> + user_offset -= segment->size;
> + else
> + break;
> + }
> +
> + if (&segment->node == segments) {
> + *data_left = 0;
> + return 0;
> + }
> +
> + *data_left = segment->size - user_offset;
> +
> + return segment->da + user_offset;
> +}
> +
> +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t count,
> + void *data, size_t header_size)
> +{
> + void *device_mem;
> + size_t data_left, copy_size, bytes_left = count;
> + unsigned long addr;
> + struct rproc_coredump_state *dump_state = data;
> + struct rproc *rproc = dump_state->rproc;
> + void *elfcore = dump_state->header;
> +
> + /* Copy the header first */
> + if (offset < header_size) {
> + copy_size = header_size - offset;
> + copy_size = min(copy_size, bytes_left);
> +
> + memcpy(buffer, elfcore + offset, copy_size);
> + offset += copy_size;
> + bytes_left -= copy_size;
> + buffer += copy_size;
> + }
> +
> + while (bytes_left) {
> + addr = resolve_addr(offset - header_size,
> + &rproc->dump_segments, &data_left);
> + /* EOF check */
> + if (data_left == 0) {
> + pr_info("Ramdump complete %lld bytes read",
> offset);
> + break;
> + }
> +
> + copy_size = min_t(size_t, bytes_left, data_left);
> +
> + device_mem = rproc->ops->da_to_va(rproc, addr,
> copy_size);
> + if (!device_mem) {
> + pr_err("Address:%lx with size %zd out of remoteproc
> carveout\n",
> + addr, copy_size);
> + return -ENOMEM;
> + }
> + memcpy(buffer, device_mem, copy_size);
> +
> + offset += copy_size;
> + buffer += copy_size;
> + bytes_left -= copy_size;
> + }
> +
> + return count - bytes_left;
> +}
> +
> static void create_elf_header(void *data, int phnum, struct rproc *rproc)
> {
> struct elf32_phdr *phdr;
> @@ -55,6 +133,58 @@ static void create_elf_header(void *data, int phnum,
> struct rproc *rproc)
> }
>
> /**
> + * rproc_inline_coredump() - perform synchronized coredump
> + * @rproc: rproc handle
> + *
> + * This function will generate an ELF header for the registered segments
> + * and create a devcoredump device associated with rproc. This function
> + * directly copies the segments from device memory to userspace. The
> + * recovery is stalled until the enitire coredump is read. This approach
Typo entire -> entire
> + * avoids using extra vmalloc memory(which can be really large).
> + */
> +void rproc_inline_coredump(struct rproc *rproc)
> +{
> + struct rproc_dump_segment *segment;
> + struct elf32_phdr *phdr;
> + struct elf32_hdr *ehdr;
> + struct rproc_coredump_state *dump_state;
> + size_t header_size;
> + void *data;
> + int phnum = 0;
> +
> + if (list_empty(&rproc->dump_segments))
> + return;
> +
> + header_size = sizeof(*ehdr);
> + list_for_each_entry(segment, &rproc->dump_segments, node) {
> + header_size += sizeof(*phdr);
> +
> + phnum++;
> + }
> +
> + data = vmalloc(header_size);
> + if (!data)
> + return;
> +
> + ehdr = data;
> + create_elf_header(data, phnum, rproc);
> +
> + dump_state = kzalloc(sizeof(*dump_state), GFP_KERNEL);
> + dump_state->rproc = rproc;
> + dump_state->header = data;
> + init_completion(&dump_state->dump_done);
> +
> + dev_coredumpm(&rproc->dev, NULL, dump_state, header_size,
> GFP_KERNEL,
> + rproc_read_dump, rproc_free_dump);
> +
> + /* Wait until the dump is read and free is called */
> + wait_for_completion(&dump_state->dump_done);
Maybe good to add a timeout with value programmable via debugfs?
Regards,
Loic
> +
> + kfree(dump_state);
> +}
> +EXPORT_SYMBOL(rproc_inline_coredump);
> +
> +/**
> * rproc_default_coredump() - perform coredump
> * @rproc: rproc handle
> *
> diff --git a/drivers/remoteproc/remoteproc_internal.h
> b/drivers/remoteproc/remoteproc_internal.h
> index 28b6af2..ea6146e 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -24,6 +24,18 @@ struct rproc_debug_trace {
> struct rproc_mem_entry trace_mem;
> };
>
> +struct rproc_coredump_state {
> + struct rproc *rproc;
> + void *header;
> + struct completion dump_done;
> +};
> +
> +enum rproc_coredump_conf {
> + COREDUMP_DEFAULT,
> + COREDUMP_INLINE,
> + COREDUMP_DISABLED,
> +};
> +
> /* from remoteproc_core.c */
> void rproc_release(struct kref *kref);
> irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> @@ -49,6 +61,7 @@ struct dentry *rproc_create_trace_file(const char
> *name, struct rproc *rproc,
>
> /* from remoteproc_coredump.c */
> void rproc_default_coredump(struct rproc *rproc);
> +void rproc_inline_coredump(struct rproc *rproc);
>
> void rproc_free_vring(struct rproc_vring *rvring);
> int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> @@ -125,8 +138,14 @@ struct resource_table
> *rproc_find_loaded_rsc_table(struct rproc *rproc,
> static inline
> void rproc_coredump(struct rproc *rproc)
> {
> - return rproc_default_coredump(rproc);
> -
> + switch (rproc->coredump_conf) {
> + case COREDUMP_DEFAULT:
> + return rproc_default_coredump(rproc);
> + case COREDUMP_INLINE:
> + return rproc_inline_coredump(rproc);
> + default:
> + break;
> + }
> }
>
> #endif /* REMOTEPROC_INTERNAL_H */
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 16ad666..23298ce 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -459,6 +459,7 @@ struct rproc_dump_segment {
> * @dev: virtual device for refcounting and common remoteproc behavior
> * @power: refcount of users who need this rproc powered up
> * @state: state of the device
> + * @coredump_conf: Currenlty selected coredump configuration
> * @lock: lock which protects concurrent manipulations of the rproc
> * @dbg_dir: debugfs directory of this rproc device
> * @traces: list of trace buffers
> @@ -492,6 +493,7 @@ struct rproc {
> struct device dev;
> atomic_t power;
> unsigned int state;
> + unsigned int coredump_conf;
> struct mutex lock;
> struct dentry *dbg_dir;
> struct list_head traces;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-04-17 7:52 ` Loic PALLARDY
2020-04-17 7:52 ` Loic PALLARDY
@ 2020-04-17 17:11 ` Bjorn Andersson
2020-04-17 17:11 ` Bjorn Andersson
1 sibling, 1 reply; 27+ messages in thread
From: Bjorn Andersson @ 2020-04-17 17:11 UTC (permalink / raw)
To: Loic PALLARDY
Cc: Rishabh Bhatnagar, linux-remoteproc, linux-kernel, ohad,
mathieu.poirier, tsoni, psodagud, sidgup
On Fri 17 Apr 00:52 PDT 2020, Loic PALLARDY wrote:
> Hi Rishabh,
>
> > -----Original Message-----
> > From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
> > owner@vger.kernel.org> On Behalf Of Rishabh Bhatnagar
> > Sent: jeudi 16 avril 2020 20:39
> > To: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> > Cc: bjorn.andersson@linaro.org; ohad@wizery.com;
> > mathieu.poirier@linaro.org; tsoni@codeaurora.org;
> > psodagud@codeaurora.org; sidgup@codeaurora.org; Rishabh Bhatnagar
> > <rishabhb@codeaurora.org>
> > Subject: [PATCH 2/3] remoteproc: Add inline coredump functionality
> >
> > This patch adds the inline coredump functionality. The current
> > coredump implementation uses vmalloc area to copy all the segments.
> > But this might put a lot of strain on low memory targets as the
> > firmware size sometimes is in ten's of MBs. The situation becomes
> > worse if there are multiple remote processors undergoing recovery
> > at the same time. This patch directly copies the device memory to
> > userspace buffer and avoids extra memory usage. This requires
> > recovery to be halted until data is read by userspace and free
> > function is called.
> >
> > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> > ---
> > drivers/remoteproc/remoteproc_coredump.c | 130
> > +++++++++++++++++++++++++++++++
> > drivers/remoteproc/remoteproc_internal.h | 23 +++++-
> > include/linux/remoteproc.h | 2 +
> > 3 files changed, 153 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_coredump.c
> > b/drivers/remoteproc/remoteproc_coredump.c
> > index 9de0467..888b7dec91 100644
> > --- a/drivers/remoteproc/remoteproc_coredump.c
> > +++ b/drivers/remoteproc/remoteproc_coredump.c
> > @@ -12,6 +12,84 @@
> > #include <linux/remoteproc.h>
> > #include "remoteproc_internal.h"
> >
> > +static void rproc_free_dump(void *data)
> > +{
> > + struct rproc_coredump_state *dump_state = data;
> > +
> > + complete(&dump_state->dump_done);
> > +}
> > +
> > +static unsigned long resolve_addr(loff_t user_offset,
> > + struct list_head *segments,
> > + unsigned long *data_left)
> > +{
> > + struct rproc_dump_segment *segment;
> > +
> > + list_for_each_entry(segment, segments, node) {
> > + if (user_offset >= segment->size)
> > + user_offset -= segment->size;
> > + else
> > + break;
> > + }
> > +
> > + if (&segment->node == segments) {
> > + *data_left = 0;
> > + return 0;
> > + }
> > +
> > + *data_left = segment->size - user_offset;
> > +
> > + return segment->da + user_offset;
> > +}
> > +
> > +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t count,
> > + void *data, size_t header_size)
> > +{
> > + void *device_mem;
> > + size_t data_left, copy_size, bytes_left = count;
> > + unsigned long addr;
> > + struct rproc_coredump_state *dump_state = data;
> > + struct rproc *rproc = dump_state->rproc;
> > + void *elfcore = dump_state->header;
> > +
> > + /* Copy the header first */
> > + if (offset < header_size) {
> > + copy_size = header_size - offset;
> > + copy_size = min(copy_size, bytes_left);
> > +
> > + memcpy(buffer, elfcore + offset, copy_size);
> > + offset += copy_size;
> > + bytes_left -= copy_size;
> > + buffer += copy_size;
> > + }
> > +
> > + while (bytes_left) {
> > + addr = resolve_addr(offset - header_size,
> > + &rproc->dump_segments, &data_left);
> > + /* EOF check */
> > + if (data_left == 0) {
> > + pr_info("Ramdump complete %lld bytes read",
> > offset);
> > + break;
> > + }
> > +
> > + copy_size = min_t(size_t, bytes_left, data_left);
> > +
> > + device_mem = rproc->ops->da_to_va(rproc, addr,
> > copy_size);
> > + if (!device_mem) {
> > + pr_err("Address:%lx with size %zd out of remoteproc
> > carveout\n",
> > + addr, copy_size);
> > + return -ENOMEM;
> > + }
> > + memcpy(buffer, device_mem, copy_size);
> > +
> > + offset += copy_size;
> > + buffer += copy_size;
> > + bytes_left -= copy_size;
> > + }
> > +
> > + return count - bytes_left;
> > +}
> > +
> > static void create_elf_header(void *data, int phnum, struct rproc *rproc)
> > {
> > struct elf32_phdr *phdr;
> > @@ -55,6 +133,58 @@ static void create_elf_header(void *data, int phnum,
> > struct rproc *rproc)
> > }
> >
> > /**
> > + * rproc_inline_coredump() - perform synchronized coredump
> > + * @rproc: rproc handle
> > + *
> > + * This function will generate an ELF header for the registered segments
> > + * and create a devcoredump device associated with rproc. This function
> > + * directly copies the segments from device memory to userspace. The
> > + * recovery is stalled until the enitire coredump is read. This approach
> Typo entire -> entire
> > + * avoids using extra vmalloc memory(which can be really large).
> > + */
> > +void rproc_inline_coredump(struct rproc *rproc)
> > +{
> > + struct rproc_dump_segment *segment;
> > + struct elf32_phdr *phdr;
> > + struct elf32_hdr *ehdr;
> > + struct rproc_coredump_state *dump_state;
> > + size_t header_size;
> > + void *data;
> > + int phnum = 0;
> > +
> > + if (list_empty(&rproc->dump_segments))
> > + return;
> > +
> > + header_size = sizeof(*ehdr);
> > + list_for_each_entry(segment, &rproc->dump_segments, node) {
> > + header_size += sizeof(*phdr);
> > +
> > + phnum++;
> > + }
> > +
> > + data = vmalloc(header_size);
> > + if (!data)
> > + return;
> > +
> > + ehdr = data;
> > + create_elf_header(data, phnum, rproc);
> > +
> > + dump_state = kzalloc(sizeof(*dump_state), GFP_KERNEL);
> > + dump_state->rproc = rproc;
> > + dump_state->header = data;
> > + init_completion(&dump_state->dump_done);
> > +
> > + dev_coredumpm(&rproc->dev, NULL, dump_state, header_size,
> > GFP_KERNEL,
> > + rproc_read_dump, rproc_free_dump);
> > +
> > + /* Wait until the dump is read and free is called */
> > + wait_for_completion(&dump_state->dump_done);
>
> Maybe good to add a timeout with value programmable via debugfs?
>
devcoredump provides a timeout already, although not configurable today.
I believe this is sufficient, but a mentioning in the comment would be
useful.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-04-17 17:11 ` Bjorn Andersson
@ 2020-04-17 17:11 ` Bjorn Andersson
0 siblings, 0 replies; 27+ messages in thread
From: Bjorn Andersson @ 2020-04-17 17:11 UTC (permalink / raw)
To: Loic PALLARDY
Cc: Rishabh Bhatnagar, linux-remoteproc, linux-kernel, ohad,
mathieu.poirier, tsoni, psodagud, sidgup
On Fri 17 Apr 00:52 PDT 2020, Loic PALLARDY wrote:
> Hi Rishabh,
>
> > -----Original Message-----
> > From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
> > owner@vger.kernel.org> On Behalf Of Rishabh Bhatnagar
> > Sent: jeudi 16 avril 2020 20:39
> > To: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> > Cc: bjorn.andersson@linaro.org; ohad@wizery.com;
> > mathieu.poirier@linaro.org; tsoni@codeaurora.org;
> > psodagud@codeaurora.org; sidgup@codeaurora.org; Rishabh Bhatnagar
> > <rishabhb@codeaurora.org>
> > Subject: [PATCH 2/3] remoteproc: Add inline coredump functionality
> >
> > This patch adds the inline coredump functionality. The current
> > coredump implementation uses vmalloc area to copy all the segments.
> > But this might put a lot of strain on low memory targets as the
> > firmware size sometimes is in ten's of MBs. The situation becomes
> > worse if there are multiple remote processors undergoing recovery
> > at the same time. This patch directly copies the device memory to
> > userspace buffer and avoids extra memory usage. This requires
> > recovery to be halted until data is read by userspace and free
> > function is called.
> >
> > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> > ---
> > drivers/remoteproc/remoteproc_coredump.c | 130
> > +++++++++++++++++++++++++++++++
> > drivers/remoteproc/remoteproc_internal.h | 23 +++++-
> > include/linux/remoteproc.h | 2 +
> > 3 files changed, 153 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_coredump.c
> > b/drivers/remoteproc/remoteproc_coredump.c
> > index 9de0467..888b7dec91 100644
> > --- a/drivers/remoteproc/remoteproc_coredump.c
> > +++ b/drivers/remoteproc/remoteproc_coredump.c
> > @@ -12,6 +12,84 @@
> > #include <linux/remoteproc.h>
> > #include "remoteproc_internal.h"
> >
> > +static void rproc_free_dump(void *data)
> > +{
> > + struct rproc_coredump_state *dump_state = data;
> > +
> > + complete(&dump_state->dump_done);
> > +}
> > +
> > +static unsigned long resolve_addr(loff_t user_offset,
> > + struct list_head *segments,
> > + unsigned long *data_left)
> > +{
> > + struct rproc_dump_segment *segment;
> > +
> > + list_for_each_entry(segment, segments, node) {
> > + if (user_offset >= segment->size)
> > + user_offset -= segment->size;
> > + else
> > + break;
> > + }
> > +
> > + if (&segment->node == segments) {
> > + *data_left = 0;
> > + return 0;
> > + }
> > +
> > + *data_left = segment->size - user_offset;
> > +
> > + return segment->da + user_offset;
> > +}
> > +
> > +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t count,
> > + void *data, size_t header_size)
> > +{
> > + void *device_mem;
> > + size_t data_left, copy_size, bytes_left = count;
> > + unsigned long addr;
> > + struct rproc_coredump_state *dump_state = data;
> > + struct rproc *rproc = dump_state->rproc;
> > + void *elfcore = dump_state->header;
> > +
> > + /* Copy the header first */
> > + if (offset < header_size) {
> > + copy_size = header_size - offset;
> > + copy_size = min(copy_size, bytes_left);
> > +
> > + memcpy(buffer, elfcore + offset, copy_size);
> > + offset += copy_size;
> > + bytes_left -= copy_size;
> > + buffer += copy_size;
> > + }
> > +
> > + while (bytes_left) {
> > + addr = resolve_addr(offset - header_size,
> > + &rproc->dump_segments, &data_left);
> > + /* EOF check */
> > + if (data_left == 0) {
> > + pr_info("Ramdump complete %lld bytes read",
> > offset);
> > + break;
> > + }
> > +
> > + copy_size = min_t(size_t, bytes_left, data_left);
> > +
> > + device_mem = rproc->ops->da_to_va(rproc, addr,
> > copy_size);
> > + if (!device_mem) {
> > + pr_err("Address:%lx with size %zd out of remoteproc
> > carveout\n",
> > + addr, copy_size);
> > + return -ENOMEM;
> > + }
> > + memcpy(buffer, device_mem, copy_size);
> > +
> > + offset += copy_size;
> > + buffer += copy_size;
> > + bytes_left -= copy_size;
> > + }
> > +
> > + return count - bytes_left;
> > +}
> > +
> > static void create_elf_header(void *data, int phnum, struct rproc *rproc)
> > {
> > struct elf32_phdr *phdr;
> > @@ -55,6 +133,58 @@ static void create_elf_header(void *data, int phnum,
> > struct rproc *rproc)
> > }
> >
> > /**
> > + * rproc_inline_coredump() - perform synchronized coredump
> > + * @rproc: rproc handle
> > + *
> > + * This function will generate an ELF header for the registered segments
> > + * and create a devcoredump device associated with rproc. This function
> > + * directly copies the segments from device memory to userspace. The
> > + * recovery is stalled until the enitire coredump is read. This approach
> Typo entire -> entire
> > + * avoids using extra vmalloc memory(which can be really large).
> > + */
> > +void rproc_inline_coredump(struct rproc *rproc)
> > +{
> > + struct rproc_dump_segment *segment;
> > + struct elf32_phdr *phdr;
> > + struct elf32_hdr *ehdr;
> > + struct rproc_coredump_state *dump_state;
> > + size_t header_size;
> > + void *data;
> > + int phnum = 0;
> > +
> > + if (list_empty(&rproc->dump_segments))
> > + return;
> > +
> > + header_size = sizeof(*ehdr);
> > + list_for_each_entry(segment, &rproc->dump_segments, node) {
> > + header_size += sizeof(*phdr);
> > +
> > + phnum++;
> > + }
> > +
> > + data = vmalloc(header_size);
> > + if (!data)
> > + return;
> > +
> > + ehdr = data;
> > + create_elf_header(data, phnum, rproc);
> > +
> > + dump_state = kzalloc(sizeof(*dump_state), GFP_KERNEL);
> > + dump_state->rproc = rproc;
> > + dump_state->header = data;
> > + init_completion(&dump_state->dump_done);
> > +
> > + dev_coredumpm(&rproc->dev, NULL, dump_state, header_size,
> > GFP_KERNEL,
> > + rproc_read_dump, rproc_free_dump);
> > +
> > + /* Wait until the dump is read and free is called */
> > + wait_for_completion(&dump_state->dump_done);
>
> Maybe good to add a timeout with value programmable via debugfs?
>
devcoredump provides a timeout already, although not configurable today.
I believe this is sufficient, but a mentioning in the comment would be
useful.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-04-16 18:38 ` [PATCH 2/3] remoteproc: Add inline coredump functionality Rishabh Bhatnagar
2020-04-16 18:38 ` Rishabh Bhatnagar
2020-04-17 7:52 ` Loic PALLARDY
@ 2020-04-20 6:01 ` kbuild test robot
2020-04-20 6:01 ` kbuild test robot
2020-04-23 20:38 ` Mathieu Poirier
2020-05-07 20:21 ` Bjorn Andersson
4 siblings, 1 reply; 27+ messages in thread
From: kbuild test robot @ 2020-04-20 6:01 UTC (permalink / raw)
To: Rishabh Bhatnagar, linux-remoteproc, linux-kernel
Cc: kbuild-all, bjorn.andersson, ohad, mathieu.poirier, tsoni,
psodagud, sidgup
[-- Attachment #1: Type: text/plain, Size: 3637 bytes --]
Hi Rishabh,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v5.6]
[cannot apply to linus/master linux/master remoteproc/for-next rpmsg/for-next v5.7-rc1 next-20200416]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Rishabh-Bhatnagar/remoteproc-Make-coredump-functionality-configurable/20200417-042251
base: 7111951b8d4973bda27ff663f2cf18b663d15b48
config: arm-omap2plus_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/remoteproc/remoteproc_coredump.c: In function 'rproc_read_dump':
>> drivers/remoteproc/remoteproc_coredump.c:68:32: error: passing argument 3 of 'resolve_addr' from incompatible pointer type [-Werror=incompatible-pointer-types]
68 | &rproc->dump_segments, &data_left);
| ^~~~~~~~~~
| |
| size_t * {aka unsigned int *}
drivers/remoteproc/remoteproc_coredump.c:24:23: note: expected 'long unsigned int *' but argument is of type 'size_t *' {aka 'unsigned int *'}
24 | unsigned long *data_left)
| ~~~~~~~~~~~~~~~^~~~~~~~~
cc1: some warnings being treated as errors
vim +/resolve_addr +68 drivers/remoteproc/remoteproc_coredump.c
44
45 static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t count,
46 void *data, size_t header_size)
47 {
48 void *device_mem;
49 size_t data_left, copy_size, bytes_left = count;
50 unsigned long addr;
51 struct rproc_coredump_state *dump_state = data;
52 struct rproc *rproc = dump_state->rproc;
53 void *elfcore = dump_state->header;
54
55 /* Copy the header first */
56 if (offset < header_size) {
57 copy_size = header_size - offset;
58 copy_size = min(copy_size, bytes_left);
59
60 memcpy(buffer, elfcore + offset, copy_size);
61 offset += copy_size;
62 bytes_left -= copy_size;
63 buffer += copy_size;
64 }
65
66 while (bytes_left) {
67 addr = resolve_addr(offset - header_size,
> 68 &rproc->dump_segments, &data_left);
69 /* EOF check */
70 if (data_left == 0) {
71 pr_info("Ramdump complete %lld bytes read", offset);
72 break;
73 }
74
75 copy_size = min_t(size_t, bytes_left, data_left);
76
77 device_mem = rproc->ops->da_to_va(rproc, addr, copy_size);
78 if (!device_mem) {
79 pr_err("Address:%lx with size %zd out of remoteproc carveout\n",
80 addr, copy_size);
81 return -ENOMEM;
82 }
83 memcpy(buffer, device_mem, copy_size);
84
85 offset += copy_size;
86 buffer += copy_size;
87 bytes_left -= copy_size;
88 }
89
90 return count - bytes_left;
91 }
92
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37511 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-04-20 6:01 ` kbuild test robot
@ 2020-04-20 6:01 ` kbuild test robot
0 siblings, 0 replies; 27+ messages in thread
From: kbuild test robot @ 2020-04-20 6:01 UTC (permalink / raw)
To: Rishabh Bhatnagar, linux-remoteproc, linux-kernel
Cc: kbuild-all, bjorn.andersson, ohad, mathieu.poirier, tsoni,
psodagud, sidgup, Rishabh Bhatnagar
[-- Attachment #1: Type: text/plain, Size: 3637 bytes --]
Hi Rishabh,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v5.6]
[cannot apply to linus/master linux/master remoteproc/for-next rpmsg/for-next v5.7-rc1 next-20200416]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Rishabh-Bhatnagar/remoteproc-Make-coredump-functionality-configurable/20200417-042251
base: 7111951b8d4973bda27ff663f2cf18b663d15b48
config: arm-omap2plus_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/remoteproc/remoteproc_coredump.c: In function 'rproc_read_dump':
>> drivers/remoteproc/remoteproc_coredump.c:68:32: error: passing argument 3 of 'resolve_addr' from incompatible pointer type [-Werror=incompatible-pointer-types]
68 | &rproc->dump_segments, &data_left);
| ^~~~~~~~~~
| |
| size_t * {aka unsigned int *}
drivers/remoteproc/remoteproc_coredump.c:24:23: note: expected 'long unsigned int *' but argument is of type 'size_t *' {aka 'unsigned int *'}
24 | unsigned long *data_left)
| ~~~~~~~~~~~~~~~^~~~~~~~~
cc1: some warnings being treated as errors
vim +/resolve_addr +68 drivers/remoteproc/remoteproc_coredump.c
44
45 static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t count,
46 void *data, size_t header_size)
47 {
48 void *device_mem;
49 size_t data_left, copy_size, bytes_left = count;
50 unsigned long addr;
51 struct rproc_coredump_state *dump_state = data;
52 struct rproc *rproc = dump_state->rproc;
53 void *elfcore = dump_state->header;
54
55 /* Copy the header first */
56 if (offset < header_size) {
57 copy_size = header_size - offset;
58 copy_size = min(copy_size, bytes_left);
59
60 memcpy(buffer, elfcore + offset, copy_size);
61 offset += copy_size;
62 bytes_left -= copy_size;
63 buffer += copy_size;
64 }
65
66 while (bytes_left) {
67 addr = resolve_addr(offset - header_size,
> 68 &rproc->dump_segments, &data_left);
69 /* EOF check */
70 if (data_left == 0) {
71 pr_info("Ramdump complete %lld bytes read", offset);
72 break;
73 }
74
75 copy_size = min_t(size_t, bytes_left, data_left);
76
77 device_mem = rproc->ops->da_to_va(rproc, addr, copy_size);
78 if (!device_mem) {
79 pr_err("Address:%lx with size %zd out of remoteproc carveout\n",
80 addr, copy_size);
81 return -ENOMEM;
82 }
83 memcpy(buffer, device_mem, copy_size);
84
85 offset += copy_size;
86 buffer += copy_size;
87 bytes_left -= copy_size;
88 }
89
90 return count - bytes_left;
91 }
92
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37511 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-04-16 18:38 ` [PATCH 2/3] remoteproc: Add inline coredump functionality Rishabh Bhatnagar
` (2 preceding siblings ...)
2020-04-20 6:01 ` kbuild test robot
@ 2020-04-23 20:38 ` Mathieu Poirier
2020-05-07 20:21 ` Bjorn Andersson
4 siblings, 0 replies; 27+ messages in thread
From: Mathieu Poirier @ 2020-04-23 20:38 UTC (permalink / raw)
To: Rishabh Bhatnagar
Cc: linux-remoteproc, linux-kernel, bjorn.andersson, ohad, tsoni,
psodagud, sidgup
On Thu, Apr 16, 2020 at 11:38:31AM -0700, Rishabh Bhatnagar wrote:
> This patch adds the inline coredump functionality. The current
> coredump implementation uses vmalloc area to copy all the segments.
> But this might put a lot of strain on low memory targets as the
> firmware size sometimes is in ten's of MBs. The situation becomes
s/ten's/tens
> worse if there are multiple remote processors undergoing recovery
/processors undergoing/processor undergoing
> at the same time. This patch directly copies the device memory to
> userspace buffer and avoids extra memory usage. This requires
> recovery to be halted until data is read by userspace and free
> function is called.
>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
> drivers/remoteproc/remoteproc_coredump.c | 130 +++++++++++++++++++++++++++++++
> drivers/remoteproc/remoteproc_internal.h | 23 +++++-
> include/linux/remoteproc.h | 2 +
> 3 files changed, 153 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
> index 9de0467..888b7dec91 100644
> --- a/drivers/remoteproc/remoteproc_coredump.c
> +++ b/drivers/remoteproc/remoteproc_coredump.c
> @@ -12,6 +12,84 @@
> #include <linux/remoteproc.h>
> #include "remoteproc_internal.h"
>
> +static void rproc_free_dump(void *data)
> +{
> + struct rproc_coredump_state *dump_state = data;
> +
> + complete(&dump_state->dump_done);
> +}
> +
> +static unsigned long resolve_addr(loff_t user_offset,
> + struct list_head *segments,
> + unsigned long *data_left)
> +{
> + struct rproc_dump_segment *segment;
> +
> + list_for_each_entry(segment, segments, node) {
> + if (user_offset >= segment->size)
> + user_offset -= segment->size;
> + else
> + break;
> + }
> +
> + if (&segment->node == segments) {
> + *data_left = 0;
> + return 0;
> + }
> +
> + *data_left = segment->size - user_offset;
> +
> + return segment->da + user_offset;
> +}
> +
> +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t count,
> + void *data, size_t header_size)
> +{
> + void *device_mem;
> + size_t data_left, copy_size, bytes_left = count;
> + unsigned long addr;
> + struct rproc_coredump_state *dump_state = data;
> + struct rproc *rproc = dump_state->rproc;
> + void *elfcore = dump_state->header;
> +
> + /* Copy the header first */
> + if (offset < header_size) {
> + copy_size = header_size - offset;
> + copy_size = min(copy_size, bytes_left);
> +
> + memcpy(buffer, elfcore + offset, copy_size);
> + offset += copy_size;
> + bytes_left -= copy_size;
> + buffer += copy_size;
> + }
> +
> + while (bytes_left) {
> + addr = resolve_addr(offset - header_size,
> + &rproc->dump_segments, &data_left);
> + /* EOF check */
> + if (data_left == 0) {
> + pr_info("Ramdump complete %lld bytes read", offset);
> + break;
> + }
> +
> + copy_size = min_t(size_t, bytes_left, data_left);
> +
> + device_mem = rproc->ops->da_to_va(rproc, addr, copy_size);
> + if (!device_mem) {
> + pr_err("Address:%lx with size %zd out of remoteproc carveout\n",
> + addr, copy_size);
> + return -ENOMEM;
> + }
> + memcpy(buffer, device_mem, copy_size);
> +
> + offset += copy_size;
> + buffer += copy_size;
> + bytes_left -= copy_size;
> + }
> +
> + return count - bytes_left;
> +}
> +
> static void create_elf_header(void *data, int phnum, struct rproc *rproc)
> {
> struct elf32_phdr *phdr;
> @@ -55,6 +133,58 @@ static void create_elf_header(void *data, int phnum, struct rproc *rproc)
> }
>
> /**
> + * rproc_inline_coredump() - perform synchronized coredump
> + * @rproc: rproc handle
> + *
> + * This function will generate an ELF header for the registered segments
> + * and create a devcoredump device associated with rproc. This function
> + * directly copies the segments from device memory to userspace. The
> + * recovery is stalled until the enitire coredump is read. This approach
s/enitire/entire
> + * avoids using extra vmalloc memory(which can be really large).
> + */
> +void rproc_inline_coredump(struct rproc *rproc)
> +{
> + struct rproc_dump_segment *segment;
> + struct elf32_phdr *phdr;
> + struct elf32_hdr *ehdr;
> + struct rproc_coredump_state *dump_state;
> + size_t header_size;
> + void *data;
> + int phnum = 0;
> +
> + if (list_empty(&rproc->dump_segments))
> + return;
The same check is also done in rproc_default_coredump(). As such it should
probably be moved to rproc_coredump().
> +
> + header_size = sizeof(*ehdr);
> + list_for_each_entry(segment, &rproc->dump_segments, node) {
> + header_size += sizeof(*phdr);
> +
> + phnum++;
> + }
> +
> + data = vmalloc(header_size);
> + if (!data)
> + return;
> +
> + ehdr = data;
> + create_elf_header(data, phnum, rproc);
> +
> + dump_state = kzalloc(sizeof(*dump_state), GFP_KERNEL);
> + dump_state->rproc = rproc;
> + dump_state->header = data;
> + init_completion(&dump_state->dump_done);
> +
> + dev_coredumpm(&rproc->dev, NULL, dump_state, header_size, GFP_KERNEL,
> + rproc_read_dump, rproc_free_dump);
> +
> + /* Wait until the dump is read and free is called */
> + wait_for_completion(&dump_state->dump_done);
> +
> + kfree(dump_state);
> +}
> +EXPORT_SYMBOL(rproc_inline_coredump);
Because this is part of remoteproc.o and the symbol is not needed in the
individual drivers, I don't think you need the export. The same applies to
rproc_default_coredump.
> +
> +/**
> * rproc_default_coredump() - perform coredump
> * @rproc: rproc handle
> *
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 28b6af2..ea6146e 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -24,6 +24,18 @@ struct rproc_debug_trace {
> struct rproc_mem_entry trace_mem;
> };
>
> +struct rproc_coredump_state {
> + struct rproc *rproc;
> + void *header;
> + struct completion dump_done;
> +};
> +
> +enum rproc_coredump_conf {
> + COREDUMP_DEFAULT,
> + COREDUMP_INLINE,
> + COREDUMP_DISABLED,
> +};
> +
> /* from remoteproc_core.c */
> void rproc_release(struct kref *kref);
> irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> @@ -49,6 +61,7 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
>
> /* from remoteproc_coredump.c */
> void rproc_default_coredump(struct rproc *rproc);
> +void rproc_inline_coredump(struct rproc *rproc);
>
> void rproc_free_vring(struct rproc_vring *rvring);
> int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> @@ -125,8 +138,14 @@ struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
> static inline
> void rproc_coredump(struct rproc *rproc)
> {
> - return rproc_default_coredump(rproc);
> -
> + switch (rproc->coredump_conf) {
> + case COREDUMP_DEFAULT:
> + return rproc_default_coredump(rproc);
> + case COREDUMP_INLINE:
> + return rproc_inline_coredump(rproc);
> + default:
> + break;
> + }
> }
>
> #endif /* REMOTEPROC_INTERNAL_H */
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 16ad666..23298ce 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -459,6 +459,7 @@ struct rproc_dump_segment {
> * @dev: virtual device for refcounting and common remoteproc behavior
> * @power: refcount of users who need this rproc powered up
> * @state: state of the device
> + * @coredump_conf: Currenlty selected coredump configuration
> * @lock: lock which protects concurrent manipulations of the rproc
> * @dbg_dir: debugfs directory of this rproc device
> * @traces: list of trace buffers
> @@ -492,6 +493,7 @@ struct rproc {
> struct device dev;
> atomic_t power;
> unsigned int state;
> + unsigned int coredump_conf;
Please make this an 'enum rproc_coredump_conf'.
> struct mutex lock;
> struct dentry *dbg_dir;
> struct list_head traces;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-04-16 18:38 ` [PATCH 2/3] remoteproc: Add inline coredump functionality Rishabh Bhatnagar
` (3 preceding siblings ...)
2020-04-23 20:38 ` Mathieu Poirier
@ 2020-05-07 20:21 ` Bjorn Andersson
2020-05-12 0:11 ` rishabhb
2020-05-14 18:07 ` rishabhb
4 siblings, 2 replies; 27+ messages in thread
From: Bjorn Andersson @ 2020-05-07 20:21 UTC (permalink / raw)
To: Rishabh Bhatnagar
Cc: linux-remoteproc, linux-kernel, ohad, mathieu.poirier, tsoni,
psodagud, sidgup
On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:
> This patch adds the inline coredump functionality. The current
> coredump implementation uses vmalloc area to copy all the segments.
> But this might put a lot of strain on low memory targets as the
> firmware size sometimes is in ten's of MBs. The situation becomes
> worse if there are multiple remote processors undergoing recovery
> at the same time. This patch directly copies the device memory to
> userspace buffer and avoids extra memory usage. This requires
> recovery to be halted until data is read by userspace and free
> function is called.
>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
> drivers/remoteproc/remoteproc_coredump.c | 130 +++++++++++++++++++++++++++++++
> drivers/remoteproc/remoteproc_internal.h | 23 +++++-
> include/linux/remoteproc.h | 2 +
> 3 files changed, 153 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
> index 9de0467..888b7dec91 100644
> --- a/drivers/remoteproc/remoteproc_coredump.c
> +++ b/drivers/remoteproc/remoteproc_coredump.c
> @@ -12,6 +12,84 @@
> #include <linux/remoteproc.h>
> #include "remoteproc_internal.h"
>
> +static void rproc_free_dump(void *data)
rproc_coredump_free()
> +{
> + struct rproc_coredump_state *dump_state = data;
> +
> + complete(&dump_state->dump_done);
vfree(dump_state->header);
> +}
> +
> +static unsigned long resolve_addr(loff_t user_offset,
rproc_coredump_find_segment()
> + struct list_head *segments,
> + unsigned long *data_left)
> +{
> + struct rproc_dump_segment *segment;
> +
> + list_for_each_entry(segment, segments, node) {
> + if (user_offset >= segment->size)
> + user_offset -= segment->size;
> + else
> + break;
if (user_offset < segment->size) {
*data_left = segment->size - user_offset;
return segment->da + user_offset;
}
user_offset -= segment->size;
> + }
*data_left = 0;
return 0;
> +
> + if (&segment->node == segments) {
> + *data_left = 0;
> + return 0;
> + }
> +
> + *data_left = segment->size - user_offset;
> +
> + return segment->da + user_offset;
> +}
> +
> +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t count,
> + void *data, size_t header_size)
> +{
> + void *device_mem;
> + size_t data_left, copy_size, bytes_left = count;
> + unsigned long addr;
> + struct rproc_coredump_state *dump_state = data;
> + struct rproc *rproc = dump_state->rproc;
> + void *elfcore = dump_state->header;
> +
> + /* Copy the header first */
> + if (offset < header_size) {
> + copy_size = header_size - offset;
> + copy_size = min(copy_size, bytes_left);
> +
> + memcpy(buffer, elfcore + offset, copy_size);
> + offset += copy_size;
> + bytes_left -= copy_size;
> + buffer += copy_size;
> + }
Perhaps you can take inspiration from devcd_readv() here?
> +
> + while (bytes_left) {
> + addr = resolve_addr(offset - header_size,
> + &rproc->dump_segments, &data_left);
> + /* EOF check */
> + if (data_left == 0) {
Afaict data_left denotes the amount of data left in this particular
segment, rather than in the entire core.
I think you should start by making bytes_left the minimum of the core
size and @count and then have this loop as long as bytes_left, copying
data to the buffer either from header or an appropriate segment based on
the current offset.
> + pr_info("Ramdump complete %lld bytes read", offset);
dev_dbg(&rproc->dev, ...)
> + break;
> + }
> +
> + copy_size = min_t(size_t, bytes_left, data_left);
> +
> + device_mem = rproc->ops->da_to_va(rproc, addr, copy_size);
rproc_da_to_va()
> + if (!device_mem) {
> + pr_err("Address:%lx with size %zd out of remoteproc carveout\n",
dev_err(&rproc->dev, "coredump: %#lx size %#zx outside of carveouts\n",
..);
> + addr, copy_size);
> + return -ENOMEM;
> + }
> + memcpy(buffer, device_mem, copy_size);
> +
> + offset += copy_size;
> + buffer += copy_size;
> + bytes_left -= copy_size;
> + }
> +
> + return count - bytes_left;
> +}
> +
> static void create_elf_header(void *data, int phnum, struct rproc *rproc)
> {
> struct elf32_phdr *phdr;
> @@ -55,6 +133,58 @@ static void create_elf_header(void *data, int phnum, struct rproc *rproc)
> }
>
> /**
> + * rproc_inline_coredump() - perform synchronized coredump
> + * @rproc: rproc handle
> + *
> + * This function will generate an ELF header for the registered segments
> + * and create a devcoredump device associated with rproc. This function
> + * directly copies the segments from device memory to userspace. The
> + * recovery is stalled until the enitire coredump is read. This approach
> + * avoids using extra vmalloc memory(which can be really large).
> + */
> +void rproc_inline_coredump(struct rproc *rproc)
> +{
> + struct rproc_dump_segment *segment;
> + struct elf32_phdr *phdr;
> + struct elf32_hdr *ehdr;
> + struct rproc_coredump_state *dump_state;
This can live on the stack, unless you follow my suggestion below...
> + size_t header_size;
> + void *data;
> + int phnum = 0;
> +
> + if (list_empty(&rproc->dump_segments))
> + return;
> +
> + header_size = sizeof(*ehdr);
> + list_for_each_entry(segment, &rproc->dump_segments, node) {
> + header_size += sizeof(*phdr);
> +
> + phnum++;
> + }
> +
> + data = vmalloc(header_size);
> + if (!data)
> + return;
> +
> + ehdr = data;
ehdr is unused.
> + create_elf_header(data, phnum, rproc);
> +
> + dump_state = kzalloc(sizeof(*dump_state), GFP_KERNEL);
> + dump_state->rproc = rproc;
> + dump_state->header = data;
> + init_completion(&dump_state->dump_done);
> +
> + dev_coredumpm(&rproc->dev, NULL, dump_state, header_size, GFP_KERNEL,
> + rproc_read_dump, rproc_free_dump);
I can help feeling that if you vmalloc() either the header or the entire
thing depending on DEFAULT vs INLINE and populate it with either all
segments or just the header, then you should be able to use the same
(custom) read function to serve both cases.
You should by doing this be able to avoid some duplication, your two
code paths would not diverge and the main difference would be if you
wait or not below (the kfree would have to go in the rproc_free_dump).
> +
> + /* Wait until the dump is read and free is called */
> + wait_for_completion(&dump_state->dump_done);
> +
> + kfree(dump_state);
> +}
> +EXPORT_SYMBOL(rproc_inline_coredump);
> +
> +/**
> * rproc_default_coredump() - perform coredump
> * @rproc: rproc handle
> *
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 28b6af2..ea6146e 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -24,6 +24,18 @@ struct rproc_debug_trace {
> struct rproc_mem_entry trace_mem;
> };
>
> +struct rproc_coredump_state {
This is only used within remoteproc_coredump.c, so please move it there.
> + struct rproc *rproc;
> + void *header;
> + struct completion dump_done;
> +};
> +
> +enum rproc_coredump_conf {
How about rproc_coredump_mechanism?
> + COREDUMP_DEFAULT,
> + COREDUMP_INLINE,
> + COREDUMP_DISABLED,
> +};
> +
> /* from remoteproc_core.c */
> void rproc_release(struct kref *kref);
> irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> @@ -49,6 +61,7 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
>
> /* from remoteproc_coredump.c */
> void rproc_default_coredump(struct rproc *rproc);
> +void rproc_inline_coredump(struct rproc *rproc);
>
> void rproc_free_vring(struct rproc_vring *rvring);
> int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> @@ -125,8 +138,14 @@ struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
> static inline
> void rproc_coredump(struct rproc *rproc)
> {
> - return rproc_default_coredump(rproc);
> -
> + switch (rproc->coredump_conf) {
> + case COREDUMP_DEFAULT:
> + return rproc_default_coredump(rproc);
> + case COREDUMP_INLINE:
> + return rproc_inline_coredump(rproc);
> + default:
> + break;
> + }
I think this better belong inside remoteproc_coredump.c
Regards,
Bjorn
> }
>
> #endif /* REMOTEPROC_INTERNAL_H */
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 16ad666..23298ce 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -459,6 +459,7 @@ struct rproc_dump_segment {
> * @dev: virtual device for refcounting and common remoteproc behavior
> * @power: refcount of users who need this rproc powered up
> * @state: state of the device
> + * @coredump_conf: Currenlty selected coredump configuration
> * @lock: lock which protects concurrent manipulations of the rproc
> * @dbg_dir: debugfs directory of this rproc device
> * @traces: list of trace buffers
> @@ -492,6 +493,7 @@ struct rproc {
> struct device dev;
> atomic_t power;
> unsigned int state;
> + unsigned int coredump_conf;
> struct mutex lock;
> struct dentry *dbg_dir;
> struct list_head traces;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-05-07 20:21 ` Bjorn Andersson
@ 2020-05-12 0:11 ` rishabhb
2020-05-12 0:30 ` Bjorn Andersson
2020-05-14 18:07 ` rishabhb
1 sibling, 1 reply; 27+ messages in thread
From: rishabhb @ 2020-05-12 0:11 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-remoteproc, linux-kernel, ohad, mathieu.poirier, tsoni,
psodagud, sidgup, linux-remoteproc-owner
On 2020-05-07 13:21, Bjorn Andersson wrote:
> On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:
>
>> This patch adds the inline coredump functionality. The current
>> coredump implementation uses vmalloc area to copy all the segments.
>> But this might put a lot of strain on low memory targets as the
>> firmware size sometimes is in ten's of MBs. The situation becomes
>> worse if there are multiple remote processors undergoing recovery
>> at the same time. This patch directly copies the device memory to
>> userspace buffer and avoids extra memory usage. This requires
>> recovery to be halted until data is read by userspace and free
>> function is called.
>>
>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>> ---
>> drivers/remoteproc/remoteproc_coredump.c | 130
>> +++++++++++++++++++++++++++++++
>> drivers/remoteproc/remoteproc_internal.h | 23 +++++-
>> include/linux/remoteproc.h | 2 +
>> 3 files changed, 153 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_coredump.c
>> b/drivers/remoteproc/remoteproc_coredump.c
>> index 9de0467..888b7dec91 100644
>> --- a/drivers/remoteproc/remoteproc_coredump.c
>> +++ b/drivers/remoteproc/remoteproc_coredump.c
>> @@ -12,6 +12,84 @@
>> #include <linux/remoteproc.h>
>> #include "remoteproc_internal.h"
>>
>> +static void rproc_free_dump(void *data)
>
> rproc_coredump_free()
>
>> +{
>> + struct rproc_coredump_state *dump_state = data;
>> +
>> + complete(&dump_state->dump_done);
>
> vfree(dump_state->header);
>
>> +}
>> +
>> +static unsigned long resolve_addr(loff_t user_offset,
>
> rproc_coredump_find_segment()
>
>> + struct list_head *segments,
>> + unsigned long *data_left)
>> +{
>> + struct rproc_dump_segment *segment;
>> +
>> + list_for_each_entry(segment, segments, node) {
>> + if (user_offset >= segment->size)
>> + user_offset -= segment->size;
>> + else
>> + break;
>
> if (user_offset < segment->size) {
> *data_left = segment->size - user_offset;
> return segment->da + user_offset;
> }
>
> user_offset -= segment->size;
>> + }
>
> *data_left = 0;
> return 0;
>
>> +
>> + if (&segment->node == segments) {
>> + *data_left = 0;
>> + return 0;
>> + }
>> +
>> + *data_left = segment->size - user_offset;
>> +
>> + return segment->da + user_offset;
>> +}
>> +
>> +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t
>> count,
>> + void *data, size_t header_size)
>> +{
>> + void *device_mem;
>> + size_t data_left, copy_size, bytes_left = count;
>> + unsigned long addr;
>> + struct rproc_coredump_state *dump_state = data;
>> + struct rproc *rproc = dump_state->rproc;
>> + void *elfcore = dump_state->header;
>> +
>> + /* Copy the header first */
>> + if (offset < header_size) {
>> + copy_size = header_size - offset;
>> + copy_size = min(copy_size, bytes_left);
>> +
>> + memcpy(buffer, elfcore + offset, copy_size);
>> + offset += copy_size;
>> + bytes_left -= copy_size;
>> + buffer += copy_size;
>> + }
>
> Perhaps you can take inspiration from devcd_readv() here?
>
>> +
>> + while (bytes_left) {
>> + addr = resolve_addr(offset - header_size,
>> + &rproc->dump_segments, &data_left);
>> + /* EOF check */
>> + if (data_left == 0) {
>
> Afaict data_left denotes the amount of data left in this particular
> segment, rather than in the entire core.
>
Yes, but it only returns 0 when the final segment has been copied
completely.
Otherwise it gives data left to copy for every segment and moves to next
segment
once the current one is copied.
> I think you should start by making bytes_left the minimum of the core
> size and @count and then have this loop as long as bytes_left, copying
> data to the buffer either from header or an appropriate segment based
> on
> the current offset.
>
That would require an extra function that calculates entire core size,
as its not available right now. Do you see any missed corner cases with
this
approach?
>> + pr_info("Ramdump complete %lld bytes read", offset);
>
> dev_dbg(&rproc->dev, ...)
>
>> + break;
>> + }
>> +
>> + copy_size = min_t(size_t, bytes_left, data_left);
>> +
>> + device_mem = rproc->ops->da_to_va(rproc, addr, copy_size);
>
> rproc_da_to_va()
>
>> + if (!device_mem) {
>> + pr_err("Address:%lx with size %zd out of remoteproc carveout\n",
>
> dev_err(&rproc->dev, "coredump: %#lx size %#zx outside of carveouts\n",
> ..);
>
>> + addr, copy_size);
>> + return -ENOMEM;
>> + }
>> + memcpy(buffer, device_mem, copy_size);
>> +
>> + offset += copy_size;
>> + buffer += copy_size;
>> + bytes_left -= copy_size;
>> + }
>> +
>> + return count - bytes_left;
>> +}
>> +
>> static void create_elf_header(void *data, int phnum, struct rproc
>> *rproc)
>> {
>> struct elf32_phdr *phdr;
>> @@ -55,6 +133,58 @@ static void create_elf_header(void *data, int
>> phnum, struct rproc *rproc)
>> }
>>
>> /**
>> + * rproc_inline_coredump() - perform synchronized coredump
>> + * @rproc: rproc handle
>> + *
>> + * This function will generate an ELF header for the registered
>> segments
>> + * and create a devcoredump device associated with rproc. This
>> function
>> + * directly copies the segments from device memory to userspace. The
>> + * recovery is stalled until the enitire coredump is read. This
>> approach
>> + * avoids using extra vmalloc memory(which can be really large).
>> + */
>> +void rproc_inline_coredump(struct rproc *rproc)
>> +{
>> + struct rproc_dump_segment *segment;
>> + struct elf32_phdr *phdr;
>> + struct elf32_hdr *ehdr;
>> + struct rproc_coredump_state *dump_state;
>
> This can live on the stack, unless you follow my suggestion below...
>
>> + size_t header_size;
>> + void *data;
>> + int phnum = 0;
>> +
>> + if (list_empty(&rproc->dump_segments))
>> + return;
>> +
>> + header_size = sizeof(*ehdr);
>> + list_for_each_entry(segment, &rproc->dump_segments, node) {
>> + header_size += sizeof(*phdr);
>> +
>> + phnum++;
>> + }
>> +
>> + data = vmalloc(header_size);
>> + if (!data)
>> + return;
>> +
>> + ehdr = data;
>
> ehdr is unused.
>
>> + create_elf_header(data, phnum, rproc);
>> +
>> + dump_state = kzalloc(sizeof(*dump_state), GFP_KERNEL);
>> + dump_state->rproc = rproc;
>> + dump_state->header = data;
>> + init_completion(&dump_state->dump_done);
>> +
>> + dev_coredumpm(&rproc->dev, NULL, dump_state, header_size,
>> GFP_KERNEL,
>> + rproc_read_dump, rproc_free_dump);
>
> I can help feeling that if you vmalloc() either the header or the
> entire
> thing depending on DEFAULT vs INLINE and populate it with either all
> segments or just the header, then you should be able to use the same
> (custom) read function to serve both cases.
>
> You should by doing this be able to avoid some duplication, your two
> code paths would not diverge and the main difference would be if you
> wait or not below (the kfree would have to go in the rproc_free_dump).
>
>> +
>> + /* Wait until the dump is read and free is called */
>> + wait_for_completion(&dump_state->dump_done);
>> +
>> + kfree(dump_state);
>> +}
>> +EXPORT_SYMBOL(rproc_inline_coredump);
>> +
>> +/**
>> * rproc_default_coredump() - perform coredump
>> * @rproc: rproc handle
>> *
>> diff --git a/drivers/remoteproc/remoteproc_internal.h
>> b/drivers/remoteproc/remoteproc_internal.h
>> index 28b6af2..ea6146e 100644
>> --- a/drivers/remoteproc/remoteproc_internal.h
>> +++ b/drivers/remoteproc/remoteproc_internal.h
>> @@ -24,6 +24,18 @@ struct rproc_debug_trace {
>> struct rproc_mem_entry trace_mem;
>> };
>>
>> +struct rproc_coredump_state {
>
> This is only used within remoteproc_coredump.c, so please move it
> there.
>
>> + struct rproc *rproc;
>> + void *header;
>> + struct completion dump_done;
>> +};
>> +
>> +enum rproc_coredump_conf {
>
> How about rproc_coredump_mechanism?
>
>> + COREDUMP_DEFAULT,
>> + COREDUMP_INLINE,
>> + COREDUMP_DISABLED,
>> +};
>> +
>> /* from remoteproc_core.c */
>> void rproc_release(struct kref *kref);
>> irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
>> @@ -49,6 +61,7 @@ struct dentry *rproc_create_trace_file(const char
>> *name, struct rproc *rproc,
>>
>> /* from remoteproc_coredump.c */
>> void rproc_default_coredump(struct rproc *rproc);
>> +void rproc_inline_coredump(struct rproc *rproc);
>>
>> void rproc_free_vring(struct rproc_vring *rvring);
>> int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>> @@ -125,8 +138,14 @@ struct resource_table
>> *rproc_find_loaded_rsc_table(struct rproc *rproc,
>> static inline
>> void rproc_coredump(struct rproc *rproc)
>> {
>> - return rproc_default_coredump(rproc);
>> -
>> + switch (rproc->coredump_conf) {
>> + case COREDUMP_DEFAULT:
>> + return rproc_default_coredump(rproc);
>> + case COREDUMP_INLINE:
>> + return rproc_inline_coredump(rproc);
>> + default:
>> + break;
>> + }
>
> I think this better belong inside remoteproc_coredump.c
>
> Regards,
> Bjorn
>
>> }
>>
>> #endif /* REMOTEPROC_INTERNAL_H */
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 16ad666..23298ce 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -459,6 +459,7 @@ struct rproc_dump_segment {
>> * @dev: virtual device for refcounting and common remoteproc
>> behavior
>> * @power: refcount of users who need this rproc powered up
>> * @state: state of the device
>> + * @coredump_conf: Currenlty selected coredump configuration
>> * @lock: lock which protects concurrent manipulations of the rproc
>> * @dbg_dir: debugfs directory of this rproc device
>> * @traces: list of trace buffers
>> @@ -492,6 +493,7 @@ struct rproc {
>> struct device dev;
>> atomic_t power;
>> unsigned int state;
>> + unsigned int coredump_conf;
>> struct mutex lock;
>> struct dentry *dbg_dir;
>> struct list_head traces;
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-05-12 0:11 ` rishabhb
@ 2020-05-12 0:30 ` Bjorn Andersson
2020-05-12 0:41 ` rishabhb
2020-05-13 18:05 ` Mathieu Poirier
0 siblings, 2 replies; 27+ messages in thread
From: Bjorn Andersson @ 2020-05-12 0:30 UTC (permalink / raw)
To: rishabhb
Cc: linux-remoteproc, linux-kernel, ohad, mathieu.poirier, tsoni,
psodagud, sidgup, linux-remoteproc-owner
On Mon 11 May 17:11 PDT 2020, rishabhb@codeaurora.org wrote:
> On 2020-05-07 13:21, Bjorn Andersson wrote:
> > On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:
> >
> > > This patch adds the inline coredump functionality. The current
> > > coredump implementation uses vmalloc area to copy all the segments.
> > > But this might put a lot of strain on low memory targets as the
> > > firmware size sometimes is in ten's of MBs. The situation becomes
> > > worse if there are multiple remote processors undergoing recovery
> > > at the same time. This patch directly copies the device memory to
> > > userspace buffer and avoids extra memory usage. This requires
> > > recovery to be halted until data is read by userspace and free
> > > function is called.
> > >
> > > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> > > ---
> > > drivers/remoteproc/remoteproc_coredump.c | 130
> > > +++++++++++++++++++++++++++++++
> > > drivers/remoteproc/remoteproc_internal.h | 23 +++++-
> > > include/linux/remoteproc.h | 2 +
> > > 3 files changed, 153 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_coredump.c
> > > b/drivers/remoteproc/remoteproc_coredump.c
> > > index 9de0467..888b7dec91 100644
> > > --- a/drivers/remoteproc/remoteproc_coredump.c
> > > +++ b/drivers/remoteproc/remoteproc_coredump.c
> > > @@ -12,6 +12,84 @@
> > > #include <linux/remoteproc.h>
> > > #include "remoteproc_internal.h"
> > >
> > > +static void rproc_free_dump(void *data)
> >
> > rproc_coredump_free()
> >
> > > +{
> > > + struct rproc_coredump_state *dump_state = data;
> > > +
> > > + complete(&dump_state->dump_done);
> >
> > vfree(dump_state->header);
> >
> > > +}
> > > +
> > > +static unsigned long resolve_addr(loff_t user_offset,
> >
> > rproc_coredump_find_segment()
> >
> > > + struct list_head *segments,
> > > + unsigned long *data_left)
> > > +{
> > > + struct rproc_dump_segment *segment;
> > > +
> > > + list_for_each_entry(segment, segments, node) {
> > > + if (user_offset >= segment->size)
> > > + user_offset -= segment->size;
> > > + else
> > > + break;
> >
> > if (user_offset < segment->size) {
> > *data_left = segment->size - user_offset;
> > return segment->da + user_offset;
> > }
> >
> > user_offset -= segment->size;
> > > + }
> >
> > *data_left = 0;
> > return 0;
> >
> > > +
> > > + if (&segment->node == segments) {
> > > + *data_left = 0;
> > > + return 0;
> > > + }
> > > +
> > > + *data_left = segment->size - user_offset;
> > > +
> > > + return segment->da + user_offset;
> > > +}
> > > +
> > > +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t
> > > count,
> > > + void *data, size_t header_size)
> > > +{
> > > + void *device_mem;
> > > + size_t data_left, copy_size, bytes_left = count;
> > > + unsigned long addr;
> > > + struct rproc_coredump_state *dump_state = data;
> > > + struct rproc *rproc = dump_state->rproc;
> > > + void *elfcore = dump_state->header;
> > > +
> > > + /* Copy the header first */
> > > + if (offset < header_size) {
> > > + copy_size = header_size - offset;
> > > + copy_size = min(copy_size, bytes_left);
> > > +
> > > + memcpy(buffer, elfcore + offset, copy_size);
> > > + offset += copy_size;
> > > + bytes_left -= copy_size;
> > > + buffer += copy_size;
> > > + }
> >
> > Perhaps you can take inspiration from devcd_readv() here?
> >
> > > +
> > > + while (bytes_left) {
> > > + addr = resolve_addr(offset - header_size,
> > > + &rproc->dump_segments, &data_left);
> > > + /* EOF check */
> > > + if (data_left == 0) {
> >
> > Afaict data_left denotes the amount of data left in this particular
> > segment, rather than in the entire core.
> >
> Yes, but it only returns 0 when the final segment has been copied
> completely. Otherwise it gives data left to copy for every segment
> and moves to next segment once the current one is copied.
You're right.
> > I think you should start by making bytes_left the minimum of the core
> > size and @count and then have this loop as long as bytes_left, copying
> > data to the buffer either from header or an appropriate segment based on
> > the current offset.
> >
> That would require an extra function that calculates entire core size,
> as its not available right now. Do you see any missed corner cases with this
> approach?
You're looping over all the segments as you're building the header
anyways, so you could simply store this in the dump_state. I think this
depend more on the ability to reuse the read function between inline and
default coredump.
Regards,
Bjorn
> > > + pr_info("Ramdump complete %lld bytes read", offset);
> >
> > dev_dbg(&rproc->dev, ...)
> >
> > > + break;
> > > + }
> > > +
> > > + copy_size = min_t(size_t, bytes_left, data_left);
> > > +
> > > + device_mem = rproc->ops->da_to_va(rproc, addr, copy_size);
> >
> > rproc_da_to_va()
> >
> > > + if (!device_mem) {
> > > + pr_err("Address:%lx with size %zd out of remoteproc carveout\n",
> >
> > dev_err(&rproc->dev, "coredump: %#lx size %#zx outside of carveouts\n",
> > ..);
> >
> > > + addr, copy_size);
> > > + return -ENOMEM;
> > > + }
> > > + memcpy(buffer, device_mem, copy_size);
> > > +
> > > + offset += copy_size;
> > > + buffer += copy_size;
> > > + bytes_left -= copy_size;
> > > + }
> > > +
> > > + return count - bytes_left;
> > > +}
> > > +
> > > static void create_elf_header(void *data, int phnum, struct rproc
> > > *rproc)
> > > {
> > > struct elf32_phdr *phdr;
> > > @@ -55,6 +133,58 @@ static void create_elf_header(void *data, int
> > > phnum, struct rproc *rproc)
> > > }
> > >
> > > /**
> > > + * rproc_inline_coredump() - perform synchronized coredump
> > > + * @rproc: rproc handle
> > > + *
> > > + * This function will generate an ELF header for the registered
> > > segments
> > > + * and create a devcoredump device associated with rproc. This
> > > function
> > > + * directly copies the segments from device memory to userspace. The
> > > + * recovery is stalled until the enitire coredump is read. This
> > > approach
> > > + * avoids using extra vmalloc memory(which can be really large).
> > > + */
> > > +void rproc_inline_coredump(struct rproc *rproc)
> > > +{
> > > + struct rproc_dump_segment *segment;
> > > + struct elf32_phdr *phdr;
> > > + struct elf32_hdr *ehdr;
> > > + struct rproc_coredump_state *dump_state;
> >
> > This can live on the stack, unless you follow my suggestion below...
> >
> > > + size_t header_size;
> > > + void *data;
> > > + int phnum = 0;
> > > +
> > > + if (list_empty(&rproc->dump_segments))
> > > + return;
> > > +
> > > + header_size = sizeof(*ehdr);
> > > + list_for_each_entry(segment, &rproc->dump_segments, node) {
> > > + header_size += sizeof(*phdr);
> > > +
> > > + phnum++;
> > > + }
> > > +
> > > + data = vmalloc(header_size);
> > > + if (!data)
> > > + return;
> > > +
> > > + ehdr = data;
> >
> > ehdr is unused.
> >
> > > + create_elf_header(data, phnum, rproc);
> > > +
> > > + dump_state = kzalloc(sizeof(*dump_state), GFP_KERNEL);
> > > + dump_state->rproc = rproc;
> > > + dump_state->header = data;
> > > + init_completion(&dump_state->dump_done);
> > > +
> > > + dev_coredumpm(&rproc->dev, NULL, dump_state, header_size,
> > > GFP_KERNEL,
> > > + rproc_read_dump, rproc_free_dump);
> >
> > I can help feeling that if you vmalloc() either the header or the entire
> > thing depending on DEFAULT vs INLINE and populate it with either all
> > segments or just the header, then you should be able to use the same
> > (custom) read function to serve both cases.
> >
> > You should by doing this be able to avoid some duplication, your two
> > code paths would not diverge and the main difference would be if you
> > wait or not below (the kfree would have to go in the rproc_free_dump).
> >
> > > +
> > > + /* Wait until the dump is read and free is called */
> > > + wait_for_completion(&dump_state->dump_done);
> > > +
> > > + kfree(dump_state);
> > > +}
> > > +EXPORT_SYMBOL(rproc_inline_coredump);
> > > +
> > > +/**
> > > * rproc_default_coredump() - perform coredump
> > > * @rproc: rproc handle
> > > *
> > > diff --git a/drivers/remoteproc/remoteproc_internal.h
> > > b/drivers/remoteproc/remoteproc_internal.h
> > > index 28b6af2..ea6146e 100644
> > > --- a/drivers/remoteproc/remoteproc_internal.h
> > > +++ b/drivers/remoteproc/remoteproc_internal.h
> > > @@ -24,6 +24,18 @@ struct rproc_debug_trace {
> > > struct rproc_mem_entry trace_mem;
> > > };
> > >
> > > +struct rproc_coredump_state {
> >
> > This is only used within remoteproc_coredump.c, so please move it there.
> >
> > > + struct rproc *rproc;
> > > + void *header;
> > > + struct completion dump_done;
> > > +};
> > > +
> > > +enum rproc_coredump_conf {
> >
> > How about rproc_coredump_mechanism?
> >
> > > + COREDUMP_DEFAULT,
> > > + COREDUMP_INLINE,
> > > + COREDUMP_DISABLED,
> > > +};
> > > +
> > > /* from remoteproc_core.c */
> > > void rproc_release(struct kref *kref);
> > > irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> > > @@ -49,6 +61,7 @@ struct dentry *rproc_create_trace_file(const char
> > > *name, struct rproc *rproc,
> > >
> > > /* from remoteproc_coredump.c */
> > > void rproc_default_coredump(struct rproc *rproc);
> > > +void rproc_inline_coredump(struct rproc *rproc);
> > >
> > > void rproc_free_vring(struct rproc_vring *rvring);
> > > int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> > > @@ -125,8 +138,14 @@ struct resource_table
> > > *rproc_find_loaded_rsc_table(struct rproc *rproc,
> > > static inline
> > > void rproc_coredump(struct rproc *rproc)
> > > {
> > > - return rproc_default_coredump(rproc);
> > > -
> > > + switch (rproc->coredump_conf) {
> > > + case COREDUMP_DEFAULT:
> > > + return rproc_default_coredump(rproc);
> > > + case COREDUMP_INLINE:
> > > + return rproc_inline_coredump(rproc);
> > > + default:
> > > + break;
> > > + }
> >
> > I think this better belong inside remoteproc_coredump.c
> >
> > Regards,
> > Bjorn
> >
> > > }
> > >
> > > #endif /* REMOTEPROC_INTERNAL_H */
> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > index 16ad666..23298ce 100644
> > > --- a/include/linux/remoteproc.h
> > > +++ b/include/linux/remoteproc.h
> > > @@ -459,6 +459,7 @@ struct rproc_dump_segment {
> > > * @dev: virtual device for refcounting and common remoteproc
> > > behavior
> > > * @power: refcount of users who need this rproc powered up
> > > * @state: state of the device
> > > + * @coredump_conf: Currenlty selected coredump configuration
> > > * @lock: lock which protects concurrent manipulations of the rproc
> > > * @dbg_dir: debugfs directory of this rproc device
> > > * @traces: list of trace buffers
> > > @@ -492,6 +493,7 @@ struct rproc {
> > > struct device dev;
> > > atomic_t power;
> > > unsigned int state;
> > > + unsigned int coredump_conf;
> > > struct mutex lock;
> > > struct dentry *dbg_dir;
> > > struct list_head traces;
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > > Forum,
> > > a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-05-12 0:30 ` Bjorn Andersson
@ 2020-05-12 0:41 ` rishabhb
2020-05-12 5:13 ` Bjorn Andersson
2020-05-13 18:05 ` Mathieu Poirier
1 sibling, 1 reply; 27+ messages in thread
From: rishabhb @ 2020-05-12 0:41 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-remoteproc, linux-kernel, ohad, mathieu.poirier, tsoni,
psodagud, sidgup, linux-remoteproc-owner
On 2020-05-11 17:30, Bjorn Andersson wrote:
> On Mon 11 May 17:11 PDT 2020, rishabhb@codeaurora.org wrote:
>
>> On 2020-05-07 13:21, Bjorn Andersson wrote:
>> > On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:
>> >
>> > > This patch adds the inline coredump functionality. The current
>> > > coredump implementation uses vmalloc area to copy all the segments.
>> > > But this might put a lot of strain on low memory targets as the
>> > > firmware size sometimes is in ten's of MBs. The situation becomes
>> > > worse if there are multiple remote processors undergoing recovery
>> > > at the same time. This patch directly copies the device memory to
>> > > userspace buffer and avoids extra memory usage. This requires
>> > > recovery to be halted until data is read by userspace and free
>> > > function is called.
>> > >
>> > > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>> > > ---
>> > > drivers/remoteproc/remoteproc_coredump.c | 130
>> > > +++++++++++++++++++++++++++++++
>> > > drivers/remoteproc/remoteproc_internal.h | 23 +++++-
>> > > include/linux/remoteproc.h | 2 +
>> > > 3 files changed, 153 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/drivers/remoteproc/remoteproc_coredump.c
>> > > b/drivers/remoteproc/remoteproc_coredump.c
>> > > index 9de0467..888b7dec91 100644
>> > > --- a/drivers/remoteproc/remoteproc_coredump.c
>> > > +++ b/drivers/remoteproc/remoteproc_coredump.c
>> > > @@ -12,6 +12,84 @@
>> > > #include <linux/remoteproc.h>
>> > > #include "remoteproc_internal.h"
>> > >
>> > > +static void rproc_free_dump(void *data)
>> >
>> > rproc_coredump_free()
>> >
>> > > +{
>> > > + struct rproc_coredump_state *dump_state = data;
>> > > +
>> > > + complete(&dump_state->dump_done);
>> >
>> > vfree(dump_state->header);
>> >
>> > > +}
>> > > +
>> > > +static unsigned long resolve_addr(loff_t user_offset,
>> >
>> > rproc_coredump_find_segment()
>> >
>> > > + struct list_head *segments,
>> > > + unsigned long *data_left)
>> > > +{
>> > > + struct rproc_dump_segment *segment;
>> > > +
>> > > + list_for_each_entry(segment, segments, node) {
>> > > + if (user_offset >= segment->size)
>> > > + user_offset -= segment->size;
>> > > + else
>> > > + break;
>> >
>> > if (user_offset < segment->size) {
>> > *data_left = segment->size - user_offset;
>> > return segment->da + user_offset;
>> > }
>> >
>> > user_offset -= segment->size;
>> > > + }
>> >
>> > *data_left = 0;
>> > return 0;
>> >
>> > > +
>> > > + if (&segment->node == segments) {
>> > > + *data_left = 0;
>> > > + return 0;
>> > > + }
>> > > +
>> > > + *data_left = segment->size - user_offset;
>> > > +
>> > > + return segment->da + user_offset;
>> > > +}
>> > > +
>> > > +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t
>> > > count,
>> > > + void *data, size_t header_size)
>> > > +{
>> > > + void *device_mem;
>> > > + size_t data_left, copy_size, bytes_left = count;
>> > > + unsigned long addr;
>> > > + struct rproc_coredump_state *dump_state = data;
>> > > + struct rproc *rproc = dump_state->rproc;
>> > > + void *elfcore = dump_state->header;
>> > > +
>> > > + /* Copy the header first */
>> > > + if (offset < header_size) {
>> > > + copy_size = header_size - offset;
>> > > + copy_size = min(copy_size, bytes_left);
>> > > +
>> > > + memcpy(buffer, elfcore + offset, copy_size);
>> > > + offset += copy_size;
>> > > + bytes_left -= copy_size;
>> > > + buffer += copy_size;
>> > > + }
>> >
>> > Perhaps you can take inspiration from devcd_readv() here?
>> >
>> > > +
>> > > + while (bytes_left) {
>> > > + addr = resolve_addr(offset - header_size,
>> > > + &rproc->dump_segments, &data_left);
>> > > + /* EOF check */
>> > > + if (data_left == 0) {
>> >
>> > Afaict data_left denotes the amount of data left in this particular
>> > segment, rather than in the entire core.
>> >
>> Yes, but it only returns 0 when the final segment has been copied
>> completely. Otherwise it gives data left to copy for every segment
>> and moves to next segment once the current one is copied.
>
> You're right.
>
>> > I think you should start by making bytes_left the minimum of the core
>> > size and @count and then have this loop as long as bytes_left, copying
>> > data to the buffer either from header or an appropriate segment based on
>> > the current offset.
>> >
>> That would require an extra function that calculates entire core size,
>> as its not available right now. Do you see any missed corner cases
>> with this
>> approach?
>
> You're looping over all the segments as you're building the header
> anyways, so you could simply store this in the dump_state. I think this
> depend more on the ability to reuse the read function between inline
> and
> default coredump.
>
> Regards,
> Bjorn
Wouldn't the first if condition take care of "default" dump as it is?
The header_size in that case would involve the 'header + all segments'.
>
>> > > + pr_info("Ramdump complete %lld bytes read", offset);
>> >
>> > dev_dbg(&rproc->dev, ...)
>> >
>> > > + break;
>> > > + }
>> > > +
>> > > + copy_size = min_t(size_t, bytes_left, data_left);
>> > > +
>> > > + device_mem = rproc->ops->da_to_va(rproc, addr, copy_size);
>> >
>> > rproc_da_to_va()
>> >
>> > > + if (!device_mem) {
>> > > + pr_err("Address:%lx with size %zd out of remoteproc carveout\n",
>> >
>> > dev_err(&rproc->dev, "coredump: %#lx size %#zx outside of carveouts\n",
>> > ..);
>> >
>> > > + addr, copy_size);
>> > > + return -ENOMEM;
>> > > + }
>> > > + memcpy(buffer, device_mem, copy_size);
>> > > +
>> > > + offset += copy_size;
>> > > + buffer += copy_size;
>> > > + bytes_left -= copy_size;
>> > > + }
>> > > +
>> > > + return count - bytes_left;
>> > > +}
>> > > +
>> > > static void create_elf_header(void *data, int phnum, struct rproc
>> > > *rproc)
>> > > {
>> > > struct elf32_phdr *phdr;
>> > > @@ -55,6 +133,58 @@ static void create_elf_header(void *data, int
>> > > phnum, struct rproc *rproc)
>> > > }
>> > >
>> > > /**
>> > > + * rproc_inline_coredump() - perform synchronized coredump
>> > > + * @rproc: rproc handle
>> > > + *
>> > > + * This function will generate an ELF header for the registered
>> > > segments
>> > > + * and create a devcoredump device associated with rproc. This
>> > > function
>> > > + * directly copies the segments from device memory to userspace. The
>> > > + * recovery is stalled until the enitire coredump is read. This
>> > > approach
>> > > + * avoids using extra vmalloc memory(which can be really large).
>> > > + */
>> > > +void rproc_inline_coredump(struct rproc *rproc)
>> > > +{
>> > > + struct rproc_dump_segment *segment;
>> > > + struct elf32_phdr *phdr;
>> > > + struct elf32_hdr *ehdr;
>> > > + struct rproc_coredump_state *dump_state;
>> >
>> > This can live on the stack, unless you follow my suggestion below...
>> >
>> > > + size_t header_size;
>> > > + void *data;
>> > > + int phnum = 0;
>> > > +
>> > > + if (list_empty(&rproc->dump_segments))
>> > > + return;
>> > > +
>> > > + header_size = sizeof(*ehdr);
>> > > + list_for_each_entry(segment, &rproc->dump_segments, node) {
>> > > + header_size += sizeof(*phdr);
>> > > +
>> > > + phnum++;
>> > > + }
>> > > +
>> > > + data = vmalloc(header_size);
>> > > + if (!data)
>> > > + return;
>> > > +
>> > > + ehdr = data;
>> >
>> > ehdr is unused.
>> >
>> > > + create_elf_header(data, phnum, rproc);
>> > > +
>> > > + dump_state = kzalloc(sizeof(*dump_state), GFP_KERNEL);
>> > > + dump_state->rproc = rproc;
>> > > + dump_state->header = data;
>> > > + init_completion(&dump_state->dump_done);
>> > > +
>> > > + dev_coredumpm(&rproc->dev, NULL, dump_state, header_size,
>> > > GFP_KERNEL,
>> > > + rproc_read_dump, rproc_free_dump);
>> >
>> > I can help feeling that if you vmalloc() either the header or the entire
>> > thing depending on DEFAULT vs INLINE and populate it with either all
>> > segments or just the header, then you should be able to use the same
>> > (custom) read function to serve both cases.
>> >
>> > You should by doing this be able to avoid some duplication, your two
>> > code paths would not diverge and the main difference would be if you
>> > wait or not below (the kfree would have to go in the rproc_free_dump).
>> >
>> > > +
>> > > + /* Wait until the dump is read and free is called */
>> > > + wait_for_completion(&dump_state->dump_done);
>> > > +
>> > > + kfree(dump_state);
>> > > +}
>> > > +EXPORT_SYMBOL(rproc_inline_coredump);
>> > > +
>> > > +/**
>> > > * rproc_default_coredump() - perform coredump
>> > > * @rproc: rproc handle
>> > > *
>> > > diff --git a/drivers/remoteproc/remoteproc_internal.h
>> > > b/drivers/remoteproc/remoteproc_internal.h
>> > > index 28b6af2..ea6146e 100644
>> > > --- a/drivers/remoteproc/remoteproc_internal.h
>> > > +++ b/drivers/remoteproc/remoteproc_internal.h
>> > > @@ -24,6 +24,18 @@ struct rproc_debug_trace {
>> > > struct rproc_mem_entry trace_mem;
>> > > };
>> > >
>> > > +struct rproc_coredump_state {
>> >
>> > This is only used within remoteproc_coredump.c, so please move it there.
>> >
>> > > + struct rproc *rproc;
>> > > + void *header;
>> > > + struct completion dump_done;
>> > > +};
>> > > +
>> > > +enum rproc_coredump_conf {
>> >
>> > How about rproc_coredump_mechanism?
>> >
>> > > + COREDUMP_DEFAULT,
>> > > + COREDUMP_INLINE,
>> > > + COREDUMP_DISABLED,
>> > > +};
>> > > +
>> > > /* from remoteproc_core.c */
>> > > void rproc_release(struct kref *kref);
>> > > irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
>> > > @@ -49,6 +61,7 @@ struct dentry *rproc_create_trace_file(const char
>> > > *name, struct rproc *rproc,
>> > >
>> > > /* from remoteproc_coredump.c */
>> > > void rproc_default_coredump(struct rproc *rproc);
>> > > +void rproc_inline_coredump(struct rproc *rproc);
>> > >
>> > > void rproc_free_vring(struct rproc_vring *rvring);
>> > > int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>> > > @@ -125,8 +138,14 @@ struct resource_table
>> > > *rproc_find_loaded_rsc_table(struct rproc *rproc,
>> > > static inline
>> > > void rproc_coredump(struct rproc *rproc)
>> > > {
>> > > - return rproc_default_coredump(rproc);
>> > > -
>> > > + switch (rproc->coredump_conf) {
>> > > + case COREDUMP_DEFAULT:
>> > > + return rproc_default_coredump(rproc);
>> > > + case COREDUMP_INLINE:
>> > > + return rproc_inline_coredump(rproc);
>> > > + default:
>> > > + break;
>> > > + }
>> >
>> > I think this better belong inside remoteproc_coredump.c
>> >
>> > Regards,
>> > Bjorn
>> >
>> > > }
>> > >
>> > > #endif /* REMOTEPROC_INTERNAL_H */
>> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> > > index 16ad666..23298ce 100644
>> > > --- a/include/linux/remoteproc.h
>> > > +++ b/include/linux/remoteproc.h
>> > > @@ -459,6 +459,7 @@ struct rproc_dump_segment {
>> > > * @dev: virtual device for refcounting and common remoteproc
>> > > behavior
>> > > * @power: refcount of users who need this rproc powered up
>> > > * @state: state of the device
>> > > + * @coredump_conf: Currenlty selected coredump configuration
>> > > * @lock: lock which protects concurrent manipulations of the rproc
>> > > * @dbg_dir: debugfs directory of this rproc device
>> > > * @traces: list of trace buffers
>> > > @@ -492,6 +493,7 @@ struct rproc {
>> > > struct device dev;
>> > > atomic_t power;
>> > > unsigned int state;
>> > > + unsigned int coredump_conf;
>> > > struct mutex lock;
>> > > struct dentry *dbg_dir;
>> > > struct list_head traces;
>> > > --
>> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> > > Forum,
>> > > a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-05-12 0:41 ` rishabhb
@ 2020-05-12 5:13 ` Bjorn Andersson
0 siblings, 0 replies; 27+ messages in thread
From: Bjorn Andersson @ 2020-05-12 5:13 UTC (permalink / raw)
To: rishabhb
Cc: linux-remoteproc, linux-kernel, ohad, mathieu.poirier, tsoni,
psodagud, sidgup, linux-remoteproc-owner
On Mon 11 May 17:41 PDT 2020, rishabhb@codeaurora.org wrote:
> On 2020-05-11 17:30, Bjorn Andersson wrote:
> > On Mon 11 May 17:11 PDT 2020, rishabhb@codeaurora.org wrote:
> > > On 2020-05-07 13:21, Bjorn Andersson wrote:
> > > > On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:
> > > > > diff --git a/drivers/remoteproc/remoteproc_coredump.c
> > > > > b/drivers/remoteproc/remoteproc_coredump.c
[..]
> > > > > +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t
> > > > > count,
> > > > > + void *data, size_t header_size)
> > > > > +{
> > > > > + void *device_mem;
> > > > > + size_t data_left, copy_size, bytes_left = count;
> > > > > + unsigned long addr;
> > > > > + struct rproc_coredump_state *dump_state = data;
> > > > > + struct rproc *rproc = dump_state->rproc;
> > > > > + void *elfcore = dump_state->header;
> > > > > +
> > > > > + /* Copy the header first */
> > > > > + if (offset < header_size) {
> > > > > + copy_size = header_size - offset;
> > > > > + copy_size = min(copy_size, bytes_left);
> > > > > +
> > > > > + memcpy(buffer, elfcore + offset, copy_size);
> > > > > + offset += copy_size;
> > > > > + bytes_left -= copy_size;
> > > > > + buffer += copy_size;
> > > > > + }
> > > >
> > > > Perhaps you can take inspiration from devcd_readv() here?
> > > >
> > > > > +
> > > > > + while (bytes_left) {
> > > > > + addr = resolve_addr(offset - header_size,
> > > > > + &rproc->dump_segments, &data_left);
> > > > > + /* EOF check */
> > > > > + if (data_left == 0) {
> > > >
> > > > Afaict data_left denotes the amount of data left in this particular
> > > > segment, rather than in the entire core.
> > > >
> > > Yes, but it only returns 0 when the final segment has been copied
> > > completely. Otherwise it gives data left to copy for every segment
> > > and moves to next segment once the current one is copied.
> >
> > You're right.
> >
> > > > I think you should start by making bytes_left the minimum of the core
> > > > size and @count and then have this loop as long as bytes_left, copying
> > > > data to the buffer either from header or an appropriate segment based on
> > > > the current offset.
> > > >
> > > That would require an extra function that calculates entire core size,
> > > as its not available right now. Do you see any missed corner cases
> > > with this
> > > approach?
> >
> > You're looping over all the segments as you're building the header
> > anyways, so you could simply store this in the dump_state. I think this
> > depend more on the ability to reuse the read function between inline and
> > default coredump.
> >
> > Regards,
> > Bjorn
>
> Wouldn't the first if condition take care of "default" dump as it is?
> The header_size in that case would involve the 'header + all segments'.
Correct.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-05-12 0:30 ` Bjorn Andersson
2020-05-12 0:41 ` rishabhb
@ 2020-05-13 18:05 ` Mathieu Poirier
1 sibling, 0 replies; 27+ messages in thread
From: Mathieu Poirier @ 2020-05-13 18:05 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rishabh Bhatnagar, linux-remoteproc, Linux Kernel Mailing List,
Ohad Ben-Cohen, tsoni, psodagud, Siddharth Gupta,
linux-remoteproc-owner
On Mon, 11 May 2020 at 18:32, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Mon 11 May 17:11 PDT 2020, rishabhb@codeaurora.org wrote:
>
> > On 2020-05-07 13:21, Bjorn Andersson wrote:
> > > On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:
> > >
> > > > This patch adds the inline coredump functionality. The current
> > > > coredump implementation uses vmalloc area to copy all the segments.
> > > > But this might put a lot of strain on low memory targets as the
> > > > firmware size sometimes is in ten's of MBs. The situation becomes
> > > > worse if there are multiple remote processors undergoing recovery
> > > > at the same time. This patch directly copies the device memory to
> > > > userspace buffer and avoids extra memory usage. This requires
> > > > recovery to be halted until data is read by userspace and free
> > > > function is called.
> > > >
> > > > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> > > > ---
> > > > drivers/remoteproc/remoteproc_coredump.c | 130
> > > > +++++++++++++++++++++++++++++++
> > > > drivers/remoteproc/remoteproc_internal.h | 23 +++++-
> > > > include/linux/remoteproc.h | 2 +
> > > > 3 files changed, 153 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/remoteproc/remoteproc_coredump.c
> > > > b/drivers/remoteproc/remoteproc_coredump.c
> > > > index 9de0467..888b7dec91 100644
> > > > --- a/drivers/remoteproc/remoteproc_coredump.c
> > > > +++ b/drivers/remoteproc/remoteproc_coredump.c
> > > > @@ -12,6 +12,84 @@
> > > > #include <linux/remoteproc.h>
> > > > #include "remoteproc_internal.h"
> > > >
> > > > +static void rproc_free_dump(void *data)
> > >
> > > rproc_coredump_free()
> > >
> > > > +{
> > > > + struct rproc_coredump_state *dump_state = data;
> > > > +
> > > > + complete(&dump_state->dump_done);
> > >
> > > vfree(dump_state->header);
> > >
> > > > +}
> > > > +
> > > > +static unsigned long resolve_addr(loff_t user_offset,
> > >
> > > rproc_coredump_find_segment()
> > >
> > > > + struct list_head *segments,
> > > > + unsigned long *data_left)
> > > > +{
> > > > + struct rproc_dump_segment *segment;
> > > > +
> > > > + list_for_each_entry(segment, segments, node) {
> > > > + if (user_offset >= segment->size)
> > > > + user_offset -= segment->size;
> > > > + else
> > > > + break;
> > >
> > > if (user_offset < segment->size) {
> > > *data_left = segment->size - user_offset;
> > > return segment->da + user_offset;
> > > }
> > >
> > > user_offset -= segment->size;
> > > > + }
> > >
> > > *data_left = 0;
> > > return 0;
> > >
> > > > +
> > > > + if (&segment->node == segments) {
> > > > + *data_left = 0;
> > > > + return 0;
> > > > + }
> > > > +
> > > > + *data_left = segment->size - user_offset;
> > > > +
> > > > + return segment->da + user_offset;
> > > > +}
> > > > +
> > > > +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t
> > > > count,
> > > > + void *data, size_t header_size)
> > > > +{
> > > > + void *device_mem;
> > > > + size_t data_left, copy_size, bytes_left = count;
> > > > + unsigned long addr;
> > > > + struct rproc_coredump_state *dump_state = data;
> > > > + struct rproc *rproc = dump_state->rproc;
> > > > + void *elfcore = dump_state->header;
> > > > +
> > > > + /* Copy the header first */
> > > > + if (offset < header_size) {
> > > > + copy_size = header_size - offset;
> > > > + copy_size = min(copy_size, bytes_left);
> > > > +
> > > > + memcpy(buffer, elfcore + offset, copy_size);
> > > > + offset += copy_size;
> > > > + bytes_left -= copy_size;
> > > > + buffer += copy_size;
> > > > + }
> > >
> > > Perhaps you can take inspiration from devcd_readv() here?
> > >
> > > > +
> > > > + while (bytes_left) {
> > > > + addr = resolve_addr(offset - header_size,
> > > > + &rproc->dump_segments, &data_left);
> > > > + /* EOF check */
> > > > + if (data_left == 0) {
> > >
> > > Afaict data_left denotes the amount of data left in this particular
> > > segment, rather than in the entire core.
> > >
> > Yes, but it only returns 0 when the final segment has been copied
> > completely. Otherwise it gives data left to copy for every segment
> > and moves to next segment once the current one is copied.
>
> You're right.
I remember spending a lot of time looking at this function and now
Bjorn has stumbled on it as well. As such either a redesign or adding
a generous amount of comments is in order.
Thanks,
Mathieu
>
> > > I think you should start by making bytes_left the minimum of the core
> > > size and @count and then have this loop as long as bytes_left, copying
> > > data to the buffer either from header or an appropriate segment based on
> > > the current offset.
> > >
> > That would require an extra function that calculates entire core size,
> > as its not available right now. Do you see any missed corner cases with this
> > approach?
>
> You're looping over all the segments as you're building the header
> anyways, so you could simply store this in the dump_state. I think this
> depend more on the ability to reuse the read function between inline and
> default coredump.
>
> Regards,
> Bjorn
>
> > > > + pr_info("Ramdump complete %lld bytes read", offset);
> > >
> > > dev_dbg(&rproc->dev, ...)
> > >
> > > > + break;
> > > > + }
> > > > +
> > > > + copy_size = min_t(size_t, bytes_left, data_left);
> > > > +
> > > > + device_mem = rproc->ops->da_to_va(rproc, addr, copy_size);
> > >
> > > rproc_da_to_va()
> > >
> > > > + if (!device_mem) {
> > > > + pr_err("Address:%lx with size %zd out of remoteproc carveout\n",
> > >
> > > dev_err(&rproc->dev, "coredump: %#lx size %#zx outside of carveouts\n",
> > > ..);
> > >
> > > > + addr, copy_size);
> > > > + return -ENOMEM;
> > > > + }
> > > > + memcpy(buffer, device_mem, copy_size);
> > > > +
> > > > + offset += copy_size;
> > > > + buffer += copy_size;
> > > > + bytes_left -= copy_size;
> > > > + }
> > > > +
> > > > + return count - bytes_left;
> > > > +}
> > > > +
> > > > static void create_elf_header(void *data, int phnum, struct rproc
> > > > *rproc)
> > > > {
> > > > struct elf32_phdr *phdr;
> > > > @@ -55,6 +133,58 @@ static void create_elf_header(void *data, int
> > > > phnum, struct rproc *rproc)
> > > > }
> > > >
> > > > /**
> > > > + * rproc_inline_coredump() - perform synchronized coredump
> > > > + * @rproc: rproc handle
> > > > + *
> > > > + * This function will generate an ELF header for the registered
> > > > segments
> > > > + * and create a devcoredump device associated with rproc. This
> > > > function
> > > > + * directly copies the segments from device memory to userspace. The
> > > > + * recovery is stalled until the enitire coredump is read. This
> > > > approach
> > > > + * avoids using extra vmalloc memory(which can be really large).
> > > > + */
> > > > +void rproc_inline_coredump(struct rproc *rproc)
> > > > +{
> > > > + struct rproc_dump_segment *segment;
> > > > + struct elf32_phdr *phdr;
> > > > + struct elf32_hdr *ehdr;
> > > > + struct rproc_coredump_state *dump_state;
> > >
> > > This can live on the stack, unless you follow my suggestion below...
> > >
> > > > + size_t header_size;
> > > > + void *data;
> > > > + int phnum = 0;
> > > > +
> > > > + if (list_empty(&rproc->dump_segments))
> > > > + return;
> > > > +
> > > > + header_size = sizeof(*ehdr);
> > > > + list_for_each_entry(segment, &rproc->dump_segments, node) {
> > > > + header_size += sizeof(*phdr);
> > > > +
> > > > + phnum++;
> > > > + }
> > > > +
> > > > + data = vmalloc(header_size);
> > > > + if (!data)
> > > > + return;
> > > > +
> > > > + ehdr = data;
> > >
> > > ehdr is unused.
> > >
> > > > + create_elf_header(data, phnum, rproc);
> > > > +
> > > > + dump_state = kzalloc(sizeof(*dump_state), GFP_KERNEL);
> > > > + dump_state->rproc = rproc;
> > > > + dump_state->header = data;
> > > > + init_completion(&dump_state->dump_done);
> > > > +
> > > > + dev_coredumpm(&rproc->dev, NULL, dump_state, header_size,
> > > > GFP_KERNEL,
> > > > + rproc_read_dump, rproc_free_dump);
> > >
> > > I can help feeling that if you vmalloc() either the header or the entire
> > > thing depending on DEFAULT vs INLINE and populate it with either all
> > > segments or just the header, then you should be able to use the same
> > > (custom) read function to serve both cases.
> > >
> > > You should by doing this be able to avoid some duplication, your two
> > > code paths would not diverge and the main difference would be if you
> > > wait or not below (the kfree would have to go in the rproc_free_dump).
> > >
> > > > +
> > > > + /* Wait until the dump is read and free is called */
> > > > + wait_for_completion(&dump_state->dump_done);
> > > > +
> > > > + kfree(dump_state);
> > > > +}
> > > > +EXPORT_SYMBOL(rproc_inline_coredump);
> > > > +
> > > > +/**
> > > > * rproc_default_coredump() - perform coredump
> > > > * @rproc: rproc handle
> > > > *
> > > > diff --git a/drivers/remoteproc/remoteproc_internal.h
> > > > b/drivers/remoteproc/remoteproc_internal.h
> > > > index 28b6af2..ea6146e 100644
> > > > --- a/drivers/remoteproc/remoteproc_internal.h
> > > > +++ b/drivers/remoteproc/remoteproc_internal.h
> > > > @@ -24,6 +24,18 @@ struct rproc_debug_trace {
> > > > struct rproc_mem_entry trace_mem;
> > > > };
> > > >
> > > > +struct rproc_coredump_state {
> > >
> > > This is only used within remoteproc_coredump.c, so please move it there.
> > >
> > > > + struct rproc *rproc;
> > > > + void *header;
> > > > + struct completion dump_done;
> > > > +};
> > > > +
> > > > +enum rproc_coredump_conf {
> > >
> > > How about rproc_coredump_mechanism?
> > >
> > > > + COREDUMP_DEFAULT,
> > > > + COREDUMP_INLINE,
> > > > + COREDUMP_DISABLED,
> > > > +};
> > > > +
> > > > /* from remoteproc_core.c */
> > > > void rproc_release(struct kref *kref);
> > > > irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> > > > @@ -49,6 +61,7 @@ struct dentry *rproc_create_trace_file(const char
> > > > *name, struct rproc *rproc,
> > > >
> > > > /* from remoteproc_coredump.c */
> > > > void rproc_default_coredump(struct rproc *rproc);
> > > > +void rproc_inline_coredump(struct rproc *rproc);
> > > >
> > > > void rproc_free_vring(struct rproc_vring *rvring);
> > > > int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> > > > @@ -125,8 +138,14 @@ struct resource_table
> > > > *rproc_find_loaded_rsc_table(struct rproc *rproc,
> > > > static inline
> > > > void rproc_coredump(struct rproc *rproc)
> > > > {
> > > > - return rproc_default_coredump(rproc);
> > > > -
> > > > + switch (rproc->coredump_conf) {
> > > > + case COREDUMP_DEFAULT:
> > > > + return rproc_default_coredump(rproc);
> > > > + case COREDUMP_INLINE:
> > > > + return rproc_inline_coredump(rproc);
> > > > + default:
> > > > + break;
> > > > + }
> > >
> > > I think this better belong inside remoteproc_coredump.c
> > >
> > > Regards,
> > > Bjorn
> > >
> > > > }
> > > >
> > > > #endif /* REMOTEPROC_INTERNAL_H */
> > > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > > index 16ad666..23298ce 100644
> > > > --- a/include/linux/remoteproc.h
> > > > +++ b/include/linux/remoteproc.h
> > > > @@ -459,6 +459,7 @@ struct rproc_dump_segment {
> > > > * @dev: virtual device for refcounting and common remoteproc
> > > > behavior
> > > > * @power: refcount of users who need this rproc powered up
> > > > * @state: state of the device
> > > > + * @coredump_conf: Currenlty selected coredump configuration
> > > > * @lock: lock which protects concurrent manipulations of the rproc
> > > > * @dbg_dir: debugfs directory of this rproc device
> > > > * @traces: list of trace buffers
> > > > @@ -492,6 +493,7 @@ struct rproc {
> > > > struct device dev;
> > > > atomic_t power;
> > > > unsigned int state;
> > > > + unsigned int coredump_conf;
> > > > struct mutex lock;
> > > > struct dentry *dbg_dir;
> > > > struct list_head traces;
> > > > --
> > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > > > Forum,
> > > > a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] remoteproc: Add inline coredump functionality
2020-05-07 20:21 ` Bjorn Andersson
2020-05-12 0:11 ` rishabhb
@ 2020-05-14 18:07 ` rishabhb
1 sibling, 0 replies; 27+ messages in thread
From: rishabhb @ 2020-05-14 18:07 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-remoteproc, linux-kernel, ohad, mathieu.poirier, tsoni,
psodagud, sidgup, linux-remoteproc-owner
On 2020-05-07 13:21, Bjorn Andersson wrote:
> On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:
>
>> This patch adds the inline coredump functionality. The current
>> coredump implementation uses vmalloc area to copy all the segments.
>> But this might put a lot of strain on low memory targets as the
>> firmware size sometimes is in ten's of MBs. The situation becomes
>> worse if there are multiple remote processors undergoing recovery
>> at the same time. This patch directly copies the device memory to
>> userspace buffer and avoids extra memory usage. This requires
>> recovery to be halted until data is read by userspace and free
>> function is called.
>>
>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>> ---
>> drivers/remoteproc/remoteproc_coredump.c | 130
>> +++++++++++++++++++++++++++++++
>> drivers/remoteproc/remoteproc_internal.h | 23 +++++-
>> include/linux/remoteproc.h | 2 +
>> 3 files changed, 153 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_coredump.c
>> b/drivers/remoteproc/remoteproc_coredump.c
>> index 9de0467..888b7dec91 100644
>> --- a/drivers/remoteproc/remoteproc_coredump.c
>> +++ b/drivers/remoteproc/remoteproc_coredump.c
>> @@ -12,6 +12,84 @@
>> #include <linux/remoteproc.h>
>> #include "remoteproc_internal.h"
>>
>> +static void rproc_free_dump(void *data)
>
> rproc_coredump_free()
>
>> +{
>> + struct rproc_coredump_state *dump_state = data;
>> +
>> + complete(&dump_state->dump_done);
>
> vfree(dump_state->header);
>
>> +}
>> +
>> +static unsigned long resolve_addr(loff_t user_offset,
>
> rproc_coredump_find_segment()
>
>> + struct list_head *segments,
>> + unsigned long *data_left)
>> +{
>> + struct rproc_dump_segment *segment;
>> +
>> + list_for_each_entry(segment, segments, node) {
>> + if (user_offset >= segment->size)
>> + user_offset -= segment->size;
>> + else
>> + break;
>
> if (user_offset < segment->size) {
> *data_left = segment->size - user_offset;
> return segment->da + user_offset;
> }
>
> user_offset -= segment->size;
>> + }
>
> *data_left = 0;
> return 0;
>
>> +
>> + if (&segment->node == segments) {
>> + *data_left = 0;
>> + return 0;
>> + }
>> +
>> + *data_left = segment->size - user_offset;
>> +
>> + return segment->da + user_offset;
>> +}
>> +
>> +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t
>> count,
>> + void *data, size_t header_size)
>> +{
>> + void *device_mem;
>> + size_t data_left, copy_size, bytes_left = count;
>> + unsigned long addr;
>> + struct rproc_coredump_state *dump_state = data;
>> + struct rproc *rproc = dump_state->rproc;
>> + void *elfcore = dump_state->header;
>> +
>> + /* Copy the header first */
>> + if (offset < header_size) {
>> + copy_size = header_size - offset;
>> + copy_size = min(copy_size, bytes_left);
>> +
>> + memcpy(buffer, elfcore + offset, copy_size);
>> + offset += copy_size;
>> + bytes_left -= copy_size;
>> + buffer += copy_size;
>> + }
>
> Perhaps you can take inspiration from devcd_readv() here?
>
>> +
>> + while (bytes_left) {
>> + addr = resolve_addr(offset - header_size,
>> + &rproc->dump_segments, &data_left);
>> + /* EOF check */
>> + if (data_left == 0) {
>
> Afaict data_left denotes the amount of data left in this particular
> segment, rather than in the entire core.
>
> I think you should start by making bytes_left the minimum of the core
> size and @count and then have this loop as long as bytes_left, copying
> data to the buffer either from header or an appropriate segment based
> on
> the current offset.
>
>> + pr_info("Ramdump complete %lld bytes read", offset);
>
> dev_dbg(&rproc->dev, ...)
>
>> + break;
>> + }
>> +
>> + copy_size = min_t(size_t, bytes_left, data_left);
>> +
>> + device_mem = rproc->ops->da_to_va(rproc, addr, copy_size);
>
> rproc_da_to_va()
>
>> + if (!device_mem) {
>> + pr_err("Address:%lx with size %zd out of remoteproc carveout\n",
>
> dev_err(&rproc->dev, "coredump: %#lx size %#zx outside of carveouts\n",
> ..);
>
>> + addr, copy_size);
>> + return -ENOMEM;
>> + }
>> + memcpy(buffer, device_mem, copy_size);
>> +
>> + offset += copy_size;
>> + buffer += copy_size;
>> + bytes_left -= copy_size;
>> + }
>> +
>> + return count - bytes_left;
>> +}
>> +
>> static void create_elf_header(void *data, int phnum, struct rproc
>> *rproc)
>> {
>> struct elf32_phdr *phdr;
>> @@ -55,6 +133,58 @@ static void create_elf_header(void *data, int
>> phnum, struct rproc *rproc)
>> }
>>
>> /**
>> + * rproc_inline_coredump() - perform synchronized coredump
>> + * @rproc: rproc handle
>> + *
>> + * This function will generate an ELF header for the registered
>> segments
>> + * and create a devcoredump device associated with rproc. This
>> function
>> + * directly copies the segments from device memory to userspace. The
>> + * recovery is stalled until the enitire coredump is read. This
>> approach
>> + * avoids using extra vmalloc memory(which can be really large).
>> + */
>> +void rproc_inline_coredump(struct rproc *rproc)
>> +{
>> + struct rproc_dump_segment *segment;
>> + struct elf32_phdr *phdr;
>> + struct elf32_hdr *ehdr;
>> + struct rproc_coredump_state *dump_state;
>
> This can live on the stack, unless you follow my suggestion below...
>
>> + size_t header_size;
>> + void *data;
>> + int phnum = 0;
>> +
>> + if (list_empty(&rproc->dump_segments))
>> + return;
>> +
>> + header_size = sizeof(*ehdr);
>> + list_for_each_entry(segment, &rproc->dump_segments, node) {
>> + header_size += sizeof(*phdr);
>> +
>> + phnum++;
>> + }
>> +
>> + data = vmalloc(header_size);
>> + if (!data)
>> + return;
>> +
>> + ehdr = data;
>
> ehdr is unused.
>
>> + create_elf_header(data, phnum, rproc);
>> +
>> + dump_state = kzalloc(sizeof(*dump_state), GFP_KERNEL);
>> + dump_state->rproc = rproc;
>> + dump_state->header = data;
>> + init_completion(&dump_state->dump_done);
>> +
>> + dev_coredumpm(&rproc->dev, NULL, dump_state, header_size,
>> GFP_KERNEL,
>> + rproc_read_dump, rproc_free_dump);
>
> I can help feeling that if you vmalloc() either the header or the
> entire
> thing depending on DEFAULT vs INLINE and populate it with either all
> segments or just the header, then you should be able to use the same
> (custom) read function to serve both cases.
>
> You should by doing this be able to avoid some duplication, your two
> code paths would not diverge and the main difference would be if you
> wait or not below (the kfree would have to go in the rproc_free_dump).
>
Moving the kfree to rproc_free_dump causes a problem as the data
structure
might get freed before the thread waiting for completion(this thread)
gets a
chance to run. So i will follow your above suggestion of having one
function
but not all go all the way i.e. I'll call dev_coredumpv or dev_coredumpm
based on
the dump configuration. This way dump_state can live on stack without
worrying
about freeing it.
>> +
>> + /* Wait until the dump is read and free is called */
>> + wait_for_completion(&dump_state->dump_done);
>> +
>> + kfree(dump_state);
>> +}
>> +EXPORT_SYMBOL(rproc_inline_coredump);
>> +
>> +/**
>> * rproc_default_coredump() - perform coredump
>> * @rproc: rproc handle
>> *
>> diff --git a/drivers/remoteproc/remoteproc_internal.h
>> b/drivers/remoteproc/remoteproc_internal.h
>> index 28b6af2..ea6146e 100644
>> --- a/drivers/remoteproc/remoteproc_internal.h
>> +++ b/drivers/remoteproc/remoteproc_internal.h
>> @@ -24,6 +24,18 @@ struct rproc_debug_trace {
>> struct rproc_mem_entry trace_mem;
>> };
>>
>> +struct rproc_coredump_state {
>
> This is only used within remoteproc_coredump.c, so please move it
> there.
>
>> + struct rproc *rproc;
>> + void *header;
>> + struct completion dump_done;
>> +};
>> +
>> +enum rproc_coredump_conf {
>
> How about rproc_coredump_mechanism?
>
>> + COREDUMP_DEFAULT,
>> + COREDUMP_INLINE,
>> + COREDUMP_DISABLED,
>> +};
>> +
>> /* from remoteproc_core.c */
>> void rproc_release(struct kref *kref);
>> irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
>> @@ -49,6 +61,7 @@ struct dentry *rproc_create_trace_file(const char
>> *name, struct rproc *rproc,
>>
>> /* from remoteproc_coredump.c */
>> void rproc_default_coredump(struct rproc *rproc);
>> +void rproc_inline_coredump(struct rproc *rproc);
>>
>> void rproc_free_vring(struct rproc_vring *rvring);
>> int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>> @@ -125,8 +138,14 @@ struct resource_table
>> *rproc_find_loaded_rsc_table(struct rproc *rproc,
>> static inline
>> void rproc_coredump(struct rproc *rproc)
>> {
>> - return rproc_default_coredump(rproc);
>> -
>> + switch (rproc->coredump_conf) {
>> + case COREDUMP_DEFAULT:
>> + return rproc_default_coredump(rproc);
>> + case COREDUMP_INLINE:
>> + return rproc_inline_coredump(rproc);
>> + default:
>> + break;
>> + }
>
> I think this better belong inside remoteproc_coredump.c
>
> Regards,
> Bjorn
>
>> }
>>
>> #endif /* REMOTEPROC_INTERNAL_H */
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 16ad666..23298ce 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -459,6 +459,7 @@ struct rproc_dump_segment {
>> * @dev: virtual device for refcounting and common remoteproc
>> behavior
>> * @power: refcount of users who need this rproc powered up
>> * @state: state of the device
>> + * @coredump_conf: Currenlty selected coredump configuration
>> * @lock: lock which protects concurrent manipulations of the rproc
>> * @dbg_dir: debugfs directory of this rproc device
>> * @traces: list of trace buffers
>> @@ -492,6 +493,7 @@ struct rproc {
>> struct device dev;
>> atomic_t power;
>> unsigned int state;
>> + unsigned int coredump_conf;
>> struct mutex lock;
>> struct dentry *dbg_dir;
>> struct list_head traces;
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/3] remoteproc: Add coredump sysfs attribute
2020-04-16 18:38 [PATCH 1/3] remoteproc: Make coredump functionality configurable Rishabh Bhatnagar
2020-04-16 18:38 ` Rishabh Bhatnagar
2020-04-16 18:38 ` [PATCH 2/3] remoteproc: Add inline coredump functionality Rishabh Bhatnagar
@ 2020-04-16 18:38 ` Rishabh Bhatnagar
2020-04-16 18:38 ` Rishabh Bhatnagar
` (2 more replies)
2020-04-23 18:07 ` [PATCH 1/3] remoteproc: Make coredump functionality configurable Mathieu Poirier
2020-05-07 19:33 ` Bjorn Andersson
4 siblings, 3 replies; 27+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-16 18:38 UTC (permalink / raw)
To: linux-remoteproc, linux-kernel
Cc: bjorn.andersson, ohad, mathieu.poirier, tsoni, psodagud, sidgup,
Rishabh Bhatnagar
Add coredump sysfs attribute to configure the type of memory dump.
User can select between default or inline coredump functionality.
Also coredump collection can be disabled through this interface.
This functionality can be configured differently for different
remote processors.
This provides an option to dynamically configure the dump type
based on userpsace capability.
Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
drivers/remoteproc/remoteproc_sysfs.c | 57 +++++++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 7f8536b..d112664 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -9,6 +9,62 @@
#define to_rproc(d) container_of(d, struct rproc, dev)
+/*
+ * A coredump-configuration-to-string lookup table, for exposing a
+ * human readable configuration via sysfs. Always keep in sync with
+ * enum rproc_coredump_conf
+ */
+static const char * const rproc_coredump_str[] = {
+ [COREDUMP_DEFAULT] = "default",
+ [COREDUMP_INLINE] = "inline",
+ [COREDUMP_DISABLED] = "disabled",
+};
+
+/* Expose the current coredump configuration via sysfs */
+static ssize_t coredump_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct rproc *rproc = to_rproc(dev);
+
+ return sprintf(buf, "%s\n", rproc_coredump_str[rproc->coredump_conf]);
+}
+
+/* Change the coredump configuration via sysfs */
+static ssize_t coredump_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct rproc *rproc = to_rproc(dev);
+ int err;
+
+ err = mutex_lock_interruptible(&rproc->lock);
+ if (err) {
+ dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
+ return -EINVAL;
+ }
+
+ if (rproc->state == RPROC_CRASHED) {
+ dev_err(dev, "can't change coredump configuration\n");
+ err = -EBUSY;
+ goto out;
+ }
+
+ if (sysfs_streq(buf, "disable"))
+ rproc->coredump_conf = COREDUMP_DISABLED;
+ else if (sysfs_streq(buf, "inline"))
+ rproc->coredump_conf = COREDUMP_INLINE;
+ else if (sysfs_streq(buf, "default"))
+ rproc->coredump_conf = COREDUMP_DEFAULT;
+ else {
+ dev_err(dev, "Invalid coredump configuration\n");
+ err = -EINVAL;
+ }
+out:
+ mutex_unlock(&rproc->lock);
+
+ return err ? err : count;
+}
+static DEVICE_ATTR_RW(coredump);
+
/* Expose the loaded / running firmware name via sysfs */
static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
char *buf)
@@ -127,6 +183,7 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
&dev_attr_firmware.attr,
&dev_attr_state.attr,
&dev_attr_name.attr,
+ &dev_attr_coredump.attr,
NULL
};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 3/3] remoteproc: Add coredump sysfs attribute
2020-04-16 18:38 ` [PATCH 3/3] remoteproc: Add coredump sysfs attribute Rishabh Bhatnagar
@ 2020-04-16 18:38 ` Rishabh Bhatnagar
2020-04-17 7:54 ` Loic PALLARDY
2020-04-23 20:47 ` Mathieu Poirier
2 siblings, 0 replies; 27+ messages in thread
From: Rishabh Bhatnagar @ 2020-04-16 18:38 UTC (permalink / raw)
To: linux-remoteproc, linux-kernel
Cc: bjorn.andersson, ohad, mathieu.poirier, tsoni, psodagud, sidgup,
Rishabh Bhatnagar
Add coredump sysfs attribute to configure the type of memory dump.
User can select between default or inline coredump functionality.
Also coredump collection can be disabled through this interface.
This functionality can be configured differently for different
remote processors.
This provides an option to dynamically configure the dump type
based on userpsace capability.
Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
drivers/remoteproc/remoteproc_sysfs.c | 57 +++++++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 7f8536b..d112664 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -9,6 +9,62 @@
#define to_rproc(d) container_of(d, struct rproc, dev)
+/*
+ * A coredump-configuration-to-string lookup table, for exposing a
+ * human readable configuration via sysfs. Always keep in sync with
+ * enum rproc_coredump_conf
+ */
+static const char * const rproc_coredump_str[] = {
+ [COREDUMP_DEFAULT] = "default",
+ [COREDUMP_INLINE] = "inline",
+ [COREDUMP_DISABLED] = "disabled",
+};
+
+/* Expose the current coredump configuration via sysfs */
+static ssize_t coredump_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct rproc *rproc = to_rproc(dev);
+
+ return sprintf(buf, "%s\n", rproc_coredump_str[rproc->coredump_conf]);
+}
+
+/* Change the coredump configuration via sysfs */
+static ssize_t coredump_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct rproc *rproc = to_rproc(dev);
+ int err;
+
+ err = mutex_lock_interruptible(&rproc->lock);
+ if (err) {
+ dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
+ return -EINVAL;
+ }
+
+ if (rproc->state == RPROC_CRASHED) {
+ dev_err(dev, "can't change coredump configuration\n");
+ err = -EBUSY;
+ goto out;
+ }
+
+ if (sysfs_streq(buf, "disable"))
+ rproc->coredump_conf = COREDUMP_DISABLED;
+ else if (sysfs_streq(buf, "inline"))
+ rproc->coredump_conf = COREDUMP_INLINE;
+ else if (sysfs_streq(buf, "default"))
+ rproc->coredump_conf = COREDUMP_DEFAULT;
+ else {
+ dev_err(dev, "Invalid coredump configuration\n");
+ err = -EINVAL;
+ }
+out:
+ mutex_unlock(&rproc->lock);
+
+ return err ? err : count;
+}
+static DEVICE_ATTR_RW(coredump);
+
/* Expose the loaded / running firmware name via sysfs */
static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
char *buf)
@@ -127,6 +183,7 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
&dev_attr_firmware.attr,
&dev_attr_state.attr,
&dev_attr_name.attr,
+ &dev_attr_coredump.attr,
NULL
};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 27+ messages in thread* RE: [PATCH 3/3] remoteproc: Add coredump sysfs attribute
2020-04-16 18:38 ` [PATCH 3/3] remoteproc: Add coredump sysfs attribute Rishabh Bhatnagar
2020-04-16 18:38 ` Rishabh Bhatnagar
@ 2020-04-17 7:54 ` Loic PALLARDY
2020-04-17 7:54 ` Loic PALLARDY
2020-04-17 17:48 ` rishabhb
2020-04-23 20:47 ` Mathieu Poirier
2 siblings, 2 replies; 27+ messages in thread
From: Loic PALLARDY @ 2020-04-17 7:54 UTC (permalink / raw)
To: Rishabh Bhatnagar, linux-remoteproc, linux-kernel
Cc: bjorn.andersson, ohad, mathieu.poirier, tsoni, psodagud, sidgup
Hi Rishabh,
> -----Original Message-----
> From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
> owner@vger.kernel.org> On Behalf Of Rishabh Bhatnagar
> Sent: jeudi 16 avril 2020 20:39
> To: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: bjorn.andersson@linaro.org; ohad@wizery.com;
> mathieu.poirier@linaro.org; tsoni@codeaurora.org;
> psodagud@codeaurora.org; sidgup@codeaurora.org; Rishabh Bhatnagar
> <rishabhb@codeaurora.org>
> Subject: [PATCH 3/3] remoteproc: Add coredump sysfs attribute
>
> Add coredump sysfs attribute to configure the type of memory dump.
> User can select between default or inline coredump functionality.
> Also coredump collection can be disabled through this interface.
> This functionality can be configured differently for different
> remote processors.
> This provides an option to dynamically configure the dump type
> based on userpsace capability.
I think this should be under debugfs as it is not link to remoteproc control but only
to its debug capability. Moreover other fields related to coredump are already un debugfs control.
Regards,
Loic
>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
> drivers/remoteproc/remoteproc_sysfs.c | 57
> +++++++++++++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c
> b/drivers/remoteproc/remoteproc_sysfs.c
> index 7f8536b..d112664 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -9,6 +9,62 @@
>
> #define to_rproc(d) container_of(d, struct rproc, dev)
>
> +/*
> + * A coredump-configuration-to-string lookup table, for exposing a
> + * human readable configuration via sysfs. Always keep in sync with
> + * enum rproc_coredump_conf
> + */
> +static const char * const rproc_coredump_str[] = {
> + [COREDUMP_DEFAULT] = "default",
> + [COREDUMP_INLINE] = "inline",
> + [COREDUMP_DISABLED] = "disabled",
> +};
> +
> +/* Expose the current coredump configuration via sysfs */
> +static ssize_t coredump_show(struct device *dev, struct device_attribute
> *attr,
> + char *buf)
> +{
> + struct rproc *rproc = to_rproc(dev);
> +
> + return sprintf(buf, "%s\n", rproc_coredump_str[rproc-
> >coredump_conf]);
> +}
> +
> +/* Change the coredump configuration via sysfs */
> +static ssize_t coredump_store(struct device *dev, struct device_attribute
> *attr,
> + const char *buf, size_t count)
> +{
> + struct rproc *rproc = to_rproc(dev);
> + int err;
> +
> + err = mutex_lock_interruptible(&rproc->lock);
> + if (err) {
> + dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
> + return -EINVAL;
> + }
> +
> + if (rproc->state == RPROC_CRASHED) {
> + dev_err(dev, "can't change coredump configuration\n");
> + err = -EBUSY;
> + goto out;
> + }
> +
> + if (sysfs_streq(buf, "disable"))
> + rproc->coredump_conf = COREDUMP_DISABLED;
> + else if (sysfs_streq(buf, "inline"))
> + rproc->coredump_conf = COREDUMP_INLINE;
> + else if (sysfs_streq(buf, "default"))
> + rproc->coredump_conf = COREDUMP_DEFAULT;
> + else {
> + dev_err(dev, "Invalid coredump configuration\n");
> + err = -EINVAL;
> + }
> +out:
> + mutex_unlock(&rproc->lock);
> +
> + return err ? err : count;
> +}
> +static DEVICE_ATTR_RW(coredump);
> +
> /* Expose the loaded / running firmware name via sysfs */
> static ssize_t firmware_show(struct device *dev, struct device_attribute
> *attr,
> char *buf)
> @@ -127,6 +183,7 @@ static ssize_t name_show(struct device *dev, struct
> device_attribute *attr,
> &dev_attr_firmware.attr,
> &dev_attr_state.attr,
> &dev_attr_name.attr,
> + &dev_attr_coredump.attr,
> NULL
> };
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread* RE: [PATCH 3/3] remoteproc: Add coredump sysfs attribute
2020-04-17 7:54 ` Loic PALLARDY
@ 2020-04-17 7:54 ` Loic PALLARDY
2020-04-17 17:48 ` rishabhb
1 sibling, 0 replies; 27+ messages in thread
From: Loic PALLARDY @ 2020-04-17 7:54 UTC (permalink / raw)
To: Rishabh Bhatnagar, linux-remoteproc, linux-kernel
Cc: bjorn.andersson, ohad, mathieu.poirier, tsoni, psodagud, sidgup
Hi Rishabh,
> -----Original Message-----
> From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
> owner@vger.kernel.org> On Behalf Of Rishabh Bhatnagar
> Sent: jeudi 16 avril 2020 20:39
> To: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: bjorn.andersson@linaro.org; ohad@wizery.com;
> mathieu.poirier@linaro.org; tsoni@codeaurora.org;
> psodagud@codeaurora.org; sidgup@codeaurora.org; Rishabh Bhatnagar
> <rishabhb@codeaurora.org>
> Subject: [PATCH 3/3] remoteproc: Add coredump sysfs attribute
>
> Add coredump sysfs attribute to configure the type of memory dump.
> User can select between default or inline coredump functionality.
> Also coredump collection can be disabled through this interface.
> This functionality can be configured differently for different
> remote processors.
> This provides an option to dynamically configure the dump type
> based on userpsace capability.
I think this should be under debugfs as it is not link to remoteproc control but only
to its debug capability. Moreover other fields related to coredump are already un debugfs control.
Regards,
Loic
>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
> drivers/remoteproc/remoteproc_sysfs.c | 57
> +++++++++++++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c
> b/drivers/remoteproc/remoteproc_sysfs.c
> index 7f8536b..d112664 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -9,6 +9,62 @@
>
> #define to_rproc(d) container_of(d, struct rproc, dev)
>
> +/*
> + * A coredump-configuration-to-string lookup table, for exposing a
> + * human readable configuration via sysfs. Always keep in sync with
> + * enum rproc_coredump_conf
> + */
> +static const char * const rproc_coredump_str[] = {
> + [COREDUMP_DEFAULT] = "default",
> + [COREDUMP_INLINE] = "inline",
> + [COREDUMP_DISABLED] = "disabled",
> +};
> +
> +/* Expose the current coredump configuration via sysfs */
> +static ssize_t coredump_show(struct device *dev, struct device_attribute
> *attr,
> + char *buf)
> +{
> + struct rproc *rproc = to_rproc(dev);
> +
> + return sprintf(buf, "%s\n", rproc_coredump_str[rproc-
> >coredump_conf]);
> +}
> +
> +/* Change the coredump configuration via sysfs */
> +static ssize_t coredump_store(struct device *dev, struct device_attribute
> *attr,
> + const char *buf, size_t count)
> +{
> + struct rproc *rproc = to_rproc(dev);
> + int err;
> +
> + err = mutex_lock_interruptible(&rproc->lock);
> + if (err) {
> + dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
> + return -EINVAL;
> + }
> +
> + if (rproc->state == RPROC_CRASHED) {
> + dev_err(dev, "can't change coredump configuration\n");
> + err = -EBUSY;
> + goto out;
> + }
> +
> + if (sysfs_streq(buf, "disable"))
> + rproc->coredump_conf = COREDUMP_DISABLED;
> + else if (sysfs_streq(buf, "inline"))
> + rproc->coredump_conf = COREDUMP_INLINE;
> + else if (sysfs_streq(buf, "default"))
> + rproc->coredump_conf = COREDUMP_DEFAULT;
> + else {
> + dev_err(dev, "Invalid coredump configuration\n");
> + err = -EINVAL;
> + }
> +out:
> + mutex_unlock(&rproc->lock);
> +
> + return err ? err : count;
> +}
> +static DEVICE_ATTR_RW(coredump);
> +
> /* Expose the loaded / running firmware name via sysfs */
> static ssize_t firmware_show(struct device *dev, struct device_attribute
> *attr,
> char *buf)
> @@ -127,6 +183,7 @@ static ssize_t name_show(struct device *dev, struct
> device_attribute *attr,
> &dev_attr_firmware.attr,
> &dev_attr_state.attr,
> &dev_attr_name.attr,
> + &dev_attr_coredump.attr,
> NULL
> };
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 3/3] remoteproc: Add coredump sysfs attribute
2020-04-17 7:54 ` Loic PALLARDY
2020-04-17 7:54 ` Loic PALLARDY
@ 2020-04-17 17:48 ` rishabhb
2020-04-17 17:48 ` rishabhb
1 sibling, 1 reply; 27+ messages in thread
From: rishabhb @ 2020-04-17 17:48 UTC (permalink / raw)
To: Loic PALLARDY
Cc: linux-remoteproc, linux-kernel, bjorn.andersson, ohad,
mathieu.poirier, tsoni, psodagud, sidgup
On 2020-04-17 00:54, Loic PALLARDY wrote:
> Hi Rishabh,
>
>> -----Original Message-----
>> From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
>> owner@vger.kernel.org> On Behalf Of Rishabh Bhatnagar
>> Sent: jeudi 16 avril 2020 20:39
>> To: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
>> Cc: bjorn.andersson@linaro.org; ohad@wizery.com;
>> mathieu.poirier@linaro.org; tsoni@codeaurora.org;
>> psodagud@codeaurora.org; sidgup@codeaurora.org; Rishabh Bhatnagar
>> <rishabhb@codeaurora.org>
>> Subject: [PATCH 3/3] remoteproc: Add coredump sysfs attribute
>>
>> Add coredump sysfs attribute to configure the type of memory dump.
>> User can select between default or inline coredump functionality.
>> Also coredump collection can be disabled through this interface.
>> This functionality can be configured differently for different
>> remote processors.
>> This provides an option to dynamically configure the dump type
>> based on userpsace capability.
> I think this should be under debugfs as it is not link to remoteproc
> control but only
> to its debug capability. Moreover other fields related to coredump are
> already un debugfs control.
>
> Regards,
> Loic
Hi Loic,
We initially thought of that but the problem is that debugfs is not
mounted for production builds. So we would be limited to only default
coredump and loose the capability to disable/move to inline coredump.
>>
>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>> ---
>> drivers/remoteproc/remoteproc_sysfs.c | 57
>> +++++++++++++++++++++++++++++++++++
>> 1 file changed, 57 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c
>> b/drivers/remoteproc/remoteproc_sysfs.c
>> index 7f8536b..d112664 100644
>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>> @@ -9,6 +9,62 @@
>>
>> #define to_rproc(d) container_of(d, struct rproc, dev)
>>
>> +/*
>> + * A coredump-configuration-to-string lookup table, for exposing a
>> + * human readable configuration via sysfs. Always keep in sync with
>> + * enum rproc_coredump_conf
>> + */
>> +static const char * const rproc_coredump_str[] = {
>> + [COREDUMP_DEFAULT] = "default",
>> + [COREDUMP_INLINE] = "inline",
>> + [COREDUMP_DISABLED] = "disabled",
>> +};
>> +
>> +/* Expose the current coredump configuration via sysfs */
>> +static ssize_t coredump_show(struct device *dev, struct
>> device_attribute
>> *attr,
>> + char *buf)
>> +{
>> + struct rproc *rproc = to_rproc(dev);
>> +
>> + return sprintf(buf, "%s\n", rproc_coredump_str[rproc-
>> >coredump_conf]);
>> +}
>> +
>> +/* Change the coredump configuration via sysfs */
>> +static ssize_t coredump_store(struct device *dev, struct
>> device_attribute
>> *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct rproc *rproc = to_rproc(dev);
>> + int err;
>> +
>> + err = mutex_lock_interruptible(&rproc->lock);
>> + if (err) {
>> + dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
>> + return -EINVAL;
>> + }
>> +
>> + if (rproc->state == RPROC_CRASHED) {
>> + dev_err(dev, "can't change coredump configuration\n");
>> + err = -EBUSY;
>> + goto out;
>> + }
>> +
>> + if (sysfs_streq(buf, "disable"))
>> + rproc->coredump_conf = COREDUMP_DISABLED;
>> + else if (sysfs_streq(buf, "inline"))
>> + rproc->coredump_conf = COREDUMP_INLINE;
>> + else if (sysfs_streq(buf, "default"))
>> + rproc->coredump_conf = COREDUMP_DEFAULT;
>> + else {
>> + dev_err(dev, "Invalid coredump configuration\n");
>> + err = -EINVAL;
>> + }
>> +out:
>> + mutex_unlock(&rproc->lock);
>> +
>> + return err ? err : count;
>> +}
>> +static DEVICE_ATTR_RW(coredump);
>> +
>> /* Expose the loaded / running firmware name via sysfs */
>> static ssize_t firmware_show(struct device *dev, struct
>> device_attribute
>> *attr,
>> char *buf)
>> @@ -127,6 +183,7 @@ static ssize_t name_show(struct device *dev,
>> struct
>> device_attribute *attr,
>> &dev_attr_firmware.attr,
>> &dev_attr_state.attr,
>> &dev_attr_name.attr,
>> + &dev_attr_coredump.attr,
>> NULL
>> };
>>
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 3/3] remoteproc: Add coredump sysfs attribute
2020-04-17 17:48 ` rishabhb
@ 2020-04-17 17:48 ` rishabhb
0 siblings, 0 replies; 27+ messages in thread
From: rishabhb @ 2020-04-17 17:48 UTC (permalink / raw)
To: Loic PALLARDY
Cc: linux-remoteproc, linux-kernel, bjorn.andersson, ohad,
mathieu.poirier, tsoni, psodagud, sidgup
On 2020-04-17 00:54, Loic PALLARDY wrote:
> Hi Rishabh,
>
>> -----Original Message-----
>> From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
>> owner@vger.kernel.org> On Behalf Of Rishabh Bhatnagar
>> Sent: jeudi 16 avril 2020 20:39
>> To: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
>> Cc: bjorn.andersson@linaro.org; ohad@wizery.com;
>> mathieu.poirier@linaro.org; tsoni@codeaurora.org;
>> psodagud@codeaurora.org; sidgup@codeaurora.org; Rishabh Bhatnagar
>> <rishabhb@codeaurora.org>
>> Subject: [PATCH 3/3] remoteproc: Add coredump sysfs attribute
>>
>> Add coredump sysfs attribute to configure the type of memory dump.
>> User can select between default or inline coredump functionality.
>> Also coredump collection can be disabled through this interface.
>> This functionality can be configured differently for different
>> remote processors.
>> This provides an option to dynamically configure the dump type
>> based on userpsace capability.
> I think this should be under debugfs as it is not link to remoteproc
> control but only
> to its debug capability. Moreover other fields related to coredump are
> already un debugfs control.
>
> Regards,
> Loic
Hi Loic,
We initially thought of that but the problem is that debugfs is not
mounted for production builds. So we would be limited to only default
coredump and loose the capability to disable/move to inline coredump.
>>
>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>> ---
>> drivers/remoteproc/remoteproc_sysfs.c | 57
>> +++++++++++++++++++++++++++++++++++
>> 1 file changed, 57 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c
>> b/drivers/remoteproc/remoteproc_sysfs.c
>> index 7f8536b..d112664 100644
>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>> @@ -9,6 +9,62 @@
>>
>> #define to_rproc(d) container_of(d, struct rproc, dev)
>>
>> +/*
>> + * A coredump-configuration-to-string lookup table, for exposing a
>> + * human readable configuration via sysfs. Always keep in sync with
>> + * enum rproc_coredump_conf
>> + */
>> +static const char * const rproc_coredump_str[] = {
>> + [COREDUMP_DEFAULT] = "default",
>> + [COREDUMP_INLINE] = "inline",
>> + [COREDUMP_DISABLED] = "disabled",
>> +};
>> +
>> +/* Expose the current coredump configuration via sysfs */
>> +static ssize_t coredump_show(struct device *dev, struct
>> device_attribute
>> *attr,
>> + char *buf)
>> +{
>> + struct rproc *rproc = to_rproc(dev);
>> +
>> + return sprintf(buf, "%s\n", rproc_coredump_str[rproc-
>> >coredump_conf]);
>> +}
>> +
>> +/* Change the coredump configuration via sysfs */
>> +static ssize_t coredump_store(struct device *dev, struct
>> device_attribute
>> *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct rproc *rproc = to_rproc(dev);
>> + int err;
>> +
>> + err = mutex_lock_interruptible(&rproc->lock);
>> + if (err) {
>> + dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
>> + return -EINVAL;
>> + }
>> +
>> + if (rproc->state == RPROC_CRASHED) {
>> + dev_err(dev, "can't change coredump configuration\n");
>> + err = -EBUSY;
>> + goto out;
>> + }
>> +
>> + if (sysfs_streq(buf, "disable"))
>> + rproc->coredump_conf = COREDUMP_DISABLED;
>> + else if (sysfs_streq(buf, "inline"))
>> + rproc->coredump_conf = COREDUMP_INLINE;
>> + else if (sysfs_streq(buf, "default"))
>> + rproc->coredump_conf = COREDUMP_DEFAULT;
>> + else {
>> + dev_err(dev, "Invalid coredump configuration\n");
>> + err = -EINVAL;
>> + }
>> +out:
>> + mutex_unlock(&rproc->lock);
>> +
>> + return err ? err : count;
>> +}
>> +static DEVICE_ATTR_RW(coredump);
>> +
>> /* Expose the loaded / running firmware name via sysfs */
>> static ssize_t firmware_show(struct device *dev, struct
>> device_attribute
>> *attr,
>> char *buf)
>> @@ -127,6 +183,7 @@ static ssize_t name_show(struct device *dev,
>> struct
>> device_attribute *attr,
>> &dev_attr_firmware.attr,
>> &dev_attr_state.attr,
>> &dev_attr_name.attr,
>> + &dev_attr_coredump.attr,
>> NULL
>> };
>>
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] remoteproc: Add coredump sysfs attribute
2020-04-16 18:38 ` [PATCH 3/3] remoteproc: Add coredump sysfs attribute Rishabh Bhatnagar
2020-04-16 18:38 ` Rishabh Bhatnagar
2020-04-17 7:54 ` Loic PALLARDY
@ 2020-04-23 20:47 ` Mathieu Poirier
2 siblings, 0 replies; 27+ messages in thread
From: Mathieu Poirier @ 2020-04-23 20:47 UTC (permalink / raw)
To: Rishabh Bhatnagar
Cc: linux-remoteproc, linux-kernel, bjorn.andersson, ohad, tsoni,
psodagud, sidgup
On Thu, Apr 16, 2020 at 11:38:32AM -0700, Rishabh Bhatnagar wrote:
> Add coredump sysfs attribute to configure the type of memory dump.
> User can select between default or inline coredump functionality.
> Also coredump collection can be disabled through this interface.
> This functionality can be configured differently for different
> remote processors.
> This provides an option to dynamically configure the dump type
> based on userpsace capability.
>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
> drivers/remoteproc/remoteproc_sysfs.c | 57 +++++++++++++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 7f8536b..d112664 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -9,6 +9,62 @@
>
> #define to_rproc(d) container_of(d, struct rproc, dev)
>
> +/*
> + * A coredump-configuration-to-string lookup table, for exposing a
> + * human readable configuration via sysfs. Always keep in sync with
> + * enum rproc_coredump_conf
> + */
> +static const char * const rproc_coredump_str[] = {
> + [COREDUMP_DEFAULT] = "default",
> + [COREDUMP_INLINE] = "inline",
> + [COREDUMP_DISABLED] = "disabled",
> +};
> +
> +/* Expose the current coredump configuration via sysfs */
> +static ssize_t coredump_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct rproc *rproc = to_rproc(dev);
> +
> + return sprintf(buf, "%s\n", rproc_coredump_str[rproc->coredump_conf]);
> +}
> +
> +/* Change the coredump configuration via sysfs */
> +static ssize_t coredump_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct rproc *rproc = to_rproc(dev);
> + int err;
> +
> + err = mutex_lock_interruptible(&rproc->lock);
> + if (err) {
> + dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
> + return -EINVAL;
> + }
> +
> + if (rproc->state == RPROC_CRASHED) {
> + dev_err(dev, "can't change coredump configuration\n");
> + err = -EBUSY;
> + goto out;
> + }
> +
> + if (sysfs_streq(buf, "disable"))
> + rproc->coredump_conf = COREDUMP_DISABLED;
> + else if (sysfs_streq(buf, "inline"))
> + rproc->coredump_conf = COREDUMP_INLINE;
> + else if (sysfs_streq(buf, "default"))
> + rproc->coredump_conf = COREDUMP_DEFAULT;
> + else {
> + dev_err(dev, "Invalid coredump configuration\n");
> + err = -EINVAL;
> + }
> +out:
> + mutex_unlock(&rproc->lock);
> +
> + return err ? err : count;
> +}
> +static DEVICE_ATTR_RW(coredump);
> +
> /* Expose the loaded / running firmware name via sysfs */
> static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> @@ -127,6 +183,7 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> &dev_attr_firmware.attr,
> &dev_attr_state.attr,
> &dev_attr_name.attr,
> + &dev_attr_coredump.attr,
This new entry needs to be documented in [1]. The set also needs to be rebased
on rproc-next.
Thanks,
Mathieu
[1]. https://elixir.bootlin.com/linux/v5.7-rc2/source/Documentation/ABI/testing/sysfs-class-remoteproc
> NULL
> };
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] remoteproc: Make coredump functionality configurable
2020-04-16 18:38 [PATCH 1/3] remoteproc: Make coredump functionality configurable Rishabh Bhatnagar
` (2 preceding siblings ...)
2020-04-16 18:38 ` [PATCH 3/3] remoteproc: Add coredump sysfs attribute Rishabh Bhatnagar
@ 2020-04-23 18:07 ` Mathieu Poirier
2020-05-07 19:33 ` Bjorn Andersson
4 siblings, 0 replies; 27+ messages in thread
From: Mathieu Poirier @ 2020-04-23 18:07 UTC (permalink / raw)
To: Rishabh Bhatnagar
Cc: linux-remoteproc, linux-kernel, bjorn.andersson, ohad, tsoni,
psodagud, sidgup
Hi Rishabh,
On Thu, Apr 16, 2020 at 11:38:30AM -0700, Rishabh Bhatnagar wrote:
> Add a new file implementing coredump specific functionality.
> This would enable clients to have the option to implement custom
> dump functionality. The default coredump functionality remains same
> as rproc_coredump function.
Next time please add a cover letter that mentions the baseline your work applies
on and version your patches.
>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/remoteproc_core.c | 83 -----------------------
> drivers/remoteproc/remoteproc_coredump.c | 111 +++++++++++++++++++++++++++++++
> drivers/remoteproc/remoteproc_internal.h | 10 +++
> 4 files changed, 122 insertions(+), 83 deletions(-)
> create mode 100644 drivers/remoteproc/remoteproc_coredump.c
>
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index e30a1b1..f1d1264 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -9,6 +9,7 @@ remoteproc-y += remoteproc_debugfs.o
> remoteproc-y += remoteproc_sysfs.o
> remoteproc-y += remoteproc_virtio.o
> remoteproc-y += remoteproc_elf_loader.o
> +remoteproc-y += remoteproc_coredump.o
> obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
> obj-$(CONFIG_MTK_SCP) += mtk_scp.o mtk_scp_ipi.o
> obj-$(CONFIG_OMAP_REMOTEPROC) += omap_remoteproc.o
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 097f33e..c0e9e5d 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1555,89 +1555,6 @@ int rproc_coredump_add_custom_segment(struct rproc *rproc,
> EXPORT_SYMBOL(rproc_coredump_add_custom_segment);
>
> /**
> - * rproc_coredump() - perform coredump
> - * @rproc: rproc handle
> - *
> - * This function will generate an ELF header for the registered segments
> - * and create a devcoredump device associated with rproc.
> - */
> -static void rproc_coredump(struct rproc *rproc)
> -{
> - struct rproc_dump_segment *segment;
> - struct elf32_phdr *phdr;
> - struct elf32_hdr *ehdr;
> - size_t data_size;
> - size_t offset;
> - void *data;
> - void *ptr;
> - int phnum = 0;
> -
> - if (list_empty(&rproc->dump_segments))
> - return;
> -
> - data_size = sizeof(*ehdr);
> - list_for_each_entry(segment, &rproc->dump_segments, node) {
> - data_size += sizeof(*phdr) + segment->size;
> -
> - phnum++;
> - }
> -
> - data = vmalloc(data_size);
> - if (!data)
> - return;
> -
> - ehdr = data;
> -
> - memset(ehdr, 0, sizeof(*ehdr));
> - memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
> - ehdr->e_ident[EI_CLASS] = ELFCLASS32;
> - ehdr->e_ident[EI_DATA] = ELFDATA2LSB;
> - ehdr->e_ident[EI_VERSION] = EV_CURRENT;
> - ehdr->e_ident[EI_OSABI] = ELFOSABI_NONE;
> - ehdr->e_type = ET_CORE;
> - ehdr->e_machine = EM_NONE;
> - ehdr->e_version = EV_CURRENT;
> - ehdr->e_entry = rproc->bootaddr;
> - ehdr->e_phoff = sizeof(*ehdr);
> - ehdr->e_ehsize = sizeof(*ehdr);
> - ehdr->e_phentsize = sizeof(*phdr);
> - ehdr->e_phnum = phnum;
> -
> - phdr = data + ehdr->e_phoff;
> - offset = ehdr->e_phoff + sizeof(*phdr) * ehdr->e_phnum;
> - list_for_each_entry(segment, &rproc->dump_segments, node) {
> - memset(phdr, 0, sizeof(*phdr));
> - phdr->p_type = PT_LOAD;
> - phdr->p_offset = offset;
> - phdr->p_vaddr = segment->da;
> - phdr->p_paddr = segment->da;
> - phdr->p_filesz = segment->size;
> - phdr->p_memsz = segment->size;
> - phdr->p_flags = PF_R | PF_W | PF_X;
> - phdr->p_align = 0;
> -
> - if (segment->dump) {
> - segment->dump(rproc, segment, data + offset);
> - } else {
> - ptr = rproc_da_to_va(rproc, segment->da, segment->size);
> - if (!ptr) {
> - dev_err(&rproc->dev,
> - "invalid coredump segment (%pad, %zu)\n",
> - &segment->da, segment->size);
> - memset(data + offset, 0xff, segment->size);
> - } else {
> - memcpy(data + offset, ptr, segment->size);
> - }
> - }
> -
> - offset += phdr->p_filesz;
> - phdr++;
> - }
> -
> - dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL);
> -}
> -
> -/**
> * rproc_trigger_recovery() - recover a remoteproc
> * @rproc: the remote processor
> *
> diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
> new file mode 100644
> index 0000000..9de0467
> --- /dev/null
> +++ b/drivers/remoteproc/remoteproc_coredump.c
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Coredump functionality for Remoteproc framework.
> + *
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/devcoredump.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/remoteproc.h>
> +#include "remoteproc_internal.h"
> +
> +static void create_elf_header(void *data, int phnum, struct rproc *rproc)
> +{
> + struct elf32_phdr *phdr;
> + struct elf32_hdr *ehdr;
> + struct rproc_dump_segment *segment;
> + int offset;
> +
> + ehdr = data;
> +
> + memset(ehdr, 0, sizeof(*ehdr));
> + memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
> + ehdr->e_ident[EI_CLASS] = ELFCLASS32;
> + ehdr->e_ident[EI_DATA] = ELFDATA2LSB;
> + ehdr->e_ident[EI_VERSION] = EV_CURRENT;
> + ehdr->e_ident[EI_OSABI] = ELFOSABI_NONE;
> + ehdr->e_type = ET_CORE;
> + ehdr->e_machine = EM_NONE;
> + ehdr->e_version = EV_CURRENT;
> + ehdr->e_entry = rproc->bootaddr;
> + ehdr->e_phoff = sizeof(*ehdr);
> + ehdr->e_ehsize = sizeof(*ehdr);
> + ehdr->e_phentsize = sizeof(*phdr);
> + ehdr->e_phnum = phnum;
> +
> + phdr = data + ehdr->e_phoff;
> + offset = ehdr->e_phoff + sizeof(*phdr) * ehdr->e_phnum;
> + list_for_each_entry(segment, &rproc->dump_segments, node) {
> + memset(phdr, 0, sizeof(*phdr));
> + phdr->p_type = PT_LOAD;
> + phdr->p_offset = offset;
> + phdr->p_vaddr = segment->da;
> + phdr->p_paddr = segment->da;
> + phdr->p_filesz = segment->size;
> + phdr->p_memsz = segment->size;
> + phdr->p_flags = PF_R | PF_W | PF_X;
> + phdr->p_align = 0;
> +
> + offset += phdr->p_filesz;
> + phdr++;
> + }
> +}
> +
> +/**
> + * rproc_default_coredump() - perform coredump
> + * @rproc: rproc handle
> + *
> + * This function will generate an ELF header for the registered segments
> + * and create a devcoredump device associated with rproc.
> + */
> +void rproc_default_coredump(struct rproc *rproc)
> +{
> + struct rproc_dump_segment *segment;
> + struct elf32_phdr *phdr;
> + struct elf32_hdr *ehdr;
> + size_t data_size;
> + void *data, *ptr;
> + int offset, phnum = 0;
> +
> + if (list_empty(&rproc->dump_segments))
> + return;
> +
> + data_size = sizeof(*ehdr);
> + list_for_each_entry(segment, &rproc->dump_segments, node) {
> + data_size += sizeof(*phdr) + segment->size;
> +
> + phnum++;
> + }
> +
> + data = vmalloc(data_size);
> + if (!data)
> + return;
> +
> + ehdr = data;
> + create_elf_header(data, phnum, rproc);
Please leave function rproc_default_coredump() the same way it originally was
for this patch. Modifications can be done in later patches.
> +
> + phdr = data + ehdr->e_phoff;
> + offset = ehdr->e_phoff + sizeof(*phdr) * ehdr->e_phnum;
> + list_for_each_entry(segment, &rproc->dump_segments, node) {
> + if (segment->dump) {
> + segment->dump(rproc, segment, data + offset);
> + } else {
> + ptr = rproc_da_to_va(rproc, segment->da, segment->size);
> + if (!ptr) {
> + dev_err(&rproc->dev,
> + "invalid coredump segment (%pad, %zu)\n",
> + &segment->da, segment->size);
> + memset(data + offset, 0xff, segment->size);
> + } else {
> + memcpy(data + offset, ptr, segment->size);
> + }
> + }
> + phdr++;
Here @offset is not updated and as such will trample the previous value with
each iteration. To be honest I would rather keep the initialisation of @phdr in
this loop, that way it is done only once.
Thanks,
Mathieu
> + }
> +
> + dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL);
> +}
> +EXPORT_SYMBOL(rproc_default_coredump);
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 493ef92..28b6af2 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -47,6 +47,9 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
> int rproc_init_sysfs(void);
> void rproc_exit_sysfs(void);
>
> +/* from remoteproc_coredump.c */
> +void rproc_default_coredump(struct rproc *rproc);
> +
> void rproc_free_vring(struct rproc_vring *rvring);
> int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>
> @@ -119,4 +122,11 @@ struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
> return NULL;
> }
>
> +static inline
> +void rproc_coredump(struct rproc *rproc)
> +{
> + return rproc_default_coredump(rproc);
> +
> +}
> +
> #endif /* REMOTEPROC_INTERNAL_H */
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/3] remoteproc: Make coredump functionality configurable
2020-04-16 18:38 [PATCH 1/3] remoteproc: Make coredump functionality configurable Rishabh Bhatnagar
` (3 preceding siblings ...)
2020-04-23 18:07 ` [PATCH 1/3] remoteproc: Make coredump functionality configurable Mathieu Poirier
@ 2020-05-07 19:33 ` Bjorn Andersson
4 siblings, 0 replies; 27+ messages in thread
From: Bjorn Andersson @ 2020-05-07 19:33 UTC (permalink / raw)
To: Rishabh Bhatnagar
Cc: linux-remoteproc, linux-kernel, ohad, mathieu.poirier, tsoni,
psodagud, sidgup
On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:
$subject says that this patch makes coredump configurable, but this
patch simply moves the rproc_coredump() to its own file.
I think this is the right thing to do and should be done as verbatim as
possible (i.e. just make sure it still works). I do think you should
include the remaining functions for adding segments etc, so that we
capture all of "coredump" in this new file.
> Add a new file implementing coredump specific functionality.
> This would enable clients to have the option to implement custom
> dump functionality. The default coredump functionality remains same
> as rproc_coredump function.
So please update $subject and reword this message to describe the fact
that you're just moving code out of the core, for the purpose of
upcoming enhancements.
>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/remoteproc_core.c | 83 -----------------------
> drivers/remoteproc/remoteproc_coredump.c | 111 +++++++++++++++++++++++++++++++
> drivers/remoteproc/remoteproc_internal.h | 10 +++
> 4 files changed, 122 insertions(+), 83 deletions(-)
> create mode 100644 drivers/remoteproc/remoteproc_coredump.c
>
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index e30a1b1..f1d1264 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -9,6 +9,7 @@ remoteproc-y += remoteproc_debugfs.o
> remoteproc-y += remoteproc_sysfs.o
> remoteproc-y += remoteproc_virtio.o
> remoteproc-y += remoteproc_elf_loader.o
> +remoteproc-y += remoteproc_coredump.o
If you move this up right below remoteproc_core.o you're maintaining the
almost alphabetical order of these.
> obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
> obj-$(CONFIG_MTK_SCP) += mtk_scp.o mtk_scp_ipi.o
> obj-$(CONFIG_OMAP_REMOTEPROC) += omap_remoteproc.o
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 097f33e..c0e9e5d 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1555,89 +1555,6 @@ int rproc_coredump_add_custom_segment(struct rproc *rproc,
> EXPORT_SYMBOL(rproc_coredump_add_custom_segment);
>
> /**
> - * rproc_coredump() - perform coredump
> - * @rproc: rproc handle
> - *
> - * This function will generate an ELF header for the registered segments
> - * and create a devcoredump device associated with rproc.
> - */
> -static void rproc_coredump(struct rproc *rproc)
> -{
> - struct rproc_dump_segment *segment;
> - struct elf32_phdr *phdr;
> - struct elf32_hdr *ehdr;
> - size_t data_size;
> - size_t offset;
> - void *data;
> - void *ptr;
> - int phnum = 0;
> -
> - if (list_empty(&rproc->dump_segments))
> - return;
> -
> - data_size = sizeof(*ehdr);
> - list_for_each_entry(segment, &rproc->dump_segments, node) {
> - data_size += sizeof(*phdr) + segment->size;
> -
> - phnum++;
> - }
> -
> - data = vmalloc(data_size);
> - if (!data)
> - return;
> -
> - ehdr = data;
> -
> - memset(ehdr, 0, sizeof(*ehdr));
> - memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
> - ehdr->e_ident[EI_CLASS] = ELFCLASS32;
> - ehdr->e_ident[EI_DATA] = ELFDATA2LSB;
> - ehdr->e_ident[EI_VERSION] = EV_CURRENT;
> - ehdr->e_ident[EI_OSABI] = ELFOSABI_NONE;
> - ehdr->e_type = ET_CORE;
> - ehdr->e_machine = EM_NONE;
> - ehdr->e_version = EV_CURRENT;
> - ehdr->e_entry = rproc->bootaddr;
> - ehdr->e_phoff = sizeof(*ehdr);
> - ehdr->e_ehsize = sizeof(*ehdr);
> - ehdr->e_phentsize = sizeof(*phdr);
> - ehdr->e_phnum = phnum;
> -
> - phdr = data + ehdr->e_phoff;
> - offset = ehdr->e_phoff + sizeof(*phdr) * ehdr->e_phnum;
> - list_for_each_entry(segment, &rproc->dump_segments, node) {
> - memset(phdr, 0, sizeof(*phdr));
> - phdr->p_type = PT_LOAD;
> - phdr->p_offset = offset;
> - phdr->p_vaddr = segment->da;
> - phdr->p_paddr = segment->da;
> - phdr->p_filesz = segment->size;
> - phdr->p_memsz = segment->size;
> - phdr->p_flags = PF_R | PF_W | PF_X;
> - phdr->p_align = 0;
> -
> - if (segment->dump) {
> - segment->dump(rproc, segment, data + offset);
> - } else {
> - ptr = rproc_da_to_va(rproc, segment->da, segment->size);
> - if (!ptr) {
> - dev_err(&rproc->dev,
> - "invalid coredump segment (%pad, %zu)\n",
> - &segment->da, segment->size);
> - memset(data + offset, 0xff, segment->size);
> - } else {
> - memcpy(data + offset, ptr, segment->size);
> - }
> - }
> -
> - offset += phdr->p_filesz;
> - phdr++;
> - }
> -
> - dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL);
> -}
> -
> -/**
> * rproc_trigger_recovery() - recover a remoteproc
> * @rproc: the remote processor
> *
> diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
> new file mode 100644
> index 0000000..9de0467
> --- /dev/null
> +++ b/drivers/remoteproc/remoteproc_coredump.c
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Coredump functionality for Remoteproc framework.
> + *
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/devcoredump.h>
You should be able to remove this from remoteproc_core.c.
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/remoteproc.h>
> +#include "remoteproc_internal.h"
> +
> +static void create_elf_header(void *data, int phnum, struct rproc *rproc)
As Mathieu pointed out, please move the code (as) verbatim (as possible)
in this commit and then follow up with enhancements in subsequent
commits.
> +{
> + struct elf32_phdr *phdr;
> + struct elf32_hdr *ehdr;
> + struct rproc_dump_segment *segment;
> + int offset;
> +
> + ehdr = data;
> +
> + memset(ehdr, 0, sizeof(*ehdr));
> + memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
> + ehdr->e_ident[EI_CLASS] = ELFCLASS32;
> + ehdr->e_ident[EI_DATA] = ELFDATA2LSB;
> + ehdr->e_ident[EI_VERSION] = EV_CURRENT;
> + ehdr->e_ident[EI_OSABI] = ELFOSABI_NONE;
> + ehdr->e_type = ET_CORE;
> + ehdr->e_machine = EM_NONE;
> + ehdr->e_version = EV_CURRENT;
> + ehdr->e_entry = rproc->bootaddr;
> + ehdr->e_phoff = sizeof(*ehdr);
> + ehdr->e_ehsize = sizeof(*ehdr);
> + ehdr->e_phentsize = sizeof(*phdr);
> + ehdr->e_phnum = phnum;
> +
> + phdr = data + ehdr->e_phoff;
> + offset = ehdr->e_phoff + sizeof(*phdr) * ehdr->e_phnum;
> + list_for_each_entry(segment, &rproc->dump_segments, node) {
> + memset(phdr, 0, sizeof(*phdr));
> + phdr->p_type = PT_LOAD;
> + phdr->p_offset = offset;
> + phdr->p_vaddr = segment->da;
> + phdr->p_paddr = segment->da;
> + phdr->p_filesz = segment->size;
> + phdr->p_memsz = segment->size;
> + phdr->p_flags = PF_R | PF_W | PF_X;
> + phdr->p_align = 0;
> +
> + offset += phdr->p_filesz;
> + phdr++;
> + }
> +}
> +
> +/**
> + * rproc_default_coredump() - perform coredump
> + * @rproc: rproc handle
> + *
> + * This function will generate an ELF header for the registered segments
> + * and create a devcoredump device associated with rproc.
> + */
> +void rproc_default_coredump(struct rproc *rproc)
> +{
> + struct rproc_dump_segment *segment;
> + struct elf32_phdr *phdr;
> + struct elf32_hdr *ehdr;
> + size_t data_size;
> + void *data, *ptr;
> + int offset, phnum = 0;
> +
> + if (list_empty(&rproc->dump_segments))
> + return;
> +
> + data_size = sizeof(*ehdr);
> + list_for_each_entry(segment, &rproc->dump_segments, node) {
> + data_size += sizeof(*phdr) + segment->size;
> +
> + phnum++;
> + }
> +
> + data = vmalloc(data_size);
> + if (!data)
> + return;
> +
> + ehdr = data;
> + create_elf_header(data, phnum, rproc);
> +
> + phdr = data + ehdr->e_phoff;
> + offset = ehdr->e_phoff + sizeof(*phdr) * ehdr->e_phnum;
> + list_for_each_entry(segment, &rproc->dump_segments, node) {
> + if (segment->dump) {
> + segment->dump(rproc, segment, data + offset);
> + } else {
> + ptr = rproc_da_to_va(rproc, segment->da, segment->size);
> + if (!ptr) {
> + dev_err(&rproc->dev,
> + "invalid coredump segment (%pad, %zu)\n",
> + &segment->da, segment->size);
> + memset(data + offset, 0xff, segment->size);
> + } else {
> + memcpy(data + offset, ptr, segment->size);
> + }
> + }
> + phdr++;
> + }
> +
> + dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL);
> +}
> +EXPORT_SYMBOL(rproc_default_coredump);
As Mathieu pointed out, there's no need to EXPORT_SYMBOL within
remoteproc.ko
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 493ef92..28b6af2 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -47,6 +47,9 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
> int rproc_init_sysfs(void);
> void rproc_exit_sysfs(void);
>
> +/* from remoteproc_coredump.c */
> +void rproc_default_coredump(struct rproc *rproc);
> +
> void rproc_free_vring(struct rproc_vring *rvring);
> int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>
> @@ -119,4 +122,11 @@ struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
> return NULL;
> }
>
> +static inline
> +void rproc_coredump(struct rproc *rproc)
We keep these inline functions to make the following of a ops function
pointer nicer looking, but this is just a function call. So please drop
this and let the core just call straight into the remoteproc_coredump.c.
Regards,
Bjorn
> +{
> + return rproc_default_coredump(rproc);
> +
> +}
> +
> #endif /* REMOTEPROC_INTERNAL_H */
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 27+ messages in thread