RE: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform drv register

[Justin He]

Hi Borislav

> -----Original Message-----
> From: Borislav Petkov <bp@xxxxxxxxx>
> Sent: Tuesday, August 9, 2022 7:16 PM
> To: Justin He <Justin.He@xxxxxxx>
> Cc: Kani, Toshi <toshi.kani@xxxxxxx>; Rafael J. Wysocki <rafael@xxxxxxxxxx>;
> Len Brown <lenb@xxxxxxxxxx>; James Morse <James.Morse@xxxxxxx>; Tony
> Luck <tony.luck@xxxxxxxxx>; Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>;
> Robert Richter <rric@xxxxxxxxxx>; Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx>;
> Jarkko Sakkinen <jarkko@xxxxxxxxxx>; ACPI Devel Maling List
> <linux-acpi@xxxxxxxxxxxxxxx>; Linux Kernel Mailing List
> <linux-kernel@xxxxxxxxxxxxxxx>; open list:EDAC-CORE
> <linux-edac@xxxxxxxxxxxxxxx>
> Subject: Re: 回复: [PATCH] ACPI: APEI: move edac_init ahead of ghes platform
> drv register
>
> On Tue, Aug 09, 2022 at 09:24:33AM +0000, Justin He wrote:
> > If no, what is the detail logic of ghes_edac_driver_is_preferred()?
>
> That should be moved from ghes_edac.c to ghes.c:
>
> /*
>  * Known systems that are safe to enable this module.
>  */
> static struct acpi_platform_list plat_list[] = {
>         {"HPE   ", "Server  ", 0, ACPI_SIG_FADT, all_versions},
>         { } /* End */
> };
>
> and then
>
> bool ghes_edac_driver_is_preferred()
> {
>       if (IS_ENABLED(CONFIG_X86)) {
>                 /* Check if safe to enable on this system */
>                 idx = acpi_match_platform_list(plat_list);
>       } else if (ARM) {
>               /* insert ARM logic here */
>       }
> }
>
> That function should be called by the EDAC modules which compete with
> ghes_edac.
>
> In the x86 case, that's sb_edac, skx_edac and amd64_edac, I guess.

Ok, thanks a lot for elaborating it more.
Let me summarize it before sending v2 (maybe tomorrow):
1. implement a ghes_edac_driver_is_preferred() to replace the old ghes_edac_register()
and move it to ghes.c together with plat_list
2. save the ghes and dev to a global structure and pass it to the new module_init
3.move the remain logic of old ghes_edac_register() to a new XX_init() which is under the
module_init path.
4. introduce a stub ghes_edac_driver_is_preferred() for sb_edac, skx_edac and amd64_edac
to check.
5. add the check condition in the XXX_init of sb_edac, skx_edac and amd64_edac

Please let me know if I missed anything above.


--
Cheers,
Justin (Jia He)


>
> It all depends on what platforms Toshi wants to load it - I'm guessing HPE has
> both Intel and AMD platforms where they prefer ghes_edac.
>
> On ARM, that's up to ARM folks.
>
> > Because I notice that lots of other edac drivers are probing like:
> > ...
> >         owner = edac_get_owner();
> >         if (owner && strncmp(owner, EDAC_MOD_STR,
> sizeof(EDAC_MOD_STR)))
> >                 return -EBUSY;
>
> No, that's a silly mechanism to allow a single EDAC driver to load on the
> system. But your test will go before it, at the very beginning of the init
> function.
>
> HTH.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.