linux-kernel - Re: [PATCH v3 4/6] remoteproc: qcom: q6v5-pil: Add custom dump function for modem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181008064505.GO12063@builder>
Date:   Sun, 7 Oct 2018 23:45:05 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Sibi Sankar <sibis@...eaurora.org>
Cc:     linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org,
        ohad@...ery.com, kyan@...eaurora.org, sricharan@...eaurora.org,
        akdwived@...eaurora.org, linux-arm-msm@...r.kernel.org,
        tsoni@...eaurora.org
Subject: Re: [PATCH v3 4/6] remoteproc: qcom: q6v5-pil: Add custom dump
 function for modem

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".

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.

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

> +}
> +

Regards,
Bjorn