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

[Patch 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.

This is logical continuation of
"[Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs"
https://lkml.org/lkml/2015/3/16/1070

Based on efi/next

Last changes:
	- renamed dmi_table to dmi_decode_table
	- moved tables under /sys/firmware/dmi/tables/
	- avoid "subsystem" word
	- split in separate patch correction of dmi-sysfs documentation
	- using -ENOSYS instead of -EINVAL
	- use user read only rights for table attributes
	- use "static" for attributes
	- check only dmi_available for table presents
	- don't correct dmi_walk to constantly map dmi_table
	- explicitly delete binary attributes
	- assign dmi_kobj = NULL in case of error
	- don't export dmi_kobj if CONFIG_DMI is not set
	- improve read callback for table attributes

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                       | 11 +--
 drivers/firmware/dmi_scan.c                        | 90 ++++++++++++++++++++--
 include/linux/dmi.h                                |  1 +
 5 files changed, 113 insertions(+), 13 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 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.

This is logical continuation of
"[Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs"
https://lkml.org/lkml/2015/3/16/1070

Based on efi/next

Last changes:
	- renamed dmi_table to dmi_decode_table
	- moved tables under /sys/firmware/dmi/tables/
	- avoid "subsystem" word
	- split in separate patch correction of dmi-sysfs documentation
	- using -ENOSYS instead of -EINVAL
	- use user read only rights for table attributes
	- use "static" for attributes
	- check only dmi_available for table presents
	- don't correct dmi_walk to constantly map dmi_table
	- explicitly delete binary attributes
	- assign dmi_kobj = NULL in case of error
	- don't export dmi_kobj if CONFIG_DMI is not set
	- improve read callback for table attributes

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                       | 11 +--
 drivers/firmware/dmi_scan.c                        | 90 ++++++++++++++++++++--
 include/linux/dmi.h                                |  1 +
 5 files changed, 113 insertions(+), 13 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 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.

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 c9cb725..d3aae09 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;
@@ -128,7 +128,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);
 
@@ -906,7 +906,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 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.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
---
 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 c9cb725..d3aae09 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;
@@ -128,7 +128,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);
 
@@ -906,7 +906,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 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                       | 11 ++-
 drivers/firmware/dmi_scan.c                        | 80 ++++++++++++++++++++++
 include/linux/dmi.h                                |  1 +
 4 files changed, 107 insertions(+), 7 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..f46158c
--- /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, equal as 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 raw attributes and are accessible via:
+
+		/sys/firmware/dmi/tables/smbios_entry_point
+		/sys/firmware/dmi/tables/DMI
+
+		The complete DMI information can be taken using these two
+		tables.
diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index e0f1cb3..8e1a411 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 */
@@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
 	int error = -ENOMEM;
 	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 = -ENOSYS;
 		goto err;
+	}
 
 	dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
 	if (!dmi_kset)
@@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
 err:
 	cleanup_entry_list();
 	kset_unregister(dmi_kset);
-	kobject_put(dmi_kobj);
 	return error;
 }
 
@@ -685,8 +684,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 d3aae09..bb19f8b 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():
  */
@@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
 }
 
 static phys_addr_t dmi_base;
+static u8 *dmi_table;
 
 static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
 		void *))
@@ -476,6 +483,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) {
@@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
 					dmi_ver >> 8, dmi_ver & 0xFF,
 					(dmi_ver < 0x0300) ? "" : ".x");
 			} else {
+				smbios_entry_point_size = 15;
+				memcpy(smbios_entry_point, buf,
+				       smbios_entry_point_size);
 				dmi_ver = (buf[14] & 0xF0) << 4 |
 					   (buf[14] & 0x0F);
 				pr_info("Legacy DMI %d.%d present.\n",
@@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
 		dmi_ver &= 0xFFFFFF;
 		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);
 
 		/*
 		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
@@ -638,6 +652,72 @@ 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);
+struct bin_attribute bin_attr_dmi_table =
+			__BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
+
+static int __init dmi_init(void)
+{
+	int ret = -ENOMEM;
+	struct kobject *tables_kobj = NULL;
+
+	if (!dmi_available) {
+		ret = -ENOSYS;
+		goto err;
+	}
+
+	/* Set up dmi directory at /sys/firmware/dmi */
+	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
+	if (!dmi_kobj)
+		goto err;
+
+	tables_kobj = kobject_create_and_add("tables", dmi_kobj);
+	if (!tables_kobj)
+		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)
+		goto err;
+
+	bin_attr_dmi_table.size = dmi_len;
+	bin_attr_dmi_table.private = dmi_table;
+	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
+	if (ret) {
+		dmi_unmap(dmi_table);
+		goto err;
+	}
+
+	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);
+	kobject_del(dmi_kobj);
+	kobject_put(dmi_kobj);
+	dmi_kobj = NULL;
+
+	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..9f55f46 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -93,6 +93,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 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-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
---
 .../ABI/testing/sysfs-firmware-dmi-tables          | 22 ++++++
 drivers/firmware/dmi-sysfs.c                       | 11 ++-
 drivers/firmware/dmi_scan.c                        | 80 ++++++++++++++++++++++
 include/linux/dmi.h                                |  1 +
 4 files changed, 107 insertions(+), 7 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..f46158c
--- /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-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
+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, equal as 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 raw attributes and are accessible via:
+
+		/sys/firmware/dmi/tables/smbios_entry_point
+		/sys/firmware/dmi/tables/DMI
+
+		The complete DMI information can be taken using these two
+		tables.
diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index e0f1cb3..8e1a411 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 */
@@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
 	int error = -ENOMEM;
 	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 = -ENOSYS;
 		goto err;
+	}
 
 	dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
 	if (!dmi_kset)
@@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
 err:
 	cleanup_entry_list();
 	kset_unregister(dmi_kset);
-	kobject_put(dmi_kobj);
 	return error;
 }
 
@@ -685,8 +684,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 d3aae09..bb19f8b 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():
  */
@@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
 }
 
 static phys_addr_t dmi_base;
+static u8 *dmi_table;
 
 static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
 		void *))
@@ -476,6 +483,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) {
@@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
 					dmi_ver >> 8, dmi_ver & 0xFF,
 					(dmi_ver < 0x0300) ? "" : ".x");
 			} else {
+				smbios_entry_point_size = 15;
+				memcpy(smbios_entry_point, buf,
+				       smbios_entry_point_size);
 				dmi_ver = (buf[14] & 0xF0) << 4 |
 					   (buf[14] & 0x0F);
 				pr_info("Legacy DMI %d.%d present.\n",
@@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
 		dmi_ver &= 0xFFFFFF;
 		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);
 
 		/*
 		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
@@ -638,6 +652,72 @@ 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);
+struct bin_attribute bin_attr_dmi_table =
+			__BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
+
+static int __init dmi_init(void)
+{
+	int ret = -ENOMEM;
+	struct kobject *tables_kobj = NULL;
+
+	if (!dmi_available) {
+		ret = -ENOSYS;
+		goto err;
+	}
+
+	/* Set up dmi directory at /sys/firmware/dmi */
+	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
+	if (!dmi_kobj)
+		goto err;
+
+	tables_kobj = kobject_create_and_add("tables", dmi_kobj);
+	if (!tables_kobj)
+		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)
+		goto err;
+
+	bin_attr_dmi_table.size = dmi_len;
+	bin_attr_dmi_table.private = dmi_table;
+	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
+	if (ret) {
+		dmi_unmap(dmi_table);
+		goto err;
+	}
+
+	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);
+	kobject_del(dmi_kobj);
+	kobject_put(dmi_kobj);
+	dmi_kobj = NULL;
+
+	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..9f55f46 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -93,6 +93,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 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.

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

[Patch 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.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
---
 .../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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
 Description:
-- 
1.9.1

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

[Ivan.khoronzhuk]

On 02.04.15 15:57, 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                       | 11 ++-
>   drivers/firmware/dmi_scan.c                        | 80 ++++++++++++++++++++++
>   include/linux/dmi.h                                |  1 +
>   4 files changed, 107 insertions(+), 7 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..f46158c
> --- /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, equal as 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 raw attributes and are accessible via:
> +
> +		/sys/firmware/dmi/tables/smbios_entry_point
> +		/sys/firmware/dmi/tables/DMI
> +
> +		The complete DMI information can be taken using these two
> +		tables.
> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
> index e0f1cb3..8e1a411 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 */
> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>   	int error = -ENOMEM;
>   	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 = -ENOSYS;
>   		goto err;
> +	}
>   
>   	dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>   	if (!dmi_kset)
> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>   err:
>   	cleanup_entry_list();
>   	kset_unregister(dmi_kset);
> -	kobject_put(dmi_kobj);
>   	return error;
>   }
>   
> @@ -685,8 +684,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 d3aae09..bb19f8b 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():
>    */
> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>   }
>   
>   static phys_addr_t dmi_base;
> +static u8 *dmi_table;
>   
>   static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>   		void *))
> @@ -476,6 +483,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) {
> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>   					dmi_ver >> 8, dmi_ver & 0xFF,
>   					(dmi_ver < 0x0300) ? "" : ".x");
>   			} else {
> +				smbios_entry_point_size = 15;
> +				memcpy(smbios_entry_point, buf,
> +				       smbios_entry_point_size);
>   				dmi_ver = (buf[14] & 0xF0) << 4 |
>   					   (buf[14] & 0x0F);
>   				pr_info("Legacy DMI %d.%d present.\n",
> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>   		dmi_ver &= 0xFFFFFF;
>   		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);
>   
>   		/*
>   		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
> @@ -638,6 +652,72 @@ 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);
> +struct bin_attribute bin_attr_dmi_table =
> +			__BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);

missed static here

> +
> +static int __init dmi_init(void)
> +{
> +	int ret = -ENOMEM;
> +	struct kobject *tables_kobj = NULL;
> +
> +	if (!dmi_available) {
> +		ret = -ENOSYS;
> +		goto err;
> +	}
> +
> +	/* Set up dmi directory at /sys/firmware/dmi */
> +	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> +	if (!dmi_kobj)
> +		goto err;
> +
> +	tables_kobj = kobject_create_and_add("tables", dmi_kobj);
> +	if (!tables_kobj)
> +		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)
> +		goto err;
> +
> +	bin_attr_dmi_table.size = dmi_len;
> +	bin_attr_dmi_table.private = dmi_table;
> +	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
> +	if (ret) {
> +		dmi_unmap(dmi_table);
> +		goto err;
> +	}
> +
> +	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);
> +	kobject_del(dmi_kobj);
> +	kobject_put(dmi_kobj);
> +	dmi_kobj = NULL;
> +
> +	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..9f55f46 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -93,6 +93,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);

-- 
Regards,
Ivan Khoronzhuk

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

[Roy Franz]

On Fri, Apr 3, 2015 at 2:36 AM, Ivan.khoronzhuk
<ivan.khoronzhuk@globallogic.com> wrote:
>
>
> On 02.04.15 15:57, 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.
>>

I have made modifications to dmidecode to support this interface, and it
works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
The only open issue I am aware of is how the SMBIOS v3 entry point
will be handled,
especially in cases where there is a v2 and a v3 entry point.
In principal I think this a good change - are there any other open issues?

Thanks,
Roy



>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
>> ---
>>   .../ABI/testing/sysfs-firmware-dmi-tables          | 22 ++++++
>>   drivers/firmware/dmi-sysfs.c                       | 11 ++-
>>   drivers/firmware/dmi_scan.c                        | 80
>> ++++++++++++++++++++++
>>   include/linux/dmi.h                                |  1 +
>>   4 files changed, 107 insertions(+), 7 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..f46158c
>> --- /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, equal as 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 raw attributes and are accessible via:
>> +
>> +               /sys/firmware/dmi/tables/smbios_entry_point
>> +               /sys/firmware/dmi/tables/DMI
>> +
>> +               The complete DMI information can be taken using these two
>> +               tables.
>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>> index e0f1cb3..8e1a411 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 */
>> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>>         int error = -ENOMEM;
>>         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 = -ENOSYS;
>>                 goto err;
>> +       }
>>         dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>>         if (!dmi_kset)
>> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>>   err:
>>         cleanup_entry_list();
>>         kset_unregister(dmi_kset);
>> -       kobject_put(dmi_kobj);
>>         return error;
>>   }
>>   @@ -685,8 +684,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 d3aae09..bb19f8b 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():
>>    */
>> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>>   }
>>     static phys_addr_t dmi_base;
>> +static u8 *dmi_table;
>>     static int __init dmi_walk_early(void (*decode)(const struct
>> dmi_header *,
>>                 void *))
>> @@ -476,6 +483,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) {
>> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>>                                         dmi_ver >> 8, dmi_ver & 0xFF,
>>                                         (dmi_ver < 0x0300) ? "" : ".x");
>>                         } else {
>> +                               smbios_entry_point_size = 15;
>> +                               memcpy(smbios_entry_point, buf,
>> +                                      smbios_entry_point_size);
>>                                 dmi_ver = (buf[14] & 0xF0) << 4 |
>>                                            (buf[14] & 0x0F);
>>                                 pr_info("Legacy DMI %d.%d present.\n",
>> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>>                 dmi_ver &= 0xFFFFFF;
>>                 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);
>>                 /*
>>                  * The 64-bit SMBIOS 3.0 entry point no longer has a field
>> @@ -638,6 +652,72 @@ 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);
>> +struct bin_attribute bin_attr_dmi_table =
>> +                       __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>
>
> missed static here
>
>
>> +
>> +static int __init dmi_init(void)
>> +{
>> +       int ret = -ENOMEM;
>> +       struct kobject *tables_kobj = NULL;
>> +
>> +       if (!dmi_available) {
>> +               ret = -ENOSYS;
>> +               goto err;
>> +       }
>> +
>> +       /* Set up dmi directory at /sys/firmware/dmi */
>> +       dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> +       if (!dmi_kobj)
>> +               goto err;
>> +
>> +       tables_kobj = kobject_create_and_add("tables", dmi_kobj);
>> +       if (!tables_kobj)
>> +               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)
>> +               goto err;
>> +
>> +       bin_attr_dmi_table.size = dmi_len;
>> +       bin_attr_dmi_table.private = dmi_table;
>> +       ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
>> +       if (ret) {
>> +               dmi_unmap(dmi_table);
>> +               goto err;
>> +       }
>> +
>> +       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);
>> +       kobject_del(dmi_kobj);
>> +       kobject_put(dmi_kobj);
>> +       dmi_kobj = NULL;
>> +
>> +       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..9f55f46 100644
>> --- a/include/linux/dmi.h
>> +++ b/include/linux/dmi.h
>> @@ -93,6 +93,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);
>
>
> --
> Regards,
> Ivan Khoronzhuk
>

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

