Re: [PATCH v8 4/5] remoteproc: Add inline coredump functionality

2020-07-20 Thread Mathieu Poirier
On Thu, Jul 16, 2020 at 03:20:34PM -0700, Rishabh Bhatnagar wrote:
> The current coredump implementation uses vmalloc area to copy
> all the segments. But this might put strain on low memory targets
> as the firmware size sometimes is in tens of MBs. The situation
> becomes worse if there are multiple remote processors undergoing
> recovery at the same time. This patch adds inline coredump
> functionality that 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 
> Tested-by: Sibi Sankar 

Thanks for doing the modifications.

Reviewed-by: Mathieu Poirier 

> ---
>  drivers/remoteproc/remoteproc_coredump.c | 156 
> +++
>  include/linux/remoteproc.h   |  16 
>  2 files changed, 154 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_coredump.c 
> b/drivers/remoteproc/remoteproc_coredump.c
> index 390f563..bb15a29 100644
> --- a/drivers/remoteproc/remoteproc_coredump.c
> +++ b/drivers/remoteproc/remoteproc_coredump.c
> @@ -5,6 +5,7 @@
>   * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -12,6 +13,12 @@
>  #include "remoteproc_internal.h"
>  #include "remoteproc_elf_helpers.h"
>  
> +struct rproc_coredump_state {
> + struct rproc *rproc;
> + void *header;
> + struct completion dump_done;
> +};
> +
>  /**
>   * rproc_coredump_cleanup() - clean up dump_segments list
>   * @rproc: the remote processor handle
> @@ -115,12 +122,110 @@ int rproc_coredump_set_elf_info(struct rproc *rproc, 
> u8 class, u16 machine)
>  }
>  EXPORT_SYMBOL(rproc_coredump_set_elf_info);
>  
> +static void rproc_coredump_free(void *data)
> +{
> + struct rproc_coredump_state *dump_state = data;
> +
> + vfree(dump_state->header);
> + complete(&dump_state->dump_done);
> +}
> +
> +static void *rproc_coredump_find_segment(loff_t user_offset,
> +  struct list_head *segments,
> +  size_t *data_left)
> +{
> + struct rproc_dump_segment *segment;
> +
> + list_for_each_entry(segment, segments, node) {
> + if (user_offset < segment->size) {
> + *data_left = segment->size - user_offset;
> + return segment;
> + }
> + user_offset -= segment->size;
> + }
> +
> + *data_left = 0;
> + return NULL;
> +}
> +
> +static void rproc_copy_segment(struct rproc *rproc, void *dest,
> +struct rproc_dump_segment *segment,
> +size_t offset, size_t size)
> +{
> + void *ptr;
> +
> + if (segment->dump) {
> + segment->dump(rproc, segment, dest, offset, size);
> + } else {
> + ptr = rproc_da_to_va(rproc, segment->da + offset, size);
> + if (!ptr) {
> + dev_err(&rproc->dev,
> + "invalid copy request for segment %pad with 
> offset %zu and size %zu)\n",
> + &segment->da, offset, size);
> + memset(dest, 0xff, size);
> + } else {
> + memcpy(dest, ptr, size);
> + }
> + }
> +}
> +
> +static ssize_t rproc_coredump_read(char *buffer, loff_t offset, size_t count,
> +void *data, size_t header_sz)
> +{
> + size_t seg_data, bytes_left = count;
> + ssize_t copy_sz;
> + struct rproc_dump_segment *seg;
> + struct rproc_coredump_state *dump_state = data;
> + struct rproc *rproc = dump_state->rproc;
> + void *elfcore = dump_state->header;
> +
> + /* Copy the vmalloc'ed header first. */
> + if (offset < header_sz) {
> + copy_sz = memory_read_from_buffer(buffer, count, &offset,
> +   elfcore, header_sz);
> +
> + return copy_sz;
> + }
> +
> + /*
> +  * Find out the segment memory chunk to be copied based on offset.
> +  * Keep copying data until count bytes are read.
> +  */
> + while (bytes_left) {
> + seg = rproc_coredump_find_segment(offset - header_sz,
> +   &rproc->dump_segments,
> +   &seg_data);
> + /* EOF check */
> + if (!seg) {
> + dev_info(&rproc->dev, "Ramdump done, %lld bytes read",
> +  offset);
> + break;
> + }
> +
> + copy_sz = min_t(size_t, bytes_left, seg_data);
> +
> + rproc_copy_segment(rproc, buffer, seg, seg->size - seg_data,
> +copy_sz);
> +
> + offset += copy_sz;
> + buffer += copy_sz;
> + bytes_left -= copy_sz;
> + }
> +
> + 

