[PATCH V3] acpi: apei: clear error status before acknowledging the error

[PATCH V3] acpi: apei: clear error status before acknowledging the error

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

V3: Seperate check for -ENOENT return value

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

Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
---
 drivers/acpi/apei/ghes.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index d661d45..8d43b1c 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -743,17 +743,19 @@ static int ghes_proc(struct ghes *ghes)
 	}
 	ghes_do_proc(ghes, ghes->estatus);
 
+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)) {
-		rc = ghes_ack_error(ghes->generic_v2);
-		if (rc)
-			return rc;
-	}
-out:
-	ghes_clear_estatus(ghes);
+	if (is_hest_type_generic_v2(ghes))
+		return ghes_ack_error(ghes->generic_v2);
+
 	return rc;
 }
 
-- 
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.

Re: [PATCH V3] acpi: apei: clear error status before acknowledging the error

[Andy Shevchenko]

On Mon, 2017-08-28 at 10:53 -0600, Tyler Baicar wrote:
> 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.
> 


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

You can also do this here, like
	if (is_hest_type_generic_v2(ghes) && rc != -ENOENT)

though I left this for Rafael to choose which one is preferable.

> +		return ghes_ack_error(ghes->generic_v2);
> +


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH V3] acpi: apei: clear error status before acknowledging the error

[Borislav Petkov]

On Mon, Aug 28, 2017 at 08:01:00PM +0300, Andy Shevchenko wrote:
> You can also do this here, like
> 	if (is_hest_type_generic_v2(ghes) && rc != -ENOENT)

https://lkml.kernel.org/r/20170824081408.iwg7qyyr226btivo@pd.tnic

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH V3] acpi: apei: clear error status before acknowledging the error

[Andy Shevchenko]

On Mon, Aug 28, 2017 at 8:14 PM, Borislav Petkov <bp@suse.de> wrote:
> On Mon, Aug 28, 2017 at 08:01:00PM +0300, Andy Shevchenko wrote:
>> You can also do this here, like
>>       if (is_hest_type_generic_v2(ghes) && rc != -ENOENT)
>
> https://lkml.kernel.org/r/20170824081408.iwg7qyyr226btivo@pd.tnic

       if (rc != -ENOENT && is_hest_type_generic_v2(ghes))
 :-)

But again, your call to choose :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3] acpi: apei: clear error status before acknowledging the error

[Borislav Petkov]

On Mon, Aug 28, 2017 at 08:20:37PM +0300, Andy Shevchenko wrote:
>        if (rc != -ENOENT && is_hest_type_generic_v2(ghes))
>  :-)
> 
> But again, your call to choose :-)

Slow down please.

What do you think is more readable: a compound if-statement or two
simple ones, the second one with an explanatory comment?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH V3] acpi: apei: clear error status before acknowledging the error

[Andy Shevchenko]

On Mon, Aug 28, 2017 at 8:25 PM, Borislav Petkov <bp@suse.de> wrote:
> On Mon, Aug 28, 2017 at 08:20:37PM +0300, Andy Shevchenko wrote:
>>        if (rc != -ENOENT && is_hest_type_generic_v2(ghes))
>>  :-)
>>
>> But again, your call to choose :-)
>
> Slow down please.
>
> What do you think is more readable: a compound if-statement or two
> simple ones, the second one with an explanatory comment?

For my opinion, since you asked, the either case needs a comment on
top of that additional check.
Separate conditionals in independent cases are, of course, better.
Though, here they are dependent (as I read from commit message).
So, _personally_ I would go with compound one, but note first sentence
in this response.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3] acpi: apei: clear error status before acknowledging the error

[Borislav Petkov]

On Mon, Aug 28, 2017 at 08:44:21PM +0300, Andy Shevchenko wrote:
> For my opinion, since you asked, the either case needs a comment on
> top of that additional check.

That's because the comment belongs to the v2 part of the check.

> Separate conditionals in independent cases are, of course, better.

Yes, and separate are easier to read if you read them like this:

+       if (rc == -ENOENT)
+               return rc;

<--- Ok, we got the missing entry out of the way, now, here, we have a
valid entry. Now we can concentrate on processing it further.

	... other check and ack and ...

And this becomes a lot more natural when you're staring at a big function
which does a lot of things and you concentrate only on the main path.

Oh, and this is how those checks get translated to asm as there you
don't really have compound if-statements. So if you switch your mind to
reading such checks separately, you're practically ready to read their
asm translation too...

Anyway, this is how I see it.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH V3] acpi: apei: clear error status before acknowledging the error