[Roy Franz]

On Fri, Apr 3, 2015 at 2:36 AM, Ivan.khoronzhuk
<ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org> wrote:
>
>
> On 02.04.15 15:57, 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.
>>

I have made modifications to dmidecode to support this interface, and it
works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
The only open issue I am aware of is how the SMBIOS v3 entry point
will be handled,
especially in cases where there is a v2 and a v3 entry point.
In principal I think this a good change - are there any other open issues?

Thanks,
Roy



>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
>> ---
>>   .../ABI/testing/sysfs-firmware-dmi-tables          | 22 ++++++
>>   drivers/firmware/dmi-sysfs.c                       | 11 ++-
>>   drivers/firmware/dmi_scan.c                        | 80
>> ++++++++++++++++++++++
>>   include/linux/dmi.h                                |  1 +
>>   4 files changed, 107 insertions(+), 7 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..f46158c
>> --- /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-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
>> +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, equal as 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 raw attributes and are accessible via:
>> +
>> +               /sys/firmware/dmi/tables/smbios_entry_point
>> +               /sys/firmware/dmi/tables/DMI
>> +
>> +               The complete DMI information can be taken using these two
>> +               tables.
>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>> index e0f1cb3..8e1a411 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 */
>> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>>         int error = -ENOMEM;
>>         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 = -ENOSYS;
>>                 goto err;
>> +       }
>>         dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>>         if (!dmi_kset)
>> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>>   err:
>>         cleanup_entry_list();
>>         kset_unregister(dmi_kset);
>> -       kobject_put(dmi_kobj);
>>         return error;
>>   }
>>   @@ -685,8 +684,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 d3aae09..bb19f8b 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():
>>    */
>> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>>   }
>>     static phys_addr_t dmi_base;
>> +static u8 *dmi_table;
>>     static int __init dmi_walk_early(void (*decode)(const struct
>> dmi_header *,
>>                 void *))
>> @@ -476,6 +483,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) {
>> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>>                                         dmi_ver >> 8, dmi_ver & 0xFF,
>>                                         (dmi_ver < 0x0300) ? "" : ".x");
>>                         } else {
>> +                               smbios_entry_point_size = 15;
>> +                               memcpy(smbios_entry_point, buf,
>> +                                      smbios_entry_point_size);
>>                                 dmi_ver = (buf[14] & 0xF0) << 4 |
>>                                            (buf[14] & 0x0F);
>>                                 pr_info("Legacy DMI %d.%d present.\n",
>> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>>                 dmi_ver &= 0xFFFFFF;
>>                 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);
>>                 /*
>>                  * The 64-bit SMBIOS 3.0 entry point no longer has a field
>> @@ -638,6 +652,72 @@ 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);
>> +struct bin_attribute bin_attr_dmi_table =
>> +                       __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>
>
> missed static here
>
>
>> +
>> +static int __init dmi_init(void)
>> +{
>> +       int ret = -ENOMEM;
>> +       struct kobject *tables_kobj = NULL;
>> +
>> +       if (!dmi_available) {
>> +               ret = -ENOSYS;
>> +               goto err;
>> +       }
>> +
>> +       /* Set up dmi directory at /sys/firmware/dmi */
>> +       dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> +       if (!dmi_kobj)
>> +               goto err;
>> +
>> +       tables_kobj = kobject_create_and_add("tables", dmi_kobj);
>> +       if (!tables_kobj)
>> +               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)
>> +               goto err;
>> +
>> +       bin_attr_dmi_table.size = dmi_len;
>> +       bin_attr_dmi_table.private = dmi_table;
>> +       ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
>> +       if (ret) {
>> +               dmi_unmap(dmi_table);
>> +               goto err;
>> +       }
>> +
>> +       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);
>> +       kobject_del(dmi_kobj);
>> +       kobject_put(dmi_kobj);
>> +       dmi_kobj = NULL;
>> +
>> +       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..9f55f46 100644
>> --- a/include/linux/dmi.h
>> +++ b/include/linux/dmi.h
>> @@ -93,6 +93,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);
>
>
> --
> Regards,
> Ivan Khoronzhuk
>

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

[Jean Delvare]

On Thu,  2 Apr 2015 15:57:01 +0300, Ivan Khoronzhuk wrote:
> 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.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
> ---
>  drivers/firmware/dmi_scan.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> (...)

Yes, as discussed before, good idea.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

Hi Ivan,

On Thu,  2 Apr 2015 15:57:03 +0300, Ivan Khoronzhuk wrote:
> 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.
> 
> 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:

Thanks for doing this.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

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

[Matt Fleming]

On Thu, 02 Apr, at 03:57:01PM, Ivan Khoronzhuk wrote:
> 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.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
> ---
>  drivers/firmware/dmi_scan.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Looks good to me.

Jean, do you want me to pick this patch up or are you going to?

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Matt Fleming]

On Thu, 02 Apr, at 03:57:01PM, Ivan Khoronzhuk wrote:
> 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.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
> ---
>  drivers/firmware/dmi_scan.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Looks good to me.

Jean, do you want me to pick this patch up or are you going to?

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Roy Franz]

On Tue, Apr 14, 2015 at 9:19 PM, Roy Franz <roy.franz@linaro.org> wrote:
> On Fri, Apr 3, 2015 at 2:36 AM, Ivan.khoronzhuk
> <ivan.khoronzhuk@globallogic.com> wrote:
>>
>>
>> On 02.04.15 15:57, 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.
>>>
>
> I have made modifications to dmidecode to support this interface, and it
> works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
> The only open issue I am aware of is how the SMBIOS v3 entry point
> will be handled,
> especially in cases where there is a v2 and a v3 entry point.
> In principal I think this a good change - are there any other open issues?
>
> Thanks,
> Roy
>

I looked at the SMBIOS spec again, and the platform can provide either or
both of the 32-bit and 64-bit entry points.  The spec says that an
implementation
"should" provide a 32-bit entry point for compatibility.

These 2 entry point structures can both point to the same SMBIOS
structure table,
or two distinct ones.  If distinct, the one referenced by the 32-bit
entry point must be
a consistent subset of the 64-bit one.
There does not seem to be any prohibition from using new SMBIOS v3 structures
in a table referenced by a 32-bit entry point, so backwards
compatibility is completely
up to the implementation.

Since the point of this patchset (and related changes to dmidecode) is
to provide the
SMBIOS information without using /dev/mem, I think the interface we
define should
support all the cases outlined in the specification.  I would like to
avoid a case where
we're back to using /dev/mem to deal with the dual entry/dual table
case if that becomes
important down the road.

Here's a proposal for files in /sys/firmware/dmi/tables:

smbios_entry_point32   -  32 bit (existing entry point type), if it exists.
smbios_entry_point64   -  64 bit entry, new in SMBIOS v3.0
DMI32                         -  smbios structure tables referenced by
32 bit entry point, if it exists
DMI64                         -  smbios structure tables referenced by
64 bit entry point, if it exists.
                                     symlink to DMI32 if identical
smbios_entry_point    - symlink to smbios_entry_point64 if it exists,
otherwise symlink to smbios_entry_point64
DMI                           - symlink to DMI64 if it exists,
otherwise symlink to DMI32

These last two would provide names to the 'preferred' tables, and
names that would always exist on all systems, which
I think is a nice property to have.

Thoughts?

Thanks,
Roy



>
>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
>>> ---
>>>   .../ABI/testing/sysfs-firmware-dmi-tables          | 22 ++++++
>>>   drivers/firmware/dmi-sysfs.c                       | 11 ++-
>>>   drivers/firmware/dmi_scan.c                        | 80
>>> ++++++++++++++++++++++
>>>   include/linux/dmi.h                                |  1 +
>>>   4 files changed, 107 insertions(+), 7 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..f46158c
>>> --- /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, equal as 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 raw attributes and are accessible via:
>>> +
>>> +               /sys/firmware/dmi/tables/smbios_entry_point
>>> +               /sys/firmware/dmi/tables/DMI
>>> +
>>> +               The complete DMI information can be taken using these two
>>> +               tables.
>>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>>> index e0f1cb3..8e1a411 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 */
>>> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>>>         int error = -ENOMEM;
>>>         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 = -ENOSYS;
>>>                 goto err;
>>> +       }
>>>         dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>>>         if (!dmi_kset)
>>> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>>>   err:
>>>         cleanup_entry_list();
>>>         kset_unregister(dmi_kset);
>>> -       kobject_put(dmi_kobj);
>>>         return error;
>>>   }
>>>   @@ -685,8 +684,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 d3aae09..bb19f8b 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():
>>>    */
>>> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>>>   }
>>>     static phys_addr_t dmi_base;
>>> +static u8 *dmi_table;
>>>     static int __init dmi_walk_early(void (*decode)(const struct
>>> dmi_header *,
>>>                 void *))
>>> @@ -476,6 +483,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) {
>>> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>>>                                         dmi_ver >> 8, dmi_ver & 0xFF,
>>>                                         (dmi_ver < 0x0300) ? "" : ".x");
>>>                         } else {
>>> +                               smbios_entry_point_size = 15;
>>> +                               memcpy(smbios_entry_point, buf,
>>> +                                      smbios_entry_point_size);
>>>                                 dmi_ver = (buf[14] & 0xF0) << 4 |
>>>                                            (buf[14] & 0x0F);
>>>                                 pr_info("Legacy DMI %d.%d present.\n",
>>> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>>>                 dmi_ver &= 0xFFFFFF;
>>>                 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);
>>>                 /*
>>>                  * The 64-bit SMBIOS 3.0 entry point no longer has a field
>>> @@ -638,6 +652,72 @@ 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);
>>> +struct bin_attribute bin_attr_dmi_table =
>>> +                       __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>>
>>
>> missed static here
>>
>>
>>> +
>>> +static int __init dmi_init(void)
>>> +{
>>> +       int ret = -ENOMEM;
>>> +       struct kobject *tables_kobj = NULL;
>>> +
>>> +       if (!dmi_available) {
>>> +               ret = -ENOSYS;
>>> +               goto err;
>>> +       }
>>> +
>>> +       /* Set up dmi directory at /sys/firmware/dmi */
>>> +       dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>>> +       if (!dmi_kobj)
>>> +               goto err;
>>> +
>>> +       tables_kobj = kobject_create_and_add("tables", dmi_kobj);
>>> +       if (!tables_kobj)
>>> +               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)
>>> +               goto err;
>>> +
>>> +       bin_attr_dmi_table.size = dmi_len;
>>> +       bin_attr_dmi_table.private = dmi_table;
>>> +       ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
>>> +       if (ret) {
>>> +               dmi_unmap(dmi_table);
>>> +               goto err;
>>> +       }
>>> +
>>> +       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);
>>> +       kobject_del(dmi_kobj);
>>> +       kobject_put(dmi_kobj);
>>> +       dmi_kobj = NULL;
>>> +
>>> +       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..9f55f46 100644
>>> --- a/include/linux/dmi.h
>>> +++ b/include/linux/dmi.h
>>> @@ -93,6 +93,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);
>>
>>
>> --
>> Regards,
>> Ivan Khoronzhuk
>>

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

[Roy Franz]

On Tue, Apr 14, 2015 at 9:19 PM, Roy Franz <roy.franz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Apr 3, 2015 at 2:36 AM, Ivan.khoronzhuk
> <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org> wrote:
>>
>>
>> On 02.04.15 15:57, 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.
>>>
>
> I have made modifications to dmidecode to support this interface, and it
> works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
> The only open issue I am aware of is how the SMBIOS v3 entry point
> will be handled,
> especially in cases where there is a v2 and a v3 entry point.
> In principal I think this a good change - are there any other open issues?
>
> Thanks,
> Roy
>

I looked at the SMBIOS spec again, and the platform can provide either or
both of the 32-bit and 64-bit entry points.  The spec says that an
implementation
"should" provide a 32-bit entry point for compatibility.

These 2 entry point structures can both point to the same SMBIOS
structure table,
or two distinct ones.  If distinct, the one referenced by the 32-bit
entry point must be
a consistent subset of the 64-bit one.
There does not seem to be any prohibition from using new SMBIOS v3 structures
in a table referenced by a 32-bit entry point, so backwards
compatibility is completely
up to the implementation.

Since the point of this patchset (and related changes to dmidecode) is
to provide the
SMBIOS information without using /dev/mem, I think the interface we
define should
support all the cases outlined in the specification.  I would like to
avoid a case where
we're back to using /dev/mem to deal with the dual entry/dual table
case if that becomes
important down the road.

Here's a proposal for files in /sys/firmware/dmi/tables:

smbios_entry_point32   -  32 bit (existing entry point type), if it exists.
smbios_entry_point64   -  64 bit entry, new in SMBIOS v3.0
DMI32                         -  smbios structure tables referenced by
32 bit entry point, if it exists
DMI64                         -  smbios structure tables referenced by
64 bit entry point, if it exists.
                                     symlink to DMI32 if identical
