| Message ID | 20220822154048.188253-6-justin.he@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Make ghes_edac a proper module | expand |
On Monday, August 22, 2022 9:41 AM, Jia He wrote: > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index e17e0ee8f842..327386f3cf33 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -1537,16 +1537,25 @@ static struct acpi_platform_list plat_list[] = { > { } /* End */ > }; > > -struct list_head *ghes_get_devices(void) > +bool ghes_edac_preferred(void) > { > int idx = -1; > > if (IS_ENABLED(CONFIG_X86)) { > idx = acpi_match_platform_list(plat_list); > if (idx < 0 && !ghes_edac_force) > - return NULL; > + return false; > } > > + return true; > +} > +EXPORT_SYMBOL_GPL(ghes_edac_preferred); > + > +struct list_head *ghes_get_devices(void) > +{ > + if (!ghes_edac_preferred()) > + return NULL; > + > return &ghes_devs; > } ghes_get_devices() changing multiple times in the series is confusing to me. Can you simply introduce ghes_get_devices() and ghes_preferred() in the right state in a patch? Perhaps, patch #2, #5, #6 can collapse to introduce the two funcs? The rest of patch #5 adding the call to ghes_edac_preferred() into other edac drivers can remain as a separate patch. Toshi
> -----Original Message----- > From: Kani, Toshi <toshi.kani@hpe.com> > Sent: Thursday, August 25, 2022 7:04 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>; Jonathan Corbet <corbet@lwn.net>; 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>; Paul E. McKenney > <paulmck@kernel.org>; Andrew Morton <akpm@linux-foundation.org>; > Neeraj Upadhyay <quic_neeraju@quicinc.com>; Randy Dunlap > <rdunlap@infradead.org>; Damien Le Moal > <damien.lemoal@opensource.wdc.com>; Muchun Song > <songmuchun@bytedance.com>; linux-doc@vger.kernel.org > Subject: RE: [RESEND PATCH v3 5/9] EDAC: Don't load chipset-specific edac > drivers when ghes_edac is preferred > > On Monday, August 22, 2022 9:41 AM, Jia He wrote: > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index > > e17e0ee8f842..327386f3cf33 100644 > > --- a/drivers/acpi/apei/ghes.c > > +++ b/drivers/acpi/apei/ghes.c > > @@ -1537,16 +1537,25 @@ static struct acpi_platform_list plat_list[] = { > > { } /* End */ > > }; > > > > -struct list_head *ghes_get_devices(void) > > +bool ghes_edac_preferred(void) > > { > > int idx = -1; > > > > if (IS_ENABLED(CONFIG_X86)) { > > idx = acpi_match_platform_list(plat_list); > > if (idx < 0 && !ghes_edac_force) > > - return NULL; > > + return false; > > } > > > > + return true; > > +} > > +EXPORT_SYMBOL_GPL(ghes_edac_preferred); > > + > > +struct list_head *ghes_get_devices(void) { > > + if (!ghes_edac_preferred()) > > + return NULL; > > + > > return &ghes_devs; > > } > > ghes_get_devices() changing multiple times in the series is > confusing to me. Can you simply introduce ghes_get_devices() > and ghes_preferred() in the right state in a patch? Perhaps, patch #2, #5, #6 > can collapse to introduce the two funcs? > My purpose was to make it easy for review. I am ok to merge these patches into one. > The rest of patch #5 adding the call to ghes_edac_preferred() into other edac > drivers can remain as a separate patch. Okay, I assume you mean to merge all of the ghes_edac_preferred() checking for intel and Arm edac drivers into 1 separate patch, am I understanding well? -- Cheers, Justin (Jia He)
On Thursday, August 25, 2022 3:46 AM, Justin He wrote: > > ghes_get_devices() changing multiple times in the series is > > confusing to me. Can you simply introduce ghes_get_devices() > > and ghes_preferred() in the right state in a patch? Perhaps, patch #2, #5, > > #6 can collapse to introduce the two funcs? > > My purpose was to make it easy for review. I am ok to merge these patches > into one. This series starts with your original patchset and then has additional patches to address the issues with the original patchset. The number of the patches continues to increase in this way... You do not need to record the history of discussions and design changes in the series. > > The rest of patch #5 adding the call to ghes_edac_preferred() into other edac > > drivers can remain as a separate patch. > > Okay, I assume you mean to merge all of the ghes_edac_preferred() > checking for intel and > Arm edac drivers into 1 separate patch, am I understanding well? No, that's not what I meant. ghes_get_devices() and ghes_edac_preferred() look good to me after all patches are applied. So, instead of introducing the original design of ghes_get_devices() and then changing its design multiple times in the series, please simply add the final version of ghes_get_devices() and ghes_edac_preferred() in a single patch. They are small functions anyway. That is, your series includes something like: - PATCH: Add ghes_get_devices() and ghes_edac_preferred() interfaces - PATCH: Add ghes_edac_preferred check to chipset-specific edac drivers Toshi
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index e17e0ee8f842..327386f3cf33 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -1537,16 +1537,25 @@ static struct acpi_platform_list plat_list[] = { { } /* End */ }; -struct list_head *ghes_get_devices(void) +bool ghes_edac_preferred(void) { int idx = -1; if (IS_ENABLED(CONFIG_X86)) { idx = acpi_match_platform_list(plat_list); if (idx < 0 && !ghes_edac_force) - return NULL; + return false; } + return true; +} +EXPORT_SYMBOL_GPL(ghes_edac_preferred); + +struct list_head *ghes_get_devices(void) +{ + if (!ghes_edac_preferred()) + return NULL; + return &ghes_devs; } EXPORT_SYMBOL_GPL(ghes_get_devices); diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index 2f854feeeb23..5314a934c2bf 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -4329,6 +4329,9 @@ static int __init amd64_edac_init(void) int err = -ENODEV; int i; + if (ghes_edac_preferred()) + return -EBUSY; + owner = edac_get_owner(); if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) return -EBUSY; diff --git a/drivers/edac/edac_module.h b/drivers/edac/edac_module.h index 96f6de0c8ff6..3826f82de487 100644 --- a/drivers/edac/edac_module.h +++ b/drivers/edac/edac_module.h @@ -11,6 +11,7 @@ #ifndef __EDAC_MODULE_H__ #define __EDAC_MODULE_H__ +#include <acpi/ghes.h> #include "edac_mc.h" #include "edac_pci.h" #include "edac_device.h" diff --git a/drivers/edac/i10nm_base.c b/drivers/edac/i10nm_base.c index 6cf50ee0b77c..691df9b51622 100644 --- a/drivers/edac/i10nm_base.c +++ b/drivers/edac/i10nm_base.c @@ -548,6 +548,9 @@ static int __init i10nm_init(void) edac_dbg(2, "\n"); + if (ghes_edac_preferred()) + return -EBUSY; + owner = edac_get_owner(); if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) return -EBUSY; diff --git a/drivers/edac/igen6_edac.c b/drivers/edac/igen6_edac.c index a07bbfd075d0..4ac6d0c533ec 100644 --- a/drivers/edac/igen6_edac.c +++ b/drivers/edac/igen6_edac.c @@ -1271,6 +1271,9 @@ static int __init igen6_init(void) edac_dbg(2, "\n"); + if (ghes_edac_preferred()) + return -EBUSY; + owner = edac_get_owner(); if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) return -ENODEV; diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c index a20b299f1202..4ddc43e6c420 100644 --- a/drivers/edac/pnd2_edac.c +++ b/drivers/edac/pnd2_edac.c @@ -1528,6 +1528,9 @@ static int __init pnd2_init(void) edac_dbg(2, "\n"); + if (ghes_edac_preferred()) + return -EBUSY; + owner = edac_get_owner(); if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) return -EBUSY; diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c index 9678ab97c7ac..3ff604a5e95a 100644 --- a/drivers/edac/sb_edac.c +++ b/drivers/edac/sb_edac.c @@ -3506,6 +3506,9 @@ static int __init sbridge_init(void) edac_dbg(2, "\n"); + if (ghes_edac_preferred()) + return -EBUSY; + owner = edac_get_owner(); if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) return -EBUSY; diff --git a/drivers/edac/skx_base.c b/drivers/edac/skx_base.c index 1abc020d49ab..286370728552 100644 --- a/drivers/edac/skx_base.c +++ b/drivers/edac/skx_base.c @@ -653,6 +653,9 @@ static int __init skx_init(void) edac_dbg(2, "\n"); + if (ghes_edac_preferred()) + return -EBUSY; + owner = edac_get_owner(); if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) return -EBUSY; diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h index e29327ee0b83..1c47018a1e43 100644 --- a/include/acpi/ghes.h +++ b/include/acpi/ghes.h @@ -73,9 +73,11 @@ int ghes_register_vendor_record_notifier(struct notifier_block *nb); void ghes_unregister_vendor_record_notifier(struct notifier_block *nb); struct list_head *ghes_get_devices(void); +bool ghes_edac_preferred(void); extern bool ghes_edac_force; #else static inline struct list_head *ghes_get_devices(void) { return NULL; } +static bool ghes_edac_preferred(void) { return false; } static bool ghes_edac_force; #endif
edac_mc_add_mc* is too late in the init path and that check should happen as the very first thing in the driver init function. Suggested-by: Toshi Kani <toshi.kani@hpe.com> Signed-off-by: Jia He <justin.he@arm.com> --- drivers/acpi/apei/ghes.c | 13 +++++++++++-- drivers/edac/amd64_edac.c | 3 +++ drivers/edac/edac_module.h | 1 + drivers/edac/i10nm_base.c | 3 +++ drivers/edac/igen6_edac.c | 3 +++ drivers/edac/pnd2_edac.c | 3 +++ drivers/edac/sb_edac.c | 3 +++ drivers/edac/skx_base.c | 3 +++ include/acpi/ghes.h | 2 ++ 9 files changed, 32 insertions(+), 2 deletions(-)