Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH] remoteproc: qcom: pas: Add decrypt shutdown support for
 modem
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
CC:     <dmitry.baryshkov@linaro.org>, <agross@kernel.org>,
        <mathieu.poirier@linaro.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <1653031684-14771-1-git-send-email-quic_sibis@quicinc.com>
 <Yr35Q2G8NNvYaI8M@builder.lan>
From:   Sibi Sankar <quic_sibis@quicinc.com>
Message-ID: <a4373b5d-fd01-9692-2660-792b5b84d08c@quicinc.com>
Date:   Tue, 5 Jul 2022 17:23:24 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <Yr35Q2G8NNvYaI8M@builder.lan>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hey Bjorn,
Thanks for taking time to review the series.

On 7/1/22 12:58 AM, Bjorn Andersson wrote:
> 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@quicinc.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.

ack

> 
>> +	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);
> 

Just checking on ret would cover the -EINVAL case as well but like you
said pas_shutdown() won't return any other error. So I'll just stick
with your suggestion.

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

ack and ack to the adsp_shutdown_poll_decrypt() as well.

> 
> Regards,
> Bjorn
>