smbios_entry_point    - symlink to smbios_entry_point64 if it exists,
otherwise symlink to smbios_entry_point64
DMI                           - symlink to DMI64 if it exists,
otherwise symlink to DMI32

These last two would provide names to the 'preferred' tables, and
names that would always exist on all systems, which
I think is a nice property to have.

Thoughts?

Thanks,
Roy



>
>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
>>> ---
>>>   .../ABI/testing/sysfs-firmware-dmi-tables          | 22 ++++++
>>>   drivers/firmware/dmi-sysfs.c                       | 11 ++-
>>>   drivers/firmware/dmi_scan.c                        | 80
>>> ++++++++++++++++++++++
>>>   include/linux/dmi.h                                |  1 +
>>>   4 files changed, 107 insertions(+), 7 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..f46158c
>>> --- /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-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
>>> +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, equal as 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 raw attributes and are accessible via:
>>> +
>>> +               /sys/firmware/dmi/tables/smbios_entry_point
>>> +               /sys/firmware/dmi/tables/DMI
>>> +
>>> +               The complete DMI information can be taken using these two
>>> +               tables.
>>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>>> index e0f1cb3..8e1a411 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 */
>>> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>>>         int error = -ENOMEM;
>>>         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 = -ENOSYS;
>>>                 goto err;
>>> +       }
>>>         dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>>>         if (!dmi_kset)
>>> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>>>   err:
>>>         cleanup_entry_list();
>>>         kset_unregister(dmi_kset);
>>> -       kobject_put(dmi_kobj);
>>>         return error;
>>>   }
>>>   @@ -685,8 +684,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 d3aae09..bb19f8b 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():
>>>    */
>>> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>>>   }
>>>     static phys_addr_t dmi_base;
>>> +static u8 *dmi_table;
>>>     static int __init dmi_walk_early(void (*decode)(const struct
>>> dmi_header *,
>>>                 void *))
>>> @@ -476,6 +483,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) {
>>> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>>>                                         dmi_ver >> 8, dmi_ver & 0xFF,
>>>                                         (dmi_ver < 0x0300) ? "" : ".x");
>>>                         } else {
>>> +                               smbios_entry_point_size = 15;
>>> +                               memcpy(smbios_entry_point, buf,
>>> +                                      smbios_entry_point_size);
>>>                                 dmi_ver = (buf[14] & 0xF0) << 4 |
>>>                                            (buf[14] & 0x0F);
>>>                                 pr_info("Legacy DMI %d.%d present.\n",
>>> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>>>                 dmi_ver &= 0xFFFFFF;
>>>                 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);
>>>                 /*
>>>                  * The 64-bit SMBIOS 3.0 entry point no longer has a field
>>> @@ -638,6 +652,72 @@ 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);
>>> +struct bin_attribute bin_attr_dmi_table =
>>> +                       __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>>
>>
>> missed static here
>>
>>
>>> +
>>> +static int __init dmi_init(void)
>>> +{
>>> +       int ret = -ENOMEM;
>>> +       struct kobject *tables_kobj = NULL;
>>> +
>>> +       if (!dmi_available) {
>>> +               ret = -ENOSYS;
>>> +               goto err;
>>> +       }
>>> +
>>> +       /* Set up dmi directory at /sys/firmware/dmi */
>>> +       dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>>> +       if (!dmi_kobj)
>>> +               goto err;
>>> +
>>> +       tables_kobj = kobject_create_and_add("tables", dmi_kobj);
>>> +       if (!tables_kobj)
>>> +               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)
>>> +               goto err;
>>> +
>>> +       bin_attr_dmi_table.size = dmi_len;
>>> +       bin_attr_dmi_table.private = dmi_table;
>>> +       ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
>>> +       if (ret) {
>>> +               dmi_unmap(dmi_table);
>>> +               goto err;
>>> +       }
>>> +
>>> +       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);
>>> +       kobject_del(dmi_kobj);
>>> +       kobject_put(dmi_kobj);
>>> +       dmi_kobj = NULL;
>>> +
>>> +       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..9f55f46 100644
>>> --- a/include/linux/dmi.h
>>> +++ b/include/linux/dmi.h
>>> @@ -93,6 +93,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);
>>
>>
>> --
>> Regards,
>> Ivan Khoronzhuk
>>

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

[Jean Delvare]

Hi Roy,

On Wed, 15 Apr 2015 17:54:51 -0700, Roy Franz wrote:
> On Tue, Apr 14, 2015 at 9:19 PM, Roy Franz <roy.franz@linaro.org> wrote:
> > I have made modifications to dmidecode to support this interface, and it
> > works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
> > The only open issue I am aware of is how the SMBIOS v3 entry point
> > will be handled,
> > especially in cases where there is a v2 and a v3 entry point.
> > In principal I think this a good change - are there any other open issues?
> 
> I looked at the SMBIOS spec again, and the platform can provide either or
> both of the 32-bit and 64-bit entry points.  The spec says that an
> implementation
> "should" provide a 32-bit entry point for compatibility.
> 
> These 2 entry point structures can both point to the same SMBIOS
> structure table,
> or two distinct ones.  If distinct, the one referenced by the 32-bit
> entry point must be
> a consistent subset of the 64-bit one.
> There does not seem to be any prohibition from using new SMBIOS v3 structures
> in a table referenced by a 32-bit entry point, so backwards
> compatibility is completely
> up to the implementation.

What do you mean with "new SMBIOS v3 structures"? I took a brief look
at the change log at the end of the SMBIOS 3.0.0 specification, and all
I can see are new enumerated values for existing fields, as well as 3
new fields in the type 4 structure. I can't see any new structure type.
Either way this is all backwards compatible, which is why both entry
points can point to the same table.

> Since the point of this patchset (and related changes to dmidecode) is
> to provide the
> SMBIOS information without using /dev/mem, I think the interface we
> define should
> support all the cases outlined in the specification.  I would like to
> avoid a case where
> we're back to using /dev/mem to deal with the dual entry/dual table
> case if that becomes
> important down the road.
> 
> Here's a proposal for files in /sys/firmware/dmi/tables:
> 
> smbios_entry_point32   -  32 bit (existing entry point type), if it exists.
> smbios_entry_point64   -  64 bit entry, new in SMBIOS v3.0
> DMI32                         -  smbios structure tables referenced by
> 32 bit entry point, if it exists
> DMI64                         -  smbios structure tables referenced by
> 64 bit entry point, if it exists.
>                                      symlink to DMI32 if identical
> smbios_entry_point    - symlink to smbios_entry_point64 if it exists,
> otherwise symlink to smbios_entry_point64
> DMI                           - symlink to DMI64 if it exists,
> otherwise symlink to DMI32
> 
> These last two would provide names to the 'preferred' tables, and
> names that would always exist on all systems, which
> I think is a nice property to have.
> 
> Thoughts?

The idea was discussed before, and discarded. The question is, what use
cases do you envision from a user-space application perspective? As you
found out, the table pointed to by the _SM3_ entry point must be a
super-set of the one pointed to by the _SM_ entry point, so for a
compliant implementation there is no reason to follow the _SM_ entry
point when both exist. And if only one entry point exists, there is
nothing to choose from.

The kernel itself will have to choose one of the entry points when it
comes to dmi_scan and dmi-id. It can't provide two sets of DMI strings.
So it seems reasonable to only expose though sysfs that one entry point
and table that the kernel itself used.

The only case where it would make some sense to expose everything to
user-space is for BIOS debugging purpose: if both entry points exist
and point to different tables, and if the _SM3_ table is broken and
the _SM_ table is correct, then you may want to be able to temporarily
read the _SM_ table instead of the _SM3_ table, to check what needs to
be fixed in the latter. But I would argue that this is beyond the scope
of our code: neither the kernel nor dmidecode are DMI table or BIOS
authoring tools.

If there really ever is a need to ignore the _SM3_ entry point, I'd
rather have a kernel boot parameter for this. This would guarantee that
the kernel and user-space applications always operate on the same data.

So I would keep the sysfs interface simple, as Ivan implemented it.

-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

Hi Roy,

On Wed, 15 Apr 2015 17:54:51 -0700, Roy Franz wrote:
> On Tue, Apr 14, 2015 at 9:19 PM, Roy Franz <roy.franz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > I have made modifications to dmidecode to support this interface, and it
> > works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
> > The only open issue I am aware of is how the SMBIOS v3 entry point
> > will be handled,
> > especially in cases where there is a v2 and a v3 entry point.
> > In principal I think this a good change - are there any other open issues?
> 
> I looked at the SMBIOS spec again, and the platform can provide either or
> both of the 32-bit and 64-bit entry points.  The spec says that an
> implementation
> "should" provide a 32-bit entry point for compatibility.
> 
> These 2 entry point structures can both point to the same SMBIOS
> structure table,
> or two distinct ones.  If distinct, the one referenced by the 32-bit
> entry point must be
> a consistent subset of the 64-bit one.
> There does not seem to be any prohibition from using new SMBIOS v3 structures
> in a table referenced by a 32-bit entry point, so backwards
> compatibility is completely
> up to the implementation.

What do you mean with "new SMBIOS v3 structures"? I took a brief look
at the change log at the end of the SMBIOS 3.0.0 specification, and all
I can see are new enumerated values for existing fields, as well as 3
new fields in the type 4 structure. I can't see any new structure type.
Either way this is all backwards compatible, which is why both entry
points can point to the same table.

> Since the point of this patchset (and related changes to dmidecode) is
> to provide the
> SMBIOS information without using /dev/mem, I think the interface we
> define should
> support all the cases outlined in the specification.  I would like to
> avoid a case where
> we're back to using /dev/mem to deal with the dual entry/dual table
> case if that becomes
> important down the road.
> 
> Here's a proposal for files in /sys/firmware/dmi/tables:
> 
> smbios_entry_point32   -  32 bit (existing entry point type), if it exists.
> smbios_entry_point64   -  64 bit entry, new in SMBIOS v3.0
> DMI32                         -  smbios structure tables referenced by
> 32 bit entry point, if it exists
> DMI64                         -  smbios structure tables referenced by
> 64 bit entry point, if it exists.
>                                      symlink to DMI32 if identical
> smbios_entry_point    - symlink to smbios_entry_point64 if it exists,
> otherwise symlink to smbios_entry_point64
> DMI                           - symlink to DMI64 if it exists,
> otherwise symlink to DMI32
> 
> These last two would provide names to the 'preferred' tables, and
> names that would always exist on all systems, which
> I think is a nice property to have.
> 
> Thoughts?

The idea was discussed before, and discarded. The question is, what use
cases do you envision from a user-space application perspective? As you
found out, the table pointed to by the _SM3_ entry point must be a
super-set of the one pointed to by the _SM_ entry point, so for a
compliant implementation there is no reason to follow the _SM_ entry
point when both exist. And if only one entry point exists, there is
nothing to choose from.

The kernel itself will have to choose one of the entry points when it
comes to dmi_scan and dmi-id. It can't provide two sets of DMI strings.
So it seems reasonable to only expose though sysfs that one entry point
and table that the kernel itself used.

The only case where it would make some sense to expose everything to
user-space is for BIOS debugging purpose: if both entry points exist
and point to different tables, and if the _SM3_ table is broken and
the _SM_ table is correct, then you may want to be able to temporarily
read the _SM_ table instead of the _SM3_ table, to check what needs to
be fixed in the latter. But I would argue that this is beyond the scope
of our code: neither the kernel nor dmidecode are DMI table or BIOS
authoring tools.

If there really ever is a need to ignore the _SM3_ entry point, I'd
rather have a kernel boot parameter for this. This would guarantee that
the kernel and user-space applications always operate on the same data.

So I would keep the sysfs interface simple, as Ivan implemented it.

-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

Hi Matt,

On Wed, 15 Apr 2015 15:35:30 +0100, Matt Fleming wrote:
> On Thu, 02 Apr, at 03:57:01PM, Ivan Khoronzhuk wrote:
> > 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.
> > 
> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
> > ---
> >  drivers/firmware/dmi_scan.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Looks good to me.
> 
> Jean, do you want me to pick this patch up or are you going to?

Good question, we need to agree on a strategy.

There are 3 patch sets to consider here.

1* My patch fixing the UUID ordering bug. It must go in first and
   immediately, as it fixes a regression and will have to be backported
   to stable branches.

2* 2 older patches from Ivan which are currently in your efi-next tree
   if I'm not mistaken. Both were based on an old tree so they do not
   apply cleanly on kernel v4.0, I had to fix them up manually. I have
   no idea if git would be able to merge them properly, as the fix
   above changed the context even more.

3* The 3 new patches from Ivan which I am reviewing now, which are not
   applied in any public tree AFAIK.

I don't really care who picks these patches up and sends them to Linus,
but I think they should all follow the same route so that Linus has as
little merge work to do as possible. So either you pick them all, or I
do. If I do, you'll have to drop the 2 patches you have in efi-next.
Again I'm fine either way, so please let me know what makes your life
easier and let's do that.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

Hi Matt,

On Wed, 15 Apr 2015 15:35:30 +0100, Matt Fleming wrote:
> On Thu, 02 Apr, at 03:57:01PM, Ivan Khoronzhuk wrote:
> > 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.
> > 
> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
> > ---
> >  drivers/firmware/dmi_scan.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Looks good to me.
> 
> Jean, do you want me to pick this patch up or are you going to?

Good question, we need to agree on a strategy.

There are 3 patch sets to consider here.

1* My patch fixing the UUID ordering bug. It must go in first and
   immediately, as it fixes a regression and will have to be backported
   to stable branches.

2* 2 older patches from Ivan which are currently in your efi-next tree
   if I'm not mistaken. Both were based on an old tree so they do not
   apply cleanly on kernel v4.0, I had to fix them up manually. I have
   no idea if git would be able to merge them properly, as the fix
   above changed the context even more.

3* The 3 new patches from Ivan which I am reviewing now, which are not
   applied in any public tree AFAIK.

