Skip to contentSkip to conversation

acpi: apei: clear error status before acknowledging the error

PATCH Superseded Tyler Baicar 2 participants 4 messages Aug 3, 2017 – Aug 24, 2017

Subsystems (2)
ACPI ACPI APEI

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Tyler Baicar

author
#0 lore raw

PATCH v2 1/1 acpi: apei: clear error status before acknowledging the error

Currently we acknowledge errors before clearing the error status. This could cause a new error to be populated by firmware in-between the error acknowledgment and the error status clearing which would cause the second error's status to be cleared without being handled. So, clear the error status before acknowledging the errors.

Also, make sure to acknowledge the error if the error status read fails.

V2: Only send error ack if there was an error populated
Remove curly braces that are no longer needed

Signed-off-by Tyler Baicar <[email protected]>
---
 drivers/acpi/apei/ghes.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/acpi/apei/ghes.c +3 -6
index d661d45..cda4a4f 100644
@@ -743,17 +743,14 @@ static int ghes_proc(struct ghes *ghes)
 	}
 	ghes_do_proc(ghes, ghes->estatus);
 
+out:
+	ghes_clear_estatus(ghes);
 	/*
 	 * GHESv2 type HEST entries introduce support for error acknowledgment,
 	 * so only acknowledge the error if this support is present.
 	 */
-	if (is_hest_type_generic_v2(ghes)) {
+	if (is_hest_type_generic_v2(ghes) && rc != -ENOENT)
 		rc = ghes_ack_error(ghes->generic_v2);
-		if (rc)
-			return rc;
-	}
-out:
-	ghes_clear_estatus(ghes);
 	return rc;
 }
Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
author
#1 lore raw

Hello Boris,

And input on this patch?

Thanks,
Tyler

Quoted 42 lines · Tyler Baicar

Currently we acknowledge errors before clearing the error status. This could cause a new error to be populated by firmware in-between the error acknowledgment and the error status clearing which would cause the second error's status to be cleared without being handled. So, clear the error status before acknowledging the errors.

Also, make sure to acknowledge the error if the error status read fails.

V2: Only send error ack if there was an error populated
Remove curly braces that are no longer needed

Signed-off-by Tyler Baicar <[email protected]>
---
  drivers/acpi/apei/ghes.c | 9 +++------
  1 file changed, 3 insertions(+), 6 deletions(-)
drivers/acpi/apei/ghes.c +3 -6
index d661d45..cda4a4f 100644
@@ -743,17 +743,14 @@ static int ghes_proc(struct ghes *ghes)
  	}
  	ghes_do_proc(ghes, ghes->estatus);
 
+out:
+	ghes_clear_estatus(ghes);
  	/*
  	 * GHESv2 type HEST entries introduce support for error acknowledgment,
  	 * so only acknowledge the error if this support is present.
  	 */
-	if (is_hest_type_generic_v2(ghes)) {
+	if (is_hest_type_generic_v2(ghes) && rc != -ENOENT)
  		rc = ghes_ack_error(ghes->generic_v2);
-		if (rc)
-			return rc;
-	}
-out:
-	ghes_clear_estatus(ghes);
  	return rc;
  }
Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
#2 lore raw
In reply to Tyler Baicar
Quoted 23 lines · Tyler Baicar

Currently we acknowledge errors before clearing the error status. This could cause a new error to be populated by firmware in-between the error acknowledgment and the error status clearing which would cause the second error's status to be cleared without being handled. So, clear the error status before acknowledging the errors.

Also, make sure to acknowledge the error if the error status read fails.

V2: Only send error ack if there was an error populated
Remove curly braces that are no longer needed

Signed-off-by Tyler Baicar <[email protected]>
---
 drivers/acpi/apei/ghes.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/acpi/apei/ghes.c +0 -0
index d661d45..cda4a4f 100644
@@ -743,17 +743,14 @@ static int ghes_proc(struct ghes *ghes)
 	}

drivers/acpi/apei/ghes.c static int ghes_proc(struct ghes *ghes) · line 752

Quoted 10 lines
drivers/acpi/apei/ghes.c +3 -1
 	ghes_do_proc(ghes, ghes->estatus);
 
+out:
+	ghes_clear_estatus(ghes);
 	/*
 	 * GHESv2 type HEST entries introduce support for error acknowledgment,
 	 * so only acknowledge the error if this support is present.
 	 */
-	if (is_hest_type_generic_v2(ghes)) {
+	if (is_hest_type_generic_v2(ghes) && rc != -ENOENT)

I think you should do something like this:

out:

        ghes_clear_estatus(ghes);

	if (rc == -ENOENT)
                return rc;

	/*
         * GHESv2 type HEST entries introduce support for error acknowledgment,
         * so only acknowledge the error if this support is present.
         */
        if (is_hest_type_generic_v2(ghes))
                return ghes_ack_error(ghes->generic_v2);

        return rc;

First check the -ENOENT because if there's no entry, you don't need to ACK anything anyway.

And then only if it is an entry present and it is v2, you can ACK it.

Hmmm?

Signature · 5 lines
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
author
#3 lore raw
Quoted 52 lines · Borislav Petkov
Quoted 23 lines · Tyler Baicar

Currently we acknowledge errors before clearing the error status. This could cause a new error to be populated by firmware in-between the error acknowledgment and the error status clearing which would cause the second error's status to be cleared without being handled. So, clear the error status before acknowledging the errors.

Also, make sure to acknowledge the error if the error status read fails.

V2: Only send error ack if there was an error populated
Remove curly braces that are no longer needed

Signed-off-by Tyler Baicar <[email protected]>
---
  drivers/acpi/apei/ghes.c | 9 +++------
  1 file changed, 3 insertions(+), 6 deletions(-)
drivers/acpi/apei/ghes.c +0 -0
index d661d45..cda4a4f 100644
@@ -743,17 +743,14 @@ static int ghes_proc(struct ghes *ghes)
  	}

drivers/acpi/apei/ghes.c static int ghes_proc(struct ghes *ghes) · line 752

Quoted 10 lines
drivers/acpi/apei/ghes.c +3 -1
  	ghes_do_proc(ghes, ghes->estatus);
 
+out:
+	ghes_clear_estatus(ghes);
  	/*
  	 * GHESv2 type HEST entries introduce support for error acknowledgment,
  	 * so only acknowledge the error if this support is present.
  	 */
-	if (is_hest_type_generic_v2(ghes)) {
+	if (is_hest_type_generic_v2(ghes) && rc != -ENOENT)

I think you should do something like this:

out:

         ghes_clear_estatus(ghes);

	if (rc == -ENOENT)
                 return rc;

	/*
          * GHESv2 type HEST entries introduce support for error acknowledgment,
          * so only acknowledge the error if this support is present.
          */
         if (is_hest_type_generic_v2(ghes))
                 return ghes_ack_error(ghes->generic_v2);

         return rc;
Quoted 6 lines

First check the -ENOENT because if there's no entry, you don't need to ACK anything anyway.

And then only if it is an entry present and it is v2, you can ACK it.

Hmmm?

Looks good to me, I'll send a v3 with this change.

Thanks,
Tyler

Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.