linux-kernel - Re: [PATCH V6 5/5] remoteproc: qcom: Add q6v5-wcss rproc ops

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: <20180518122946.GC31996@vkoul-mobl>
Date:   Fri, 18 May 2018 17:59:46 +0530
From:   Vinod <vkoul@...nel.org>
To:     Sricharan R <sricharan@...eaurora.org>
Cc:     bjorn.andersson@...aro.org, ohad@...ery.com, robh+dt@...nel.org,
        mark.rutland@....com, andy.gross@...aro.org,
        david.brown@...aro.org, linux-remoteproc@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        sibis@...eaurora.org
Subject: Re: [PATCH V6 5/5] remoteproc: qcom: Add q6v5-wcss rproc ops

On 14-05-18, 16:16, Sricharan R wrote:

> +static int q6v5_wcss_start(struct rproc *rproc)
> +{
> +	struct q6v5 *qproc = rproc->priv;
> +	int ret = 0;

Superfluous initialization

> +
> +	ret = q6v5_clk_enable(qproc->dev, qproc->active_clks,
> +			      qproc->active_clk_count);
> +	if (ret) {
> +		dev_err(qproc->dev, "failed to enable clocks\n");
> +		return ret;
> +	}
> +
> +	/* Release Q6 and WCSS reset */
> +	ret = reset_control_deassert(qproc->wcss_reset);
> +	if (ret)
> +		dev_err(qproc->dev, "wcss_reset failed\n");
> +
> +	ret = reset_control_deassert(qproc->wcss_q6_reset);
> +	if (ret)
> +		dev_err(qproc->dev, "wcss_q6_reset failed\n");

shouldn't we abort on these two errors?

> +
> +	/* Lithium configuration - clock gating and bus arbitration */
> +	ret = regmap_update_bits(qproc->halt_map,
> +				 qproc->halt_nc + TCSR_GLOBAL_CFG0,
> +				 0x1F, 0x14);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(qproc->halt_map,
> +				 qproc->halt_nc + TCSR_GLOBAL_CFG1,
> +				 1, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Write bootaddr to EVB so that Q6WCSS will jump there after reset */
> +	writel(rproc->bootaddr >> 4, qproc->reg_base + QDSP6SS_RST_EVB);
> +
> +	ret = q6v5_reset(qproc);
> +	if (ret)
> +		return ret;

all these returns, aren't we leaving device in some dangling state?

> +static int q6v5_wcss_powerdown(struct q6v5 *qproc)
> +{
> +	unsigned int val = 0;

superfluous initialization

> +	int ret;
> +
> +	/* 1 - Assert WCSS/Q6 HALTREQ */
> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
> +
> +	/* 2 - Enable WCSSAON_CONFIG */
> +	val = readl(qproc->rmb_base + SSCAON_CONFIG);
> +	val |= SSCAON_ENABLE;
> +	writel(val, qproc->rmb_base + SSCAON_CONFIG);
> +
> +	/* 3 - Set SSCAON_CONFIG */
> +	val |= BIT(15);
> +	val &= ~BIT(16);
> +	val &= ~BIT(17);
> +	val &= ~BIT(18);

shouldn't bit 15 thru 18 be defined on what they mean?

> +static int q6v5_q6_powerdown(struct q6v5 *qproc)
> +{
> +	int i = 0, ret;
> +	unsigned int val = 0;
> +
> +	/* 1 - Halt Q6 bus interface */
> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
> +
> +	/* 2 - Disable Q6 Core clock */
> +	val = readl(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
> +	val &= ~Q6SS_CLK_ENABLE;
> +	writel(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
> +
> +	/* 3 - Clamp I/O */
> +	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	val |= Q6SS_CLAMP_IO;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* 4 - Clamp WL */
> +	val |= QDSS_BHS_ON;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* 5 - Clear Erase standby */
> +	val &= ~Q6SS_L2DATA_STBY_N;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* 6 - Clear Sleep RTN */
> +	val &= ~Q6SS_SLP_RET_N;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* 7 - turn off QDSP6 memory foot/head switch one bank at a time */
> +	for (i = 0; i < 20; i++) {
> +		val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
> +		val &= ~BIT(i);
> +		writel(val, qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
> +		mdelay(1);
> +	}
> +	/* 8 - Assert QMC memory RTN */
> +	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	val |= QDSP6v56_CLAMP_QMC_MEM;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* 9 - Turn off BHS */
> +	val &= ~QDSP6v56_BHS_ON;
> +	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	udelay(1);
> +	/* 10 - Wait till BHS Reset is done */

would help in readability if you can be consistent and add empty lines after each
step as done for rest of the routine

> +static int q6v5_wcss_stop(struct rproc *rproc)
> +{
> +	struct q6v5 *qproc = rproc->priv;
> +	int ret = 0;

this one too, I think if you run with sparse, it should warn you about these

> +
> +	qproc->running = false;
> +
> +	/* WCSS powerdown */
> +	qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit),
> +				    BIT(qproc->stop_bit));
> +
> +	ret = wait_for_completion_timeout(&qproc->stop_done,
> +					  msecs_to_jiffies(5000));
> +	if (ret == 0) {
> +		dev_err(qproc->dev, "timed out on wait\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit), 0);
> +
> +	ret = q6v5_wcss_powerdown(qproc);
> +	if (ret)
> +		return ret;
> +
> +	/* Q6 Power down */
> +	ret = q6v5_q6_powerdown(qproc);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

this could be optimized to:
        return q6v5_q6_powerdown()
-- 
~Vinod