From 8cf4e6a04f734e831c2ac7f405071d1cde690ba8 Mon Sep 17 00:00:00 2001 From: Jean Delvare Date: Sat, 3 Feb 2018 11:25:20 +0100 Subject: [PATCH 1/4] firmware: dmi: Optimize dmi_matches Function dmi_matches can me made a bit faster: * The documented purpose of dmi_initialized is to catch too early calls to dmi_check_system(). I'm not fully convinced it justifies slowing down the initialization of all systems out there, but at least the check should not have been moved from dmi_check_system() to dmi_matches(). dmi_matches() is being called for every entry of the table passed to dmi_check_system(), causing the same redundant check to be performed again and again. So move it back to dmi_check_system(), reverting this specific portion of commit d7b1956fed33 ("DMI: Introduce dmi_first_match to make the interface more flexible"). * Don't check for the exact_match flag again when we already know its value. Signed-off-by: Jean Delvare Fixes: d7b1956fed33 ("DMI: Introduce dmi_first_match to make the interface more flexible") Cc: Jani Nikula Cc: Daniel Vetter Cc: Rafael J. Wysocki Cc: Jeff Garzik --- drivers/firmware/dmi_scan.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c index 783041964439..84356d86f359 100644 --- a/drivers/firmware/dmi_scan.c +++ b/drivers/firmware/dmi_scan.c @@ -784,19 +784,20 @@ static bool dmi_matches(const struct dmi_system_id *dmi) { int i; - WARN(!dmi_initialized, KERN_ERR "dmi check: not initialized yet.\n"); - for (i = 0; i < ARRAY_SIZE(dmi->matches); i++) { int s = dmi->matches[i].slot; if (s == DMI_NONE) break; if (dmi_ident[s]) { - if (!dmi->matches[i].exact_match && - strstr(dmi_ident[s], dmi->matches[i].substr)) - continue; - else if (dmi->matches[i].exact_match && - !strcmp(dmi_ident[s], dmi->matches[i].substr)) - continue; + if (dmi->matches[i].exact_match) { + if (!strcmp(dmi_ident[s], + dmi->matches[i].substr)) + continue; + } else { + if (strstr(dmi_ident[s], + dmi->matches[i].substr)) + continue; + } } /* No match */ @@ -832,6 +833,8 @@ int dmi_check_system(const struct dmi_system_id *list) int count = 0; const struct dmi_system_id *d; + WARN(!dmi_initialized, KERN_ERR "dmi check: not initialized yet.\n"); + for (d = list; !dmi_is_end_of_table(d); d++) if (dmi_matches(d)) { count++; From 7117794feb1602ea5efca1c7bfd5b78c3278d29d Mon Sep 17 00:00:00 2001 From: Jean Delvare Date: Sat, 3 Feb 2018 11:25:20 +0100 Subject: [PATCH 2/4] firmware: dmi_scan: Drop dmi_initialized I don't think it makes sense to check for a possible bad initialization order at run time on every system when it is all decided at build time. A more efficient way to make sure developers do not introduce new calls to dmi_check_system() too early in the initialization sequence is to simply document the expected call order. That way, developers have a chance to get it right immediately, without having to test-boot their kernel, wonder why it does not work, and parse the kernel logs for a warning message. And we get rid of the run-time performance penalty as a nice side effect. Signed-off-by: Jean Delvare Cc: Ingo Molnar --- drivers/firmware/dmi_scan.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c index 84356d86f359..8cd5db6691b2 100644 --- a/drivers/firmware/dmi_scan.c +++ b/drivers/firmware/dmi_scan.c @@ -26,11 +26,6 @@ static u16 dmi_num; static u8 smbios_entry_point[32]; static int smbios_entry_point_size; -/* - * Catch too early calls to dmi_check_system(): - */ -static int dmi_initialized; - /* DMI system identification string used during boot */ static char dmi_ids_string[128] __initdata; @@ -633,7 +628,7 @@ void __init dmi_scan_machine(void) if (!dmi_smbios3_present(buf)) { dmi_available = 1; - goto out; + return; } } if (efi.smbios == EFI_INVALID_TABLE_ADDR) @@ -651,7 +646,7 @@ void __init dmi_scan_machine(void) if (!dmi_present(buf)) { dmi_available = 1; - goto out; + return; } } else if (IS_ENABLED(CONFIG_DMI_SCAN_MACHINE_NON_EFI_FALLBACK)) { p = dmi_early_remap(0xF0000, 0x10000); @@ -668,7 +663,7 @@ void __init dmi_scan_machine(void) if (!dmi_smbios3_present(buf)) { dmi_available = 1; dmi_early_unmap(p, 0x10000); - goto out; + return; } memcpy(buf, buf + 16, 16); } @@ -686,7 +681,7 @@ void __init dmi_scan_machine(void) if (!dmi_present(buf)) { dmi_available = 1; dmi_early_unmap(p, 0x10000); - goto out; + return; } memcpy(buf, buf + 16, 16); } @@ -694,8 +689,6 @@ void __init dmi_scan_machine(void) } error: pr_info("DMI not present or invalid.\n"); - out: - dmi_initialized = 1; } static ssize_t raw_table_read(struct file *file, struct kobject *kobj, @@ -827,14 +820,14 @@ static bool dmi_is_end_of_table(const struct dmi_system_id *dmi) * Walk the blacklist table running matching functions until someone * returns non zero or we hit the end. Callback function is called for * each successful match. Returns the number of matches. + * + * dmi_scan_machine must be called before this function is called. */ int dmi_check_system(const struct dmi_system_id *list) { int count = 0; const struct dmi_system_id *d; - WARN(!dmi_initialized, KERN_ERR "dmi check: not initialized yet.\n"); - for (d = list; !dmi_is_end_of_table(d); d++) if (dmi_matches(d)) { count++; @@ -857,6 +850,8 @@ EXPORT_SYMBOL(dmi_check_system); * * Walk the blacklist table until the first match is found. Return the * pointer to the matching entry or NULL if there's no match. + * + * dmi_scan_machine must be called before this function is called. */ const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list) { From a7770ae194569e96a93c48aceb304edded9cc648 Mon Sep 17 00:00:00 2001 From: Jean Delvare Date: Sat, 3 Feb 2018 11:25:20 +0100 Subject: [PATCH 3/4] firmware: dmi_scan: Fix handling of empty DMI strings The handling of empty DMI strings looks quite broken to me: * Strings from 1 to 7 spaces are not considered empty. * True empty DMI strings (string index set to 0) are not considered empty, and result in allocating a 0-char string. * Strings with invalid index also result in allocating a 0-char string. * Strings starting with 8 spaces are all considered empty, even if non-space characters follow (sounds like a weird thing to do, but I have actually seen occurrences of this in DMI tables before.) * Strings which are considered empty are reported as 8 spaces, instead of being actually empty. Some of these issues are the result of an off-by-one error in memcmp, the rest is incorrect by design. So let's get it square: missing strings and strings made of only spaces, regardless of their length, should be treated as empty and no memory should be allocated for them. All other strings are non-empty and should be allocated. Signed-off-by: Jean Delvare Fixes: 79da4721117f ("x86: fix DMI out of memory problems") Cc: Parag Warudkar Cc: Ingo Molnar Cc: Thomas Gleixner --- drivers/firmware/dmi_scan.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c index 8cd5db6691b2..a7072e7880ee 100644 --- a/drivers/firmware/dmi_scan.c +++ b/drivers/firmware/dmi_scan.c @@ -18,7 +18,7 @@ EXPORT_SYMBOL_GPL(dmi_kobj); * of and an antecedent to, SMBIOS, which stands for System * Management BIOS. See further: http://www.dmtf.org/standards */ -static const char dmi_empty_string[] = " "; +static const char dmi_empty_string[] = ""; static u32 dmi_ver __initdata; static u32 dmi_len; @@ -39,25 +39,21 @@ static int dmi_memdev_nr; static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s) { const u8 *bp = ((u8 *) dm) + dm->length; + const u8 *nsp; if (s) { - s--; - while (s > 0 && *bp) { + while (--s > 0 && *bp) bp += strlen(bp) + 1; - s--; - } - if (*bp != 0) { - size_t len = strlen(bp)+1; - size_t cmp_len = len > 8 ? 8 : len; - - if (!memcmp(bp, dmi_empty_string, cmp_len)) - return dmi_empty_string; + /* Strings containing only spaces are considered empty */ + nsp = bp; + while (*nsp == ' ') + nsp++; + if (*nsp != '\0') return bp; - } } - return ""; + return dmi_empty_string; } static const char * __init dmi_string(const struct dmi_header *dm, u8 s) From a81114d03e4a529c4b68293249f75438b3c1783f Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Sat, 3 Feb 2018 11:25:20 +0100 Subject: [PATCH 4/4] firmware: dmi: handle missing DMI data gracefully MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, when booting a kernel with DMI support on a platform that has no DMI tables, the following output is emitted into the kernel log: [ 0.128818] DMI not present or invalid. ... [ 1.306659] dmi: Firmware registration failed. ... [ 2.908681] dmi-sysfs: dmi entry is absent. The first one is a pr_info(), but the subsequent ones are pr_err()s that complain about a condition that is not really an error to begin with. So let's clean this up, and give up silently if dma_available is not set. Signed-off-by: Ard Biesheuvel Acked-by: Martin Hundebøll Signed-off-by: Jean Delvare --- drivers/firmware/dmi-sysfs.c | 2 +- drivers/firmware/dmi_scan.c | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c index d5de6ee8466d..ecf2eeb5f6f9 100644 --- a/drivers/firmware/dmi-sysfs.c +++ b/drivers/firmware/dmi-sysfs.c @@ -652,7 +652,7 @@ static int __init dmi_sysfs_init(void) int val; if (!dmi_kobj) { - pr_err("dmi-sysfs: dmi entry is absent.\n"); + pr_debug("dmi-sysfs: dmi entry is absent.\n"); error = -ENODATA; goto err; } diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c index a7072e7880ee..e763e1484331 100644 --- a/drivers/firmware/dmi_scan.c +++ b/drivers/firmware/dmi_scan.c @@ -704,10 +704,8 @@ static int __init dmi_init(void) u8 *dmi_table; int ret = -ENOMEM; - if (!dmi_available) { - ret = -ENODATA; - goto err; - } + if (!dmi_available) + return 0; /* * Set up dmi directory at /sys/firmware/dmi. This entry should stay