Re: [PATCH v8 4/5] remoteproc: Add inline coredump functionality

2020-07-17 Thread Sibi Sankar

On 2020-07-17 03:50, Rishabh Bhatnagar wrote:

The current coredump implementation uses vmalloc area to copy
all the segments. But this might put strain on low memory targets
as the firmware size sometimes is in tens of MBs. The situation
becomes worse if there are multiple remote processors undergoing
recovery at the same time. This patch adds inline coredump
functionality that 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 


Reviewed-by: Sibi Sankar 


Tested-by: Sibi Sankar 
---
 drivers/remoteproc/remoteproc_coredump.c | 156 
+++

 include/linux/remoteproc.h   |  16 
 2 files changed, 154 insertions(+), 18 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_coredump.c
b/drivers/remoteproc/remoteproc_coredump.c
index 390f563..bb15a29 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2020, The Linux Foundation. All rights reserved.
  */

+#include 
 #include 
 #include 
 #include 
@@ -12,6 +13,12 @@
 #include "remoteproc_internal.h"
 #include "remoteproc_elf_helpers.h"

+struct rproc_coredump_state {
+   struct rproc *rproc;
+   void *header;
+   struct completion dump_done;
+};
+
 /**
  * rproc_coredump_cleanup() - clean up dump_segments list
  * @rproc: the remote processor handle
@@ -115,12 +122,110 @@ int rproc_coredump_set_elf_info(struct rproc
*rproc, u8 class, u16 machine)
 }
 EXPORT_SYMBOL(rproc_coredump_set_elf_info);

+static void rproc_coredump_free(void *data)
+{
+   struct rproc_coredump_state *dump_state = data;
+
+   vfree(dump_state->header);
+   complete(&dump_state->dump_done);
+}
+
+static void *rproc_coredump_find_segment(loff_t user_offset,
+struct list_head *segments,
+size_t *data_left)
+{
+   struct rproc_dump_segment *segment;
+
+   list_for_each_entry(segment, segments, node) {
+   if (user_offset < segment->size) {
+   *data_left = segment->size - user_offset;
+   return segment;
+   }
+   user_offset -= segment->size;
+   }
+
+   *data_left = 0;
+   return NULL;
+}
+
+static void rproc_copy_segment(struct rproc *rproc, void *dest,
+  struct rproc_dump_segment *segment,
+  size_t offset, size_t size)
+{
+   void *ptr;
+
+   if (segment->dump) {
+   segment->dump(rproc, segment, dest, offset, size);
+   } else {
+   ptr = rproc_da_to_va(rproc, segment->da + offset, size);
+   if (!ptr) {
+   dev_err(&rproc->dev,
+"invalid copy request for segment %pad with offset %zu and size 
%zu)\n",

+   &segment->da, offset, size);
+   memset(dest, 0xff, size);
+   } else {
+   memcpy(dest, ptr, size);
+   }
+   }
+}
+
+static ssize_t rproc_coredump_read(char *buffer, loff_t offset, size_t 
count,

+  void *data, size_t header_sz)
+{
+   size_t seg_data, bytes_left = count;
+   ssize_t copy_sz;
+   struct rproc_dump_segment *seg;
+   struct rproc_coredump_state *dump_state = data;
+   struct rproc *rproc = dump_state->rproc;
+   void *elfcore = dump_state->header;
+
+   /* Copy the vmalloc'ed header first. */
+   if (offset < header_sz) {
+   copy_sz = memory_read_from_buffer(buffer, count, &offset,
+ elfcore, header_sz);
+
+   return copy_sz;
+   }
+
+   /*
+* Find out the segment memory chunk to be copied based on offset.
+* Keep copying data until count bytes are read.
+*/
+   while (bytes_left) {
+   seg = rproc_coredump_find_segment(offset - header_sz,
+ &rproc->dump_segments,
+ &seg_data);
+   /* EOF check */
+   if (!seg) {
+   dev_info(&rproc->dev, "Ramdump done, %lld bytes read",
+offset);
+   break;
+   }
+
+   copy_sz = min_t(size_t, bytes_left, seg_data);
+
+   rproc_copy_segment(rproc, buffer, seg, seg->size - seg_data,
+  copy_sz);
+
+   offset += copy_sz;
+   buffer += copy_sz;
+   bytes_left -= copy_sz;
+   }
+
+   return count - bytes_left;
+}
+
 /**
  * rproc_coredump() - perform coredump
  * @rproc: rproc handle
  *
  * This function will generate an ELF header for the registered 
segments

- * and create a devcoredump device associ

Re: [PATCH v8 4/5] remoteproc: Add inline coredump functionality

2020-07-16 Thread Bjorn Andersson
On Thu 16 Jul 15:20 PDT 2020, Rishabh Bhatnagar wrote:

> The current coredump implementation uses vmalloc area to copy
> all the segments. But this might put strain on low memory targets
> as the firmware size sometimes is in tens of MBs. The situation
> becomes worse if there are multiple remote processors undergoing
> recovery at the same time. This patch adds inline coredump
> functionality that 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 
> Tested-by: Sibi Sankar 

Reviewed-by: Bjorn Andersson 

> ---
>  drivers/remoteproc/remoteproc_coredump.c | 156 
> +++
>  include/linux/remoteproc.h   |  16 
>  2 files changed, 154 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_coredump.c 
> b/drivers/remoteproc/remoteproc_coredump.c
> index 390f563..bb15a29 100644
> --- a/drivers/remoteproc/remoteproc_coredump.c
> +++ b/drivers/remoteproc/remoteproc_coredump.c
> @@ -5,6 +5,7 @@
>   * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -12,6 +13,12 @@
>  #include "remoteproc_internal.h"
>  #include "remoteproc_elf_helpers.h"
>  
> +struct rproc_coredump_state {
> + struct rproc *rproc;
> + void *header;
> + struct completion dump_done;
> +};
> +
>  /**
>   * rproc_coredump_cleanup() - clean up dump_segments list
>   * @rproc: the remote processor handle
> @@ -115,12 +122,110 @@ int rproc_coredump_set_elf_info(struct rproc *rproc, 
> u8 class, u16 machine)
>  }
>  EXPORT_SYMBOL(rproc_coredump_set_elf_info);
>  
> +static void rproc_coredump_free(void *data)
> +{
> + struct rproc_coredump_state *dump_state = data;
> +
> + vfree(dump_state->header);
> + complete(&dump_state->dump_done);
> +}
> +
> +static void *rproc_coredump_find_segment(loff_t user_offset,
> +  struct list_head *segments,
> +  size_t *data_left)
> +{
> + struct rproc_dump_segment *segment;
> +
> + list_for_each_entry(segment, segments, node) {
> + if (user_offset < segment->size) {
> + *data_left = segment->size - user_offset;
> + return segment;
> + }
> + user_offset -= segment->size;
> + }
> +
> + *data_left = 0;
> + return NULL;
> +}
> +
> +static void rproc_copy_segment(struct rproc *rproc, void *dest,
> +struct rproc_dump_segment *segment,
> +size_t offset, size_t size)
> +{
> + void *ptr;
> +
> + if (segment->dump) {
> + segment->dump(rproc, segment, dest, offset, size);
> + } else {
> + ptr = rproc_da_to_va(rproc, segment->da + offset, size);
> + if (!ptr) {
> + dev_err(&rproc->dev,
> + "invalid copy request for segment %pad with 
> offset %zu and size %zu)\n",
> + &segment->da, offset, size);
> + memset(dest, 0xff, size);
> + } else {
> + memcpy(dest, ptr, size);
> + }
> + }
> +}
> +
> +static ssize_t rproc_coredump_read(char *buffer, loff_t offset, size_t count,
> +void *data, size_t header_sz)
> +{
> + size_t seg_data, bytes_left = count;
> + ssize_t copy_sz;
> + struct rproc_dump_segment *seg;
> + struct rproc_coredump_state *dump_state = data;
> + struct rproc *rproc = dump_state->rproc;
> + void *elfcore = dump_state->header;
> +
> + /* Copy the vmalloc'ed header first. */
> + if (offset < header_sz) {
> + copy_sz = memory_read_from_buffer(buffer, count, &offset,
> +   elfcore, header_sz);
> +
> + return copy_sz;
> + }
> +
> + /*
> +  * Find out the segment memory chunk to be copied based on offset.
> +  * Keep copying data until count bytes are read.
> +  */
> + while (bytes_left) {
> + seg = rproc_coredump_find_segment(offset - header_sz,
> +   &rproc->dump_segments,
> +   &seg_data);
> + /* EOF check */
> + if (!seg) {
> + dev_info(&rproc->dev, "Ramdump done, %lld bytes read",
> +  offset);
> + break;
> + }
> +
> + copy_sz = min_t(size_t, bytes_left, seg_data);
> +
> + rproc_copy_segment(rproc, buffer, seg, seg->size - seg_data,
> +copy_sz);
> +
> + offset += copy_sz;
> + buffer += copy_sz;
> + bytes_left -= copy_sz;
> + }
> +
> + return count - bytes_left;
> +}
> +
>  /**
>   *

[PATCH v8 4/5] remoteproc: Add inline coredump functionality

2020-07-16 Thread Rishabh Bhatnagar
The current coredump implementation uses vmalloc area to copy
all the segments. But this might put strain on low memory targets
as the firmware size sometimes is in tens of MBs. The situation
becomes worse if there are multiple remote processors undergoing
recovery at the same time. This patch adds inline coredump
functionality that 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 
Tested-by: Sibi Sankar 
---
 drivers/remoteproc/remoteproc_coredump.c | 156 +++
 include/linux/remoteproc.h   |  16 
 2 files changed, 154 insertions(+), 18 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_coredump.c 
b/drivers/remoteproc/remoteproc_coredump.c
index 390f563..bb15a29 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2020, The Linux Foundation. All rights reserved.
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -12,6 +13,12 @@
 #include "remoteproc_internal.h"
 #include "remoteproc_elf_helpers.h"
 
+struct rproc_coredump_state {
+   struct rproc *rproc;
+   void *header;
+   struct completion dump_done;
+};
+
 /**
  * rproc_coredump_cleanup() - clean up dump_segments list
  * @rproc: the remote processor handle
@@ -115,12 +122,110 @@ int rproc_coredump_set_elf_info(struct rproc *rproc, u8 
class, u16 machine)
 }
 EXPORT_SYMBOL(rproc_coredump_set_elf_info);
 
+static void rproc_coredump_free(void *data)
+{
+   struct rproc_coredump_state *dump_state = data;
+
+   vfree(dump_state->header);
+   complete(&dump_state->dump_done);
+}
+
+static void *rproc_coredump_find_segment(loff_t user_offset,
+struct list_head *segments,
+size_t *data_left)
+{
+   struct rproc_dump_segment *segment;
+
+   list_for_each_entry(segment, segments, node) {
+   if (user_offset < segment->size) {
+   *data_left = segment->size - user_offset;
+   return segment;
+   }
+   user_offset -= segment->size;
+   }
+
+   *data_left = 0;
+   return NULL;
+}
+
+static void rproc_copy_segment(struct rproc *rproc, void *dest,
+  struct rproc_dump_segment *segment,
+  size_t offset, size_t size)
+{
+   void *ptr;
+
+   if (segment->dump) {
+   segment->dump(rproc, segment, dest, offset, size);
+   } else {
+   ptr = rproc_da_to_va(rproc, segment->da + offset, size);
+   if (!ptr) {
+   dev_err(&rproc->dev,
+   "invalid copy request for segment %pad with 
offset %zu and size %zu)\n",
+   &segment->da, offset, size);
+   memset(dest, 0xff, size);
+   } else {
+   memcpy(dest, ptr, size);
+   }
+   }
+}
+
+static ssize_t rproc_coredump_read(char *buffer, loff_t offset, size_t count,
+  void *data, size_t header_sz)
+{
+   size_t seg_data, bytes_left = count;
+   ssize_t copy_sz;
+   struct rproc_dump_segment *seg;
+   struct rproc_coredump_state *dump_state = data;
+   struct rproc *rproc = dump_state->rproc;
+   void *elfcore = dump_state->header;
+
+   /* Copy the vmalloc'ed header first. */
+   if (offset < header_sz) {
+   copy_sz = memory_read_from_buffer(buffer, count, &offset,
+ elfcore, header_sz);
+
+   return copy_sz;
+   }
+
+   /*
+* Find out the segment memory chunk to be copied based on offset.
+* Keep copying data until count bytes are read.
+*/
+   while (bytes_left) {
+   seg = rproc_coredump_find_segment(offset - header_sz,
+ &rproc->dump_segments,
+ &seg_data);
+   /* EOF check */
+   if (!seg) {
+   dev_info(&rproc->dev, "Ramdump done, %lld bytes read",
+offset);
+   break;
+   }
+
+   copy_sz = min_t(size_t, bytes_left, seg_data);
+
+   rproc_copy_segment(rproc, buffer, seg, seg->size - seg_data,
+  copy_sz);
+
+   offset += copy_sz;
+   buffer += copy_sz;
+   bytes_left -= copy_sz;
+   }
+
+   return count - bytes_left;
+}
+
 /**
  * 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.
+ * and create a devcoredump device