RE: [RESEND PATCH v3 4/9] EDAC/ghes: Move ghes_edac.force_load to setup parameter

-----Original Message-----

[...]

quoted

+static int __init setup_ghes_edac_load(char *str) {
+	if (str)
+		if (!strcmp("true", str) || !strcmp("1", str))
+			ghes_edac_force = true;
+
+	if (!IS_ENABLED(CONFIG_X86))
+		ghes_edac_force = true;
+
+	return 1;
+}
+__setup("ghes_edac_force=", setup_ghes_edac_load);

Why all that?

Isn't specifying

ghes.edac_force_load

Ok, I will use module parameter ghes.edac_force_load.

on the kernel command line enough? I.e., you don't need to parse the passed
in option - just the presence of the parameter is enough.

quoted

+
 static ATOMIC_NOTIFIER_HEAD(ghes_report_chain);

 static inline bool is_hest_type_generic_v2(struct ghes *ghes) @@
-1517,13 +1537,13 @@ static struct acpi_platform_list plat_list[] = {
 	{ } /* End */
 };

-struct list_head *ghes_get_devices(bool force)
+struct list_head *ghes_get_devices(void)
 {
 	int idx = -1;

 	if (IS_ENABLED(CONFIG_X86)) {
 		idx = acpi_match_platform_list(plat_list);
-		if (idx < 0 && !force)
+		if (idx < 0 && !ghes_edac_force)
 			return NULL;
 	}

diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index

bb3ea42ba70b..6a2b54cc7240 100644

--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c

@@ -54,10 +54,6 @@ static DEFINE_MUTEX(ghes_reg_mutex);
  */
 static DEFINE_SPINLOCK(ghes_lock);

-/* "ghes_edac.force_load=1" skips the platform check */ -static bool

__read_mostly force_load; -module_param(force_load, bool, 0);
-
 static bool system_scanned;

 static struct list_head *ghes_devs;

@@ -437,23 +433,12 @@ static int ghes_edac_register(struct device *dev)
 	mci->ctl_name = "ghes_edac";
 	mci->dev_name = "ghes";

-	if (fake) {
-		pr_info("This system has a very crappy BIOS: It doesn't even list the

DIMMS.\n");

quoted

-		pr_info("Its SMBIOS info is wrong. It is doubtful that the error report

would\n");

quoted

-		pr_info("work on such system. Use this driver with caution\n");
-	} else if (force_load) {
-		pr_info("This EDAC driver relies on BIOS to enumerate memory and

get error reports.\n");

quoted

-		pr_info("Unfortunately, not all BIOSes reflect the memory layout

correctly.\n");

quoted

-		pr_info("So, the end result of using this driver varies from vendor to

vendor.\n");

quoted

-		pr_info("If you find incorrect reports, please contact your hardware

vendor\n");

quoted

-		pr_info("to correct its BIOS.\n");
-		pr_info("This system has %d DIMM sockets.\n",

ghes_hw.num_dimms);

quoted

-	}
-
 	if (!fake) {
 		struct dimm_info *src, *dst;
 		int i = 0;

+		pr_info("This system has %d DIMM sockets.\n",

ghes_hw.num_dimms);

quoted

+
 		mci_for_each_dimm(mci, dst) {
 			src = &ghes_hw.dimms[i];

This hunk...

Sorry, should move pr_info after this line

quoted

@@ -478,6 +463,17 @@ static int ghes_edac_register(struct device *dev)
 	} else {
 		struct dimm_info *dimm = edac_get_dimm(mci, 0, 0, 0);

+		pr_info("This system has a very crappy BIOS: It doesn't even list the

DIMMS.\n");

quoted

+		pr_info("Its SMBIOS info is wrong. It is doubtful that the error report

would\n");

quoted

+		pr_info("work on such system. Use this driver with caution\n");
+
+		if (ghes_edac_force) {
+			pr_info("This EDAC driver relies on BIOS to enumerate memory

and get\n");

quoted

+			pr_info("error reports. Unfortunately, not all BIOSes reflect

the\n");

quoted

+			pr_info("memory layout correctly. If you find incorrect reports,

please\n");

quoted

+			pr_info("contact your hardware vendor for its in correct

BIOS.\n");

quoted

+		}
+
 		dimm->nr_pages = 1;
 		dimm->grain = 128;
 		dimm->mtype = MEM_UNKNOWN;

... and this hunk look unrelated to what this patch is doing. What are they for?

I tried to keep the previous same logic.
 if (fake)
	print_something
 - !fake && ghes not preferred.
	print_otherthings


--
Cheers,
Justin (Jia He)