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

[Pierre Gondois <pierre.gondois@arm.com>]

Hello Jonathan,

On 10/20/26 19:09, Jonathan Cameron wrote:
> On Mon, 19 Oct 2026 22:38:43 +0200
> Pierre Gondois <pierre.gondois@arm.com> 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@arm.com>
>> ---
>>   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?
>
>