> > + 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
