[2/3] remoteproc: Add inline coredump functionality

Commit Message

Rishabh Bhatnagar April 16, 2020, 6:38 p.m. UTC

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 <[e-mail]>
---
 drivers/remoteproc/remoteproc_coredump.c | 130 +++++++++++++++++++++++++++++++
 drivers/remoteproc/remoteproc_internal.h |  23 +++++-
 include/linux/remoteproc.h               |   2 +
 3 files changed, 153 insertions(+), 2 deletions(-)

Comments

Loic PALLARDY April 17, 2020, 7:52 a.m. UTC | #1

Hi Rishabh,

> -----Original Message-----
> From: [e-mail] <linux-remoteproc-
> [e-mail]> On Behalf Of Rishabh Bhatnagar
> Sent: jeudi 16 avril 2020 20:39
> To: [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Rishabh Bhatnagar
> <[e-mail]>
> 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 <[e-mail]>
> ---
>  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

Bjorn Andersson April 17, 2020, 5:11 p.m. UTC | #2

On Fri 17 Apr 00:52 PDT 2020, Loic PALLARDY wrote:

> Hi Rishabh,
> 
> > -----Original Message-----
> > From: [e-mail] <linux-remoteproc-
> > [e-mail]> On Behalf Of Rishabh Bhatnagar
> > Sent: jeudi 16 avril 2020 20:39
> > To: [e-mail]; [e-mail]
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; Rishabh Bhatnagar
> > <[e-mail]>
> > 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 <[e-mail]>
> > ---
> >  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

kernel test robot April 20, 2020, 6:01 a.m. UTC | #3

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 <[e-mail]>

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/[e-mail]

Mathieu Poirier April 23, 2020, 8:38 p.m. UTC | #4

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 <[e-mail]>
> ---
>  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

Bjorn Andersson May 7, 2020, 8:21 p.m. UTC | #5

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 <[e-mail]>
> ---
>  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

Rishabh Bhatnagar May 12, 2020, 12:11 a.m. UTC | #6

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 <[e-mail]>
>> ---
>>  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

Bjorn Andersson May 12, 2020, 12:30 a.m. UTC | #7

On Mon 11 May 17:11 PDT 2020, [e-mail] 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 <[e-mail]>
> > > ---
> > >  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

Rishabh Bhatnagar May 12, 2020, 12:41 a.m. UTC | #8

On 2020-05-11 17:30, Bjorn Andersson wrote:
> On Mon 11 May 17:11 PDT 2020, [e-mail] 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 <[e-mail]>
>> > > ---
>> > >  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

Bjorn Andersson May 12, 2020, 5:13 a.m. UTC | #9

On Mon 11 May 17:41 PDT 2020, [e-mail] wrote:

> On 2020-05-11 17:30, Bjorn Andersson wrote:
> > On Mon 11 May 17:11 PDT 2020, [e-mail] 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

Mathieu Poirier May 13, 2020, 6:05 p.m. UTC | #10

On Mon, 11 May 2020 at 18:32, Bjorn Andersson
<[e-mail]> wrote:
>
> On Mon 11 May 17:11 PDT 2020, [e-mail] 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 <[e-mail]>
> > > > ---
> > > >  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

Rishabh Bhatnagar May 14, 2020, 6:07 p.m. UTC | #11

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 <[e-mail]>
>> ---
>>  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

Patch

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;