[Patch v2 0/3] firmware: dmi_scan: add SBMIOS entry point and DMI tables

[Patch v2 0/3] firmware: dmi_scan: add SBMIOS entry point and DMI tables

[Ivan Khoronzhuk]

This series adds SMBIOS entry point table and DMI table under
/sys/firmware/dmi/tables in order to use as an alternative to utilities
reading them from /dev/mem.

Based on linux next
+ 2 patches that have not been sent by Jean Dalvere yet. They can be changed,
but, anyway, the series should be based on smth and it's desirable w/o
conflicts. The patches can be obtained with links:

http://jdelvare.nerim.net/devel/linux-3/jdelvare-dmi/firmware-dmi-03-simplify-displayed-version.patch
http://jdelvare.nerim.net/devel/linux-3/jdelvare-dmi/firmware-dmi-04-fix-product-uuid.patch

Link on V1:
https://lkml.org/lkml/2015/4/2/297

Changes since V1
	- correct error path in dmi-sysfs
	- don't use globally dmi_table var
	- use "DMI" in attribute name
	- correct error path in dmi_init
	- leave dmi_kobj even in case of error
	- include linux/kobject.h in header

Ivan Khoronzhuk (3):
  firmware: dmi_scan: rename dmi_table to dmi_decode_table
  firmware: dmi_scan: add SBMIOS entry and DMI tables
  Documentation: ABI: sysfs-firmware-dmi: add -entries suffix to file
    name

 ...sfs-firmware-dmi => sysfs-firmware-dmi-entries} |  2 +-
 .../ABI/testing/sysfs-firmware-dmi-tables          | 22 ++++++
 drivers/firmware/dmi-sysfs.c                       | 17 ++--
 drivers/firmware/dmi_scan.c                        | 92 ++++++++++++++++++++--
 include/linux/dmi.h                                |  2 +
 5 files changed, 120 insertions(+), 15 deletions(-)
 rename Documentation/ABI/testing/{sysfs-firmware-dmi => sysfs-firmware-dmi-entries} (99%)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables

-- 
1.9.1

[Patch v2 1/3] firmware: dmi_scan: rename dmi_table to dmi_decode_table

[Ivan Khoronzhuk]

The "dmi_table" function looks like data instance, but it does DMI
table decode. This patch renames it to "dmi_decode_table" name as
more appropriate. That allows us to use "dmi_table" name for correct
purposes.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
---
 drivers/firmware/dmi_scan.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 97b1616..a864a6b 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -80,9 +80,9 @@ static const char * __init dmi_string(const struct dmi_header *dm, u8 s)
  *	We have to be cautious here. We have seen BIOSes with DMI pointers
  *	pointing to completely the wrong place for example
  */
