On Mon 11 May 17:41 PDT 2020, risha...@codeaurora.org wrote:
> On 2020-05-11 17:30, Bjorn Andersson wrote:
> > On Mon 11 May 17:11 PDT 2020, risha...@codeaurora.org wrote:
> > > On 2020-05-07 13:21, Bjorn Andersson wrote:
> > > > On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:
> > > > > diff --git a/drivers/remoteproc/remoteproc_coredump.c
> > > > > b/drivers/remoteproc/remoteproc_coredump.c
[..]
> > > > > +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t
> > > > > count,
> > > > > + void *data, size_t header_size)
> > > > > +{
> > > > > + void *device_mem;
> > > > > + size_t data_left, copy_size, bytes_left = count;
> > > > > + unsigned long addr;
> > > > > + struct rproc_coredump_state *dump_state = data;
> > > > > + struct rproc *rproc = dump_state->rproc;
> > > > > + void *elfcore = dump_state->header;
> > > > > +
> > > > > + /* Copy the header first */
> > > > > + if (offset < header_size) {
> > > > > + copy_size = header_size - offset;
> > > > > + copy_size = min(copy_size, bytes_left);
> > > > > +
> > > > > + memcpy(buffer, elfcore + offset, copy_size);
> > > > > + offset += copy_size;
> > > > > + bytes_left -= copy_size;
> > > > > + buffer += copy_size;
> > > > > + }
> > > >
> > > > Perhaps you can take inspiration from devcd_readv() here?
> > > >
> > > > > +
> > > > > + while (bytes_left) {
> > > > > + addr = resolve_addr(offset - header_size,
> > > > > + &rproc->dump_segments, &data_left);
> > > > > + /* EOF check */
> > > > > + if (data_left == 0) {
> > > >
> > > > Afaict data_left denotes the amount of data left in this particular
> > > > segment, rather than in the entire core.
> > > >
> > > Yes, but it only returns 0 when the final segment has been copied
> > > completely. Otherwise it gives data left to copy for every segment
> > > and moves to next segment once the current one is copied.
> >
> > You're right.
> >
> > > > I think you should start by making bytes_left the minimum of the core
> > > > size and @count and then have this loop as long as bytes_left, copying
> > > > data to the buffer either from header or an appropriate segment based on
> > > > the current offset.
> > > >
> > > That would require an extra function that calculates entire core size,
> > > as its not available right now. Do you see any missed corner cases
> > > with this
> > > approach?
> >
> > You're looping over all the segments as you're building the header
> > anyways, so you could simply store this in the dump_state. I think this
> > depend more on the ability to reuse the read function between inline and
> > default coredump.
> >
> > Regards,
> > Bjorn
>
> Wouldn't the first if condition take care of "default" dump as it is?
> The header_size in that case would involve the 'header + all segments'.
Correct.
Regards,
Bjorn
