diff mbox series

remoteproc: qcom: Add fallback mechanism for full coredump collection

Message ID 1649269662-20338-1-git-send-email-quic_ylal@quicinc.com (mailing list archive)
State Superseded
Headers show
Series remoteproc: qcom: Add fallback mechanism for full coredump collection | expand

Commit Message

Yogesh Lal April 6, 2022, 6:27 p.m. UTC
In case remoteproc's firmware missing minidump support, during crash
scenario coredump does not collected. This change adds a fallback
mechanism for full coredump collection in the event of a crash.

Signed-off-by: Yogesh Lal <quic_ylal@quicinc.com>
---
 drivers/remoteproc/qcom_common.c   | 11 ++++++++---
 drivers/remoteproc/qcom_q6v5_pas.c |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

Comments

Bjorn Andersson April 22, 2022, 3:22 p.m. UTC | #1
On Wed 06 Apr 13:27 CDT 2022, Yogesh Lal wrote:

> In case remoteproc's firmware missing minidump support, during crash
> scenario coredump does not collected. This change adds a fallback
> mechanism for full coredump collection in the event of a crash.
> 
> Signed-off-by: Yogesh Lal <quic_ylal@quicinc.com>
> ---
>  drivers/remoteproc/qcom_common.c   | 11 ++++++++---
>  drivers/remoteproc/qcom_q6v5_pas.c |  1 +
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> index 4b91e3c..68bd0bc 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -162,13 +162,18 @@ void qcom_minidump(struct rproc *rproc, unsigned int minidump_id)
>  	 * is initialized in memory and encryption status is set.
>  	 */
>  	if (subsystem->regions_baseptr == 0 ||
> -	    le32_to_cpu(subsystem->status) != 1 ||
> -	    le32_to_cpu(subsystem->enabled) != MD_SS_ENABLED ||
> -	    le32_to_cpu(subsystem->encryption_status) != MD_SS_ENCR_DONE) {
> +		le32_to_cpu(subsystem->status) != 1 ||
> +		le32_to_cpu(subsystem->enabled) != MD_SS_ENABLED) {
> +			return rproc_coredump(rproc);
> +		}
> +
> +	if (le32_to_cpu(subsystem->encryption_status) != MD_SS_ENCR_DONE) {
>  		dev_err(&rproc->dev, "Minidump not ready, skipping\n");
>  		return;
>  	}
>  
> +	rproc_coredump_cleanup(rproc);

The patch looks good, but could you please explain in the commit message
why this needs to be added? If the thing described in the message
happens this code path wouldn't be taken.

Should it be a separate patch, or is it needed because of the fallback
etc?

Thanks,
Bjorn

