| Message ID | 20220905152439.77770-1-justin.he@arm.com |
|---|---|
| Headers | show |
| Series | Make ghes_edac a proper module | expand |
On Monday, September 5, 2022 9:25 AM, Jia He wrote: > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 8cb65f757d06..b0a6445c6da2 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -109,6 +109,14 @@ static inline bool is_hest_type_generic_v2(struct > ghes *ghes) > bool ghes_disable; > module_param_named(disable, ghes_disable, bool, 0); > > +/* > + * "ghes.edac_force_enable" forcibly enables ghes_edac and skips the > platform > + * check. > + */ > +bool ghes_edac_force_enable; > +EXPORT_SYMBOL(ghes_edac_force_enable); > +module_param_named(edac_force_enable, ghes_edac_force_enable, > bool, 0); Thanks for the update. It looks good to me. Reviewed-by: Toshi Kani <toshi.kani@hpe.com> Toshi
On Monday, September 5, 2022 9:25 AM, Jia He wrote: > -void ghes_edac_unregister(struct ghes *ghes) > +static void ghes_edac_unregister(struct ghes *ghes) > { > struct mem_ctl_info *mci; > unsigned long flags; > > - if (!ghes_edac_force_enable) > - return; > - I am not familiar with this original check. Assuming it is safe to delete it now (as you mentioned in other email w/o description), I suggest adding some description to the commit log. At high-level, the patch looks good to me. Acked-by: Toshi Kani <toshi.kani@hpe.com> Toshi
Hi Kani > -----Original Message----- > From: Kani, Toshi <toshi.kani@hpe.com> > Sent: Thursday, September 8, 2022 8:31 AM > To: Justin He <Justin.He@arm.com>; Len Brown <lenb@kernel.org>; James > Morse <James.Morse@arm.com>; Tony Luck <tony.luck@intel.com>; Borislav > Petkov <bp@alien8.de>; Mauro Carvalho Chehab <mchehab@kernel.org>; > Robert Richter <rric@kernel.org>; Robert Moore <robert.moore@intel.com>; > Qiuxu Zhuo <qiuxu.zhuo@intel.com>; Yazen Ghannam > <yazen.ghannam@amd.com>; Jan Luebbe <jlu@pengutronix.de>; Khuong > Dinh <khuong@os.amperecomputing.com> > Cc: Ard Biesheuvel <ardb@kernel.org>; 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> > Subject: RE: [PATCH v5 5/8] EDAC/ghes: Make ghes_edac a proper module to > remove the dependency on ghes > > On Monday, September 5, 2022 9:25 AM, Jia He wrote: > > -void ghes_edac_unregister(struct ghes *ghes) > > +static void ghes_edac_unregister(struct ghes *ghes) > > { > > struct mem_ctl_info *mci; > > unsigned long flags; > > > > - if (!ghes_edac_force_enable) > > - return; > > - > > I am not familiar with this original check. Assuming it is safe to delete it now > (as you mentioned in other email w/o description), I suggest adding some > description to the commit log. > Thanks, will do I've tested the insmod/rmmod multiple times on Arm host. I will wait for others' comments for a while before sending the new version. > At high-level, the patch looks good to me. > Acked-by: Toshi Kani <toshi.kani@hpe.com> Thanks a lot for the help 😊 -- Cheers, Justin (Jia He)
Commit dc4e8c07e9e2 ("ACPI: APEI: explicit init of HEST and GHES in apci_init()") introduced a bug that ghes_edac_register() would be invoked before edac_init(). Because at that time, the bus "edac" hasn't been even registered, this created sysfs /devices/mc0 instead of /sys/devices/system/edac/mc/mc0 on an Ampere eMag server. The proper solution is to make ghes_edac a proper module. Changelog: v5: - add the review-by from Toshi for patch 04 and 06 - refine the commit msg - remove the unconditional set of ghes_edac_force_enable on Arm v4: https://lore.kernel.org/lkml/20220831074027.13849-6-justin.he@arm.com/ - move the kernel boot option to ghes module parameter - collapse th ghes_present and ghes_edac_preferred into one patch v3: https://lore.kernel.org/lkml/20220822154048.188253-1-justin.he@arm.com/ - refine the commit logs - introduce ghes preferred and present flag (by Toshi) - move force_load to setup parameter - add the ghes_edac_preferred() check for x86/Arm edac drivers v2: https://lore.kernel.org/lkml/20220817143458.335938-1-justin.he@arm.com/ - add acked-by tag of Patch 1 from Ard - split the notifier patch - add 2 patch to get regular drivers selected when ghes edac is not loaded - fix an errno in igen6 driver - add a patch to fix the sparse warning of ghes - refine the commit logs v1: https://lore.kernel.org/lkml/20220811091713.10427-1-justin.he@arm.com/ Jia He (8): efi/cper: export several helpers for ghes_edac to use EDAC/ghes: Add a notifier for reporting memory errors EDAC:ghes: Move ghes_edac.force_load to ghes module parameter ghes: Introduce a helper ghes_edac_preferred() EDAC/ghes: Make ghes_edac a proper module to remove the dependency on ghes EDAC: Add the ghes_edac_preferred check for chipset-specific edac drivers apei/ghes: Use unrcu_pointer for cmpxchg EDAC/igen6: Return consistent errno when another edac driver is enabled drivers/acpi/apei/ghes.c | 82 +++++++++++++++++++++++++++++++--- drivers/edac/Kconfig | 4 +- drivers/edac/amd64_edac.c | 3 ++ drivers/edac/armada_xp_edac.c | 3 ++ drivers/edac/edac_module.h | 1 + drivers/edac/ghes_edac.c | 78 ++++++++++++++++++++------------ drivers/edac/i10nm_base.c | 3 ++ drivers/edac/igen6_edac.c | 5 ++- drivers/edac/layerscape_edac.c | 3 ++ drivers/edac/pnd2_edac.c | 3 ++ drivers/edac/sb_edac.c | 3 ++ drivers/edac/skx_base.c | 3 ++ drivers/edac/thunderx_edac.c | 3 ++ drivers/edac/xgene_edac.c | 3 ++ drivers/firmware/efi/cper.c | 3 ++ include/acpi/apei.h | 2 + include/acpi/ghes.h | 36 +++++---------- 17 files changed, 175 insertions(+), 63 deletions(-)