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