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