> +
>  	ret = qcom_add_minidump_segments(rproc, subsystem);
>  	if (ret) {
>  		dev_err(&rproc->dev, "Failed with error: %d while adding minidump entries\n", ret);
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 1ae47cc..40bf747 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -293,6 +293,7 @@ static const struct rproc_ops adsp_minidump_ops = {
>  	.start = adsp_start,
>  	.stop = adsp_stop,
>  	.da_to_va = adsp_da_to_va,
> +	.parse_fw = qcom_register_dump_segments,
>  	.load = adsp_load,
>  	.panic = adsp_panic,
>  	.coredump = adsp_minidump,
> -- 
> 2.7.4
>
Yogesh Lal April 26, 2022, 10:15 a.m. UTC | #2
On 4/22/2022 8:52 PM, Bjorn Andersson wrote:
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
>
> On Wed 06 Apr 13:27 CDT 2022, Yogesh Lal wrote:
>
>> In case remoteproc's firmware missing minidump support, during crash
>> scenario coredump does not collected. This change adds a fallback
>> mechanism for full coredump collection in the event of a crash.
>>
>> Signed-off-by: Yogesh Lal <quic_ylal@quicinc.com>
>> ---
>>   drivers/remoteproc/qcom_common.c   | 11 ++++++++---
>>   drivers/remoteproc/qcom_q6v5_pas.c |  1 +
>>   2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
>> index 4b91e3c..68bd0bc 100644
>> --- a/drivers/remoteproc/qcom_common.c
>> +++ b/drivers/remoteproc/qcom_common.c
>> @@ -162,13 +162,18 @@ void qcom_minidump(struct rproc *rproc, unsigned int minidump_id)
>>         * is initialized in memory and encryption status is set.
>>         */
>>        if (subsystem->regions_baseptr == 0 ||
>> -         le32_to_cpu(subsystem->status) != 1 ||
>> -         le32_to_cpu(subsystem->enabled) != MD_SS_ENABLED ||
>> -         le32_to_cpu(subsystem->encryption_status) != MD_SS_ENCR_DONE) {
>> +             le32_to_cpu(subsystem->status) != 1 ||
>> +             le32_to_cpu(subsystem->enabled) != MD_SS_ENABLED) {
>> +                     return rproc_coredump(rproc);
>> +             }
>> +
>> +     if (le32_to_cpu(subsystem->encryption_status) != MD_SS_ENCR_DONE) {
>>                dev_err(&rproc->dev, "Minidump not ready, skipping\n");
>>                return;
>>        }
>>
>> +     rproc_coredump_cleanup(rproc);
> The patch looks good, but could you please explain in the commit message
> why this needs to be added? If the thing described in the message
> happens this code path wouldn't be taken.
>
> Should it be a separate patch, or is it needed because of the fallback
> etc?
>
> Thanks,
> Bjorn

Will push separate patch for it.

Thanks

Yogesh Lal


>> +
>>        ret = qcom_add_minidump_segments(rproc, subsystem);
>>        if (ret) {
>>                dev_err(&rproc->dev, "Failed with error: %d while adding minidump entries\n", ret);
>> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
>> index 1ae47cc..40bf747 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pas.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
>> @@ -293,6 +293,7 @@ static const struct rproc_ops adsp_minidump_ops = {
>>        .start = adsp_start,
>>        .stop = adsp_stop,
>>        .da_to_va = adsp_da_to_va,
>> +     .parse_fw = qcom_register_dump_segments,
>>        .load = adsp_load,
>>        .panic = adsp_panic,
>>        .coredump = adsp_minidump,
>> --
>> 2.7.4
>>
diff mbox series

Patch

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 4b91e3c..68bd0bc 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -162,13 +162,18 @@  void qcom_minidump(struct rproc *rproc, unsigned int minidump_id)
 	 * is initialized in memory and encryption status is set.
 	 */
 	if (subsystem->regions_baseptr == 0 ||
-	    le32_to_cpu(subsystem->status) != 1 ||
-	    le32_to_cpu(subsystem->enabled) != MD_SS_ENABLED ||
-	    le32_to_cpu(subsystem->encryption_status) != MD_SS_ENCR_DONE) {
+		le32_to_cpu(subsystem->status) != 1 ||
+		le32_to_cpu(subsystem->enabled) != MD_SS_ENABLED) {
+			return rproc_coredump(rproc);
+		}
+
+	if (le32_to_cpu(subsystem->encryption_status) != MD_SS_ENCR_DONE) {
 		dev_err(&rproc->dev, "Minidump not ready, skipping\n");
 		return;
 	}
 
+	rproc_coredump_cleanup(rproc);
+
 	ret = qcom_add_minidump_segments(rproc, subsystem);
 	if (ret) {
 		dev_err(&rproc->dev, "Failed with error: %d while adding minidump entries\n", ret);
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 1ae47cc..40bf747 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -293,6 +293,7 @@  static const struct rproc_ops adsp_minidump_ops = {
 	.start = adsp_start,
 	.stop = adsp_stop,
 	.da_to_va = adsp_da_to_va,
+	.parse_fw = qcom_register_dump_segments,
 	.load = adsp_load,
 	.panic = adsp_panic,
 	.coredump = adsp_minidump,