I don't really care who picks these patches up and sends them to Linus,
but I think they should all follow the same route so that Linus has as
little merge work to do as possible. So either you pick them all, or I
do. If I do, you'll have to drop the 2 patches you have in efi-next.
Again I'm fine either way, so please let me know what makes your life
easier and let's do that.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

Hi Ivan,

On Thu,  2 Apr 2015 15:57:02 +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                       | 11 ++-
>  drivers/firmware/dmi_scan.c                        | 80 ++++++++++++++++++++++
>  include/linux/dmi.h                                |  1 +
>  4 files changed, 107 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables

First of all: thanks for doing this, it looks mostly good now, and I've
tested it successfully on my own system.

> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
> new file mode 100644
> index 0000000..f46158c
> --- /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, equal as DMI structures
> +		can be read in SMBIOS specification.

"equal as" sounds strange, I think a simple "and" would be better.

> +
> +		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 raw attributes and are accessible via:

"binary attributes" rather than "raw attributes"? The "raw" nature is
already mentioned earlier in the sentence.

> +
> +		/sys/firmware/dmi/tables/smbios_entry_point
> +		/sys/firmware/dmi/tables/DMI
> +
> +		The complete DMI information can be taken using these two

"obtained" or "decoded" would sound better than "taken" IMHO.

> +		tables.
> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
> index e0f1cb3..8e1a411 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 */
> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>  	int error = -ENOMEM;

This initialization can be moved to the single error path left that
needs it. (I can do it myself in a separate patch if you don't want to
do it here.)

>  	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 = -ENOSYS;
>  		goto err;
> +	}
>  
>  	dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>  	if (!dmi_kset)
> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>  err:
>  	cleanup_entry_list();
>  	kset_unregister(dmi_kset);
> -	kobject_put(dmi_kobj);
>  	return error;
>  }
>  
> @@ -685,8 +684,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 d3aae09..bb19f8b 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():
>   */
> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>  }
>  
>  static phys_addr_t dmi_base;
> +static u8 *dmi_table;

This variable is only ever used in a single function (dmi_init). Does
it actually need to be a global variable? I suspect not.

>  
>  static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>  		void *))
> @@ -476,6 +483,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) {
> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>  					dmi_ver >> 8, dmi_ver & 0xFF,
>  					(dmi_ver < 0x0300) ? "" : ".x");
>  			} else {
> +				smbios_entry_point_size = 15;
> +				memcpy(smbios_entry_point, buf,
> +				       smbios_entry_point_size);
>  				dmi_ver = (buf[14] & 0xF0) << 4 |
>  					   (buf[14] & 0x0F);
>  				pr_info("Legacy DMI %d.%d present.\n",
> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>  		dmi_ver &= 0xFFFFFF;
>  		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);
>  
>  		/*
>  		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
> @@ -638,6 +652,72 @@ 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);

This one could be world-readable as it contains no sensitive
information.

> +struct bin_attribute bin_attr_dmi_table =
> +			__BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);

I do not understand why you don't use BIN_ATTR here too? I tried naming
the attribute bin_attr_DMI and it seems to work just fine, checkpatch
doesn't even complain!

> +
> +static int __init dmi_init(void)
> +{
> +	int ret = -ENOMEM;
> +	struct kobject *tables_kobj = NULL;
> +
> +	if (!dmi_available) {
> +		ret = -ENOSYS;
> +		goto err;
> +	}

This is weird. Can this actually happen?

We currently have two ways to enter this module: dmi_scan_machine(),
which is called by the architecture code, and dmi_init(), which is
called at subsys_initcall time. As far as I can see,
core/arch_initcalls are guaranteed to be always called before
subsys_initcalls. If we can rely on that, the test above is not needed.
If for any reason we can't, that means that dmi_init() should not be a
subsys_initcall, but should instead be called explicitly at the end of
dmi_scan_machine().

> +
> +	/* Set up dmi directory at /sys/firmware/dmi */
> +	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> +	if (!dmi_kobj)
> +		goto err;
> +
> +	tables_kobj = kobject_create_and_add("tables", dmi_kobj);
> +	if (!tables_kobj)
> +		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)
> +		goto err;

At this point "ret" has value 0 so you will take the error path but
return with success. No good. I suggest that you move the call to
dmi_remap before the first call to sysfs_create_bin_file above, so that
"ret" still has value -ENOMEM.

> +
> +	bin_attr_dmi_table.size = dmi_len;
> +	bin_attr_dmi_table.private = dmi_table;
> +	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
> +	if (ret) {
> +		dmi_unmap(dmi_table);

Doing this here goes against the logic of having a single error path.
Instead you should have an additional error label before err: and jump
there.

> +		goto err;
> +	}
> +
> +	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);

These last two calls could be moved inside the conditional above. Even
better would be a separate error label so that you know exactly what
needs to be undone.

> +	kobject_del(dmi_kobj);
> +	kobject_put(dmi_kobj);
> +	dmi_kobj = NULL;

I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
appropriate comment), so that dmi-sysfs has a chance to load? As it is
now, a bug or some unexpected behavior in this new code could cause a
regression for dmi-sysfs users. Just because I don't like dmi_sysfs
doesn't mean we can break it ;-)

> +
> +	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..9f55f46 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -93,6 +93,7 @@ struct dmi_dev_onboard {
>  	int devfn;
>  };
>  
> +extern struct kobject *dmi_kobj;

struct kobject is defined in <linux/kobject.h> so I think you should
include that file to avoid random build failures in the future.

>  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);


-- 
Jean Delvare
SUSE L3 Support

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

[Ivan.khoronzhuk]

Hi Jean,

On 16.04.15 12:52, Jean Delvare wrote:
> Hi Ivan,
>
> On Thu,  2 Apr 2015 15:57:02 +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                       | 11 ++-
>>   drivers/firmware/dmi_scan.c                        | 80 ++++++++++++++++++++++
>>   include/linux/dmi.h                                |  1 +
>>   4 files changed, 107 insertions(+), 7 deletions(-)
>>   create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
> First of all: thanks for doing this, it looks mostly good now, and I've
> tested it successfully on my own system.
>
>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> new file mode 100644
>> index 0000000..f46158c
>> --- /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, equal as DMI structures
>> +		can be read in SMBIOS specification.
> "equal as" sounds strange, I think a simple "and" would be better.

Ok

>
>> +
>> +		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 raw attributes and are accessible via:
> "binary attributes" rather than "raw attributes"? The "raw" nature is
> already mentioned earlier in the sentence.

Ok

>
>> +
>> +		/sys/firmware/dmi/tables/smbios_entry_point
>> +		/sys/firmware/dmi/tables/DMI
>> +
>> +		The complete DMI information can be taken using these two
> "obtained" or "decoded" would sound better than "taken" IMHO.

Ok

>
>> +		tables.
>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>> index e0f1cb3..8e1a411 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 */
>> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>>   	int error = -ENOMEM;
> This initialization can be moved to the single error path left that
> needs it. (I can do it myself in a separate patch if you don't want to
> do it here.)

Let it be in this patch.

>>   	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 = -ENOSYS;
>>   		goto err;
>> +	}
>>   
>>   	dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>>   	if (!dmi_kset)
>> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>>   err:
>>   	cleanup_entry_list();
>>   	kset_unregister(dmi_kset);
>> -	kobject_put(dmi_kobj);
>>   	return error;
>>   }
>>   
>> @@ -685,8 +684,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 d3aae09..bb19f8b 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():
>>    */
>> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>>   }
>>   
>>   static phys_addr_t dmi_base;
>> +static u8 *dmi_table;
> This variable is only ever used in a single function (dmi_init). Does
> it actually need to be a global variable? I suspect not.

, Yes.

>
>>   
>>   static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>>   		void *))
>> @@ -476,6 +483,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) {
>> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>>   					dmi_ver >> 8, dmi_ver & 0xFF,
>>   					(dmi_ver < 0x0300) ? "" : ".x");
>>   			} else {
>> +				smbios_entry_point_size = 15;
>> +				memcpy(smbios_entry_point, buf,
>> +				       smbios_entry_point_size);
>>   				dmi_ver = (buf[14] & 0xF0) << 4 |
>>   					   (buf[14] & 0x0F);
>>   				pr_info("Legacy DMI %d.%d present.\n",
>> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>>   		dmi_ver &= 0xFFFFFF;
>>   		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);
>>   
>>   		/*
>>   		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
>> @@ -638,6 +652,72 @@ 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);
> This one could be world-readable as it contains no sensitive
> information.

It contains the address of DMI table containing sensitive information.
Who knows which ways can be used to take it. Anyway, no see reasons in this
w/o DMI table. But if you insist I can do it "world-readable".

>
>> +struct bin_attribute bin_attr_dmi_table =
>> +			__BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
> I do not understand why you don't use BIN_ATTR here too? I tried naming
> the attribute bin_attr_DMI and it seems to work just fine, checkpatch
> doesn't even complain!

I dislike upper case in names, at least in such simple names.
It makes me using bin_attr_DMI lower in the code. That's why.
But if you like it, I will name it "bin_attr_DMI"

>
>> +
>> +static int __init dmi_init(void)
>> +{
>> +	int ret = -ENOMEM;
>> +	struct kobject *tables_kobj = NULL;
>> +
>> +	if (!dmi_available) {
>> +		ret = -ENOSYS;
>> +		goto err;
>> +	}
> This is weird. Can this actually happen?
>
> We currently have two ways to enter this module: dmi_scan_machine(),
> which is called by the architecture code, and dmi_init(), which is
> called at subsys_initcall time. As far as I can see,
> core/arch_initcalls are guaranteed to be always called before
> subsys_initcalls. If we can rely on that, the test above is not needed.
> If for any reason we can't, that means that dmi_init() should not be a
> subsys_initcall, but should instead be called explicitly at the end of
> dmi_scan_machine().

We cannot be sure that firmware_kobj created at time of dmi_init().
The sources don't oblige you to call it at core level,
for instance like it was done for arm64. For x86, dmi_init() can be called
before firmware_kobj is created. And if I call it from dmi_init() I suppose
I would face an error. As I can't call it in dmi_init I can't be sure that
DMI is available at all. So, no, we have to check dmi_available here and 
call
it at subsys layer, where it's supposed to be.

>> +
>> +	/* Set up dmi directory at /sys/firmware/dmi */
>> +	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> +	if (!dmi_kobj)
>> +		goto err;
>> +
>> +	tables_kobj = kobject_create_and_add("tables", dmi_kobj);
>> +	if (!tables_kobj)
>> +		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)
>> +		goto err;
> At this point "ret" has value 0 so you will take the error path but
> return with success. No good. I suggest that you move the call to
> dmi_remap before the first call to sysfs_create_bin_file above, so that
> "ret" still has value -ENOMEM.

Yes.

>
>> +
>> +	bin_attr_dmi_table.size = dmi_len;
>> +	bin_attr_dmi_table.private = dmi_table;
>> +	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
>> +	if (ret) {
>> +		dmi_unmap(dmi_table);
> Doing this here goes against the logic of having a single error path.
> Instead you should have an additional error label before err: and jump
> there.

I will redo this.

>
>> +		goto err;
>> +	}
>> +
>> +	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);
> These last two calls could be moved inside the conditional above. Even
> better would be a separate error label so that you know exactly what
> needs to be undone.

Let it be.

>
>> +	kobject_del(dmi_kobj);
>> +	kobject_put(dmi_kobj);
>> +	dmi_kobj = NULL;
> I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
> appropriate comment), so that dmi-sysfs has a chance to load? As it is
> now, a bug or some unexpected behavior in this new code could cause a
> regression for dmi-sysfs users. Just because I don't like dmi_sysfs
> doesn't mean we can break it ;-)

As I remember it was not so critical for you last time.
"I don't care which way you choose". And I've explained my position.
But it's not very hard to me to change it. Anyway patch requires re-push.

>
>> +
>> +	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..9f55f46 100644
>> --- a/include/linux/dmi.h
>> +++ b/include/linux/dmi.h
>> @@ -93,6 +93,7 @@ struct dmi_dev_onboard {
>>   	int devfn;
>>   };
>>   
>> +extern struct kobject *dmi_kobj;
> struct kobject is defined in <linux/kobject.h> so I think you should
> include that file to avoid random build failures in the future.

Ok

>
>>   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);
>

Thanks Jean.
I will add you propositions and re-push the series.
If you are OK with my answer of-course.

-- 
Regards,
Ivan Khoronzhuk

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

[Ivan.khoronzhuk]

Hi Jean,

On 16.04.15 12:52, Jean Delvare wrote:
> Hi Ivan,
>
> On Thu,  2 Apr 2015 15:57:02 +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-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
>> ---
>>   .../ABI/testing/sysfs-firmware-dmi-tables          | 22 ++++++
>>   drivers/firmware/dmi-sysfs.c                       | 11 ++-
>>   drivers/firmware/dmi_scan.c                        | 80 ++++++++++++++++++++++
>>   include/linux/dmi.h                                |  1 +
>>   4 files changed, 107 insertions(+), 7 deletions(-)
>>   create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
> First of all: thanks for doing this, it looks mostly good now, and I've
> tested it successfully on my own system.
>
>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> new file mode 100644
>> index 0000000..f46158c
>> --- /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-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
>> +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, equal as DMI structures
>> +		can be read in SMBIOS specification.
> "equal as" sounds strange, I think a simple "and" would be better.

Ok

>
>> +
>> +		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 raw attributes and are accessible via:
> "binary attributes" rather than "raw attributes"? The "raw" nature is
> already mentioned earlier in the sentence.

Ok

>
>> +
>> +		/sys/firmware/dmi/tables/smbios_entry_point
>> +		/sys/firmware/dmi/tables/DMI
>> +
>> +		The complete DMI information can be taken using these two
> "obtained" or "decoded" would sound better than "taken" IMHO.

Ok

>
>> +		tables.
>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>> index e0f1cb3..8e1a411 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 */
>> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>>   	int error = -ENOMEM;
> This initialization can be moved to the single error path left that
> needs it. (I can do it myself in a separate patch if you don't want to
> do it here.)

Let it be in this patch.

>>   	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 = -ENOSYS;
>>   		goto err;
>> +	}
>>   
>>   	dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>>   	if (!dmi_kset)
>> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>>   err:
>>   	cleanup_entry_list();
>>   	kset_unregister(dmi_kset);
>> -	kobject_put(dmi_kobj);
>>   	return error;
>>   }
>>   
>> @@ -685,8 +684,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 d3aae09..bb19f8b 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():
>>    */
>> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>>   }
>>   
>>   static phys_addr_t dmi_base;
>> +static u8 *dmi_table;
> This variable is only ever used in a single function (dmi_init). Does
> it actually need to be a global variable? I suspect not.

, Yes.

>
>>   
>>   static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>>   		void *))
>> @@ -476,6 +483,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) {
>> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>>   					dmi_ver >> 8, dmi_ver & 0xFF,
>>   					(dmi_ver < 0x0300) ? "" : ".x");
>>   			} else {
>> +				smbios_entry_point_size = 15;
>> +				memcpy(smbios_entry_point, buf,
>> +				       smbios_entry_point_size);
>>   				dmi_ver = (buf[14] & 0xF0) << 4 |
>>   					   (buf[14] & 0x0F);
>>   				pr_info("Legacy DMI %d.%d present.\n",
>> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>>   		dmi_ver &= 0xFFFFFF;
>>   		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);
>>   
>>   		/*
>>   		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
>> @@ -638,6 +652,72 @@ 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);
> This one could be world-readable as it contains no sensitive
> information.

It contains the address of DMI table containing sensitive information.
Who knows which ways can be used to take it. Anyway, no see reasons in this
w/o DMI table. But if you insist I can do it "world-readable".

>
>> +struct bin_attribute bin_attr_dmi_table =
>> +			__BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
> I do not understand why you don't use BIN_ATTR here too? I tried naming
> the attribute bin_attr_DMI and it seems to work just fine, checkpatch
> doesn't even complain!

I dislike upper case in names, at least in such simple names.
It makes me using bin_attr_DMI lower in the code. That's why.
But if you like it, I will name it "bin_attr_DMI"

>
>> +
>> +static int __init dmi_init(void)
>> +{
>> +	int ret = -ENOMEM;
>> +	struct kobject *tables_kobj = NULL;
>> +
>> +	if (!dmi_available) {
>> +		ret = -ENOSYS;
>> +		goto err;
>> +	}
> This is weird. Can this actually happen?
>
> We currently have two ways to enter this module: dmi_scan_machine(),
> which is called by the architecture code, and dmi_init(), which is
> called at subsys_initcall time. As far as I can see,
> core/arch_initcalls are guaranteed to be always called before
> subsys_initcalls. If we can rely on that, the test above is not needed.
> If for any reason we can't, that means that dmi_init() should not be a
> subsys_initcall, but should instead be called explicitly at the end of
> dmi_scan_machine().

We cannot be sure that firmware_kobj created at time of dmi_init().
The sources don't oblige you to call it at core level,
for instance like it was done for arm64. For x86, dmi_init() can be called
before firmware_kobj is created. And if I call it from dmi_init() I suppose
I would face an error. As I can't call it in dmi_init I can't be sure that
DMI is available at all. So, no, we have to check dmi_available here and 
call
it at subsys layer, where it's supposed to be.

>> +
>> +	/* Set up dmi directory at /sys/firmware/dmi */
>> +	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> +	if (!dmi_kobj)
>> +		goto err;
>> +
>> +	tables_kobj = kobject_create_and_add("tables", dmi_kobj);
>> +	if (!tables_kobj)
>> +		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)
>> +		goto err;
> At this point "ret" has value 0 so you will take the error path but
> return with success. No good. I suggest that you move the call to
> dmi_remap before the first call to sysfs_create_bin_file above, so that
> "ret" still has value -ENOMEM.

Yes.

>
>> +
>> +	bin_attr_dmi_table.size = dmi_len;
>> +	bin_attr_dmi_table.private = dmi_table;
>> +	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
>> +	if (ret) {
>> +		dmi_unmap(dmi_table);
> Doing this here goes against the logic of having a single error path.
> Instead you should have an additional error label before err: and jump
> there.

I will redo this.

>
>> +		goto err;
>> +	}
>> +
>> +	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);
> These last two calls could be moved inside the conditional above. Even
> better would be a separate error label so that you know exactly what
> needs to be undone.

Let it be.

>
>> +	kobject_del(dmi_kobj);
>> +	kobject_put(dmi_kobj);
>> +	dmi_kobj = NULL;
> I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
> appropriate comment), so that dmi-sysfs has a chance to load? As it is
> now, a bug or some unexpected behavior in this new code could cause a
> regression for dmi-sysfs users. Just because I don't like dmi_sysfs
> doesn't mean we can break it ;-)

As I remember it was not so critical for you last time.
"I don't care which way you choose". And I've explained my position.
But it's not very hard to me to change it. Anyway patch requires re-push.

>
>> +
>> +	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..9f55f46 100644
>> --- a/include/linux/dmi.h
>> +++ b/include/linux/dmi.h
>> @@ -93,6 +93,7 @@ struct dmi_dev_onboard {
>>   	int devfn;
>>   };
>>   
>> +extern struct kobject *dmi_kobj;
> struct kobject is defined in <linux/kobject.h> so I think you should
> include that file to avoid random build failures in the future.

Ok

>
>>   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);
>

Thanks Jean.
I will add you propositions and re-push the series.
If you are OK with my answer of-course.

-- 
Regards,
Ivan Khoronzhuk

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

[Jean Delvare]

Hi Ivan,

Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
> On 16.04.15 12:52, Jean Delvare wrote:
> > On Thu,  2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
> >> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
> > This one could be world-readable as it contains no sensitive
> > information.
> 
> It contains the address of DMI table containing sensitive information.
> Who knows which ways can be used to take it. Anyway, no see reasons in this
> w/o DMI table. But if you insist I can do it "world-readable".

OK, you convinced me.

> >
> >> +struct bin_attribute bin_attr_dmi_table =
> >> +			__BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
> > I do not understand why you don't use BIN_ATTR here too? I tried naming
> > the attribute bin_attr_DMI and it seems to work just fine, checkpatch
> > doesn't even complain!
> 
> I dislike upper case in names, at least in such simple names.
> It makes me using bin_attr_DMI lower in the code. That's why.
> But if you like it, I will name it "bin_attr_DMI"

I don't like upper case in names either, but in this specific case, I'd
do it for consistency. As you wish though, I really only wanted to know
the reason.

> >> +
> >> +static int __init dmi_init(void)
> >> +{
> >> +	int ret = -ENOMEM;
> >> +	struct kobject *tables_kobj = NULL;
> >> +
> >> +	if (!dmi_available) {
> >> +		ret = -ENOSYS;
> >> +		goto err;
> >> +	}
> > This is weird. Can this actually happen?
> >
> > We currently have two ways to enter this module: dmi_scan_machine(),
> > which is called by the architecture code, and dmi_init(), which is
> > called at subsys_initcall time. As far as I can see,
> > core/arch_initcalls are guaranteed to be always called before
> > subsys_initcalls. If we can rely on that, the test above is not needed.
> > If for any reason we can't, that means that dmi_init() should not be a
> > subsys_initcall, but should instead be called explicitly at the end of
> > dmi_scan_machine().
> 
> We cannot be sure that firmware_kobj created at time of dmi_init().
> The sources don't oblige you to call it at core level,
> for instance like it was done for arm64. For x86, dmi_init() can be called
> before firmware_kobj is created.

Looking at the code, it seems that firmware_kobj is created very, very
early in the boot process. In do_basic_setup(), you can see that
driver_init() (which in turn calls firmware_init(), creating
firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
defined before dmi_scan_machine() or dmi_init() is called.

Oh, and this wasn't even my point ;-) I'm fine with you checking if
firmware_kobj is defined. My question was about the dmi_available check
above. But that question was silly anyway, sorry. I confused
dmi_available with dmi_initialized. Checking for dmi_available is
perfectly reasonable, please scratch my objection.

> And if I call it from dmi_init() I suppose
> I would face an error. As I can't call it in dmi_init I can't be sure that
> DMI is available at all. So, no, we have to check dmi_available here and 
> call it at subsys layer, where it's supposed to be.

I can't parse that, I suspect you wrote dmi_init where you actually
meant dmi_scan_machine? Given how early firmware_kobj is created, I
think the code currently in dmi_init could in fact go at the end of
dmi_scan_machine. But it's not important for the time being, this can be
attempted later.

> >> (...)
> >> +	kobject_del(dmi_kobj);
> >> +	kobject_put(dmi_kobj);
> >> +	dmi_kobj = NULL;
> > I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
> > appropriate comment), so that dmi-sysfs has a chance to load? As it is
> > now, a bug or some unexpected behavior in this new code could cause a
> > regression for dmi-sysfs users. Just because I don't like dmi_sysfs
> > doesn't mean we can break it ;-)
> 
> As I remember it was not so critical for you last time.
> "I don't care which way you choose". And I've explained my position.
> But it's not very hard to me to change it. Anyway patch requires re-push.

You're right, I did not remember we had discussed this already,
sorry :-(

Well, I still agree that it doesn't really matter, but of two acceptable
solutions for an event which will most likely never happen, why not go
for the ones with the fewer lines of code? ;-)

> (...)
> Thanks Jean.
> I will add you propositions and re-push the series.
> If you are OK with my answer of-course.

You're welcome. Yes I think we have clarified everything now so you can
submit the next (hopefully last) iteration.

Thank you,
-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

Hi Ivan,

Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
> On 16.04.15 12:52, Jean Delvare wrote:
> > On Thu,  2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
> >> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
> > This one could be world-readable as it contains no sensitive
> > information.
> 
> It contains the address of DMI table containing sensitive information.
> Who knows which ways can be used to take it. Anyway, no see reasons in this
> w/o DMI table. But if you insist I can do it "world-readable".

OK, you convinced me.

> >
> >> +struct bin_attribute bin_attr_dmi_table =
> >> +			__BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
> > I do not understand why you don't use BIN_ATTR here too? I tried naming
> > the attribute bin_attr_DMI and it seems to work just fine, checkpatch
> > doesn't even complain!
> 
> I dislike upper case in names, at least in such simple names.
> It makes me using bin_attr_DMI lower in the code. That's why.
> But if you like it, I will name it "bin_attr_DMI"

I don't like upper case in names either, but in this specific case, I'd
do it for consistency. As you wish though, I really only wanted to know
the reason.

> >> +
> >> +static int __init dmi_init(void)
> >> +{
> >> +	int ret = -ENOMEM;
> >> +	struct kobject *tables_kobj = NULL;
> >> +
> >> +	if (!dmi_available) {
> >> +		ret = -ENOSYS;
> >> +		goto err;
> >> +	}
> > This is weird. Can this actually happen?
> >
> > We currently have two ways to enter this module: dmi_scan_machine(),
> > which is called by the architecture code, and dmi_init(), which is
> > called at subsys_initcall time. As far as I can see,
> > core/arch_initcalls are guaranteed to be always called before
> > subsys_initcalls. If we can rely on that, the test above is not needed.
> > If for any reason we can't, that means that dmi_init() should not be a
> > subsys_initcall, but should instead be called explicitly at the end of
> > dmi_scan_machine().
> 
> We cannot be sure that firmware_kobj created at time of dmi_init().
> The sources don't oblige you to call it at core level,
> for instance like it was done for arm64. For x86, dmi_init() can be called
> before firmware_kobj is created.

Looking at the code, it seems that firmware_kobj is created very, very
early in the boot process. In do_basic_setup(), you can see that
driver_init() (which in turn calls firmware_init(), creating
firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
defined before dmi_scan_machine() or dmi_init() is called.

Oh, and this wasn't even my point ;-) I'm fine with you checking if
firmware_kobj is defined. My question was about the dmi_available check
above. But that question was silly anyway, sorry. I confused
dmi_available with dmi_initialized. Checking for dmi_available is
perfectly reasonable, please scratch my objection.

> And if I call it from dmi_init() I suppose
> I would face an error. As I can't call it in dmi_init I can't be sure that
> DMI is available at all. So, no, we have to check dmi_available here and 
> call it at subsys layer, where it's supposed to be.

I can't parse that, I suspect you wrote dmi_init where you actually
meant dmi_scan_machine? Given how early firmware_kobj is created, I
think the code currently in dmi_init could in fact go at the end of
dmi_scan_machine. But it's not important for the time being, this can be
attempted later.

> >> (...)
> >> +	kobject_del(dmi_kobj);
> >> +	kobject_put(dmi_kobj);
> >> +	dmi_kobj = NULL;
> > I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
> > appropriate comment), so that dmi-sysfs has a chance to load? As it is
> > now, a bug or some unexpected behavior in this new code could cause a
> > regression for dmi-sysfs users. Just because I don't like dmi_sysfs
> > doesn't mean we can break it ;-)
> 
> As I remember it was not so critical for you last time.
> "I don't care which way you choose". And I've explained my position.
> But it's not very hard to me to change it. Anyway patch requires re-push.

You're right, I did not remember we had discussed this already,
sorry :-(

Well, I still agree that it doesn't really matter, but of two acceptable
solutions for an event which will most likely never happen, why not go
for the ones with the fewer lines of code? ;-)

> (...)
> Thanks Jean.
> I will add you propositions and re-push the series.
> If you are OK with my answer of-course.

You're welcome. Yes I think we have clarified everything now so you can
submit the next (hopefully last) iteration.

Thank you,
-- 
Jean Delvare
SUSE L3 Support


_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel

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

[Roy Franz]

On Wed, Apr 15, 2015 at 11:48 PM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Roy,
>
> On Wed, 15 Apr 2015 17:54:51 -0700, Roy Franz wrote:
>> On Tue, Apr 14, 2015 at 9:19 PM, Roy Franz <roy.franz@linaro.org> wrote:
>> > I have made modifications to dmidecode to support this interface, and it
>> > works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
>> > The only open issue I am aware of is how the SMBIOS v3 entry point
>> > will be handled,
>> > especially in cases where there is a v2 and a v3 entry point.
>> > In principal I think this a good change - are there any other open issues?
>>
>> I looked at the SMBIOS spec again, and the platform can provide either or
>> both of the 32-bit and 64-bit entry points.  The spec says that an
>> implementation
>> "should" provide a 32-bit entry point for compatibility.
>>
>> These 2 entry point structures can both point to the same SMBIOS
>> structure table,
>> or two distinct ones.  If distinct, the one referenced by the 32-bit
>> entry point must be
>> a consistent subset of the 64-bit one.
>> There does not seem to be any prohibition from using new SMBIOS v3 structures
>> in a table referenced by a 32-bit entry point, so backwards
>> compatibility is completely
>> up to the implementation.
>
> What do you mean with "new SMBIOS v3 structures"? I took a brief look
> at the change log at the end of the SMBIOS 3.0.0 specification, and all
> I can see are new enumerated values for existing fields, as well as 3
> new fields in the type 4 structure. I can't see any new structure type.
> Either way this is all backwards compatible, which is why both entry
> points can point to the same table.
Sorry, I used that imprecisely - I was referring to the new things defined
in the 3.0 specification in addition to the new entry point.  It sounds like
there is not much.
>
>> Since the point of this patchset (and related changes to dmidecode) is
>> to provide the
>> SMBIOS information without using /dev/mem, I think the interface we
>> define should
>> support all the cases outlined in the specification.  I would like to
>> avoid a case where
>> we're back to using /dev/mem to deal with the dual entry/dual table
>> case if that becomes
>> important down the road.
>>
>> Here's a proposal for files in /sys/firmware/dmi/tables:
>>
>> smbios_entry_point32   -  32 bit (existing entry point type), if it exists.
>> smbios_entry_point64   -  64 bit entry, new in SMBIOS v3.0
>> DMI32                         -  smbios structure tables referenced by
>> 32 bit entry point, if it exists
>> DMI64                         -  smbios structure tables referenced by
>> 64 bit entry point, if it exists.
>>                                      symlink to DMI32 if identical
>> smbios_entry_point    - symlink to smbios_entry_point64 if it exists,
>> otherwise symlink to smbios_entry_point64
>> DMI                           - symlink to DMI64 if it exists,
>> otherwise symlink to DMI32
>>
>> These last two would provide names to the 'preferred' tables, and
>> names that would always exist on all systems, which
>> I think is a nice property to have.
>>
>> Thoughts?
>
> The idea was discussed before, and discarded. The question is, what use
> cases do you envision from a user-space application perspective? As you
> found out, the table pointed to by the _SM3_ entry point must be a
> super-set of the one pointed to by the _SM_ entry point, so for a
> compliant implementation there is no reason to follow the _SM_ entry
> point when both exist. And if only one entry point exists, there is
> nothing to choose from.
>
> The kernel itself will have to choose one of the entry points when it
> comes to dmi_scan and dmi-id. It can't provide two sets of DMI strings.
> So it seems reasonable to only expose though sysfs that one entry point
> and table that the kernel itself used.
>
> The only case where it would make some sense to expose everything to
> user-space is for BIOS debugging purpose: if both entry points exist
> and point to different tables, and if the _SM3_ table is broken and
> the _SM_ table is correct, then you may want to be able to temporarily
> read the _SM_ table instead of the _SM3_ table, to check what needs to
> be fixed in the latter. But I would argue that this is beyond the scope
> of our code: neither the kernel nor dmidecode are DMI table or BIOS
> authoring tools.
>
> If there really ever is a need to ignore the _SM3_ entry point, I'd
> rather have a kernel boot parameter for this. This would guarantee that
> the kernel and user-space applications always operate on the same data.
>
> So I would keep the sysfs interface simple, as Ivan implemented it.

Apologies for missing that previous discussion - I was intentionally
bringing an old topic up.
I don't feel strongly, and I think the main use case is debugging
something in the future,
as it doesn't address any current problems. My main motivation for
proposing this was to
avoid a future case where the answer is "just map /dev/mem".  This is
acceptable if the only
people doing it are developers debugging a BIOS/UEFI problem, but I
don't think this is the
right answer for general use case.  If a general use case emerges in
the future that requires
both tables exposed, we can extend the sysfs interface then (and this
may well never happen.)

Thanks,
Roy


>
> --
> Jean Delvare
> SUSE L3 Support

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

[subscivan]

Jean,

On 16.04.15 18:44, Jean Delvare wrote:
> Hi Ivan,
>
> Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
>> On 16.04.15 12:52, Jean Delvare wrote:
>>> On Thu,  2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
>>>> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
>>> This one could be world-readable as it contains no sensitive
>>> information.
>> It contains the address of DMI table containing sensitive information.
>> Who knows which ways can be used to take it. Anyway, no see reasons in this
>> w/o DMI table. But if you insist I can do it "world-readable".
> OK, you convinced me.
>
>>>> +struct bin_attribute bin_attr_dmi_table =
>>>> +			__BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>>> I do not understand why you don't use BIN_ATTR here too? I tried naming
>>> the attribute bin_attr_DMI and it seems to work just fine, checkpatch
>>> doesn't even complain!
>> I dislike upper case in names, at least in such simple names.
>> It makes me using bin_attr_DMI lower in the code. That's why.
>> But if you like it, I will name it "bin_attr_DMI"
> I don't like upper case in names either, but in this specific case, I'd
> do it for consistency. As you wish though, I really only wanted to know
> the reason.
>
>>>> +
>>>> +static int __init dmi_init(void)
>>>> +{
>>>> +	int ret = -ENOMEM;
>>>> +	struct kobject *tables_kobj = NULL;
>>>> +
>>>> +	if (!dmi_available) {
>>>> +		ret = -ENOSYS;
>>>> +		goto err;
>>>> +	}
>>> This is weird. Can this actually happen?
>>>
>>> We currently have two ways to enter this module: dmi_scan_machine(),
>>> which is called by the architecture code, and dmi_init(), which is
>>> called at subsys_initcall time. As far as I can see,
>>> core/arch_initcalls are guaranteed to be always called before
>>> subsys_initcalls. If we can rely on that, the test above is not needed.
>>> If for any reason we can't, that means that dmi_init() should not be a
>>> subsys_initcall, but should instead be called explicitly at the end of
>>> dmi_scan_machine().
>> We cannot be sure that firmware_kobj created at time of dmi_init().
>> The sources don't oblige you to call it at core level,
>> for instance like it was done for arm64. For x86, dmi_init() can be called
>> before firmware_kobj is created.
> Looking at the code, it seems that firmware_kobj is created very, very
> early in the boot process. In do_basic_setup(), you can see that
> driver_init() (which in turn calls firmware_init(), creating
> firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
> defined before dmi_scan_machine() or dmi_init() is called.

No. Not must, rather should. See below.

>
> Oh, and this wasn't even my point ;-) I'm fine with you checking if
> firmware_kobj is defined. My question was about the dmi_available check
> above. But that question was silly anyway, sorry. I confused
> dmi_available with dmi_initialized. Checking for dmi_available is
> perfectly reasonable, please scratch my objection.
>
>> And if I call it from dmi_init() I suppose
>> I would face an error. As I can't call it in dmi_init I can't be sure that
>> DMI is available at all. So, no, we have to check dmi_available here and
>> call it at subsys layer, where it's supposed to be.
> I can't parse that, I suspect you wrote dmi_init where you actually
> meant dmi_scan_machine? Given how early firmware_kobj is created, I
> think the code currently in dmi_init could in fact go at the end of
> dmi_scan_machine.

Actually, dmi_scan_machine can be called even earlier.
As I've sad, for x86, it's called before firmware_kobj is created.

kernel_start()
     setup_arch()
         dmi_scan_machine()

And for firmware_init(), as you noticed already:

start_kernel()
     rest_init()
         kernel_init()
             kernel_init_freeable()
                 do_basic_setup()
                     driver_init()
                         firmware_init()

Pay attentions that setup_arch() is called much earlier than rest_init().
So dmi_init couldn't in fact go at the end of dmi_scan_machine.

> But it's not important for the time being, this can be
> attempted later.
>
>>>> (...)
>>>> +	kobject_del(dmi_kobj);
>>>> +	kobject_put(dmi_kobj);
>>>> +	dmi_kobj = NULL;
>>> I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
>>> appropriate comment), so that dmi-sysfs has a chance to load? As it is
>>> now, a bug or some unexpected behavior in this new code could cause a
>>> regression for dmi-sysfs users. Just because I don't like dmi_sysfs
>>> doesn't mean we can break it ;-)
>> As I remember it was not so critical for you last time.
>> "I don't care which way you choose". And I've explained my position.
>> But it's not very hard to me to change it. Anyway patch requires re-push.
> You're right, I did not remember we had discussed this already,
> sorry :-(
>
> Well, I still agree that it doesn't really matter, but of two acceptable
> solutions for an event which will most likely never happen, why not go
> for the ones with the fewer lines of code? ;-)

Ok.

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

[Ivan.khoronzhuk]

Sorry, sent it from not original mail.

On 16.04.15 20:27, subscivan wrote:
> Jean,
>
> On 16.04.15 18:44, Jean Delvare wrote:
>> Hi Ivan,
>>
>> Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
>>> On 16.04.15 12:52, Jean Delvare wrote:
>>>> On Thu,  2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
>>>>> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, 
>>>>> NULL, 0);
>>>> This one could be world-readable as it contains no sensitive
>>>> information.
>>> It contains the address of DMI table containing sensitive information.
>>> Who knows which ways can be used to take it. Anyway, no see reasons 
>>> in this
>>> w/o DMI table. But if you insist I can do it "world-readable".
>> OK, you convinced me.
>>
>>>>> +struct bin_attribute bin_attr_dmi_table =
>>>>> +            __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>>>> I do not understand why you don't use BIN_ATTR here too? I tried 
>>>> naming
>>>> the attribute bin_attr_DMI and it seems to work just fine, checkpatch
>>>> doesn't even complain!
>>> I dislike upper case in names, at least in such simple names.
>>> It makes me using bin_attr_DMI lower in the code. That's why.
>>> But if you like it, I will name it "bin_attr_DMI"
>> I don't like upper case in names either, but in this specific case, I'd
>> do it for consistency. As you wish though, I really only wanted to know
>> the reason.
>>
>>>>> +
>>>>> +static int __init dmi_init(void)
>>>>> +{
>>>>> +    int ret = -ENOMEM;
>>>>> +    struct kobject *tables_kobj = NULL;
>>>>> +
>>>>> +    if (!dmi_available) {
>>>>> +        ret = -ENOSYS;
>>>>> +        goto err;
>>>>> +    }
>>>> This is weird. Can this actually happen?
>>>>
>>>> We currently have two ways to enter this module: dmi_scan_machine(),
>>>> which is called by the architecture code, and dmi_init(), which is
>>>> called at subsys_initcall time. As far as I can see,
>>>> core/arch_initcalls are guaranteed to be always called before
>>>> subsys_initcalls. If we can rely on that, the test above is not 
>>>> needed.
>>>> If for any reason we can't, that means that dmi_init() should not be a
>>>> subsys_initcall, but should instead be called explicitly at the end of
>>>> dmi_scan_machine().
>>> We cannot be sure that firmware_kobj created at time of dmi_init().
>>> The sources don't oblige you to call it at core level,
>>> for instance like it was done for arm64. For x86, dmi_init() can be 
>>> called
>>> before firmware_kobj is created.
>> Looking at the code, it seems that firmware_kobj is created very, very
>> early in the boot process. In do_basic_setup(), you can see that
>> driver_init() (which in turn calls firmware_init(), creating
>> firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
>> defined before dmi_scan_machine() or dmi_init() is called.
>
> No. Not must, rather should. See below.
>
>>
>> Oh, and this wasn't even my point ;-) I'm fine with you checking if
>> firmware_kobj is defined. My question was about the dmi_available check
>> above. But that question was silly anyway, sorry. I confused
>> dmi_available with dmi_initialized. Checking for dmi_available is
>> perfectly reasonable, please scratch my objection.
>>
>>> And if I call it from dmi_init() I suppose
>>> I would face an error. As I can't call it in dmi_init I can't be 
>>> sure that
>>> DMI is available at all. So, no, we have to check dmi_available here 
>>> and
>>> call it at subsys layer, where it's supposed to be.
>> I can't parse that, I suspect you wrote dmi_init where you actually
>> meant dmi_scan_machine? Given how early firmware_kobj is created, I
>> think the code currently in dmi_init could in fact go at the end of
>> dmi_scan_machine.
>
> Actually, dmi_scan_machine can be called even earlier.
> As I've sad, for x86, it's called before firmware_kobj is created.
>
> kernel_start()
>     setup_arch()
>         dmi_scan_machine()
>
> And for firmware_init(), as you noticed already:
>
> start_kernel()
>     rest_init()
>         kernel_init()
>             kernel_init_freeable()
>                 do_basic_setup()
>                     driver_init()
>                         firmware_init()
>
> Pay attentions that setup_arch() is called much earlier than rest_init().
> So dmi_init couldn't in fact go at the end of dmi_scan_machine.
>
>> But it's not important for the time being, this can be
>> attempted later.
>>
>>>>> (...)
>>>>> +    kobject_del(dmi_kobj);
>>>>> +    kobject_put(dmi_kobj);
>>>>> +    dmi_kobj = NULL;
>>>> I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
>>>> appropriate comment), so that dmi-sysfs has a chance to load? As it is
>>>> now, a bug or some unexpected behavior in this new code could cause a
>>>> regression for dmi-sysfs users. Just because I don't like dmi_sysfs
>>>> doesn't mean we can break it ;-)
>>> As I remember it was not so critical for you last time.
>>> "I don't care which way you choose". And I've explained my position.
>>> But it's not very hard to me to change it. Anyway patch requires 
>>> re-push.
>> You're right, I did not remember we had discussed this already,
>> sorry :-(
>>
>> Well, I still agree that it doesn't really matter, but of two acceptable
>> solutions for an event which will most likely never happen, why not go
>> for the ones with the fewer lines of code? ;-)
>
> Ok.

-- 
Regards,
Ivan Khoronzhuk

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

[Ivan.khoronzhuk]

Sorry, sent it from not original mail.

On 16.04.15 20:27, subscivan wrote:
> Jean,
>
> On 16.04.15 18:44, Jean Delvare wrote:
>> Hi Ivan,
>>
>> Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
>>> On 16.04.15 12:52, Jean Delvare wrote:
>>>> On Thu,  2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
>>>>> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, 
>>>>> NULL, 0);
>>>> This one could be world-readable as it contains no sensitive
>>>> information.
>>> It contains the address of DMI table containing sensitive information.
>>> Who knows which ways can be used to take it. Anyway, no see reasons 
>>> in this
>>> w/o DMI table. But if you insist I can do it "world-readable".
>> OK, you convinced me.
>>
>>>>> +struct bin_attribute bin_attr_dmi_table =
>>>>> +            __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>>>> I do not understand why you don't use BIN_ATTR here too? I tried 
>>>> naming
>>>> the attribute bin_attr_DMI and it seems to work just fine, checkpatch
>>>> doesn't even complain!
>>> I dislike upper case in names, at least in such simple names.
>>> It makes me using bin_attr_DMI lower in the code. That's why.
>>> But if you like it, I will name it "bin_attr_DMI"
>> I don't like upper case in names either, but in this specific case, I'd
>> do it for consistency. As you wish though, I really only wanted to know
>> the reason.
>>
>>>>> +
>>>>> +static int __init dmi_init(void)
>>>>> +{
>>>>> +    int ret = -ENOMEM;
>>>>> +    struct kobject *tables_kobj = NULL;
>>>>> +
>>>>> +    if (!dmi_available) {
>>>>> +        ret = -ENOSYS;
>>>>> +        goto err;
>>>>> +    }
>>>> This is weird. Can this actually happen?
>>>>
>>>> We currently have two ways to enter this module: dmi_scan_machine(),
>>>> which is called by the architecture code, and dmi_init(), which is
>>>> called at subsys_initcall time. As far as I can see,
>>>> core/arch_initcalls are guaranteed to be always called before
>>>> subsys_initcalls. If we can rely on that, the test above is not 
>>>> needed.
>>>> If for any reason we can't, that means that dmi_init() should not be a
>>>> subsys_initcall, but should instead be called explicitly at the end of
>>>> dmi_scan_machine().
>>> We cannot be sure that firmware_kobj created at time of dmi_init().
>>> The sources don't oblige you to call it at core level,
>>> for instance like it was done for arm64. For x86, dmi_init() can be 
>>> called
>>> before firmware_kobj is created.
>> Looking at the code, it seems that firmware_kobj is created very, very
>> early in the boot process. In do_basic_setup(), you can see that
>> driver_init() (which in turn calls firmware_init(), creating
>> firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
>> defined before dmi_scan_machine() or dmi_init() is called.
>
> No. Not must, rather should. See below.
>
>>
>> Oh, and this wasn't even my point ;-) I'm fine with you checking if
>> firmware_kobj is defined. My question was about the dmi_available check
>> above. But that question was silly anyway, sorry. I confused
>> dmi_available with dmi_initialized. Checking for dmi_available is
>> perfectly reasonable, please scratch my objection.
>>
>>> And if I call it from dmi_init() I suppose
>>> I would face an error. As I can't call it in dmi_init I can't be 
>>> sure that
>>> DMI is available at all. So, no, we have to check dmi_available here 
>>> and
>>> call it at subsys layer, where it's supposed to be.
>> I can't parse that, I suspect you wrote dmi_init where you actually
>> meant dmi_scan_machine? Given how early firmware_kobj is created, I
>> think the code currently in dmi_init could in fact go at the end of
>> dmi_scan_machine.
>
> Actually, dmi_scan_machine can be called even earlier.
> As I've sad, for x86, it's called before firmware_kobj is created.
>
> kernel_start()
>     setup_arch()
>         dmi_scan_machine()
>
> And for firmware_init(), as you noticed already:
>
> start_kernel()
>     rest_init()
>         kernel_init()
>             kernel_init_freeable()
>                 do_basic_setup()
>                     driver_init()
>                         firmware_init()
>
> Pay attentions that setup_arch() is called much earlier than rest_init().
> So dmi_init couldn't in fact go at the end of dmi_scan_machine.
>
>> But it's not important for the time being, this can be
>> attempted later.
>>
>>>>> (...)
>>>>> +    kobject_del(dmi_kobj);
>>>>> +    kobject_put(dmi_kobj);
>>>>> +    dmi_kobj = NULL;
>>>> I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
>>>> appropriate comment), so that dmi-sysfs has a chance to load? As it is
>>>> now, a bug or some unexpected behavior in this new code could cause a
>>>> regression for dmi-sysfs users. Just because I don't like dmi_sysfs
>>>> doesn't mean we can break it ;-)
>>> As I remember it was not so critical for you last time.
>>> "I don't care which way you choose". And I've explained my position.
>>> But it's not very hard to me to change it. Anyway patch requires 
>>> re-push.
>> You're right, I did not remember we had discussed this already,
>> sorry :-(
>>
>> Well, I still agree that it doesn't really matter, but of two acceptable
>> solutions for an event which will most likely never happen, why not go
>> for the ones with the fewer lines of code? ;-)
>
> Ok.

-- 
Regards,
Ivan Khoronzhuk

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

[Ivan.khoronzhuk]

Hi Jean,

On 16.04.15 11:35, Jean Delvare wrote:
> Hi Matt,
>
> On Wed, 15 Apr 2015 15:35:30 +0100, Matt Fleming wrote:
>> On Thu, 02 Apr, at 03:57:01PM, Ivan Khoronzhuk wrote:
>>> 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.
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
>>> ---
>>>   drivers/firmware/dmi_scan.c | 10 +++++-----
>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>> Looks good to me.
>>
>> Jean, do you want me to pick this patch up or are you going to?
> Good question, we need to agree on a strategy.
>
> There are 3 patch sets to consider here.
>
> 1* My patch fixing the UUID ordering bug. It must go in first and
>     immediately, as it fixes a regression and will have to be backported
>     to stable branches.

||
V

>
> 2* 2 older patches from Ivan which are currently in your efi-next tree
>     if I'm not mistaken. Both were based on an old tree so they do not
>     apply cleanly on kernel v4.0, I had to fix them up manually. I have

They are in master tree already.

>     no idea if git would be able to merge them properly, as the fix
>     above changed the context even more.

Current efi-next already merged, so you should base your patches on
top of last changes.

>
> 3* The 3 new patches from Ivan which I am reviewing now, which are not
>     applied in any public tree AFAIK.

It shouldn't happen,
I've been verifying just now once again.
They are applied on top of linux_next cleanly.
Equal as on efi/next.

>
> I don't really care who picks these patches up and sends them to Linus,
> but I think they should all follow the same route so that Linus has as
> little merge work to do as possible. So either you pick them all, or I
> do. If I do, you'll have to drop the 2 patches you have in efi-next.
> Again I'm fine either way, so please let me know what makes your life
> easier and let's do that.
>
> Thanks,

Jean,
I'm going to base my series
"firmware: dmi_scan: add SBMIOS entry point and DMI tables"
on top of linux next unless you have already your tree to pick up changes.
Please let me know, if you have one.


-- 
Regards,
Ivan Khoronzhuk

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

[Jean Delvare]

Hi Ivan,

On Thu, 16 Apr 2015 23:16:59 +0300, Ivan.khoronzhuk wrote:
> On 16.04.15 11:35, Jean Delvare wrote:
> > On Wed, 15 Apr 2015 15:35:30 +0100, Matt Fleming wrote:
> >> Jean, do you want me to pick this patch up or are you going to?
> > Good question, we need to agree on a strategy.
> >
> > There are 3 patch sets to consider here.
> >
> > 1* My patch fixing the UUID ordering bug. It must go in first and
> >     immediately, as it fixes a regression and will have to be backported
> >     to stable branches.
> 
> ||
> V
> 
> >
> > 2* 2 older patches from Ivan which are currently in your efi-next tree
> >     if I'm not mistaken. Both were based on an old tree so they do not
> >     apply cleanly on kernel v4.0, I had to fix them up manually. I have
> 
> They are in master tree already.
> 
> >     no idea if git would be able to merge them properly, as the fix
> >     above changed the context even more.
> 
> Current efi-next already merged, so you should base your patches on
> top of last changes.

Correct. I looked at the result and the merge looks correct. I'll turn
my objections into fixup patches to apply on top, where still worth it.
In particular I'll start with the ".x" revert, as it will make
backporting the bug fix easier.

> >
> > 3* The 3 new patches from Ivan which I am reviewing now, which are not
> >     applied in any public tree AFAIK.
> 
> It shouldn't happen,
> I've been verifying just now once again.
> They are applied on top of linux_next cleanly.
> Equal as on efi/next.

I can't see them at
http://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/log/?h=next

To clarify: I do not claim that they can't be applied, I'm only saying
they're not there yet (which is OK as they were still pending my
review.) They do apply just fine, no problem with this.

> > I don't really care who picks these patches up and sends them to Linus,
> > but I think they should all follow the same route so that Linus has as
> > little merge work to do as possible. So either you pick them all, or I
> > do. If I do, you'll have to drop the 2 patches you have in efi-next.
> > Again I'm fine either way, so please let me know what makes your life
> > easier and let's do that.
> 
> I'm going to base my series
> "firmware: dmi_scan: add SBMIOS entry point and DMI tables"
> on top of linux next unless you have already your tree to pick up changes.
> Please let me know, if you have one.

I have no formal tree yet, but my current patch set can be seen at:
http://jdelvare.nerim.net/devel/linux-3/jdelvare-dmi/

First 2 patches from you are already upstream. You should rebase your
updated patch series on top of upstream + patches 03 and 04, as they
will go in first.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

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

[Ivan.khoronzhuk]

On 17.04.15 11:54, Jean Delvare wrote:
> Hi Ivan,
>
> On Thu, 16 Apr 2015 23:16:59 +0300, Ivan.khoronzhuk wrote:
>> On 16.04.15 11:35, Jean Delvare wrote:
>>> On Wed, 15 Apr 2015 15:35:30 +0100, Matt Fleming wrote:
>>>> Jean, do you want me to pick this patch up or are you going to?
>>> Good question, we need to agree on a strategy.
>>>
>>> There are 3 patch sets to consider here.
>>>
>>> 1* My patch fixing the UUID ordering bug. It must go in first and
>>>      immediately, as it fixes a regression and will have to be backported
>>>      to stable branches.
>> ||
>> V
>>
>>> 2* 2 older patches from Ivan which are currently in your efi-next tree
>>>      if I'm not mistaken. Both were based on an old tree so they do not
>>>      apply cleanly on kernel v4.0, I had to fix them up manually. I have
>> They are in master tree already.
>>
>>>      no idea if git would be able to merge them properly, as the fix
>>>      above changed the context even more.
>> Current efi-next already merged, so you should base your patches on
>> top of last changes.
> Correct. I looked at the result and the merge looks correct. I'll turn
> my objections into fixup patches to apply on top, where still worth it.
> In particular I'll start with the ".x" revert, as it will make
> backporting the bug fix easier.
>
>>> 3* The 3 new patches from Ivan which I am reviewing now, which are not
>>>      applied in any public tree AFAIK.
>> It shouldn't happen,
>> I've been verifying just now once again.
>> They are applied on top of linux_next cleanly.
>> Equal as on efi/next.
> I can't see them at
> http://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/log/?h=next
>
> To clarify: I do not claim that they can't be applied, I'm only saying
> they're not there yet (which is OK as they were still pending my
> review.) They do apply just fine, no problem with this.
>
>>> I don't really care who picks these patches up and sends them to Linus,
>>> but I think they should all follow the same route so that Linus has as
>>> little merge work to do as possible. So either you pick them all, or I
>>> do. If I do, you'll have to drop the 2 patches you have in efi-next.
>>> Again I'm fine either way, so please let me know what makes your life
>>> easier and let's do that.
>> I'm going to base my series
>> "firmware: dmi_scan: add SBMIOS entry point and DMI tables"
>> on top of linux next unless you have already your tree to pick up changes.
>> Please let me know, if you have one.
> I have no formal tree yet, but my current patch set can be seen at:
> http://jdelvare.nerim.net/devel/linux-3/jdelvare-dmi/
>
> First 2 patches from you are already upstream. You should rebase your
> updated patch series on top of upstream + patches 03 and 04, as they
> will go in first.
>
> Thanks,

Not sure that's a good idea to base on patches that doesn't path any 
review and
no one cannot apply. At least it be good you send them that I can point 
on them in
commit message.

-- 
Regards,
Ivan Khoronzhuk

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

[Ivan.khoronzhuk]

Hi Jean,

On 17.04.15 13:11, Ivan.khoronzhuk wrote:
>
>
> On 17.04.15 11:54, Jean Delvare wrote:
>> Hi Ivan,
>>
>> On Thu, 16 Apr 2015 23:16:59 +0300, Ivan.khoronzhuk wrote:
>>> On 16.04.15 11:35, Jean Delvare wrote:
>>>> On Wed, 15 Apr 2015 15:35:30 +0100, Matt Fleming wrote:
>>>>> Jean, do you want me to pick this patch up or are you going to?
>>>> Good question, we need to agree on a strategy.
>>>>
>>>> There are 3 patch sets to consider here.
>>>>
>>>> 1* My patch fixing the UUID ordering bug. It must go in first and
>>>>      immediately, as it fixes a regression and will have to be 
>>>> backported
>>>>      to stable branches.
>>> ||
>>> V
>>>
>>>> 2* 2 older patches from Ivan which are currently in your efi-next tree
>>>>      if I'm not mistaken. Both were based on an old tree so they do 
>>>> not
>>>>      apply cleanly on kernel v4.0, I had to fix them up manually. I 
>>>> have
>>> They are in master tree already.
>>>
>>>>      no idea if git would be able to merge them properly, as the fix
>>>>      above changed the context even more.
>>> Current efi-next already merged, so you should base your patches on
>>> top of last changes.
>> Correct. I looked at the result and the merge looks correct. I'll turn
>> my objections into fixup patches to apply on top, where still worth it.
>> In particular I'll start with the ".x" revert, as it will make
>> backporting the bug fix easier.
>>
>>>> 3* The 3 new patches from Ivan which I am reviewing now, which are not
>>>>      applied in any public tree AFAIK.
>>> It shouldn't happen,
>>> I've been verifying just now once again.
>>> They are applied on top of linux_next cleanly.
>>> Equal as on efi/next.
>> I can't see them at
>> http://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/log/?h=next
>>
>> To clarify: I do not claim that they can't be applied, I'm only saying
>> they're not there yet (which is OK as they were still pending my
>> review.) They do apply just fine, no problem with this.
>>
>>>> I don't really care who picks these patches up and sends them to 
>>>> Linus,
>>>> but I think they should all follow the same route so that Linus has as
>>>> little merge work to do as possible. So either you pick them all, or I
>>>> do. If I do, you'll have to drop the 2 patches you have in efi-next.
>>>> Again I'm fine either way, so please let me know what makes your life
>>>> easier and let's do that.
>>> I'm going to base my series
>>> "firmware: dmi_scan: add SBMIOS entry point and DMI tables"
>>> on top of linux next unless you have already your tree to pick up 
>>> changes.
>>> Please let me know, if you have one.
>> I have no formal tree yet, but my current patch set can be seen at:
>> http://jdelvare.nerim.net/devel/linux-3/jdelvare-dmi/
>>
>> First 2 patches from you are already upstream. You should rebase your
>> updated patch series on top of upstream + patches 03 and 04, as they
>> will go in first.
>>
>> Thanks,
>
> Not sure that's a good idea to base on patches that doesn't path any 
> review and
> no one cannot apply. At least it be good you send them that I can 
> point on them in
> commit message.
>

