Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1715398 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/5] enable ghes_edac on selected platforms Toshi Kani <toshi.kani@hpe.com> - 2017-08-18 22:00 +0200
    [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() Toshi Kani <toshi.kani@hpe.com> - 2017-08-18 22:00 +0200
      Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() Borislav Petkov <bp@alien8.de> - 2017-08-21 13:30 +0200
        Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() "Rafael J. Wysocki" <rafael@kernel.org> - 2017-08-21 14:30 +0200
          [PATCH] ACPICA: Check whether ACPI is disabled before getting a table Borislav Petkov <bp@alien8.de> - 2017-08-21 15:30 +0200
            Re: [PATCH] ACPICA: Check whether ACPI is disabled before getting a table "Rafael J. Wysocki" <rafael@kernel.org> - 2017-08-21 15:40 +0200
              Re: [PATCH] ACPICA: Check whether ACPI is disabled before getting a  table Borislav Petkov <bp@alien8.de> - 2017-08-21 17:40 +0200
        Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-21 18:50 +0200
          Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() Borislav Petkov <bp@alien8.de> - 2017-08-21 19:10 +0200
            Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-21 19:30 +0200
              Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() Borislav Petkov <bp@alien8.de> - 2017-08-21 19:40 +0200
                Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() "Rafael J. Wysocki" <rafael@kernel.org> - 2017-08-21 22:40 +0200
                  Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-21 23:10 +0200
                    Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() "Rafael J. Wysocki" <rafael@kernel.org> - 2017-08-21 23:50 +0200
                      Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-22 00:30 +0200
                        Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() "Rafael J. Wysocki" <rafael@kernel.org> - 2017-08-22 00:30 +0200
    [PATCH v3 4/5] EDAC: add edac_get_owner() to check MC owner Toshi Kani <toshi.kani@hpe.com> - 2017-08-18 22:00 +0200
    [PATCH v3 5/5] edac drivers: add MC owner check in init Toshi Kani <toshi.kani@hpe.com> - 2017-08-18 22:00 +0200
    [PATCH v3 2/5] intel_pstate: convert to use acpi_match_platform_list() Toshi Kani <toshi.kani@hpe.com> - 2017-08-18 22:00 +0200
      Re: [PATCH v3 2/5] intel_pstate: convert to use  acpi_match_platform_list() Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> - 2017-08-21 20:00 +0200
      Re: [PATCH v3 2/5] intel_pstate: convert to use  acpi_match_platform_list() Borislav Petkov <bp@alien8.de> - 2017-08-23 17:50 +0200
        Re: [PATCH v3 2/5] intel_pstate: convert to use  acpi_match_platform_list() "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-23 18:00 +0200
    [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac Toshi Kani <toshi.kani@hpe.com> - 2017-08-18 22:00 +0200
      Re: [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac Borislav Petkov <bp@alien8.de> - 2017-08-23 18:30 +0200
        Re: [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-08-23 23:00 +0200
          Re: [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac Borislav Petkov <bp@alien8.de> - 2017-08-24 10:00 +0200

Page 1 of 2  [1] 2  Next page →

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

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(-)

[toc] | [next] | [standalone]

#1715403 — [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 <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);
 

[toc] | [prev] | [next] | [standalone]

#1716344 — 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 <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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

#1716618 — 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.

[toc] | [prev] | [next] | [standalone]

#1716703 — 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

[toc] | [prev] | [next] | [standalone]

#1716722 — 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.

[toc] | [prev] | [next] | [standalone]

#1716741 — 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

[toc] | [prev] | [next] | [standalone]

#1716752 — 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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1716878 — 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 <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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1716920 — 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 <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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1715410 — [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 <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.
  *

[toc] | [prev] | [next] | [standalone]

#1715416 — [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 <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;

[toc] | [prev] | [next] | [standalone]

#1715418 — [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 <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;

[toc] | [prev] | [next] | [standalone]

#1716758 — 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 <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;

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web