On Thu, Jan 10, 2019 at 1:23 PM James Morse <james.morse@xxxxxxx> wrote: > >> > >> + if (is_hest_type_generic_v2(ghes) && ghes_ack_error(ghes->generic_v2)) > > > > Since ghes_ack_error() is always prepended with this check, you could > > push it down into the function: > > > > ghes_ack_error(ghes) > > ... > > > > if (!is_hest_type_generic_v2(ghes)) > > return 0; > > > > and simplify the two callsites :) > > Great idea! ... > > .. huh. Turns out for ghes_proc() we discard any errors other than ENOENT from > ghes_read_estatus() if is_hest_type_generic_v2(). This masks EIO. > > Most of the error sources discard the result, the worst thing I can find is > ghes_irq_func() will return IRQ_HANDLED, instead of IRQ_NONE when we didn't > really handle the IRQ. They're registered as SHARED, but I don't have an example > of what goes wrong next. > > I think this will also stop the spurious handling code kicking in to shut it up > if its broken and screaming. Unlikely, but not impossible. > > Fixed in a prior patch, with Boris' suggestion, ghes_proc()s tail ends up look > like this: > ----------------------%<---------------------- > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 0321d9420b1e..8d1f9930b159 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -700,18 +708,11 @@ static int ghes_proc(struct ghes *ghes) > > out: > ghes_clear_estatus(ghes, buf_paddr); > + if (rc != -ENOENT) > + rc_ack = ghes_ack_error(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)) > - return ghes_ack_error(ghes->generic_v2); > - > - return rc; > + /* If rc and rc_ack failed, return the first one */ > + return rc ? rc : rc_ack; > } > ----------------------%<---------------------- > Looks good to me, I guess there's no harm in acking invalid error status blocks. T
