LKML Archive on lore.kernel.org
 help / Atom feed
From: Sibi Sankar <sibis@codeaurora.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
	ohad@wizery.com, kyan@codeaurora.org, sricharan@codeaurora.org,
	akdwived@codeaurora.org, linux-arm-msm@vger.kernel.org,
	tsoni@codeaurora.org
Subject: Re: [PATCH v3 4/6] remoteproc: qcom: q6v5-pil: Add custom dump function for modem
Date: Tue, 09 Oct 2018 21:49:44 +0530
Message-ID: <ab7ccaf06b92de38224202f3ca16749b@codeaurora.org> (raw)
In-Reply-To: <20181008064505.GO12063@builder>

Hi Bjorn,
Thanks for the review !

On 2018-10-08 12:15, Bjorn Andersson wrote:
> On Fri 27 Jul 08:20 PDT 2018, Sibi Sankar wrote:
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c 
>> b/drivers/remoteproc/qcom_q6v5_pil.c
> [..]
>> +static void qcom_q6v5_dump_segment(struct rproc *rproc, void *ptr, 
>> size_t len,
>> +								   void *priv)
>> +{
>> +	int ret = 0;
>> +	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>> +	static u32 pending_mask;
> 
> I dislike that this is a static variable. And it tracks the segments
> that has already been dumped, i.e. the !pending.
> 
>> +
>> +	/* Unlock mba before copying segments */
>> +	if (!qproc->mba_loaded)
>> +		ret = q6v5_mba_load(qproc);
>> +
>> +	if (!ptr || ret)
>> +		memset(priv, 0xff, len);
>> +	else
>> +		memcpy(priv, ptr, len);
>> +
>> +	pending_mask++;
> 
> This is a "count" and not a "mask".
> 

will rename it to count in the next re-spin. We can implement this as
a mask as well, the only disadvantage I see in that case is the need
for another flag to determine if mba is loaded since the mask for the
first segment may not be zero (the first segment may not be valid).

> I can see a few different cases where one would like to be able to pass
> custom data/information from the segment-registration to the dump
> function. So how about adding a "void *priv" to the dump segment.
> 
> For this particular case we could typecast segment->priv to an unsigned
> long (as this is always the same size) and use that as a bitmask, which
> we use to update pending_mask.
> 

sure will do the same

>> +	if (pending_mask == qproc->valid_mask) {
>> +		if (qproc->mba_loaded)
>> +			q6v5_mba_reclaim(qproc);
>> +		pending_mask = 0;
>> +	}
> 
> I think it would be cleaner to reset pending_mask in the start 
> function,
> and then return early in this function when we have dumped all the
> segments.
> 
> If so can pending_mask == 0 and pending_mask == all be the triggers for
> loading and reclaiming the mba? So we don't have two different trackers
> for this?
> 

with the private data stored per dump segment this becomes much simpler 
:)

>> +}
>> +
> 
> Regards,
> Bjorn

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-27 15:19 [PATCH v3 0/6] Add coredump support for Q6v5 Modem remoteproc Sibi Sankar
2018-07-27 15:19 ` [PATCH v3 1/6] remoteproc: Introduce custom dump function for each remoteproc segment Sibi Sankar
2018-08-07  6:15   ` Vinod
2018-08-08 14:10     ` Sibi Sankar
2018-10-08  6:23   ` Bjorn Andersson
2018-10-09 16:08     ` Sibi Sankar
2018-07-27 15:19 ` [PATCH v3 2/6] remoteproc: Add mechanism for custom dump function assignment Sibi Sankar
2018-07-27 15:20 ` [PATCH v3 3/6] remoteproc: qcom: q6v5-pil: Refactor mba load/unload sequence Sibi Sankar
2018-07-27 15:20 ` [PATCH v3 4/6] remoteproc: qcom: q6v5-pil: Add custom dump function for modem Sibi Sankar
2018-10-08  6:45   ` Bjorn Andersson
2018-10-09 16:19     ` Sibi Sankar [this message]
2018-07-27 15:20 ` [PATCH v3 5/6] remoteproc: qcom: q6v5-pil: Register segments/dumpfn for coredump Sibi Sankar
2018-10-08  6:48   ` Bjorn Andersson
2018-10-09 16:21     ` Sibi Sankar
2018-07-27 15:20 ` [PATCH v3 6/6] remoteproc: qcom: q6v5-pil: Assign the relocated address Sibi Sankar
2018-10-08  6:55   ` Bjorn Andersson

Reply instructions:

You may reply publically 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=ab7ccaf06b92de38224202f3ca16749b@codeaurora.org \
    --to=sibis@codeaurora.org \
    --cc=akdwived@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=kyan@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=ohad@wizery.com \
    --cc=sricharan@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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox