* [PATCH v3 0/5] enable ghes_edac on selected platforms @ 2017-08-18 19:46 Toshi Kani 2017-08-18 19:46 ` [v3,1/5] " Toshi Kani ` (4 more replies) 0 siblings, 5 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-18 19:46 UTC (permalink / raw) To: rjw, bp; +Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel 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(-) ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-18 19:46 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-18 19:46 UTC (permalink / raw) To: rjw, bp Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel, Toshi Kani ACPI OEM ID / OEM Table ID / Revision can be used to identify a platform based on ACPI firmware info. acpi_blacklisted(), intel_pstate_platform_pwr_mgmt_exists(), and some other funcs, have been using similar check to detect a list of platforms that require special handlings. Move the platform check in acpi_blacklisted() to a new common utility function, acpi_match_platform_list(), so that other drivers do not have to implement their own version. There is no change in functionality. Signed-off-by: Toshi Kani <toshi.kani@hpe.com> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Borislav Petkov <bp@alien8.de> --- drivers/acpi/blacklist.c | 83 ++++++++-------------------------------------- drivers/acpi/utils.c | 40 ++++++++++++++++++++++ include/linux/acpi.h | 19 +++++++++++ 3 files changed, 73 insertions(+), 69 deletions(-) diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c index bb542ac..037fd53 100644 --- a/drivers/acpi/blacklist.c +++ b/drivers/acpi/blacklist.c @@ -30,30 +30,13 @@ #include "internal.h" -enum acpi_blacklist_predicates { - all_versions, - less_than_or_equal, - equal, - greater_than_or_equal, -}; - -struct acpi_blacklist_item { - char oem_id[7]; - char oem_table_id[9]; - u32 oem_revision; - char *table; - enum acpi_blacklist_predicates oem_revision_predicate; - char *reason; - u32 is_critical_error; -}; - static struct dmi_system_id acpi_rev_dmi_table[] __initdata; /* * POLICY: If *anything* doesn't work, put it on the blacklist. * If they are critical errors, mark it critical, and abort driver load. */ -static struct acpi_blacklist_item acpi_blacklist[] __initdata = { +static struct acpi_platform_list acpi_blacklist[] __initdata = { /* Compaq Presario 1700 */ {"PTLTD ", " DSDT ", 0x06040000, ACPI_SIG_DSDT, less_than_or_equal, "Multiple problems", 1}, @@ -67,65 +50,27 @@ static struct acpi_blacklist_item acpi_blacklist[] __initdata = { {"IBM ", "TP600E ", 0x00000105, ACPI_SIG_DSDT, less_than_or_equal, "Incorrect _ADR", 1}, - {""} + { } }; int __init acpi_blacklisted(void) { - int i = 0; + int i; int blacklisted = 0; - struct acpi_table_header table_header; - - while (acpi_blacklist[i].oem_id[0] != '\0') { - if (acpi_get_table_header(acpi_blacklist[i].table, 0, &table_header)) { - i++; - continue; - } - - if (strncmp(acpi_blacklist[i].oem_id, table_header.oem_id, 6)) { - i++; - continue; - } - - if (strncmp - (acpi_blacklist[i].oem_table_id, table_header.oem_table_id, - 8)) { - i++; - continue; - } - - if ((acpi_blacklist[i].oem_revision_predicate == all_versions) - || (acpi_blacklist[i].oem_revision_predicate == - less_than_or_equal - && table_header.oem_revision <= - acpi_blacklist[i].oem_revision) - || (acpi_blacklist[i].oem_revision_predicate == - greater_than_or_equal - && table_header.oem_revision >= - acpi_blacklist[i].oem_revision) - || (acpi_blacklist[i].oem_revision_predicate == equal - && table_header.oem_revision == - acpi_blacklist[i].oem_revision)) { - printk(KERN_ERR PREFIX - "Vendor \"%6.6s\" System \"%8.8s\" " - "Revision 0x%x has a known ACPI BIOS problem.\n", - acpi_blacklist[i].oem_id, - acpi_blacklist[i].oem_table_id, - acpi_blacklist[i].oem_revision); + i = acpi_match_platform_list(acpi_blacklist); + if (i >= 0) { + pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\" Revision 0x%x has a known ACPI BIOS problem.\n", + acpi_blacklist[i].oem_id, + acpi_blacklist[i].oem_table_id, + acpi_blacklist[i].oem_revision); - printk(KERN_ERR PREFIX - "Reason: %s. This is a %s error\n", - acpi_blacklist[i].reason, - (acpi_blacklist[i]. - is_critical_error ? "non-recoverable" : - "recoverable")); + pr_err(PREFIX "Reason: %s. This is a %s error\n", + acpi_blacklist[i].reason, + (acpi_blacklist[i].data ? + "non-recoverable" : "recoverable")); - blacklisted = acpi_blacklist[i].is_critical_error; - break; - } else { - i++; - } + blacklisted = acpi_blacklist[i].data; } (void)early_acpi_osi_init(); diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index b9d956c..998aaf5 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -816,3 +816,43 @@ static int __init acpi_backlight(char *str) return 1; } __setup("acpi_backlight=", acpi_backlight); + +/** + * acpi_match_platform_list - Check if the system matches with a given list + * @plat: pointer to acpi_platform_list table terminated by a NULL entry + * + * Return the matched index if the system is found in the platform list. + * Otherwise, return a negative error code. + */ +int acpi_match_platform_list(const struct acpi_platform_list *plat) +{ + struct acpi_table_header hdr; + int idx = 0; + + if (acpi_disabled) + return -ENODEV; + + for (; plat->oem_id[0]; plat++, idx++) { + if (ACPI_FAILURE(acpi_get_table_header(plat->table, 0, &hdr))) + continue; + + if (strncmp(plat->oem_id, hdr.oem_id, ACPI_OEM_ID_SIZE)) + continue; + + if (strncmp(plat->oem_table_id, hdr.oem_table_id, + ACPI_OEM_TABLE_ID_SIZE)) + continue; + + if ((plat->pred == all_versions) || + (plat->pred == less_than_or_equal + && hdr.oem_revision <= plat->oem_revision) || + (plat->pred == greater_than_or_equal + && hdr.oem_revision >= plat->oem_revision) || + (plat->pred == equal + && hdr.oem_revision == plat->oem_revision)) + return idx; + } + + return -ENODEV; +} +EXPORT_SYMBOL(acpi_match_platform_list); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 27b4b66..a9b6dc2 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -556,6 +556,25 @@ extern acpi_status acpi_pci_osc_control_set(acpi_handle handle, #define ACPI_OST_SC_DRIVER_LOAD_FAILURE 0x81 #define ACPI_OST_SC_INSERT_NOT_SUPPORTED 0x82 +enum acpi_predicate { + all_versions, + less_than_or_equal, + equal, + greater_than_or_equal, +}; + +/* Table must be terminted by a NULL entry */ +struct acpi_platform_list { + char oem_id[ACPI_OEM_ID_SIZE]; + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE]; + u32 oem_revision; + char *table; + enum acpi_predicate pred; + char *reason; + u32 data; +}; +int acpi_match_platform_list(const struct acpi_platform_list *plat); + extern void acpi_early_init(void); extern void acpi_subsystem_init(void); ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [v3,1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-18 19:46 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-18 19:46 UTC (permalink / raw) To: rjw, bp Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel, Toshi Kani ACPI OEM ID / OEM Table ID / Revision can be used to identify a platform based on ACPI firmware info. acpi_blacklisted(), intel_pstate_platform_pwr_mgmt_exists(), and some other funcs, have been using similar check to detect a list of platforms that require special handlings. Move the platform check in acpi_blacklisted() to a new common utility function, acpi_match_platform_list(), so that other drivers do not have to implement their own version. There is no change in functionality. Signed-off-by: Toshi Kani <toshi.kani@hpe.com> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Borislav Petkov <bp@alien8.de> --- drivers/acpi/blacklist.c | 83 ++++++++-------------------------------------- drivers/acpi/utils.c | 40 ++++++++++++++++++++++ include/linux/acpi.h | 19 +++++++++++ 3 files changed, 73 insertions(+), 69 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c index bb542ac..037fd53 100644 --- a/drivers/acpi/blacklist.c +++ b/drivers/acpi/blacklist.c @@ -30,30 +30,13 @@ #include "internal.h" -enum acpi_blacklist_predicates { - all_versions, - less_than_or_equal, - equal, - greater_than_or_equal, -}; - -struct acpi_blacklist_item { - char oem_id[7]; - char oem_table_id[9]; - u32 oem_revision; - char *table; - enum acpi_blacklist_predicates oem_revision_predicate; - char *reason; - u32 is_critical_error; -}; - static struct dmi_system_id acpi_rev_dmi_table[] __initdata; /* * POLICY: If *anything* doesn't work, put it on the blacklist. * If they are critical errors, mark it critical, and abort driver load. */ -static struct acpi_blacklist_item acpi_blacklist[] __initdata = { +static struct acpi_platform_list acpi_blacklist[] __initdata = { /* Compaq Presario 1700 */ {"PTLTD ", " DSDT ", 0x06040000, ACPI_SIG_DSDT, less_than_or_equal, "Multiple problems", 1}, @@ -67,65 +50,27 @@ static struct acpi_blacklist_item acpi_blacklist[] __initdata = { {"IBM ", "TP600E ", 0x00000105, ACPI_SIG_DSDT, less_than_or_equal, "Incorrect _ADR", 1}, - {""} + { } }; int __init acpi_blacklisted(void) { - int i = 0; + int i; int blacklisted = 0; - struct acpi_table_header table_header; - - while (acpi_blacklist[i].oem_id[0] != '\0') { - if (acpi_get_table_header(acpi_blacklist[i].table, 0, &table_header)) { - i++; - continue; - } - - if (strncmp(acpi_blacklist[i].oem_id, table_header.oem_id, 6)) { - i++; - continue; - } - - if (strncmp - (acpi_blacklist[i].oem_table_id, table_header.oem_table_id, - 8)) { - i++; - continue; - } - - if ((acpi_blacklist[i].oem_revision_predicate == all_versions) - || (acpi_blacklist[i].oem_revision_predicate == - less_than_or_equal - && table_header.oem_revision <= - acpi_blacklist[i].oem_revision) - || (acpi_blacklist[i].oem_revision_predicate == - greater_than_or_equal - && table_header.oem_revision >= - acpi_blacklist[i].oem_revision) - || (acpi_blacklist[i].oem_revision_predicate == equal - && table_header.oem_revision == - acpi_blacklist[i].oem_revision)) { - printk(KERN_ERR PREFIX - "Vendor \"%6.6s\" System \"%8.8s\" " - "Revision 0x%x has a known ACPI BIOS problem.\n", - acpi_blacklist[i].oem_id, - acpi_blacklist[i].oem_table_id, - acpi_blacklist[i].oem_revision); + i = acpi_match_platform_list(acpi_blacklist); + if (i >= 0) { + pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\" Revision 0x%x has a known ACPI BIOS problem.\n", + acpi_blacklist[i].oem_id, + acpi_blacklist[i].oem_table_id, + acpi_blacklist[i].oem_revision); - printk(KERN_ERR PREFIX - "Reason: %s. This is a %s error\n", - acpi_blacklist[i].reason, - (acpi_blacklist[i]. - is_critical_error ? "non-recoverable" : - "recoverable")); + pr_err(PREFIX "Reason: %s. This is a %s error\n", + acpi_blacklist[i].reason, + (acpi_blacklist[i].data ? + "non-recoverable" : "recoverable")); - blacklisted = acpi_blacklist[i].is_critical_error; - break; - } else { - i++; - } + blacklisted = acpi_blacklist[i].data; } (void)early_acpi_osi_init(); diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index b9d956c..998aaf5 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -816,3 +816,43 @@ static int __init acpi_backlight(char *str) return 1; } __setup("acpi_backlight=", acpi_backlight); + +/** + * acpi_match_platform_list - Check if the system matches with a given list + * @plat: pointer to acpi_platform_list table terminated by a NULL entry + * + * Return the matched index if the system is found in the platform list. + * Otherwise, return a negative error code. + */ +int acpi_match_platform_list(const struct acpi_platform_list *plat) +{ + struct acpi_table_header hdr; + int idx = 0; + + if (acpi_disabled) + return -ENODEV; + + for (; plat->oem_id[0]; plat++, idx++) { + if (ACPI_FAILURE(acpi_get_table_header(plat->table, 0, &hdr))) + continue; + + if (strncmp(plat->oem_id, hdr.oem_id, ACPI_OEM_ID_SIZE)) + continue; + + if (strncmp(plat->oem_table_id, hdr.oem_table_id, + ACPI_OEM_TABLE_ID_SIZE)) + continue; + + if ((plat->pred == all_versions) || + (plat->pred == less_than_or_equal + && hdr.oem_revision <= plat->oem_revision) || + (plat->pred == greater_than_or_equal + && hdr.oem_revision >= plat->oem_revision) || + (plat->pred == equal + && hdr.oem_revision == plat->oem_revision)) + return idx; + } + + return -ENODEV; +} +EXPORT_SYMBOL(acpi_match_platform_list); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 27b4b66..a9b6dc2 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -556,6 +556,25 @@ extern acpi_status acpi_pci_osc_control_set(acpi_handle handle, #define ACPI_OST_SC_DRIVER_LOAD_FAILURE 0x81 #define ACPI_OST_SC_INSERT_NOT_SUPPORTED 0x82 +enum acpi_predicate { + all_versions, + less_than_or_equal, + equal, + greater_than_or_equal, +}; + +/* Table must be terminted by a NULL entry */ +struct acpi_platform_list { + char oem_id[ACPI_OEM_ID_SIZE]; + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE]; + u32 oem_revision; + char *table; + enum acpi_predicate pred; + char *reason; + u32 data; +}; +int acpi_match_platform_list(const struct acpi_platform_list *plat); + extern void acpi_early_init(void); extern void acpi_subsystem_init(void); ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 11:27 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-21 11:27 UTC (permalink / raw) To: Toshi Kani, rjw Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel 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. ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [v3,1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 11:27 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-21 11:27 UTC (permalink / raw) To: Toshi Kani, rjw Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel On Fri, Aug 18, 2017 at 01:46:40PM -0600, Toshi Kani wrote: > ACPI OEM ID / OEM Table ID / Revision can be used to identify > a platform based on ACPI firmware info. acpi_blacklisted(), > intel_pstate_platform_pwr_mgmt_exists(), and some other funcs, > have been using similar check to detect a list of platforms > that require special handlings. > > Move the platform check in acpi_blacklisted() to a new common > utility function, acpi_match_platform_list(), so that other > drivers do not have to implement their own version. > > There is no change in functionality. > > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Borislav Petkov <bp@alien8.de> > --- > drivers/acpi/blacklist.c | 83 ++++++++-------------------------------------- > drivers/acpi/utils.c | 40 ++++++++++++++++++++++ > include/linux/acpi.h | 19 +++++++++++ > 3 files changed, 73 insertions(+), 69 deletions(-) ... > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index b9d956c..998aaf5 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -816,3 +816,43 @@ static int __init acpi_backlight(char *str) > return 1; > } > __setup("acpi_backlight=", acpi_backlight); > + > +/** > + * acpi_match_platform_list - Check if the system matches with a given list > + * @plat: pointer to acpi_platform_list table terminated by a NULL entry > + * > + * Return the matched index if the system is found in the platform list. > + * Otherwise, return a negative error code. > + */ > +int acpi_match_platform_list(const struct acpi_platform_list *plat) > +{ > + struct acpi_table_header hdr; > + int idx = 0; > + > + if (acpi_disabled) > + return -ENODEV; Btw, Rafael, should we do something like this: --- in order to avoid that sprinkling of if (acpi_disabled) everywhere? > + > + for (; plat->oem_id[0]; plat++, idx++) { > + if (ACPI_FAILURE(acpi_get_table_header(plat->table, 0, &hdr))) > + continue; > + > + if (strncmp(plat->oem_id, hdr.oem_id, ACPI_OEM_ID_SIZE)) > + continue; > + > + if (strncmp(plat->oem_table_id, hdr.oem_table_id, > + ACPI_OEM_TABLE_ID_SIZE)) Let that stick out. > + continue; > + > + if ((plat->pred == all_versions) || > + (plat->pred == less_than_or_equal > + && hdr.oem_revision <= plat->oem_revision) || > + (plat->pred == greater_than_or_equal > + && hdr.oem_revision >= plat->oem_revision) || > + (plat->pred == equal > + && hdr.oem_revision == plat->oem_revision)) > + return idx; Make that more readable: if ((plat->pred == all_versions) || (plat->pred == less_than_or_equal && hdr.oem_revision <= plat->oem_revision) || (plat->pred == greater_than_or_equal && hdr.oem_revision >= plat->oem_revision) || (plat->pred == equal && hdr.oem_revision == plat->oem_revision)) return idx; > + } > + > + return -ENODEV; > +} > +EXPORT_SYMBOL(acpi_match_platform_list); > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 27b4b66..a9b6dc2 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -556,6 +556,25 @@ extern acpi_status acpi_pci_osc_control_set(acpi_handle handle, > #define ACPI_OST_SC_DRIVER_LOAD_FAILURE 0x81 > #define ACPI_OST_SC_INSERT_NOT_SUPPORTED 0x82 > > +enum acpi_predicate { > + all_versions, > + less_than_or_equal, > + equal, > + greater_than_or_equal, > +}; > + > +/* Table must be terminted by a NULL entry */ > +struct acpi_platform_list { > + char oem_id[ACPI_OEM_ID_SIZE]; + 1 > + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE]; + 1 > + u32 oem_revision; > + char *table; > + enum acpi_predicate pred; > + char *reason; > + u32 data; Ok, turning that into data from is_critical_error is a step in the right direction. Let's make it even better: u32 flags; and do #define ACPI_PLAT_IS_CRITICAL_ERROR BIT(0) so that future elements add new bits instead of wasting a whole u32 as a boolean. diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c index 010b1c43df92..881b0d5b2838 100644 --- a/drivers/acpi/acpica/tbxface.c +++ b/drivers/acpi/acpica/tbxface.c @@ -226,6 +226,9 @@ acpi_get_table_header(char *signature, u32 j; struct acpi_table_header *header; + if (acpi_disabled) + return (AE_ERROR); + /* Parameter validation */ if (!signature || !out_table_header) { ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 12:25 ` Rafael J. Wysocki 0 siblings, 0 replies; 55+ messages in thread From: Rafael J. Wysocki @ 2017-08-21 12:25 UTC (permalink / raw) To: Borislav Petkov Cc: Toshi Kani, Rafael J. Wysocki, Mauro Carvalho Chehab, Tony Luck, Len Brown, ACPI Devel Maling List, open list:EDAC-CORE, Linux Kernel Mailing 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 12:25 ` Rafael J. Wysocki 0 siblings, 0 replies; 55+ messages in thread From: Rafael J. Wysocki @ 2017-08-21 12:25 UTC (permalink / raw) To: Borislav Petkov Cc: Toshi Kani, Rafael J. Wysocki, Mauro Carvalho Chehab, Tony Luck, Len Brown, ACPI Devel Maling List, open list:EDAC-CORE, Linux Kernel Mailing 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 --- To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] ACPICA: Check whether ACPI is disabled before getting a table @ 2017-08-21 13:20 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-21 13:20 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Toshi Kani, Rafael J. Wysocki, Mauro Carvalho Chehab, Tony Luck, Len Brown, ACPI Devel Maling List, open list:EDAC-CORE, Linux Kernel Mailing List 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. ^ permalink raw reply related [flat|nested] 55+ messages in thread
* ACPICA: Check whether ACPI is disabled before getting a table @ 2017-08-21 13:20 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-21 13:20 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Toshi Kani, Rafael J. Wysocki, Mauro Carvalho Chehab, Tony Luck, Len Brown, ACPI Devel Maling List, open list:EDAC-CORE, Linux Kernel Mailing List 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) { ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH] ACPICA: Check whether ACPI is disabled before getting a table @ 2017-08-21 13:30 ` Rafael J. Wysocki 0 siblings, 0 replies; 55+ messages in thread From: Rafael J. Wysocki @ 2017-08-21 13:30 UTC (permalink / raw) To: Borislav Petkov Cc: Rafael J. Wysocki, Toshi Kani, Rafael J. Wysocki, Mauro Carvalho Chehab, Tony Luck, Len Brown, ACPI Devel Maling List, open list:EDAC-CORE, Linux Kernel Mailing List 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* ACPICA: Check whether ACPI is disabled before getting a table @ 2017-08-21 13:30 ` Rafael J. Wysocki 0 siblings, 0 replies; 55+ messages in thread From: Rafael J. Wysocki @ 2017-08-21 13:30 UTC (permalink / raw) To: Borislav Petkov Cc: Rafael J. Wysocki, Toshi Kani, Rafael J. Wysocki, Mauro Carvalho Chehab, Tony Luck, Len Brown, ACPI Devel Maling List, open list:EDAC-CORE, Linux Kernel Mailing List On Mon, Aug 21, 2017 at 3:20 PM, Borislav Petkov <bp@alien8.de> wrote: > Borislav Petkov <bp@suse.de> > > ... and save us the splrinking of this test around in the callers. > > No functionality change. > > Signed-off-by: Borislav Petkov <bp@suse.de> > --- > drivers/acpi/acpica/tbxface.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c > index 010b1c43df92..881b0d5b2838 100644 > --- a/drivers/acpi/acpica/tbxface.c > +++ b/drivers/acpi/acpica/tbxface.c > @@ -226,6 +226,9 @@ acpi_get_table_header(char *signature, > u32 j; > struct acpi_table_header *header; > > + if (acpi_disabled) > + return (AE_ERROR); > + > /* Parameter validation */ > > if (!signature || !out_table_header) { > -- Is acpi_disabled an ACPICA variable? If not, this cannot go upstream. Not a big deal I guess, but somewhat yucky. --- To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] ACPICA: Check whether ACPI is disabled before getting a table @ 2017-08-21 15:34 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-21 15:34 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Toshi Kani, Rafael J. Wysocki, Mauro Carvalho Chehab, Tony Luck, Len Brown, ACPI Devel Maling List, open list:EDAC-CORE, Linux Kernel Mailing List 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* ACPICA: Check whether ACPI is disabled before getting a table @ 2017-08-21 15:34 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-21 15:34 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Toshi Kani, Rafael J. Wysocki, Mauro Carvalho Chehab, Tony Luck, Len Brown, ACPI Devel Maling List, open list:EDAC-CORE, Linux Kernel Mailing List 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] ACPICA: Check whether ACPI is disabled before getting a table @ 2017-09-03 0:43 ` kbuild test robot 0 siblings, 0 replies; 55+ messages in thread From: kbuild test robot @ 2017-09-03 0:43 UTC (permalink / raw) To: Borislav Petkov Cc: kbuild-all, Rafael J. Wysocki, Toshi Kani, Rafael J. Wysocki, Mauro Carvalho Chehab, Tony Luck, Len Brown, ACPI Devel Maling List, open list:EDAC-CORE, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 3985 bytes --] Hi Borislav, [auto build test ERROR on pm/linux-next] [also build test ERROR on v4.13-rc7 next-20170901] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Borislav-Petkov/ACPICA-Check-whether-ACPI-is-disabled-before-getting-a-table/20170823-041359 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next config: arm64-defconfig (attached as .config) compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm64 All errors (new ones prefixed by >>): drivers/acpi/acpica/tbxface.c: In function 'acpi_get_table_header': >> drivers/acpi/acpica/tbxface.c:254:6: error: 'acpi_disabled' undeclared (first use in this function) if (acpi_disabled) ^~~~~~~~~~~~~ drivers/acpi/acpica/tbxface.c:254:6: note: each undeclared identifier is reported only once for each function it appears in vim +/acpi_disabled +254 drivers/acpi/acpica/tbxface.c 229 230 /******************************************************************************* 231 * 232 * FUNCTION: acpi_get_table_header 233 * 234 * PARAMETERS: signature - ACPI signature of needed table 235 * instance - Which instance (for SSDTs) 236 * out_table_header - The pointer to the table header to fill 237 * 238 * RETURN: Status and pointer to mapped table header 239 * 240 * DESCRIPTION: Finds an ACPI table header. 241 * 242 * NOTE: Caller is responsible in unmapping the header with 243 * acpi_os_unmap_memory 244 * 245 ******************************************************************************/ 246 acpi_status 247 acpi_get_table_header(char *signature, 248 u32 instance, struct acpi_table_header *out_table_header) 249 { 250 u32 i; 251 u32 j; 252 struct acpi_table_header *header; 253 > 254 if (acpi_disabled) 255 return (AE_ERROR); 256 257 /* Parameter validation */ 258 259 if (!signature || !out_table_header) { 260 return (AE_BAD_PARAMETER); 261 } 262 263 /* Walk the root table list */ 264 265 for (i = 0, j = 0; i < acpi_gbl_root_table_list.current_table_count; 266 i++) { 267 if (!ACPI_COMPARE_NAME 268 (&(acpi_gbl_root_table_list.tables[i].signature), 269 signature)) { 270 continue; 271 } 272 273 if (++j < instance) { 274 continue; 275 } 276 277 if (!acpi_gbl_root_table_list.tables[i].pointer) { 278 if ((acpi_gbl_root_table_list.tables[i].flags & 279 ACPI_TABLE_ORIGIN_MASK) == 280 ACPI_TABLE_ORIGIN_INTERNAL_PHYSICAL) { 281 header = 282 acpi_os_map_memory(acpi_gbl_root_table_list. 283 tables[i].address, 284 sizeof(struct 285 acpi_table_header)); 286 if (!header) { 287 return (AE_NO_MEMORY); 288 } 289 290 memcpy(out_table_header, header, 291 sizeof(struct acpi_table_header)); 292 acpi_os_unmap_memory(header, 293 sizeof(struct 294 acpi_table_header)); 295 } else { 296 return (AE_NOT_FOUND); 297 } 298 } else { 299 memcpy(out_table_header, 300 acpi_gbl_root_table_list.tables[i].pointer, 301 sizeof(struct acpi_table_header)); 302 } 303 return (AE_OK); 304 } 305 306 return (AE_NOT_FOUND); 307 } 308 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 36306 bytes --] ^ permalink raw reply [flat|nested] 55+ messages in thread
* ACPICA: Check whether ACPI is disabled before getting a table @ 2017-09-03 0:43 ` kbuild test robot 0 siblings, 0 replies; 55+ messages in thread From: kbuild test robot @ 2017-09-03 0:43 UTC (permalink / raw) To: Borislav Petkov Cc: kbuild-all, Rafael J. Wysocki, Toshi Kani, Rafael J. Wysocki, Mauro Carvalho Chehab, Tony Luck, Len Brown, ACPI Devel Maling List, open list:EDAC-CORE, Linux Kernel Mailing List Hi Borislav, [auto build test ERROR on pm/linux-next] [also build test ERROR on v4.13-rc7 next-20170901] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Borislav-Petkov/ACPICA-Check-whether-ACPI-is-disabled-before-getting-a-table/20170823-041359 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next config: arm64-defconfig (attached as .config) compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm64 All errors (new ones prefixed by >>): drivers/acpi/acpica/tbxface.c: In function 'acpi_get_table_header': >> drivers/acpi/acpica/tbxface.c:254:6: error: 'acpi_disabled' undeclared (first use in this function) if (acpi_disabled) ^~~~~~~~~~~~~ drivers/acpi/acpica/tbxface.c:254:6: note: each undeclared identifier is reported only once for each function it appears in vim +/acpi_disabled +254 drivers/acpi/acpica/tbxface.c 229 230 /******************************************************************************* 231 * 232 * FUNCTION: acpi_get_table_header 233 * 234 * PARAMETERS: signature - ACPI signature of needed table 235 * instance - Which instance (for SSDTs) 236 * out_table_header - The pointer to the table header to fill 237 * 238 * RETURN: Status and pointer to mapped table header 239 * 240 * DESCRIPTION: Finds an ACPI table header. 241 * 242 * NOTE: Caller is responsible in unmapping the header with 243 * acpi_os_unmap_memory 244 * 245 ******************************************************************************/ 246 acpi_status 247 acpi_get_table_header(char *signature, 248 u32 instance, struct acpi_table_header *out_table_header) 249 { 250 u32 i; 251 u32 j; 252 struct acpi_table_header *header; 253 > 254 if (acpi_disabled) 255 return (AE_ERROR); 256 257 /* Parameter validation */ 258 259 if (!signature || !out_table_header) { 260 return (AE_BAD_PARAMETER); 261 } 262 263 /* Walk the root table list */ 264 265 for (i = 0, j = 0; i < acpi_gbl_root_table_list.current_table_count; 266 i++) { 267 if (!ACPI_COMPARE_NAME 268 (&(acpi_gbl_root_table_list.tables[i].signature), 269 signature)) { 270 continue; 271 } 272 273 if (++j < instance) { 274 continue; 275 } 276 277 if (!acpi_gbl_root_table_list.tables[i].pointer) { 278 if ((acpi_gbl_root_table_list.tables[i].flags & 279 ACPI_TABLE_ORIGIN_MASK) == 280 ACPI_TABLE_ORIGIN_INTERNAL_PHYSICAL) { 281 header = 282 acpi_os_map_memory(acpi_gbl_root_table_list. 283 tables[i].address, 284 sizeof(struct 285 acpi_table_header)); 286 if (!header) { 287 return (AE_NO_MEMORY); 288 } 289 290 memcpy(out_table_header, header, 291 sizeof(struct acpi_table_header)); 292 acpi_os_unmap_memory(header, 293 sizeof(struct 294 acpi_table_header)); 295 } else { 296 return (AE_NOT_FOUND); 297 } 298 } else { 299 memcpy(out_table_header, 300 acpi_gbl_root_table_list.tables[i].pointer, 301 sizeof(struct acpi_table_header)); 302 } 303 return (AE_OK); 304 } 305 306 return (AE_NOT_FOUND); 307 } 308 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 16:41 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Kani, Toshimitsu @ 2017-08-21 16:41 UTC (permalink / raw) To: bp, rjw; +Cc: lenb, mchehab, tony.luck, linux-kernel, linux-acpi, linux-edac 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 16:41 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-21 16:41 UTC (permalink / raw) To: bp, rjw; +Cc: lenb, mchehab, tony.luck, linux-kernel, linux-acpi, linux-edac 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 17:04 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-21 17:04 UTC (permalink / raw) To: Kani, Toshimitsu Cc: rjw, lenb, mchehab, tony.luck, linux-kernel, linux-acpi, linux-edac 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 17:04 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-21 17:04 UTC (permalink / raw) To: Kani, Toshimitsu Cc: rjw, lenb, mchehab, tony.luck, linux-kernel, linux-acpi, linux-edac 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 17:23 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Kani, Toshimitsu @ 2017-08-21 17:23 UTC (permalink / raw) To: bp; +Cc: linux-edac, lenb, mchehab, tony.luck, linux-kernel, rjw, linux-acpi 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 17:23 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-21 17:23 UTC (permalink / raw) To: bp; +Cc: linux-edac, lenb, mchehab, tony.luck, linux-kernel, rjw, linux-acpi 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 17:36 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-21 17:36 UTC (permalink / raw) To: Kani, Toshimitsu Cc: linux-edac, lenb, mchehab, tony.luck, linux-kernel, rjw, linux-acpi 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 17:36 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-21 17:36 UTC (permalink / raw) To: Kani, Toshimitsu Cc: linux-edac, lenb, mchehab, tony.luck, linux-kernel, rjw, linux-acpi 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. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 20:31 ` Rafael J. Wysocki 0 siblings, 0 replies; 55+ messages in thread From: Rafael J. Wysocki @ 2017-08-21 20:31 UTC (permalink / raw) To: Borislav Petkov Cc: Kani, Toshimitsu, linux-edac, lenb, mchehab, tony.luck, linux-kernel, rjw, linux-acpi 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 20:31 ` Rafael J. Wysocki 0 siblings, 0 replies; 55+ messages in thread From: Rafael J. Wysocki @ 2017-08-21 20:31 UTC (permalink / raw) To: Borislav Petkov Cc: Kani, Toshimitsu, linux-edac, lenb, mchehab, tony.luck, linux-kernel, rjw, linux-acpi On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de> wrote: > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote: >> > > 'data' here is private to the caller. So, I do not think we need >> > > to define the bits. Shall I change the name to 'driver_data' to >> > > make it more explicit? >> > >> > You changed it to 'data'. It was a u32-used-as-boolean >> > is_critical_error before. >> > >> > So you can just as well make it into flags and people can extend >> > those flags if needed. A flag bit should be enough in most cases >> > anyway. If they really need driver_data, then they can add a void * >> > member. >> >> Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses this >> field for PSS and PCC, which are enum values. I think we should allow >> drivers to set any values here. I agree that it may need to be void * >> if we also allow drivers to set a pointer here. > > Let's see what Rafael prefers. I would retain the is_critical_error field and use that for printing the recoverable / non-recoverable message. This is kind of orthogonal to whether or not any extra data is needed and that can be an additional field. In that case unsigned long should be sufficient to accommodate a pointer if need be. Thanks, Rafael --- To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 21:06 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Kani, Toshimitsu @ 2017-08-21 21:06 UTC (permalink / raw) To: bp, rafael Cc: linux-acpi, lenb, mchehab, tony.luck, linux-kernel, linux-edac, rjw 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 21:06 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-21 21:06 UTC (permalink / raw) To: bp, rafael Cc: linux-acpi, lenb, mchehab, tony.luck, linux-kernel, linux-edac, rjw 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 21:49 ` Rafael J. Wysocki 0 siblings, 0 replies; 55+ messages in thread From: Rafael J. Wysocki @ 2017-08-21 21:49 UTC (permalink / raw) To: Kani, Toshimitsu Cc: bp, rafael, linux-acpi, lenb, mchehab, tony.luck, linux-kernel, linux-edac, rjw 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 21:49 ` Rafael J. Wysocki 0 siblings, 0 replies; 55+ messages in thread From: Rafael J. Wysocki @ 2017-08-21 21:49 UTC (permalink / raw) To: Kani, Toshimitsu Cc: bp, rafael, linux-acpi, lenb, mchehab, tony.luck, linux-kernel, linux-edac, rjw On Mon, Aug 21, 2017 at 11:06 PM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote: > On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote: >> On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de> >> wrote: >> > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote: >> > > > > 'data' here is private to the caller. So, I do not think we >> > > > > need to define the bits. Shall I change the name to >> > > > > 'driver_data' to make it more explicit? >> > > > >> > > > You changed it to 'data'. It was a u32-used-as-boolean >> > > > is_critical_error before. >> > > > >> > > > So you can just as well make it into flags and people can >> > > > extend those flags if needed. A flag bit should be enough in >> > > > most cases anyway. If they really need driver_data, then they >> > > > can add a void *member. >> > > >> > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses >> > > this field for PSS and PCC, which are enum values. I think we >> > > should allow drivers to set any values here. I agree that it may >> > > need to be void * if we also allow drivers to set a pointer here. >> > >> > Let's see what Rafael prefers. >> >> I would retain the is_critical_error field and use that for printing >> the recoverable / non-recoverable message. This is kind of >> orthogonal to whether or not any extra data is needed and that can be >> an additional field. In that case unsigned long should be sufficient >> to accommodate a pointer if need be. > > Yes, we will retain the field. The question is whether this field > should be retained as a driver's private data or ACPI-managed flags. Thanks for the clarification. > My patch implements the former, which lets the callers to define the > data values. For instance, acpi_blacklisted() uses this field as > is_critical_error value, and intel_pstate_platform_pwr_mgmt_exists() > uses it as oem_pwr_table value. > > Boris suggested the latter, which lets ACPI to define the flags, which > are then used by the callers. For instance, he suggested ACPI to > define bit0 as is_critical_error. > > #define ACPI_PLAT_IS_CRITICAL_ERROR BIT(0) So my point is that we can have both the ACPI-managed flags and the the caller-defined data at the same time as separate items. That would allow of maximum flexibility IMO. Thanks, Rafael --- To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 22:21 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Kani, Toshimitsu @ 2017-08-21 22:21 UTC (permalink / raw) To: rafael Cc: linux-kernel, mchehab, rjw, bp, tony.luck, lenb, linux-acpi, linux-edac 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 22:21 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-21 22:21 UTC (permalink / raw) To: rafael Cc: linux-kernel, mchehab, rjw, bp, tony.luck, lenb, linux-acpi, linux-edac 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 22:26 ` Rafael J. Wysocki 0 siblings, 0 replies; 55+ messages in thread From: Rafael J. Wysocki @ 2017-08-21 22:26 UTC (permalink / raw) To: Kani, Toshimitsu Cc: rafael, linux-kernel, mchehab, rjw, bp, tony.luck, lenb, linux-acpi, linux-edac 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 ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,1/5] ACPI / blacklist: add acpi_match_platform_list() @ 2017-08-21 22:26 ` Rafael J. Wysocki 0 siblings, 0 replies; 55+ messages in thread From: Rafael J. Wysocki @ 2017-08-21 22:26 UTC (permalink / raw) To: Kani, Toshimitsu Cc: rafael, linux-kernel, mchehab, rjw, bp, tony.luck, lenb, linux-acpi, linux-edac On Tue, Aug 22, 2017 at 12:21 AM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote: > On Mon, 2017-08-21 at 23:49 +0200, Rafael J. Wysocki wrote: >> On Mon, Aug 21, 2017 at 11:06 PM, Kani, Toshimitsu <toshi.kani@hpe.co >> m> wrote: >> > On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote: >> > > On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de> >> > > wrote: >> > > > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu >> > > > wrote: >> > > > > > > 'data' here is private to the caller. So, I do not think >> > > > > > > we need to define the bits. Shall I change the name to >> > > > > > > 'driver_data' to make it more explicit? >> > > > > > >> > > > > > You changed it to 'data'. It was a u32-used-as-boolean >> > > > > > is_critical_error before. >> > > > > > >> > > > > > So you can just as well make it into flags and people can >> > > > > > extend those flags if needed. A flag bit should be enough >> > > > > > in most cases anyway. If they really need driver_data, then >> > > > > > they can add a void *member. >> > > > > >> > > > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() >> > > > > uses this field for PSS and PCC, which are enum values. I >> > > > > think we should allow drivers to set any values here. I >> > > > > agree that it may need to be void * if we also allow drivers >> > > > > to set a pointer here. >> > > > >> > > > Let's see what Rafael prefers. >> > > >> > > I would retain the is_critical_error field and use that for >> > > printing the recoverable / non-recoverable message. This is kind >> > > of orthogonal to whether or not any extra data is needed and that >> > > can be an additional field. In that case unsigned long should be >> > > sufficient to accommodate a pointer if need be. >> > >> > Yes, we will retain the field. The question is whether this field >> > should be retained as a driver's private data or ACPI-managed >> > flags. >> >> Thanks for the clarification. >> >> > My patch implements the former, which lets the callers to define >> > the data values. For instance, acpi_blacklisted() uses this field >> > as is_critical_error value, and >> > intel_pstate_platform_pwr_mgmt_exists() uses it as oem_pwr_table >> > value. >> > >> > Boris suggested the latter, which lets ACPI to define the flags, >> > which are then used by the callers. For instance, he suggested >> > ACPI to define bit0 as is_critical_error. >> > >> > #define ACPI_PLAT_IS_CRITICAL_ERROR BIT(0) >> >> So my point is that we can have both the ACPI-managed flags and the >> the caller-defined data at the same time as separate items. >> >> That would allow of maximum flexibility IMO. > > I agree in general. Driver private data allows flexibility to drivers > when the values are driver-private. ACPI-managed flags allows ACPI to > control the interfaces based on the flags. > > Since we do not have use-case of the latter case yet, i.e. > acpi_match_platform_list() does not need to check the flags, I'd > suggest that we keep 'data' as driver-private. We can add 'flags' as a > separate member to the structure when we find the latter use-case. OK Thanks, Rafael --- To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v3 2/5] intel_pstate: convert to use acpi_match_platform_list() @ 2017-08-18 19:46 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-18 19:46 UTC (permalink / raw) To: rjw, bp Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel, Toshi Kani, Srinivas Pandruvada 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; ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [v3,2/5] intel_pstate: convert to use acpi_match_platform_list() @ 2017-08-18 19:46 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-18 19:46 UTC (permalink / raw) To: rjw, bp Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel, Toshi Kani, Srinivas Pandruvada Convert to use acpi_match_platform_list() for the platform check. There is no change in functionality. Signed-off-by: Toshi Kani <toshi.kani@hpe.com> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> Cc: Len Brown <lenb@kernel.org> Cc: Borislav Petkov <bp@alien8.de> --- drivers/cpufreq/intel_pstate.c | 64 ++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 39 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c index 65ee4fc..ad713cd 100644 --- a/drivers/cpufreq/intel_pstate.c +++ b/drivers/cpufreq/intel_pstate.c @@ -2466,39 +2466,31 @@ enum { PPC, }; -struct hw_vendor_info { - u16 valid; - char oem_id[ACPI_OEM_ID_SIZE]; - char oem_table_id[ACPI_OEM_TABLE_ID_SIZE]; - int oem_pwr_table; -}; - /* Hardware vendor-specific info that has its own power management modes */ -static struct hw_vendor_info vendor_info[] __initdata = { - {1, "HP ", "ProLiant", PSS}, - {1, "ORACLE", "X4-2 ", PPC}, - {1, "ORACLE", "X4-2L ", PPC}, - {1, "ORACLE", "X4-2B ", PPC}, - {1, "ORACLE", "X3-2 ", PPC}, - {1, "ORACLE", "X3-2L ", PPC}, - {1, "ORACLE", "X3-2B ", PPC}, - {1, "ORACLE", "X4470M2 ", PPC}, - {1, "ORACLE", "X4270M3 ", PPC}, - {1, "ORACLE", "X4270M2 ", PPC}, - {1, "ORACLE", "X4170M2 ", PPC}, - {1, "ORACLE", "X4170 M3", PPC}, - {1, "ORACLE", "X4275 M3", PPC}, - {1, "ORACLE", "X6-2 ", PPC}, - {1, "ORACLE", "Sudbury ", PPC}, - {0, "", ""}, +static struct acpi_platform_list plat_info[] __initdata = { + {"HP ", "ProLiant", 0, ACPI_SIG_FADT, all_versions, 0, PSS}, + {"ORACLE", "X4-2 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + {"ORACLE", "X4-2L ", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + {"ORACLE", "X4-2B ", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + {"ORACLE", "X3-2 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + {"ORACLE", "X3-2L ", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + {"ORACLE", "X3-2B ", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + {"ORACLE", "X4470M2 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + {"ORACLE", "X4270M3 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + {"ORACLE", "X4270M2 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + {"ORACLE", "X4170M2 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + {"ORACLE", "X4170 M3", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + {"ORACLE", "X4275 M3", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + {"ORACLE", "X6-2 ", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + {"ORACLE", "Sudbury ", 0, ACPI_SIG_FADT, all_versions, 0, PPC}, + { } /* End */ }; static bool __init intel_pstate_platform_pwr_mgmt_exists(void) { - struct acpi_table_header hdr; - struct hw_vendor_info *v_info; const struct x86_cpu_id *id; u64 misc_pwr; + int idx; id = x86_match_cpu(intel_pstate_cpu_oob_ids); if (id) { @@ -2507,21 +2499,15 @@ static bool __init intel_pstate_platform_pwr_mgmt_exists(void) return true; } - if (acpi_disabled || - ACPI_FAILURE(acpi_get_table_header(ACPI_SIG_FADT, 0, &hdr))) + idx = acpi_match_platform_list(plat_info); + if (idx < 0) return false; - for (v_info = vendor_info; v_info->valid; v_info++) { - if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) && - !strncmp(hdr.oem_table_id, v_info->oem_table_id, - ACPI_OEM_TABLE_ID_SIZE)) - switch (v_info->oem_pwr_table) { - case PSS: - return intel_pstate_no_acpi_pss(); - case PPC: - return intel_pstate_has_acpi_ppc() && - (!force_load); - } + switch (plat_info[idx].data) { + case PSS: + return intel_pstate_no_acpi_pss(); + case PPC: + return intel_pstate_has_acpi_ppc() && !force_load; } return false; ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v3 2/5] intel_pstate: convert to use acpi_match_platform_list() @ 2017-08-21 17:53 ` Srinivas Pandruvada 0 siblings, 0 replies; 55+ messages in thread From: Srinivas Pandruvada @ 2017-08-21 17:53 UTC (permalink / raw) To: Toshi Kani, rjw, bp Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel 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; ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,2/5] intel_pstate: convert to use acpi_match_platform_list() @ 2017-08-21 17:53 ` Srinivas Pandruvada 0 siblings, 0 replies; 55+ messages in thread From: Srinivas Pandruvada @ 2017-08-21 17:53 UTC (permalink / raw) To: Toshi Kani, rjw, bp Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel On Fri, 2017-08-18 at 13:46 -0600, Toshi Kani wrote: > Convert to use acpi_match_platform_list() for the platform check. > There is no change in functionality. > > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Cc: Len Brown <lenb@kernel.org> > Cc: Borislav Petkov <bp@alien8.de> Can't test but the change itself is OK. Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > drivers/cpufreq/intel_pstate.c | 64 ++++++++++++++++------------ > ------------ > 1 file changed, 25 insertions(+), 39 deletions(-) > > diff --git a/drivers/cpufreq/intel_pstate.c > b/drivers/cpufreq/intel_pstate.c > index 65ee4fc..ad713cd 100644 > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -2466,39 +2466,31 @@ enum { > PPC, > }; > > -struct hw_vendor_info { > - u16 valid; > - char oem_id[ACPI_OEM_ID_SIZE]; > - char oem_table_id[ACPI_OEM_TABLE_ID_SIZE]; > - int oem_pwr_table; > -}; > - > /* Hardware vendor-specific info that has its own power management > modes */ > -static struct hw_vendor_info vendor_info[] __initdata = { > - {1, "HP ", "ProLiant", PSS}, > - {1, "ORACLE", "X4-2 ", PPC}, > - {1, "ORACLE", "X4-2L ", PPC}, > - {1, "ORACLE", "X4-2B ", PPC}, > - {1, "ORACLE", "X3-2 ", PPC}, > - {1, "ORACLE", "X3-2L ", PPC}, > - {1, "ORACLE", "X3-2B ", PPC}, > - {1, "ORACLE", "X4470M2 ", PPC}, > - {1, "ORACLE", "X4270M3 ", PPC}, > - {1, "ORACLE", "X4270M2 ", PPC}, > - {1, "ORACLE", "X4170M2 ", PPC}, > - {1, "ORACLE", "X4170 M3", PPC}, > - {1, "ORACLE", "X4275 M3", PPC}, > - {1, "ORACLE", "X6-2 ", PPC}, > - {1, "ORACLE", "Sudbury ", PPC}, > - {0, "", ""}, > +static struct acpi_platform_list plat_info[] __initdata = { > + {"HP ", "ProLiant", 0, ACPI_SIG_FADT, all_versions, 0, > PSS}, > + {"ORACLE", "X4-2 ", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + {"ORACLE", "X4-2L ", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + {"ORACLE", "X4-2B ", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + {"ORACLE", "X3-2 ", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + {"ORACLE", "X3-2L ", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + {"ORACLE", "X3-2B ", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + {"ORACLE", "X4470M2 ", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + {"ORACLE", "X4270M3 ", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + {"ORACLE", "X4270M2 ", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + {"ORACLE", "X4170M2 ", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + {"ORACLE", "X4170 M3", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + {"ORACLE", "X4275 M3", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + {"ORACLE", "X6-2 ", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + {"ORACLE", "Sudbury ", 0, ACPI_SIG_FADT, all_versions, 0, > PPC}, > + { } /* End */ > }; > > static bool __init intel_pstate_platform_pwr_mgmt_exists(void) > { > - struct acpi_table_header hdr; > - struct hw_vendor_info *v_info; > const struct x86_cpu_id *id; > u64 misc_pwr; > + int idx; > > id = x86_match_cpu(intel_pstate_cpu_oob_ids); > if (id) { > @@ -2507,21 +2499,15 @@ static bool __init > intel_pstate_platform_pwr_mgmt_exists(void) > return true; > } > > - if (acpi_disabled || > - ACPI_FAILURE(acpi_get_table_header(ACPI_SIG_FADT, 0, > &hdr))) > + idx = acpi_match_platform_list(plat_info); > + if (idx < 0) > return false; > > - for (v_info = vendor_info; v_info->valid; v_info++) { > - if (!strncmp(hdr.oem_id, v_info->oem_id, > ACPI_OEM_ID_SIZE) && > - !strncmp(hdr.oem_table_id, v_info- > >oem_table_id, > - ACPI_OEM_TABLE_ID_SI > ZE)) > - switch (v_info->oem_pwr_table) { > - case PSS: > - return intel_pstate_no_acpi_pss(); > - case PPC: > - return intel_pstate_has_acpi_ppc() > && > - (!force_load); > - } > + switch (plat_info[idx].data) { > + case PSS: > + return intel_pstate_no_acpi_pss(); > + case PPC: > + return intel_pstate_has_acpi_ppc() && !force_load; > } > > return false; --- To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 2/5] intel_pstate: convert to use acpi_match_platform_list() @ 2017-08-23 15:46 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-23 15:46 UTC (permalink / raw) To: Toshi Kani Cc: rjw, mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel, Srinivas Pandruvada On Fri, Aug 18, 2017 at 01:46:41PM -0600, Toshi Kani wrote: > Convert to use acpi_match_platform_list() for the platform check. > There is no change in functionality. > > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Cc: Len Brown <lenb@kernel.org> > Cc: Borislav Petkov <bp@alien8.de> > --- > drivers/cpufreq/intel_pstate.c | 64 ++++++++++++++++------------------------ > 1 file changed, 25 insertions(+), 39 deletions(-) > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c > index 65ee4fc..ad713cd 100644 > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -2466,39 +2466,31 @@ enum { > PPC, > }; > > -struct hw_vendor_info { > - u16 valid; > - char oem_id[ACPI_OEM_ID_SIZE]; > - char oem_table_id[ACPI_OEM_TABLE_ID_SIZE]; > - int oem_pwr_table; > -}; > - > /* Hardware vendor-specific info that has its own power management modes */ > -static struct hw_vendor_info vendor_info[] __initdata = { > - {1, "HP ", "ProLiant", PSS}, > - {1, "ORACLE", "X4-2 ", PPC}, > - {1, "ORACLE", "X4-2L ", PPC}, > - {1, "ORACLE", "X4-2B ", PPC}, > - {1, "ORACLE", "X3-2 ", PPC}, > - {1, "ORACLE", "X3-2L ", PPC}, > - {1, "ORACLE", "X3-2B ", PPC}, > - {1, "ORACLE", "X4470M2 ", PPC}, > - {1, "ORACLE", "X4270M3 ", PPC}, > - {1, "ORACLE", "X4270M2 ", PPC}, > - {1, "ORACLE", "X4170M2 ", PPC}, > - {1, "ORACLE", "X4170 M3", PPC}, > - {1, "ORACLE", "X4275 M3", PPC}, > - {1, "ORACLE", "X6-2 ", PPC}, > - {1, "ORACLE", "Sudbury ", PPC}, > - {0, "", ""}, > +static struct acpi_platform_list plat_info[] __initdata = { > + {"HP ", "ProLiant", 0, ACPI_SIG_FADT, all_versions, 0, PSS}, Btw, why is that ACPI_SIG_FADT's description not "FADT" ? #define ACPI_SIG_FADT "FACP" /* Fixed ACPI Description Table */ More ACPI fun? I don't think I can take any more fun. Oh well, Reviewed-by: Borislav Petkov <bp@suse.de> -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,2/5] intel_pstate: convert to use acpi_match_platform_list() @ 2017-08-23 15:46 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-23 15:46 UTC (permalink / raw) To: Toshi Kani Cc: rjw, mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel, Srinivas Pandruvada On Fri, Aug 18, 2017 at 01:46:41PM -0600, Toshi Kani wrote: > Convert to use acpi_match_platform_list() for the platform check. > There is no change in functionality. > > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Cc: Len Brown <lenb@kernel.org> > Cc: Borislav Petkov <bp@alien8.de> > --- > drivers/cpufreq/intel_pstate.c | 64 ++++++++++++++++------------------------ > 1 file changed, 25 insertions(+), 39 deletions(-) > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c > index 65ee4fc..ad713cd 100644 > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -2466,39 +2466,31 @@ enum { > PPC, > }; > > -struct hw_vendor_info { > - u16 valid; > - char oem_id[ACPI_OEM_ID_SIZE]; > - char oem_table_id[ACPI_OEM_TABLE_ID_SIZE]; > - int oem_pwr_table; > -}; > - > /* Hardware vendor-specific info that has its own power management modes */ > -static struct hw_vendor_info vendor_info[] __initdata = { > - {1, "HP ", "ProLiant", PSS}, > - {1, "ORACLE", "X4-2 ", PPC}, > - {1, "ORACLE", "X4-2L ", PPC}, > - {1, "ORACLE", "X4-2B ", PPC}, > - {1, "ORACLE", "X3-2 ", PPC}, > - {1, "ORACLE", "X3-2L ", PPC}, > - {1, "ORACLE", "X3-2B ", PPC}, > - {1, "ORACLE", "X4470M2 ", PPC}, > - {1, "ORACLE", "X4270M3 ", PPC}, > - {1, "ORACLE", "X4270M2 ", PPC}, > - {1, "ORACLE", "X4170M2 ", PPC}, > - {1, "ORACLE", "X4170 M3", PPC}, > - {1, "ORACLE", "X4275 M3", PPC}, > - {1, "ORACLE", "X6-2 ", PPC}, > - {1, "ORACLE", "Sudbury ", PPC}, > - {0, "", ""}, > +static struct acpi_platform_list plat_info[] __initdata = { > + {"HP ", "ProLiant", 0, ACPI_SIG_FADT, all_versions, 0, PSS}, Btw, why is that ACPI_SIG_FADT's description not "FADT" ? #define ACPI_SIG_FADT "FACP" /* Fixed ACPI Description Table */ More ACPI fun? I don't think I can take any more fun. Oh well, Reviewed-by: Borislav Petkov <bp@suse.de> ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 2/5] intel_pstate: convert to use acpi_match_platform_list() 2017-08-23 15:46 ` [v3,2/5] " Borislav Petkov (?) @ 2017-08-23 15:56 ` Kani, Toshimitsu -1 siblings, 0 replies; 55+ messages in thread From: Kani, Toshimitsu @ 2017-08-23 15:56 UTC (permalink / raw) To: bp Cc: linux-kernel, mchehab, rjw, srinivas.pandruvada, tony.luck, lenb, linux-acpi, linux-edac On Wed, 2017-08-23 at 17:46 +0200, Borislav Petkov wrote: > On Fri, Aug 18, 2017 at 01:46:41PM -0600, Toshi Kani wrote: > > Convert to use acpi_match_platform_list() for the platform check. > > There is no change in functionality. > > : > > Btw, why is that ACPI_SIG_FADT's description not "FADT" ? > > #define ACPI_SIG_FADT "FACP" /* Fixed ACPI Description > Table */ > > More ACPI fun? I don't think I can take any more fun. Yes, more ACPI fun. :-) According to the spec: ‘FACP’. Signature for the Fixed ACPI Description Table. (This signature predates ACPI 1.0, explaining the mismatch with this table's name.) > Oh well, > > Reviewed-by: Borislav Petkov <bp@suse.de> Thanks! -Toshi ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,2/5] intel_pstate: convert to use acpi_match_platform_list() @ 2017-08-23 15:56 ` Kani, Toshimitsu 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-23 15:56 UTC (permalink / raw) To: bp Cc: linux-kernel, mchehab, rjw, srinivas.pandruvada, tony.luck, lenb, linux-acpi, linux-edac T24gV2VkLCAyMDE3LTA4LTIzIGF0IDE3OjQ2ICswMjAwLCBCb3Jpc2xhdiBQZXRrb3Ygd3JvdGU6 DQo+IE9uIEZyaSwgQXVnIDE4LCAyMDE3IGF0IDAxOjQ2OjQxUE0gLTA2MDAsIFRvc2hpIEthbmkg d3JvdGU6DQo+ID4gQ29udmVydCB0byB1c2UgYWNwaV9tYXRjaF9wbGF0Zm9ybV9saXN0KCkgZm9y IHRoZSBwbGF0Zm9ybSBjaGVjay4NCj4gPiBUaGVyZSBpcyBubyBjaGFuZ2UgaW4gZnVuY3Rpb25h bGl0eS4NCj4gPiANCiA6DQo+IA0KPiBCdHcsIHdoeSBpcyB0aGF0IEFDUElfU0lHX0ZBRFQncyBk ZXNjcmlwdGlvbiBub3QgIkZBRFQiID8NCj4gDQo+ICNkZWZpbmUgQUNQSV9TSUdfRkFEVMKgwqDC oMKgwqDCoMKgwqDCoMKgwqAiRkFDUCLCoMKgLyogRml4ZWQgQUNQSSBEZXNjcmlwdGlvbg0KPiBU YWJsZSAqLw0KPiANCj4gTW9yZSBBQ1BJIGZ1bj8gSSBkb24ndCB0aGluayBJIGNhbiB0YWtlIGFu eSBtb3JlIGZ1bi4NCg0KWWVzLCBtb3JlIEFDUEkgZnVuLiA6LSkgIEFjY29yZGluZyB0byB0aGUg c3BlYzoNCg0K4oCYRkFDUOKAmS4gU2lnbmF0dXJlIGZvciB0aGUgRml4ZWQgQUNQSSBEZXNjcmlw dGlvbiBUYWJsZS4gKFRoaXMgc2lnbmF0dXJlDQpwcmVkYXRlcyBBQ1BJIDEuMCwgZXhwbGFpbmlu ZyB0aGUgbWlzbWF0Y2ggd2l0aCB0aGlzIHRhYmxlJ3MgbmFtZS4pDQoNCj4gT2ggd2VsbCwNCj4g DQo+IFJldmlld2VkLWJ5OiBCb3Jpc2xhdiBQZXRrb3YgPGJwQHN1c2UuZGU+DQoNClRoYW5rcyEN Ci1Ub3NoaQ0K --- To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 2/5] intel_pstate: convert to use acpi_match_platform_list() @ 2017-08-23 15:56 ` Kani, Toshimitsu 0 siblings, 0 replies; 55+ messages in thread From: Kani, Toshimitsu @ 2017-08-23 15:56 UTC (permalink / raw) To: bp Cc: linux-kernel, mchehab, rjw, srinivas.pandruvada, tony.luck, lenb, linux-acpi, linux-edac On Wed, 2017-08-23 at 17:46 +0200, Borislav Petkov wrote: > On Fri, Aug 18, 2017 at 01:46:41PM -0600, Toshi Kani wrote: > > Convert to use acpi_match_platform_list() for the platform check. > > There is no change in functionality. > > : > > Btw, why is that ACPI_SIG_FADT's description not "FADT" ? > > #define ACPI_SIG_FADT "FACP" /* Fixed ACPI Description > Table */ > > More ACPI fun? I don't think I can take any more fun. Yes, more ACPI fun. :-) According to the spec: ‘FACP’. Signature for the Fixed ACPI Description Table. (This signature predates ACPI 1.0, explaining the mismatch with this table's name.) > Oh well, > > Reviewed-by: Borislav Petkov <bp@suse.de> Thanks! -Toshi ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac @ 2017-08-18 19:46 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-18 19:46 UTC (permalink / raw) To: rjw, bp Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel, Toshi Kani The ghes_edac driver was introduced in 2013 [1], but it has not been enabled by any distro yet. This driver obtains error info from firmware interfaces, which are not properly implemented on many platforms, as the driver always emits the messages below: This EDAC driver relies on BIOS to enumerate memory and get error reports. Unfortunately, not all BIOSes reflect the memory layout correctly So, the end result of using this driver varies from vendor to vendor If you find incorrect reports, please contact your hardware vendor to correct its BIOS. To get out from this situation, add a platform check to selectively enable the driver on the platforms that are known to have proper firmware implementation. Platform vendors can add their platforms to the list when they support ghes_edac. "ghes_edac.force_load=1" skips this platform check. [1]: https://lwn.net/Articles/538438/ Signed-off-by: Toshi Kani <toshi.kani@hpe.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Tony Luck <tony.luck@intel.com> --- drivers/edac/ghes_edac.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index 4e61a62..367e106 100644 --- a/drivers/edac/ghes_edac.c +++ b/drivers/edac/ghes_edac.c @@ -34,6 +34,9 @@ static LIST_HEAD(ghes_reglist); static DEFINE_MUTEX(ghes_edac_lock); static int ghes_edac_mc_num; +/* Set 1 to skip the platform check */ +static bool __read_mostly ghes_edac_force_load; +module_param_named(force_load, ghes_edac_force_load, bool, 0); /* Memory Device - Type 17 of SMBIOS spec */ struct memdev_dmi_entry { @@ -405,6 +408,15 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev, } EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error); +/* + * Known systems that are safe to enable this module. + * "ghes_edac.force_load=1" skips this check if necessary. + */ +static struct acpi_platform_list plat_list[] = { + {"HPE ", "Server ", 0, ACPI_SIG_FADT, all_versions}, + { } /* End */ +}; + int ghes_edac_register(struct ghes *ghes, struct device *dev) { bool fake = false; @@ -413,6 +425,12 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) struct edac_mc_layer layers[1]; struct ghes_edac_pvt *pvt; struct ghes_edac_dimm_fill dimm_fill; + int idx; + + /* Check if safe to enable on this system */ + idx = acpi_match_platform_list(plat_list); + if (!ghes_edac_force_load && idx < 0) + return 0; /* Get the number of DIMMs */ dmi_walk(ghes_edac_count_dimms, &num_dimm); @@ -456,7 +474,11 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) mci->dev_name = "ghes"; if (!ghes_edac_mc_num) { - if (!fake) { + if (fake) { + pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n"); + pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n"); + pr_info("work on such system. Use this driver with caution\n"); + } else if (idx < 0) { pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n"); pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n"); pr_info("So, the end result of using this driver varies from vendor to vendor.\n"); @@ -464,10 +486,6 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) pr_info("to correct its BIOS.\n"); pr_info("This system has %d DIMM sockets.\n", num_dimm); - } else { - pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n"); - pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n"); - pr_info("work on such system. Use this driver with caution\n"); } } ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [v3,3/5] ghes_edac: add platform check to enable ghes_edac @ 2017-08-18 19:46 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-18 19:46 UTC (permalink / raw) To: rjw, bp Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel, Toshi Kani The ghes_edac driver was introduced in 2013 [1], but it has not been enabled by any distro yet. This driver obtains error info from firmware interfaces, which are not properly implemented on many platforms, as the driver always emits the messages below: This EDAC driver relies on BIOS to enumerate memory and get error reports. Unfortunately, not all BIOSes reflect the memory layout correctly So, the end result of using this driver varies from vendor to vendor If you find incorrect reports, please contact your hardware vendor to correct its BIOS. To get out from this situation, add a platform check to selectively enable the driver on the platforms that are known to have proper firmware implementation. Platform vendors can add their platforms to the list when they support ghes_edac. "ghes_edac.force_load=1" skips this platform check. [1]: https://lwn.net/Articles/538438/ Signed-off-by: Toshi Kani <toshi.kani@hpe.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Tony Luck <tony.luck@intel.com> --- drivers/edac/ghes_edac.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index 4e61a62..367e106 100644 --- a/drivers/edac/ghes_edac.c +++ b/drivers/edac/ghes_edac.c @@ -34,6 +34,9 @@ static LIST_HEAD(ghes_reglist); static DEFINE_MUTEX(ghes_edac_lock); static int ghes_edac_mc_num; +/* Set 1 to skip the platform check */ +static bool __read_mostly ghes_edac_force_load; +module_param_named(force_load, ghes_edac_force_load, bool, 0); /* Memory Device - Type 17 of SMBIOS spec */ struct memdev_dmi_entry { @@ -405,6 +408,15 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev, } EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error); +/* + * Known systems that are safe to enable this module. + * "ghes_edac.force_load=1" skips this check if necessary. + */ +static struct acpi_platform_list plat_list[] = { + {"HPE ", "Server ", 0, ACPI_SIG_FADT, all_versions}, + { } /* End */ +}; + int ghes_edac_register(struct ghes *ghes, struct device *dev) { bool fake = false; @@ -413,6 +425,12 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) struct edac_mc_layer layers[1]; struct ghes_edac_pvt *pvt; struct ghes_edac_dimm_fill dimm_fill; + int idx; + + /* Check if safe to enable on this system */ + idx = acpi_match_platform_list(plat_list); + if (!ghes_edac_force_load && idx < 0) + return 0; /* Get the number of DIMMs */ dmi_walk(ghes_edac_count_dimms, &num_dimm); @@ -456,7 +474,11 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) mci->dev_name = "ghes"; if (!ghes_edac_mc_num) { - if (!fake) { + if (fake) { + pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n"); + pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n"); + pr_info("work on such system. Use this driver with caution\n"); + } else if (idx < 0) { pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n"); pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n"); pr_info("So, the end result of using this driver varies from vendor to vendor.\n"); @@ -464,10 +486,6 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) pr_info("to correct its BIOS.\n"); pr_info("This system has %d DIMM sockets.\n", num_dimm); - } else { - pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n"); - pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n"); - pr_info("work on such system. Use this driver with caution\n"); } } ^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac 2017-08-18 19:46 ` [v3,3/5] " Toshi Kani (?) @ 2017-08-23 16:20 ` Borislav Petkov -1 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-23 16:20 UTC (permalink / raw) To: Toshi Kani Cc: rjw, mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel On Fri, Aug 18, 2017 at 01:46:42PM -0600, Toshi Kani wrote: > The ghes_edac driver was introduced in 2013 [1], but it has not > been enabled by any distro yet. This driver obtains error info > from firmware interfaces, which are not properly implemented on > many platforms, as the driver always emits the messages below: > > This EDAC driver relies on BIOS to enumerate memory and get error reports. > Unfortunately, not all BIOSes reflect the memory layout correctly > So, the end result of using this driver varies from vendor to vendor > If you find incorrect reports, please contact your hardware vendor > to correct its BIOS. > > To get out from this situation, add a platform check to selectively > enable the driver on the platforms that are known to have proper > firmware implementation. Platform vendors can add their platforms > to the list when they support ghes_edac. > > "ghes_edac.force_load=1" skips this platform check. > > [1]: https://lwn.net/Articles/538438/ > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Tony Luck <tony.luck@intel.com> > --- > drivers/edac/ghes_edac.c | 28 +++++++++++++++++++++++----- > 1 file changed, 23 insertions(+), 5 deletions(-) Ok, for the remaining three, I've updated my "ghes" branch here: https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=ghes Please, redo them ontop. @Rafael: how do you want to handle this? The first two are ACPI patches and the remaining three are EDAC. It would be probably easier if you acked the ACPI ones (but wait until Toshi's next version) and took them all through the EDAC tree as I have two more reworking that ghes_edac driver. Alternatively, they could all go through the ACPI tree but you'll have to pick them all up together. That shouldn't be a problem either as all changes are solely to drivers/edac/ghes_edac.c and there's one other patch in my EDAC pile which touches ghes_edac.c: https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/diff/drivers/edac/ghes_edac.c?h=for-next&id=c54182ec0e157988f0cafd1e8d37b68ab4210f87 That's why I say, it'll be easier if I carried them all. :) But I'm sure we can work something out. Thanks. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,3/5] ghes_edac: add platform check to enable ghes_edac @ 2017-08-23 16:20 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-23 16:20 UTC (permalink / raw) To: Toshi Kani, Rafael J. Wysocki Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel On Fri, Aug 18, 2017 at 01:46:42PM -0600, Toshi Kani wrote: > The ghes_edac driver was introduced in 2013 [1], but it has not > been enabled by any distro yet. This driver obtains error info > from firmware interfaces, which are not properly implemented on > many platforms, as the driver always emits the messages below: > > This EDAC driver relies on BIOS to enumerate memory and get error reports. > Unfortunately, not all BIOSes reflect the memory layout correctly > So, the end result of using this driver varies from vendor to vendor > If you find incorrect reports, please contact your hardware vendor > to correct its BIOS. > > To get out from this situation, add a platform check to selectively > enable the driver on the platforms that are known to have proper > firmware implementation. Platform vendors can add their platforms > to the list when they support ghes_edac. > > "ghes_edac.force_load=1" skips this platform check. > > [1]: https://lwn.net/Articles/538438/ > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Tony Luck <tony.luck@intel.com> > --- > drivers/edac/ghes_edac.c | 28 +++++++++++++++++++++++----- > 1 file changed, 23 insertions(+), 5 deletions(-) Ok, for the remaining three, I've updated my "ghes" branch here: https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=ghes Please, redo them ontop. @Rafael: how do you want to handle this? The first two are ACPI patches and the remaining three are EDAC. It would be probably easier if you acked the ACPI ones (but wait until Toshi's next version) and took them all through the EDAC tree as I have two more reworking that ghes_edac driver. Alternatively, they could all go through the ACPI tree but you'll have to pick them all up together. That shouldn't be a problem either as all changes are solely to drivers/edac/ghes_edac.c and there's one other patch in my EDAC pile which touches ghes_edac.c: https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/diff/drivers/edac/ghes_edac.c?h=for-next&id=c54182ec0e157988f0cafd1e8d37b68ab4210f87 That's why I say, it'll be easier if I carried them all. :) But I'm sure we can work something out. Thanks. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac @ 2017-08-23 16:20 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-23 16:20 UTC (permalink / raw) To: Toshi Kani, Rafael J. Wysocki Cc: rjw, mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel On Fri, Aug 18, 2017 at 01:46:42PM -0600, Toshi Kani wrote: > The ghes_edac driver was introduced in 2013 [1], but it has not > been enabled by any distro yet. This driver obtains error info > from firmware interfaces, which are not properly implemented on > many platforms, as the driver always emits the messages below: > > This EDAC driver relies on BIOS to enumerate memory and get error reports. > Unfortunately, not all BIOSes reflect the memory layout correctly > So, the end result of using this driver varies from vendor to vendor > If you find incorrect reports, please contact your hardware vendor > to correct its BIOS. > > To get out from this situation, add a platform check to selectively > enable the driver on the platforms that are known to have proper > firmware implementation. Platform vendors can add their platforms > to the list when they support ghes_edac. > > "ghes_edac.force_load=1" skips this platform check. > > [1]: https://lwn.net/Articles/538438/ > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Tony Luck <tony.luck@intel.com> > --- > drivers/edac/ghes_edac.c | 28 +++++++++++++++++++++++----- > 1 file changed, 23 insertions(+), 5 deletions(-) Ok, for the remaining three, I've updated my "ghes" branch here: https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=ghes Please, redo them ontop. @Rafael: how do you want to handle this? The first two are ACPI patches and the remaining three are EDAC. It would be probably easier if you acked the ACPI ones (but wait until Toshi's next version) and took them all through the EDAC tree as I have two more reworking that ghes_edac driver. Alternatively, they could all go through the ACPI tree but you'll have to pick them all up together. That shouldn't be a problem either as all changes are solely to drivers/edac/ghes_edac.c and there's one other patch in my EDAC pile which touches ghes_edac.c: https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/diff/drivers/edac/ghes_edac.c?h=for-next&id=c54182ec0e157988f0cafd1e8d37b68ab4210f87 That's why I say, it'll be easier if I carried them all. :) But I'm sure we can work something out. Thanks. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac @ 2017-08-23 20:46 ` Rafael J. Wysocki 0 siblings, 0 replies; 55+ messages in thread From: Rafael J. Wysocki @ 2017-08-23 20:46 UTC (permalink / raw) To: Borislav Petkov Cc: Toshi Kani, mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel On Wednesday, August 23, 2017 6:20:53 PM CEST Borislav Petkov wrote: > On Fri, Aug 18, 2017 at 01:46:42PM -0600, Toshi Kani wrote: > > The ghes_edac driver was introduced in 2013 [1], but it has not > > been enabled by any distro yet. This driver obtains error info > > from firmware interfaces, which are not properly implemented on > > many platforms, as the driver always emits the messages below: > > > > This EDAC driver relies on BIOS to enumerate memory and get error reports. > > Unfortunately, not all BIOSes reflect the memory layout correctly > > So, the end result of using this driver varies from vendor to vendor > > If you find incorrect reports, please contact your hardware vendor > > to correct its BIOS. > > > > To get out from this situation, add a platform check to selectively > > enable the driver on the platforms that are known to have proper > > firmware implementation. Platform vendors can add their platforms > > to the list when they support ghes_edac. > > > > "ghes_edac.force_load=1" skips this platform check. > > > > [1]: https://lwn.net/Articles/538438/ > > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > > Cc: Borislav Petkov <bp@alien8.de> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > > Cc: Tony Luck <tony.luck@intel.com> > > --- > > drivers/edac/ghes_edac.c | 28 +++++++++++++++++++++++----- > > 1 file changed, 23 insertions(+), 5 deletions(-) > > Ok, for the remaining three, I've updated my "ghes" branch here: > > https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=ghes > > Please, redo them ontop. > > @Rafael: how do you want to handle this? > > The first two are ACPI patches and the remaining three are EDAC. It > would be probably easier if you acked the ACPI ones (but wait until > Toshi's next version) and took them all through the EDAC tree as I have > two more reworking that ghes_edac driver. So I have some pending intel_pstate and the intel_pstate changes are likely to conflict with it. > Alternatively, they could all go through the ACPI tree but you'll have > to pick them all up together. That can be done. :-) > That shouldn't be a problem either as all > changes are solely to drivers/edac/ghes_edac.c and there's one other > patch in my EDAC pile which touches ghes_edac.c: > > https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/diff/drivers/edac/ghes_edac.c?h=for-next&id=c54182ec0e157988f0cafd1e8d37b68ab4210f87 > > That's why I say, it'll be easier if I carried them all. :) > > But I'm sure we can work something out. I can expose a branch with this series for you to merge if that helps. Thanks, Rafael ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,3/5] ghes_edac: add platform check to enable ghes_edac @ 2017-08-23 20:46 ` Rafael J. Wysocki 0 siblings, 0 replies; 55+ messages in thread From: Rafael J. Wysocki @ 2017-08-23 20:46 UTC (permalink / raw) To: Borislav Petkov Cc: Toshi Kani, mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel On Wednesday, August 23, 2017 6:20:53 PM CEST Borislav Petkov wrote: > On Fri, Aug 18, 2017 at 01:46:42PM -0600, Toshi Kani wrote: > > The ghes_edac driver was introduced in 2013 [1], but it has not > > been enabled by any distro yet. This driver obtains error info > > from firmware interfaces, which are not properly implemented on > > many platforms, as the driver always emits the messages below: > > > > This EDAC driver relies on BIOS to enumerate memory and get error reports. > > Unfortunately, not all BIOSes reflect the memory layout correctly > > So, the end result of using this driver varies from vendor to vendor > > If you find incorrect reports, please contact your hardware vendor > > to correct its BIOS. > > > > To get out from this situation, add a platform check to selectively > > enable the driver on the platforms that are known to have proper > > firmware implementation. Platform vendors can add their platforms > > to the list when they support ghes_edac. > > > > "ghes_edac.force_load=1" skips this platform check. > > > > [1]: https://lwn.net/Articles/538438/ > > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > > Cc: Borislav Petkov <bp@alien8.de> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > > Cc: Tony Luck <tony.luck@intel.com> > > --- > > drivers/edac/ghes_edac.c | 28 +++++++++++++++++++++++----- > > 1 file changed, 23 insertions(+), 5 deletions(-) > > Ok, for the remaining three, I've updated my "ghes" branch here: > > https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=ghes > > Please, redo them ontop. > > @Rafael: how do you want to handle this? > > The first two are ACPI patches and the remaining three are EDAC. It > would be probably easier if you acked the ACPI ones (but wait until > Toshi's next version) and took them all through the EDAC tree as I have > two more reworking that ghes_edac driver. So I have some pending intel_pstate and the intel_pstate changes are likely to conflict with it. > Alternatively, they could all go through the ACPI tree but you'll have > to pick them all up together. That can be done. :-) > That shouldn't be a problem either as all > changes are solely to drivers/edac/ghes_edac.c and there's one other > patch in my EDAC pile which touches ghes_edac.c: > > https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/diff/drivers/edac/ghes_edac.c?h=for-next&id=c54182ec0e157988f0cafd1e8d37b68ab4210f87 > > That's why I say, it'll be easier if I carried them all. :) > > But I'm sure we can work something out. I can expose a branch with this series for you to merge if that helps. Thanks, Rafael --- To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac @ 2017-08-24 7:54 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-24 7:54 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Toshi Kani, mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel On Wed, Aug 23, 2017 at 10:46:42PM +0200, Rafael J. Wysocki wrote: > I can expose a branch with this series for you to merge if that helps. I think that'll be the best solution. So yes, pls lemme know when you have it so that I can pick up the rest. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 55+ messages in thread
* [v3,3/5] ghes_edac: add platform check to enable ghes_edac @ 2017-08-24 7:54 ` Borislav Petkov 0 siblings, 0 replies; 55+ messages in thread From: Borislav Petkov @ 2017-08-24 7:54 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Toshi Kani, mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel On Wed, Aug 23, 2017 at 10:46:42PM +0200, Rafael J. Wysocki wrote: > I can expose a branch with this series for you to merge if that helps. I think that'll be the best solution. So yes, pls lemme know when you have it so that I can pick up the rest. Thx. ^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v3 4/5] EDAC: add edac_get_owner() to check MC owner @ 2017-08-18 19:46 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-18 19:46 UTC (permalink / raw) To: rjw, bp Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel, Toshi Kani Only a single edac driver can be enabled for EDAC MC. When ghes_edac is enabled, a regular edac driver for the CPU type / platform still attempts to register itself and fails in edac_mc_add_mc(). Add edac_get_owner() so that regular edac drivers can check the owner of EDAC MC without calling edac_mc_add_mc(). Signed-off-by: Toshi Kani <toshi.kani@hpe.com> Suggested-by: Borislav Petkov <bp@alien8.de> Cc: Borislav Petkov <bp@alien8.de> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Tony Luck <tony.luck@intel.com> --- drivers/edac/edac_mc.c | 7 ++++++- drivers/edac/edac_mc.h | 8 ++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c index 4800721..48193f5 100644 --- a/drivers/edac/edac_mc.c +++ b/drivers/edac/edac_mc.c @@ -53,7 +53,7 @@ static LIST_HEAD(mc_devices); * Used to lock EDAC MC to just one module, avoiding two drivers e. g. * apei/ghes and i7core_edac to be used at the same time. */ -static void const *edac_mc_owner; +static const char *edac_mc_owner; static struct bus_type mc_bus[EDAC_MAX_MCS]; @@ -701,6 +701,11 @@ struct mem_ctl_info *edac_mc_find(int idx) } EXPORT_SYMBOL(edac_mc_find); +const char *edac_get_owner(void) +{ + return edac_mc_owner; +} +EXPORT_SYMBOL_GPL(edac_get_owner); /* FIXME - should a warning be printed if no error detection? correction? */ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci, diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h index 5357800..4165e15 100644 --- a/drivers/edac/edac_mc.h +++ b/drivers/edac/edac_mc.h @@ -128,6 +128,14 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num, unsigned sz_pvt); /** + * edac_get_owner - Return the owner's mod_name of EDAC MC + * + * Returns: + * Pointer to mod_name string when EDAC MC is owned. NULL otherwise. + */ +extern const char *edac_get_owner(void); + +/* * edac_mc_add_mc_with_groups() - Insert the @mci structure into the mci * global list and create sysfs entries associated with @mci structure. * ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [v3,4/5] EDAC: add edac_get_owner() to check MC owner @ 2017-08-18 19:46 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-18 19:46 UTC (permalink / raw) To: rjw, bp Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel, Toshi Kani Only a single edac driver can be enabled for EDAC MC. When ghes_edac is enabled, a regular edac driver for the CPU type / platform still attempts to register itself and fails in edac_mc_add_mc(). Add edac_get_owner() so that regular edac drivers can check the owner of EDAC MC without calling edac_mc_add_mc(). Signed-off-by: Toshi Kani <toshi.kani@hpe.com> Suggested-by: Borislav Petkov <bp@alien8.de> Cc: Borislav Petkov <bp@alien8.de> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Tony Luck <tony.luck@intel.com> --- drivers/edac/edac_mc.c | 7 ++++++- drivers/edac/edac_mc.h | 8 ++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c index 4800721..48193f5 100644 --- a/drivers/edac/edac_mc.c +++ b/drivers/edac/edac_mc.c @@ -53,7 +53,7 @@ static LIST_HEAD(mc_devices); * Used to lock EDAC MC to just one module, avoiding two drivers e. g. * apei/ghes and i7core_edac to be used at the same time. */ -static void const *edac_mc_owner; +static const char *edac_mc_owner; static struct bus_type mc_bus[EDAC_MAX_MCS]; @@ -701,6 +701,11 @@ struct mem_ctl_info *edac_mc_find(int idx) } EXPORT_SYMBOL(edac_mc_find); +const char *edac_get_owner(void) +{ + return edac_mc_owner; +} +EXPORT_SYMBOL_GPL(edac_get_owner); /* FIXME - should a warning be printed if no error detection? correction? */ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci, diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h index 5357800..4165e15 100644 --- a/drivers/edac/edac_mc.h +++ b/drivers/edac/edac_mc.h @@ -128,6 +128,14 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num, unsigned sz_pvt); /** + * edac_get_owner - Return the owner's mod_name of EDAC MC + * + * Returns: + * Pointer to mod_name string when EDAC MC is owned. NULL otherwise. + */ +extern const char *edac_get_owner(void); + +/* * edac_mc_add_mc_with_groups() - Insert the @mci structure into the mci * global list and create sysfs entries associated with @mci structure. * ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v3 5/5] edac drivers: add MC owner check in init @ 2017-08-18 19:46 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-18 19:46 UTC (permalink / raw) To: rjw, bp Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel, Toshi Kani Change generic x86 edac drivers, which probe CPU type with x86_match_cpu(), to verify the module owner at the beginning of their module init functions. This allows them to fail their init immediately when ghes_edac is enabled. Similar change can be made to other edac drivers as necessary. Also, remove ".c" from module names of pnp2_edac, sb_edac, and skx_edac. Signed-off-by: Toshi Kani <toshi.kani@hpe.com> Suggested-by: Borislav Petkov <bp@alien8.de> Cc: Borislav Petkov <bp@alien8.de> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Tony Luck <tony.luck@intel.com> --- drivers/edac/amd64_edac.c | 5 +++++ drivers/edac/pnd2_edac.c | 9 ++++++++- drivers/edac/sb_edac.c | 9 +++++++-- drivers/edac/skx_edac.c | 8 +++++++- 4 files changed, 27 insertions(+), 4 deletions(-) diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index 3aea556..529bcbf 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -3435,9 +3435,14 @@ MODULE_DEVICE_TABLE(x86cpu, amd64_cpuids); static int __init amd64_edac_init(void) { + const char *owner; int err = -ENODEV; int i; + owner = edac_get_owner(); + if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) + return -EBUSY; + if (!x86_match_cpu(amd64_cpuids)) return -ENODEV; diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c index 8e59949..6609041 100644 --- a/drivers/edac/pnd2_edac.c +++ b/drivers/edac/pnd2_edac.c @@ -45,6 +45,8 @@ #include "edac_module.h" #include "pnd2_edac.h" +#define EDAC_MOD_STR "pnd2_edac" + #define APL_NUM_CHANNELS 4 #define DNV_NUM_CHANNELS 2 #define DNV_MAX_DIMMS 2 /* Max DIMMs per channel */ @@ -1313,7 +1315,7 @@ static int pnd2_register_mci(struct mem_ctl_info **ppmci) pvt = mci->pvt_info; memset(pvt, 0, sizeof(*pvt)); - mci->mod_name = "pnd2_edac.c"; + mci->mod_name = EDAC_MOD_STR; mci->dev_name = ops->name; mci->ctl_name = "Pondicherry2"; @@ -1505,10 +1507,15 @@ MODULE_DEVICE_TABLE(x86cpu, pnd2_cpuids); static int __init pnd2_init(void) { const struct x86_cpu_id *id; + const char *owner; int rc; edac_dbg(2, "\n"); + owner = edac_get_owner(); + if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) + return -EBUSY; + id = x86_match_cpu(pnd2_cpuids); if (!id) return -ENODEV; diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c index 80d860c..d411aa5c 100644 --- a/drivers/edac/sb_edac.c +++ b/drivers/edac/sb_edac.c @@ -36,7 +36,7 @@ static LIST_HEAD(sbridge_edac_list); * Alter this version for the module when modifications are made */ #define SBRIDGE_REVISION " Ver: 1.1.2 " -#define EDAC_MOD_STR "sbridge_edac" +#define EDAC_MOD_STR "sb_edac" /* * Debug macros @@ -3124,7 +3124,7 @@ static int sbridge_register_mci(struct sbridge_dev *sbridge_dev, enum type type) MEM_FLAG_DDR4 : MEM_FLAG_DDR3; mci->edac_ctl_cap = EDAC_FLAG_NONE; mci->edac_cap = EDAC_FLAG_NONE; - mci->mod_name = "sb_edac.c"; + mci->mod_name = EDAC_MOD_STR; mci->mod_ver = SBRIDGE_REVISION; mci->dev_name = pci_name(pdev); mci->ctl_page_to_phys = NULL; @@ -3372,10 +3372,15 @@ static void sbridge_remove(void) static int __init sbridge_init(void) { const struct x86_cpu_id *id; + const char *owner; int rc; edac_dbg(2, "\n"); + owner = edac_get_owner(); + if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) + return -EBUSY; + id = x86_match_cpu(sbridge_cpuids); if (!id) return -ENODEV; diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c index 64bef6c9..17a2fbd 100644 --- a/drivers/edac/skx_edac.c +++ b/drivers/edac/skx_edac.c @@ -31,6 +31,7 @@ #include "edac_module.h" +#define EDAC_MOD_STR "skx_edac" #define SKX_REVISION " Ver: 1.0 " /* @@ -471,7 +472,7 @@ static int skx_register_mci(struct skx_imc *imc) mci->mtype_cap = MEM_FLAG_DDR4; mci->edac_ctl_cap = EDAC_FLAG_NONE; mci->edac_cap = EDAC_FLAG_NONE; - mci->mod_name = "skx_edac.c"; + mci->mod_name = EDAC_MOD_STR; mci->dev_name = pci_name(imc->chan[0].cdev); mci->mod_ver = SKX_REVISION; mci->ctl_page_to_phys = NULL; @@ -1042,12 +1043,17 @@ static int __init skx_init(void) { const struct x86_cpu_id *id; const struct munit *m; + const char *owner; int rc = 0, i; u8 mc = 0, src_id, node_id; struct skx_dev *d; edac_dbg(2, "\n"); + owner = edac_get_owner(); + if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) + return -EBUSY; + id = x86_match_cpu(skx_cpuids); if (!id) return -ENODEV; ^ permalink raw reply related [flat|nested] 55+ messages in thread
* [v3,5/5] edac drivers: add MC owner check in init @ 2017-08-18 19:46 ` Toshi Kani 0 siblings, 0 replies; 55+ messages in thread From: Toshi Kani @ 2017-08-18 19:46 UTC (permalink / raw) To: rjw, bp Cc: mchehab, tony.luck, lenb, linux-acpi, linux-edac, linux-kernel, Toshi Kani Change generic x86 edac drivers, which probe CPU type with x86_match_cpu(), to verify the module owner at the beginning of their module init functions. This allows them to fail their init immediately when ghes_edac is enabled. Similar change can be made to other edac drivers as necessary. Also, remove ".c" from module names of pnp2_edac, sb_edac, and skx_edac. Signed-off-by: Toshi Kani <toshi.kani@hpe.com> Suggested-by: Borislav Petkov <bp@alien8.de> Cc: Borislav Petkov <bp@alien8.de> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Tony Luck <tony.luck@intel.com> --- drivers/edac/amd64_edac.c | 5 +++++ drivers/edac/pnd2_edac.c | 9 ++++++++- drivers/edac/sb_edac.c | 9 +++++++-- drivers/edac/skx_edac.c | 8 +++++++- 4 files changed, 27 insertions(+), 4 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index 3aea556..529bcbf 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -3435,9 +3435,14 @@ MODULE_DEVICE_TABLE(x86cpu, amd64_cpuids); static int __init amd64_edac_init(void) { + const char *owner; int err = -ENODEV; int i; + owner = edac_get_owner(); + if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) + return -EBUSY; + if (!x86_match_cpu(amd64_cpuids)) return -ENODEV; diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c index 8e59949..6609041 100644 --- a/drivers/edac/pnd2_edac.c +++ b/drivers/edac/pnd2_edac.c @@ -45,6 +45,8 @@ #include "edac_module.h" #include "pnd2_edac.h" +#define EDAC_MOD_STR "pnd2_edac" + #define APL_NUM_CHANNELS 4 #define DNV_NUM_CHANNELS 2 #define DNV_MAX_DIMMS 2 /* Max DIMMs per channel */ @@ -1313,7 +1315,7 @@ static int pnd2_register_mci(struct mem_ctl_info **ppmci) pvt = mci->pvt_info; memset(pvt, 0, sizeof(*pvt)); - mci->mod_name = "pnd2_edac.c"; + mci->mod_name = EDAC_MOD_STR; mci->dev_name = ops->name; mci->ctl_name = "Pondicherry2"; @@ -1505,10 +1507,15 @@ MODULE_DEVICE_TABLE(x86cpu, pnd2_cpuids); static int __init pnd2_init(void) { const struct x86_cpu_id *id; + const char *owner; int rc; edac_dbg(2, "\n"); + owner = edac_get_owner(); + if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) + return -EBUSY; + id = x86_match_cpu(pnd2_cpuids); if (!id) return -ENODEV; diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c index 80d860c..d411aa5c 100644 --- a/drivers/edac/sb_edac.c +++ b/drivers/edac/sb_edac.c @@ -36,7 +36,7 @@ static LIST_HEAD(sbridge_edac_list); * Alter this version for the module when modifications are made */ #define SBRIDGE_REVISION " Ver: 1.1.2 " -#define EDAC_MOD_STR "sbridge_edac" +#define EDAC_MOD_STR "sb_edac" /* * Debug macros @@ -3124,7 +3124,7 @@ static int sbridge_register_mci(struct sbridge_dev *sbridge_dev, enum type type) MEM_FLAG_DDR4 : MEM_FLAG_DDR3; mci->edac_ctl_cap = EDAC_FLAG_NONE; mci->edac_cap = EDAC_FLAG_NONE; - mci->mod_name = "sb_edac.c"; + mci->mod_name = EDAC_MOD_STR; mci->mod_ver = SBRIDGE_REVISION; mci->dev_name = pci_name(pdev); mci->ctl_page_to_phys = NULL; @@ -3372,10 +3372,15 @@ static void sbridge_remove(void) static int __init sbridge_init(void) { const struct x86_cpu_id *id; + const char *owner; int rc; edac_dbg(2, "\n"); + owner = edac_get_owner(); + if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) + return -EBUSY; + id = x86_match_cpu(sbridge_cpuids); if (!id) return -ENODEV; diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c index 64bef6c9..17a2fbd 100644 --- a/drivers/edac/skx_edac.c +++ b/drivers/edac/skx_edac.c @@ -31,6 +31,7 @@ #include "edac_module.h" +#define EDAC_MOD_STR "skx_edac" #define SKX_REVISION " Ver: 1.0 " /* @@ -471,7 +472,7 @@ static int skx_register_mci(struct skx_imc *imc) mci->mtype_cap = MEM_FLAG_DDR4; mci->edac_ctl_cap = EDAC_FLAG_NONE; mci->edac_cap = EDAC_FLAG_NONE; - mci->mod_name = "skx_edac.c"; + mci->mod_name = EDAC_MOD_STR; mci->dev_name = pci_name(imc->chan[0].cdev); mci->mod_ver = SKX_REVISION; mci->ctl_page_to_phys = NULL; @@ -1042,12 +1043,17 @@ static int __init skx_init(void) { const struct x86_cpu_id *id; const struct munit *m; + const char *owner; int rc = 0, i; u8 mc = 0, src_id, node_id; struct skx_dev *d; edac_dbg(2, "\n"); + owner = edac_get_owner(); + if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) + return -EBUSY; + id = x86_match_cpu(skx_cpuids); if (!id) return -ENODEV; ^ permalink raw reply related [flat|nested] 55+ messages in thread
end of thread, other threads:[~2017-09-03 0:44 UTC | newest] Thread overview: 55+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-18 19:46 [PATCH v3 0/5] enable ghes_edac on selected platforms Toshi Kani 2017-08-18 19:46 ` [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() Toshi Kani 2017-08-18 19:46 ` [v3,1/5] " Toshi Kani 2017-08-21 11:27 ` [PATCH v3 1/5] " Borislav Petkov 2017-08-21 11:27 ` [v3,1/5] " Borislav Petkov 2017-08-21 12:25 ` [PATCH v3 1/5] " Rafael J. Wysocki 2017-08-21 12:25 ` [v3,1/5] " Rafael J. Wysocki 2017-08-21 13:20 ` [PATCH] ACPICA: Check whether ACPI is disabled before getting a table Borislav Petkov 2017-08-21 13:20 ` Borislav Petkov 2017-08-21 13:30 ` [PATCH] " Rafael J. Wysocki 2017-08-21 13:30 ` Rafael J. Wysocki 2017-08-21 15:34 ` [PATCH] " Borislav Petkov 2017-08-21 15:34 ` Borislav Petkov 2017-09-03 0:43 ` [PATCH] " kbuild test robot 2017-09-03 0:43 ` kbuild test robot 2017-08-21 16:41 ` [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list() Kani, Toshimitsu 2017-08-21 16:41 ` [v3,1/5] " Toshi Kani 2017-08-21 17:04 ` [PATCH v3 1/5] " Borislav Petkov 2017-08-21 17:04 ` [v3,1/5] " Borislav Petkov 2017-08-21 17:23 ` [PATCH v3 1/5] " Kani, Toshimitsu 2017-08-21 17:23 ` [v3,1/5] " Toshi Kani 2017-08-21 17:36 ` [PATCH v3 1/5] " Borislav Petkov 2017-08-21 17:36 ` [v3,1/5] " Borislav Petkov 2017-08-21 20:31 ` [PATCH v3 1/5] " Rafael J. Wysocki 2017-08-21 20:31 ` [v3,1/5] " Rafael J. Wysocki 2017-08-21 21:06 ` [PATCH v3 1/5] " Kani, Toshimitsu 2017-08-21 21:06 ` [v3,1/5] " Toshi Kani 2017-08-21 21:49 ` [PATCH v3 1/5] " Rafael J. Wysocki 2017-08-21 21:49 ` [v3,1/5] " Rafael J. Wysocki 2017-08-21 22:21 ` [PATCH v3 1/5] " Kani, Toshimitsu 2017-08-21 22:21 ` [v3,1/5] " Toshi Kani 2017-08-21 22:26 ` [PATCH v3 1/5] " Rafael J. Wysocki 2017-08-21 22:26 ` [v3,1/5] " Rafael J. Wysocki 2017-08-18 19:46 ` [PATCH v3 2/5] intel_pstate: convert to use acpi_match_platform_list() Toshi Kani 2017-08-18 19:46 ` [v3,2/5] " Toshi Kani 2017-08-21 17:53 ` [PATCH v3 2/5] " Srinivas Pandruvada 2017-08-21 17:53 ` [v3,2/5] " Srinivas Pandruvada 2017-08-23 15:46 ` [PATCH v3 2/5] " Borislav Petkov 2017-08-23 15:46 ` [v3,2/5] " Borislav Petkov 2017-08-23 15:56 ` [PATCH v3 2/5] " Kani, Toshimitsu 2017-08-23 15:56 ` [v3,2/5] " Toshi Kani 2017-08-23 15:56 ` [PATCH v3 2/5] " Kani, Toshimitsu 2017-08-18 19:46 ` [PATCH v3 3/5] ghes_edac: add platform check to enable ghes_edac Toshi Kani 2017-08-18 19:46 ` [v3,3/5] " Toshi Kani 2017-08-23 16:20 ` [PATCH v3 3/5] " Borislav Petkov 2017-08-23 16:20 ` [v3,3/5] " Borislav Petkov 2017-08-23 16:20 ` [PATCH v3 3/5] " Borislav Petkov 2017-08-23 20:46 ` Rafael J. Wysocki 2017-08-23 20:46 ` [v3,3/5] " Rafael J. Wysocki 2017-08-24 7:54 ` [PATCH v3 3/5] " Borislav Petkov 2017-08-24 7:54 ` [v3,3/5] " Borislav Petkov 2017-08-18 19:46 ` [PATCH v3 4/5] EDAC: add edac_get_owner() to check MC owner Toshi Kani 2017-08-18 19:46 ` [v3,4/5] " Toshi Kani 2017-08-18 19:46 ` [PATCH v3 5/5] edac drivers: add MC owner check in init Toshi Kani 2017-08-18 19:46 ` [v3,5/5] " Toshi Kani
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.