[Andy Shevchenko]

On Mon, Aug 28, 2017 at 9:27 PM, Borislav Petkov <bp@suse.de> wrote:
> On Mon, Aug 28, 2017 at 08:44:21PM +0300, Andy Shevchenko wrote:
>> For my opinion, since you asked, the either case needs a comment on
>> top of that additional check.
>
> That's because the comment belongs to the v2 part of the check.
>
>> Separate conditionals in independent cases are, of course, better.
>
> Yes, and separate are easier to read if you read them like this:
>
> +       if (rc == -ENOENT)
> +               return rc;
>
> <--- Ok, we got the missing entry out of the way, now, here, we have a
> valid entry. Now we can concentrate on processing it further.
>
>         ... other check and ack and ...
>
> And this becomes a lot more natural when you're staring at a big function
> which does a lot of things and you concentrate only on the main path.
>
> Oh, and this is how those checks get translated to asm as there you
> don't really have compound if-statements. So if you switch your mind to
> reading such checks separately, you're practically ready to read their
> asm translation too...
>
> Anyway, this is how I see it.
>
> --
> Regards/Gruss,
>     Boris.
>
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> --



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3] acpi: apei: clear error status before acknowledging the error

[Andy Shevchenko]

On Mon, Aug 28, 2017 at 9:27 PM, Borislav Petkov <bp@suse.de> wrote:
> On Mon, Aug 28, 2017 at 08:44:21PM +0300, Andy Shevchenko wrote:
>> For my opinion, since you asked, the either case needs a comment on
>> top of that additional check.
>
> That's because the comment belongs to the v2 part of the check.

Sorry not being clear, I meant another, separate comment.

>> Separate conditionals in independent cases are, of course, better.
>
> Yes, and separate are easier to read if you read them like this:
>
> +       if (rc == -ENOENT)
> +               return rc;
>
> <--- Ok, we got the missing entry out of the way, now, here, we have a
> valid entry. Now we can concentrate on processing it further.
>
>         ... other check and ack and ...
>
> And this becomes a lot more natural when you're staring at a big function
> which does a lot of things and you concentrate only on the main path.
>
> Oh, and this is how those checks get translated to asm as there you
> don't really have compound if-statements. So if you switch your mind to
> reading such checks separately, you're practically ready to read their
> asm translation too...
>
> Anyway, this is how I see it.

Looking into commit message again I think the word 'also' creates all
this. Two separate commits might be perfect, though good enough is to
have an additional comment to the new check.

Thanks for sharing detailed point of view.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3] acpi: apei: clear error status before acknowledging the error

[Borislav Petkov]

On Mon, Aug 28, 2017 at 10:53:41AM -0600, Tyler Baicar wrote:
> 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.
> 
> V3: Seperate check for -ENOENT return value
> 
> V2: Only send error ack if there was an error populated
>     Remove curly braces that are no longer needed
> 
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> ---
>  drivers/acpi/apei/ghes.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V3] acpi: apei: clear error status before acknowledging the error

[Baicar, Tyler]

On 8/29/2017 2:16 AM, Borislav Petkov wrote:
> On Mon, Aug 28, 2017 at 10:53:41AM -0600, Tyler Baicar wrote:
>> 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.
>>
>> V3: Seperate check for -ENOENT return value
>>
>> V2: Only send error ack if there was an error populated
>>      Remove curly braces that are no longer needed
>>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> ---
>>   drivers/acpi/apei/ghes.c | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
> Reviewed-by: Borislav Petkov <bp@suse.de>
Hello Rafael,

Were you able to apply this patch?

Thanks,
Tyler

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

Re: [PATCH V3] acpi: apei: clear error status before acknowledging the error

[Tyler Baicar]

On 9/13/2017 8:40 AM, Baicar, Tyler wrote:
> On 8/29/2017 2:16 AM, Borislav Petkov wrote:
>> On Mon, Aug 28, 2017 at 10:53:41AM -0600, Tyler Baicar wrote:
>>> 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.
>>>
>>> V3: Seperate check for -ENOENT return value
>>>
>>> V2: Only send error ack if there was an error populated
>>>      Remove curly braces that are no longer needed
>>>
>>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>>> ---
>>>   drivers/acpi/apei/ghes.c | 16 +++++++++-------
>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>> Reviewed-by: Borislav Petkov <bp@suse.de>
> Hello Rafael,
>
> Were you able to apply this patch?
I haven't heard anything on this patch. It would be great to have it in 
4.14 :)

Thanks,
Tyler