[Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs

[Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs

[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 subsystem 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 were
proposed by Jean Delvare.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
---

This patch is logical continuation of
"[dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute"
https://lkml.org/lkml/2015/2/4/475

Pay attention that this includes /sys/firmware/dmi for holding tables instead of
/sys/firmware/dmi/table as were proposed.

 Documentation/ABI/testing/sysfs-firmware-dmi       | 122 +++------------------
 .../ABI/testing/sysfs-firmware-dmi-entries         | 110 +++++++++++++++++++
 drivers/firmware/dmi-sysfs.c                       |  12 +-
 drivers/firmware/dmi_scan.c                        | 115 +++++++++++++++++--
 include/linux/dmi.h                                |   2 +
 5 files changed, 238 insertions(+), 123 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-entries

diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
index c78f9ab..6413128 100644
--- a/Documentation/ABI/testing/sysfs-firmware-dmi
+++ b/Documentation/ABI/testing/sysfs-firmware-dmi
@@ -1,110 +1,16 @@
 What:		/sys/firmware/dmi/
-Date:		February 2011
-Contact:	Mike Waychison <mikew@google.com>
+Date:		March 2015
+Contact:	Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
 Description:
-		Many machines' firmware (x86 and ia64) export DMI /
-		SMBIOS tables to the operating system.  Getting at this
-		information is often valuable to userland, especially in
-		cases where there are OEM extensions used.
-
-		The kernel itself does not rely on the majority of the
-		information in these tables being correct.  It equally
-		cannot ensure that the data as exported to userland is
-		without error either.
-
-		DMI is structured as a large table of entries, where
-		each entry has a common header indicating the type and
-		length of the entry, as well as a firmware-provided
-		'handle' that is supposed to be unique amongst all
-		entries.
-
-		Some entries are required by the specification, but many
-		others are optional.  In general though, users should
-		never expect to find a specific entry type on their
-		system unless they know for certain what their firmware
-		is doing.  Machine to machine experiences will vary.
-
-		Multiple entries of the same type are allowed.  In order
-		to handle these duplicate entry types, each entry is
-		assigned by the operating system an 'instance', which is
-		derived from an entry type's ordinal position.  That is
-		to say, if there are 'N' multiple entries with the same type
-		'T' in the DMI tables (adjacent or spread apart, it
-		doesn't matter), they will be represented in sysfs as
-		entries "T-0" through "T-(N-1)":
-
-		Example entry directories:
-
-			/sys/firmware/dmi/entries/17-0
-			/sys/firmware/dmi/entries/17-1
-			/sys/firmware/dmi/entries/17-2
-			/sys/firmware/dmi/entries/17-3
-			...
-
-		Instance numbers are used in lieu of the firmware
-		assigned entry handles as the kernel itself makes no
-		guarantees that handles as exported are unique, and
-		there are likely firmware images that get this wrong in
-		the wild.
-
-		Each DMI entry in sysfs has the common header values
-		exported as attributes:
-
-		handle	: The 16bit 'handle' that is assigned to this
-			  entry by the firmware.  This handle may be
-			  referred to by other entries.
-		length	: The length of the entry, as presented in the
-			  entry itself.  Note that this is _not the
-			  total count of bytes associated with the
-			  entry_.  This value represents the length of
-			  the "formatted" portion of the entry.  This
-			  "formatted" region is sometimes followed by
-			  the "unformatted" region composed of nul
-			  terminated strings, with termination signalled
-			  by a two nul characters in series.
-		raw	: The raw bytes of the entry. This includes the
-			  "formatted" portion of the entry, the
-			  "unformatted" strings portion of the entry,
-			  and the two terminating nul characters.
-		type	: The type of the entry.  This value is the same
-			  as found in the directory name.  It indicates
-			  how the rest of the entry should be interpreted.
-		instance: The instance ordinal of the entry for the
-			  given type.  This value is the same as found
-			  in the parent directory name.
-		position: The ordinal position (zero-based) of the entry
-			  within the entirety of the DMI entry table.
-
-		=== Entry Specialization ===
-
-		Some entry types may have other information available in
-		sysfs.  Not all types are specialized.
-
-		--- Type 15 - System Event Log ---
-
-		This entry allows the firmware to export a log of
-		events the system has taken.  This information is
-		typically backed by nvram, but the implementation
-		details are abstracted by this table.  This entry's data
-		is exported in the directory:
-
-		/sys/firmware/dmi/entries/15-0/system_event_log
-
-		and has the following attributes (documented in the
-		SMBIOS / DMI specification under "System Event Log (Type 15)":
-
-		area_length
-		header_start_offset
-		data_start_offset
-		access_method
-		status
-		change_token
-		access_method_address
-		header_format
-		per_log_type_descriptor_length
-		type_descriptors_supported_count
-
-		As well, the kernel exports the binary attribute:
-
-		raw_event_log	: The raw binary bits of the event log
-				  as described by the DMI entry.
+		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.
+		That's why SMBIOS entry point is represented in dmi sysfs
+		like a raw attribute and is accessible via
+		/sys/firmware/dmi/smbios_entry_point. The format of SMBIOS
+		entry point header can be read in SMBIOS specification.
+		To simplify access and processing delay in user space,
+		subsystem provides also raw dmi table under
+		/sys/firmware/dmi/dmi_table.
diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-entries b/Documentation/ABI/testing/sysfs-firmware-dmi-entries
new file mode 100644
index 0000000..c3b4d4c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-dmi-entries
@@ -0,0 +1,110 @@
+What:		/sys/firmware/dmi/entries
+Date:		February 2011
+Contact:	Mike Waychison <mikew@google.com>
+Description:
+		Many machines' firmware (x86 and ia64) export DMI /
+		SMBIOS tables to the operating system.  Getting at this
+		information is often valuable to userland, especially in
+		cases where there are OEM extensions used.
+
+		The kernel itself does not rely on the majority of the
+		information in these tables being correct.  It equally
+		cannot ensure that the data as exported to userland is
+		without error either.
+
+		DMI is structured as a large table of entries, where
+		each entry has a common header indicating the type and
+		length of the entry, as well as a firmware-provided
+		'handle' that is supposed to be unique amongst all
+		entries.
+
+		Some entries are required by the specification, but many
+		others are optional.  In general though, users should
+		never expect to find a specific entry type on their
+		system unless they know for certain what their firmware
+		is doing.  Machine to machine experiences will vary.
+
+		Multiple entries of the same type are allowed.  In order
+		to handle these duplicate entry types, each entry is
+		assigned by the operating system an 'instance', which is
+		derived from an entry type's ordinal position.  That is
+		to say, if there are 'N' multiple entries with the same type
+		'T' in the DMI tables (adjacent or spread apart, it
+		doesn't matter), they will be represented in sysfs as
+		entries "T-0" through "T-(N-1)":
+
+		Example entry directories:
+
+			/sys/firmware/dmi/entries/17-0
+			/sys/firmware/dmi/entries/17-1
+			/sys/firmware/dmi/entries/17-2
+			/sys/firmware/dmi/entries/17-3
+			...
+
+		Instance numbers are used in lieu of the firmware
+		assigned entry handles as the kernel itself makes no
+		guarantees that handles as exported are unique, and
+		there are likely firmware images that get this wrong in
+		the wild.
+
+		Each DMI entry in sysfs has the common header values
+		exported as attributes:
+
+		handle	: The 16bit 'handle' that is assigned to this
+			  entry by the firmware.  This handle may be
+			  referred to by other entries.
+		length	: The length of the entry, as presented in the
+			  entry itself.  Note that this is _not the
+			  total count of bytes associated with the
+			  entry_.  This value represents the length of
+			  the "formatted" portion of the entry.  This
+			  "formatted" region is sometimes followed by
+			  the "unformatted" region composed of nul
+			  terminated strings, with termination signalled
+			  by a two nul characters in series.
+		raw	: The raw bytes of the entry. This includes the
+			  "formatted" portion of the entry, the
+			  "unformatted" strings portion of the entry,
+			  and the two terminating nul characters.
+		type	: The type of the entry.  This value is the same
+			  as found in the directory name.  It indicates
+			  how the rest of the entry should be interpreted.
+		instance: The instance ordinal of the entry for the
+			  given type.  This value is the same as found
+			  in the parent directory name.
+		position: The ordinal position (zero-based) of the entry
+			  within the entirety of the DMI entry table.
+
+		=== Entry Specialization ===
+
+		Some entry types may have other information available in
+		sysfs.  Not all types are specialized.
+
+		--- Type 15 - System Event Log ---
+
+		This entry allows the firmware to export a log of
+		events the system has taken.  This information is
+		typically backed by nvram, but the implementation
+		details are abstracted by this table.  This entry's data
+		is exported in the directory:
+
+		/sys/firmware/dmi/entries/15-0/system_event_log
+
+		and has the following attributes (documented in the
+		SMBIOS / DMI specification under "System Event Log (Type 15)":
+
+		area_length
+		header_start_offset
+		data_start_offset
+		access_method
+		status
+		change_token
+		access_method_address
+		header_format
+		per_log_type_descriptor_length
+		type_descriptors_supported_count
+
+		As well, the kernel exports the binary attribute:
+
+		raw_event_log	: The raw binary bits of the event log
+				  as described by the DMI entry.
diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index e0f1cb3..390067d 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,10 @@ 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)
-		goto err;
+	if (!dmi_kobj) {
+		pr_err("dmi-sysfs: dmi subsysterm is absent.\n");
+		return -EINVAL;
+	}
 
 	dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
 	if (!dmi_kset)
@@ -675,7 +674,6 @@ static int __init dmi_sysfs_init(void)
 err:
 	cleanup_entry_list();
 	kset_unregister(dmi_kset);
-	kobject_put(dmi_kobj);
 	return error;
 }
 
@@ -685,8 +683,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 c9cb725..3fca52a 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_table(u8 *buf,
 }
 
 static phys_addr_t dmi_base;
+static u8 *dmi_tb;
 
 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,8 @@ 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, 15);
 				dmi_ver = (buf[14] & 0xF0) << 4 |
 					   (buf[14] & 0x0F);
 				pr_info("Legacy DMI %d.%d present.\n",
@@ -535,6 +546,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 +651,95 @@ void __init dmi_scan_machine(void)
 	dmi_initialized = 1;
 }
 
+static ssize_t smbios_entry_point_read(struct file *filp,
+				       struct kobject *kobj,
+				       struct bin_attribute *bin_attr,
+				       char *buf, loff_t pos, size_t count)
+{
+	ssize_t size;
+
+	size = bin_attr->size;
+
+	if (size > pos)
+		size -= pos;
+	else
+		return 0;
+
+	if (count < size)
+		size = count;
+
+	memcpy(buf, &smbios_entry_point[pos], size);
+
+	return size;
+}
+
+static ssize_t dmi_table_read(struct file *filp,
+			      struct kobject *kobj,
+			      struct bin_attribute *bin_attr,
+			      char *buf, loff_t pos, size_t count)
+{
+	ssize_t size;
+
+	size = bin_attr->size;
+
+	if (size > pos)
+		size -= pos;
+	else
+		return 0;
+
+	if (count < size)
+		size = count;
+
+	memcpy(buf, &dmi_tb[pos], size);
+
+	return size;
+}
+
+BIN_ATTR_RO(dmi_table, 0);
+BIN_ATTR_RO(smbios_entry_point, 0);
+
+/*
+ * Register the dmi subsystem under the firmware subsysterm
+ */
+static int __init dmisubsys_init(void)
+{
+	int ret = -ENOMEM;
+
+	if (!smbios_entry_point_size || !dmi_available) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	/* Set up dmi directory at /sys/firmware/dmi */
+	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
+	if (!dmi_kobj)
+		goto err;
+
+	bin_attr_smbios_entry_point.size = smbios_entry_point_size;
+	ret = sysfs_create_bin_file(dmi_kobj, &bin_attr_smbios_entry_point);
+	if (ret)
+		goto err;
+
+	if (!dmi_tb) {
+		dmi_tb = dmi_remap(dmi_base, dmi_len);
+		if (!dmi_tb)
+			goto err;
+	}
+
+	bin_attr_dmi_table.size = dmi_len;
+	ret = sysfs_create_bin_file(dmi_kobj, &bin_attr_dmi_table);
+	if (ret)
+		goto err;
+
+	return 0;
+err:
+	pr_err("dmi: Firmware registration failed.\n");
+	kobject_del(dmi_kobj);
+	kobject_put(dmi_kobj);
+	return ret;
+}
+subsys_initcall(dmisubsys_init);
+
 /**
  * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
  *
@@ -897,18 +999,17 @@ EXPORT_SYMBOL(dmi_get_date);
 int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	     void *private_data)
 {
-	u8 *buf;
-
 	if (!dmi_available)
 		return -1;
 
-	buf = dmi_remap(dmi_base, dmi_len);
-	if (buf == NULL)
-		return -1;
+	if (!dmi_tb) {
+		dmi_tb = dmi_remap(dmi_base, dmi_len);
+		if (!dmi_tb)
+			return -1;
+	}
 
-	dmi_table(buf, decode, private_data);
+	dmi_table(dmi_tb, decode, private_data);
 
-	dmi_unmap(buf);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(dmi_walk);
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index f820f0a..316293e 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);
@@ -112,6 +113,7 @@ extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
 
 #else
 
+extern struct kobject *dmi_kobj;
 static inline int dmi_check_system(const struct dmi_system_id *list) { return 0; }
 static inline const char * dmi_get_system_info(int field) { return NULL; }
 static inline const struct dmi_device * dmi_find_device(int type, const char *name,
-- 
1.9.1

Re: [Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs

[Jean Delvare]

Hi Ivan,

Le Monday 16 March 2015 à 22:57 +0200, Ivan Khoronzhuk a écrit :
> 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 subsystem 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 were
> proposed by Jean Delvare.

"as was proposed" or even "as proposed" sounds more correct.

BTW, which tree is your patch based on? I can't get it to apply cleanly
on top of any kernel version I tried. I adjusted the patch to my tree
but it means I'm not reviewing your code exactly. Please send patches
which can be applied on top of some recent vanilla tree (3.19 or 4.0-rc4
would be OK at the moment.)

> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
> ---
> 
> This patch is logical continuation of
> "[dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute"
> https://lkml.org/lkml/2015/2/4/475
> 
> Pay attention that this includes /sys/firmware/dmi for holding tables instead of
> /sys/firmware/dmi/table as were proposed.

Why is that? I proposed /sys/firmware/dmi/tables because the acpi
subsystem puts its own tables in /sys/firmware/acpi/tables. It seemed
reasonable to follow that example for consistency. What is your reason
for doing it differently?

>  Documentation/ABI/testing/sysfs-firmware-dmi       | 122 +++------------------
>  .../ABI/testing/sysfs-firmware-dmi-entries         | 110 +++++++++++++++++++

This is adding a lot of noise to your patch, making it harder to review
and backport. If you think that the contents of sysfs-firmware-dmi would
rather go in a documentation file named sysfs-firmware-dmi-entries, I
have no objection, but it should be done in a separate patch.

>  drivers/firmware/dmi-sysfs.c                       |  12 +-
>  drivers/firmware/dmi_scan.c                        | 115 +++++++++++++++++--
>  include/linux/dmi.h                                |   2 +
>  5 files changed, 238 insertions(+), 123 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-entries
> 
> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
> index c78f9ab..6413128 100644
> --- a/Documentation/ABI/testing/sysfs-firmware-dmi
> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi
> @@ -1,110 +1,16 @@
>  What:		/sys/firmware/dmi/
> -Date:		February 2011
> -Contact:	Mike Waychison <mikew@google.com>
> +Date:		March 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.
> +		That's why SMBIOS entry point is represented in dmi sysfs
> +		like a raw attribute and is accessible via
> +		/sys/firmware/dmi/smbios_entry_point. The format of SMBIOS
> +		entry point header can be read in SMBIOS specification.
> +		To simplify access and processing delay in user space,

"processing delay" doesn't really describe the problem that was present
with the previous patch set. It was more a problem of processing time,
CPU and memory usage, and code complexity. Anyway I see no reason to
explain that here, given that this proposal was rejected.

You'd rather just explain that the raw data is provided through sysfs as
an alternative to utilities reading them from /dev/mem (as you already
correctly explain in the patch description.) That's what your new patch
is all about.

> +		subsystem provides also raw dmi table under

DMI better spelled capitalized. I'd also avoid mentioning "subsystem",
I'm not sure why you're using that word (and also in the subject), sysfs
is a virtual file system, and there is no such thing as a "dmi
subsystem".

> +		/sys/firmware/dmi/dmi_table.

As said before I'd make it /sys/firmware/dmi/tables/DMI to mimic what
acpi does.

> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
> index e0f1cb3..390067d 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,10 @@ 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)
> -		goto err;
> +	if (!dmi_kobj) {
> +		pr_err("dmi-sysfs: dmi subsysterm is absent.\n");

Typo: subsystem. Then again the use of this word keeps confusing me, but
it's a minor thing.

> +		return -EINVAL;

The original code had a single error path and I see no reason to change
that. -EINVAL seems inappropriate for this case, I'd rather return
-ENOSYS.

> +	}
>  
>  	dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>  	if (!dmi_kset)
> @@ -675,7 +674,6 @@ static int __init dmi_sysfs_init(void)
>  err:
>  	cleanup_entry_list();
>  	kset_unregister(dmi_kset);
> -	kobject_put(dmi_kobj);
>  	return error;
>  }
>  
> @@ -685,8 +683,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 c9cb725..3fca52a 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_table(u8 *buf,
>  }
>  
>  static phys_addr_t dmi_base;
> +static u8 *dmi_tb;

Where "tb" stands for... "table", but you couldn't use that because a
function by that name already exist, right? I can think of two less
cryptic ways to solve this: either you rename function dmi_table to,
say, dmi_decode_table (which would be a better name anyway IMHO), or you
name your variable dmi_table_p or dmi_raw_table. But "tb" is not
immediate enough to understand.

>  
>  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,8 @@ 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, 15);
>  				dmi_ver = (buf[14] & 0xF0) << 4 |
>  					   (buf[14] & 0x0F);
>  				pr_info("Legacy DMI %d.%d present.\n",
> @@ -535,6 +546,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

This is inconsistent. You should either use smbios_entry_point_size as
the last argument to memcpy() in all 3 cases (my preference), or you
should use buf[5], 15 and buf[6] respectively, but not mix.

> @@ -638,6 +651,95 @@ void __init dmi_scan_machine(void)
>  	dmi_initialized = 1;
>  }
>  
> +static ssize_t smbios_entry_point_read(struct file *filp,
> +				       struct kobject *kobj,
> +				       struct bin_attribute *bin_attr,
> +				       char *buf, loff_t pos, size_t count)
> +{
> +	ssize_t size;
> +
> +	size = bin_attr->size;
> +
> +	if (size > pos)
> +		size -= pos;
> +	else
> +		return 0;
> +
> +	if (count < size)
> +		size = count;
> +
> +	memcpy(buf, &smbios_entry_point[pos], size);
> +
> +	return size;
> +}
> +
> +static ssize_t dmi_table_read(struct file *filp,
> +			      struct kobject *kobj,
> +			      struct bin_attribute *bin_attr,
> +			      char *buf, loff_t pos, size_t count)
> +{
> +	ssize_t size;
> +
> +	size = bin_attr->size;
> +
> +	if (size > pos)
> +		size -= pos;
> +	else
> +		return 0;
> +
> +	if (count < size)
> +		size = count;
> +
> +	memcpy(buf, &dmi_tb[pos], size);
> +
> +	return size;
> +}

Looking at drivers/acpi/bgrt.c, there seems to be a more simple way to
implement this: fill in the file size and contents (.private) at
run-time and let sysfs handle all the rest. You already do the former so
you're actually very close. 

> +BIN_ATTR_RO(dmi_table, 0);
> +BIN_ATTR_RO(smbios_entry_point, 0);

These should both be static.

This will make dmi_table readable by all users, instead of just root as
it should be. The DMI data contains private information (serial numbers,
UUID...) You will see that some files under /sys/devices/virtual/dmi/id
can't be read by regular users for this reason.

> +/*
> + * Register the dmi subsystem under the firmware subsysterm

Same typo again: subsystem.

> + */
> +static int __init dmisubsys_init(void)
> +{
> +	int ret = -ENOMEM;

There's only one error case which returns that value so it would be
better set in that error path.

> +
> +	if (!smbios_entry_point_size || !dmi_available) {

I already mentioned in a previous review that I don't think you need to
check for !dmi_available, and you said you agreed.

> +		ret = -EINVAL;

Again -ENOSYS or similar would be more appropriate (not that it matters
a lot in practice though as I don't think there is any way to actually
retrieve this value.)

> +		goto err;
> +	}
> +
> +	/* Set up dmi directory at /sys/firmware/dmi */
> +	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> +	if (!dmi_kobj)
> +		goto err;
> +
> +	bin_attr_smbios_entry_point.size = smbios_entry_point_size;
> +	ret = sysfs_create_bin_file(dmi_kobj, &bin_attr_smbios_entry_point);
> +	if (ret)
> +		goto err;
> +
> +	if (!dmi_tb) {

I don't like this. You should know which of this function and dmi_walk()
is called first. If you don't, then how can you guarantee that a race
condition can't happen?

This makes me wonder if that code wouldn't rather go in
dmi_scan_machine() rather than a separate function.

> +		dmi_tb = dmi_remap(dmi_base, dmi_len);
> +		if (!dmi_tb)
> +			goto err;
> +	}
> +
> +	bin_attr_dmi_table.size = dmi_len;
> +	ret = sysfs_create_bin_file(dmi_kobj, &bin_attr_dmi_table);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +err:
> +	pr_err("dmi: Firmware registration failed.\n");

I said in a previous review that files that have been created should be
explicitly deleted, and I still think so.

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

I think you also need to explicitly set dmi_kobj to NULL here, otherwise
dmi-sysfs will try to use an object which no longer exists.

An alternative approach would be to leave dmi_kobj available even if the
binary files could not be created. As this case is very unlikely to
happen, I don't care which way you choose.

> +	return ret;
> +}
> +subsys_initcall(dmisubsys_init);
> +
>  /**
>   * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
>   *
> @@ -897,18 +999,17 @@ EXPORT_SYMBOL(dmi_get_date);
>  int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>  	     void *private_data)
>  {
> -	u8 *buf;
> -
>  	if (!dmi_available)
>  		return -1;
>  
> -	buf = dmi_remap(dmi_base, dmi_len);
> -	if (buf == NULL)
> -		return -1;
> +	if (!dmi_tb) {
> +		dmi_tb = dmi_remap(dmi_base, dmi_len);
> +		if (!dmi_tb)
> +			return -1;
> +	}
>  
> -	dmi_table(buf, decode, private_data);
> +	dmi_table(dmi_tb, decode, private_data);
>  
> -	dmi_unmap(buf);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(dmi_walk);
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index f820f0a..316293e 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);
> @@ -112,6 +113,7 @@ extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
>  
>  #else
>  
> +extern struct kobject *dmi_kobj;

No, if CONFIG_DMI is not set then dmi_scan isn't built and dmi_kobj does
not exist.

>  static inline int dmi_check_system(const struct dmi_system_id *list) { return 0; }
>  static inline const char * dmi_get_system_info(int field) { return NULL; }
>  static inline const struct dmi_device * dmi_find_device(int type, const char *name,


-- 
Jean Delvare
SUSE L3 Support

Re: [Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs

[Ivan.khoronzhuk]

On 19.03.15 17:30, Jean Delvare wrote:
> Hi Ivan,
>
> Le Monday 16 March 2015 à 22:57 +0200, Ivan Khoronzhuk a écrit :
>> 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 subsystem 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 were
>> proposed by Jean Delvare.
> "as was proposed" or even "as proposed" sounds more correct.
>
> BTW, which tree is your patch based on? I can't get it to apply cleanly
> on top of any kernel version I tried. I adjusted the patch to my tree
> but it means I'm not reviewing your code exactly. Please send patches
> which can be applied on top of some recent vanilla tree (3.19 or 4.0-rc4
> would be OK at the moment.)

Oh, sorry I forgot to mention, it's based on efi/next, but with consumption
that Matt will remove old version:
  85c83ea firmware: dmi-sysfs: Add SMBIOS entry point area attribute

As you remember, your propositions were slightly late,
and patch had been applied on efi/next when you commented.

Matt, Could you please remove old version.
I hope it will be replaced by new one.

>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
>> ---
>>
>> This patch is logical continuation of
>> "[dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute"
>> https://lkml.org/lkml/2015/2/4/475
>>
>> Pay attention that this includes /sys/firmware/dmi for holding tables instead of
>> /sys/firmware/dmi/table as were proposed.
> Why is that? I proposed /sys/firmware/dmi/tables because the acpi
> subsystem puts its own tables in /sys/firmware/acpi/tables. It seemed
> reasonable to follow that example for consistency. What is your reason
> for doing it differently?

I just like it more instead of catalog/catalog/catalog...
But it's only proposition. Let's it be under tables.

>
>>   Documentation/ABI/testing/sysfs-firmware-dmi       | 122 +++------------------
>>   .../ABI/testing/sysfs-firmware-dmi-entries         | 110 +++++++++++++++++++
> This is adding a lot of noise to your patch, making it harder to review
> and backport. If you think that the contents of sysfs-firmware-dmi would
> rather go in a documentation file named sysfs-firmware-dmi-entries, I
> have no objection, but it should be done in a separate patch.

yes.

>>   drivers/firmware/dmi-sysfs.c                       |  12 +-
>>   drivers/firmware/dmi_scan.c                        | 115 +++++++++++++++++--
>>   include/linux/dmi.h                                |   2 +
>>   5 files changed, 238 insertions(+), 123 deletions(-)
>>   create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-entries
>>
>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
>> index c78f9ab..6413128 100644
>> --- a/Documentation/ABI/testing/sysfs-firmware-dmi
>> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi
>> @@ -1,110 +1,16 @@
>>   What:		/sys/firmware/dmi/
>> -Date:		February 2011
>> -Contact:	Mike Waychison <mikew@google.com>
>> +Date:		March 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.
>> +		That's why SMBIOS entry point is represented in dmi sysfs
>> +		like a raw attribute and is accessible via
>> +		/sys/firmware/dmi/smbios_entry_point. The format of SMBIOS
>> +		entry point header can be read in SMBIOS specification.
>> +		To simplify access and processing delay in user space,
> "processing delay" doesn't really describe the problem that was present
> with the previous patch set. It was more a problem of processing time,
> CPU and memory usage, and code complexity. Anyway I see no reason to
> explain that here, given that this proposal was rejected.
>
> You'd rather just explain that the raw data is provided through sysfs as
> an alternative to utilities reading them from /dev/mem (as you already
> correctly explain in the patch description.) That's what your new patch
> is all about.

yes

>
>> +		subsystem provides also raw dmi table under
> DMI better spelled capitalized. I'd also avoid mentioning "subsystem",
> I'm not sure why you're using that word (and also in the subject), sysfs
> is a virtual file system, and there is no such thing as a "dmi

dmi -> DMI.
According subsystem, it was erroneously noticed from efi.c.
It seems confusing to me too, let avoid it.

> subsystem".
>
>> +		/sys/firmware/dmi/dmi_table.
> As said before I'd make it /sys/firmware/dmi/tables/DMI to mimic what
> acpi does.
>
>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>> index e0f1cb3..390067d 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,10 @@ 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)
>> -		goto err;
>> +	if (!dmi_kobj) {
>> +		pr_err("dmi-sysfs: dmi subsysterm is absent.\n");
> Typo: subsystem. Then again the use of this word keeps confusing me, but
> it's a minor thing.
>
>> +		return -EINVAL;
> The original code had a single error path and I see no reason to change
> that. -EINVAL seems inappropriate for this case, I'd rather return
> -ENOSYS.

ENOSYS better.

>
>> +	}
>>   
>>   	dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>>   	if (!dmi_kset)
>> @@ -675,7 +674,6 @@ static int __init dmi_sysfs_init(void)
>>   err:
>>   	cleanup_entry_list();
>>   	kset_unregister(dmi_kset);
>> -	kobject_put(dmi_kobj);
>>   	return error;
>>   }
>>   
>> @@ -685,8 +683,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 c9cb725..3fca52a 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_table(u8 *buf,
>>   }
>>   
>>   static phys_addr_t dmi_base;
>> +static u8 *dmi_tb;
> Where "tb" stands for... "table", but you couldn't use that because a
> function by that name already exist, right? I can think of two less
> cryptic ways to solve this: either you rename function dmi_table to,
> say, dmi_decode_table (which would be a better name anyway IMHO), or you
> name your variable dmi_table_p or dmi_raw_table. But "tb" is not
> immediate enough to understand.

If others are OK, I'll better rename dmi_table function to 
dmi_decode_table()
(or maybe dmidecode_table :), joke)
as it's more appropriate to what it's doing.
And accordingly dmi_tb to 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,8 @@ 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, 15);
>>   				dmi_ver = (buf[14] & 0xF0) << 4 |
>>   					   (buf[14] & 0x0F);
>>   				pr_info("Legacy DMI %d.%d present.\n",
>> @@ -535,6 +546,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
> This is inconsistent. You should either use smbios_entry_point_size as
> the last argument to memcpy() in all 3 cases (my preference), or you
> should use buf[5], 15 and buf[6] respectively, but not mix.

You probably meant "memcpy(smbios_entry_point, buf, 15)"
Just wanted to avoid redundant line break.

>
>> @@ -638,6 +651,95 @@ void __init dmi_scan_machine(void)
>>   	dmi_initialized = 1;
>>   }
>>   
>> +static ssize_t smbios_entry_point_read(struct file *filp,
>> +				       struct kobject *kobj,
>> +				       struct bin_attribute *bin_attr,
>> +				       char *buf, loff_t pos, size_t count)
>> +{
>> +	ssize_t size;
>> +
>> +	size = bin_attr->size;
>> +
>> +	if (size > pos)
>> +		size -= pos;
>> +	else
>> +		return 0;
>> +
>> +	if (count < size)
>> +		size = count;
>> +
>> +	memcpy(buf, &smbios_entry_point[pos], size);
>> +
>> +	return size;
>> +}
>> +
>> +static ssize_t dmi_table_read(struct file *filp,
>> +			      struct kobject *kobj,
>> +			      struct bin_attribute *bin_attr,
>> +			      char *buf, loff_t pos, size_t count)
>> +{
>> +	ssize_t size;
>> +
>> +	size = bin_attr->size;
>> +
>> +	if (size > pos)
>> +		size -= pos;
>> +	else
>> +		return 0;
>> +
>> +	if (count < size)
>> +		size = count;
>> +
>> +	memcpy(buf, &dmi_tb[pos], size);
>> +
>> +	return size;
>> +}
> Looking at drivers/acpi/bgrt.c, there seems to be a more simple way to
> implement this: fill in the file size and contents (.private) at
> run-time and let sysfs handle all the rest. You already do the former so
> you're actually very close.

I'll look.

>
>> +BIN_ATTR_RO(dmi_table, 0);
>> +BIN_ATTR_RO(smbios_entry_point, 0);
> These should both be static.

Yes.

>
> This will make dmi_table readable by all users, instead of just root as
> it should be. The DMI data contains private information (serial numbers,
> UUID...) You will see that some files under /sys/devices/virtual/dmi/id
> can't be read by regular users for this reason.

Yes.

>
>> +/*
>> + * Register the dmi subsystem under the firmware subsysterm
> Same typo again: subsystem.
>
>> + */
>> +static int __init dmisubsys_init(void)
>> +{
>> +	int ret = -ENOMEM;
> There's only one error case which returns that value so it would be
> better set in that error path.

yep.

>
>> +
>> +	if (!smbios_entry_point_size || !dmi_available) {
> I already mentioned in a previous review that I don't think you need to
> check for !dmi_available, and you said you agreed.

No.
Previously only smbios entry point was exported.
Now DMI table was added.
smbios_entry_point_size doesn't mean DMI table is present.


>
>> +		ret = -EINVAL;
> Again -ENOSYS or similar would be more appropriate (not that it matters
> a lot in practice though as I don't think there is any way to actually
> retrieve this value.)

ENOSYS better

>
>> +		goto err;
>> +	}
>> +
>> +	/* Set up dmi directory at /sys/firmware/dmi */
>> +	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> +	if (!dmi_kobj)
>> +		goto err;
>> +
>> +	bin_attr_smbios_entry_point.size = smbios_entry_point_size;
>> +	ret = sysfs_create_bin_file(dmi_kobj, &bin_attr_smbios_entry_point);
>> +	if (ret)
>> +		goto err;
>> +
>> +	if (!dmi_tb) {
> I don't like this. You should know which of this function and dmi_walk()
> is called first. If you don't, then how can you guarantee that a race
> condition can't happen?

Shit.
Maybe better to leave dmi_walk as it was
and map it only here.

>
> This makes me wonder if that code wouldn't rather go in
> dmi_scan_machine() rather than a separate function.
>
>> +		dmi_tb = dmi_remap(dmi_base, dmi_len);
>> +		if (!dmi_tb)
>> +			goto err;
>> +	}
>> +
>> +	bin_attr_dmi_table.size = dmi_len;
>> +	ret = sysfs_create_bin_file(dmi_kobj, &bin_attr_dmi_table);
>> +	if (ret)
>> +		goto err;
>> +
>> +	return 0;
>> +err:
>> +	pr_err("dmi: Firmware registration failed.\n");
> n
> I said in a previous review that files that have been created should be
> explicitly deleted, and I still think so.

I dislike it, but if you insist I'll do this.

>
>> +	kobject_del(dmi_kobj);
>> +	kobject_put(dmi_kobj);
> I think you also need to explicitly set dmi_kobj to NULL here, otherwise
> dmi-sysfs will try to use an object which no longer exists.

Yes.

>
> An alternative approach would be to leave dmi_kobj available even if the
> binary files could not be created. As this case is very unlikely to
> happen, I don't care which way you choose.

I also thought about such approach. But imaged a situation when DMI 
catalog is
empty and no one uses dmi-sysfs (which probably is more frequently), decided
to delete it. So dmi_kobj = 0.

>
>> +	return ret;
>> +}
>> +subsys_initcall(dmisubsys_init);
>> +
>>   /**
>>    * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
>>    *
>> @@ -897,18 +999,17 @@ EXPORT_SYMBOL(dmi_get_date);
>>   int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>>   	     void *private_data)
>>   {
>> -	u8 *buf;
>> -
>>   	if (!dmi_available)
>>   		return -1;
>>   
>> -	buf = dmi_remap(dmi_base, dmi_len);
>> -	if (buf == NULL)
>> -		return -1;
>> +	if (!dmi_tb) {
>> +		dmi_tb = dmi_remap(dmi_base, dmi_len);
>> +		if (!dmi_tb)
>> +			return -1;
>> +	}
>>   
>> -	dmi_table(buf, decode, private_data);
>> +	dmi_table(dmi_tb, decode, private_data);
>>   
>> -	dmi_unmap(buf);
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(dmi_walk);
>> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
>> index f820f0a..316293e 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);
>> @@ -112,6 +113,7 @@ extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
>>   
>>   #else
>>   
>> +extern struct kobject *dmi_kobj;
> No, if CONFIG_DMI is not set then dmi_scan isn't built and dmi_kobj does
> not exist.

Yep

>
>>   static inline int dmi_check_system(const struct dmi_system_id *list) { return 0; }
>>   static inline const char * dmi_get_system_info(int field) { return NULL; }
>>   static inline const struct dmi_device * dmi_find_device(int type, const char *name,
>

-- 
Regards,
Ivan Khoronzhuk

Re: [Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs

[Jean Delvare]

Hi Ivan,

On Thu, 19 Mar 2015 19:35:34 +0200, Ivan.khoronzhuk wrote:
> On 19.03.15 17:30, Jean Delvare wrote:
> > Le Monday 16 March 2015 à 22:57 +0200, Ivan Khoronzhuk a écrit :
> >> 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 subsystem 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 were
> >> proposed by Jean Delvare.
> > "as was proposed" or even "as proposed" sounds more correct.
> >
> > BTW, which tree is your patch based on? I can't get it to apply cleanly
> > on top of any kernel version I tried. I adjusted the patch to my tree
> > but it means I'm not reviewing your code exactly. Please send patches
> > which can be applied on top of some recent vanilla tree (3.19 or 4.0-rc4
> > would be OK at the moment.)
> 
> Oh, sorry I forgot to mention, it's based on efi/next, but with consumption
> that Matt will remove old version:
>   85c83ea firmware: dmi-sysfs: Add SMBIOS entry point area attribute
> 
> As you remember, your propositions were slightly late,
> and patch had been applied on efi/next when you commented.

OK, I understand. Now I see the two patches I was missing, I'll pick
them so that I can apply your patch cleanly on top of my tree.

I would have appreciated to be Cc'd on these two patches, so I knew
they existed, and maybe I could even have commented on them.

> >> (...)
> >> @@ -118,6 +124,7 @@ static void dmi_table(u8 *buf,
> >>   }
> >>   
> >>   static phys_addr_t dmi_base;
> >> +static u8 *dmi_tb;
> > Where "tb" stands for... "table", but you couldn't use that because a
> > function by that name already exist, right? I can think of two less
> > cryptic ways to solve this: either you rename function dmi_table to,
> > say, dmi_decode_table (which would be a better name anyway IMHO), or you
> > name your variable dmi_table_p or dmi_raw_table. But "tb" is not
> > immediate enough to understand.
> 
> If others are OK, I'll better rename dmi_table function to 
> dmi_decode_table()
> (or maybe dmidecode_table :), joke)
> as it's more appropriate to what it's doing.
> And accordingly dmi_tb to dmi_table.

Yes, that's fine with me. The function rename should be a separate
patch for clarity.

> >> (...)
> >>   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,8 @@ 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, 15);
> >>   				dmi_ver = (buf[14] & 0xF0) << 4 |
> >>   					   (buf[14] & 0x0F);
> >>   				pr_info("Legacy DMI %d.%d present.\n",
> >> @@ -535,6 +546,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
> > This is inconsistent. You should either use smbios_entry_point_size as
> > the last argument to memcpy() in all 3 cases (my preference), or you
> > should use buf[5], 15 and buf[6] respectively, but not mix.
> 
> You probably meant "memcpy(smbios_entry_point, buf, 15)"

Yes.

> Just wanted to avoid redundant line break.

Line breaks are just fine, code consistency is more important.

> >> (...)
> >> +	if (!smbios_entry_point_size || !dmi_available) {
> > I already mentioned in a previous review that I don't think you need to
> > check for !dmi_available, and you said you agreed.
> 
> No.
> Previously only smbios entry point was exported.
> Now DMI table was added.
> smbios_entry_point_size doesn't mean DMI table is present.

Thanks for the explanation, I understand the reasoning now. I can see
how smbios_entry_point_size could be set while dmi_available would
not. But I can't see how dmi_available could be set and
smbios_entry_point_size would not. So I think checking for
dmi_available is enough?

> >> (...)
> >> +		goto err;
> >> +	}
> >> +
> >> +	/* Set up dmi directory at /sys/firmware/dmi */
> >> +	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> >> +	if (!dmi_kobj)
> >> +		goto err;
> >> +
> >> +	bin_attr_smbios_entry_point.size = smbios_entry_point_size;
> >> +	ret = sysfs_create_bin_file(dmi_kobj, &bin_attr_smbios_entry_point);
> >> +	if (ret)
> >> +		goto err;
> >> +
> >> +	if (!dmi_tb) {
> > I don't like this. You should know which of this function and dmi_walk()
> > is called first. If you don't, then how can you guarantee that a race
> > condition can't happen?
> 
> Shit.
> Maybe better to leave dmi_walk as it was
> and map it only here.

For the time being, yes, that might be better. We can always optimize
it later in a separate patch.

> > This makes me wonder if that code wouldn't rather go in
> > dmi_scan_machine() rather than a separate function.

> >> (...)
> >> +		dmi_tb = dmi_remap(dmi_base, dmi_len);
> >> +		if (!dmi_tb)
> >> +			goto err;
> >> +	}
> >> +
> >> +	bin_attr_dmi_table.size = dmi_len;
> >> +	ret = sysfs_create_bin_file(dmi_kobj, &bin_attr_dmi_table);
> >> +	if (ret)
> >> +		goto err;
> >> +
> >> +	return 0;
> >> +err:
> >> +	pr_err("dmi: Firmware registration failed.\n");
> >
> > I said in a previous review that files that have been created should be
> > explicitly deleted, and I still think so.
> 
> I dislike it, but if you insist I'll do this.

I'm only repeating something I was told myself when submitting that
kind of code long ago. Almost all other drivers seem to be cleaning up
such files explicitly, just look at the firmware drivers, efivars for
example.

If you don't want to do it, I don't really mind, that's an error path
that most probably nobody will ever walk unless explicitly testing it.
But then if something breaks, you'll be asked to fix it.

> >> (...)
> >> +	kobject_del(dmi_kobj);
> >> +	kobject_put(dmi_kobj);
> > I think you also need to explicitly set dmi_kobj to NULL here, otherwise
> > dmi-sysfs will try to use an object which no longer exists.
> 
> Yes.
> 
> > An alternative approach would be to leave dmi_kobj available even if the
> > binary files could not be created. As this case is very unlikely to
> > happen, I don't care which way you choose.
> 
> I also thought about such approach. But imaged a situation when DMI 
> catalog is
> empty and no one uses dmi-sysfs (which probably is more frequently), decided
> to delete it. So dmi_kobj = 0.

Fine with me (but = NULL, not = 0.)

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs

[Jean Delvare]

On Fri, 20 Mar 2015 12:00:34 +0000, Matt Fleming wrote:
> On Fri, 2015-03-20 at 09:16 +0100, Jean Delvare wrote:
> > 
> > OK, I understand. Now I see the two patches I was missing, I'll pick
> > them so that I can apply your patch cleanly on top of my tree.
> > 
> > I would have appreciated to be Cc'd on these two patches, so I knew
> > they existed, and maybe I could even have commented on them.
> 
> Jean, I would suggest you add your name to an entry in MAINTAINERS if
> you want to pickup these patches or be Cc'd on them.

Yeah, I had the same thought when writing the above. Will do.

-- 
Jean Delvare
SUSE L3 Support

Re: [Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs

[Ivan.khoronzhuk]

Hi Jean,
Sorry for the late reply.
I've send new series
"[Patch 0/3] firmware: dmi_scan: add SBMIOS entry point and DMI tables"
with all last propositions.

On 20.03.15 10:16, Jean Delvare wrote:
> Hi Ivan,
>
> On Thu, 19 Mar 2015 19:35:34 +0200, Ivan.khoronzhuk wrote:
>> On 19.03.15 17:30, Jean Delvare wrote:
>>> Le Monday 16 March 2015 à 22:57 +0200, Ivan Khoronzhuk a écrit :
>>>> 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 subsystem 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 were
>>>> proposed by Jean Delvare.
>>> "as was proposed" or even "as proposed" sounds more correct.
>>>
>>> BTW, which tree is your patch based on? I can't get it to apply cleanly
>>> on top of any kernel version I tried. I adjusted the patch to my tree
>>> but it means I'm not reviewing your code exactly. Please send patches
>>> which can be applied on top of some recent vanilla tree (3.19 or 4.0-rc4
>>> would be OK at the moment.)
>> Oh, sorry I forgot to mention, it's based on efi/next, but with consumption
>> that Matt will remove old version:
>>    85c83ea firmware: dmi-sysfs: Add SMBIOS entry point area attribute
>>
>> As you remember, your propositions were slightly late,
>> and patch had been applied on efi/next when you commented.
> OK, I understand. Now I see the two patches I was missing, I'll pick
> them so that I can apply your patch cleanly on top of my tree.
>
> I would have appreciated to be Cc'd on these two patches, so I knew
> they existed, and maybe I could even have commented on them.
>
>>>> (...)
>>>> @@ -118,6 +124,7 @@ static void dmi_table(u8 *buf,
>>>>    }
>>>>    
>>>>    static phys_addr_t dmi_base;
>>>> +static u8 *dmi_tb;
>>> Where "tb" stands for... "table", but you couldn't use that because a
>>> function by that name already exist, right? I can think of two less
>>> cryptic ways to solve this: either you rename function dmi_table to,
>>> say, dmi_decode_table (which would be a better name anyway IMHO), or you
>>> name your variable dmi_table_p or dmi_raw_table. But "tb" is not
>>> immediate enough to understand.
>> If others are OK, I'll better rename dmi_table function to
>> dmi_decode_table()
>> (or maybe dmidecode_table :), joke)
>> as it's more appropriate to what it's doing.
>> And accordingly dmi_tb to dmi_table.
> Yes, that's fine with me. The function rename should be a separate
> patch for clarity.
>
>>>> (...)
>>>>    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,8 @@ 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, 15);
>>>>    				dmi_ver = (buf[14] & 0xF0) << 4 |
>>>>    					   (buf[14] & 0x0F);
>>>>    				pr_info("Legacy DMI %d.%d present.\n",
>>>> @@ -535,6 +546,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
>>> This is inconsistent. You should either use smbios_entry_point_size as
>>> the last argument to memcpy() in all 3 cases (my preference), or you
>>> should use buf[5], 15 and buf[6] respectively, but not mix.
>> You probably meant "memcpy(smbios_entry_point, buf, 15)"
> Yes.
>
>> Just wanted to avoid redundant line break.
> Line breaks are just fine, code consistency is more important.
>
>>>> (...)
>>>> +	if (!smbios_entry_point_size || !dmi_available) {
>>> I already mentioned in a previous review that I don't think you need to
>>> check for !dmi_available, and you said you agreed.
>> No.
>> Previously only smbios entry point was exported.
>> Now DMI table was added.
>> smbios_entry_point_size doesn't mean DMI table is present.
> Thanks for the explanation, I understand the reasoning now. I can see
> how smbios_entry_point_size could be set while dmi_available would
> not. But I can't see how dmi_available could be set and
> smbios_entry_point_size would not. So I think checking for
> dmi_available is enough?
>
>>>> (...)
>>>> +		goto err;
>>>> +	}
>>>> +
>>>> +	/* Set up dmi directory at /sys/firmware/dmi */
>>>> +	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>>>> +	if (!dmi_kobj)
>>>> +		goto err;
>>>> +
>>>> +	bin_attr_smbios_entry_point.size = smbios_entry_point_size;
>>>> +	ret = sysfs_create_bin_file(dmi_kobj, &bin_attr_smbios_entry_point);
>>>> +	if (ret)
>>>> +		goto err;
>>>> +
>>>> +	if (!dmi_tb) {
>>> I don't like this. You should know which of this function and dmi_walk()
>>> is called first. If you don't, then how can you guarantee that a race
>>> condition can't happen?
>> Shit.
>> Maybe better to leave dmi_walk as it was
>> and map it only here.
> For the time being, yes, that might be better. We can always optimize
> it later in a separate patch.
>
>>> This makes me wonder if that code wouldn't rather go in
>>> dmi_scan_machine() rather than a separate function.
>>>> (...)
>>>> +		dmi_tb = dmi_remap(dmi_base, dmi_len);
>>>> +		if (!dmi_tb)
>>>> +			goto err;
>>>> +	}
>>>> +
>>>> +	bin_attr_dmi_table.size = dmi_len;
>>>> +	ret = sysfs_create_bin_file(dmi_kobj, &bin_attr_dmi_table);
>>>> +	if (ret)
>>>> +		goto err;
>>>> +
>>>> +	return 0;
>>>> +err:
>>>> +	pr_err("dmi: Firmware registration failed.\n");
>>> I said in a previous review that files that have been created should be
>>> explicitly deleted, and I still think so.
>> I dislike it, but if you insist I'll do this.
> I'm only repeating something I was told myself when submitting that
> kind of code long ago. Almost all other drivers seem to be cleaning up
> such files explicitly, just look at the firmware drivers, efivars for
> example.
>
> If you don't want to do it, I don't really mind, that's an error path
> that most probably nobody will ever walk unless explicitly testing it.
> But then if something breaks, you'll be asked to fix it.
>
>>>> (...)
>>>> +	kobject_del(dmi_kobj);
>>>> +	kobject_put(dmi_kobj);
>>> I think you also need to explicitly set dmi_kobj to NULL here, otherwise
>>> dmi-sysfs will try to use an object which no longer exists.
>> Yes.
>>
>>> An alternative approach would be to leave dmi_kobj available even if the
>>> binary files could not be created. As this case is very unlikely to
>>> happen, I don't care which way you choose.
>> I also thought about such approach. But imaged a situation when DMI
>> catalog is
>> empty and no one uses dmi-sysfs (which probably is more frequently), decided
>> to delete it. So dmi_kobj = 0.
> Fine with me (but = NULL, not = 0.)
>
> Thanks,

-- 
Regards,
Ivan Khoronzhuk

Re: [Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs

[Jean Delvare]

Le Thursday 02 April 2015 à 16:02 +0300, Ivan.khoronzhuk a écrit :
> Hi Jean,
> Sorry for the late reply.
> I've send new series
> "[Patch 0/3] firmware: dmi_scan: add SBMIOS entry point and DMI tables"
> with all last propositions.

Thanks Ivan, no problem. I'll look at it when I have time.

Two related notes:

* On week from April 13th to 17th, this is Hack Week 12 at SUSE. This
time I decided to work on dmidecode. I plan to add full support for
SMBIOS 3.0.0, including the new entry point format. Then I'll work on
integrating your work (the sysfs interface.) Project page:

https://hackweek.suse.com/12/projects/766

If you or anyone else want to participate, you are very welcome. Ideally
we would release dmidecode 2.13 at the end of that week.

* Do you know of any hardware already implementing SMBIOS 3.0 (either
the new entry point format, or the new enumerated values, or both)?
Having a real-world example would make testing much easier.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs

[Ivan.khoronzhuk]

+ Roy

On 02.04.15 18:08, Jean Delvare wrote:
> Le Thursday 02 April 2015 à 16:02 +0300, Ivan.khoronzhuk a écrit :
>> Hi Jean,
>> Sorry for the late reply.
>> I've send new series
>> "[Patch 0/3] firmware: dmi_scan: add SBMIOS entry point and DMI tables"
>> with all last propositions.
> Thanks Ivan, no problem. I'll look at it when I have time.
>
> Two related notes:
>
> * On week from April 13th to 17th, this is Hack Week 12 at SUSE. This
> time I decided to work on dmidecode. I plan to add full support for
> SMBIOS 3.0.0, including the new entry point format. Then I'll work on
> integrating your work (the sysfs interface.) Project page:
>
> https://hackweek.suse.com/12/projects/766
>
> If you or anyone else want to participate, you are very welcome. Ideally
> we would release dmidecode 2.13 at the end of that week.
>
> * Do you know of any hardware already implementing SMBIOS 3.0 (either
> the new entry point format, or the new enumerated values, or both)?
> Having a real-world example would make testing much easier.
>
> Thanks,

-- 
Regards,
Ivan Khoronzhuk