LinuxLists.cc - [PATCH v3 0/5] enable ghes

[permalink] [raw]

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

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 <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Borislav Petkov <[email protected]>
---
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);

2017-08-18 19:56:59

[permalink] [raw]

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

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 <[email protected]>
Suggested-by: Borislav Petkov <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Tony Luck <[email protected]>
---
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;

2017-08-18 19:56:44

[permalink] [raw]

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

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 <[email protected]>
Suggested-by: Borislav Petkov <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Tony Luck <[email protected]>
---
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.
*

2017-08-18 19:57:01

[permalink] [raw]

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

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 <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Tony Luck <[email protected]>
---
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");
}
}

2017-08-18 19:57:29

[permalink] [raw]

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

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

Signed-off-by: Toshi Kani <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Srinivas Pandruvada <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Borislav Petkov <[email protected]>
---
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;

2017-08-21 11:27:49

[permalink] [raw]

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

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 <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> ---
> 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.

2017-08-21 12:25:05

[permalink] [raw]

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

On Mon, Aug 21, 2017 at 1:27 PM, Borislav Petkov <[email protected]> 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 <[email protected]>
>> Cc: "Rafael J. Wysocki" <[email protected]>
>> Cc: Borislav Petkov <[email protected]>
>> ---
>> 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

2017-08-21 13:21:07

[permalink] [raw]

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

Borislav Petkov <[email protected]>

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

No functionality change.

Signed-off-by: Borislav Petkov <[email protected]>
---
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.

2017-08-21 13:30:52

[permalink] [raw]

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

On Mon, Aug 21, 2017 at 3:20 PM, Borislav Petkov <[email protected]> wrote:
> Borislav Petkov <[email protected]>
>
> ... and save us the splrinking of this test around in the callers.
>
> No functionality change.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> 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.

2017-08-21 15:34:33

[permalink] [raw]

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

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.

2017-08-21 16:42:23

[permalink] [raw]

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

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

2017-08-21 17:04:27

[permalink] [raw]

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

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.

2017-08-21 17:24:29

[permalink] [raw]

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

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

2017-08-21 17:36:24

[permalink] [raw]

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

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.

2017-08-21 17:53:39

by srinivas pandruvada

[permalink] [raw]

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

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 <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Srinivas Pandruvada <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Borislav Petkov <[email protected]>
Can't test but the change itself is OK.

Acked-by: Srinivas Pandruvada <[email protected]>

> ---
> 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;

2017-08-21 20:31:30

[permalink] [raw]

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

On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <[email protected]> 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

2017-08-21 21:06:48

[permalink] [raw]

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

On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote:
> On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <[email protected]>
> 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

2017-08-21 21:49:49

[permalink] [raw]

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

On Mon, Aug 21, 2017 at 11:06 PM, Kani, Toshimitsu <[email protected]> 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 <[email protected]>
>> 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

2017-08-21 22:21:46

[permalink] [raw]

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

On Mon, 2017-08-21 at 23:49 +0200, Rafael J. Wysocki wrote:
> On Mon, Aug 21, 2017 at 11:06 PM, Kani, Toshimitsu <[email protected]
> 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 <[email protected]>
> > > 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

2017-08-21 22:26:54

[permalink] [raw]

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

On Tue, Aug 22, 2017 at 12:21 AM, Kani, Toshimitsu <[email protected]> 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 <[email protected]
>> 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 <[email protected]>
>> > > 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

2017-08-23 15:46:29

[permalink] [raw]

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

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 <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Srinivas Pandruvada <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> ---
> 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 <[email protected]>

--
Regards/Gruss,
Boris.

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

2017-08-23 15:56:12

[permalink] [raw]

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

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 <[email protected]>

Thanks!
-Toshi

2017-08-23 16:21:21

[permalink] [raw]

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

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 <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Tony Luck <[email protected]>
> ---
> 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.

2017-08-23 20:55:19

[permalink] [raw]

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

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 <[email protected]>
> > Cc: Borislav Petkov <[email protected]>
> > Cc: Mauro Carvalho Chehab <[email protected]>
> > Cc: Tony Luck <[email protected]>
> > ---
> > 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

2017-08-24 07:55:00