From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: rishabhb@codeaurora.org
Cc: linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
ohad@wizery.com, mathieu.poirier@linaro.org,
tsoni@codeaurora.org, psodagud@codeaurora.org,
sidgup@codeaurora.org, linux-remoteproc-owner@vger.kernel.org
Subject: Re: [PATCH 2/3] remoteproc: Add inline coredump functionality
Date: Mon, 11 May 2020 22:13:21 -0700 [thread overview]
Message-ID: <20200512051321.GA16107@builder.lan> (raw)
In-Reply-To: <7396b8707d4cf38173f2d1b3968e7fc6@codeaurora.org>
On Mon 11 May 17:41 PDT 2020, rishabhb@codeaurora.org wrote:
> On 2020-05-11 17:30, Bjorn Andersson wrote:
> > On Mon 11 May 17:11 PDT 2020, rishabhb@codeaurora.org wrote:
> > > On 2020-05-07 13:21, Bjorn Andersson wrote:
> > > > On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:
> > > > > diff --git a/drivers/remoteproc/remoteproc_coredump.c
> > > > > b/drivers/remoteproc/remoteproc_coredump.c
[..]
> > > > > +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t
> > > > > count,
> > > > > + void *data, size_t header_size)
> > > > > +{
> > > > > + void *device_mem;
> > > > > + size_t data_left, copy_size, bytes_left = count;
> > > > > + unsigned long addr;
> > > > > + struct rproc_coredump_state *dump_state = data;
> > > > > + struct rproc *rproc = dump_state->rproc;
> > > > > + void *elfcore = dump_state->header;
> > > > > +
> > > > > + /* Copy the header first */
> > > > > + if (offset < header_size) {
> > > > > + copy_size = header_size - offset;
> > > > > + copy_size = min(copy_size, bytes_left);
> > > > > +
> > > > > + memcpy(buffer, elfcore + offset, copy_size);
> > > > > + offset += copy_size;
> > > > > + bytes_left -= copy_size;
> > > > > + buffer += copy_size;
> > > > > + }
> > > >
> > > > Perhaps you can take inspiration from devcd_readv() here?
> > > >
> > > > > +
> > > > > + while (bytes_left) {
> > > > > + addr = resolve_addr(offset - header_size,
> > > > > + &rproc->dump_segments, &data_left);
> > > > > + /* EOF check */
> > > > > + if (data_left == 0) {
> > > >
> > > > Afaict data_left denotes the amount of data left in this particular
> > > > segment, rather than in the entire core.
> > > >
> > > Yes, but it only returns 0 when the final segment has been copied
> > > completely. Otherwise it gives data left to copy for every segment
> > > and moves to next segment once the current one is copied.
> >
> > You're right.
> >
> > > > I think you should start by making bytes_left the minimum of the core
> > > > size and @count and then have this loop as long as bytes_left, copying
> > > > data to the buffer either from header or an appropriate segment based on
> > > > the current offset.
> > > >
> > > That would require an extra function that calculates entire core size,
> > > as its not available right now. Do you see any missed corner cases
> > > with this
> > > approach?
> >
> > You're looping over all the segments as you're building the header
> > anyways, so you could simply store this in the dump_state. I think this
> > depend more on the ability to reuse the read function between inline and
> > default coredump.
> >
> > Regards,
> > Bjorn
>
> Wouldn't the first if condition take care of "default" dump as it is?
> The header_size in that case would involve the 'header + all segments'.
Correct.
Regards,
Bjorn
next prev parent reply other threads:[~2020-05-12 5:14 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-16 18:38 [PATCH 1/3] remoteproc: Make coredump functionality configurable Rishabh Bhatnagar
2020-04-16 18:38 ` [PATCH 2/3] remoteproc: Add inline coredump functionality Rishabh Bhatnagar
2020-04-17 7:52 ` Loic PALLARDY
2020-04-17 17:11 ` Bjorn Andersson
2020-04-20 6:01 ` kbuild test robot
2020-04-23 20:38 ` Mathieu Poirier
2020-05-07 20:21 ` Bjorn Andersson
2020-05-12 0:11 ` rishabhb
2020-05-12 0:30 ` Bjorn Andersson
2020-05-12 0:41 ` rishabhb
2020-05-12 5:13 ` Bjorn Andersson [this message]
2020-05-13 18:05 ` Mathieu Poirier
2020-05-14 18:07 ` rishabhb
2020-04-16 18:38 ` [PATCH 3/3] remoteproc: Add coredump sysfs attribute Rishabh Bhatnagar
2020-04-17 7:54 ` Loic PALLARDY
2020-04-17 17:48 ` rishabhb
2020-04-23 20:47 ` Mathieu Poirier
2020-04-23 18:07 ` [PATCH 1/3] remoteproc: Make coredump functionality configurable Mathieu Poirier
2020-05-07 19:33 ` Bjorn Andersson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200512051321.GA16107@builder.lan \
--to=bjorn.andersson@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc-owner@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=mathieu.poirier@linaro.org \
--cc=ohad@wizery.com \
--cc=psodagud@codeaurora.org \
--cc=rishabhb@codeaurora.org \
--cc=sidgup@codeaurora.org \
--cc=tsoni@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).