linux-kernel - Re: [PATCH] remoteproc: qcom: pas: Add decrypt shutdown support 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: <Yr35Q2G8NNvYaI8M@builder.lan>
Date:   Thu, 30 Jun 2022 14:28:03 -0500
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Sibi Sankar <quic_sibis@...cinc.com>
Cc:     dmitry.baryshkov@...aro.org, agross@...nel.org,
        mathieu.poirier@...aro.org, linux-arm-msm@...r.kernel.org,
        linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] remoteproc: qcom: pas: Add decrypt shutdown support for
 modem

On Fri 20 May 02:28 CDT 2022, Sibi Sankar wrote:

> The initial shutdown request to modem on SM8450 SoCs would start the
> decryption process and will keep returning errors until the modem shutdown
> is complete. Fix this by retrying shutdowns in fixed intervals.
> 
> Err Logs on modem shutdown:
> qcom_q6v5_pas 4080000.remoteproc: failed to shutdown: -22
> remoteproc remoteproc3: can't stop rproc: -22
> 
> Fixes: 5cef9b48458d ("remoteproc: qcom: pas: Add SM8450 remoteproc support")
> Signed-off-by: Sibi Sankar <quic_sibis@...cinc.com>

Looks reasonable, just two inquiries below.

> ---
>  drivers/remoteproc/qcom_q6v5_pas.c | 67 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 6ae39c5653b1..d04c4b877e12 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -8,6 +8,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/firmware.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -29,6 +30,8 @@
>  #include "qcom_q6v5.h"
>  #include "remoteproc_internal.h"
>  
> +#define ADSP_DECRYPT_SHUTDOWN_DELAY_MS	100
> +
>  struct adsp_data {
>  	int crash_reason_smem;
>  	const char *firmware_name;
> @@ -36,6 +39,7 @@ struct adsp_data {
>  	unsigned int minidump_id;
>  	bool has_aggre2_clk;
>  	bool auto_boot;
> +	bool decrypt_shutdown;
>  
>  	char **proxy_pd_names;
>  
> @@ -65,6 +69,7 @@ struct qcom_adsp {
>  	unsigned int minidump_id;
>  	int crash_reason_smem;
>  	bool has_aggre2_clk;
> +	bool decrypt_shutdown;
>  	const char *info_name;
>  
>  	struct completion start_done;
> @@ -128,6 +133,20 @@ static void adsp_pds_disable(struct qcom_adsp *adsp, struct device **pds,
>  	}
>  }
>  
> +static int adsp_decrypt_shutdown(struct qcom_adsp *adsp)
> +{
> +	int retry_num = 50;

Seems unsigned to me.

> +	int ret = -EINVAL;
> +
> +	while (retry_num && ret) {
> +		msleep(ADSP_DECRYPT_SHUTDOWN_DELAY_MS);
> +		ret = qcom_scm_pas_shutdown(adsp->pas_id);
> +		retry_num--;
> +	}

Will qcom_scm_pas_shutdown() ever return any other errors than -EINVAL?

Would it make sense to make this:

	do {
		...;
	} while (ret == -EINVAL && --retry_num);

> +
> +	return ret;
> +}
> +
>  static int adsp_unprepare(struct rproc *rproc)
>  {
>  	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
> @@ -249,6 +268,9 @@ static int adsp_stop(struct rproc *rproc)
>  		dev_err(adsp->dev, "timed out on wait\n");
>  
>  	ret = qcom_scm_pas_shutdown(adsp->pas_id);
> +	if (ret && adsp->decrypt_shutdown)
> +		ret = adsp_decrypt_shutdown(adsp);
> +
>  	if (ret)
>  		dev_err(adsp->dev, "failed to shutdown: %d\n", ret);
>  
> @@ -459,6 +481,7 @@ static int adsp_probe(struct platform_device *pdev)
>  	adsp->pas_id = desc->pas_id;
>  	adsp->has_aggre2_clk = desc->has_aggre2_clk;
>  	adsp->info_name = desc->sysmon_name;
> +	adsp->decrypt_shutdown = desc->decrypt_shutdown;
>  	platform_set_drvdata(pdev, adsp);
>  
>  	device_wakeup_enable(adsp->dev);
> @@ -533,6 +556,7 @@ static const struct adsp_data adsp_resource_init = {
>  		.pas_id = 1,
>  		.has_aggre2_clk = false,
>  		.auto_boot = true,
> +		.decrypt_shutdown = false,

With all these booleans, I would prefer if we cleaned it up to not list
the disabled options. That would make it quicker to spot which features
are actually enabled for each remoteproc.

Regards,
Bjorn