Re: [PATCH v2 4/6] EDAC/ghes: Extract error report initialization

[Pierre Gondois]

Hello Jonathan,

On 10/20/26 19:09, Jonathan Cameron wrote:
> On Mon, 19 Oct 2026 22:38:43 +0200
> Pierre Gondois <pierre.gondois@xxxxxxx> wrote:
>
> > Extract a ghes_edac_init_error() function to prepare for
> > Memory Error Section 2 support (and re-usage).
> >
> > Also use a guard in the caller ghes_edac_report_mem_error()
> > function for code simplification.
> >
> > Signed-off-by: Pierre Gondois <pierre.gondois@xxxxxxx>
> > ---
> >   drivers/edac/ghes_edac.c | 66 +++++++++++++++++++++-------------------
> >   1 file changed, 35 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> > index d80c88818691..b90150c67f4b 100644
> > --- a/drivers/edac/ghes_edac.c
> > +++ b/drivers/edac/ghes_edac.c
> > @@ -267,37 +267,14 @@ static int print_mem_error_other_detail(const struct cper_sec_mem_err *mem, char
> >   	return n;
> >   }
> >   
> > -static int ghes_edac_report_mem_error(struct notifier_block *nb,
> > -				      unsigned long val, void *data)
> > +/* Caller must hold ghes_lock. */
> > +static struct edac_raw_error_desc *
> > +ghes_edac_init_error(struct ghes_pvt *pvt, unsigned long sev)
> Slightly unusual to have an init function that doesn't directly take the thing
> being initialized. I'd pass it in. The clearly of pvt->msg and pvt->other_detail
> is also a bit of a surprise for a function that claims to be initializing something
> else so I'd do those outside of the helper.
>
> Or maybe rename helper to something like
> ghes_edac_fill_error_from_pvt()
> Not sure that really conveys the modification of pvt data hidden in here though.

I can rename the function to ghes_edac_fill_error_from_pvt() and factor out:

*pvt->other_detail = '\0'; *pvt->msg = '\0';

if that helps making the function clearer.

> Perhaps this refactor was a bad suggestion from me and better to just duplicate
> the code.  What do ACPI maintainers think?