Don't know why your patches don't apply on top of linux next.
They looks w/o conflicts. I've applied them by hand. To avoid mess, 
could you
please send them in order I can refer on them in my commit message.

-- 
Regards,
Ivan Khoronzhuk

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

[Jean Delvare]

On Fri, 17 Apr 2015 15:04:23 +0300, Ivan.khoronzhuk wrote:
> On 17.04.15 13:11, Ivan.khoronzhuk wrote:
> > On 17.04.15 11:54, Jean Delvare wrote:
> >> I have no formal tree yet, but my current patch set can be seen at:
> >> http://jdelvare.nerim.net/devel/linux-3/jdelvare-dmi/
> >>
> >> First 2 patches from you are already upstream. You should rebase your
> >> updated patch series on top of upstream + patches 03 and 04, as they
> >> will go in first.
> >
> > Not sure that's a good idea to base on patches that doesn't path any 
> > review and
> > no one cannot apply. At least it be good you send them that I can 
> > point on them in
> > commit message.
> 
> Don't know why your patches don't apply on top of linux next.
> They looks w/o conflicts. I've applied them by hand. To avoid mess, 
> could you
> please send them in order I can refer on them in my commit message.

Sorry, I had the the whitespace wrong when backporting one of your two
old patches, so I ended up with a code base different from what upstream
has. It's all fixed now, you can download the patches again and they
should apply fine (starting from 01 if working with kernel v4.0,
starting from 03 if working with linux-next.) Sorry for the trouble.

