[PATCH v3 0/5] enable ghes_edac on selected platforms

[PATCH v3 0/5] enable ghes_edac on selected platforms

[Toshi Kani]

The ghes_edac driver was introduced in 2013 [1], but it has not been
enabled by any distro yet.  This is because the driver obtains error
info from firmware interfaces, which are not properly implemented on
many platforms.

To get out from this situation, add a platform check to selectively
enable the driver on the platforms that are known to have proper
firmware implementation.  Platform vendors can add their platforms to
the list when they support ghes_edac.

Patch 1-2 introduces a common function for platform check.
Patch 3 introduces platform check to ghes_edac.
Patch 4-5 optimizes regular edac driver's init code when ghes_edac is used.

v3:
 - Change struct & func names to "platform" from "oem" (patch 1)
 - Drop a patch that checks OSC APEI bit (remove v2 patch 3)
 - Drop a patch that avoids multiple calls to dmi_walk() (remove v2 patch 4)
 - Change parameter name to ghes_edac.force_load (patch 3)
 - Change function to edac_get_owner() (patch 4)
 - Change edac_mc_owner to const char * (patch 4)
 - Change to call edac_get_owner() at the beginning (patch 5)
 - Remove ".c" from mod_name (patch 5)

v2:
 - Address review comments (patch 1)
 - Add OSC APEI check (patch 3)
 - Avoid multiple dmi_walk (patch 4)
 - Add EDAC MC owner check (patch 6,7)

---
Toshi Kani (5):
 1/5 ACPI / blacklist: add acpi_match_platform_list()
 2/5 intel_pstate: convert to use acpi_match_platform_list()
 3/5 ghes_edac: add platform check to enable ghes_edac
 4/5 EDAC: add edac_get_owner() to check MC owner
 5/5 edac drivers: add MC owner check in init

---
 drivers/acpi/blacklist.c       | 83 +++++++-----------------------------------
 drivers/acpi/utils.c           | 40 ++++++++++++++++++++
 drivers/cpufreq/intel_pstate.c | 64 +++++++++++++-------------------
 drivers/edac/amd64_edac.c      |  5 +++
 drivers/edac/edac_mc.c         |  7 +++-
 drivers/edac/edac_mc.h         |  8 ++++
 drivers/edac/ghes_edac.c       | 28 +++++++++++---
 drivers/edac/pnd2_edac.c       |  9 ++++-
 drivers/edac/sb_edac.c         |  9 ++++-
 drivers/edac/skx_edac.c        |  8 +++-
 include/linux/acpi.h           | 19 ++++++++++
 11 files changed, 162 insertions(+), 118 deletions(-)

[PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

[Toshi Kani]

ACPI OEM ID / OEM Table ID / Revision can be used to identify
a platform based on ACPI firmware info.  acpi_blacklisted(),
intel_pstate_platform_pwr_mgmt_exists(), and some other funcs,
have been using similar check to detect a list of platforms
that require special handlings.

Move the platform check in acpi_blacklisted() to a new common
utility function, acpi_match_platform_list(), so that other
drivers do not have to implement their own version.

There is no change in functionality.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Borislav Petkov <bp@alien8.de>
---
 drivers/acpi/blacklist.c |   83 ++++++++--------------------------------------
 drivers/acpi/utils.c     |   40 ++++++++++++++++++++++
 include/linux/acpi.h     |   19 +++++++++++
 3 files changed, 73 insertions(+), 69 deletions(-)

diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c
index bb542ac..037fd53 100644
--- a/drivers/acpi/blacklist.c
+++ b/drivers/acpi/blacklist.c
@@ -30,30 +30,13 @@
 
 #include "internal.h"
 
-enum acpi_blacklist_predicates {
-	all_versions,
-	less_than_or_equal,
-	equal,
-	greater_than_or_equal,
-};
-
-struct acpi_blacklist_item {
-	char oem_id[7];
-	char oem_table_id[9];
-	u32 oem_revision;
-	char *table;
-	enum acpi_blacklist_predicates oem_revision_predicate;
-	char *reason;
-	u32 is_critical_error;
-};
-
 static struct dmi_system_id acpi_rev_dmi_table[] __initdata;
 
 /*
  * POLICY: If *anything* doesn't work, put it on the blacklist.
  *	   If they are critical errors, mark it critical, and abort driver load.
  */
-static struct acpi_blacklist_item acpi_blacklist[] __initdata = {
+static struct acpi_platform_list acpi_blacklist[] __initdata = {
 	/* Compaq Presario 1700 */
 	{"PTLTD ", "  DSDT  ", 0x06040000, ACPI_SIG_DSDT, less_than_or_equal,
 	 "Multiple problems", 1},
@@ -67,65 +50,27 @@ static struct acpi_blacklist_item acpi_blacklist[] __initdata = {
 	{"IBM   ", "TP600E  ", 0x00000105, ACPI_SIG_DSDT, less_than_or_equal,
 	 "Incorrect _ADR", 1},
 
-	{""}
+	{ }
 };
 
 int __init acpi_blacklisted(void)
 {
-	int i = 0;
+	int i;
 	int blacklisted = 0;
-	struct acpi_table_header table_header;
-
-	while (acpi_blacklist[i].oem_id[0] != '\0') {
-		if (acpi_get_table_header(acpi_blacklist[i].table, 0, &table_header)) {
-			i++;
-			continue;
-		}
-
-		if (strncmp(acpi_blacklist[i].oem_id, table_header.oem_id, 6)) {
-			i++;
-			continue;
-		}
-
-		if (strncmp
-		    (acpi_blacklist[i].oem_table_id, table_header.oem_table_id,
-		     8)) {
-			i++;
-			continue;
-		}
-
-		if ((acpi_blacklist[i].oem_revision_predicate == all_versions)
-		    || (acpi_blacklist[i].oem_revision_predicate ==
-			less_than_or_equal
-			&& table_header.oem_revision <=
-			acpi_blacklist[i].oem_revision)
-		    || (acpi_blacklist[i].oem_revision_predicate ==
-			greater_than_or_equal
-			&& table_header.oem_revision >=
-			acpi_blacklist[i].oem_revision)
-		    || (acpi_blacklist[i].oem_revision_predicate == equal
-			&& table_header.oem_revision ==
-			acpi_blacklist[i].oem_revision)) {
 
-			printk(KERN_ERR PREFIX
-			       "Vendor \"%6.6s\" System \"%8.8s\" "
-			       "Revision 0x%x has a known ACPI BIOS problem.\n",
-			       acpi_blacklist[i].oem_id,
-			       acpi_blacklist[i].oem_table_id,
-			       acpi_blacklist[i].oem_revision);
+	i = acpi_match_platform_list(acpi_blacklist);
+	if (i >= 0) {
+		pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\" Revision 0x%x has a known ACPI BIOS problem.\n",
+		       acpi_blacklist[i].oem_id,
+		       acpi_blacklist[i].oem_table_id,
+		       acpi_blacklist[i].oem_revision);
 
-			printk(KERN_ERR PREFIX
-			       "Reason: %s. This is a %s error\n",
-			       acpi_blacklist[i].reason,
-			       (acpi_blacklist[i].
-				is_critical_error ? "non-recoverable" :
-				"recoverable"));
+		pr_err(PREFIX "Reason: %s. This is a %s error\n",
+		       acpi_blacklist[i].reason,
+		       (acpi_blacklist[i].data ?
+			"non-recoverable" : "recoverable"));
 
-			blacklisted = acpi_blacklist[i].is_critical_error;
-			break;
-		} else {
-			i++;
-		}
+		blacklisted = acpi_blacklist[i].data;
 	}
 
 	(void)early_acpi_osi_init();
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index b9d956c..998aaf5 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -816,3 +816,43 @@ static int __init acpi_backlight(char *str)
 	return 1;
 }
 __setup("acpi_backlight=", acpi_backlight);
+
+/**
+ * acpi_match_platform_list - Check if the system matches with a given list
+ * @plat: pointer to acpi_platform_list table terminated by a NULL entry
+ *
+ * Return the matched index if the system is found in the platform list.
+ * Otherwise, return a negative error code.
+ */
+int acpi_match_platform_list(const struct acpi_platform_list *plat)
+{
+	struct acpi_table_header hdr;
+	int idx = 0;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	for (; plat->oem_id[0]; plat++, idx++) {
+		if (ACPI_FAILURE(acpi_get_table_header(plat->table, 0, &hdr)))
+			continue;
+
+		if (strncmp(plat->oem_id, hdr.oem_id, ACPI_OEM_ID_SIZE))
+			continue;
+
+		if (strncmp(plat->oem_table_id, hdr.oem_table_id,
+			    ACPI_OEM_TABLE_ID_SIZE))
+			continue;
+
+		if ((plat->pred == all_versions) ||
+		    (plat->pred == less_than_or_equal
+			&& hdr.oem_revision <= plat->oem_revision) ||
+		    (plat->pred == greater_than_or_equal
+			&& hdr.oem_revision >= plat->oem_revision) ||
+		    (plat->pred == equal
+			&& hdr.oem_revision == plat->oem_revision))
+			return idx;
+	}
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL(acpi_match_platform_list);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 27b4b66..a9b6dc2 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -556,6 +556,25 @@ extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
 #define ACPI_OST_SC_DRIVER_LOAD_FAILURE		0x81
 #define ACPI_OST_SC_INSERT_NOT_SUPPORTED	0x82
 
+enum acpi_predicate {
+	all_versions,
+	less_than_or_equal,
+	equal,
+	greater_than_or_equal,
+};
+
+/* Table must be terminted by a NULL entry */
+struct acpi_platform_list {
+	char	oem_id[ACPI_OEM_ID_SIZE];
+	char	oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
+	u32	oem_revision;
+	char	*table;
+	enum acpi_predicate pred;
+	char	*reason;
+	u32	data;
+};
+int acpi_match_platform_list(const struct acpi_platform_list *plat);
+
 extern void acpi_early_init(void);
 extern void acpi_subsystem_init(void);
 

[v3,1/5] ACPI / blacklist: add acpi_match_platform_list()

[Toshi Kani]

ACPI OEM ID / OEM Table ID / Revision can be used to identify
a platform based on ACPI firmware info.  acpi_blacklisted(),
intel_pstate_platform_pwr_mgmt_exists(), and some other funcs,
have been using similar check to detect a list of platforms
that require special handlings.

Move the platform check in acpi_blacklisted() to a new common
utility function, acpi_match_platform_list(), so that other
drivers do not have to implement their own version.

There is no change in functionality.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Borislav Petkov <bp@alien8.de>
---
 drivers/acpi/blacklist.c |   83 ++++++++--------------------------------------
 drivers/acpi/utils.c     |   40 ++++++++++++++++++++++
 include/linux/acpi.h     |   19 +++++++++++
 3 files changed, 73 insertions(+), 69 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c
index bb542ac..037fd53 100644
--- a/drivers/acpi/blacklist.c
+++ b/drivers/acpi/blacklist.c
@@ -30,30 +30,13 @@
 
 #include "internal.h"
 
-enum acpi_blacklist_predicates {
-	all_versions,
-	less_than_or_equal,
-	equal,
-	greater_than_or_equal,
-};
-
-struct acpi_blacklist_item {
-	char oem_id[7];
-	char oem_table_id[9];
-	u32 oem_revision;
-	char *table;
-	enum acpi_blacklist_predicates oem_revision_predicate;
-	char *reason;
-	u32 is_critical_error;
-};
-
 static struct dmi_system_id acpi_rev_dmi_table[] __initdata;
 
 /*
  * POLICY: If *anything* doesn't work, put it on the blacklist.
  *	   If they are critical errors, mark it critical, and abort driver load.
  */
-static struct acpi_blacklist_item acpi_blacklist[] __initdata = {
+static struct acpi_platform_list acpi_blacklist[] __initdata = {
 	/* Compaq Presario 1700 */
 	{"PTLTD ", "  DSDT  ", 0x06040000, ACPI_SIG_DSDT, less_than_or_equal,
 	 "Multiple problems", 1},
@@ -67,65 +50,27 @@ static struct acpi_blacklist_item acpi_blacklist[] __initdata = {
 	{"IBM   ", "TP600E  ", 0x00000105, ACPI_SIG_DSDT, less_than_or_equal,
 	 "Incorrect _ADR", 1},
 
-	{""}
+	{ }
 };
 
 int __init acpi_blacklisted(void)
 {
-	int i = 0;
+	int i;
 	int blacklisted = 0;
-	struct acpi_table_header table_header;
-
-	while (acpi_blacklist[i].oem_id[0] != '\0') {
-		if (acpi_get_table_header(acpi_blacklist[i].table, 0, &table_header)) {
-			i++;
-			continue;
-		}
-
-		if (strncmp(acpi_blacklist[i].oem_id, table_header.oem_id, 6)) {
-			i++;
-			continue;
-		}
-
-		if (strncmp
-		    (acpi_blacklist[i].oem_table_id, table_header.oem_table_id,
-		     8)) {
-			i++;
-			continue;
-		}
-
-		if ((acpi_blacklist[i].oem_revision_predicate == all_versions)
-		    || (acpi_blacklist[i].oem_revision_predicate ==
-			less_than_or_equal
-			&& table_header.oem_revision <=
-			acpi_blacklist[i].oem_revision)
-		    || (acpi_blacklist[i].oem_revision_predicate ==
-			greater_than_or_equal
-			&& table_header.oem_revision >=
-			acpi_blacklist[i].oem_revision)
-		    || (acpi_blacklist[i].oem_revision_predicate == equal
-			&& table_header.oem_revision ==
-			acpi_blacklist[i].oem_revision)) {
 
-			printk(KERN_ERR PREFIX
-			       "Vendor \"%6.6s\" System \"%8.8s\" "
-			       "Revision 0x%x has a known ACPI BIOS problem.\n",
-			       acpi_blacklist[i].oem_id,
-			       acpi_blacklist[i].oem_table_id,
-			       acpi_blacklist[i].oem_revision);
+	i = acpi_match_platform_list(acpi_blacklist);
+	if (i >= 0) {
+		pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\" Revision 0x%x has a known ACPI BIOS problem.\n",
+		       acpi_blacklist[i].oem_id,
+		       acpi_blacklist[i].oem_table_id,
+		       acpi_blacklist[i].oem_revision);
 
-			printk(KERN_ERR PREFIX
-			       "Reason: %s. This is a %s error\n",
-			       acpi_blacklist[i].reason,
-			       (acpi_blacklist[i].
-				is_critical_error ? "non-recoverable" :
-				"recoverable"));
+		pr_err(PREFIX "Reason: %s. This is a %s error\n",
+		       acpi_blacklist[i].reason,
+		       (acpi_blacklist[i].data ?
+			"non-recoverable" : "recoverable"));
 
-			blacklisted = acpi_blacklist[i].is_critical_error;
-			break;
-		} else {
-			i++;
-		}
+		blacklisted = acpi_blacklist[i].data;
 	}
 
 	(void)early_acpi_osi_init();
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index b9d956c..998aaf5 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -816,3 +816,43 @@ static int __init acpi_backlight(char *str)
 	return 1;
 }
 __setup("acpi_backlight=", acpi_backlight);
+
+/**
+ * acpi_match_platform_list - Check if the system matches with a given list
+ * @plat: pointer to acpi_platform_list table terminated by a NULL entry
+ *
+ * Return the matched index if the system is found in the platform list.
+ * Otherwise, return a negative error code.
+ */
+int acpi_match_platform_list(const struct acpi_platform_list *plat)
+{
+	struct acpi_table_header hdr;
+	int idx = 0;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	for (; plat->oem_id[0]; plat++, idx++) {
+		if (ACPI_FAILURE(acpi_get_table_header(plat->table, 0, &hdr)))
+			continue;
+
+		if (strncmp(plat->oem_id, hdr.oem_id, ACPI_OEM_ID_SIZE))
+			continue;
+
+		if (strncmp(plat->oem_table_id, hdr.oem_table_id,
+			    ACPI_OEM_TABLE_ID_SIZE))
+			continue;
+
+		if ((plat->pred == all_versions) ||
+		    (plat->pred == less_than_or_equal
+			&& hdr.oem_revision <= plat->oem_revision) ||
+		    (plat->pred == greater_than_or_equal
+			&& hdr.oem_revision >= plat->oem_revision) ||
+		    (plat->pred == equal
+			&& hdr.oem_revision == plat->oem_revision))
+			return idx;
+	}
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL(acpi_match_platform_list);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 27b4b66..a9b6dc2 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -556,6 +556,25 @@ extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
 #define ACPI_OST_SC_DRIVER_LOAD_FAILURE		0x81
 #define ACPI_OST_SC_INSERT_NOT_SUPPORTED	0x82
 
+enum acpi_predicate {
+	all_versions,
+	less_than_or_equal,
+	equal,
+	greater_than_or_equal,
+};
+
+/* Table must be terminted by a NULL entry */
+struct acpi_platform_list {
+	char	oem_id[ACPI_OEM_ID_SIZE];
+	char	oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
+	u32	oem_revision;
+	char	*table;
+	enum acpi_predicate pred;
+	char	*reason;
+	u32	data;
+};
+int acpi_match_platform_list(const struct acpi_platform_list *plat);
+
 extern void acpi_early_init(void);
 extern void acpi_subsystem_init(void);
 

Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

[Borislav Petkov]

On Fri, Aug 18, 2017 at 01:46:40PM -0600, Toshi Kani wrote:
> ACPI OEM ID / OEM Table ID / Revision can be used to identify
> a platform based on ACPI firmware info.  acpi_blacklisted(),
> intel_pstate_platform_pwr_mgmt_exists(), and some other funcs,
> have been using similar check to detect a list of platforms
> that require special handlings.
> 
> Move the platform check in acpi_blacklisted() to a new common
> utility function, acpi_match_platform_list(), so that other
> drivers do not have to implement their own version.
> 
> There is no change in functionality.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Borislav Petkov <bp@alien8.de>
> ---
>  drivers/acpi/blacklist.c |   83 ++++++++--------------------------------------
>  drivers/acpi/utils.c     |   40 ++++++++++++++++++++++
>  include/linux/acpi.h     |   19 +++++++++++
>  3 files changed, 73 insertions(+), 69 deletions(-)

...

> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index b9d956c..998aaf5 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -816,3 +816,43 @@ static int __init acpi_backlight(char *str)
>  	return 1;
>  }
>  __setup("acpi_backlight=", acpi_backlight);
> +
> +/**
> + * acpi_match_platform_list - Check if the system matches with a given list
> + * @plat: pointer to acpi_platform_list table terminated by a NULL entry
> + *
> + * Return the matched index if the system is found in the platform list.
> + * Otherwise, return a negative error code.
> + */
> +int acpi_match_platform_list(const struct acpi_platform_list *plat)
> +{
> +	struct acpi_table_header hdr;
> +	int idx = 0;
> +
> +	if (acpi_disabled)
> +		return -ENODEV;

Btw, Rafael, should we do something like this:

diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
index 010b1c43df92..881b0d5b2838 100644
--- a/drivers/acpi/acpica/tbxface.c
+++ b/drivers/acpi/acpica/tbxface.c
@@ -226,6 +226,9 @@ acpi_get_table_header(char *signature,
 	u32 j;
 	struct acpi_table_header *header;
 
+	if (acpi_disabled)
+		return (AE_ERROR);
+
 	/* Parameter validation */
 
 	if (!signature || !out_table_header) {
---

in order to avoid that sprinkling of if (acpi_disabled) everywhere?

> +
> +	for (; plat->oem_id[0]; plat++, idx++) {
> +		if (ACPI_FAILURE(acpi_get_table_header(plat->table, 0, &hdr)))
> +			continue;
> +
> +		if (strncmp(plat->oem_id, hdr.oem_id, ACPI_OEM_ID_SIZE))
> +			continue;
> +
> +		if (strncmp(plat->oem_table_id, hdr.oem_table_id,
> +			    ACPI_OEM_TABLE_ID_SIZE))

Let that stick out.

> +			continue;
> +
> +		if ((plat->pred == all_versions) ||
> +		    (plat->pred == less_than_or_equal
> +			&& hdr.oem_revision <= plat->oem_revision) ||
> +		    (plat->pred == greater_than_or_equal
> +			&& hdr.oem_revision >= plat->oem_revision) ||
> +		    (plat->pred == equal
> +			&& hdr.oem_revision == plat->oem_revision))
> +			return idx;

Make that more readable:

                if ((plat->pred == all_versions) ||
                    (plat->pred == less_than_or_equal    && hdr.oem_revision <= plat->oem_revision) ||
                    (plat->pred == greater_than_or_equal && hdr.oem_revision >= plat->oem_revision) ||
                    (plat->pred == equal                 && hdr.oem_revision == plat->oem_revision))
                        return idx;


> +	}
> +
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL(acpi_match_platform_list);
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 27b4b66..a9b6dc2 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -556,6 +556,25 @@ extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
>  #define ACPI_OST_SC_DRIVER_LOAD_FAILURE		0x81
>  #define ACPI_OST_SC_INSERT_NOT_SUPPORTED	0x82
>  
> +enum acpi_predicate {
> +	all_versions,
> +	less_than_or_equal,
> +	equal,
> +	greater_than_or_equal,
> +};
> +
> +/* Table must be terminted by a NULL entry */
> +struct acpi_platform_list {
> +	char	oem_id[ACPI_OEM_ID_SIZE];

					+ 1

> +	char	oem_table_id[ACPI_OEM_TABLE_ID_SIZE];

						   + 1
> +	u32	oem_revision;
> +	char	*table;
> +	enum acpi_predicate pred;
> +	char	*reason;
> +	u32	data;

Ok, turning that into data from is_critical_error is a step in the right
direction. Let's make it even better:

	u32	flags;

and do

#define ACPI_PLAT_IS_CRITICAL_ERROR	BIT(0)

so that future elements add new bits instead of wasting a whole u32 as a
boolean.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[v3,1/5] ACPI / blacklist: add acpi_match_platform_list()

[Borislav Petkov]

On Fri, Aug 18, 2017 at 01:46:40PM -0600, Toshi Kani wrote:
> ACPI OEM ID / OEM Table ID / Revision can be used to identify
> a platform based on ACPI firmware info.  acpi_blacklisted(),
> intel_pstate_platform_pwr_mgmt_exists(), and some other funcs,
> have been using similar check to detect a list of platforms
> that require special handlings.
> 
> Move the platform check in acpi_blacklisted() to a new common
> utility function, acpi_match_platform_list(), so that other
> drivers do not have to implement their own version.
> 
> There is no change in functionality.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Borislav Petkov <bp@alien8.de>
> ---
>  drivers/acpi/blacklist.c |   83 ++++++++--------------------------------------
>  drivers/acpi/utils.c     |   40 ++++++++++++++++++++++
>  include/linux/acpi.h     |   19 +++++++++++
>  3 files changed, 73 insertions(+), 69 deletions(-)

...

> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index b9d956c..998aaf5 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -816,3 +816,43 @@ static int __init acpi_backlight(char *str)
>  	return 1;
>  }
>  __setup("acpi_backlight=", acpi_backlight);
> +
> +/**
> + * acpi_match_platform_list - Check if the system matches with a given list
> + * @plat: pointer to acpi_platform_list table terminated by a NULL entry
> + *
> + * Return the matched index if the system is found in the platform list.
> + * Otherwise, return a negative error code.
> + */
> +int acpi_match_platform_list(const struct acpi_platform_list *plat)
> +{
> +	struct acpi_table_header hdr;
> +	int idx = 0;
> +
> +	if (acpi_disabled)
> +		return -ENODEV;

Btw, Rafael, should we do something like this:
---

in order to avoid that sprinkling of if (acpi_disabled) everywhere?

> +
> +	for (; plat->oem_id[0]; plat++, idx++) {
> +		if (ACPI_FAILURE(acpi_get_table_header(plat->table, 0, &hdr)))
> +			continue;
> +
> +		if (strncmp(plat->oem_id, hdr.oem_id, ACPI_OEM_ID_SIZE))
> +			continue;
> +
> +		if (strncmp(plat->oem_table_id, hdr.oem_table_id,
> +			    ACPI_OEM_TABLE_ID_SIZE))

Let that stick out.

> +			continue;
> +
> +		if ((plat->pred == all_versions) ||
> +		    (plat->pred == less_than_or_equal
> +			&& hdr.oem_revision <= plat->oem_revision) ||
> +		    (plat->pred == greater_than_or_equal
> +			&& hdr.oem_revision >= plat->oem_revision) ||
> +		    (plat->pred == equal
> +			&& hdr.oem_revision == plat->oem_revision))
> +			return idx;

Make that more readable:

                if ((plat->pred == all_versions) ||
                    (plat->pred == less_than_or_equal    && hdr.oem_revision <= plat->oem_revision) ||
                    (plat->pred == greater_than_or_equal && hdr.oem_revision >= plat->oem_revision) ||
                    (plat->pred == equal                 && hdr.oem_revision == plat->oem_revision))
                        return idx;


> +	}
> +
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL(acpi_match_platform_list);
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 27b4b66..a9b6dc2 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -556,6 +556,25 @@ extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
>  #define ACPI_OST_SC_DRIVER_LOAD_FAILURE		0x81
>  #define ACPI_OST_SC_INSERT_NOT_SUPPORTED	0x82
>  
> +enum acpi_predicate {
> +	all_versions,
> +	less_than_or_equal,
> +	equal,
> +	greater_than_or_equal,
> +};
> +
> +/* Table must be terminted by a NULL entry */
> +struct acpi_platform_list {
> +	char	oem_id[ACPI_OEM_ID_SIZE];

					+ 1

> +	char	oem_table_id[ACPI_OEM_TABLE_ID_SIZE];

						   + 1
> +	u32	oem_revision;
> +	char	*table;
> +	enum acpi_predicate pred;
> +	char	*reason;
> +	u32	data;

Ok, turning that into data from is_critical_error is a step in the right
direction. Let's make it even better:

	u32	flags;

and do

#define ACPI_PLAT_IS_CRITICAL_ERROR	BIT(0)

so that future elements add new bits instead of wasting a whole u32 as a
boolean.

diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
index 010b1c43df92..881b0d5b2838 100644
--- a/drivers/acpi/acpica/tbxface.c
+++ b/drivers/acpi/acpica/tbxface.c
@@ -226,6 +226,9 @@ acpi_get_table_header(char *signature,
 	u32 j;
 	struct acpi_table_header *header;
 
+	if (acpi_disabled)
+		return (AE_ERROR);
+
 	/* Parameter validation */
 
 	if (!signature || !out_table_header) {

Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

[Rafael J. Wysocki]

On Mon, Aug 21, 2017 at 1:27 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Aug 18, 2017 at 01:46:40PM -0600, Toshi Kani wrote:
>> ACPI OEM ID / OEM Table ID / Revision can be used to identify
>> a platform based on ACPI firmware info.  acpi_blacklisted(),
>> intel_pstate_platform_pwr_mgmt_exists(), and some other funcs,
>> have been using similar check to detect a list of platforms
>> that require special handlings.
>>
>> Move the platform check in acpi_blacklisted() to a new common
>> utility function, acpi_match_platform_list(), so that other
>> drivers do not have to implement their own version.
>>
>> There is no change in functionality.
>>
>> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: Borislav Petkov <bp@alien8.de>
>> ---
>>  drivers/acpi/blacklist.c |   83 ++++++++--------------------------------------
>>  drivers/acpi/utils.c     |   40 ++++++++++++++++++++++
>>  include/linux/acpi.h     |   19 +++++++++++
>>  3 files changed, 73 insertions(+), 69 deletions(-)
>
> ...
>
>> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
>> index b9d956c..998aaf5 100644
>> --- a/drivers/acpi/utils.c
>> +++ b/drivers/acpi/utils.c
>> @@ -816,3 +816,43 @@ static int __init acpi_backlight(char *str)
>>       return 1;
>>  }
>>  __setup("acpi_backlight=", acpi_backlight);
>> +
>> +/**
>> + * acpi_match_platform_list - Check if the system matches with a given list
>> + * @plat: pointer to acpi_platform_list table terminated by a NULL entry
>> + *
>> + * Return the matched index if the system is found in the platform list.
>> + * Otherwise, return a negative error code.
>> + */
>> +int acpi_match_platform_list(const struct acpi_platform_list *plat)
>> +{
>> +     struct acpi_table_header hdr;
>> +     int idx = 0;
>> +
>> +     if (acpi_disabled)
>> +             return -ENODEV;
>
> Btw, Rafael, should we do something like this:
>
> diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
> index 010b1c43df92..881b0d5b2838 100644
> --- a/drivers/acpi/acpica/tbxface.c
> +++ b/drivers/acpi/acpica/tbxface.c
> @@ -226,6 +226,9 @@ acpi_get_table_header(char *signature,
>         u32 j;
>         struct acpi_table_header *header;
>
> +       if (acpi_disabled)
> +               return (AE_ERROR);
> +
>         /* Parameter validation */
>
>         if (!signature || !out_table_header) {
> ---
>
> in order to avoid that sprinkling of if (acpi_disabled) everywhere?

Yes, let's catch this as early as possible.

Thanks,
Rafael

[v3,1/5] ACPI / blacklist: add acpi_match_platform_list()

[Rafael J. Wysocki]

On Mon, Aug 21, 2017 at 1:27 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Aug 18, 2017 at 01:46:40PM -0600, Toshi Kani wrote:
>> ACPI OEM ID / OEM Table ID / Revision can be used to identify
>> a platform based on ACPI firmware info.  acpi_blacklisted(),
>> intel_pstate_platform_pwr_mgmt_exists(), and some other funcs,
>> have been using similar check to detect a list of platforms
>> that require special handlings.
>>
>> Move the platform check in acpi_blacklisted() to a new common
>> utility function, acpi_match_platform_list(), so that other
>> drivers do not have to implement their own version.
>>
>> There is no change in functionality.
>>
>> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: Borislav Petkov <bp@alien8.de>
>> ---
>>  drivers/acpi/blacklist.c |   83 ++++++++--------------------------------------
>>  drivers/acpi/utils.c     |   40 ++++++++++++++++++++++
>>  include/linux/acpi.h     |   19 +++++++++++
>>  3 files changed, 73 insertions(+), 69 deletions(-)
>
> ...
>
>> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
>> index b9d956c..998aaf5 100644
>> --- a/drivers/acpi/utils.c
>> +++ b/drivers/acpi/utils.c
>> @@ -816,3 +816,43 @@ static int __init acpi_backlight(char *str)
>>       return 1;
>>  }
>>  __setup("acpi_backlight=", acpi_backlight);
>> +
>> +/**
>> + * acpi_match_platform_list - Check if the system matches with a given list
>> + * @plat: pointer to acpi_platform_list table terminated by a NULL entry
>> + *
>> + * Return the matched index if the system is found in the platform list.
>> + * Otherwise, return a negative error code.
>> + */
>> +int acpi_match_platform_list(const struct acpi_platform_list *plat)
>> +{
>> +     struct acpi_table_header hdr;
>> +     int idx = 0;
>> +
>> +     if (acpi_disabled)
>> +             return -ENODEV;
>
> Btw, Rafael, should we do something like this:
>
> diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
> index 010b1c43df92..881b0d5b2838 100644
> --- a/drivers/acpi/acpica/tbxface.c
> +++ b/drivers/acpi/acpica/tbxface.c
> @@ -226,6 +226,9 @@ acpi_get_table_header(char *signature,
>         u32 j;
>         struct acpi_table_header *header;
>
> +       if (acpi_disabled)
> +               return (AE_ERROR);
> +
>         /* Parameter validation */
>
>         if (!signature || !out_table_header) {
> ---
>
> in order to avoid that sprinkling of if (acpi_disabled) everywhere?

Yes, let's catch this as early as possible.

Thanks,
Rafael
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ACPICA: Check whether ACPI is disabled before getting a table

[Borislav Petkov]

Borislav Petkov <bp@suse.de>

... and save us the splrinking of this test around in the callers.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 drivers/acpi/acpica/tbxface.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
index 010b1c43df92..881b0d5b2838 100644
--- a/drivers/acpi/acpica/tbxface.c
+++ b/drivers/acpi/acpica/tbxface.c
@@ -226,6 +226,9 @@ acpi_get_table_header(char *signature,
 	u32 j;
 	struct acpi_table_header *header;
 
+	if (acpi_disabled)
+		return (AE_ERROR);
+
 	/* Parameter validation */
 
 	if (!signature || !out_table_header) {
-- 
2.13.0

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

ACPICA: Check whether ACPI is disabled before getting a table

[Borislav Petkov]

Borislav Petkov <bp@suse.de>

... and save us the splrinking of this test around in the callers.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 drivers/acpi/acpica/tbxface.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
index 010b1c43df92..881b0d5b2838 100644
--- a/drivers/acpi/acpica/tbxface.c
+++ b/drivers/acpi/acpica/tbxface.c
@@ -226,6 +226,9 @@ acpi_get_table_header(char *signature,
 	u32 j;
 	struct acpi_table_header *header;
 
+	if (acpi_disabled)
+		return (AE_ERROR);
+
 	/* Parameter validation */
 
 	if (!signature || !out_table_header) {

Re: [PATCH] ACPICA: Check whether ACPI is disabled before getting a table

[Rafael J. Wysocki]

On Mon, Aug 21, 2017 at 3:20 PM, Borislav Petkov <bp@alien8.de> wrote:
> Borislav Petkov <bp@suse.de>
>
> ... and save us the splrinking of this test around in the callers.
>
> No functionality change.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  drivers/acpi/acpica/tbxface.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
> index 010b1c43df92..881b0d5b2838 100644
> --- a/drivers/acpi/acpica/tbxface.c
> +++ b/drivers/acpi/acpica/tbxface.c
> @@ -226,6 +226,9 @@ acpi_get_table_header(char *signature,
>         u32 j;
>         struct acpi_table_header *header;
>
> +       if (acpi_disabled)
> +               return (AE_ERROR);
> +
>         /* Parameter validation */
>
>         if (!signature || !out_table_header) {
> --

Is acpi_disabled an ACPICA variable?  If not, this cannot go upstream.
Not a big deal I guess, but somewhat yucky.

ACPICA: Check whether ACPI is disabled before getting a table

[Rafael J. Wysocki]

On Mon, Aug 21, 2017 at 3:20 PM, Borislav Petkov <bp@alien8.de> wrote:
> Borislav Petkov <bp@suse.de>
>
> ... and save us the splrinking of this test around in the callers.
>
> No functionality change.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  drivers/acpi/acpica/tbxface.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
> index 010b1c43df92..881b0d5b2838 100644
> --- a/drivers/acpi/acpica/tbxface.c
> +++ b/drivers/acpi/acpica/tbxface.c
> @@ -226,6 +226,9 @@ acpi_get_table_header(char *signature,
>         u32 j;
>         struct acpi_table_header *header;
>
> +       if (acpi_disabled)
> +               return (AE_ERROR);
> +
>         /* Parameter validation */
>
>         if (!signature || !out_table_header) {
> --

Is acpi_disabled an ACPICA variable?  If not, this cannot go upstream.
Not a big deal I guess, but somewhat yucky.
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPICA: Check whether ACPI is disabled before getting a table

[Borislav Petkov]

On Mon, Aug 21, 2017 at 03:30:47PM +0200, Rafael J. Wysocki wrote:
> Is acpi_disabled an ACPICA variable?

Doesn't look like it: it is defined in arch/x86/kernel/acpi/boot.c

> If not, this cannot go upstream. Not a big deal I guess, but somewhat
> yucky.

Uff, there's that "sync" of keeping ACPICA code and its version in the
kernel the same. Yuck.

Oh well.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

ACPICA: Check whether ACPI is disabled before getting a table

[Borislav Petkov]

On Mon, Aug 21, 2017 at 03:30:47PM +0200, Rafael J. Wysocki wrote:
> Is acpi_disabled an ACPICA variable?

Doesn't look like it: it is defined in arch/x86/kernel/acpi/boot.c

> If not, this cannot go upstream. Not a big deal I guess, but somewhat
> yucky.

Uff, there's that "sync" of keeping ACPICA code and its version in the
kernel the same. Yuck.

Oh well.

Re: [PATCH] ACPICA: Check whether ACPI is disabled before getting a table

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3985 bytes --]

Hi Borislav,

[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.13-rc7 next-20170901]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Borislav-Petkov/ACPICA-Check-whether-ACPI-is-disabled-before-getting-a-table/20170823-041359
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   drivers/acpi/acpica/tbxface.c: In function 'acpi_get_table_header':
>> drivers/acpi/acpica/tbxface.c:254:6: error: 'acpi_disabled' undeclared (first use in this function)
     if (acpi_disabled)
         ^~~~~~~~~~~~~
   drivers/acpi/acpica/tbxface.c:254:6: note: each undeclared identifier is reported only once for each function it appears in

vim +/acpi_disabled +254 drivers/acpi/acpica/tbxface.c

   229	
   230	/*******************************************************************************
   231	 *
   232	 * FUNCTION:    acpi_get_table_header
   233	 *
   234	 * PARAMETERS:  signature           - ACPI signature of needed table
   235	 *              instance            - Which instance (for SSDTs)
   236	 *              out_table_header    - The pointer to the table header to fill
   237	 *
   238	 * RETURN:      Status and pointer to mapped table header
   239	 *
   240	 * DESCRIPTION: Finds an ACPI table header.
   241	 *
   242	 * NOTE:        Caller is responsible in unmapping the header with
   243	 *              acpi_os_unmap_memory
   244	 *
   245	 ******************************************************************************/
   246	acpi_status
   247	acpi_get_table_header(char *signature,
   248			      u32 instance, struct acpi_table_header *out_table_header)
   249	{
   250		u32 i;
   251		u32 j;
   252		struct acpi_table_header *header;
   253	
 > 254		if (acpi_disabled)
   255			return (AE_ERROR);
   256	
   257		/* Parameter validation */
   258	
   259		if (!signature || !out_table_header) {
   260			return (AE_BAD_PARAMETER);
   261		}
   262	
   263		/* Walk the root table list */
   264	
   265		for (i = 0, j = 0; i < acpi_gbl_root_table_list.current_table_count;
   266		     i++) {
   267			if (!ACPI_COMPARE_NAME
   268			    (&(acpi_gbl_root_table_list.tables[i].signature),
   269			     signature)) {
   270				continue;
   271			}
   272	
   273			if (++j < instance) {
   274				continue;
   275			}
   276	
   277			if (!acpi_gbl_root_table_list.tables[i].pointer) {
   278				if ((acpi_gbl_root_table_list.tables[i].flags &
   279				     ACPI_TABLE_ORIGIN_MASK) ==
   280				    ACPI_TABLE_ORIGIN_INTERNAL_PHYSICAL) {
   281					header =
   282					    acpi_os_map_memory(acpi_gbl_root_table_list.
   283							       tables[i].address,
   284							       sizeof(struct
   285								      acpi_table_header));
   286					if (!header) {
   287						return (AE_NO_MEMORY);
   288					}
   289	
   290					memcpy(out_table_header, header,
   291					       sizeof(struct acpi_table_header));
   292					acpi_os_unmap_memory(header,
   293							     sizeof(struct
   294								    acpi_table_header));
   295				} else {
   296					return (AE_NOT_FOUND);
   297				}
   298			} else {
   299				memcpy(out_table_header,
   300				       acpi_gbl_root_table_list.tables[i].pointer,
   301				       sizeof(struct acpi_table_header));
   302			}
   303			return (AE_OK);
   304		}
   305	
   306		return (AE_NOT_FOUND);
   307	}
   308	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36306 bytes --]

ACPICA: Check whether ACPI is disabled before getting a table

[kbuild test robot]

Hi Borislav,

[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.13-rc7 next-20170901]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Borislav-Petkov/ACPICA-Check-whether-ACPI-is-disabled-before-getting-a-table/20170823-041359
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   drivers/acpi/acpica/tbxface.c: In function 'acpi_get_table_header':
>> drivers/acpi/acpica/tbxface.c:254:6: error: 'acpi_disabled' undeclared (first use in this function)
     if (acpi_disabled)
         ^~~~~~~~~~~~~
   drivers/acpi/acpica/tbxface.c:254:6: note: each undeclared identifier is reported only once for each function it appears in

vim +/acpi_disabled +254 drivers/acpi/acpica/tbxface.c

   229	
   230	/*******************************************************************************
   231	 *
   232	 * FUNCTION:    acpi_get_table_header
   233	 *
   234	 * PARAMETERS:  signature           - ACPI signature of needed table
   235	 *              instance            - Which instance (for SSDTs)
   236	 *              out_table_header    - The pointer to the table header to fill
   237	 *
   238	 * RETURN:      Status and pointer to mapped table header
   239	 *
   240	 * DESCRIPTION: Finds an ACPI table header.
   241	 *
   242	 * NOTE:        Caller is responsible in unmapping the header with
   243	 *              acpi_os_unmap_memory
   244	 *
   245	 ******************************************************************************/
   246	acpi_status
   247	acpi_get_table_header(char *signature,
   248			      u32 instance, struct acpi_table_header *out_table_header)
   249	{
   250		u32 i;
   251		u32 j;
   252		struct acpi_table_header *header;
   253	
 > 254		if (acpi_disabled)
   255			return (AE_ERROR);
   256	
   257		/* Parameter validation */
   258	
   259		if (!signature || !out_table_header) {
   260			return (AE_BAD_PARAMETER);
   261		}
   262	
   263		/* Walk the root table list */
   264	
   265		for (i = 0, j = 0; i < acpi_gbl_root_table_list.current_table_count;
   266		     i++) {
   267			if (!ACPI_COMPARE_NAME
   268			    (&(acpi_gbl_root_table_list.tables[i].signature),
   269			     signature)) {
   270				continue;
   271			}
   272	
   273			if (++j < instance) {
   274				continue;
   275			}
   276	
   277			if (!acpi_gbl_root_table_list.tables[i].pointer) {
   278				if ((acpi_gbl_root_table_list.tables[i].flags &
   279				     ACPI_TABLE_ORIGIN_MASK) ==
   280				    ACPI_TABLE_ORIGIN_INTERNAL_PHYSICAL) {
   281					header =
   282					    acpi_os_map_memory(acpi_gbl_root_table_list.
   283							       tables[i].address,
   284							       sizeof(struct
   285								      acpi_table_header));
   286					if (!header) {
   287						return (AE_NO_MEMORY);
   288					}
   289	
   290					memcpy(out_table_header, header,
   291					       sizeof(struct acpi_table_header));
   292					acpi_os_unmap_memory(header,
   293							     sizeof(struct
   294								    acpi_table_header));
   295				} else {
   296					return (AE_NOT_FOUND);
   297				}
   298			} else {
   299				memcpy(out_table_header,
   300				       acpi_gbl_root_table_list.tables[i].pointer,
   301				       sizeof(struct acpi_table_header));
   302			}
   303			return (AE_OK);
   304		}
   305	
   306		return (AE_NOT_FOUND);
   307	}
   308
---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

[Kani, Toshimitsu]

On Mon, 2017-08-21 at 13:27 +0200, Borislav Petkov wrote:
> On Fri, Aug 18, 2017 at 01:46:40PM -0600, Toshi Kani wrote:
> > ACPI OEM ID / OEM Table ID / Revision can be used to identify
> > a platform based on ACPI firmware info.  acpi_blacklisted(),
> > intel_pstate_platform_pwr_mgmt_exists(), and some other funcs,
> > have been using similar check to detect a list of platforms
> > that require special handlings.
> > 
> > Move the platform check in acpi_blacklisted() to a new common
> > utility function, acpi_match_platform_list(), so that other
> > drivers do not have to implement their own version.
> > 
> > There is no change in functionality.
 :
> > +
> > +	for (; plat->oem_id[0]; plat++, idx++) {
> > +		if (ACPI_FAILURE(acpi_get_table_header(plat-
> > >table, 0, &hdr)))
> > +			continue;
> > +
> > +		if (strncmp(plat->oem_id, hdr.oem_id,
> > ACPI_OEM_ID_SIZE))
> > +			continue;
> > +
> > +		if (strncmp(plat->oem_table_id, hdr.oem_table_id,
> > +			    ACPI_OEM_TABLE_ID_SIZE))
> 
> Let that stick out.

Putting to a single line leads to "line over 80 characters" warning
from checkpatch.pl.  Would you still advice to do that?

> > +			continue;
> > +
> > +		if ((plat->pred == all_versions) ||
> > +		    (plat->pred == less_than_or_equal
> > +			&& hdr.oem_revision <= plat->oem_revision) 
> > ||
> > +		    (plat->pred == greater_than_or_equal
> > +			&& hdr.oem_revision >= plat->oem_revision) 
> > ||
> > +		    (plat->pred == equal
> > +			&& hdr.oem_revision == plat-
> > >oem_revision))
> > +			return idx;
> 
> Make that more readable:
> 
>                 if ((plat->pred == all_versions) ||
>                     (plat->pred == less_than_or_equal    &&
> hdr.oem_revision <= plat->oem_revision) ||
>                     (plat->pred == greater_than_or_equal &&
> hdr.oem_revision >= plat->oem_revision) ||
>                     (plat->pred == equal                 &&
> hdr.oem_revision == plat->oem_revision))
>                         return idx;

Same here.  These lead to checkpatch warnings.

> > +	}
> > +
> > +	return -ENODEV;
> > +}
> > +EXPORT_SYMBOL(acpi_match_platform_list);
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index 27b4b66..a9b6dc2 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -556,6 +556,25 @@ extern acpi_status
> > acpi_pci_osc_control_set(acpi_handle handle,
> >  #define ACPI_OST_SC_DRIVER_LOAD_FAILURE		0x81
> >  #define ACPI_OST_SC_INSERT_NOT_SUPPORTED	0x82
> >  
> > +enum acpi_predicate {
> > +	all_versions,
> > +	less_than_or_equal,
> > +	equal,
> > +	greater_than_or_equal,
> > +};
> > +
> > +/* Table must be terminted by a NULL entry */
> > +struct acpi_platform_list {
> > +	char	oem_id[ACPI_OEM_ID_SIZE];
> 
> 					+ 1
> 
> > +	char	oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
> 
> 						   + 1

strncmp() is fine without these, but it'd be prudent in case someone
decides to print these strings with printk().  Will do.

> > +	u32	oem_revision;
> > +	char	*table;
> > +	enum acpi_predicate pred;
> > +	char	*reason;
> > +	u32	data;
> 
> Ok, turning that into data from is_critical_error is a step in the
> right direction. Let's make it even better:
> 
> 	u32	flags;
> 
> and do
> 
> #define ACPI_PLAT_IS_CRITICAL_ERROR	BIT(0)
> 
> so that future elements add new bits instead of wasting a whole u32
> as a boolean.

'data' here is private to the caller.  So, I do not think we need to
define the bits.  Shall I change the name to 'driver_data' to make it
more explicit?

Thanks,
-Toshi

[v3,1/5] ACPI / blacklist: add acpi_match_platform_list()

[Toshi Kani]

On Mon, 2017-08-21 at 13:27 +0200, Borislav Petkov wrote:
> On Fri, Aug 18, 2017 at 01:46:40PM -0600, Toshi Kani wrote:
> > ACPI OEM ID / OEM Table ID / Revision can be used to identify
> > a platform based on ACPI firmware info.  acpi_blacklisted(),
> > intel_pstate_platform_pwr_mgmt_exists(), and some other funcs,
> > have been using similar check to detect a list of platforms
> > that require special handlings.
> > 
> > Move the platform check in acpi_blacklisted() to a new common
> > utility function, acpi_match_platform_list(), so that other
> > drivers do not have to implement their own version.
> > 
> > There is no change in functionality.
 :
> > +
> > +	for (; plat->oem_id[0]; plat++, idx++) {
> > +		if (ACPI_FAILURE(acpi_get_table_header(plat-
> > >table, 0, &hdr)))
> > +			continue;
> > +
> > +		if (strncmp(plat->oem_id, hdr.oem_id,
> > ACPI_OEM_ID_SIZE))
> > +			continue;
> > +
> > +		if (strncmp(plat->oem_table_id, hdr.oem_table_id,
> > +			    ACPI_OEM_TABLE_ID_SIZE))
> 
> Let that stick out.

Putting to a single line leads to "line over 80 characters" warning
from checkpatch.pl.  Would you still advice to do that?

> > +			continue;
> > +
> > +		if ((plat->pred == all_versions) ||
> > +		    (plat->pred == less_than_or_equal
> > +			&& hdr.oem_revision <= plat->oem_revision) 
> > ||
> > +		    (plat->pred == greater_than_or_equal
> > +			&& hdr.oem_revision >= plat->oem_revision) 
> > ||
> > +		    (plat->pred == equal
> > +			&& hdr.oem_revision == plat-
> > >oem_revision))
> > +			return idx;
> 
> Make that more readable:
> 
>                 if ((plat->pred == all_versions) ||
>                     (plat->pred == less_than_or_equal    &&
> hdr.oem_revision <= plat->oem_revision) ||
>                     (plat->pred == greater_than_or_equal &&
> hdr.oem_revision >= plat->oem_revision) ||
>                     (plat->pred == equal                 &&
> hdr.oem_revision == plat->oem_revision))
>                         return idx;

Same here.  These lead to checkpatch warnings.

> > +	}
> > +
> > +	return -ENODEV;
> > +}
> > +EXPORT_SYMBOL(acpi_match_platform_list);
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index 27b4b66..a9b6dc2 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -556,6 +556,25 @@ extern acpi_status
> > acpi_pci_osc_control_set(acpi_handle handle,
> >  #define ACPI_OST_SC_DRIVER_LOAD_FAILURE		0x81
> >  #define ACPI_OST_SC_INSERT_NOT_SUPPORTED	0x82
> >  
> > +enum acpi_predicate {
> > +	all_versions,
> > +	less_than_or_equal,
> > +	equal,
> > +	greater_than_or_equal,
> > +};
> > +
> > +/* Table must be terminted by a NULL entry */
> > +struct acpi_platform_list {
> > +	char	oem_id[ACPI_OEM_ID_SIZE];
> 
> 					+ 1
> 
> > +	char	oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
> 
> 						   + 1

strncmp() is fine without these, but it'd be prudent in case someone
decides to print these strings with printk().  Will do.

> > +	u32	oem_revision;
> > +	char	*table;
> > +	enum acpi_predicate pred;
> > +	char	*reason;
> > +	u32	data;
> 
> Ok, turning that into data from is_critical_error is a step in the
> right direction. Let's make it even better:
> 
> 	u32	flags;
> 
> and do
> 
> #define ACPI_PLAT_IS_CRITICAL_ERROR	BIT(0)
> 
> so that future elements add new bits instead of wasting a whole u32
> as a boolean.

'data' here is private to the caller.  So, I do not think we need to
define the bits.  Shall I change the name to 'driver_data' to make it
more explicit?

Thanks,
-Toshi

Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

[Borislav Petkov]

On Mon, Aug 21, 2017 at 04:41:38PM +0000, Kani, Toshimitsu wrote:
> Putting to a single line leads to "line over 80 characters" warning
> from checkpatch.pl.  Would you still advice to do that?

Yes, the 80 cols rule is not a hard one. Rather, it should be overridden
by human good judgement, like making the code more readable.

> strncmp() is fine without these, but it'd be prudent in case someone
> decides to print these strings with printk().  Will do.

Someone does already use them in printk():

+               pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\" Revision 0x%x has a known ACPI BIOS problem.\n",
+                      acpi_blacklist[i].oem_id,
+                      acpi_blacklist[i].oem_table_id,
+                      acpi_blacklist[i].oem_revision);


> 'data' here is private to the caller.  So, I do not think we need to
> define the bits.  Shall I change the name to 'driver_data' to make it
> more explicit?

You changed it to 'data'. It was a u32-used-as-boolean is_critical_error
before.

So you can just as well make it into flags and people can extend those
flags if needed. A flag bit should be enough in most cases anyway. If
they really need driver_data, then they can add a void * member.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[v3,1/5] ACPI / blacklist: add acpi_match_platform_list()

[Borislav Petkov]

On Mon, Aug 21, 2017 at 04:41:38PM +0000, Kani, Toshimitsu wrote:
> Putting to a single line leads to "line over 80 characters" warning
> from checkpatch.pl.  Would you still advice to do that?

Yes, the 80 cols rule is not a hard one. Rather, it should be overridden
by human good judgement, like making the code more readable.

> strncmp() is fine without these, but it'd be prudent in case someone
> decides to print these strings with printk().  Will do.

Someone does already use them in printk():

+               pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\" Revision 0x%x has a known ACPI BIOS problem.\n",
+                      acpi_blacklist[i].oem_id,
+                      acpi_blacklist[i].oem_table_id,
+                      acpi_blacklist[i].oem_revision);


> 'data' here is private to the caller.  So, I do not think we need to
> define the bits.  Shall I change the name to 'driver_data' to make it
> more explicit?

You changed it to 'data'. It was a u32-used-as-boolean is_critical_error
before.

So you can just as well make it into flags and people can extend those
flags if needed. A flag bit should be enough in most cases anyway. If
they really need driver_data, then they can add a void * member.

Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

[Kani, Toshimitsu]

On Mon, 2017-08-21 at 19:04 +0200, Borislav Petkov wrote:
> On Mon, Aug 21, 2017 at 04:41:38PM +0000, Kani, Toshimitsu wrote:
> > Putting to a single line leads to "line over 80 characters" warning
> > from checkpatch.pl.  Would you still advice to do that?
> 
> Yes, the 80 cols rule is not a hard one. Rather, it should be
> overridden by human good judgement, like making the code more
> readable.

I see.  I will make these changes.  (It's really personal preference,
but long lines of if-conditions are not so easy to read to my eyes,
though.)

> > strncmp() is fine without these, but it'd be prudent in case
> > someone decides to print these strings with printk().  Will do.
> 
> Someone does already use them in printk():
> 
> +               pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\"
> Revision 0x%x has a known ACPI BIOS problem.\n",
> +                      acpi_blacklist[i].oem_id,
> +                      acpi_blacklist[i].oem_table_id,
> +                      acpi_blacklist[i].oem_revision);

Oh, you are right about that!

> > 'data' here is private to the caller.  So, I do not think we need
> > to define the bits.  Shall I change the name to 'driver_data' to
> > make it more explicit?
> 
> You changed it to 'data'. It was a u32-used-as-boolean
> is_critical_error before.
> 
> So you can just as well make it into flags and people can extend
> those flags if needed. A flag bit should be enough in most cases
> anyway. If they really need driver_data, then they can add a void *
> member.

Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses this
field for PSS and PCC, which are enum values.  I think we should allow
drivers to set any values here.  I agree that it may need to be void *
if we also allow drivers to set a pointer here.

Thanks,
-Toshi

[v3,1/5] ACPI / blacklist: add acpi_match_platform_list()

[Toshi Kani]

On Mon, 2017-08-21 at 19:04 +0200, Borislav Petkov wrote:
> On Mon, Aug 21, 2017 at 04:41:38PM +0000, Kani, Toshimitsu wrote:
> > Putting to a single line leads to "line over 80 characters" warning
> > from checkpatch.pl.  Would you still advice to do that?
> 
> Yes, the 80 cols rule is not a hard one. Rather, it should be
> overridden by human good judgement, like making the code more
> readable.

I see.  I will make these changes.  (It's really personal preference,
but long lines of if-conditions are not so easy to read to my eyes,
though.)

> > strncmp() is fine without these, but it'd be prudent in case
> > someone decides to print these strings with printk().  Will do.
> 
> Someone does already use them in printk():
> 
> +               pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\"
> Revision 0x%x has a known ACPI BIOS problem.\n",
> +                      acpi_blacklist[i].oem_id,
> +                      acpi_blacklist[i].oem_table_id,
> +                      acpi_blacklist[i].oem_revision);

Oh, you are right about that!

> > 'data' here is private to the caller.  So, I do not think we need
> > to define the bits.  Shall I change the name to 'driver_data' to
> > make it more explicit?
> 
> You changed it to 'data'. It was a u32-used-as-boolean
> is_critical_error before.
> 
> So you can just as well make it into flags and people can extend
> those flags if needed. A flag bit should be enough in most cases
> anyway. If they really need driver_data, then they can add a void *
> member.

Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses this
field for PSS and PCC, which are enum values.  I think we should allow
drivers to set any values here.  I agree that it may need to be void *
if we also allow drivers to set a pointer here.

Thanks,
-Toshi

Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

[Borislav Petkov]

On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote:
> > > 'data' here is private to the caller.  So, I do not think we need
> > > to define the bits.  Shall I change the name to 'driver_data' to
> > > make it more explicit?
> > 
> > You changed it to 'data'. It was a u32-used-as-boolean
> > is_critical_error before.
> > 
> > So you can just as well make it into flags and people can extend
> > those flags if needed. A flag bit should be enough in most cases
> > anyway. If they really need driver_data, then they can add a void *
> > member.
> 
> Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses this
> field for PSS and PCC, which are enum values.  I think we should allow
> drivers to set any values here.  I agree that it may need to be void *
> if we also allow drivers to set a pointer here.

Let's see what Rafael prefers.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[v3,1/5] ACPI / blacklist: add acpi_match_platform_list()

[Borislav Petkov]

On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote:
> > > 'data' here is private to the caller.  So, I do not think we need
> > > to define the bits.  Shall I change the name to 'driver_data' to
> > > make it more explicit?
> > 
> > You changed it to 'data'. It was a u32-used-as-boolean
> > is_critical_error before.
> > 
> > So you can just as well make it into flags and people can extend
> > those flags if needed. A flag bit should be enough in most cases
> > anyway. If they really need driver_data, then they can add a void *
> > member.
> 
> Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses this
> field for PSS and PCC, which are enum values.  I think we should allow
> drivers to set any values here.  I agree that it may need to be void *
> if we also allow drivers to set a pointer here.

Let's see what Rafael prefers.

Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

[Rafael J. Wysocki]

On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote:
>> > > 'data' here is private to the caller.  So, I do not think we need
>> > > to define the bits.  Shall I change the name to 'driver_data' to
>> > > make it more explicit?
>> >
>> > You changed it to 'data'. It was a u32-used-as-boolean
>> > is_critical_error before.
>> >
>> > So you can just as well make it into flags and people can extend
>> > those flags if needed. A flag bit should be enough in most cases
>> > anyway. If they really need driver_data, then they can add a void *
>> > member.
>>
>> Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses this
>> field for PSS and PCC, which are enum values.  I think we should allow
>> drivers to set any values here.  I agree that it may need to be void *
>> if we also allow drivers to set a pointer here.
>
> Let's see what Rafael prefers.

I would retain the is_critical_error field and use that for printing
the recoverable / non-recoverable message.  This is kind of orthogonal
to whether or not any extra data is needed and that can be an
additional field.  In that case unsigned long should be sufficient to
accommodate a pointer if need be.

Thanks,
Rafael

[v3,1/5] ACPI / blacklist: add acpi_match_platform_list()

[Rafael J. Wysocki]

On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote:
>> > > 'data' here is private to the caller.  So, I do not think we need
>> > > to define the bits.  Shall I change the name to 'driver_data' to
>> > > make it more explicit?
>> >
>> > You changed it to 'data'. It was a u32-used-as-boolean
>> > is_critical_error before.
>> >
>> > So you can just as well make it into flags and people can extend
>> > those flags if needed. A flag bit should be enough in most cases
>> > anyway. If they really need driver_data, then they can add a void *
>> > member.
>>
>> Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses this
>> field for PSS and PCC, which are enum values.  I think we should allow
>> drivers to set any values here.  I agree that it may need to be void *
>> if we also allow drivers to set a pointer here.
>
> Let's see what Rafael prefers.

I would retain the is_critical_error field and use that for printing
the recoverable / non-recoverable message.  This is kind of orthogonal
to whether or not any extra data is needed and that can be an
additional field.  In that case unsigned long should be sufficient to
accommodate a pointer if need be.

Thanks,
Rafael
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

[Kani, Toshimitsu]

On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote:
> On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de>
> wrote:
> > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote:
> > > > > 'data' here is private to the caller.  So, I do not think we
> > > > > need to define the bits.  Shall I change the name to
> > > > > 'driver_data' to make it more explicit?
> > > > 
> > > > You changed it to 'data'. It was a u32-used-as-boolean
> > > > is_critical_error before.
> > > > 
> > > > So you can just as well make it into flags and people can
> > > > extend those flags if needed. A flag bit should be enough in
> > > > most cases anyway. If they really need driver_data, then they
> > > > can add a void *member.
> > > 
> > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses
> > > this field for PSS and PCC, which are enum values.  I think we
> > > should allow drivers to set any values here.  I agree that it may
> > > need to be void * if we also allow drivers to set a pointer here.
> > 
> > Let's see what Rafael prefers.
> 
> I would retain the is_critical_error field and use that for printing
> the recoverable / non-recoverable message.  This is kind of
> orthogonal to whether or not any extra data is needed and that can be
> an additional field.  In that case unsigned long should be sufficient
> to accommodate a pointer if need be.

Yes, we will retain the field.  The question is whether this field
should be retained as a driver's private data or ACPI-managed flags.  

My patch implements the former, which lets the callers to define the
data values.  For instance, acpi_blacklisted() uses this field as
is_critical_error value, and intel_pstate_platform_pwr_mgmt_exists()
uses it as oem_pwr_table value.

Boris suggested the latter, which lets ACPI to define the flags, which
are then used by the callers.  For instance, he suggested ACPI to
define bit0 as is_critical_error.

#define ACPI_PLAT_IS_CRITICAL_ERROR     BIT(0)

Thanks,
-Toshi

[v3,1/5] ACPI / blacklist: add acpi_match_platform_list()

[Toshi Kani]

On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote:
> On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de>
> wrote:
> > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote:
> > > > > 'data' here is private to the caller.  So, I do not think we
> > > > > need to define the bits.  Shall I change the name to
> > > > > 'driver_data' to make it more explicit?
> > > > 
> > > > You changed it to 'data'. It was a u32-used-as-boolean
> > > > is_critical_error before.
> > > > 
> > > > So you can just as well make it into flags and people can
> > > > extend those flags if needed. A flag bit should be enough in
> > > > most cases anyway. If they really need driver_data, then they
> > > > can add a void *member.
> > > 
> > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses
> > > this field for PSS and PCC, which are enum values.  I think we
> > > should allow drivers to set any values here.  I agree that it may
> > > need to be void * if we also allow drivers to set a pointer here.
> > 
> > Let's see what Rafael prefers.
> 
> I would retain the is_critical_error field and use that for printing
> the recoverable / non-recoverable message.  This is kind of
> orthogonal to whether or not any extra data is needed and that can be
> an additional field.  In that case unsigned long should be sufficient
> to accommodate a pointer if need be.

Yes, we will retain the field.  The question is whether this field
should be retained as a driver's private data or ACPI-managed flags.  

My patch implements the former, which lets the callers to define the
data values.  For instance, acpi_blacklisted() uses this field as
is_critical_error value, and intel_pstate_platform_pwr_mgmt_exists()
uses it as oem_pwr_table value.

Boris suggested the latter, which lets ACPI to define the flags, which
are then used by the callers.  For instance, he suggested ACPI to
define bit0 as is_critical_error.

#define ACPI_PLAT_IS_CRITICAL_ERROR     BIT(0)

Thanks,
-Toshi

Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

[Rafael J. Wysocki]

On Mon, Aug 21, 2017 at 11:06 PM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
> On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote:
>> On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de>
>> wrote:
>> > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote:
>> > > > > 'data' here is private to the caller.  So, I do not think we
>> > > > > need to define the bits.  Shall I change the name to
>> > > > > 'driver_data' to make it more explicit?
>> > > >
>> > > > You changed it to 'data'. It was a u32-used-as-boolean
>> > > > is_critical_error before.
>> > > >
>> > > > So you can just as well make it into flags and people can
>> > > > extend those flags if needed. A flag bit should be enough in
>> > > > most cases anyway. If they really need driver_data, then they
>> > > > can add a void *member.
>> > >
>> > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses
>> > > this field for PSS and PCC, which are enum values.  I think we
>> > > should allow drivers to set any values here.  I agree that it may
>> > > need to be void * if we also allow drivers to set a pointer here.
>> >
>> > Let's see what Rafael prefers.
>>
>> I would retain the is_critical_error field and use that for printing
>> the recoverable / non-recoverable message.  This is kind of
>> orthogonal to whether or not any extra data is needed and that can be
>> an additional field.  In that case unsigned long should be sufficient
>> to accommodate a pointer if need be.
>
> Yes, we will retain the field.  The question is whether this field
> should be retained as a driver's private data or ACPI-managed flags.

Thanks for the clarification.

> My patch implements the former, which lets the callers to define the
> data values.  For instance, acpi_blacklisted() uses this field as
> is_critical_error value, and intel_pstate_platform_pwr_mgmt_exists()
> uses it as oem_pwr_table value.
>
> Boris suggested the latter, which lets ACPI to define the flags, which
> are then used by the callers.  For instance, he suggested ACPI to
> define bit0 as is_critical_error.
>
> #define ACPI_PLAT_IS_CRITICAL_ERROR     BIT(0)

So my point is that we can have both the ACPI-managed flags and the
the caller-defined data at the same time as separate items.

That would allow of maximum flexibility IMO.

Thanks,
Rafael

[v3,1/5] ACPI / blacklist: add acpi_match_platform_list()

[Rafael J. Wysocki]

On Mon, Aug 21, 2017 at 11:06 PM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
> On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote:
>> On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de>
>> wrote:
>> > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote:
>> > > > > 'data' here is private to the caller.  So, I do not think we
>> > > > > need to define the bits.  Shall I change the name to
>> > > > > 'driver_data' to make it more explicit?
>> > > >
>> > > > You changed it to 'data'. It was a u32-used-as-boolean
>> > > > is_critical_error before.
>> > > >
>> > > > So you can just as well make it into flags and people can
>> > > > extend those flags if needed. A flag bit should be enough in
>> > > > most cases anyway. If they really need driver_data, then they
>> > > > can add a void *member.
>> > >
>> > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses
>> > > this field for PSS and PCC, which are enum values.  I think we
>> > > should allow drivers to set any values here.  I agree that it may
>> > > need to be void * if we also allow drivers to set a pointer here.
>> >
>> > Let's see what Rafael prefers.
>>
>> I would retain the is_critical_error field and use that for printing
>> the recoverable / non-recoverable message.  This is kind of
>> orthogonal to whether or not any extra data is needed and that can be
>> an additional field.  In that case unsigned long should be sufficient
>> to accommodate a pointer if need be.
>
> Yes, we will retain the field.  The question is whether this field
> should be retained as a driver's private data or ACPI-managed flags.

Thanks for the clarification.

> My patch implements the former, which lets the callers to define the
> data values.  For instance, acpi_blacklisted() uses this field as
> is_critical_error value, and intel_pstate_platform_pwr_mgmt_exists()
> uses it as oem_pwr_table value.
>
> Boris suggested the latter, which lets ACPI to define the flags, which
> are then used by the callers.  For instance, he suggested ACPI to
> define bit0 as is_critical_error.
>
> #define ACPI_PLAT_IS_CRITICAL_ERROR     BIT(0)

So my point is that we can have both the ACPI-managed flags and the
the caller-defined data at the same time as separate items.

That would allow of maximum flexibility IMO.

Thanks,
Rafael
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

[Kani, Toshimitsu]

On Mon, 2017-08-21 at 23:49 +0200, Rafael J. Wysocki wrote:
> On Mon, Aug 21, 2017 at 11:06 PM, Kani, Toshimitsu <toshi.kani@hpe.co
> m> wrote:
> > On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote:
> > > On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de>
> > > wrote:
> > > > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu
> > > > wrote:
> > > > > > > 'data' here is private to the caller.  So, I do not think
> > > > > > > we need to define the bits.  Shall I change the name to
> > > > > > > 'driver_data' to make it more explicit?
> > > > > > 
> > > > > > You changed it to 'data'. It was a u32-used-as-boolean
> > > > > > is_critical_error before.
> > > > > > 
> > > > > > So you can just as well make it into flags and people can
> > > > > > extend those flags if needed. A flag bit should be enough
> > > > > > in most cases anyway. If they really need driver_data, then
> > > > > > they can add a void *member.
> > > > > 
> > > > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists()
> > > > > uses this field for PSS and PCC, which are enum values.  I
> > > > > think we should allow drivers to set any values here.  I
> > > > > agree that it may need to be void * if we also allow drivers
> > > > > to set a pointer here.
> > > > 
> > > > Let's see what Rafael prefers.
> > > 
> > > I would retain the is_critical_error field and use that for
> > > printing the recoverable / non-recoverable message.  This is kind
> > > of orthogonal to whether or not any extra data is needed and that
> > > can be an additional field.  In that case unsigned long should be
> > > sufficient to accommodate a pointer if need be.
> > 
> > Yes, we will retain the field.  The question is whether this field
> > should be retained as a driver's private data or ACPI-managed
> > flags.
> 
> Thanks for the clarification.
> 
> > My patch implements the former, which lets the callers to define
> > the data values.  For instance, acpi_blacklisted() uses this field
> > as is_critical_error value, and
> > intel_pstate_platform_pwr_mgmt_exists() uses it as oem_pwr_table
> > value.
> > 
> > Boris suggested the latter, which lets ACPI to define the flags,
> > which are then used by the callers.  For instance, he suggested
> > ACPI to define bit0 as is_critical_error.
> > 
> > #define ACPI_PLAT_IS_CRITICAL_ERROR     BIT(0)
> 
> So my point is that we can have both the ACPI-managed flags and the
> the caller-defined data at the same time as separate items.
> 
> That would allow of maximum flexibility IMO.

I agree in general.  Driver private data allows flexibility to drivers
when the values are driver-private.  ACPI-managed flags allows ACPI to
control the interfaces based on the flags.

Since we do not have use-case of the latter case yet, i.e.
acpi_match_platform_list() does not need to check the flags, I'd
suggest that we keep 'data' as driver-private.  We can add 'flags' as a
separate member to the structure when we find the latter use-case.

Thanks,
-Toshi

[v3,1/5] ACPI / blacklist: add acpi_match_platform_list()

[Toshi Kani]

On Mon, 2017-08-21 at 23:49 +0200, Rafael J. Wysocki wrote:
> On Mon, Aug 21, 2017 at 11:06 PM, Kani, Toshimitsu <toshi.kani@hpe.co
> m> wrote:
> > On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote:
> > > On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de>
> > > wrote:
> > > > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu
> > > > wrote:
> > > > > > > 'data' here is private to the caller.  So, I do not think
> > > > > > > we need to define the bits.  Shall I change the name to
> > > > > > > 'driver_data' to make it more explicit?
> > > > > > 
> > > > > > You changed it to 'data'. It was a u32-used-as-boolean
> > > > > > is_critical_error before.
> > > > > > 
> > > > > > So you can just as well make it into flags and people can
> > > > > > extend those flags if needed. A flag bit should be enough
> > > > > > in most cases anyway. If they really need driver_data, then
> > > > > > they can add a void *member.
> > > > > 
> > > > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists()
> > > > > uses this field for PSS and PCC, which are enum values.  I
> > > > > think we should allow drivers to set any values here.  I
> > > > > agree that it may need to be void * if we also allow drivers
> > > > > to set a pointer here.
> > > > 
> > > > Let's see what Rafael prefers.
> > > 
> > > I would retain the is_critical_error field and use that for
> > > printing the recoverable / non-recoverable message.  This is kind
> > > of orthogonal to whether or not any extra data is needed and that
> > > can be an additional field.  In that case unsigned long should be
> > > sufficient to accommodate a pointer if need be.
> > 
> > Yes, we will retain the field.  The question is whether this field
> > should be retained as a driver's private data or ACPI-managed
> > flags.
> 
> Thanks for the clarification.
> 
> > My patch implements the former, which lets the callers to define
> > the data values.  For instance, acpi_blacklisted() uses this field
> > as is_critical_error value, and
> > intel_pstate_platform_pwr_mgmt_exists() uses it as oem_pwr_table
> > value.
> > 
> > Boris suggested the latter, which lets ACPI to define the flags,
> > which are then used by the callers.  For instance, he suggested
> > ACPI to define bit0 as is_critical_error.
> > 
> > #define ACPI_PLAT_IS_CRITICAL_ERROR     BIT(0)
> 
> So my point is that we can have both the ACPI-managed flags and the
> the caller-defined data at the same time as separate items.
> 
> That would allow of maximum flexibility IMO.

I agree in general.  Driver private data allows flexibility to drivers
when the values are driver-private.  ACPI-managed flags allows ACPI to
control the interfaces based on the flags.

Since we do not have use-case of the latter case yet, i.e.
acpi_match_platform_list() does not need to check the flags, I'd
suggest that we keep 'data' as driver-private.  We can add 'flags' as a
separate member to the structure when we find the latter use-case.

Thanks,
-Toshi

Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

[Rafael J. Wysocki]

On Tue, Aug 22, 2017 at 12:21 AM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
> On Mon, 2017-08-21 at 23:49 +0200, Rafael J. Wysocki wrote:
>> On Mon, Aug 21, 2017 at 11:06 PM, Kani, Toshimitsu <toshi.kani@hpe.co
>> m> wrote:
>> > On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote:
>> > > On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de>
>> > > wrote:
>> > > > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu
>> > > > wrote:
>> > > > > > > 'data' here is private to the caller.  So, I do not think
>> > > > > > > we need to define the bits.  Shall I change the name to
>> > > > > > > 'driver_data' to make it more explicit?
>> > > > > >
>> > > > > > You changed it to 'data'. It was a u32-used-as-boolean
>> > > > > > is_critical_error before.
>> > > > > >
>> > > > > > So you can just as well make it into flags and people can
>> > > > > > extend those flags if needed. A flag bit should be enough
>> > > > > > in most cases anyway. If they really need driver_data, then
>> > > > > > they can add a void *member.
>> > > > >
>> > > > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists()
>> > > > > uses this field for PSS and PCC, which are enum values.  I
>> > > > > think we should allow drivers to set any values here.  I
>> > > > > agree that it may need to be void * if we also allow drivers
>> > > > > to set a pointer here.
>> > > >
>> > > > Let's see what Rafael prefers.
>> > >
>> > > I would retain the is_critical_error field and use that for
>> > > printing the recoverable / non-recoverable message.  This is kind
>> > > of orthogonal to whether or not any extra data is needed and that
>> > > can be an additional field.  In that case unsigned long should be
>> > > sufficient to accommodate a pointer if need be.
>> >
>> > Yes, we will retain the field.  The question is whether this field
>> > should be retained as a driver's private data or ACPI-managed
>> > flags.
>>
>> Thanks for the clarification.
>>
>> > My patch implements the former, which lets the callers to define
>> > the data values.  For instance, acpi_blacklisted() uses this field
>> > as is_critical_error value, and
>> > intel_pstate_platform_pwr_mgmt_exists() uses it as oem_pwr_table
>> > value.
>> >
>> > Boris suggested the latter, which lets ACPI to define the flags,
>> > which are then used by the callers.  For instance, he suggested
>> > ACPI to define bit0 as is_critical_error.
>> >
>> > #define ACPI_PLAT_IS_CRITICAL_ERROR     BIT(0)
>>
>> So my point is that we can have both the ACPI-managed flags and the
>> the caller-defined data at the same time as separate items.
>>
>> That would allow of maximum flexibility IMO.
>
> I agree in general.  Driver private data allows flexibility to drivers
> when the values are driver-private.  ACPI-managed flags allows ACPI to
> control the interfaces based on the flags.
>
> Since we do not have use-case of the latter case yet, i.e.
> acpi_match_platform_list() does not need to check the flags, I'd
> suggest that we keep 'data' as driver-private.  We can add 'flags' as a
> separate member to the structure when we find the latter use-case.

OK

Thanks,
Rafael

[v3,1/5] ACPI / blacklist: add acpi_match_platform_list()

[Rafael J. Wysocki]

On Tue, Aug 22, 2017 at 12:21 AM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
> On Mon, 2017-08-21 at 23:49 +0200, Rafael J. Wysocki wrote:
>> On Mon, Aug 21, 2017 at 11:06 PM, Kani, Toshimitsu <toshi.kani@hpe.co
>> m> wrote:
>> > On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote:
>> > > On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de>
>> > > wrote:
>> > > > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu
>> > > > wrote:
>> > > > > > > 'data' here is private to the caller.  So, I do not think
>> > > > > > > we need to define the bits.  Shall I change the name to
>> > > > > > > 'driver_data' to make it more explicit?
>> > > > > >
>> > > > > > You changed it to 'data'. It was a u32-used-as-boolean
>> > > > > > is_critical_error before.
>> > > > > >
>> > > > > > So you can just as well make it into flags and people can
>> > > > > > extend those flags if needed. A flag bit should be enough
>> > > > > > in most cases anyway. If they really need driver_data, then
>> > > > > > they can add a void *member.
>> > > > >
>> > > > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists()
>> > > > > uses this field for PSS and PCC, which are enum values.  I
>> > > > > think we should allow drivers to set any values here.  I
>> > > > > agree that it may need to be void * if we also allow drivers
>> > > > > to set a pointer here.
>> > > >
>> > > > Let's see what Rafael prefers.
>> > >
>> > > I would retain the is_critical_error field and use that for
>> > > printing the recoverable / non-recoverable message.  This is kind
>> > > of orthogonal to whether or not any extra data is needed and that
>> > > can be an additional field.  In that case unsigned long should be
>> > > sufficient to accommodate a pointer if need be.
>> >
>> > Yes, we will retain the field.  The question is whether this field
>> > should be retained as a driver's private data or ACPI-managed
>> > flags.
>>
>> Thanks for the clarification.
>>
>> > My patch implements the former, which lets the callers to define
>> > the data values.  For instance, acpi_blacklisted() uses this field
>> > as is_critical_error value, and
>> > intel_pstate_platform_pwr_mgmt_exists() uses it as oem_pwr_table
>> > value.
>> >
>> > Boris suggested the latter, which lets ACPI to define the flags,
>> > which are then used by the callers.  For instance, he suggested
>> > ACPI to define bit0 as is_critical_error.
>> >
>> > #define ACPI_PLAT_IS_CRITICAL_ERROR     BIT(0)
>>
>> So my point is that we can have both the ACPI-managed flags and the
>> the caller-defined data at the same time as separate items.
>>
>> That would allow of maximum flexibility IMO.
>
> I agree in general.  Driver private data allows flexibility to drivers
> when the values are driver-private.  ACPI-managed flags allows ACPI to
> control the interfaces based on the flags.
>
> Since we do not have use-case of the latter case yet, i.e.
> acpi_match_platform_list() does not need to check the flags, I'd
> suggest that we keep 'data' as driver-private.  We can add 'flags' as a
> separate member to the structure when we find the latter use-case.

OK

Thanks,
Rafael
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/5] intel_pstate: convert to use acpi_match_platform_list()

[Toshi Kani]

Convert to use acpi_match_platform_list() for the platform check.
There is no change in functionality.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Len Brown <lenb@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
---
 drivers/cpufreq/intel_pstate.c |   64 ++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 39 deletions(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 65ee4fc..ad713cd 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -2466,39 +2466,31 @@ enum {
 	PPC,
 };
 
-struct hw_vendor_info {
-	u16  valid;
-	char oem_id[ACPI_OEM_ID_SIZE];
-	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
-	int  oem_pwr_table;
-};
-
 /* Hardware vendor-specific info that has its own power management modes */
-static struct hw_vendor_info vendor_info[] __initdata = {
-	{1, "HP    ", "ProLiant", PSS},
-	{1, "ORACLE", "X4-2    ", PPC},
-	{1, "ORACLE", "X4-2L   ", PPC},
-	{1, "ORACLE", "X4-2B   ", PPC},
-	{1, "ORACLE", "X3-2    ", PPC},
-	{1, "ORACLE", "X3-2L   ", PPC},
-	{1, "ORACLE", "X3-2B   ", PPC},
-	{1, "ORACLE", "X4470M2 ", PPC},
-	{1, "ORACLE", "X4270M3 ", PPC},
-	{1, "ORACLE", "X4270M2 ", PPC},
-	{1, "ORACLE", "X4170M2 ", PPC},
-	{1, "ORACLE", "X4170 M3", PPC},
-	{1, "ORACLE", "X4275 M3", PPC},
-	{1, "ORACLE", "X6-2    ", PPC},
-	{1, "ORACLE", "Sudbury ", PPC},
-	{0, "", ""},
+static struct acpi_platform_list plat_info[] __initdata = {
+	{"HP    ", "ProLiant", 0, ACPI_SIG_FADT, all_versions, 0, PSS},
+	{"ORACLE", "X4-2    ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4-2L   ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4-2B   ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X3-2    ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X3-2L   ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X3-2B   ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4470M2 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4270M3 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4270M2 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4170M2 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4170 M3", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4275 M3", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X6-2    ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "Sudbury ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{ } /* End */
 };
 
 static bool __init intel_pstate_platform_pwr_mgmt_exists(void)
 {
-	struct acpi_table_header hdr;
-	struct hw_vendor_info *v_info;
 	const struct x86_cpu_id *id;
 	u64 misc_pwr;
+	int idx;
 
 	id = x86_match_cpu(intel_pstate_cpu_oob_ids);
 	if (id) {
@@ -2507,21 +2499,15 @@ static bool __init intel_pstate_platform_pwr_mgmt_exists(void)
 			return true;
 	}
 
-	if (acpi_disabled ||
-	    ACPI_FAILURE(acpi_get_table_header(ACPI_SIG_FADT, 0, &hdr)))
+	idx = acpi_match_platform_list(plat_info);
+	if (idx < 0)
 		return false;
 
-	for (v_info = vendor_info; v_info->valid; v_info++) {
-		if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
-			!strncmp(hdr.oem_table_id, v_info->oem_table_id,
-						ACPI_OEM_TABLE_ID_SIZE))
-			switch (v_info->oem_pwr_table) {
-			case PSS:
-				return intel_pstate_no_acpi_pss();
-			case PPC:
-				return intel_pstate_has_acpi_ppc() &&
-					(!force_load);
-			}
+	switch (plat_info[idx].data) {
+	case PSS:
+		return intel_pstate_no_acpi_pss();
+	case PPC:
+		return intel_pstate_has_acpi_ppc() && !force_load;
 	}
 
 	return false;

[v3,2/5] intel_pstate: convert to use acpi_match_platform_list()

[Toshi Kani]

Convert to use acpi_match_platform_list() for the platform check.
There is no change in functionality.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Len Brown <lenb@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
---
 drivers/cpufreq/intel_pstate.c |   64 ++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 39 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 65ee4fc..ad713cd 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -2466,39 +2466,31 @@ enum {
 	PPC,
 };
 
-struct hw_vendor_info {
-	u16  valid;
-	char oem_id[ACPI_OEM_ID_SIZE];
-	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
-	int  oem_pwr_table;
-};
-
 /* Hardware vendor-specific info that has its own power management modes */
-static struct hw_vendor_info vendor_info[] __initdata = {
-	{1, "HP    ", "ProLiant", PSS},
-	{1, "ORACLE", "X4-2    ", PPC},
-	{1, "ORACLE", "X4-2L   ", PPC},
-	{1, "ORACLE", "X4-2B   ", PPC},
-	{1, "ORACLE", "X3-2    ", PPC},
-	{1, "ORACLE", "X3-2L   ", PPC},
-	{1, "ORACLE", "X3-2B   ", PPC},
-	{1, "ORACLE", "X4470M2 ", PPC},
-	{1, "ORACLE", "X4270M3 ", PPC},
-	{1, "ORACLE", "X4270M2 ", PPC},
-	{1, "ORACLE", "X4170M2 ", PPC},
-	{1, "ORACLE", "X4170 M3", PPC},
-	{1, "ORACLE", "X4275 M3", PPC},
-	{1, "ORACLE", "X6-2    ", PPC},
-	{1, "ORACLE", "Sudbury ", PPC},
-	{0, "", ""},
+static struct acpi_platform_list plat_info[] __initdata = {
+	{"HP    ", "ProLiant", 0, ACPI_SIG_FADT, all_versions, 0, PSS},
+	{"ORACLE", "X4-2    ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4-2L   ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4-2B   ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X3-2    ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X3-2L   ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X3-2B   ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4470M2 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4270M3 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4270M2 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4170M2 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4170 M3", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X4275 M3", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "X6-2    ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{"ORACLE", "Sudbury ", 0, ACPI_SIG_FADT, all_versions, 0, PPC},
+	{ } /* End */
 };
 
 static bool __init intel_pstate_platform_pwr_mgmt_exists(void)
 {
-	struct acpi_table_header hdr;
-	struct hw_vendor_info *v_info;
 	const struct x86_cpu_id *id;
 	u64 misc_pwr;
+	int idx;
 
 	id = x86_match_cpu(intel_pstate_cpu_oob_ids);
 	if (id) {
@@ -2507,21 +2499,15 @@ static bool __init intel_pstate_platform_pwr_mgmt_exists(void)
 			return true;
 	}
 
-	if (acpi_disabled ||
-	    ACPI_FAILURE(acpi_get_table_header(ACPI_SIG_FADT, 0, &hdr)))
+	idx = acpi_match_platform_list(plat_info);
+	if (idx < 0)
 		return false;
 
-	for (v_info = vendor_info; v_info->valid; v_info++) {
-		if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
-			!strncmp(hdr.oem_table_id, v_info->oem_table_id,
-						ACPI_OEM_TABLE_ID_SIZE))
-			switch (v_info->oem_pwr_table) {
-			case PSS:
-				return intel_pstate_no_acpi_pss();
-			case PPC:
-				return intel_pstate_has_acpi_ppc() &&
-					(!force_load);
-			}
+	switch (plat_info[idx].data) {
+	case PSS:
+		return intel_pstate_no_acpi_pss();
+	case PPC:
+		return intel_pstate_has_acpi_ppc() && !force_load;
 	}
 
 	return false;

Re: [PATCH v3 2/5] intel_pstate: convert to use acpi_match_platform_list()

[Srinivas Pandruvada]

On Fri, 2017-08-18 at 13:46 -0600, Toshi Kani wrote:
> Convert to use acpi_match_platform_list() for the platform check.
> There is no change in functionality.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Borislav Petkov <bp@alien8.de>
Can't test but the change itself is OK.

Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
>  drivers/cpufreq/intel_pstate.c |   64 ++++++++++++++++------------
> ------------
>  1 file changed, 25 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/cpufreq/intel_pstate.c
> b/drivers/cpufreq/intel_pstate.c
> index 65ee4fc..ad713cd 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2466,39 +2466,31 @@ enum {
>  	PPC,
>  };
>  
> -struct hw_vendor_info {
> -	u16  valid;
> -	char oem_id[ACPI_OEM_ID_SIZE];
> -	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
> -	int  oem_pwr_table;
> -};
> -
>  /* Hardware vendor-specific info that has its own power management
> modes */
> -static struct hw_vendor_info vendor_info[] __initdata = {
> -	{1, "HP    ", "ProLiant", PSS},
> -	{1, "ORACLE", "X4-2    ", PPC},
> -	{1, "ORACLE", "X4-2L   ", PPC},
> -	{1, "ORACLE", "X4-2B   ", PPC},
> -	{1, "ORACLE", "X3-2    ", PPC},
> -	{1, "ORACLE", "X3-2L   ", PPC},
> -	{1, "ORACLE", "X3-2B   ", PPC},
> -	{1, "ORACLE", "X4470M2 ", PPC},
> -	{1, "ORACLE", "X4270M3 ", PPC},
> -	{1, "ORACLE", "X4270M2 ", PPC},
> -	{1, "ORACLE", "X4170M2 ", PPC},
> -	{1, "ORACLE", "X4170 M3", PPC},
> -	{1, "ORACLE", "X4275 M3", PPC},
> -	{1, "ORACLE", "X6-2    ", PPC},
> -	{1, "ORACLE", "Sudbury ", PPC},
> -	{0, "", ""},
> +static struct acpi_platform_list plat_info[] __initdata = {
> +	{"HP    ", "ProLiant", 0, ACPI_SIG_FADT, all_versions, 0,
> PSS},
> +	{"ORACLE", "X4-2    ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4-2L   ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4-2B   ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X3-2    ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X3-2L   ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X3-2B   ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4470M2 ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4270M3 ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4270M2 ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4170M2 ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4170 M3", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4275 M3", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X6-2    ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "Sudbury ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{ } /* End */
>  };
>  
>  static bool __init intel_pstate_platform_pwr_mgmt_exists(void)
>  {
> -	struct acpi_table_header hdr;
> -	struct hw_vendor_info *v_info;
>  	const struct x86_cpu_id *id;
>  	u64 misc_pwr;
> +	int idx;
>  
>  	id = x86_match_cpu(intel_pstate_cpu_oob_ids);
>  	if (id) {
> @@ -2507,21 +2499,15 @@ static bool __init
> intel_pstate_platform_pwr_mgmt_exists(void)
>  			return true;
>  	}
>  
> -	if (acpi_disabled ||
> -	    ACPI_FAILURE(acpi_get_table_header(ACPI_SIG_FADT, 0,
> &hdr)))
> +	idx = acpi_match_platform_list(plat_info);
> +	if (idx < 0)
>  		return false;
>  
> -	for (v_info = vendor_info; v_info->valid; v_info++) {
> -		if (!strncmp(hdr.oem_id, v_info->oem_id,
> ACPI_OEM_ID_SIZE) &&
> -			!strncmp(hdr.oem_table_id, v_info-
> >oem_table_id,
> -						ACPI_OEM_TABLE_ID_SI
> ZE))
> -			switch (v_info->oem_pwr_table) {
> -			case PSS:
> -				return intel_pstate_no_acpi_pss();
> -			case PPC:
> -				return intel_pstate_has_acpi_ppc()
> &&
> -					(!force_load);
> -			}
> +	switch (plat_info[idx].data) {
> +	case PSS:
> +		return intel_pstate_no_acpi_pss();
> +	case PPC:
> +		return intel_pstate_has_acpi_ppc() && !force_load;
>  	}
>  
>  	return false;

[v3,2/5] intel_pstate: convert to use acpi_match_platform_list()

[Srinivas Pandruvada]

On Fri, 2017-08-18 at 13:46 -0600, Toshi Kani wrote:
> Convert to use acpi_match_platform_list() for the platform check.
> There is no change in functionality.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Borislav Petkov <bp@alien8.de>
Can't test but the change itself is OK.

Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
>  drivers/cpufreq/intel_pstate.c |   64 ++++++++++++++++------------
> ------------
>  1 file changed, 25 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/cpufreq/intel_pstate.c
> b/drivers/cpufreq/intel_pstate.c
> index 65ee4fc..ad713cd 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2466,39 +2466,31 @@ enum {
>  	PPC,
>  };
>  
> -struct hw_vendor_info {
> -	u16  valid;
> -	char oem_id[ACPI_OEM_ID_SIZE];
> -	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
> -	int  oem_pwr_table;
> -};
> -
>  /* Hardware vendor-specific info that has its own power management
> modes */
> -static struct hw_vendor_info vendor_info[] __initdata = {
> -	{1, "HP    ", "ProLiant", PSS},
> -	{1, "ORACLE", "X4-2    ", PPC},
> -	{1, "ORACLE", "X4-2L   ", PPC},
> -	{1, "ORACLE", "X4-2B   ", PPC},
> -	{1, "ORACLE", "X3-2    ", PPC},
> -	{1, "ORACLE", "X3-2L   ", PPC},
> -	{1, "ORACLE", "X3-2B   ", PPC},
> -	{1, "ORACLE", "X4470M2 ", PPC},
> -	{1, "ORACLE", "X4270M3 ", PPC},
> -	{1, "ORACLE", "X4270M2 ", PPC},
> -	{1, "ORACLE", "X4170M2 ", PPC},
> -	{1, "ORACLE", "X4170 M3", PPC},
> -	{1, "ORACLE", "X4275 M3", PPC},
> -	{1, "ORACLE", "X6-2    ", PPC},
> -	{1, "ORACLE", "Sudbury ", PPC},
> -	{0, "", ""},
> +static struct acpi_platform_list plat_info[] __initdata = {
> +	{"HP    ", "ProLiant", 0, ACPI_SIG_FADT, all_versions, 0,
> PSS},
> +	{"ORACLE", "X4-2    ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4-2L   ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4-2B   ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X3-2    ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X3-2L   ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X3-2B   ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4470M2 ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4270M3 ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4270M2 ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4170M2 ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4170 M3", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X4275 M3", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "X6-2    ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{"ORACLE", "Sudbury ", 0, ACPI_SIG_FADT, all_versions, 0,
> PPC},
> +	{ } /* End */
>  };
>  
>  static bool __init intel_pstate_platform_pwr_mgmt_exists(void)
>  {
> -	struct acpi_table_header hdr;
> -	struct hw_vendor_info *v_info;
>  	const struct x86_cpu_id *id;
>  	u64 misc_pwr;
> +	int idx;
>  
>  	id = x86_match_cpu(intel_pstate_cpu_oob_ids);
>  	if (id) {
> @@ -2507,21 +2499,15 @@ static bool __init
> intel_pstate_platform_pwr_mgmt_exists(void)
>  			return true;
>  	}
>  
> -	if (acpi_disabled ||
> -	    ACPI_FAILURE(acpi_get_table_header(ACPI_SIG_FADT, 0,
> &hdr)))
> +	idx = acpi_match_platform_list(plat_info);
> +	if (idx < 0)
>  		return false;
>  
> -	for (v_info = vendor_info; v_info->valid; v_info++) {
> -		if (!strncmp(hdr.oem_id, v_info->oem_id,
> ACPI_OEM_ID_SIZE) &&
> -			!strncmp(hdr.oem_table_id, v_info-
> >oem_table_id,
> -						ACPI_OEM_TABLE_ID_SI
> ZE))
> -			switch (v_info->oem_pwr_table) {
> -			case PSS:
> -				return intel_pstate_no_acpi_pss();
> -			case PPC:
> -				return intel_pstate_has_acpi_ppc()
> &&
> -					(!force_load);
> -			}
> +	switch (plat_info[idx].data) {
> +	case PSS:
> +		return intel_pstate_no_acpi_pss();
> +	case PPC:
> +		return intel_pstate_has_acpi_ppc() && !force_load;
>  	}
>  
>  	return false;
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] intel_pstate: convert to use acpi_match_platform_list()

[Borislav Petkov]

On Fri, Aug 18, 2017 at 01:46:41PM -0600, Toshi Kani wrote:
> Convert to use acpi_match_platform_list() for the platform check.
> There is no change in functionality.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Borislav Petkov <bp@alien8.de>
> ---
>  drivers/cpufreq/intel_pstate.c |   64 ++++++++++++++++------------------------
>  1 file changed, 25 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 65ee4fc..ad713cd 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2466,39 +2466,31 @@ enum {
>  	PPC,
>  };
>  
> -struct hw_vendor_info {
> -	u16  valid;
> -	char oem_id[ACPI_OEM_ID_SIZE];
> -	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
> -	int  oem_pwr_table;
> -};
> -
>  /* Hardware vendor-specific info that has its own power management modes */
> -static struct hw_vendor_info vendor_info[] __initdata = {
> -	{1, "HP    ", "ProLiant", PSS},
> -	{1, "ORACLE", "X4-2    ", PPC},
> -	{1, "ORACLE", "X4-2L   ", PPC},
> -	{1, "ORACLE", "X4-2B   ", PPC},
> -	{1, "ORACLE", "X3-2    ", PPC},
> -	{1, "ORACLE", "X3-2L   ", PPC},
> -	{1, "ORACLE", "X3-2B   ", PPC},
> -	{1, "ORACLE", "X4470M2 ", PPC},
> -	{1, "ORACLE", "X4270M3 ", PPC},
> -	{1, "ORACLE", "X4270M2 ", PPC},
> -	{1, "ORACLE", "X4170M2 ", PPC},
> -	{1, "ORACLE", "X4170 M3", PPC},
> -	{1, "ORACLE", "X4275 M3", PPC},
> -	{1, "ORACLE", "X6-2    ", PPC},
> -	{1, "ORACLE", "Sudbury ", PPC},
> -	{0, "", ""},
> +static struct acpi_platform_list plat_info[] __initdata = {
> +	{"HP    ", "ProLiant", 0, ACPI_SIG_FADT, all_versions, 0, PSS},

Btw, why is that ACPI_SIG_FADT's description not "FADT" ?

#define ACPI_SIG_FADT           "FACP"  /* Fixed ACPI Description Table */

More ACPI fun? I don't think I can take any more fun.

Oh well,

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[v3,2/5] intel_pstate: convert to use acpi_match_platform_list()

[Borislav Petkov]

On Fri, Aug 18, 2017 at 01:46:41PM -0600, Toshi Kani wrote:
> Convert to use acpi_match_platform_list() for the platform check.
> There is no change in functionality.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Borislav Petkov <bp@alien8.de>
> ---
>  drivers/cpufreq/intel_pstate.c |   64 ++++++++++++++++------------------------
>  1 file changed, 25 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 65ee4fc..ad713cd 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2466,39 +2466,31 @@ enum {
>  	PPC,
>  };
>  
> -struct hw_vendor_info {
> -	u16  valid;
> -	char oem_id[ACPI_OEM_ID_SIZE];
> -	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
> -	int  oem_pwr_table;
> -};
> -
>  /* Hardware vendor-specific info that has its own power management modes */
> -static struct hw_vendor_info vendor_info[] __initdata = {
> -	{1, "HP    ", "ProLiant", PSS},
> -	{1, "ORACLE", "X4-2    ", PPC},
> -	{1, "ORACLE", "X4-2L   ", PPC},
> -	{1, "ORACLE", "X4-2B   ", PPC},
> -	{1, "ORACLE", "X3-2    ", PPC},
> -	{1, "ORACLE", "X3-2L   ", PPC},
> -	{1, "ORACLE", "X3-2B   ", PPC},
> -	{1, "ORACLE", "X4470M2 ", PPC},
> -	{1, "ORACLE", "X4270M3 ", PPC},
> -	{1, "ORACLE", "X4270M2 ", PPC},
> -	{1, "ORACLE", "X4170M2 ", PPC},
> -	{1, "ORACLE", "X4170 M3", PPC},
> -	{1, "ORACLE", "X4275 M3", PPC},
> -	{1, "ORACLE", "X6-2    ", PPC},
> -	{1, "ORACLE", "Sudbury ", PPC},
> -	{0, "", ""},
> +static struct acpi_platform_list plat_info[] __initdata = {
> +	{"HP    ", "ProLiant", 0, ACPI_SIG_FADT, all_versions, 0, PSS},

Btw, why is that ACPI_SIG_FADT's description not "FADT" ?

#define ACPI_SIG_FADT           "FACP"  /* Fixed ACPI Description Table */

More ACPI fun? I don't think I can take any more fun.

Oh well,

Reviewed-by: Borislav Petkov <bp@suse.de>

Re: [PATCH v3 2/5] intel_pstate: convert to use acpi_match_platform_list()

[Kani, Toshimitsu]

On Wed, 2017-08-23 at 17:46 +0200, Borislav Petkov wrote:
> On Fri, Aug 18, 2017 at 01:46:41PM -0600, Toshi Kani wrote:
> > Convert to use acpi_match_platform_list() for the platform check.
> > There is no change in functionality.
> > 
 :
> 
> Btw, why is that ACPI_SIG_FADT's description not "FADT" ?
> 
> #define ACPI_SIG_FADT           "FACP"  /* Fixed ACPI Description
> Table */
> 
> More ACPI fun? I don't think I can take any more fun.

Yes, more ACPI fun. :-)  According to the spec:

‘FACP’. Signature for the Fixed ACPI Description Table. (This signature
predates ACPI 1.0, explaining the mismatch with this table's name.)

> Oh well,
> 
> Reviewed-by: Borislav Petkov <bp@suse.de>

Thanks!
-Toshi

[v3,2/5] intel_pstate: convert to use acpi_match_platform_list()

[Toshi Kani]

T24gV2VkLCAyMDE3LTA4LTIzIGF0IDE3OjQ2ICswMjAwLCBCb3Jpc2xhdiBQZXRrb3Ygd3JvdGU6
DQo+IE9uIEZyaSwgQXVnIDE4LCAyMDE3IGF0IDAxOjQ2OjQxUE0gLTA2MDAsIFRvc2hpIEthbmkg
d3JvdGU6DQo+ID4gQ29udmVydCB0byB1c2UgYWNwaV9tYXRjaF9wbGF0Zm9ybV9saXN0KCkgZm9y
IHRoZSBwbGF0Zm9ybSBjaGVjay4NCj4gPiBUaGVyZSBpcyBubyBjaGFuZ2UgaW4gZnVuY3Rpb25h
bGl0eS4NCj4gPiANCiA6DQo+IA0KPiBCdHcsIHdoeSBpcyB0aGF0IEFDUElfU0lHX0ZBRFQncyBk
ZXNjcmlwdGlvbiBub3QgIkZBRFQiID8NCj4gDQo+ICNkZWZpbmUgQUNQSV9TSUdfRkFEVMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqAiRkFDUCLCoMKgLyogRml4ZWQgQUNQSSBEZXNjcmlwdGlvbg0KPiBU
YWJsZSAqLw0KPiANCj4gTW9yZSBBQ1BJIGZ1bj8gSSBkb24ndCB0aGluayBJIGNhbiB0YWtlIGFu
eSBtb3JlIGZ1bi4NCg0KWWVzLCBtb3JlIEFDUEkgZnVuLiA6LSkgIEFjY29yZGluZyB0byB0aGUg
c3BlYzoNCg0K4oCYRkFDUOKAmS4gU2lnbmF0dXJlIGZvciB0aGUgRml4ZWQgQUNQSSBEZXNjcmlw
dGlvbiBUYWJsZS4gKFRoaXMgc2lnbmF0dXJlDQpwcmVkYXRlcyBBQ1BJIDEuMCwgZXhwbGFpbmlu
ZyB0aGUgbWlzbWF0Y2ggd2l0aCB0aGlzIHRhYmxlJ3MgbmFtZS4pDQoNCj4gT2ggd2VsbCwNCj4g
DQo+IFJldmlld2VkLWJ5OiBCb3Jpc2xhdiBQZXRrb3YgPGJwQHN1c2UuZGU+DQoNClRoYW5rcyEN
Ci1Ub3NoaQ0K
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] intel_pstate: convert to use acpi_match_platform_list()

[Kani, Toshimitsu]

On Wed, 2017-08-23 at 17:46 +0200, Borislav Petkov wrote:
> On Fri, Aug 18, 2017 at 01:46:41PM -0600, Toshi Kani wrote:
> > Convert to use acpi_match_platform_list() for the platform check.
> > There is no change in functionality.
> > 
 :
> 
> Btw, why is that ACPI_SIG_FADT's description not "FADT" ?
> 
> #define ACPI_SIG_FADT           "FACP"  /* Fixed ACPI Description
> Table */
> 
> More ACPI fun? I don't think I can take any more fun.

Yes, more ACPI fun. :-)  According to the spec:

‘FACP’. Signature for the Fixed ACPI Description Table. (This signature
predates ACPI 1.0, explaining the mismatch with this table's name.)

> Oh well,
> 
> Reviewed-by: Borislav Petkov <bp@suse.de>

Thanks!
-Toshi

[PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac

[Toshi Kani]

The ghes_edac driver was introduced in 2013 [1], but it has not
been enabled by any distro yet.  This driver obtains error info
from firmware interfaces, which are not properly implemented on
many platforms, as the driver always emits the messages below:

 This EDAC driver relies on BIOS to enumerate memory and get error reports.
 Unfortunately, not all BIOSes reflect the memory layout correctly
 So, the end result of using this driver varies from vendor to vendor
 If you find incorrect reports, please contact your hardware vendor
 to correct its BIOS.

To get out from this situation, add a platform check to selectively
enable the driver on the platforms that are known to have proper
firmware implementation.  Platform vendors can add their platforms
to the list when they support ghes_edac.

"ghes_edac.force_load=1" skips this platform check.

[1]: https://lwn.net/Articles/538438/
Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
---
 drivers/edac/ghes_edac.c |   28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 4e61a62..367e106 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -34,6 +34,9 @@ static LIST_HEAD(ghes_reglist);
 static DEFINE_MUTEX(ghes_edac_lock);
 static int ghes_edac_mc_num;
 
+/* Set 1 to skip the platform check */
+static bool __read_mostly ghes_edac_force_load;
+module_param_named(force_load, ghes_edac_force_load, bool, 0);
 
 /* Memory Device - Type 17 of SMBIOS spec */
 struct memdev_dmi_entry {
@@ -405,6 +408,15 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
 }
 EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error);
 
+/*
+ * Known systems that are safe to enable this module.
+ * "ghes_edac.force_load=1" skips this check if necessary.
+ */
+static struct acpi_platform_list plat_list[] = {
+	{"HPE   ", "Server  ", 0, ACPI_SIG_FADT, all_versions},
+	{ } /* End */
+};
+
 int ghes_edac_register(struct ghes *ghes, struct device *dev)
 {
 	bool fake = false;
@@ -413,6 +425,12 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	struct edac_mc_layer layers[1];
 	struct ghes_edac_pvt *pvt;
 	struct ghes_edac_dimm_fill dimm_fill;
+	int idx;
+
+	/* Check if safe to enable on this system */
+	idx = acpi_match_platform_list(plat_list);
+	if (!ghes_edac_force_load && idx < 0)
+		return 0;
 
 	/* Get the number of DIMMs */
 	dmi_walk(ghes_edac_count_dimms, &num_dimm);
@@ -456,7 +474,11 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	mci->dev_name = "ghes";
 
 	if (!ghes_edac_mc_num) {
-		if (!fake) {
+		if (fake) {
+			pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
+			pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
+			pr_info("work on such system. Use this driver with caution\n");
+		} else if (idx < 0) {
 			pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n");
 			pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n");
 			pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
@@ -464,10 +486,6 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 			pr_info("to correct its BIOS.\n");
 			pr_info("This system has %d DIMM sockets.\n",
 				num_dimm);
-		} else {
-			pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
-			pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
-			pr_info("work on such system. Use this driver with caution\n");
 		}
 	}
 

[v3,3/5] ghes_edac: add platform check to enable ghes_edac

[Toshi Kani]

The ghes_edac driver was introduced in 2013 [1], but it has not
been enabled by any distro yet.  This driver obtains error info
from firmware interfaces, which are not properly implemented on
many platforms, as the driver always emits the messages below:

 This EDAC driver relies on BIOS to enumerate memory and get error reports.
 Unfortunately, not all BIOSes reflect the memory layout correctly
 So, the end result of using this driver varies from vendor to vendor
 If you find incorrect reports, please contact your hardware vendor
 to correct its BIOS.

To get out from this situation, add a platform check to selectively
enable the driver on the platforms that are known to have proper
firmware implementation.  Platform vendors can add their platforms
to the list when they support ghes_edac.

"ghes_edac.force_load=1" skips this platform check.

[1]: https://lwn.net/Articles/538438/
Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
---
 drivers/edac/ghes_edac.c |   28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 4e61a62..367e106 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -34,6 +34,9 @@ static LIST_HEAD(ghes_reglist);
 static DEFINE_MUTEX(ghes_edac_lock);
 static int ghes_edac_mc_num;
 
+/* Set 1 to skip the platform check */
+static bool __read_mostly ghes_edac_force_load;
+module_param_named(force_load, ghes_edac_force_load, bool, 0);
 
 /* Memory Device - Type 17 of SMBIOS spec */
 struct memdev_dmi_entry {
@@ -405,6 +408,15 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
 }
 EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error);
 
+/*
+ * Known systems that are safe to enable this module.
+ * "ghes_edac.force_load=1" skips this check if necessary.
+ */
+static struct acpi_platform_list plat_list[] = {
+	{"HPE   ", "Server  ", 0, ACPI_SIG_FADT, all_versions},
+	{ } /* End */
+};
+
 int ghes_edac_register(struct ghes *ghes, struct device *dev)
 {
 	bool fake = false;
@@ -413,6 +425,12 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	struct edac_mc_layer layers[1];
 	struct ghes_edac_pvt *pvt;
 	struct ghes_edac_dimm_fill dimm_fill;
+	int idx;
+
+	/* Check if safe to enable on this system */
+	idx = acpi_match_platform_list(plat_list);
+	if (!ghes_edac_force_load && idx < 0)
+		return 0;
 
 	/* Get the number of DIMMs */
 	dmi_walk(ghes_edac_count_dimms, &num_dimm);
@@ -456,7 +474,11 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	mci->dev_name = "ghes";
 
 	if (!ghes_edac_mc_num) {
-		if (!fake) {
+		if (fake) {
+			pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
+			pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
+			pr_info("work on such system. Use this driver with caution\n");
+		} else if (idx < 0) {
 			pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n");
 			pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n");
 			pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
@@ -464,10 +486,6 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 			pr_info("to correct its BIOS.\n");
 			pr_info("This system has %d DIMM sockets.\n",
 				num_dimm);
-		} else {
-			pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
-			pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
-			pr_info("work on such system. Use this driver with caution\n");
 		}
 	}
 

Re: [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac

[Borislav Petkov]

On Fri, Aug 18, 2017 at 01:46:42PM -0600, Toshi Kani wrote:
> The ghes_edac driver was introduced in 2013 [1], but it has not
> been enabled by any distro yet.  This driver obtains error info
> from firmware interfaces, which are not properly implemented on
> many platforms, as the driver always emits the messages below:
> 
>  This EDAC driver relies on BIOS to enumerate memory and get error reports.
>  Unfortunately, not all BIOSes reflect the memory layout correctly
>  So, the end result of using this driver varies from vendor to vendor
>  If you find incorrect reports, please contact your hardware vendor
>  to correct its BIOS.
> 
> To get out from this situation, add a platform check to selectively
> enable the driver on the platforms that are known to have proper
> firmware implementation.  Platform vendors can add their platforms
> to the list when they support ghes_edac.
> 
> "ghes_edac.force_load=1" skips this platform check.
> 
> [1]: https://lwn.net/Articles/538438/
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Tony Luck <tony.luck@intel.com>
> ---
>  drivers/edac/ghes_edac.c |   28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)

Ok, for the remaining three, I've updated my "ghes" branch here:

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=ghes

Please, redo them ontop.

@Rafael: how do you want to handle this?

The first two are ACPI patches and the remaining three are EDAC. It
would be probably easier if you acked the ACPI ones (but wait until
Toshi's next version) and took them all through the EDAC tree as I have
two more reworking that ghes_edac driver.

Alternatively, they could all go through the ACPI tree but you'll have
to pick them all up together. That shouldn't be a problem either as all
changes are solely to drivers/edac/ghes_edac.c and there's one other
patch in my EDAC pile which touches ghes_edac.c:

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/diff/drivers/edac/ghes_edac.c?h=for-next&id=c54182ec0e157988f0cafd1e8d37b68ab4210f87

That's why I say, it'll be easier if I carried them all. :)

But I'm sure we can work something out.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[v3,3/5] ghes_edac: add platform check to enable ghes_edac

[Borislav Petkov]

On Fri, Aug 18, 2017 at 01:46:42PM -0600, Toshi Kani wrote:
> The ghes_edac driver was introduced in 2013 [1], but it has not
> been enabled by any distro yet.  This driver obtains error info
> from firmware interfaces, which are not properly implemented on
> many platforms, as the driver always emits the messages below:
> 
>  This EDAC driver relies on BIOS to enumerate memory and get error reports.
>  Unfortunately, not all BIOSes reflect the memory layout correctly
>  So, the end result of using this driver varies from vendor to vendor
>  If you find incorrect reports, please contact your hardware vendor
>  to correct its BIOS.
> 
> To get out from this situation, add a platform check to selectively
> enable the driver on the platforms that are known to have proper
> firmware implementation.  Platform vendors can add their platforms
> to the list when they support ghes_edac.
> 
> "ghes_edac.force_load=1" skips this platform check.
> 
> [1]: https://lwn.net/Articles/538438/
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Tony Luck <tony.luck@intel.com>
> ---
>  drivers/edac/ghes_edac.c |   28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)

Ok, for the remaining three, I've updated my "ghes" branch here:

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=ghes

Please, redo them ontop.

@Rafael: how do you want to handle this?

The first two are ACPI patches and the remaining three are EDAC. It
would be probably easier if you acked the ACPI ones (but wait until
Toshi's next version) and took them all through the EDAC tree as I have
two more reworking that ghes_edac driver.

Alternatively, they could all go through the ACPI tree but you'll have
to pick them all up together. That shouldn't be a problem either as all
changes are solely to drivers/edac/ghes_edac.c and there's one other
patch in my EDAC pile which touches ghes_edac.c:

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/diff/drivers/edac/ghes_edac.c?h=for-next&id=c54182ec0e157988f0cafd1e8d37b68ab4210f87

That's why I say, it'll be easier if I carried them all. :)

But I'm sure we can work something out.

Thanks.

Re: [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac

[Borislav Petkov]

On Fri, Aug 18, 2017 at 01:46:42PM -0600, Toshi Kani wrote:
> The ghes_edac driver was introduced in 2013 [1], but it has not
> been enabled by any distro yet.  This driver obtains error info
> from firmware interfaces, which are not properly implemented on
> many platforms, as the driver always emits the messages below:
> 
>  This EDAC driver relies on BIOS to enumerate memory and get error reports.
>  Unfortunately, not all BIOSes reflect the memory layout correctly
>  So, the end result of using this driver varies from vendor to vendor
>  If you find incorrect reports, please contact your hardware vendor
>  to correct its BIOS.
> 
> To get out from this situation, add a platform check to selectively
> enable the driver on the platforms that are known to have proper
> firmware implementation.  Platform vendors can add their platforms
> to the list when they support ghes_edac.
> 
> "ghes_edac.force_load=1" skips this platform check.
> 
> [1]: https://lwn.net/Articles/538438/
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Tony Luck <tony.luck@intel.com>
> ---
>  drivers/edac/ghes_edac.c |   28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)

Ok, for the remaining three, I've updated my "ghes" branch here:

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=ghes

Please, redo them ontop.

@Rafael: how do you want to handle this?

The first two are ACPI patches and the remaining three are EDAC. It
would be probably easier if you acked the ACPI ones (but wait until
Toshi's next version) and took them all through the EDAC tree as I have
two more reworking that ghes_edac driver.

Alternatively, they could all go through the ACPI tree but you'll have
to pick them all up together. That shouldn't be a problem either as all
changes are solely to drivers/edac/ghes_edac.c and there's one other
patch in my EDAC pile which touches ghes_edac.c:

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/diff/drivers/edac/ghes_edac.c?h=for-next&id=c54182ec0e157988f0cafd1e8d37b68ab4210f87

That's why I say, it'll be easier if I carried them all. :)

But I'm sure we can work something out.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac

[Rafael J. Wysocki]

On Wednesday, August 23, 2017 6:20:53 PM CEST Borislav Petkov wrote:
> On Fri, Aug 18, 2017 at 01:46:42PM -0600, Toshi Kani wrote:
> > The ghes_edac driver was introduced in 2013 [1], but it has not
> > been enabled by any distro yet.  This driver obtains error info
> > from firmware interfaces, which are not properly implemented on
> > many platforms, as the driver always emits the messages below:
> > 
> >  This EDAC driver relies on BIOS to enumerate memory and get error reports.
> >  Unfortunately, not all BIOSes reflect the memory layout correctly
> >  So, the end result of using this driver varies from vendor to vendor
> >  If you find incorrect reports, please contact your hardware vendor
> >  to correct its BIOS.
> > 
> > To get out from this situation, add a platform check to selectively
> > enable the driver on the platforms that are known to have proper
> > firmware implementation.  Platform vendors can add their platforms
> > to the list when they support ghes_edac.
> > 
> > "ghes_edac.force_load=1" skips this platform check.
> > 
> > [1]: https://lwn.net/Articles/538438/
> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Cc: Tony Luck <tony.luck@intel.com>
> > ---
> >  drivers/edac/ghes_edac.c |   28 +++++++++++++++++++++++-----
> >  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> Ok, for the remaining three, I've updated my "ghes" branch here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=ghes
> 
> Please, redo them ontop.
> 
> @Rafael: how do you want to handle this?
> 
> The first two are ACPI patches and the remaining three are EDAC. It
> would be probably easier if you acked the ACPI ones (but wait until
> Toshi's next version) and took them all through the EDAC tree as I have
> two more reworking that ghes_edac driver.

So I have some pending intel_pstate and the intel_pstate changes are likely
to conflict with it.

> Alternatively, they could all go through the ACPI tree but you'll have
> to pick them all up together.

That can be done. :-)

> That shouldn't be a problem either as all
> changes are solely to drivers/edac/ghes_edac.c and there's one other
> patch in my EDAC pile which touches ghes_edac.c:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/diff/drivers/edac/ghes_edac.c?h=for-next&id=c54182ec0e157988f0cafd1e8d37b68ab4210f87
> 
> That's why I say, it'll be easier if I carried them all. :)
> 
> But I'm sure we can work something out.

I can expose a branch with this series for you to merge if that helps.

Thanks,
Rafael

[v3,3/5] ghes_edac: add platform check to enable ghes_edac

[Rafael J. Wysocki]

On Wednesday, August 23, 2017 6:20:53 PM CEST Borislav Petkov wrote:
> On Fri, Aug 18, 2017 at 01:46:42PM -0600, Toshi Kani wrote:
> > The ghes_edac driver was introduced in 2013 [1], but it has not
> > been enabled by any distro yet.  This driver obtains error info
> > from firmware interfaces, which are not properly implemented on
> > many platforms, as the driver always emits the messages below:
> > 
> >  This EDAC driver relies on BIOS to enumerate memory and get error reports.
> >  Unfortunately, not all BIOSes reflect the memory layout correctly
> >  So, the end result of using this driver varies from vendor to vendor
> >  If you find incorrect reports, please contact your hardware vendor
> >  to correct its BIOS.
> > 
> > To get out from this situation, add a platform check to selectively
> > enable the driver on the platforms that are known to have proper
> > firmware implementation.  Platform vendors can add their platforms
> > to the list when they support ghes_edac.
> > 
> > "ghes_edac.force_load=1" skips this platform check.
> > 
> > [1]: https://lwn.net/Articles/538438/
> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Cc: Tony Luck <tony.luck@intel.com>
> > ---
> >  drivers/edac/ghes_edac.c |   28 +++++++++++++++++++++++-----
> >  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> Ok, for the remaining three, I've updated my "ghes" branch here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=ghes
> 
> Please, redo them ontop.
> 
> @Rafael: how do you want to handle this?
> 
> The first two are ACPI patches and the remaining three are EDAC. It
> would be probably easier if you acked the ACPI ones (but wait until
> Toshi's next version) and took them all through the EDAC tree as I have
> two more reworking that ghes_edac driver.

So I have some pending intel_pstate and the intel_pstate changes are likely
to conflict with it.

> Alternatively, they could all go through the ACPI tree but you'll have
> to pick them all up together.

That can be done. :-)

> That shouldn't be a problem either as all
> changes are solely to drivers/edac/ghes_edac.c and there's one other
> patch in my EDAC pile which touches ghes_edac.c:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/diff/drivers/edac/ghes_edac.c?h=for-next&id=c54182ec0e157988f0cafd1e8d37b68ab4210f87
> 
> That's why I say, it'll be easier if I carried them all. :)
> 
> But I'm sure we can work something out.

I can expose a branch with this series for you to merge if that helps.

Thanks,
Rafael
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac

[Borislav Petkov]

On Wed, Aug 23, 2017 at 10:46:42PM +0200, Rafael J. Wysocki wrote:
> I can expose a branch with this series for you to merge if that helps.

I think that'll be the best solution. So yes, pls lemme know when you
have it so that I can pick up the rest.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[v3,3/5] ghes_edac: add platform check to enable ghes_edac

[Borislav Petkov]

On Wed, Aug 23, 2017 at 10:46:42PM +0200, Rafael J. Wysocki wrote:
> I can expose a branch with this series for you to merge if that helps.

I think that'll be the best solution. So yes, pls lemme know when you
have it so that I can pick up the rest.

Thx.

[PATCH v3 4/5] EDAC: add edac_get_owner() to check MC owner

[Toshi Kani]

Only a single edac driver can be enabled for EDAC MC.  When ghes_edac
is enabled, a regular edac driver for the CPU type / platform still
attempts to register itself and fails in edac_mc_add_mc().

Add edac_get_owner() so that regular edac drivers can check the owner
of EDAC MC without calling edac_mc_add_mc().

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Suggested-by: Borislav Petkov <bp@alien8.de>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
---
 drivers/edac/edac_mc.c |    7 ++++++-
 drivers/edac/edac_mc.h |    8 ++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 4800721..48193f5 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -53,7 +53,7 @@ static LIST_HEAD(mc_devices);
  * Used to lock EDAC MC to just one module, avoiding two drivers e. g.
  *	apei/ghes and i7core_edac to be used at the same time.
  */
-static void const *edac_mc_owner;
+static const char *edac_mc_owner;
 
 static struct bus_type mc_bus[EDAC_MAX_MCS];
 
@@ -701,6 +701,11 @@ struct mem_ctl_info *edac_mc_find(int idx)
 }
 EXPORT_SYMBOL(edac_mc_find);
 
+const char *edac_get_owner(void)
+{
+	return edac_mc_owner;
+}
+EXPORT_SYMBOL_GPL(edac_get_owner);
 
 /* FIXME - should a warning be printed if no error detection? correction? */
 int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
index 5357800..4165e15 100644
--- a/drivers/edac/edac_mc.h
+++ b/drivers/edac/edac_mc.h
@@ -128,6 +128,14 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 				   unsigned sz_pvt);
 
 /**
+ * edac_get_owner - Return the owner's mod_name of EDAC MC
+ *
+ * Returns:
+ *	Pointer to mod_name string when EDAC MC is owned. NULL otherwise.
+ */
+extern const char *edac_get_owner(void);
+
+/*
  * edac_mc_add_mc_with_groups() - Insert the @mci structure into the mci
  *	global list and create sysfs entries associated with @mci structure.
  *

[v3,4/5] EDAC: add edac_get_owner() to check MC owner

[Toshi Kani]

Only a single edac driver can be enabled for EDAC MC.  When ghes_edac
is enabled, a regular edac driver for the CPU type / platform still
attempts to register itself and fails in edac_mc_add_mc().

Add edac_get_owner() so that regular edac drivers can check the owner
of EDAC MC without calling edac_mc_add_mc().

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Suggested-by: Borislav Petkov <bp@alien8.de>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
---
 drivers/edac/edac_mc.c |    7 ++++++-
 drivers/edac/edac_mc.h |    8 ++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 4800721..48193f5 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -53,7 +53,7 @@ static LIST_HEAD(mc_devices);
  * Used to lock EDAC MC to just one module, avoiding two drivers e. g.
  *	apei/ghes and i7core_edac to be used at the same time.
  */
-static void const *edac_mc_owner;
+static const char *edac_mc_owner;
 
 static struct bus_type mc_bus[EDAC_MAX_MCS];
 
@@ -701,6 +701,11 @@ struct mem_ctl_info *edac_mc_find(int idx)
 }
 EXPORT_SYMBOL(edac_mc_find);
 
+const char *edac_get_owner(void)
+{
+	return edac_mc_owner;
+}
+EXPORT_SYMBOL_GPL(edac_get_owner);
 
 /* FIXME - should a warning be printed if no error detection? correction? */
 int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
index 5357800..4165e15 100644
--- a/drivers/edac/edac_mc.h
+++ b/drivers/edac/edac_mc.h
@@ -128,6 +128,14 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 				   unsigned sz_pvt);
 
 /**
+ * edac_get_owner - Return the owner's mod_name of EDAC MC
+ *
+ * Returns:
+ *	Pointer to mod_name string when EDAC MC is owned. NULL otherwise.
+ */
+extern const char *edac_get_owner(void);
+
+/*
  * edac_mc_add_mc_with_groups() - Insert the @mci structure into the mci
  *	global list and create sysfs entries associated with @mci structure.
  *

[PATCH v3 5/5] edac drivers: add MC owner check in init

[Toshi Kani]

Change generic x86 edac drivers, which probe CPU type with
x86_match_cpu(), to verify the module owner at the beginning
of their module init functions.  This allows them to fail
their init immediately when ghes_edac is enabled.  Similar
change can be made to other edac drivers as necessary.

Also, remove ".c" from module names of pnp2_edac, sb_edac,
and skx_edac.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Suggested-by: Borislav Petkov <bp@alien8.de>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
---
 drivers/edac/amd64_edac.c |    5 +++++
 drivers/edac/pnd2_edac.c  |    9 ++++++++-
 drivers/edac/sb_edac.c    |    9 +++++++--
 drivers/edac/skx_edac.c   |    8 +++++++-
 4 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 3aea556..529bcbf 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3435,9 +3435,14 @@ MODULE_DEVICE_TABLE(x86cpu, amd64_cpuids);
 
 static int __init amd64_edac_init(void)
 {
+	const char *owner;
 	int err = -ENODEV;
 	int i;
 
+	owner = edac_get_owner();
+	if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR)))
+		return -EBUSY;
+
 	if (!x86_match_cpu(amd64_cpuids))
 		return -ENODEV;
 
diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c
index 8e59949..6609041 100644
--- a/drivers/edac/pnd2_edac.c
+++ b/drivers/edac/pnd2_edac.c
@@ -45,6 +45,8 @@
 #include "edac_module.h"
 #include "pnd2_edac.h"
 
+#define EDAC_MOD_STR		"pnd2_edac"
+
 #define APL_NUM_CHANNELS	4
 #define DNV_NUM_CHANNELS	2
 #define DNV_MAX_DIMMS		2 /* Max DIMMs per channel */
@@ -1313,7 +1315,7 @@ static int pnd2_register_mci(struct mem_ctl_info **ppmci)
 	pvt = mci->pvt_info;
 	memset(pvt, 0, sizeof(*pvt));
 
-	mci->mod_name = "pnd2_edac.c";
+	mci->mod_name = EDAC_MOD_STR;
 	mci->dev_name = ops->name;
 	mci->ctl_name = "Pondicherry2";
 
@@ -1505,10 +1507,15 @@ MODULE_DEVICE_TABLE(x86cpu, pnd2_cpuids);
 static int __init pnd2_init(void)
 {
 	const struct x86_cpu_id *id;
+	const char *owner;
 	int rc;
 
 	edac_dbg(2, "\n");
 
+	owner = edac_get_owner();
+	if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR)))
+		return -EBUSY;
+
 	id = x86_match_cpu(pnd2_cpuids);
 	if (!id)
 		return -ENODEV;
diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index 80d860c..d411aa5c 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -36,7 +36,7 @@ static LIST_HEAD(sbridge_edac_list);
  * Alter this version for the module when modifications are made
  */
 #define SBRIDGE_REVISION    " Ver: 1.1.2 "
-#define EDAC_MOD_STR      "sbridge_edac"
+#define EDAC_MOD_STR	    "sb_edac"
 
 /*
  * Debug macros
@@ -3124,7 +3124,7 @@ static int sbridge_register_mci(struct sbridge_dev *sbridge_dev, enum type type)
 		MEM_FLAG_DDR4 : MEM_FLAG_DDR3;
 	mci->edac_ctl_cap = EDAC_FLAG_NONE;
 	mci->edac_cap = EDAC_FLAG_NONE;
-	mci->mod_name = "sb_edac.c";
+	mci->mod_name = EDAC_MOD_STR;
 	mci->mod_ver = SBRIDGE_REVISION;
 	mci->dev_name = pci_name(pdev);
 	mci->ctl_page_to_phys = NULL;
@@ -3372,10 +3372,15 @@ static void sbridge_remove(void)
 static int __init sbridge_init(void)
 {
 	const struct x86_cpu_id *id;
+	const char *owner;
 	int rc;
 
 	edac_dbg(2, "\n");
 
+	owner = edac_get_owner();
+	if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR)))
+		return -EBUSY;
+
 	id = x86_match_cpu(sbridge_cpuids);
 	if (!id)
 		return -ENODEV;
diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c
index 64bef6c9..17a2fbd 100644
--- a/drivers/edac/skx_edac.c
+++ b/drivers/edac/skx_edac.c
@@ -31,6 +31,7 @@
 
 #include "edac_module.h"
 
+#define EDAC_MOD_STR    "skx_edac"
 #define SKX_REVISION    " Ver: 1.0 "
 
 /*
@@ -471,7 +472,7 @@ static int skx_register_mci(struct skx_imc *imc)
 	mci->mtype_cap = MEM_FLAG_DDR4;
 	mci->edac_ctl_cap = EDAC_FLAG_NONE;
 	mci->edac_cap = EDAC_FLAG_NONE;
-	mci->mod_name = "skx_edac.c";
+	mci->mod_name = EDAC_MOD_STR;
 	mci->dev_name = pci_name(imc->chan[0].cdev);
 	mci->mod_ver = SKX_REVISION;
 	mci->ctl_page_to_phys = NULL;
@@ -1042,12 +1043,17 @@ static int __init skx_init(void)
 {
 	const struct x86_cpu_id *id;
 	const struct munit *m;
+	const char *owner;
 	int rc = 0, i;
 	u8 mc = 0, src_id, node_id;
 	struct skx_dev *d;
 
 	edac_dbg(2, "\n");
 
+	owner = edac_get_owner();
+	if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR)))
+		return -EBUSY;
+
 	id = x86_match_cpu(skx_cpuids);
 	if (!id)
 		return -ENODEV;

[v3,5/5] edac drivers: add MC owner check in init

[Toshi Kani]

Change generic x86 edac drivers, which probe CPU type with
x86_match_cpu(), to verify the module owner at the beginning
of their module init functions.  This allows them to fail
their init immediately when ghes_edac is enabled.  Similar
change can be made to other edac drivers as necessary.

Also, remove ".c" from module names of pnp2_edac, sb_edac,
and skx_edac.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Suggested-by: Borislav Petkov <bp@alien8.de>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
---
 drivers/edac/amd64_edac.c |    5 +++++
 drivers/edac/pnd2_edac.c  |    9 ++++++++-
 drivers/edac/sb_edac.c    |    9 +++++++--
 drivers/edac/skx_edac.c   |    8 +++++++-
 4 files changed, 27 insertions(+), 4 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 3aea556..529bcbf 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3435,9 +3435,14 @@ MODULE_DEVICE_TABLE(x86cpu, amd64_cpuids);
 
 static int __init amd64_edac_init(void)
 {
+	const char *owner;
 	int err = -ENODEV;
 	int i;
 
+	owner = edac_get_owner();
+	if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR)))
+		return -EBUSY;
+
 	if (!x86_match_cpu(amd64_cpuids))
 		return -ENODEV;
 
diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c
index 8e59949..6609041 100644
--- a/drivers/edac/pnd2_edac.c
+++ b/drivers/edac/pnd2_edac.c
@@ -45,6 +45,8 @@
 #include "edac_module.h"
 #include "pnd2_edac.h"
 
+#define EDAC_MOD_STR		"pnd2_edac"
+
 #define APL_NUM_CHANNELS	4
 #define DNV_NUM_CHANNELS	2
 #define DNV_MAX_DIMMS		2 /* Max DIMMs per channel */
@@ -1313,7 +1315,7 @@ static int pnd2_register_mci(struct mem_ctl_info **ppmci)
 	pvt = mci->pvt_info;
 	memset(pvt, 0, sizeof(*pvt));
 
-	mci->mod_name = "pnd2_edac.c";
+	mci->mod_name = EDAC_MOD_STR;
 	mci->dev_name = ops->name;
 	mci->ctl_name = "Pondicherry2";
 
@@ -1505,10 +1507,15 @@ MODULE_DEVICE_TABLE(x86cpu, pnd2_cpuids);
 static int __init pnd2_init(void)
 {
 	const struct x86_cpu_id *id;
+	const char *owner;
 	int rc;
 
 	edac_dbg(2, "\n");
 
+	owner = edac_get_owner();
+	if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR)))
+		return -EBUSY;
+
 	id = x86_match_cpu(pnd2_cpuids);
 	if (!id)
 		return -ENODEV;
diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index 80d860c..d411aa5c 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -36,7 +36,7 @@ static LIST_HEAD(sbridge_edac_list);
  * Alter this version for the module when modifications are made
  */
 #define SBRIDGE_REVISION    " Ver: 1.1.2 "
-#define EDAC_MOD_STR      "sbridge_edac"
+#define EDAC_MOD_STR	    "sb_edac"
 
 /*
  * Debug macros
@@ -3124,7 +3124,7 @@ static int sbridge_register_mci(struct sbridge_dev *sbridge_dev, enum type type)
 		MEM_FLAG_DDR4 : MEM_FLAG_DDR3;
 	mci->edac_ctl_cap = EDAC_FLAG_NONE;
 	mci->edac_cap = EDAC_FLAG_NONE;
-	mci->mod_name = "sb_edac.c";
+	mci->mod_name = EDAC_MOD_STR;
 	mci->mod_ver = SBRIDGE_REVISION;
 	mci->dev_name = pci_name(pdev);
 	mci->ctl_page_to_phys = NULL;
@@ -3372,10 +3372,15 @@ static void sbridge_remove(void)
 static int __init sbridge_init(void)
 {
 	const struct x86_cpu_id *id;
+	const char *owner;
 	int rc;
 
 	edac_dbg(2, "\n");
 
+	owner = edac_get_owner();
+	if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR)))
+		return -EBUSY;
+
 	id = x86_match_cpu(sbridge_cpuids);
 	if (!id)
 		return -ENODEV;
diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c
index 64bef6c9..17a2fbd 100644
--- a/drivers/edac/skx_edac.c
+++ b/drivers/edac/skx_edac.c
@@ -31,6 +31,7 @@
 
 #include "edac_module.h"
 
+#define EDAC_MOD_STR    "skx_edac"
 #define SKX_REVISION    " Ver: 1.0 "
 
 /*
@@ -471,7 +472,7 @@ static int skx_register_mci(struct skx_imc *imc)
 	mci->mtype_cap = MEM_FLAG_DDR4;
 	mci->edac_ctl_cap = EDAC_FLAG_NONE;
 	mci->edac_cap = EDAC_FLAG_NONE;
-	mci->mod_name = "skx_edac.c";
+	mci->mod_name = EDAC_MOD_STR;
 	mci->dev_name = pci_name(imc->chan[0].cdev);
 	mci->mod_ver = SKX_REVISION;
 	mci->ctl_page_to_phys = NULL;
@@ -1042,12 +1043,17 @@ static int __init skx_init(void)
 {
 	const struct x86_cpu_id *id;
 	const struct munit *m;
+	const char *owner;
 	int rc = 0, i;
 	u8 mc = 0, src_id, node_id;
 	struct skx_dev *d;
 
 	edac_dbg(2, "\n");
 
+	owner = edac_get_owner();
+	if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR)))
+		return -EBUSY;
+
 	id = x86_match_cpu(skx_cpuids);
 	if (!id)
 		return -ENODEV;