| Message ID | 20170717215912.26070-2-toshi.kani@hpe.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Jul 17, 2017 at 03:59:10PM -0600, Toshi Kani wrote: > ACPI OEM ID / OEM Table ID / Revision can be used to identify > platform type based on ACPI firmware. acpi_blacklisted(), > intel_pstate_platform_pwr_mgmt_exists() and some other funcs > have been using this type of check to detect a list of platforms > that require special handlings. > > Move the platform type check in acpi_blacklisted() to a common > utility function, acpi_match_oemlist(), so that other drivers > do not have to implement their own. > > 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> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > drivers/acpi/blacklist.c | 84 ++++++++-------------------------------------- > drivers/acpi/utils.c | 40 ++++++++++++++++++++++ > include/linux/acpi.h | 19 ++++++++++ > 3 files changed, 74 insertions(+), 69 deletions(-) > > diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c > index bb542ac..288fe4d 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_oemlist acpi_blacklist[] __initdata = { Why the arbitrary rename? If anything, you should shorten that enum acpi_blacklist_predicates oem_revision_predicate; unreadable insanity. > /* Compaq Presario 1700 */ > {"PTLTD ", " DSDT ", 0x06040000, ACPI_SIG_DSDT, less_than_or_equal, > "Multiple problems", 1}, > @@ -67,65 +50,28 @@ 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_oemlist(acpi_blacklist); > + if (i >= 0) { > + pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\" " > + "Revision 0x%x has a known ACPI BIOS problem.\n", Put that string on a single line for grepping. checkpatch catches that error, didn't you see it? > + 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();
On Tue, 2017-07-18 at 07:34 +0200, Borislav Petkov wrote: > On Mon, Jul 17, 2017 at 03:59:10PM -0600, Toshi Kani wrote: > > ACPI OEM ID / OEM Table ID / Revision can be used to identify > > platform type based on ACPI firmware. acpi_blacklisted(), > > intel_pstate_platform_pwr_mgmt_exists() and some other funcs > > have been using this type of check to detect a list of platforms > > that require special handlings. > > > > Move the platform type check in acpi_blacklisted() to a common > > utility function, acpi_match_oemlist(), so that other drivers > > do not have to implement their own. > > > > There is no change in functionality. : > > /* > > * 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_oemlist acpi_blacklist[] __initdata = { > > Why the arbitrary rename? This patch defines 'struct acpi_oemlist' in "include/linux/acpi.h" as a common structure, and replaces this specific 'struct acpi_blacklist'. > If anything, you should shorten that > > enum acpi_blacklist_predicates oem_revision_predicate; > > unreadable insanity. Agreed. Will change to a shorter name like below. enum acpi_oemlist_pred predicate; + i = acpi_match_oemlist(acpi_blacklist); > > + if (i >= 0) { > > + pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\" " > > + "Revision 0x%x has a known ACPI BIOS > > problem.\n", > > Put that string on a single line for grepping. checkpatch catches > that error, didn't you see it? Will do. Thanks! -Toshi
On Tue, Jul 18, 2017 at 03:48:54PM +0000, Kani, Toshimitsu wrote: > This patch defines 'struct acpi_oemlist' in "include/linux/acpi.h" as a I see that. > common structure, and replaces this specific 'struct acpi_blacklist'. And what makes acpi_oemlist "common" and acpi_blacklist "specific"? So let me save you some time - "oemlist" is more specific than "blacklist" and I can imagine a blacklist item not always being oem-specific. What I'm hinting at is, don't change that name. acpi_blacklist is just fine. > Agreed. Will change to a shorter name like below. > > enum acpi_oemlist_pred predicate; enum acpi_predicate pred; is even better.
T24gVHVlLCAyMDE3LTA3LTE4IGF0IDE4OjQzICswMjAwLCBCb3Jpc2xhdiBQZXRrb3Ygd3JvdGU6 DQo+IE9uIFR1ZSwgSnVsIDE4LCAyMDE3IGF0IDAzOjQ4OjU0UE0gKzAwMDAsIEthbmksIFRvc2hp bWl0c3Ugd3JvdGU6DQo+ID4gVGhpcyBwYXRjaCBkZWZpbmVzICdzdHJ1Y3QgYWNwaV9vZW1saXN0 JyBpbiAiaW5jbHVkZS9saW51eC9hY3BpLmgiDQo+ID4gYXMgYQ0KPiANCj4gSSBzZWUgdGhhdC4N Cj4gDQo+ID4gY29tbW9uIHN0cnVjdHVyZSwgYW5kIHJlcGxhY2VzIHRoaXMgc3BlY2lmaWMgJ3N0 cnVjdA0KPiA+IGFjcGlfYmxhY2tsaXN0Jy4NCj4gDQo+IEFuZCB3aGF0IG1ha2VzIGFjcGlfb2Vt bGlzdCAiY29tbW9uIiBhbmQgYWNwaV9ibGFja2xpc3QgInNwZWNpZmljIj8NCj4gDQo+IFNvIGxl dCBtZSBzYXZlIHlvdSBzb21lIHRpbWUgLSAib2VtbGlzdCIgaXMgbW9yZSBzcGVjaWZpYyB0aGFu DQo+ICJibGFja2xpc3QiIGFuZCBJIGNhbiBpbWFnaW5lIGEgYmxhY2tsaXN0IGl0ZW0gbm90IGFs d2F5cyBiZWluZw0KPiBvZW0tc3BlY2lmaWMuDQo+IA0KPiBXaGF0IEknbSBoaW50aW5nIGF0IGlz LCBkb24ndCBjaGFuZ2UgdGhhdCBuYW1lLiBhY3BpX2JsYWNrbGlzdCBpcw0KPiBqdXN0IGZpbmUu DQoNCldlbGwsIGEgbGlzdCBkb2VzIG5vdCBuZWVkIHRvIGJlIGEgYmxhY2stbGlzdC4gIEl0IGNh biBiZSBhIHdoaXRlLWxpc3QNCm9yIGFueXRoaW5nIHRoYXQgbWF0dGVycy4gIFRoZSBjYWxsZXIg ZGVmaW5lcyB0aGUgdXNhZ2Ugb2YgYSBsaXN0LiAgU28sDQpJIHRyaWVkIHRvIGF2b2lkIHB1dHRp bmcgYW55IHVzYWdlIHRvIHRoZSBzdHJ1Y3R1cmUgbmFtZS4NCg0KPiA+IEFncmVlZC7CoMKgV2ls bCBjaGFuZ2UgdG8gYSBzaG9ydGVyIG5hbWUgbGlrZSBiZWxvdy7CoA0KPiA+IA0KPiA+IAllbnVt IGFjcGlfb2VtbGlzdF9wcmVkIHByZWRpY2F0ZTsNCj4gDQo+IAllbnVtIGFjcGlfcHJlZGljYXRl IHByZWQ7DQo+IA0KPiBpcyBldmVuIGJldHRlci4NCg0KU291bmRzIGdvb2QuDQoNClRoYW5rcywN Ci1Ub3NoaQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 18, 2017 at 05:24:50PM +0000, Kani, Toshimitsu wrote: > Well, a list does not need to be a black-list. But this one *is* a blacklist. > So, I tried to avoid putting any usage to the structure name. So OEM is a usage. The moment you need to use it for something else besides an OEM, it is not an OEM list anymore - it is a generic blacklist which blacklists OEMs too.
T24gVHVlLCAyMDE3LTA3LTE4IGF0IDE5OjQyICswMjAwLCBCb3Jpc2xhdiBQZXRrb3Ygd3JvdGU6 DQo+IE9uIFR1ZSwgSnVsIDE4LCAyMDE3IGF0IDA1OjI0OjUwUE0gKzAwMDAsIEthbmksIFRvc2hp bWl0c3Ugd3JvdGU6DQo+ID4gV2VsbCwgYSBsaXN0IGRvZXMgbm90IG5lZWQgdG8gYmUgYSBibGFj ay1saXN0Lg0KPiANCj4gQnV0IHRoaXMgb25lICppcyogYSBibGFja2xpc3QuDQoNClJpZ2h0LiAg SGVuY2UsIGFjcGlfYmFja2xpc3RlZCgpIHN0aWxsIGRlY2xhcmVzIHRoZSBsaXN0IGFzDQonYWNw aV9ibGFja2xpc3RbXScuDQoNCj4gPiBTbywgSSB0cmllZCB0byBhdm9pZCBwdXR0aW5nIGFueSB1 c2FnZSB0byB0aGUgc3RydWN0dXJlIG5hbWUuDQo+IA0KPiBTbyBPRU0gaXMgYSB1c2FnZS4gVGhl IG1vbWVudCB5b3UgbmVlZCB0byB1c2UgaXQgZm9yIHNvbWV0aGluZyBlbHNlDQo+IGJlc2lkZXMg YW4gT0VNLCBpdCBpcyBub3QgYW4gT0VNIGxpc3QgYW55bW9yZSAtIGl0IGlzIGEgZ2VuZXJpYw0K PiBibGFja2xpc3Qgd2hpY2ggYmxhY2tsaXN0cyBPRU1zIHRvby4NCg0KVGhlIHRlcm0gIm9lbSIg cmVwcmVzZW50cyBkYXRhIHR5cGVzIG9mIHRoZSBzdHJ1Y3R1cmUsIG9lbV9pZFtdLA0Kb2VtX3Rh YmxlX2lkW10sIGFuZCBvZW1fcmV2aXNpb24sIHdoaWNoIGFyZSBkZWZpbmVkIGJ5IHRoZSBBQ1BJ IHNwZWMuICANCg0KZ2hlc19lZGFjIHVzZXMgdGhpcyBzdHJ1Y3R1cmUgYXMgYSB3aGlsZS1saXN0 LCBzbyB0aGUgdGVybSBiYWNrbGlzdCBpcw0KbWlzbGVhZGluZy4gIGludGVsX3BzdGF0ZSBhbHNv IHVzZXMgaXQgdG8gbGlzdCB0aGUgcGxhdGZvcm1zIHRoYXQgZG8NCm5vdCBuZWVkIE9TIGNvbnRy b2wuDQoNClRoYW5rcywNCi1Ub3NoaQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 18, 2017 at 06:49:51PM +0000, Kani, Toshimitsu wrote: > ghes_edac uses this structure as a while-list, so the term backlist is > misleading. So this matching function gets both blacklists and whitelists. No wonder it is confusing. Now I finally understand what you wanna do: you want to call all those lists something agnostic as platform_list or so because they contain exactly that: platforms - not OEMs. And then you want to match *platforms*. *Not* OEMs. *Now* I understand what you're trying to tell me.
On Tue, 2017-07-18 at 21:32 +0200, Borislav Petkov wrote: > On Tue, Jul 18, 2017 at 06:49:51PM +0000, Kani, Toshimitsu wrote: > > ghes_edac uses this structure as a while-list, so the term backlist > > is misleading. > > So this matching function gets both blacklists and whitelists. No > wonder it is confusing. Now I finally understand what you wanna do: > you want to call all those lists something agnostic as platform_list > or so because they contain exactly that: platforms - not OEMs. Right. > And then you want to match *platforms*. *Not* OEMs. True, there is some stretch to use OEMIDs for detecting platforms. But we do not have other standard interfaces better than this one. > *Now* I understand what you're trying to tell me. :-) Thanks, -Toshi
diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c index bb542ac..288fe4d 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_oemlist acpi_blacklist[] __initdata = { /* Compaq Presario 1700 */ {"PTLTD ", " DSDT ", 0x06040000, ACPI_SIG_DSDT, less_than_or_equal, "Multiple problems", 1}, @@ -67,65 +50,28 @@ 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_oemlist(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..e5909d5 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_oemlist - Check if the system matches with an oem list + * @oem: pointer to acpi_oemlist table terminated by a NULL entry + * + * Return the matched index if the system is found in the oem list. + * Otherwise, return a negative error code. + */ +int acpi_match_oemlist(const struct acpi_oemlist *oem) +{ + struct acpi_table_header hdr; + int idx = 0; + + if (acpi_disabled) + return -ENODEV; + + for (; oem->oem_id[0]; oem++, idx++) { + if (ACPI_FAILURE(acpi_get_table_header(oem->table, 0, &hdr))) + continue; + + if (strncmp(oem->oem_id, hdr.oem_id, ACPI_OEM_ID_SIZE)) + continue; + + if (strncmp(oem->oem_table_id, hdr.oem_table_id, + ACPI_OEM_TABLE_ID_SIZE)) + continue; + + if ((oem->oem_revision_predicate == all_versions) || + (oem->oem_revision_predicate == less_than_or_equal + && hdr.oem_revision <= oem->oem_revision) || + (oem->oem_revision_predicate == greater_than_or_equal + && hdr.oem_revision >= oem->oem_revision) || + (oem->oem_revision_predicate == equal + && hdr.oem_revision == oem->oem_revision)) + return idx; + } + + return -ENODEV; +} +EXPORT_SYMBOL(acpi_match_oemlist); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index c749eef..86479b5 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_oemlist_predicates { + all_versions, + less_than_or_equal, + equal, + greater_than_or_equal, +}; + +/* Table must be terminted by a NULL entry */ +struct acpi_oemlist { + char oem_id[ACPI_OEM_ID_SIZE]; + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE]; + u32 oem_revision; + char *table; + enum acpi_oemlist_predicates oem_revision_predicate; + char *reason; + u32 data; +}; +int acpi_match_oemlist(const struct acpi_oemlist *oem); + extern void acpi_early_init(void); extern void acpi_subsystem_init(void);
ACPI OEM ID / OEM Table ID / Revision can be used to identify platform type based on ACPI firmware. acpi_blacklisted(), intel_pstate_platform_pwr_mgmt_exists() and some other funcs have been using this type of check to detect a list of platforms that require special handlings. Move the platform type check in acpi_blacklisted() to a common utility function, acpi_match_oemlist(), so that other drivers do not have to implement their own. 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> Cc: Thomas Gleixner <tglx@linutronix.de> --- drivers/acpi/blacklist.c | 84 ++++++++-------------------------------------- drivers/acpi/utils.c | 40 ++++++++++++++++++++++ include/linux/acpi.h | 19 ++++++++++ 3 files changed, 74 insertions(+), 69 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html