The reason why my patch series is based on v4.0 and not linux-next is
that I'm going to test it on a remote development system which
implements SMBIOS 3.0 and is currently running kernel v4.0, so I don't
want to use an unstable code base which could cause problems by itself.

-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

On Fri, 17 Apr 2015 15:04:23 +0300, Ivan.khoronzhuk wrote:
> On 17.04.15 13:11, Ivan.khoronzhuk wrote:
> > On 17.04.15 11:54, Jean Delvare wrote:
> >> I have no formal tree yet, but my current patch set can be seen at:
> >> http://jdelvare.nerim.net/devel/linux-3/jdelvare-dmi/
> >>
> >> First 2 patches from you are already upstream. You should rebase your
> >> updated patch series on top of upstream + patches 03 and 04, as they
> >> will go in first.
> >
> > Not sure that's a good idea to base on patches that doesn't path any 
> > review and
> > no one cannot apply. At least it be good you send them that I can 
> > point on them in
> > commit message.
> 
> Don't know why your patches don't apply on top of linux next.
> They looks w/o conflicts. I've applied them by hand. To avoid mess, 
> could you
> please send them in order I can refer on them in my commit message.

Sorry, I had the the whitespace wrong when backporting one of your two
old patches, so I ended up with a code base different from what upstream
has. It's all fixed now, you can download the patches again and they
should apply fine (starting from 01 if working with kernel v4.0,
starting from 03 if working with linux-next.) Sorry for the trouble.

The reason why my patch series is based on v4.0 and not linux-next is
that I'm going to test it on a remote development system which
implements SMBIOS 3.0 and is currently running kernel v4.0, so I don't
want to use an unstable code base which could cause problems by itself.

-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

On Thu, 16 Apr 2015 20:27:00 +0300, subscivan wrote:
> On 16.04.15 18:44, Jean Delvare wrote:
> > Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
> >> We cannot be sure that firmware_kobj created at time of dmi_init().
> >> The sources don't oblige you to call it at core level,
> >> for instance like it was done for arm64. For x86, dmi_init() can be called
> >> before firmware_kobj is created.
> > Looking at the code, it seems that firmware_kobj is created very, very
> > early in the boot process. In do_basic_setup(), you can see that
> > driver_init() (which in turn calls firmware_init(), creating
> > firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
> > defined before dmi_scan_machine() or dmi_init() is called.
> 
> No. Not must, rather should. See below.
> 
> > Oh, and this wasn't even my point ;-) I'm fine with you checking if
> > firmware_kobj is defined. My question was about the dmi_available check
> > above. But that question was silly anyway, sorry. I confused
> > dmi_available with dmi_initialized. Checking for dmi_available is
> > perfectly reasonable, please scratch my objection.
> >
> >> And if I call it from dmi_init() I suppose
> >> I would face an error. As I can't call it in dmi_init I can't be sure that
> >> DMI is available at all. So, no, we have to check dmi_available here and
> >> call it at subsys layer, where it's supposed to be.
> > I can't parse that, I suspect you wrote dmi_init where you actually
> > meant dmi_scan_machine? Given how early firmware_kobj is created, I
> > think the code currently in dmi_init could in fact go at the end of
> > dmi_scan_machine.
> 
> Actually, dmi_scan_machine can be called even earlier.
> As I've sad, for x86, it's called before firmware_kobj is created.
> 
> kernel_start()
>      setup_arch()
>          dmi_scan_machine()
> 
> And for firmware_init(), as you noticed already:
> 
> start_kernel()
>      rest_init()
>          kernel_init()
>              kernel_init_freeable()
>                  do_basic_setup()
>                      driver_init()
>                          firmware_init()
> 
> Pay attentions that setup_arch() is called much earlier than rest_init().
> So dmi_init couldn't in fact go at the end of dmi_scan_machine.

Yeah, you're right, sorry. Somehow I thought that setup_arch was an
arch_initcall, but it is not, so I got the order all wrong.

-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

On Thu, 16 Apr 2015 20:27:00 +0300, subscivan wrote:
> On 16.04.15 18:44, Jean Delvare wrote:
> > Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
> >> We cannot be sure that firmware_kobj created at time of dmi_init().
> >> The sources don't oblige you to call it at core level,
> >> for instance like it was done for arm64. For x86, dmi_init() can be called
> >> before firmware_kobj is created.
> > Looking at the code, it seems that firmware_kobj is created very, very
> > early in the boot process. In do_basic_setup(), you can see that
> > driver_init() (which in turn calls firmware_init(), creating
> > firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
> > defined before dmi_scan_machine() or dmi_init() is called.
> 
> No. Not must, rather should. See below.
> 
> > Oh, and this wasn't even my point ;-) I'm fine with you checking if
> > firmware_kobj is defined. My question was about the dmi_available check
> > above. But that question was silly anyway, sorry. I confused
> > dmi_available with dmi_initialized. Checking for dmi_available is
> > perfectly reasonable, please scratch my objection.
> >
> >> And if I call it from dmi_init() I suppose
> >> I would face an error. As I can't call it in dmi_init I can't be sure that
> >> DMI is available at all. So, no, we have to check dmi_available here and
> >> call it at subsys layer, where it's supposed to be.
> > I can't parse that, I suspect you wrote dmi_init where you actually
> > meant dmi_scan_machine? Given how early firmware_kobj is created, I
> > think the code currently in dmi_init could in fact go at the end of
> > dmi_scan_machine.
> 
> Actually, dmi_scan_machine can be called even earlier.
> As I've sad, for x86, it's called before firmware_kobj is created.
> 
> kernel_start()
>      setup_arch()
>          dmi_scan_machine()
> 
> And for firmware_init(), as you noticed already:
> 
> start_kernel()
>      rest_init()
>          kernel_init()
>              kernel_init_freeable()
>                  do_basic_setup()
>                      driver_init()
>                          firmware_init()
> 
> Pay attentions that setup_arch() is called much earlier than rest_init().
> So dmi_init couldn't in fact go at the end of dmi_scan_machine.

Yeah, you're right, sorry. Somehow I thought that setup_arch was an
arch_initcall, but it is not, so I got the order all wrong.

-- 
Jean Delvare
SUSE L3 Support

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

[Matt Fleming]

On Thu, 16 Apr, at 10:35:11AM, Jean Delvare wrote:
> 
> I don't really care who picks these patches up and sends them to Linus,
> but I think they should all follow the same route so that Linus has as
> little merge work to do as possible. So either you pick them all, or I
> do. If I do, you'll have to drop the 2 patches you have in efi-next.
> Again I'm fine either way, so please let me know what makes your life
> easier and let's do that.

As was mentioned by Ivan, the following patches have already been merged
by Linus,

  f617b0f32da2 ("firmware: dmi_scan: Use full dmi version for SMBIOS3")
  e4b1dec448af ("firmware: dmi_scan: Use direct access to static vars")

so no patches need to be dropped from my tree and there are no pending
DMI/SMBIOS patches in any of my branches.

How about you go ahead and collect all the patches and send them to
Linus? I'm happy to pickup any patches in the future if I'm explicitly
asked, but only if OK'd by you, Jean.

Sound OK to everyone?

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Jean Delvare]

Hi Matt,

Le Friday 17 April 2015 à 14:40 +0100, Matt Fleming a écrit :
> On Thu, 16 Apr, at 10:35:11AM, Jean Delvare wrote:
> > 
> > I don't really care who picks these patches up and sends them to Linus,
> > but I think they should all follow the same route so that Linus has as
> > little merge work to do as possible. So either you pick them all, or I
> > do. If I do, you'll have to drop the 2 patches you have in efi-next.
> > Again I'm fine either way, so please let me know what makes your life
> > easier and let's do that.
> 
> As was mentioned by Ivan, the following patches have already been merged
> by Linus,
> 
>   f617b0f32da2 ("firmware: dmi_scan: Use full dmi version for SMBIOS3")
>   e4b1dec448af ("firmware: dmi_scan: Use direct access to static vars")
> 
> so no patches need to be dropped from my tree and there are no pending
> DMI/SMBIOS patches in any of my branches.

Correct.

> How about you go ahead and collect all the patches and send them to
> Linus? I'm happy to pickup any patches in the future if I'm explicitly
> asked, but only if OK'd by you, Jean.
> 
> Sound OK to everyone?

Deal! :-)

-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

Hi Matt,

Le Friday 17 April 2015 à 14:40 +0100, Matt Fleming a écrit :
> On Thu, 16 Apr, at 10:35:11AM, Jean Delvare wrote:
> > 
> > I don't really care who picks these patches up and sends them to Linus,
> > but I think they should all follow the same route so that Linus has as
> > little merge work to do as possible. So either you pick them all, or I
> > do. If I do, you'll have to drop the 2 patches you have in efi-next.
> > Again I'm fine either way, so please let me know what makes your life
> > easier and let's do that.
> 
> As was mentioned by Ivan, the following patches have already been merged
> by Linus,
> 
>   f617b0f32da2 ("firmware: dmi_scan: Use full dmi version for SMBIOS3")
>   e4b1dec448af ("firmware: dmi_scan: Use direct access to static vars")
> 
> so no patches need to be dropped from my tree and there are no pending
> DMI/SMBIOS patches in any of my branches.

Correct.

> How about you go ahead and collect all the patches and send them to
> Linus? I'm happy to pickup any patches in the future if I'm explicitly
> asked, but only if OK'd by you, Jean.
> 
> Sound OK to everyone?

Deal! :-)

-- 
Jean Delvare
SUSE L3 Support


_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel