Thanks for updating the patch.
Just a few short comments, this is not a full review.
> diff --git a/dmi.c b/dmi.c
On 09.08.2010 01:05, Sean Nelson wrote:
> Here is a new version of the internal dmi decoding. To use the new
> decoder, the user needs to use "-p internal:dmi=new", to cross-check
> with the external dmidecode use "-p internal:dmi=check". Hopefully if
> this is comitted, it will make it easier for people to test the new
> internal decoder.
>
> Signed-off-by: Sean Nelson <audiohacked@gmail.com>
>
> index f18907f..b2a5daa 100644
> --- a/dmi.c
> +++ b/dmi.c
> @@ -1,76 +1,227 @@
> /*
> * This file is part of the flashrom project.
> *
> * Copyright (C) 2009,2010 Michael Karcher
> + * Copyright (C) 2010 Sean Nelson
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> * the Free Software Foundation; either version 2 of the License, or
> * (at your option) any later version.
> *
> * This program is distributed in the hope that it will be useful,
> * but WITHOUT ANY WARRANTY; without even the implied warranty of
> * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> * GNU General Public License for more details.
> *
> * You should have received a copy of the GNU General Public License
> * along with this program; if not, write to the Free Software
> * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> */
>
> #include <string.h>
> #include <stdio.h>
> #include <stdlib.h>
>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <sys/mman.h>
>
Will this work on DOS and Windows?
> +
> #include "flash.h"
> #include "programmer.h"
>
> int has_dmi_support = 0;
>
> #if STANDALONE
>
Hm. STANDALONE should activate the builtin decoder. We might elect to
have a separate CONFIG_DMI which would replace !STANDALONE in this file.
>
> /* Stub to indicate missing DMI functionality.
> * has_dmi_support is 0 by default, so nothing to do here.
> * Because dmidecode is not available on all systems, the goal is to implement
> * the DMI subset we need directly in this file.
> */
> void dmi_init(void)
> {
> }
>
> int dmi_match(const char *pattern)
> {
> return 0;
> }
>
> #else /* STANDALONE */
>
> -static const char *dmidecode_names[] = {
> - "system-manufacturer",
> - "system-product-name",
> - "system-version",
> - "baseboard-manufacturer",
> - "baseboard-product-name",
> - "baseboard-version",
> +/* the actual code, based on the dmidecode parsing logic */
> +static const struct string_keyword {
>
Kill string_keyword.
> + const char *keyword;
> + unsigned char type;
> + unsigned char offset;
> +} flashrom_dmi_strings[] = {
> + { "system-manufacturer", 1, 0x04 },
> + { "system-product-name", 1, 0x05 },
> + { "system-version", 1, 0x06 },
> + { "baseboard-manufacturer", 2, 0x04 },
> + { "baseboard-product-name", 2, 0x05 },
> + { "baseboard-version", 2, 0x06 },
> + { "chassis-type", 3, 0x05 },
> };
>
> #define DMI_COMMAND_LEN_MAX 260
> static const char *dmidecode_command = "dmidecode";
> +static char *external_dmi[ARRAY_SIZE(flashrom_dmi_strings)];
> +#define DMI_MAX_ANSWER_LEN 4096
>
> -static char *dmistrings[ARRAY_SIZE(dmidecode_names)];
> +static char *dmistrings[ARRAY_SIZE(flashrom_dmi_strings)];
>
> -/* Strings longer than 4096 in DMI are just insane. */
>
Why did the comment disappear?
> -#define DMI_MAX_ANSWER_LEN 4096
> +#define WORD(x) (unsigned short)(*(const unsigned short *)(x))
> +#define DWORD(x) (unsigned int)(*(const unsigned int *)(x))
>
Ugh. Those #defines are really ugly. Can't you use mmio_read[bwl] or
something similar?
> +
> +static int checksum(const unsigned char *buf, size_t len)
>
dmi_checksum?
> +{
> + unsigned char sum = 0;
> + size_t a;
> +
> + for (a = 0; a < len; a++)
> + sum += buf[a];
> + return (sum == 0);
> +}
> +
> +static char *dmi_string(char *bp, unsigned char length, unsigned char s)
>
Better names for bp and s would be appreciated unless this is a straight
copy from the original dmidecode code (in which case we'd have to add
the author to the copyright header).
> +{
> + size_t i, len;
> +
> + if (s == 0)
> + return "Not Specified";
> +
> + bp += length;
> + while (s > 1 && *bp) {
> + bp += strlen(bp);
> + bp++;
>
We happily walk off the cliff here (mapped area) if the DMI strings
contain garbage.
> + s--;
> + }
> +
> + if (!*bp)
> + return "<BAD INDEX>";
> +
> + len = strlen(bp);
> + for (i = 0; i < len; i++)
> + if (bp[i] < 32 || bp[i] == 127)
> + bp[i] = '.';
>
We write to bp?
> +
> + return bp;
> +}
> +
> +static int dmi_chassis_type(unsigned char code)
> +{
> + switch(code) {
> + case 0x08: /* Portable */
> + case 0x09: /* Laptop */
> + case 0x0A: /* Notebook */
> + case 0x0E: /* Sub Notebook */
> + return 1;
> + default:
> + return 0;
> + }
> +}
> +
> +static void dmi_table(unsigned int base, unsigned short len, unsigned short num)
> +{
> + unsigned char *data;
> + unsigned char *dmi_table_mem;
> + int key;
> + int i=0, j=0;
>
spaces
> +
> + dmi_table_mem = physmap_try_ro("DMI Tables", base, len);
>
If physmap_try_ro fails this will explode spectacularly.
> + data = dmi_table_mem;
> +
> + /* 4 is the length of an SMBIOS structure header */
> + while (i < num && data+4 <= dmi_table_mem + len) {
> + unsigned char *next;
> + /*
> + * If a short entry is found (less than 4 bytes), not only it
> + * is invalid, but we cannot reliably locate the next entry.
> + * Better stop at this point, and let the user know his/her
> + * table is broken.
> + */
> + if (data[1] < 4) {
> + msg_perr("Invalid entry length (%u). DMI table is "
> + "broken! Stop.\n\n", (unsigned int)data[1]);
> + break;
> + }
> +
> + /* Stop decoding after chassis segment */
> + if (data[0] == 4)
> + break;
> +
> + /* look for the next handle */
> + next = data + data[1];
> + while (next - dmi_table_mem + 1 < len && (next[0] != 0 || next[1] != 0))
> + next++;
> + next += 2;
> +
> + for (j = 0; j < ARRAY_SIZE(flashrom_dmi_strings); j++)
> + {
> + unsigned char offset = flashrom_dmi_strings[j].offset;
> + unsigned char type = flashrom_dmi_strings[j].type;
> +
> + if (offset >= data[1])
> + return;
> +
> + key = (type << 8) | offset;
> +
> + switch (key)
> + {
> + case 0x305: /* detect if laptop */
> + if (dmi_chassis_type(data[offset])) {
> + msg_pdbg("Laptop detected via DMI\n");
>
Can you print the chassis type in hex as well, even if it is not a
laptop? This will be helpful if a vendor screws up the DMI tables.
> + is_laptop = 1;
> + }
> + break;
> + default:
> + if (type == data[0]) {
> + dmistrings[j] = dmi_string((char*)data, data[1], data[offset]);
> + msg_pinfo("DMI string %s: \"%s\"\n",
> + flashrom_dmi_strings[j].keyword, dmistrings[j]);
> + }
> + }
> + }
> + data = next;
> + i++;
> + }
> +
> + physunmap(dmi_table, len);
> +}
> +
> +static int smbios_decode(unsigned char *buf)
> +{
> + if (!checksum(buf, buf[0x05])
> + || (memcmp(buf + 0x10, "_DMI_", 5) != 0)
> + || !checksum(buf + 0x10, 0x0F))
> + return 0;
> +
> + dmi_table(DWORD(buf + 0x18), WORD(buf + 0x16), WORD(buf + 0x1C));
> +
> + return 1;
> +}
> +
> +static int legacy_decode(unsigned char *buf)
> +{
> + if (!checksum(buf, 0x0F))
> + return 0;
> +
> + dmi_table(DWORD(buf + 0x08), WORD(buf + 0x06), WORD(buf + 0x0C));
> +
> + return 1;
> +}
>
> static char *get_dmi_string(const char *string_name)
> {
> FILE *dmidecode_pipe;
> char *result;
> char answerbuf[DMI_MAX_ANSWER_LEN];
> char commandline[DMI_COMMAND_LEN_MAX + 40];
>
> snprintf(commandline, sizeof(commandline),
> "%s -s %s", dmidecode_command, string_name);
> dmidecode_pipe = popen(commandline, "r");
> if (!dmidecode_pipe) {
> msg_perr("DMI pipe open error\n");
> @@ -109,45 +260,111 @@ static char *get_dmi_string(const char *string_name)
> answerbuf[strlen(answerbuf) - 1] == '\n')
> answerbuf[strlen(answerbuf) - 1] = 0;
> msg_pdbg("DMI string %s: \"%s\"\n", string_name, answerbuf);
>
> result = strdup(answerbuf);
> if (!result)
> puts("WARNING: Out of memory - DMI support fails");
>
> return result;
> }
>
> void dmi_init(void)
> {
> - int i;
> - char *chassis_type;
> + int found = 0;
> + size_t fp;
> + unsigned char *dmi_mem = NULL;
> + char *arg = NULL;
> + int use_new_dmi = 0;
> + int i = 0;
> +
> + arg = extract_programmer_param("dmi");
> + if (arg && !strcmp(arg,"new")) {
> + use_new_dmi = 1;
> + } else if (arg && !strcmp(arg, "check")) {
> + use_new_dmi = 2;
> + } else if (arg && !strlen(arg)) {
> + msg_perr("Missing argument for dmi. Falling back to external dmi\n");
> + } else if (arg) {
> + msg_perr("Unknown argument for dmi: %s. Falling back to external dmi\n", arg);
>
You could also return here. If the user doesn't know what he wants, we
shouldn't continue.
> + }
> + free(arg);
>
> has_dmi_support = 1;
> - for (i = 0; i < ARRAY_SIZE(dmidecode_names); i++) {
> - dmistrings[i] = get_dmi_string(dmidecode_names[i]);
> - if (!dmistrings[i]) {
> - has_dmi_support = 0;
> - return;
> +
> + if (use_new_dmi > 0) {
> + dmi_mem = physmap_try_ro("DMI", 0xF0000, 0x10000);
> +
> + if (!dmi_mem)
> + goto func_exit;
> +
> + for (fp = 0; fp <= 0xFFF0; fp += 16) {
> + if (memcmp(dmi_mem + fp, "_SM_", 4) == 0 && fp <= 0xFFE0) {
> + if (smbios_decode(dmi_mem+fp)) {
> + found++;
> + fp += 16;
> + }
> + }
> + else if (memcmp(dmi_mem + fp, "_DMI_", 5) == 0)
> + if (legacy_decode(dmi_mem + fp))
> + found++;
> }
> }
>
> - chassis_type = get_dmi_string("chassis-type");
> - if (chassis_type && (!strcmp(chassis_type, "Notebook") ||
> - !strcmp(chassis_type, "Portable"))) {
> - msg_pdbg("Laptop detected via DMI\n");
> - is_laptop = 1;
> + if (use_new_dmi == 2) {
> + for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings); i++)
> + {
> + external_dmi[i] = get_dmi_string(flashrom_dmi_strings[i].keyword);
> + if (!external_dmi[i])
> + goto func_exit;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings)-1; i++)
>
spaces
> + {
> + if (strcmp(dmistrings[i], external_dmi[i]))
> + msg_perr("dmidecode vs internal-dmi differs: %s\n", flashrom_dmi_strings[i].keyword);
> + else
> + msg_pspew("Matching of dmidecode and internal-dmi succeeded!\n");
>
Hm. Do we cross-check the laptop info?
> + }
> }
> - free(chassis_type);
> +
> + if (use_new_dmi == 0) {
> + char *chassis_type;
> + has_dmi_support = 1;
> + for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings); i++) {
> + dmistrings[i] = get_dmi_string(flashrom_dmi_strings[i].keyword);
> + if (!dmistrings[i]) {
> + has_dmi_support = 0;
> + return;
> + }
> + }
> +
> + chassis_type = get_dmi_string("chassis-type");
> + if (chassis_type && (!strcmp(chassis_type, "Notebook") ||
> + !strcmp(chassis_type, "Portable"))) {
> + msg_pdbg("Laptop detected via DMI\n");
> + is_laptop = 1;
> + }
> + free(chassis_type);
> + }
> +
> +func_exit:
> + if (!found)
> + {
> + msg_pinfo("No DMI table found.\n");
> + has_dmi_support = 0;
> + }
> +
> + physunmap(dmi_mem, 0x10000);
> }
>
> /**
> * Does an substring/prefix/postfix/whole-string match.
> *
> * The pattern is matched as-is. The only metacharacters supported are '^'
> * at the beginning and '$' at the end. So you can look for "^prefix",
> * "suffix$", "substring" or "^complete string$".
> *
> * @param value The string to check.
> * @param pattern The pattern.
> * @return Nonzero if pattern matches.
> */
> @@ -185,21 +402,21 @@ static int dmi_compare(const char *value, const char *pattern)
> if (anchored)
> return strncmp(value, pattern, patternlen) == 0;
> else
> return strstr(value, pattern) != NULL;
> }
>
> int dmi_match(const char *pattern)
> {
> int i;
>
> if (!has_dmi_support)
> return 0;
>
> - for (i = 0; i < ARRAY_SIZE(dmidecode_names); i++)
> + for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings); i++)
> if (dmi_compare(dmistrings[i], pattern))
> return 1;
>
> return 0;
> }
>
> #endif /* STANDALONE */
>
>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
flashrom@flashrom.org
http://www.flashrom.org/mailman/listinfo/flashrom