Messages in this thread | | | From | Justin He <> | Subject | RE: [PATCH 2/2] EDAC/ghes: Modularize ghes_edac driver to remove the dependency on ghes | Date | Tue, 16 Aug 2022 02:20:19 +0000 |
| |
Hi Borislav & Toshi Please see my questions below:
> -----Original Message----- > From: Borislav Petkov <bp@alien8.de> > Sent: Friday, August 12, 2022 10:46 PM > To: Justin He <Justin.He@arm.com> > Cc: Ard Biesheuvel <ardb@kernel.org>; Len Brown <lenb@kernel.org>; James > Morse <James.Morse@arm.com>; Tony Luck <tony.luck@intel.com>; Mauro > Carvalho Chehab <mchehab@kernel.org>; Robert Richter <rric@kernel.org>; > Robert Moore <robert.moore@intel.com>; linux-acpi@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org; devel@acpica.org; > Rafael J . Wysocki <rafael@kernel.org>; Shuai Xue > <xueshuai@linux.alibaba.com>; Jarkko Sakkinen <jarkko@kernel.org>; > linux-efi@vger.kernel.org; nd <nd@arm.com>; toshi.kani@hpe.com; > stable@kernel.org > Subject: Re: [PATCH 2/2] EDAC/ghes: Modularize ghes_edac driver to remove > the dependency on ghes > > On Thu, Aug 11, 2022 at 09:17:13AM +0000, Jia He wrote: > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index > > d91ad378c00d..ed6519f3d45b 100644 > > --- a/drivers/acpi/apei/ghes.c > > +++ b/drivers/acpi/apei/ghes.c > > @@ -94,6 +94,8 @@ > > #define FIX_APEI_GHES_SDEI_CRITICAL __end_of_fixed_addresses > > #endif > > > > +ATOMIC_NOTIFIER_HEAD(ghes_report_chain); > > static. You need function wrappers which call the notifier from the module. > > Also, why atomic? x86_mce_decoder_chain is a blocking one. > > Also, the whole notifier adding thing needs to be a separate patch. > > > @@ -1497,3 +1504,37 @@ void __init acpi_ghes_init(void) > > else > > pr_info(GHES_PFX "Failed to enable APEI firmware first mode.\n"); > > } > > + > > +/* > > + * Known x86 systems that prefer GHES error reporting: > > + */ > > +static struct acpi_platform_list plat_list[] = { > > + {"HPE ", "Server ", 0, ACPI_SIG_FADT, all_versions}, > > + { } /* End */ > > +}; > > + > > +struct list_head *ghes_get_devices(bool force) { > > + int idx = -1; > > + > > + if (IS_ENABLED(CONFIG_X86)) { > > + idx = acpi_match_platform_list(plat_list); > > + if (idx < 0 && !force) > > + return NULL; > > + } > > + > > + return &ghes_devs; > > +} > > +EXPORT_SYMBOL_GPL(ghes_get_devices); > > And yes, as Toshi points out, the other EDAC drivers - sb_edac, skx_edac and > amd64_edac - should call this function in their init functions so that it can get > selected which driver to load on HPE server platforms. > I assume that all those edac drivers which used owner checking are impacted, right? So the impacted list should be: drivers/edac/pnd2_edac.c:1532: if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) drivers/edac/sb_edac.c:3513: if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) drivers/edac/amd64_edac.c:4333: if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) drivers/edac/i10nm_base.c:552: if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) drivers/edac/skx_base.c:657: if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) drivers/edac/igen6_edac.c:1275: if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR)))
Furthermore, should I totally remove the owner check in above driver XX_init()? Because after the new helper ghes_get_device() checking, those drivers can't be initialized after ghes_edac is loaded. If ghes_edac is NOT loaded, the edac_mc_owner check in edac_mc_add_mc_with_groups() can guarantee that there is only one regular edac driver.
What do you think of it?
-- Cheers, Justin (Jia He)
| |