* [PATCH] ACPI / APEI: Remove needless __ghes_check_estatus() calls
@ 2019-06-17 5:25 luanshi
2019-06-21 15:18 ` Borislav Petkov
0 siblings, 1 reply; 3+ messages in thread
From: luanshi @ 2019-06-17 5:25 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown
Cc: linux-acpi, James Morse, Tony Luck, Borislav Petkov
Function __ghes_check_estatus() is called after __ghes_peek_estatus(),
but it is already called in __ghes_peek_estatus(). So we should remove
some needless __ghes_check_estatus() calls.
Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
---
drivers/acpi/apei/ghes.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 993940d..1041a4d 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -372,10 +372,6 @@ static int ghes_read_estatus(struct ghes *ghes,
if (rc)
return rc;
- rc = __ghes_check_estatus(ghes, estatus);
- if (rc)
- return rc;
-
return __ghes_read_estatus(estatus, *buf_paddr, fixmap_idx,
cper_estatus_len(estatus));
}
@@ -882,12 +878,6 @@ static int ghes_in_nmi_queue_one_entry(struct ghes *ghes,
return rc;
}
- rc = __ghes_check_estatus(ghes, &tmp_header);
- if (rc) {
- ghes_clear_estatus(ghes, &tmp_header, buf_paddr, fixmap_idx);
- return rc;
- }
-
len = cper_estatus_len(&tmp_header);
node_len = GHES_ESTATUS_NODE_LEN(len);
estatus_node = (void *)gen_pool_alloc(ghes_estatus_pool, node_len);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ACPI / APEI: Remove needless __ghes_check_estatus() calls
2019-06-17 5:25 [PATCH] ACPI / APEI: Remove needless __ghes_check_estatus() calls luanshi
@ 2019-06-21 15:18 ` Borislav Petkov
2019-06-24 17:32 ` James Morse
0 siblings, 1 reply; 3+ messages in thread
From: Borislav Petkov @ 2019-06-21 15:18 UTC (permalink / raw)
To: luanshi, James Morse; +Cc: Rafael J. Wysocki, Len Brown, linux-acpi, Tony Luck
On Mon, Jun 17, 2019 at 01:25:29PM +0800, luanshi wrote:
> Function __ghes_check_estatus() is called after __ghes_peek_estatus(),
> but it is already called in __ghes_peek_estatus(). So we should remove
> some needless __ghes_check_estatus() calls.
>
> Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
> ---
> drivers/acpi/apei/ghes.c | 10 ----------
> 1 file changed, 10 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 993940d..1041a4d 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -372,10 +372,6 @@ static int ghes_read_estatus(struct ghes *ghes,
> if (rc)
> return rc;
>
> - rc = __ghes_check_estatus(ghes, estatus);
> - if (rc)
> - return rc;
> -
> return __ghes_read_estatus(estatus, *buf_paddr, fixmap_idx,
> cper_estatus_len(estatus));
> }
> @@ -882,12 +878,6 @@ static int ghes_in_nmi_queue_one_entry(struct ghes *ghes,
> return rc;
> }
>
> - rc = __ghes_check_estatus(ghes, &tmp_header);
> - if (rc) {
> - ghes_clear_estatus(ghes, &tmp_header, buf_paddr, fixmap_idx);
> - return rc;
> - }
> -
> len = cper_estatus_len(&tmp_header);
> node_len = GHES_ESTATUS_NODE_LEN(len);
> estatus_node = (void *)gen_pool_alloc(ghes_estatus_pool, node_len);
> --
Yah, looks correct to me.
James, I think the cleaner thing to do would be for
__ghes_peek_estatus() not to call __ghes_check_estatus() at the end but
to return success and we can keep the two functions - "peek" and "check"
status - separate and always do:
if (peek)
return ...;
if (check)
return ...;
because this way the checking remains separate in __ghes_check_estatus()
and so is the peeking in __ghes_peek_estatus().
We can merge the two functions because we always do peek and then check
but keeping them separate makes the code clearer.
Am I making some sense...?
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ACPI / APEI: Remove needless __ghes_check_estatus() calls
2019-06-21 15:18 ` Borislav Petkov
@ 2019-06-24 17:32 ` James Morse
0 siblings, 0 replies; 3+ messages in thread
From: James Morse @ 2019-06-24 17:32 UTC (permalink / raw)
To: Borislav Petkov, luanshi
Cc: Rafael J. Wysocki, Len Brown, linux-acpi, Tony Luck
Hi Boris,
On 21/06/2019 16:18, Borislav Petkov wrote:
> On Mon, Jun 17, 2019 at 01:25:29PM +0800, luanshi wrote:
>> Function __ghes_check_estatus() is called after __ghes_peek_estatus(),
>> but it is already called in __ghes_peek_estatus(). So we should remove
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 993940d..1041a4d 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -372,10 +372,6 @@ static int ghes_read_estatus(struct ghes *ghes,
>> if (rc)
>> return rc;
>>
>> - rc = __ghes_check_estatus(ghes, estatus);
>> - if (rc)
>> - return rc;
>> -
>> return __ghes_read_estatus(estatus, *buf_paddr, fixmap_idx,
>> cper_estatus_len(estatus));
>> }
>> @@ -882,12 +878,6 @@ static int ghes_in_nmi_queue_one_entry(struct ghes *ghes,
>> return rc;
>> }
>>
>> - rc = __ghes_check_estatus(ghes, &tmp_header);
>> - if (rc) {
>> - ghes_clear_estatus(ghes, &tmp_header, buf_paddr, fixmap_idx);
>> - return rc;
>> - }
>> -
>> len = cper_estatus_len(&tmp_header);
>> node_len = GHES_ESTATUS_NODE_LEN(len);
>> estatus_node = (void *)gen_pool_alloc(ghes_estatus_pool, node_len);
>> --
>
> Yah, looks correct to me.
Yes, looks like I changed my mind halfway through about whether peek should just get the
values needed to allocate 'enough' memory, or do some validation too.
> James, I think the cleaner thing to do would be for
> __ghes_peek_estatus() not to call __ghes_check_estatus() at the end but
> to return success and we can keep the two functions - "peek" and "check"
> status - separate and always do:
>
> if (peek)
> return ...;
>
> if (check)
> return ...;
>
> because this way the checking remains separate in __ghes_check_estatus()
> and so is the peeking in __ghes_peek_estatus().
>
> We can merge the two functions because we always do peek and then check
> but keeping them separate makes the code clearer.
>
> Am I making some sense...?
Makes sense to me.
Thanks,
James
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-06-24 17:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17 5:25 [PATCH] ACPI / APEI: Remove needless __ghes_check_estatus() calls luanshi
2019-06-21 15:18 ` Borislav Petkov
2019-06-24 17:32 ` James Morse
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).