| Message ID | 1560749129-26288-1-git-send-email-zhangliguang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | ACPI / APEI: Remove needless __ghes_check_estatus() calls | expand |
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.
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
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);
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(-)