-static void dmi_table(u8 *buf,
-		      void (*decode)(const struct dmi_header *, void *),
-		      void *private_data)
+static void dmi_decode_table(u8 *buf,
+			     void (*decode)(const struct dmi_header *, void *),
+			     void *private_data)
 {
 	u8 *data = buf;
 	int i = 0;
@@ -130,7 +130,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
 	if (buf == NULL)
 		return -1;
 
-	dmi_table(buf, decode, NULL);
+	dmi_decode_table(buf, decode, NULL);
 
 	add_device_randomness(buf, dmi_len);
 
@@ -897,7 +897,7 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	if (buf == NULL)
 		return -1;
 
-	dmi_table(buf, decode, private_data);
+	dmi_decode_table(buf, decode, private_data);
 
 	dmi_unmap(buf);
 	return 0;
-- 
1.9.1

[Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables

[Ivan Khoronzhuk]

Some utils, like dmidecode and smbios, need to access SMBIOS entry
table area in order to get information like SMBIOS version, size, etc.
Currently it's done via /dev/mem. But for situation when /dev/mem
usage is disabled, the utils have to use dmi sysfs instead, which
doesn't represent SMBIOS entry and adds code/delay redundancy when direct
access for table is needed.

So this patch creates dmi/tables and adds SMBIOS entry point to allow
utils in question to work correctly without /dev/mem. Also patch adds
raw dmi table to simplify dmi table processing in user space, as
proposed by Jean Delvare.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
---
 .../ABI/testing/sysfs-firmware-dmi-tables          | 22 ++++++
 drivers/firmware/dmi-sysfs.c                       | 17 +++--
 drivers/firmware/dmi_scan.c                        | 82 ++++++++++++++++++++++
 include/linux/dmi.h                                |  2 +
 4 files changed, 114 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables

diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
new file mode 100644
index 0000000..ff3cac8
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
@@ -0,0 +1,22 @@
+What:		/sys/firmware/dmi/tables/
+Date:		April 2015
+Contact:	Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
+Description:
+		The firmware provides DMI structures as a packed list of
+		data referenced by a SMBIOS table entry point. The SMBIOS
+		entry point contains general information, like SMBIOS
+		version, DMI table size, etc. The structure, content and
+		size of SMBIOS entry point is dependent on SMBIOS version.
+		The format of SMBIOS entry point and DMI structures
+		can be read in SMBIOS specification.
+
+		The dmi/tables provides raw SMBIOS entry point and DMI tables
+		through sysfs as an alternative to utilities reading them
+		from /dev/mem. The raw SMBIOS entry point and DMI table are
+		presented as binary attributes and are accessible via:
+
+		/sys/firmware/dmi/tables/smbios_entry_point
+		/sys/firmware/dmi/tables/DMI
+
+		The complete DMI information can be obtained using these two
+		tables.
diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index e0f1cb3..ef76e5e 100644
--- a/drivers/firmware/dmi-sysfs.c
+++ b/drivers/firmware/dmi-sysfs.c
@@ -566,7 +566,6 @@ static struct kobj_type dmi_sysfs_entry_ktype = {
 	.default_attrs = dmi_sysfs_entry_attrs,
 };
 
-static struct kobject *dmi_kobj;
 static struct kset *dmi_kset;
 
 /* Global count of all instances seen.  Only for setup */
@@ -648,17 +647,20 @@ static void cleanup_entry_list(void)
 
 static int __init dmi_sysfs_init(void)
 {
-	int error = -ENOMEM;
+	int error;
 	int val;
 
-	/* Set up our directory */
-	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
-	if (!dmi_kobj)
+	if (!dmi_kobj) {
+		pr_err("dmi-sysfs: dmi entry is absent.\n");
+		error = -ENODATA;
 		goto err;
+	}
 
 	dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
-	if (!dmi_kset)
+	if (!dmi_kset) {
+		error = -ENOMEM;
 		goto err;
+	}
 
 	val = 0;
 	error = dmi_walk(dmi_sysfs_register_handle, &val);
@@ -675,7 +677,6 @@ static int __init dmi_sysfs_init(void)
 err:
 	cleanup_entry_list();
 	kset_unregister(dmi_kset);
-	kobject_put(dmi_kobj);
 	return error;
 }
 
@@ -685,8 +686,6 @@ static void __exit dmi_sysfs_exit(void)
 	pr_debug("dmi-sysfs: unloading.\n");
 	cleanup_entry_list();
 	kset_unregister(dmi_kset);
-	kobject_del(dmi_kobj);
-	kobject_put(dmi_kobj);
 }
 
 module_init(dmi_sysfs_init);
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index a864a6b..9705be2 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -10,6 +10,9 @@
 #include <asm/dmi.h>
 #include <asm/unaligned.h>
 
+struct kobject *dmi_kobj;
+EXPORT_SYMBOL_GPL(dmi_kobj);
+
 /*
  * DMI stands for "Desktop Management Interface".  It is part
  * of and an antecedent to, SMBIOS, which stands for System
@@ -20,6 +23,9 @@ static const char dmi_empty_string[] = "        ";
 static u32 dmi_ver __initdata;
 static u32 dmi_len;
 static u16 dmi_num;
+static u8 smbios_entry_point[32];
+static int smbios_entry_point_size;
+
 /*
  * Catch too early calls to dmi_check_system():
  */
@@ -478,6 +484,8 @@ static int __init dmi_present(const u8 *buf)
 	if (memcmp(buf, "_SM_", 4) == 0 &&
 	    buf[5] < 32 && dmi_checksum(buf, buf[5])) {
 		smbios_ver = get_unaligned_be16(buf + 6);
+		smbios_entry_point_size = buf[5];
+		memcpy(smbios_entry_point, buf, smbios_entry_point_size);
 
 		/* Some BIOS report weird SMBIOS version, fix that up */
 		switch (smbios_ver) {
@@ -512,6 +520,9 @@ static int __init dmi_present(const u8 *buf)
 				pr_info("SMBIOS %d.%d present.\n",
 				       dmi_ver >> 8, dmi_ver & 0xFF);
 			} else {
+				smbios_entry_point_size = 15;
+				memcpy(smbios_entry_point, buf,
+				       smbios_entry_point_size);
 				pr_info("Legacy DMI %d.%d present.\n",
 				       dmi_ver >> 8, dmi_ver & 0xFF);
 			}
@@ -538,6 +549,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
 		dmi_num = 0;			/* No longer specified */
 		dmi_len = get_unaligned_le32(buf + 12);
 		dmi_base = get_unaligned_le64(buf + 16);
+		smbios_entry_point_size = buf[6];
+		memcpy(smbios_entry_point, buf, smbios_entry_point_size);
 
 		if (dmi_walk_early(dmi_decode) == 0) {
 			pr_info("SMBIOS %d.%d.%d present.\n",
@@ -629,6 +642,75 @@ void __init dmi_scan_machine(void)
 	dmi_initialized = 1;
 }
 
+static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
+			      struct bin_attribute *attr, char *buf,
+			      loff_t pos, size_t count)
+{
+	memcpy(buf, attr->private + pos, count);
+	return count;
+}
+
+static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
+static BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
+
+static int __init dmi_init(void)
+{
+	int ret;
+	u8 *dmi_table = NULL;
+	struct kobject *tables_kobj = NULL;
+
+	if (!dmi_available) {
+		ret = -ENODATA;
+		goto err;
+	}
+
+	/*
+	 * Set up dmi directory at /sys/firmware/dmi. This entry should stay
+	 * even after farther error, as it can be used by other modules like
+	 * dmi-sysfs.
+	 */
+	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
+	tables_kobj = kobject_create_and_add("tables", dmi_kobj);
+	if (!(dmi_kobj && tables_kobj)) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	bin_attr_smbios_entry_point.size = smbios_entry_point_size;
+	bin_attr_smbios_entry_point.private = smbios_entry_point;
+	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_smbios_entry_point);
+	if (ret)
+		goto err;
+
+	dmi_table = dmi_remap(dmi_base, dmi_len);
+	if (!dmi_table) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	bin_attr_DMI.size = dmi_len;
+	bin_attr_DMI.private = dmi_table;
+	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
+	if (!ret)
+		return 0;
+
+err:
+	pr_err("dmi: Firmware registration failed.\n");
+
+	if (tables_kobj) {
+		sysfs_remove_bin_file(tables_kobj,
+				      &bin_attr_smbios_entry_point);
+		kobject_del(tables_kobj);
+		kobject_put(tables_kobj);
+	}
+
+	if (dmi_table)
+		dmi_unmap(dmi_table);
+
+	return ret;
+}
+subsys_initcall(dmi_init);
+
 /**
  * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
  *
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index f820f0a..2f9f988 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -2,6 +2,7 @@
 #define __DMI_H__
 
 #include <linux/list.h>
+#include <linux/kobject.h>
 #include <linux/mod_devicetable.h>
 
 /* enum dmi_field is in mod_devicetable.h */
@@ -93,6 +94,7 @@ struct dmi_dev_onboard {
 	int devfn;
 };
 
+extern struct kobject *dmi_kobj;
 extern int dmi_check_system(const struct dmi_system_id *list);
 const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
 extern const char * dmi_get_system_info(int field);
-- 
1.9.1

[Patch v2 3/3] Documentation: ABI: sysfs-firmware-dmi: add -entries suffix to file name

[Ivan Khoronzhuk]

The dmi-sysfs module adds DMI table structures entries under
/sys/firmware/dmi/entries only, so rename documentation file to
sysfs-firmware-dmi-entries as more appropriate. Without renaming it's
confusing to differ this from sysfs-firmware-dmi-tables that adds raw
DMI table and actually adds "dmi" kobject.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
---
 .../ABI/testing/{sysfs-firmware-dmi => sysfs-firmware-dmi-entries}      | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 rename Documentation/ABI/testing/{sysfs-firmware-dmi => sysfs-firmware-dmi-entries} (99%)

diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi-entries
similarity index 99%
rename from Documentation/ABI/testing/sysfs-firmware-dmi
rename to Documentation/ABI/testing/sysfs-firmware-dmi-entries
index c78f9ab..210ad44 100644
--- a/Documentation/ABI/testing/sysfs-firmware-dmi
+++ b/Documentation/ABI/testing/sysfs-firmware-dmi-entries
@@ -1,4 +1,4 @@
-What:		/sys/firmware/dmi/
+What:		/sys/firmware/dmi/entries/
 Date:		February 2011
 Contact:	Mike Waychison <mikew@google.com>
 Description:
-- 
1.9.1

Re: [Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables

[Roy Franz]

On Mon, Apr 20, 2015 at 3:19 AM, Ivan Khoronzhuk
<ivan.khoronzhuk@globallogic.com> wrote:
> Some utils, like dmidecode and smbios, need to access SMBIOS entry
> table area in order to get information like SMBIOS version, size, etc.
> Currently it's done via /dev/mem. But for situation when /dev/mem
> usage is disabled, the utils have to use dmi sysfs instead, which
> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
> access for table is needed.
>
> So this patch creates dmi/tables and adds SMBIOS entry point to allow
> utils in question to work correctly without /dev/mem. Also patch adds
> raw dmi table to simplify dmi table processing in user space, as
> proposed by Jean Delvare.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>

Tested-by: Roy Franz <roy.franz@linaro.org>

Tested with dmidecode w/patches to read tables from sysfs.  The dmidecode
patches are posted on dmidecode-devel and should be commited upstream soon.

Thanks,
Roy

> ---
>  .../ABI/testing/sysfs-firmware-dmi-tables          | 22 ++++++
>  drivers/firmware/dmi-sysfs.c                       | 17 +++--
>  drivers/firmware/dmi_scan.c                        | 82 ++++++++++++++++++++++
>  include/linux/dmi.h                                |  2 +
>  4 files changed, 114 insertions(+), 9 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
>
> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
> new file mode 100644
> index 0000000..ff3cac8
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
> @@ -0,0 +1,22 @@
> +What:          /sys/firmware/dmi/tables/
> +Date:          April 2015
> +Contact:       Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
> +Description:
> +               The firmware provides DMI structures as a packed list of
> +               data referenced by a SMBIOS table entry point. The SMBIOS
> +               entry point contains general information, like SMBIOS
> +               version, DMI table size, etc. The structure, content and
> +               size of SMBIOS entry point is dependent on SMBIOS version.
> +               The format of SMBIOS entry point and DMI structures
> +               can be read in SMBIOS specification.
> +
> +               The dmi/tables provides raw SMBIOS entry point and DMI tables
> +               through sysfs as an alternative to utilities reading them
> +               from /dev/mem. The raw SMBIOS entry point and DMI table are
> +               presented as binary attributes and are accessible via:
> +
> +               /sys/firmware/dmi/tables/smbios_entry_point
> +               /sys/firmware/dmi/tables/DMI
> +
> +               The complete DMI information can be obtained using these two
> +               tables.
> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
> index e0f1cb3..ef76e5e 100644
> --- a/drivers/firmware/dmi-sysfs.c
> +++ b/drivers/firmware/dmi-sysfs.c
> @@ -566,7 +566,6 @@ static struct kobj_type dmi_sysfs_entry_ktype = {
>         .default_attrs = dmi_sysfs_entry_attrs,
>  };
>
> -static struct kobject *dmi_kobj;
>  static struct kset *dmi_kset;
>
>  /* Global count of all instances seen.  Only for setup */
> @@ -648,17 +647,20 @@ static void cleanup_entry_list(void)
>
>  static int __init dmi_sysfs_init(void)
>  {
> -       int error = -ENOMEM;
> +       int error;
>         int val;
>
> -       /* Set up our directory */
> -       dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> -       if (!dmi_kobj)
> +       if (!dmi_kobj) {
> +               pr_err("dmi-sysfs: dmi entry is absent.\n");
> +               error = -ENODATA;
>                 goto err;
> +       }
>
>         dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
> -       if (!dmi_kset)
> +       if (!dmi_kset) {
> +               error = -ENOMEM;
>                 goto err;
> +       }
>
>         val = 0;
>         error = dmi_walk(dmi_sysfs_register_handle, &val);
> @@ -675,7 +677,6 @@ static int __init dmi_sysfs_init(void)
>  err:
>         cleanup_entry_list();
>         kset_unregister(dmi_kset);
> -       kobject_put(dmi_kobj);
>         return error;
>  }
>
> @@ -685,8 +686,6 @@ static void __exit dmi_sysfs_exit(void)
>         pr_debug("dmi-sysfs: unloading.\n");
>         cleanup_entry_list();
>         kset_unregister(dmi_kset);
> -       kobject_del(dmi_kobj);
> -       kobject_put(dmi_kobj);
>  }
>
>  module_init(dmi_sysfs_init);
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index a864a6b..9705be2 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -10,6 +10,9 @@
>  #include <asm/dmi.h>
>  #include <asm/unaligned.h>
>
> +struct kobject *dmi_kobj;
> +EXPORT_SYMBOL_GPL(dmi_kobj);
> +
>  /*
>   * DMI stands for "Desktop Management Interface".  It is part
>   * of and an antecedent to, SMBIOS, which stands for System
> @@ -20,6 +23,9 @@ static const char dmi_empty_string[] = "        ";
>  static u32 dmi_ver __initdata;
>  static u32 dmi_len;
>  static u16 dmi_num;
> +static u8 smbios_entry_point[32];
> +static int smbios_entry_point_size;
> +
>  /*
>   * Catch too early calls to dmi_check_system():
>   */
> @@ -478,6 +484,8 @@ static int __init dmi_present(const u8 *buf)
>         if (memcmp(buf, "_SM_", 4) == 0 &&
>             buf[5] < 32 && dmi_checksum(buf, buf[5])) {
>                 smbios_ver = get_unaligned_be16(buf + 6);
> +               smbios_entry_point_size = buf[5];
> +               memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>
>                 /* Some BIOS report weird SMBIOS version, fix that up */
>                 switch (smbios_ver) {
> @@ -512,6 +520,9 @@ static int __init dmi_present(const u8 *buf)
>                                 pr_info("SMBIOS %d.%d present.\n",
>                                        dmi_ver >> 8, dmi_ver & 0xFF);
>                         } else {
> +                               smbios_entry_point_size = 15;
> +                               memcpy(smbios_entry_point, buf,
> +                                      smbios_entry_point_size);
>                                 pr_info("Legacy DMI %d.%d present.\n",
>                                        dmi_ver >> 8, dmi_ver & 0xFF);
>                         }
> @@ -538,6 +549,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>                 dmi_num = 0;                    /* No longer specified */
>                 dmi_len = get_unaligned_le32(buf + 12);
>                 dmi_base = get_unaligned_le64(buf + 16);
> +               smbios_entry_point_size = buf[6];
> +               memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>
>                 if (dmi_walk_early(dmi_decode) == 0) {
>                         pr_info("SMBIOS %d.%d.%d present.\n",
> @@ -629,6 +642,75 @@ void __init dmi_scan_machine(void)
>         dmi_initialized = 1;
>  }
>
> +static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
> +                             struct bin_attribute *attr, char *buf,
> +                             loff_t pos, size_t count)
> +{
> +       memcpy(buf, attr->private + pos, count);
> +       return count;
> +}
> +
> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
> +static BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
> +
> +static int __init dmi_init(void)
> +{
> +       int ret;
> +       u8 *dmi_table = NULL;
> +       struct kobject *tables_kobj = NULL;
> +
> +       if (!dmi_available) {
> +               ret = -ENODATA;
> +               goto err;
> +       }
> +
> +       /*
> +        * Set up dmi directory at /sys/firmware/dmi. This entry should stay
> +        * even after farther error, as it can be used by other modules like
> +        * dmi-sysfs.
> +        */
> +       dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> +       tables_kobj = kobject_create_and_add("tables", dmi_kobj);
> +       if (!(dmi_kobj && tables_kobj)) {
> +               ret = -ENOMEM;
> +               goto err;
> +       }
> +
> +       bin_attr_smbios_entry_point.size = smbios_entry_point_size;
> +       bin_attr_smbios_entry_point.private = smbios_entry_point;
> +       ret = sysfs_create_bin_file(tables_kobj, &bin_attr_smbios_entry_point);
> +       if (ret)
> +               goto err;
> +
> +       dmi_table = dmi_remap(dmi_base, dmi_len);
> +       if (!dmi_table) {
> +               ret = -ENOMEM;
> +               goto err;
> +       }
> +
> +       bin_attr_DMI.size = dmi_len;
> +       bin_attr_DMI.private = dmi_table;
> +       ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
> +       if (!ret)
> +               return 0;
> +
> +err:
> +       pr_err("dmi: Firmware registration failed.\n");
> +
> +       if (tables_kobj) {
> +               sysfs_remove_bin_file(tables_kobj,
> +                                     &bin_attr_smbios_entry_point);
> +               kobject_del(tables_kobj);
> +               kobject_put(tables_kobj);
> +       }
> +
> +       if (dmi_table)
> +               dmi_unmap(dmi_table);
> +
> +       return ret;
> +}
> +subsys_initcall(dmi_init);
> +
>  /**
>   * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
>   *
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index f820f0a..2f9f988 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -2,6 +2,7 @@
>  #define __DMI_H__
>
>  #include <linux/list.h>
> +#include <linux/kobject.h>
>  #include <linux/mod_devicetable.h>
>
>  /* enum dmi_field is in mod_devicetable.h */
> @@ -93,6 +94,7 @@ struct dmi_dev_onboard {
>         int devfn;
>  };
>
> +extern struct kobject *dmi_kobj;
>  extern int dmi_check_system(const struct dmi_system_id *list);
>  const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
>  extern const char * dmi_get_system_info(int field);
> --
> 1.9.1
>

Re: [Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables

[Jean Delvare]

Hi Ivan,

On Mon, 20 Apr 2015 13:19:46 +0300, Ivan Khoronzhuk wrote:
> Some utils, like dmidecode and smbios, need to access SMBIOS entry
> table area in order to get information like SMBIOS version, size, etc.
> Currently it's done via /dev/mem. But for situation when /dev/mem
> usage is disabled, the utils have to use dmi sysfs instead, which
> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
> access for table is needed.
> 
> So this patch creates dmi/tables and adds SMBIOS entry point to allow
> utils in question to work correctly without /dev/mem. Also patch adds
> raw dmi table to simplify dmi table processing in user space, as
> proposed by Jean Delvare.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
> ---
>  .../ABI/testing/sysfs-firmware-dmi-tables          | 22 ++++++
>  drivers/firmware/dmi-sysfs.c                       | 17 +++--
>  drivers/firmware/dmi_scan.c                        | 82 ++++++++++++++++++++++
>  include/linux/dmi.h                                |  2 +
>  4 files changed, 114 insertions(+), 9 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
> (...)
> +static int __init dmi_init(void)
> +{
> +	int ret;
> +	u8 *dmi_table = NULL;
> +	struct kobject *tables_kobj = NULL;
> +
> +	if (!dmi_available) {
> +		ret = -ENODATA;
> +		goto err;
> +	}
> +
> +	/*
> +	 * Set up dmi directory at /sys/firmware/dmi. This entry should stay
> +	 * even after farther error, as it can be used by other modules like
> +	 * dmi-sysfs.
> +	 */
> +	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> +	tables_kobj = kobject_create_and_add("tables", dmi_kobj);

I'm afraid you can't do that. kobject_create_and_add() doesn't check if
the parent is NULL and will happily create "tables" at the root of
sysfs if for any reason the previous call to kobject_create_and_add()
failed and returned NULL. I agree it is unlikely and would be cleaned
up immediately, but still, instantiating an object at the wrong place,
even temporarily, is wrong.

> +	if (!(dmi_kobj && tables_kobj)) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}

I'd rather go with:

	if (!(dmi_kobj = kobject_create_and_add("dmi", firmware_kobj))
	 || !(tables_kobj = kobject_create_and_add("tables", dmi_kobj))) {

I know that checkpatch complains about this construct, but in many
cases it is the right thing to do.

Another possible approach is simply:

	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
	if (dmi_kobj)
		tables_kobj = kobject_create_and_add("tables", dmi_kobj);
	if (!tables_kobj) {

> +
> +	bin_attr_smbios_entry_point.size = smbios_entry_point_size;
> +	bin_attr_smbios_entry_point.private = smbios_entry_point;
> +	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_smbios_entry_point);
> +	if (ret)
> +		goto err;
> +
> +	dmi_table = dmi_remap(dmi_base, dmi_len);
> +	if (!dmi_table) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	bin_attr_DMI.size = dmi_len;
> +	bin_attr_DMI.private = dmi_table;
> +	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
> +	if (!ret)
> +		return 0;
> +
> +err:
> +	pr_err("dmi: Firmware registration failed.\n");
> +
> +	if (tables_kobj) {
> +		sysfs_remove_bin_file(tables_kobj,
> +				      &bin_attr_smbios_entry_point);
> +		kobject_del(tables_kobj);
> +		kobject_put(tables_kobj);
> +	}
> +
> +	if (dmi_table)
> +		dmi_unmap(dmi_table);

I'm not happy with this single error label. This forces you to
initialize all your pointers to NULL and then to check for them to find
out what needs to be done in the error path. With multiple error labels,
you would know exactly what needs to be done, this is more efficient.

> +
> +	return ret;
> +}

Everything else looks good. No need to resend, I'll fix up the above
myself in place or as an incremental patch.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables

[Ivan.khoronzhuk]

Hi Jean,

On 21.04.15 17:24, Jean Delvare wrote:
> Hi Ivan,
>
> On Mon, 20 Apr 2015 13:19:46 +0300, Ivan Khoronzhuk wrote:
>> Some utils, like dmidecode and smbios, need to access SMBIOS entry
>> table area in order to get information like SMBIOS version, size, etc.
>> Currently it's done via /dev/mem. But for situation when /dev/mem
>> usage is disabled, the utils have to use dmi sysfs instead, which
>> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
>> access for table is needed.
>>
>> So this patch creates dmi/tables and adds SMBIOS entry point to allow
>> utils in question to work correctly without /dev/mem. Also patch adds
>> raw dmi table to simplify dmi table processing in user space, as
>> proposed by Jean Delvare.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
>> ---
>>   .../ABI/testing/sysfs-firmware-dmi-tables          | 22 ++++++
>>   drivers/firmware/dmi-sysfs.c                       | 17 +++--
>>   drivers/firmware/dmi_scan.c                        | 82 ++++++++++++++++++++++
>>   include/linux/dmi.h                                |  2 +
>>   4 files changed, 114 insertions(+), 9 deletions(-)
>>   create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> (...)
>> +static int __init dmi_init(void)
>> +{
>> +	int ret;
>> +	u8 *dmi_table = NULL;
>> +	struct kobject *tables_kobj = NULL;
>> +
>> +	if (!dmi_available) {
>> +		ret = -ENODATA;
>> +		goto err;
>> +	}
>> +
>> +	/*
>> +	 * Set up dmi directory at /sys/firmware/dmi. This entry should stay
>> +	 * even after farther error, as it can be used by other modules like
>> +	 * dmi-sysfs.
>> +	 */
>> +	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> +	tables_kobj = kobject_create_and_add("tables", dmi_kobj);
> I'm afraid you can't do that. kobject_create_and_add() doesn't check if
> the parent is NULL and will happily create "tables" at the root of
> sysfs if for any reason the previous call to kobject_create_and_add()
> failed and returned NULL. I agree it is unlikely and would be cleaned
> up immediately, but still, instantiating an object at the wrong place,
> even temporarily, is wrong.
>
>> +	if (!(dmi_kobj && tables_kobj)) {
>> +		ret = -ENOMEM;
>> +		goto err;
>> +	}
> I'd rather go with:
>
> 	if (!(dmi_kobj = kobject_create_and_add("dmi", firmware_kobj))
> 	 || !(tables_kobj = kobject_create_and_add("tables", dmi_kobj))) {
>
> I know that checkpatch complains about this construct, but in many
> cases it is the right thing to do.
>
> Another possible approach is simply:
>
> 	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> 	if (dmi_kobj)
> 		tables_kobj = kobject_create_and_add("tables", dmi_kobj);
> 	if (!tables_kobj) {
>
>> +
>> +	bin_attr_smbios_entry_point.size = smbios_entry_point_size;
>> +	bin_attr_smbios_entry_point.private = smbios_entry_point;
>> +	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_smbios_entry_point);
>> +	if (ret)
>> +		goto err;
>> +
>> +	dmi_table = dmi_remap(dmi_base, dmi_len);
>> +	if (!dmi_table) {
>> +		ret = -ENOMEM;
>> +		goto err;
>> +	}
>> +
>> +	bin_attr_DMI.size = dmi_len;
>> +	bin_attr_DMI.private = dmi_table;
>> +	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
>> +	if (!ret)
>> +		return 0;
>> +
>> +err:
>> +	pr_err("dmi: Firmware registration failed.\n");
>> +
>> +	if (tables_kobj) {
>> +		sysfs_remove_bin_file(tables_kobj,
>> +				      &bin_attr_smbios_entry_point);
>> +		kobject_del(tables_kobj);
>> +		kobject_put(tables_kobj);
>> +	}
>> +
>> +	if (dmi_table)
>> +		dmi_unmap(dmi_table);
> I'm not happy with this single error label. This forces you to
> initialize all your pointers to NULL and then to check for them to find
> out what needs to be done in the error path. With multiple error labels,
> you would know exactly what needs to be done, this is more efficient.
>
>> +
>> +	return ret;
>> +}
> Everything else looks good. No need to resend, I'll fix up the above
> myself in place or as an incremental patch.
>
> Thanks,

Don't bother, I'll send corrected version.

-- 
Regards,
Ivan Khoronzhuk

Re: [Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables

[Jean Delvare]

Hi again Ivan,

On Mon, 20 Apr 2015 13:19:46 +0300, Ivan Khoronzhuk wrote:
> +	bin_attr_DMI.size = dmi_len;
> +	bin_attr_DMI.private = dmi_table;
> +	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
> +	if (!ret)
> +		return 0;

I just found that more work is needed here for the SMBIOS v3 entry
point case. These entry points do not specify the exact length of the
table, but only its maximum. The real world sample I have access to
indeed specifies a maximum length of 6419 bytes, but the actual table
only spans over 2373 bytes. It is properly terminated with a type 127
DMI structure, so the kernel table parser ignores the garbage after it.
The garbage is however exported to user-space above.

I taught dmidecode to ignore the garbage, but there are two problem
left here. First problem is a waste of memory. Minor issue I suppose,
who cares about a few kilobytes these days.

Second problem is a security problem. We are leaking the contents of
physical memory to user-space. In my case it's filled with 0xffs so no
big deal. But what if actual data happens to be stored there? It
definitely shouldn't go to user-space.

So dmi_len needs to be trimmed to the actual table size before the
attribute above is created. I have an idea how this could be
implemented easily, let me give it a try.

Maybe we should trim the length for previous implementations, too.
There is no reason to walk past a type 127 structure anyway, ever.

-- 
Jean Delvare
SUSE L3 Support

Re: [Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables

[Ivan.khoronzhuk]

Jean,

On 21.04.15 18:36, Jean Delvare wrote:
> Hi again Ivan,
>
> On Mon, 20 Apr 2015 13:19:46 +0300, Ivan Khoronzhuk wrote:
>> +	bin_attr_DMI.size = dmi_len;
>> +	bin_attr_DMI.private = dmi_table;
>> +	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
>> +	if (!ret)
>> +		return 0;
> I just found that more work is needed here for the SMBIOS v3 entry
> point case. These entry points do not specify the exact length of the
> table, but only its maximum. The real world sample I have access to
> indeed specifies a maximum length of 6419 bytes, but the actual table
> only spans over 2373 bytes. It is properly terminated with a type 127
> DMI structure, so the kernel table parser ignores the garbage after it.
> The garbage is however exported to user-space above.
>
> I taught dmidecode to ignore the garbage, but there are two problem
> left here. First problem is a waste of memory. Minor issue I suppose,
> who cares about a few kilobytes these days.
>
> Second problem is a security problem. We are leaking the contents of
> physical memory to user-space. In my case it's filled with 0xffs so no
> big deal. But what if actual data happens to be stored there? It
> definitely shouldn't go to user-space.
>
> So dmi_len needs to be trimmed to the actual table size before the
> attribute above is created. I have an idea how this could be
> implemented easily, let me give it a try.
>
> Maybe we should trim the length for previous implementations, too.
> There is no reason to walk past a type 127 structure anyway, ever.
>

It can happen of-cause, I've also thought about that sometime ago,
but forget...).
I've sent the updated series already.
Let me know when your fix will be ready and I will re-base the
series if it has conflicts.

-- 
Regards,
Ivan Khoronzhuk

Re: [Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables

[Jean Delvare]

Le Tuesday 21 April 2015 à 19:03 +0300, Ivan.khoronzhuk a écrit :
> On 21.04.15 18:36, Jean Delvare wrote:
> > I just found that more work is needed here for the SMBIOS v3 entry
> > point case. These entry points do not specify the exact length of the
> > table, but only its maximum. The real world sample I have access to
> > indeed specifies a maximum length of 6419 bytes, but the actual table
> > only spans over 2373 bytes. It is properly terminated with a type 127
> > DMI structure, so the kernel table parser ignores the garbage after it.
> > The garbage is however exported to user-space above.
> >
> > I taught dmidecode to ignore the garbage, but there are two problem
> > left here. First problem is a waste of memory. Minor issue I suppose,
> > who cares about a few kilobytes these days.
> >
> > Second problem is a security problem. We are leaking the contents of
> > physical memory to user-space. In my case it's filled with 0xffs so no
> > big deal. But what if actual data happens to be stored there? It
> > definitely shouldn't go to user-space.
> >
> > So dmi_len needs to be trimmed to the actual table size before the
> > attribute above is created. I have an idea how this could be
> > implemented easily, let me give it a try.
> >
> > Maybe we should trim the length for previous implementations, too.
> > There is no reason to walk past a type 127 structure anyway, ever.
> >
> 
> It can happen of-cause, I've also thought about that sometime ago,
> but forget...).
> I've sent the updated series already.

Got it, reviewed and ready to push upstream. I'll do so as soon as I'm
done with other duties.

> Let me know when your fix will be ready and I will re-base the
> series if it has conflicts.

Don't worry, I'm quite good at manual conflict resolution :)

Thanks,
-- 
Jean Delvare
SUSE L3 Support