Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 11 May 2020 22:13:21 -0700
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
Message-ID: <20200512051321.GA16107@builder.lan>
References: <1587062312-4939-1-git-send-email-rishabhb@codeaurora.org>
 <1587062312-4939-2-git-send-email-rishabhb@codeaurora.org>
 <20200507202121.GK2329931@builder.lan>
 <7deb97ab40dd36d5a51111147cf4c14e@codeaurora.org>
 <20200512003028.GA2165@builder.lan>
 <7396b8707d4cf38173f2d1b3968e7fc6@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7396b8707d4cf38173f2d1b3968e7fc6@codeaurora.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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