All of lore.kernel.org
 help / color / mirror / Atom feed
* [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
@ 2015-02-04 17:06 ` Ivan Khoronzhuk
  0 siblings, 0 replies; 19+ messages in thread
From: Ivan Khoronzhuk @ 2015-02-04 17:06 UTC (permalink / raw)
  To: linux-kernel, ard.biesheuvel, grant.likely, matt.fleming,
	linux-api, linux-doc
  Cc: dmidecode-devel, leif.lindholm, msalter, 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. So this patch adds SMBIOS area to
dmi-sysfs in order to allow utils in question to work correctly with
dmi sysfs interface.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---

v1: https://lkml.org/lkml/2015/1/23/643
v2: https://lkml.org/lkml/2015/1/26/345
v3: https://lkml.org/lkml/2015/1/28/768

v4..v2:
  firmware: dmi_scan: add symbol to get SMBIOS entry area
  	- used u8 type for smbios_header var
  firmware: dmi-sysfs: add SMBIOS entry point area attribute
  	- replaced -ENODATA on -EINVAL

v3..v2:
  firmware: dmi_scan: add symbol to get SMBIOS entry area
  firmware: dmi-sysfs: add SMBIOS entry point area attribute
	- combined in one patch
	- added SMBIOS information to ABI sysfs-dmi documentaton

v2..v1:
  firmware: dmi_scan: add symbol to get SMBIOS entry area
	- used additional static var to hold SMBIOS raw table size
	- changed format of get_smbios_entry_area symbol
	  returned pointer on const smbios table

  firmware: dmi-sysfs: add SMBIOS entry point area attribute
	- adopted to updated get_smbios_entry_area symbol
  	- removed redundant array to save smbios table

 Documentation/ABI/testing/sysfs-firmware-dmi | 10 +++++++
 drivers/firmware/dmi-sysfs.c                 | 42 ++++++++++++++++++++++++++++
 drivers/firmware/dmi_scan.c                  | 26 +++++++++++++++++
 include/linux/dmi.h                          |  3 ++
 4 files changed, 81 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
index c78f9ab..3a9ffe8 100644
--- a/Documentation/ABI/testing/sysfs-firmware-dmi
+++ b/Documentation/ABI/testing/sysfs-firmware-dmi
@@ -12,6 +12,16 @@ Description:
 		cannot ensure that the data as exported to userland is
 		without error either.
 
+		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_raw_header. The format of SMBIOS
+		entry point header can be read in SMBIOS specification.
+
 		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
diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index e0f1cb3..9b396d7 100644
--- a/drivers/firmware/dmi-sysfs.c
+++ b/drivers/firmware/dmi-sysfs.c
@@ -29,6 +29,8 @@
 #define MAX_ENTRY_TYPE 255 /* Most of these aren't used, but we consider
 			      the top entry type is only 8 bits */
 
+static const u8 *smbios_raw_header;
+
 struct dmi_sysfs_entry {
 	struct dmi_header dh;
 	struct kobject kobj;
@@ -646,9 +648,37 @@ static void cleanup_entry_list(void)
 	}
 }
 
+static ssize_t smbios_entry_area_raw_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_raw_header[pos], size);
+
+	return size;
+}
+
+static struct bin_attribute smbios_raw_area_attr = {
+	.read = smbios_entry_area_raw_read,
+	.attr = {.name = "smbios_raw_header", .mode = 0400},
+};
+
 static int __init dmi_sysfs_init(void)
 {
 	int error = -ENOMEM;
+	int size;
 	int val;
 
 	/* Set up our directory */
@@ -669,6 +699,18 @@ static int __init dmi_sysfs_init(void)
 		goto err;
 	}
 
+	smbios_raw_header = dmi_get_smbios_entry_area(&size);
+	if (!smbios_raw_header) {
+		pr_debug("dmi-sysfs: SMBIOS raw data is not available.\n");
+		error = -EINVAL;
+		goto err;
+	}
+
+	/* Create the raw binary file to access the entry area */
+	smbios_raw_area_attr.size = size;
+	if (sysfs_create_bin_file(dmi_kobj, &smbios_raw_area_attr))
+		goto err;
+
 	pr_debug("dmi-sysfs: loaded.\n");
 
 	return 0;
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 420c8d8..99c5f6c 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -113,6 +113,8 @@ static void dmi_table(u8 *buf, int len, int num,
 	}
 }
 
+static u8 smbios_header[32];
+static int smbios_header_size;
 static phys_addr_t dmi_base;
 static u16 dmi_len;
 static u16 dmi_num;
@@ -474,6 +476,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_header_size = buf[5];
+		memcpy(smbios_header, buf, smbios_header_size);
 
 		/* Some BIOS report weird SMBIOS version, fix that up */
 		switch (smbios_ver) {
@@ -505,6 +509,8 @@ static int __init dmi_present(const u8 *buf)
 				pr_info("SMBIOS %d.%d present.\n",
 				       dmi_ver >> 8, dmi_ver & 0xFF);
 			} else {
+				smbios_header_size = 15;
+				memcpy(smbios_header, buf, smbios_header_size);
 				dmi_ver = (buf[14] & 0xF0) << 4 |
 					   (buf[14] & 0x0F);
 				pr_info("Legacy DMI %d.%d present.\n",
@@ -531,6 +537,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_header_size = buf[6];
+		memcpy(smbios_header, buf, smbios_header_size);
 
 		/*
 		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
@@ -944,3 +952,21 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device)
 	}
 }
 EXPORT_SYMBOL_GPL(dmi_memdev_name);
+
+/**
+ * dmi_get_smbios_entry_area - copy SMBIOS entry point area to array.
+ * @size - pointer to assign actual size of SMBIOS entry point area.
+ *
+ * returns NULL if table is not available, otherwise returns pointer on
+ * SMBIOS entry point area array.
+ */
+const u8 *dmi_get_smbios_entry_area(int *size)
+{
+	if (!smbios_header_size || !dmi_available)
+		return NULL;
+
+	*size = smbios_header_size;
+
+	return smbios_header;
+}
+EXPORT_SYMBOL_GPL(dmi_get_smbios_entry_area);
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index f820f0a..8e1a28d 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -109,6 +109,7 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	void *private_data);
 extern bool dmi_match(enum dmi_field f, const char *str);
 extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
+const u8 *dmi_get_smbios_entry_area(int *size);
 
 #else
 
@@ -140,6 +141,8 @@ static inline void dmi_memdev_name(u16 handle, const char **bank,
 		const char **device) { }
 static inline const struct dmi_system_id *
 	dmi_first_match(const struct dmi_system_id *list) { return NULL; }
+static inline const u8 *dmi_get_smbios_entry_area(int *size)
+	{ return NULL; }
 
 #endif
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
@ 2015-02-04 17:06 ` Ivan Khoronzhuk
  0 siblings, 0 replies; 19+ messages in thread
From: Ivan Khoronzhuk @ 2015-02-04 17:06 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
	grant.likely-QSEj5FYQhm4dnm+yROfE0A,
	matt.fleming-ral2JQCrhuEAvxtiuMwx3w,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA
  Cc: dmidecode-devel-qX2TKyscuCcdnm+yROfE0A,
	leif.lindholm-QSEj5FYQhm4dnm+yROfE0A,
	msalter-H+wXaHxf7aLQT0dZR+AlfA, 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. So this patch adds SMBIOS area to
dmi-sysfs in order to allow utils in question to work correctly with
dmi sysfs interface.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---

v1: https://lkml.org/lkml/2015/1/23/643
v2: https://lkml.org/lkml/2015/1/26/345
v3: https://lkml.org/lkml/2015/1/28/768

v4..v2:
  firmware: dmi_scan: add symbol to get SMBIOS entry area
  	- used u8 type for smbios_header var
  firmware: dmi-sysfs: add SMBIOS entry point area attribute
  	- replaced -ENODATA on -EINVAL

v3..v2:
  firmware: dmi_scan: add symbol to get SMBIOS entry area
  firmware: dmi-sysfs: add SMBIOS entry point area attribute
	- combined in one patch
	- added SMBIOS information to ABI sysfs-dmi documentaton

v2..v1:
  firmware: dmi_scan: add symbol to get SMBIOS entry area
	- used additional static var to hold SMBIOS raw table size
	- changed format of get_smbios_entry_area symbol
	  returned pointer on const smbios table

  firmware: dmi-sysfs: add SMBIOS entry point area attribute
	- adopted to updated get_smbios_entry_area symbol
  	- removed redundant array to save smbios table

 Documentation/ABI/testing/sysfs-firmware-dmi | 10 +++++++
 drivers/firmware/dmi-sysfs.c                 | 42 ++++++++++++++++++++++++++++
 drivers/firmware/dmi_scan.c                  | 26 +++++++++++++++++
 include/linux/dmi.h                          |  3 ++
 4 files changed, 81 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
index c78f9ab..3a9ffe8 100644
--- a/Documentation/ABI/testing/sysfs-firmware-dmi
+++ b/Documentation/ABI/testing/sysfs-firmware-dmi
@@ -12,6 +12,16 @@ Description:
 		cannot ensure that the data as exported to userland is
 		without error either.
 
+		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_raw_header. The format of SMBIOS
+		entry point header can be read in SMBIOS specification.
+
 		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
diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index e0f1cb3..9b396d7 100644
--- a/drivers/firmware/dmi-sysfs.c
+++ b/drivers/firmware/dmi-sysfs.c
@@ -29,6 +29,8 @@
 #define MAX_ENTRY_TYPE 255 /* Most of these aren't used, but we consider
 			      the top entry type is only 8 bits */
 
+static const u8 *smbios_raw_header;
+
 struct dmi_sysfs_entry {
 	struct dmi_header dh;
 	struct kobject kobj;
@@ -646,9 +648,37 @@ static void cleanup_entry_list(void)
 	}
 }
 
+static ssize_t smbios_entry_area_raw_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_raw_header[pos], size);
+
+	return size;
+}
+
+static struct bin_attribute smbios_raw_area_attr = {
+	.read = smbios_entry_area_raw_read,
+	.attr = {.name = "smbios_raw_header", .mode = 0400},
+};
+
 static int __init dmi_sysfs_init(void)
 {
 	int error = -ENOMEM;
+	int size;
 	int val;
 
 	/* Set up our directory */
@@ -669,6 +699,18 @@ static int __init dmi_sysfs_init(void)
 		goto err;
 	}
 
+	smbios_raw_header = dmi_get_smbios_entry_area(&size);
+	if (!smbios_raw_header) {
+		pr_debug("dmi-sysfs: SMBIOS raw data is not available.\n");
+		error = -EINVAL;
+		goto err;
+	}
+
+	/* Create the raw binary file to access the entry area */
+	smbios_raw_area_attr.size = size;
+	if (sysfs_create_bin_file(dmi_kobj, &smbios_raw_area_attr))
+		goto err;
+
 	pr_debug("dmi-sysfs: loaded.\n");
 
 	return 0;
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 420c8d8..99c5f6c 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -113,6 +113,8 @@ static void dmi_table(u8 *buf, int len, int num,
 	}
 }
 
+static u8 smbios_header[32];
+static int smbios_header_size;
 static phys_addr_t dmi_base;
 static u16 dmi_len;
 static u16 dmi_num;
@@ -474,6 +476,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_header_size = buf[5];
+		memcpy(smbios_header, buf, smbios_header_size);
 
 		/* Some BIOS report weird SMBIOS version, fix that up */
 		switch (smbios_ver) {
@@ -505,6 +509,8 @@ static int __init dmi_present(const u8 *buf)
 				pr_info("SMBIOS %d.%d present.\n",
 				       dmi_ver >> 8, dmi_ver & 0xFF);
 			} else {
+				smbios_header_size = 15;
+				memcpy(smbios_header, buf, smbios_header_size);
 				dmi_ver = (buf[14] & 0xF0) << 4 |
 					   (buf[14] & 0x0F);
 				pr_info("Legacy DMI %d.%d present.\n",
@@ -531,6 +537,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_header_size = buf[6];
+		memcpy(smbios_header, buf, smbios_header_size);
 
 		/*
 		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
@@ -944,3 +952,21 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device)
 	}
 }
 EXPORT_SYMBOL_GPL(dmi_memdev_name);
+
+/**
+ * dmi_get_smbios_entry_area - copy SMBIOS entry point area to array.
+ * @size - pointer to assign actual size of SMBIOS entry point area.
+ *
+ * returns NULL if table is not available, otherwise returns pointer on
+ * SMBIOS entry point area array.
+ */
+const u8 *dmi_get_smbios_entry_area(int *size)
+{
+	if (!smbios_header_size || !dmi_available)
+		return NULL;
+
+	*size = smbios_header_size;
+
+	return smbios_header;
+}
+EXPORT_SYMBOL_GPL(dmi_get_smbios_entry_area);
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index f820f0a..8e1a28d 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -109,6 +109,7 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	void *private_data);
 extern bool dmi_match(enum dmi_field f, const char *str);
 extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
+const u8 *dmi_get_smbios_entry_area(int *size);
 
 #else
 
@@ -140,6 +141,8 @@ static inline void dmi_memdev_name(u16 handle, const char **bank,
 		const char **device) { }
 static inline const struct dmi_system_id *
 	dmi_first_match(const struct dmi_system_id *list) { return NULL; }
+static inline const u8 *dmi_get_smbios_entry_area(int *size)
+	{ return NULL; }
 
 #endif
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
  2015-02-04 17:06 ` Ivan Khoronzhuk
  (?)
@ 2015-02-10  9:51 ` Ivan Khoronzhuk
  2015-02-11 14:17   ` Matt Fleming
  -1 siblings, 1 reply; 19+ messages in thread
From: Ivan Khoronzhuk @ 2015-02-10  9:51 UTC (permalink / raw)
  To: matt, Grant Likely, Ard Biesheuvel
  Cc: linux-kernel, linux-api, linux-doc, Leif Lindholm, Mark Salter

Hi Matt,

On 02/04/2015 07:06 PM, 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. So this patch adds SMBIOS area to
> dmi-sysfs in order to allow utils in question to work correctly with
> dmi sysfs interface.
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>
> v1: https://lkml.org/lkml/2015/1/23/643
> v2: https://lkml.org/lkml/2015/1/26/345
> v3: https://lkml.org/lkml/2015/1/28/768
>
> v4..v2:
>    firmware: dmi_scan: add symbol to get SMBIOS entry area
>    	- used u8 type for smbios_header var
>    firmware: dmi-sysfs: add SMBIOS entry point area attribute
>    	- replaced -ENODATA on -EINVAL
>
> v3..v2:
>    firmware: dmi_scan: add symbol to get SMBIOS entry area
>    firmware: dmi-sysfs: add SMBIOS entry point area attribute
> 	- combined in one patch
> 	- added SMBIOS information to ABI sysfs-dmi documentaton
>
> v2..v1:
>    firmware: dmi_scan: add symbol to get SMBIOS entry area
> 	- used additional static var to hold SMBIOS raw table size
> 	- changed format of get_smbios_entry_area symbol
> 	  returned pointer on const smbios table
>
>    firmware: dmi-sysfs: add SMBIOS entry point area attribute
> 	- adopted to updated get_smbios_entry_area symbol
>    	- removed redundant array to save smbios table
>
>   Documentation/ABI/testing/sysfs-firmware-dmi | 10 +++++++
>   drivers/firmware/dmi-sysfs.c                 | 42 ++++++++++++++++++++++++++++
>   drivers/firmware/dmi_scan.c                  | 26 +++++++++++++++++
>   include/linux/dmi.h                          |  3 ++
>   4 files changed, 81 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
> index c78f9ab..3a9ffe8 100644
> --- a/Documentation/ABI/testing/sysfs-firmware-dmi
> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi
> @@ -12,6 +12,16 @@ Description:
>   		cannot ensure that the data as exported to userland is
>   		without error either.
>   
> +		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_raw_header. The format of SMBIOS
> +		entry point header can be read in SMBIOS specification.
> +
>   		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
> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
> index e0f1cb3..9b396d7 100644
> --- a/drivers/firmware/dmi-sysfs.c
> +++ b/drivers/firmware/dmi-sysfs.c
> @@ -29,6 +29,8 @@
>   #define MAX_ENTRY_TYPE 255 /* Most of these aren't used, but we consider
>   			      the top entry type is only 8 bits */
>   
> +static const u8 *smbios_raw_header;
> +
>   struct dmi_sysfs_entry {
>   	struct dmi_header dh;
>   	struct kobject kobj;
> @@ -646,9 +648,37 @@ static void cleanup_entry_list(void)
>   	}
>   }
>   
> +static ssize_t smbios_entry_area_raw_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_raw_header[pos], size);
> +
> +	return size;
> +}
> +
> +static struct bin_attribute smbios_raw_area_attr = {
> +	.read = smbios_entry_area_raw_read,
> +	.attr = {.name = "smbios_raw_header", .mode = 0400},
> +};
> +
>   static int __init dmi_sysfs_init(void)
>   {
>   	int error = -ENOMEM;
> +	int size;
>   	int val;
>   
>   	/* Set up our directory */
> @@ -669,6 +699,18 @@ static int __init dmi_sysfs_init(void)
>   		goto err;
>   	}
>   
> +	smbios_raw_header = dmi_get_smbios_entry_area(&size);
> +	if (!smbios_raw_header) {
> +		pr_debug("dmi-sysfs: SMBIOS raw data is not available.\n");
> +		error = -EINVAL;
> +		goto err;
> +	}
> +
> +	/* Create the raw binary file to access the entry area */
> +	smbios_raw_area_attr.size = size;
> +	if (sysfs_create_bin_file(dmi_kobj, &smbios_raw_area_attr))
> +		goto err;
> +
>   	pr_debug("dmi-sysfs: loaded.\n");
>   
>   	return 0;
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 420c8d8..99c5f6c 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -113,6 +113,8 @@ static void dmi_table(u8 *buf, int len, int num,
>   	}
>   }
>   
> +static u8 smbios_header[32];
> +static int smbios_header_size;
>   static phys_addr_t dmi_base;
>   static u16 dmi_len;
>   static u16 dmi_num;
> @@ -474,6 +476,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_header_size = buf[5];
> +		memcpy(smbios_header, buf, smbios_header_size);
>   
>   		/* Some BIOS report weird SMBIOS version, fix that up */
>   		switch (smbios_ver) {
> @@ -505,6 +509,8 @@ static int __init dmi_present(const u8 *buf)
>   				pr_info("SMBIOS %d.%d present.\n",
>   				       dmi_ver >> 8, dmi_ver & 0xFF);
>   			} else {
> +				smbios_header_size = 15;
> +				memcpy(smbios_header, buf, smbios_header_size);
>   				dmi_ver = (buf[14] & 0xF0) << 4 |
>   					   (buf[14] & 0x0F);
>   				pr_info("Legacy DMI %d.%d present.\n",
> @@ -531,6 +537,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_header_size = buf[6];
> +		memcpy(smbios_header, buf, smbios_header_size);
>   
>   		/*
>   		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
> @@ -944,3 +952,21 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device)
>   	}
>   }
>   EXPORT_SYMBOL_GPL(dmi_memdev_name);
> +
> +/**
> + * dmi_get_smbios_entry_area - copy SMBIOS entry point area to array.
> + * @size - pointer to assign actual size of SMBIOS entry point area.
> + *
> + * returns NULL if table is not available, otherwise returns pointer on
> + * SMBIOS entry point area array.
> + */
> +const u8 *dmi_get_smbios_entry_area(int *size)
> +{
> +	if (!smbios_header_size || !dmi_available)
> +		return NULL;
> +
> +	*size = smbios_header_size;
> +
> +	return smbios_header;
> +}
> +EXPORT_SYMBOL_GPL(dmi_get_smbios_entry_area);
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index f820f0a..8e1a28d 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -109,6 +109,7 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>   	void *private_data);
>   extern bool dmi_match(enum dmi_field f, const char *str);
>   extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
> +const u8 *dmi_get_smbios_entry_area(int *size);
>   
>   #else
>   
> @@ -140,6 +141,8 @@ static inline void dmi_memdev_name(u16 handle, const char **bank,
>   		const char **device) { }
>   static inline const struct dmi_system_id *
>   	dmi_first_match(const struct dmi_system_id *list) { return NULL; }
> +static inline const u8 *dmi_get_smbios_entry_area(int *size)
> +	{ return NULL; }
>   
>   #endif
>   

If you are Ok with this patch, could you please pickup it?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
  2015-02-10  9:51 ` Ivan Khoronzhuk
@ 2015-02-11 14:17   ` Matt Fleming
  2015-02-11 14:43     ` Matt Fleming
  0 siblings, 1 reply; 19+ messages in thread
From: Matt Fleming @ 2015-02-11 14:17 UTC (permalink / raw)
  To: Ivan Khoronzhuk
  Cc: Grant Likely, Ard Biesheuvel, linux-kernel, linux-api, linux-doc,
	Leif Lindholm, Mark Salter

On Tue, 10 Feb, at 11:51:44AM, Ivan Khoronzhuk wrote:
> 
> If you are Ok with this patch, could you please pickup it?

Applied, thanks Ivan!

-- 
Matt Fleming, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
  2015-02-11 14:17   ` Matt Fleming
@ 2015-02-11 14:43     ` Matt Fleming
  2015-02-12  2:33         ` Ivan Khoronzhuk
  0 siblings, 1 reply; 19+ messages in thread
From: Matt Fleming @ 2015-02-11 14:43 UTC (permalink / raw)
  To: Ivan Khoronzhuk
  Cc: Grant Likely, Ard Biesheuvel, linux-kernel, linux-api, linux-doc,
	Leif Lindholm, Mark Salter

On Wed, 11 Feb, at 02:17:03PM, Matt Fleming wrote:
> On Tue, 10 Feb, at 11:51:44AM, Ivan Khoronzhuk wrote:
> > 
> > If you are Ok with this patch, could you please pickup it?
> 
> Applied, thanks Ivan!

Btw this patch doesn't apply cleanly, the reject looks like this,

--- drivers/firmware/dmi_scan.c
+++ drivers/firmware/dmi_scan.c
@@ -537,6 +543,8 @@
 		dmi_ver &= 0xFFFFFF;
 		dmi_len = get_unaligned_le32(buf + 12);
 		dmi_base = get_unaligned_le64(buf + 16);
+		smbios_header_size = buf[6];
+		memcpy(smbios_header, buf, smbios_header_size);
 
 		/*
 		 * The 64-bit SMBIOS 3.0 entry point no longer has a field

What version of the kernel did you base this patch on? The conflict is
trivial to fixup and I've done so and pushed it out on the EFI 'next'
branch, but I wanted to call out this conflict explicitly.

-- 
Matt Fleming, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
@ 2015-02-12  2:33         ` Ivan Khoronzhuk
  0 siblings, 0 replies; 19+ messages in thread
From: Ivan Khoronzhuk @ 2015-02-12  2:33 UTC (permalink / raw)
  To: Matt Fleming
  Cc: Grant Likely, Ard Biesheuvel, linux-kernel, linux-api, linux-doc,
	Leif Lindholm, Mark Salter

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]


On 02/11/2015 04:43 PM, Matt Fleming wrote:
> On Wed, 11 Feb, at 02:17:03PM, Matt Fleming wrote:
>> On Tue, 10 Feb, at 11:51:44AM, Ivan Khoronzhuk wrote:
>>> If you are Ok with this patch, could you please pickup it?
>> Applied, thanks Ivan!
> Btw this patch doesn't apply cleanly, the reject looks like this,
>
> --- drivers/firmware/dmi_scan.c
> +++ drivers/firmware/dmi_scan.c
> @@ -537,6 +543,8 @@
>   		dmi_ver &= 0xFFFFFF;

The problem is in above string.
I used linux next, but I had one patch before.
Sorry... just forgot about it.
I've attached the same patch but on top of linux_next,
it can be applied cleanly.
Sorry once again and thanks!

>   		dmi_len = get_unaligned_le32(buf + 12);
>   		dmi_base = get_unaligned_le64(buf + 16);
> +		smbios_header_size = buf[6];
> +		memcpy(smbios_header, buf, smbios_header_size);
>   
>   		/*
>   		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
>
> What version of the kernel did you base this patch on? The conflict is
> trivial to fixup and I've done so and pushed it out on the EFI 'next'
> branch, but I wanted to call out this conflict explicitly.
>


[-- Attachment #2: 0001-firmware-dmi-sysfs-add-SMBIOS-entry-point-area-attri.patch --]
[-- Type: text/x-patch, Size: 6641 bytes --]

>From b64185ddfb0b704f83324d8b0c6be7f7ee951581 Mon Sep 17 00:00:00 2001
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date: Tue, 30 Dec 2014 02:58:12 +0200
Subject: [PATCH] firmware: dmi-sysfs: add SMBIOS entry point area attribute

Some utils, like dmidecode and smbios, needs 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. So this patch adds SMBIOS area to
dmi-sysfs in order to allow utils in question to work correctly with
dmi sysfs interface.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 Documentation/ABI/testing/sysfs-firmware-dmi | 10 +++++++
 drivers/firmware/dmi-sysfs.c                 | 42 ++++++++++++++++++++++++++++
 drivers/firmware/dmi_scan.c                  | 26 +++++++++++++++++
 include/linux/dmi.h                          |  3 ++
 4 files changed, 81 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
index c78f9ab..3a9ffe8 100644
--- a/Documentation/ABI/testing/sysfs-firmware-dmi
+++ b/Documentation/ABI/testing/sysfs-firmware-dmi
@@ -12,6 +12,16 @@ Description:
 		cannot ensure that the data as exported to userland is
 		without error either.
 
+		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_raw_header. The format of SMBIOS
+		entry point header can be read in SMBIOS specification.
+
 		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
diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index e0f1cb3..9b396d7 100644
--- a/drivers/firmware/dmi-sysfs.c
+++ b/drivers/firmware/dmi-sysfs.c
@@ -29,6 +29,8 @@
 #define MAX_ENTRY_TYPE 255 /* Most of these aren't used, but we consider
 			      the top entry type is only 8 bits */
 
+static const u8 *smbios_raw_header;
+
 struct dmi_sysfs_entry {
 	struct dmi_header dh;
 	struct kobject kobj;
@@ -646,9 +648,37 @@ static void cleanup_entry_list(void)
 	}
 }
 
+static ssize_t smbios_entry_area_raw_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_raw_header[pos], size);
+
+	return size;
+}
+
+static struct bin_attribute smbios_raw_area_attr = {
+	.read = smbios_entry_area_raw_read,
+	.attr = {.name = "smbios_raw_header", .mode = 0400},
+};
+
 static int __init dmi_sysfs_init(void)
 {
 	int error = -ENOMEM;
+	int size;
 	int val;
 
 	/* Set up our directory */
@@ -669,6 +699,18 @@ static int __init dmi_sysfs_init(void)
 		goto err;
 	}
 
+	smbios_raw_header = dmi_get_smbios_entry_area(&size);
+	if (!smbios_raw_header) {
+		pr_debug("dmi-sysfs: SMBIOS raw data is not available.\n");
+		error = -EINVAL;
+		goto err;
+	}
+
+	/* Create the raw binary file to access the entry area */
+	smbios_raw_area_attr.size = size;
+	if (sysfs_create_bin_file(dmi_kobj, &smbios_raw_area_attr))
+		goto err;
+
 	pr_debug("dmi-sysfs: loaded.\n");
 
 	return 0;
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index c5f7b4e..d55c712 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -113,6 +113,8 @@ static void dmi_table(u8 *buf, int len, int num,
 	}
 }
 
+static u8 smbios_header[32];
+static int smbios_header_size;
 static phys_addr_t dmi_base;
 static u16 dmi_len;
 static u16 dmi_num;
@@ -474,6 +476,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_header_size = buf[5];
+		memcpy(smbios_header, buf, smbios_header_size);
 
 		/* Some BIOS report weird SMBIOS version, fix that up */
 		switch (smbios_ver) {
@@ -505,6 +509,8 @@ static int __init dmi_present(const u8 *buf)
 				pr_info("SMBIOS %d.%d present.\n",
 				       dmi_ver >> 8, dmi_ver & 0xFF);
 			} else {
+				smbios_header_size = 15;
+				memcpy(smbios_header, buf, smbios_header_size);
 				dmi_ver = (buf[14] & 0xF0) << 4 |
 					   (buf[14] & 0x0F);
 				pr_info("Legacy DMI %d.%d present.\n",
@@ -530,6 +536,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
 		dmi_ver = get_unaligned_be16(buf + 7);
 		dmi_len = get_unaligned_le32(buf + 12);
 		dmi_base = get_unaligned_le64(buf + 16);
+		smbios_header_size = buf[6];
+		memcpy(smbios_header, buf, smbios_header_size);
 
 		/*
 		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
@@ -941,3 +949,21 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device)
 	}
 }
 EXPORT_SYMBOL_GPL(dmi_memdev_name);
+
+/**
+ * dmi_get_smbios_entry_area - copy SMBIOS entry point area to array.
+ * @size - pointer to assign actual size of SMBIOS entry point area.
+ *
+ * returns NULL if table is not available, otherwise returns pointer on
+ * SMBIOS entry point area array.
+ */
+const u8 *dmi_get_smbios_entry_area(int *size)
+{
+	if (!smbios_header_size || !dmi_available)
+		return NULL;
+
+	*size = smbios_header_size;
+
+	return smbios_header;
+}
+EXPORT_SYMBOL_GPL(dmi_get_smbios_entry_area);
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index f820f0a..8e1a28d 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -109,6 +109,7 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	void *private_data);
 extern bool dmi_match(enum dmi_field f, const char *str);
 extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
+const u8 *dmi_get_smbios_entry_area(int *size);
 
 #else
 
@@ -140,6 +141,8 @@ static inline void dmi_memdev_name(u16 handle, const char **bank,
 		const char **device) { }
 static inline const struct dmi_system_id *
 	dmi_first_match(const struct dmi_system_id *list) { return NULL; }
+static inline const u8 *dmi_get_smbios_entry_area(int *size)
+	{ return NULL; }
 
 #endif
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
@ 2015-02-12  2:33         ` Ivan Khoronzhuk
  0 siblings, 0 replies; 19+ messages in thread
From: Ivan Khoronzhuk @ 2015-02-12  2:33 UTC (permalink / raw)
  To: Matt Fleming
  Cc: Grant Likely, Ard Biesheuvel,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA, Leif Lindholm, Mark Salter

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]


On 02/11/2015 04:43 PM, Matt Fleming wrote:
> On Wed, 11 Feb, at 02:17:03PM, Matt Fleming wrote:
>> On Tue, 10 Feb, at 11:51:44AM, Ivan Khoronzhuk wrote:
>>> If you are Ok with this patch, could you please pickup it?
>> Applied, thanks Ivan!
> Btw this patch doesn't apply cleanly, the reject looks like this,
>
> --- drivers/firmware/dmi_scan.c
> +++ drivers/firmware/dmi_scan.c
> @@ -537,6 +543,8 @@
>   		dmi_ver &= 0xFFFFFF;

The problem is in above string.
I used linux next, but I had one patch before.
Sorry... just forgot about it.
I've attached the same patch but on top of linux_next,
it can be applied cleanly.
Sorry once again and thanks!

>   		dmi_len = get_unaligned_le32(buf + 12);
>   		dmi_base = get_unaligned_le64(buf + 16);
> +		smbios_header_size = buf[6];
> +		memcpy(smbios_header, buf, smbios_header_size);
>   
>   		/*
>   		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
>
> What version of the kernel did you base this patch on? The conflict is
> trivial to fixup and I've done so and pushed it out on the EFI 'next'
> branch, but I wanted to call out this conflict explicitly.
>


[-- Attachment #2: 0001-firmware-dmi-sysfs-add-SMBIOS-entry-point-area-attri.patch --]
[-- Type: text/x-patch, Size: 6728 bytes --]

>From b64185ddfb0b704f83324d8b0c6be7f7ee951581 Mon Sep 17 00:00:00 2001
From: Ivan Khoronzhuk <ivan.khoronzhuk-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Tue, 30 Dec 2014 02:58:12 +0200
Subject: [PATCH] firmware: dmi-sysfs: add SMBIOS entry point area attribute

Some utils, like dmidecode and smbios, needs 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. So this patch adds SMBIOS area to
dmi-sysfs in order to allow utils in question to work correctly with
dmi sysfs interface.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 Documentation/ABI/testing/sysfs-firmware-dmi | 10 +++++++
 drivers/firmware/dmi-sysfs.c                 | 42 ++++++++++++++++++++++++++++
 drivers/firmware/dmi_scan.c                  | 26 +++++++++++++++++
 include/linux/dmi.h                          |  3 ++
 4 files changed, 81 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
index c78f9ab..3a9ffe8 100644
--- a/Documentation/ABI/testing/sysfs-firmware-dmi
+++ b/Documentation/ABI/testing/sysfs-firmware-dmi
@@ -12,6 +12,16 @@ Description:
 		cannot ensure that the data as exported to userland is
 		without error either.
 
+		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_raw_header. The format of SMBIOS
+		entry point header can be read in SMBIOS specification.
+
 		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
diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index e0f1cb3..9b396d7 100644
--- a/drivers/firmware/dmi-sysfs.c
+++ b/drivers/firmware/dmi-sysfs.c
@@ -29,6 +29,8 @@
 #define MAX_ENTRY_TYPE 255 /* Most of these aren't used, but we consider
 			      the top entry type is only 8 bits */
 
+static const u8 *smbios_raw_header;
+
 struct dmi_sysfs_entry {
 	struct dmi_header dh;
 	struct kobject kobj;
@@ -646,9 +648,37 @@ static void cleanup_entry_list(void)
 	}
 }
 
+static ssize_t smbios_entry_area_raw_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_raw_header[pos], size);
+
+	return size;
+}
+
+static struct bin_attribute smbios_raw_area_attr = {
+	.read = smbios_entry_area_raw_read,
+	.attr = {.name = "smbios_raw_header", .mode = 0400},
+};
+
 static int __init dmi_sysfs_init(void)
 {
 	int error = -ENOMEM;
+	int size;
 	int val;
 
 	/* Set up our directory */
@@ -669,6 +699,18 @@ static int __init dmi_sysfs_init(void)
 		goto err;
 	}
 
+	smbios_raw_header = dmi_get_smbios_entry_area(&size);
+	if (!smbios_raw_header) {
+		pr_debug("dmi-sysfs: SMBIOS raw data is not available.\n");
+		error = -EINVAL;
+		goto err;
+	}
+
+	/* Create the raw binary file to access the entry area */
+	smbios_raw_area_attr.size = size;
+	if (sysfs_create_bin_file(dmi_kobj, &smbios_raw_area_attr))
+		goto err;
+
 	pr_debug("dmi-sysfs: loaded.\n");
 
 	return 0;
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index c5f7b4e..d55c712 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -113,6 +113,8 @@ static void dmi_table(u8 *buf, int len, int num,
 	}
 }
 
+static u8 smbios_header[32];
+static int smbios_header_size;
 static phys_addr_t dmi_base;
 static u16 dmi_len;
 static u16 dmi_num;
@@ -474,6 +476,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_header_size = buf[5];
+		memcpy(smbios_header, buf, smbios_header_size);
 
 		/* Some BIOS report weird SMBIOS version, fix that up */
 		switch (smbios_ver) {
@@ -505,6 +509,8 @@ static int __init dmi_present(const u8 *buf)
 				pr_info("SMBIOS %d.%d present.\n",
 				       dmi_ver >> 8, dmi_ver & 0xFF);
 			} else {
+				smbios_header_size = 15;
+				memcpy(smbios_header, buf, smbios_header_size);
 				dmi_ver = (buf[14] & 0xF0) << 4 |
 					   (buf[14] & 0x0F);
 				pr_info("Legacy DMI %d.%d present.\n",
@@ -530,6 +536,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
 		dmi_ver = get_unaligned_be16(buf + 7);
 		dmi_len = get_unaligned_le32(buf + 12);
 		dmi_base = get_unaligned_le64(buf + 16);
+		smbios_header_size = buf[6];
+		memcpy(smbios_header, buf, smbios_header_size);
 
 		/*
 		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
@@ -941,3 +949,21 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device)
 	}
 }
 EXPORT_SYMBOL_GPL(dmi_memdev_name);
+
+/**
+ * dmi_get_smbios_entry_area - copy SMBIOS entry point area to array.
+ * @size - pointer to assign actual size of SMBIOS entry point area.
+ *
+ * returns NULL if table is not available, otherwise returns pointer on
+ * SMBIOS entry point area array.
+ */
+const u8 *dmi_get_smbios_entry_area(int *size)
+{
+	if (!smbios_header_size || !dmi_available)
+		return NULL;
+
+	*size = smbios_header_size;
+
+	return smbios_header;
+}
+EXPORT_SYMBOL_GPL(dmi_get_smbios_entry_area);
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index f820f0a..8e1a28d 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -109,6 +109,7 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	void *private_data);
 extern bool dmi_match(enum dmi_field f, const char *str);
 extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
+const u8 *dmi_get_smbios_entry_area(int *size);
 
 #else
 
@@ -140,6 +141,8 @@ static inline void dmi_memdev_name(u16 handle, const char **bank,
 		const char **device) { }
 static inline const struct dmi_system_id *
 	dmi_first_match(const struct dmi_system_id *list) { return NULL; }
+static inline const u8 *dmi_get_smbios_entry_area(int *size)
+	{ return NULL; }
 
 #endif
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
  2015-02-04 17:06 ` Ivan Khoronzhuk
  (?)
  (?)
@ 2015-02-26  9:36 ` Jean Delvare
  2015-03-04 12:30   ` Ivan.khoronzhuk
  -1 siblings, 1 reply; 19+ messages in thread
From: Jean Delvare @ 2015-02-26  9:36 UTC (permalink / raw)
  To: Ivan Khoronzhuk
  Cc: linux-kernel, ard.biesheuvel, grant.likely, matt.fleming,
	linux-api, linux-doc, dmidecode-devel, leif.lindholm, msalter

Hi Ivan,

Sorry for the late review.

On Wed,  4 Feb 2015 19:06:03 +0200, 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. So this patch adds SMBIOS area to
> dmi-sysfs in order to allow utils in question to work correctly with
> dmi sysfs interface.
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> 
> v1: https://lkml.org/lkml/2015/1/23/643
> v2: https://lkml.org/lkml/2015/1/26/345
> v3: https://lkml.org/lkml/2015/1/28/768
> 
> v4..v2:

Please always provide a list of changes from the previous version of
the patch, otherwise it's quite confusing.

>   firmware: dmi_scan: add symbol to get SMBIOS entry area
>   	- used u8 type for smbios_header var
>   firmware: dmi-sysfs: add SMBIOS entry point area attribute
>   	- replaced -ENODATA on -EINVAL
> 
> v3..v2:
>   firmware: dmi_scan: add symbol to get SMBIOS entry area
>   firmware: dmi-sysfs: add SMBIOS entry point area attribute
> 	- combined in one patch
> 	- added SMBIOS information to ABI sysfs-dmi documentaton
> 
> v2..v1:
>   firmware: dmi_scan: add symbol to get SMBIOS entry area
> 	- used additional static var to hold SMBIOS raw table size
> 	- changed format of get_smbios_entry_area symbol
> 	  returned pointer on const smbios table
> 
>   firmware: dmi-sysfs: add SMBIOS entry point area attribute
> 	- adopted to updated get_smbios_entry_area symbol
>   	- removed redundant array to save smbios table
> 
>  Documentation/ABI/testing/sysfs-firmware-dmi | 10 +++++++
>  drivers/firmware/dmi-sysfs.c                 | 42 ++++++++++++++++++++++++++++
>  drivers/firmware/dmi_scan.c                  | 26 +++++++++++++++++
>  include/linux/dmi.h                          |  3 ++
>  4 files changed, 81 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
> index c78f9ab..3a9ffe8 100644
> --- a/Documentation/ABI/testing/sysfs-firmware-dmi
> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi
> @@ -12,6 +12,16 @@ Description:
>  		cannot ensure that the data as exported to userland is
>  		without error either.
>  
> +		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_raw_header. The format of SMBIOS

As mentioned before, I don't like the name "smbios_raw_header". I think
it should be "smbios_entry_point" or similar.

> +		entry point header can be read in SMBIOS specification.
> +
>  		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
> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
> index e0f1cb3..9b396d7 100644
> --- a/drivers/firmware/dmi-sysfs.c
> +++ b/drivers/firmware/dmi-sysfs.c
> @@ -29,6 +29,8 @@
>  #define MAX_ENTRY_TYPE 255 /* Most of these aren't used, but we consider
>  			      the top entry type is only 8 bits */
>  
> +static const u8 *smbios_raw_header;
> +
>  struct dmi_sysfs_entry {
>  	struct dmi_header dh;
>  	struct kobject kobj;
> @@ -646,9 +648,37 @@ static void cleanup_entry_list(void)
>  	}
>  }
>  
> +static ssize_t smbios_entry_area_raw_read(struct file *filp,

This is confusing again, now it's named "entry_area"? Please be
consistent and use entry_point everywhere.

As mentioned before I believe that this code should live in dmi_scan
and not dmi-sysfs.

> +					  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_raw_header[pos], size);
> +
> +	return size;
> +}
> +
> +static struct bin_attribute smbios_raw_area_attr = {
> +	.read = smbios_entry_area_raw_read,
> +	.attr = {.name = "smbios_raw_header", .mode = 0400},
> +};
> +
>  static int __init dmi_sysfs_init(void)
>  {
>  	int error = -ENOMEM;
> +	int size;
>  	int val;
>  
>  	/* Set up our directory */
> @@ -669,6 +699,18 @@ static int __init dmi_sysfs_init(void)
>  		goto err;
>  	}
>  
> +	smbios_raw_header = dmi_get_smbios_entry_area(&size);
> +	if (!smbios_raw_header) {
> +		pr_debug("dmi-sysfs: SMBIOS raw data is not available.\n");
> +		error = -EINVAL;
> +		goto err;
> +	}

I don't think this should have been a fatal error. Just because for
some reason dmi_get_smbios_entry_area() returned NULL is no good reason
for nor exposing /sys/firmware/dmi/entries as we used to.

But anyway this is no longer relevant if the code is moved to dmi_scan
as I suggested.

> +
> +	/* Create the raw binary file to access the entry area */
> +	smbios_raw_area_attr.size = size;
> +	if (sysfs_create_bin_file(dmi_kobj, &smbios_raw_area_attr))
> +		goto err;

I think this should have had a corresponding call to
sysfs_remove_bin_file() in dmi_sysfs_exit(). (Again no longer relevant
if the code is moved.)

> +
>  	pr_debug("dmi-sysfs: loaded.\n");
>  
>  	return 0;
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 420c8d8..99c5f6c 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -113,6 +113,8 @@ static void dmi_table(u8 *buf, int len, int num,
>  	}
>  }
>  
> +static u8 smbios_header[32];
> +static int smbios_header_size;
>  static phys_addr_t dmi_base;
>  static u16 dmi_len;
>  static u16 dmi_num;
> @@ -474,6 +476,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_header_size = buf[5];
> +		memcpy(smbios_header, buf, smbios_header_size);
>  
>  		/* Some BIOS report weird SMBIOS version, fix that up */
>  		switch (smbios_ver) {
> @@ -505,6 +509,8 @@ static int __init dmi_present(const u8 *buf)
>  				pr_info("SMBIOS %d.%d present.\n",
>  				       dmi_ver >> 8, dmi_ver & 0xFF);
>  			} else {
> +				smbios_header_size = 15;
> +				memcpy(smbios_header, buf, smbios_header_size);
>  				dmi_ver = (buf[14] & 0xF0) << 4 |
>  					   (buf[14] & 0x0F);
>  				pr_info("Legacy DMI %d.%d present.\n",
> @@ -531,6 +537,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_header_size = buf[6];
> +		memcpy(smbios_header, buf, smbios_header_size);
>  
>  		/*
>  		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
> @@ -944,3 +952,21 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device)
>  	}
>  }
>  EXPORT_SYMBOL_GPL(dmi_memdev_name);
> +
> +/**
> + * dmi_get_smbios_entry_area - copy SMBIOS entry point area to array.
> + * @size - pointer to assign actual size of SMBIOS entry point area.
> + *
> + * returns NULL if table is not available, otherwise returns pointer on
> + * SMBIOS entry point area array.
> + */
> +const u8 *dmi_get_smbios_entry_area(int *size)
> +{
> +	if (!smbios_header_size || !dmi_available)

I don't see why you need to check for !dmi_available. If
smbios_header_size is non-zero then the required data is available. It
is independent from dmi_walk_early() having succeeded or not.

If you really believe that this function should return NULL if
dmi_walk_early() failed (I don't), then you should be consistent and
only fill up smbios_header after dmi_walk_early() has been successfully
called.

> +		return NULL;
> +
> +	*size = smbios_header_size;
> +
> +	return smbios_header;
> +}
> +EXPORT_SYMBOL_GPL(dmi_get_smbios_entry_area);
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index f820f0a..8e1a28d 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -109,6 +109,7 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>  	void *private_data);
>  extern bool dmi_match(enum dmi_field f, const char *str);
>  extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
> +const u8 *dmi_get_smbios_entry_area(int *size);
>  
>  #else
>  
> @@ -140,6 +141,8 @@ static inline void dmi_memdev_name(u16 handle, const char **bank,
>  		const char **device) { }
>  static inline const struct dmi_system_id *
>  	dmi_first_match(const struct dmi_system_id *list) { return NULL; }
> +static inline const u8 *dmi_get_smbios_entry_area(int *size)
> +	{ return NULL; }
>  
>  #endif
>  

There's one thing I do not understand. I seem to understand that the
goal behind this patch is to be able to run dmidecode without /dev/mem.
Dmidecode currently reads 2 areas from /dev/mem: the 0xF0000-0xFFFFF
area in search of the entry point, and the DMI data table itself. With
this patch you make the entry point available through sysfs. But
dmidecode will still need to access /dev/mem to access the DMI data
table. So that does not really solve anything, does it?

If we expose the raw DMI/SMBIOS entry point through sysfs, I believe we
want to expose the DMI table there too.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
  2015-02-26  9:36 ` [dmidecode] " Jean Delvare
@ 2015-03-04 12:30   ` Ivan.khoronzhuk
  2015-03-10  9:13       ` Jean Delvare
  0 siblings, 1 reply; 19+ messages in thread
From: Ivan.khoronzhuk @ 2015-03-04 12:30 UTC (permalink / raw)
  To: Jean Delvare, Ivan Khoronzhuk
  Cc: linux-kernel, ard.biesheuvel, grant.likely, matt.fleming,
	linux-api, linux-doc, dmidecode-devel, leif.lindholm, msalter


On 02/26/2015 11:36 AM, Jean Delvare wrote:
> Hi Ivan,
>
> Sorry for the late review.
>
> On Wed,  4 Feb 2015 19:06:03 +0200, 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. So this patch adds SMBIOS area to
>> dmi-sysfs in order to allow utils in question to work correctly with
>> dmi sysfs interface.
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> ---
>>
>> v1: https://lkml.org/lkml/2015/1/23/643
>> v2: https://lkml.org/lkml/2015/1/26/345
>> v3: https://lkml.org/lkml/2015/1/28/768
>>
>> v4..v2:
> Please always provide a list of changes from the previous version of
> the patch, otherwise it's quite confusing.

Typo v4..v2 -> v4..v3

>
>>    firmware: dmi_scan: add symbol to get SMBIOS entry area
>>    	- used u8 type for smbios_header var
>>    firmware: dmi-sysfs: add SMBIOS entry point area attribute
>>    	- replaced -ENODATA on -EINVAL
>>
>> v3..v2:
>>    firmware: dmi_scan: add symbol to get SMBIOS entry area
>>    firmware: dmi-sysfs: add SMBIOS entry point area attribute
>> 	- combined in one patch
>> 	- added SMBIOS information to ABI sysfs-dmi documentaton
>>
>> v2..v1:
>>    firmware: dmi_scan: add symbol to get SMBIOS entry area
>> 	- used additional static var to hold SMBIOS raw table size
>> 	- changed format of get_smbios_entry_area symbol
>> 	  returned pointer on const smbios table
>>
>>    firmware: dmi-sysfs: add SMBIOS entry point area attribute
>> 	- adopted to updated get_smbios_entry_area symbol
>>    	- removed redundant array to save smbios table
>>
>>   Documentation/ABI/testing/sysfs-firmware-dmi | 10 +++++++
>>   drivers/firmware/dmi-sysfs.c                 | 42 ++++++++++++++++++++++++++++
>>   drivers/firmware/dmi_scan.c                  | 26 +++++++++++++++++
>>   include/linux/dmi.h                          |  3 ++
>>   4 files changed, 81 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
>> index c78f9ab..3a9ffe8 100644
>> --- a/Documentation/ABI/testing/sysfs-firmware-dmi
>> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi
>> @@ -12,6 +12,16 @@ Description:
>>   		cannot ensure that the data as exported to userland is
>>   		without error either.
>>   
>> +		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_raw_header. The format of SMBIOS
> As mentioned before, I don't like the name "smbios_raw_header". I think
> it should be "smbios_entry_point" or similar.

If Matt is OK to get another version,
Let it be smbios_entry_point.
If it's more convenient, it should be changed while it's possible.

>
>> +		entry point header can be read in SMBIOS specification.
>> +
>>   		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
>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>> index e0f1cb3..9b396d7 100644
>> --- a/drivers/firmware/dmi-sysfs.c
>> +++ b/drivers/firmware/dmi-sysfs.c
>> @@ -29,6 +29,8 @@
>>   #define MAX_ENTRY_TYPE 255 /* Most of these aren't used, but we consider
>>   			      the top entry type is only 8 bits */
>>   
>> +static const u8 *smbios_raw_header;
>> +
>>   struct dmi_sysfs_entry {
>>   	struct dmi_header dh;
>>   	struct kobject kobj;
>> @@ -646,9 +648,37 @@ static void cleanup_entry_list(void)
>>   	}
>>   }
>>   
>> +static ssize_t smbios_entry_area_raw_read(struct file *filp,
> This is confusing again, now it's named "entry_area"? Please be
> consistent and use entry_point everywhere.
>
> As mentioned before I believe that this code should live in dmi_scan
> and not dmi-sysfs.
>
>> +					  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_raw_header[pos], size);
>> +
>> +	return size;
>> +}
>> +
>> +static struct bin_attribute smbios_raw_area_attr = {
>> +	.read = smbios_entry_area_raw_read,
>> +	.attr = {.name = "smbios_raw_header", .mode = 0400},
>> +};
>> +
>>   static int __init dmi_sysfs_init(void)
>>   {
>>   	int error = -ENOMEM;
>> +	int size;
>>   	int val;
>>   
>>   	/* Set up our directory */
>> @@ -669,6 +699,18 @@ static int __init dmi_sysfs_init(void)
>>   		goto err;
>>   	}
>>   
>> +	smbios_raw_header = dmi_get_smbios_entry_area(&size);
>> +	if (!smbios_raw_header) {
>> +		pr_debug("dmi-sysfs: SMBIOS raw data is not available.\n");
>> +		error = -EINVAL;
>> +		goto err;
>> +	}
> I don't think this should have been a fatal error. Just because for
> some reason dmi_get_smbios_entry_area() returned NULL is no good reason
> for nor exposing /sys/firmware/dmi/entries as we used to.

It issues an error only in case of when entry table is not available,
if entry point is absent then dmi table is not available a fortiori.
So there is no reason to continue from that point.

> But anyway this is no longer relevant if the code is moved to dmi_scan
> as I suggested.
>
>> +
>> +	/* Create the raw binary file to access the entry area */
>> +	smbios_raw_area_attr.size = size;
>> +	if (sysfs_create_bin_file(dmi_kobj, &smbios_raw_area_attr))
>> +		goto err;
> I think this should have had a corresponding call to
> sysfs_remove_bin_file() in dmi_sysfs_exit(). (Again no longer relevant
> if the code is moved.)

The removing is done in kobject_del().
Doesn't it? In another way it should be done for
dmi/entries/*/raw attributes also.

>> +
>>   	pr_debug("dmi-sysfs: loaded.\n");
>>   
>>   	return 0;
>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>> index 420c8d8..99c5f6c 100644
>> --- a/drivers/firmware/dmi_scan.c
>> +++ b/drivers/firmware/dmi_scan.c
>> @@ -113,6 +113,8 @@ static void dmi_table(u8 *buf, int len, int num,
>>   	}
>>   }
>>   
>> +static u8 smbios_header[32];
>> +static int smbios_header_size;
>>   static phys_addr_t dmi_base;
>>   static u16 dmi_len;
>>   static u16 dmi_num;
>> @@ -474,6 +476,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_header_size = buf[5];
>> +		memcpy(smbios_header, buf, smbios_header_size);
>>   
>>   		/* Some BIOS report weird SMBIOS version, fix that up */
>>   		switch (smbios_ver) {
>> @@ -505,6 +509,8 @@ static int __init dmi_present(const u8 *buf)
>>   				pr_info("SMBIOS %d.%d present.\n",
>>   				       dmi_ver >> 8, dmi_ver & 0xFF);
>>   			} else {
>> +				smbios_header_size = 15;
>> +				memcpy(smbios_header, buf, smbios_header_size);
>>   				dmi_ver = (buf[14] & 0xF0) << 4 |
>>   					   (buf[14] & 0x0F);
>>   				pr_info("Legacy DMI %d.%d present.\n",
>> @@ -531,6 +537,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_header_size = buf[6];
>> +		memcpy(smbios_header, buf, smbios_header_size);
>>   
>>   		/*
>>   		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
>> @@ -944,3 +952,21 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device)
>>   	}
>>   }
>>   EXPORT_SYMBOL_GPL(dmi_memdev_name);
>> +
>> +/**
>> + * dmi_get_smbios_entry_area - copy SMBIOS entry point area to array.
>> + * @size - pointer to assign actual size of SMBIOS entry point area.
>> + *
>> + * returns NULL if table is not available, otherwise returns pointer on
>> + * SMBIOS entry point area array.
>> + */
>> +const u8 *dmi_get_smbios_entry_area(int *size)
>> +{
>> +	if (!smbios_header_size || !dmi_available)
> I don't see why you need to check for !dmi_available. If
> smbios_header_size is non-zero then the required data is available. It
> is independent from dmi_walk_early() having succeeded or not.

Probably you are right.
It's better to check only smbios_header_size.

>
> If you really believe that this function should return NULL if
> dmi_walk_early() failed (I don't), then you should be consistent and
> only fill up smbios_header after dmi_walk_early() has been successfully
> called.
>
>> +		return NULL;
>> +
>> +	*size = smbios_header_size;
>> +
>> +	return smbios_header;
>> +}
>> +EXPORT_SYMBOL_GPL(dmi_get_smbios_entry_area);
>> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
>> index f820f0a..8e1a28d 100644
>> --- a/include/linux/dmi.h
>> +++ b/include/linux/dmi.h
>> @@ -109,6 +109,7 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>>   	void *private_data);
>>   extern bool dmi_match(enum dmi_field f, const char *str);
>>   extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
>> +const u8 *dmi_get_smbios_entry_area(int *size);
>>   
>>   #else
>>   
>> @@ -140,6 +141,8 @@ static inline void dmi_memdev_name(u16 handle, const char **bank,
>>   		const char **device) { }
>>   static inline const struct dmi_system_id *
>>   	dmi_first_match(const struct dmi_system_id *list) { return NULL; }
>> +static inline const u8 *dmi_get_smbios_entry_area(int *size)
>> +	{ return NULL; }
>>   
>>   #endif
>>   
> There's one thing I do not understand. I seem to understand that the
> goal behind this patch is to be able to run dmidecode without /dev/mem.
> Dmidecode currently reads 2 areas from /dev/mem: the 0xF0000-0xFFFFF
> area in search of the entry point, and the DMI data table itself. With
> this patch you make the entry point available through sysfs. But
> dmidecode will still need to access /dev/mem to access the DMI data
> table. So that does not really solve anything, does it?

It's supposed to read DMI table via entries presented by dmi-sysfs.
It contains raw attributes that can be used for these purposes.
No need to use /dev/mem.

Another case if you want to add binary of whole dmi table to be able to
read it directly in order to parse in dmidecode w/o any additional headache
with open/close. Well, it partly dupes currently present dmi-sysfs.
But it simplifies dmi table parsing for dmidecode, and who wants to use
dmi-sysfs, let them use it, but dmidecode will be reading raw entry.
Well let it be. Why not.

If others are OK, for dmidecode, and probably others tools also,
kernel can constantly expose two tables under /sys/firmware/dmi/tables/
smbios_entry_point and dmi_table. Independently on dmi-sysfs.


>
> If we expose the raw DMI/SMBIOS entry point through sysfs, I believe we
> want to expose the DMI table there too.
>
> Thanks,

-- 
Regards,
Ivan Khoronzhuk


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
@ 2015-03-10  9:13       ` Jean Delvare
  0 siblings, 0 replies; 19+ messages in thread
From: Jean Delvare @ 2015-03-10  9:13 UTC (permalink / raw)
  To: Ivan.khoronzhuk
  Cc: Ivan Khoronzhuk, linux-kernel, ard.biesheuvel, grant.likely,
	matt.fleming, linux-api, linux-doc, dmidecode-devel,
	leif.lindholm, msalter

Hi Ivan,

Sorry for the late reply. I think I addressed some points in other
replies already, but for completeness let me reply to this post too.

Le Wednesday 04 March 2015 à 14:30 +0200, Ivan.khoronzhuk a écrit :
> On 02/26/2015 11:36 AM, Jean Delvare wrote:
> > On Wed,  4 Feb 2015 19:06:03 +0200, Ivan Khoronzhuk wrote:
> >> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
> >> index c78f9ab..3a9ffe8 100644
> >> --- a/Documentation/ABI/testing/sysfs-firmware-dmi
> >> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi
> >> @@ -12,6 +12,16 @@ Description:
> >>   		cannot ensure that the data as exported to userland is
> >>   		without error either.
> >>   
> >> +		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_raw_header. The format of SMBIOS
> > As mentioned before, I don't like the name "smbios_raw_header". I think
> > it should be "smbios_entry_point" or similar.
> 
> If Matt is OK to get another version,
> Let it be smbios_entry_point.
> If it's more convenient, it should be changed while it's possible.

Great, thanks.

> >> @@ -669,6 +699,18 @@ static int __init dmi_sysfs_init(void)
> >>   		goto err;
> >>   	}
> >>   
> >> +	smbios_raw_header = dmi_get_smbios_entry_area(&size);
> >> +	if (!smbios_raw_header) {
> >> +		pr_debug("dmi-sysfs: SMBIOS raw data is not available.\n");
> >> +		error = -EINVAL;
> >> +		goto err;
> >> +	}
> > I don't think this should have been a fatal error. Just because for
> > some reason dmi_get_smbios_entry_area() returned NULL is no good reason
> > for nor exposing /sys/firmware/dmi/entries as we used to.
> 
> It issues an error only in case of when entry table is not available,
> if entry point is absent then dmi table is not available a fortiori.
> So there is no reason to continue from that point.

Well it could also fail because of an error in the code ;-) But OK, I
agree with you here.

> > But anyway this is no longer relevant if the code is moved to dmi_scan
> > as I suggested.
> >
> >> +
> >> +	/* Create the raw binary file to access the entry area */
> >> +	smbios_raw_area_attr.size = size;
> >> +	if (sysfs_create_bin_file(dmi_kobj, &smbios_raw_area_attr))
> >> +		goto err;
> > I think this should have had a corresponding call to
> > sysfs_remove_bin_file() in dmi_sysfs_exit(). (Again no longer relevant
> > if the code is moved.)
> 
> The removing is done in kobject_del().
> Doesn't it? In another way it should be done for
> dmi/entries/*/raw attributes also.

It _is_ done for other attributes already:

	kset_unregister(dmi_kset);

Which is exactly why I believe it should be done for
smbios_raw_area_attr too. All other kernel drivers are calling
sysfs_create_bin_file() or equivalent in their cleanup function and I
see no reason why this driver would be an exception.

> > There's one thing I do not understand. I seem to understand that the
> > goal behind this patch is to be able to run dmidecode without /dev/mem.
> > Dmidecode currently reads 2 areas from /dev/mem: the 0xF0000-0xFFFFF
> > area in search of the entry point, and the DMI data table itself. With
> > this patch you make the entry point available through sysfs. But
> > dmidecode will still need to access /dev/mem to access the DMI data
> > table. So that does not really solve anything, does it?
> 
> It's supposed to read DMI table via entries presented by dmi-sysfs.
> It contains raw attributes that can be used for these purposes.
> No need to use /dev/mem.

Yes, I understood this meanwhile, sorry.

> Another case if you want to add binary of whole dmi table to be able to
> read it directly in order to parse in dmidecode w/o any additional headache
> with open/close. Well, it partly dupes currently present dmi-sysfs.

In fact dmi-sysfs already duplicates a lot of code which was already
present in dmidecode and libsmbios. And exporting the raw table will
require way less code than the implementation you proposed originally.
So the code duplication argument doesn't hold, sorry.

> But it simplifies dmi table parsing for dmidecode, and who wants to use
> dmi-sysfs, let them use it, but dmidecode will be reading raw entry.
> Well let it be. Why not.

Yes, this is exactly my point.

> If others are OK, for dmidecode, and probably others tools also,
> kernel can constantly expose two tables under /sys/firmware/dmi/tables/
> smbios_entry_point and dmi_table. Independently on dmi-sysfs.

That's what I would like to see implemented, yes, thank you very much.

-- 
Jean Delvare
SUSE L3 Support


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
@ 2015-03-10  9:13       ` Jean Delvare
  0 siblings, 0 replies; 19+ messages in thread
From: Jean Delvare @ 2015-03-10  9:13 UTC (permalink / raw)
  To: Ivan.khoronzhuk
  Cc: Ivan Khoronzhuk, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
	grant.likely-QSEj5FYQhm4dnm+yROfE0A,
	matt.fleming-ral2JQCrhuEAvxtiuMwx3w,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	dmidecode-devel-qX2TKyscuCcdnm+yROfE0A,
	leif.lindholm-QSEj5FYQhm4dnm+yROfE0A,
	msalter-H+wXaHxf7aLQT0dZR+AlfA

Hi Ivan,

Sorry for the late reply. I think I addressed some points in other
replies already, but for completeness let me reply to this post too.

Le Wednesday 04 March 2015 à 14:30 +0200, Ivan.khoronzhuk a écrit :
> On 02/26/2015 11:36 AM, Jean Delvare wrote:
> > On Wed,  4 Feb 2015 19:06:03 +0200, Ivan Khoronzhuk wrote:
> >> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
> >> index c78f9ab..3a9ffe8 100644
> >> --- a/Documentation/ABI/testing/sysfs-firmware-dmi
> >> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi
> >> @@ -12,6 +12,16 @@ Description:
> >>   		cannot ensure that the data as exported to userland is
> >>   		without error either.
> >>   
> >> +		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_raw_header. The format of SMBIOS
> > As mentioned before, I don't like the name "smbios_raw_header". I think
> > it should be "smbios_entry_point" or similar.
> 
> If Matt is OK to get another version,
> Let it be smbios_entry_point.
> If it's more convenient, it should be changed while it's possible.

Great, thanks.

> >> @@ -669,6 +699,18 @@ static int __init dmi_sysfs_init(void)
> >>   		goto err;
> >>   	}
> >>   
> >> +	smbios_raw_header = dmi_get_smbios_entry_area(&size);
> >> +	if (!smbios_raw_header) {
> >> +		pr_debug("dmi-sysfs: SMBIOS raw data is not available.\n");
> >> +		error = -EINVAL;
> >> +		goto err;
> >> +	}
> > I don't think this should have been a fatal error. Just because for
> > some reason dmi_get_smbios_entry_area() returned NULL is no good reason
> > for nor exposing /sys/firmware/dmi/entries as we used to.
> 
> It issues an error only in case of when entry table is not available,
> if entry point is absent then dmi table is not available a fortiori.
> So there is no reason to continue from that point.

Well it could also fail because of an error in the code ;-) But OK, I
agree with you here.

> > But anyway this is no longer relevant if the code is moved to dmi_scan
> > as I suggested.
> >
> >> +
> >> +	/* Create the raw binary file to access the entry area */
> >> +	smbios_raw_area_attr.size = size;
> >> +	if (sysfs_create_bin_file(dmi_kobj, &smbios_raw_area_attr))
> >> +		goto err;
> > I think this should have had a corresponding call to
> > sysfs_remove_bin_file() in dmi_sysfs_exit(). (Again no longer relevant
> > if the code is moved.)
> 
> The removing is done in kobject_del().
> Doesn't it? In another way it should be done for
> dmi/entries/*/raw attributes also.

It _is_ done for other attributes already:

	kset_unregister(dmi_kset);

Which is exactly why I believe it should be done for
smbios_raw_area_attr too. All other kernel drivers are calling
sysfs_create_bin_file() or equivalent in their cleanup function and I
see no reason why this driver would be an exception.

> > There's one thing I do not understand. I seem to understand that the
> > goal behind this patch is to be able to run dmidecode without /dev/mem.
> > Dmidecode currently reads 2 areas from /dev/mem: the 0xF0000-0xFFFFF
> > area in search of the entry point, and the DMI data table itself. With
> > this patch you make the entry point available through sysfs. But
> > dmidecode will still need to access /dev/mem to access the DMI data
> > table. So that does not really solve anything, does it?
> 
> It's supposed to read DMI table via entries presented by dmi-sysfs.
> It contains raw attributes that can be used for these purposes.
> No need to use /dev/mem.

Yes, I understood this meanwhile, sorry.

> Another case if you want to add binary of whole dmi table to be able to
> read it directly in order to parse in dmidecode w/o any additional headache
> with open/close. Well, it partly dupes currently present dmi-sysfs.

In fact dmi-sysfs already duplicates a lot of code which was already
present in dmidecode and libsmbios. And exporting the raw table will
require way less code than the implementation you proposed originally.
So the code duplication argument doesn't hold, sorry.

> But it simplifies dmi table parsing for dmidecode, and who wants to use
> dmi-sysfs, let them use it, but dmidecode will be reading raw entry.
> Well let it be. Why not.

Yes, this is exactly my point.

> If others are OK, for dmidecode, and probably others tools also,
> kernel can constantly expose two tables under /sys/firmware/dmi/tables/
> smbios_entry_point and dmi_table. Independently on dmi-sysfs.

That's what I would like to see implemented, yes, thank you very much.

-- 
Jean Delvare
SUSE L3 Support

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
@ 2015-03-16 21:02           ` Ivan.khoronzhuk
  0 siblings, 0 replies; 19+ messages in thread
From: Ivan.khoronzhuk @ 2015-03-16 21:02 UTC (permalink / raw)
  To: Matt Fleming, Jean Delvare
  Cc: Ivan Khoronzhuk, linux-kernel, ard.biesheuvel, grant.likely,
	linux-api, linux-doc, dmidecode-devel, leif.lindholm, msalter



On 12.03.15 14:22, Matt Fleming wrote:
> On Tue, 2015-03-10 at 10:13 +0100, Jean Delvare wrote:
>>> If Matt is OK to get another version,
>>> Let it be smbios_entry_point.
>>> If it's more convenient, it should be changed while it's possible.
>> Great, thanks.
> Ivan, please go ahead and submit a new version, we've got time to get
> this right for v4.1.
>

Matt, I've sent new patch that replaces this one.
"[Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs"
It can take a while to go via review.

-- 
Regards,
Ivan Khoronzhuk


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
@ 2015-03-16 21:02           ` Ivan.khoronzhuk
  0 siblings, 0 replies; 19+ messages in thread
From: Ivan.khoronzhuk @ 2015-03-16 21:02 UTC (permalink / raw)
  To: Matt Fleming, Jean Delvare
  Cc: Ivan Khoronzhuk, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
	grant.likely-QSEj5FYQhm4dnm+yROfE0A,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	dmidecode-devel-qX2TKyscuCcdnm+yROfE0A,
	leif.lindholm-QSEj5FYQhm4dnm+yROfE0A,
	msalter-H+wXaHxf7aLQT0dZR+AlfA



On 12.03.15 14:22, Matt Fleming wrote:
> On Tue, 2015-03-10 at 10:13 +0100, Jean Delvare wrote:
>>> If Matt is OK to get another version,
>>> Let it be smbios_entry_point.
>>> If it's more convenient, it should be changed while it's possible.
>> Great, thanks.
> Ivan, please go ahead and submit a new version, we've got time to get
> this right for v4.1.
>

Matt, I've sent new patch that replaces this one.
"[Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs"
It can take a while to go via review.

-- 
Regards,
Ivan Khoronzhuk

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
@ 2015-03-19 17:33         ` Ivan.khoronzhuk
  0 siblings, 0 replies; 19+ messages in thread
From: Ivan.khoronzhuk @ 2015-03-19 17:33 UTC (permalink / raw)
  To: Jean Delvare
  Cc: Ivan Khoronzhuk, linux-kernel, ard.biesheuvel, grant.likely,
	matt.fleming, linux-api, linux-doc, dmidecode-devel,
	leif.lindholm, msalter



On 10.03.15 11:13, Jean Delvare wrote:
> Hi Ivan,
>
> Sorry for the late reply. I think I addressed some points in other
> replies already, but for completeness let me reply to this post too.
>
> Le Wednesday 04 March 2015 à 14:30 +0200, Ivan.khoronzhuk a écrit :
>> On 02/26/2015 11:36 AM, Jean Delvare wrote:
>>> On Wed,  4 Feb 2015 19:06:03 +0200, Ivan Khoronzhuk wrote:
>>>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
>>>> index c78f9ab..3a9ffe8 100644
>>>> --- a/Documentation/ABI/testing/sysfs-firmware-dmi
>>>> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi
>>>> @@ -12,6 +12,16 @@ Description:
>>>>    		cannot ensure that the data as exported to userland is
>>>>    		without error either.
>>>>    
>>>> +		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_raw_header. The format of SMBIOS
>>> As mentioned before, I don't like the name "smbios_raw_header". I think
>>> it should be "smbios_entry_point" or similar.
>> If Matt is OK to get another version,
>> Let it be smbios_entry_point.
>> If it's more convenient, it should be changed while it's possible.
> Great, thanks.
>
>>>> @@ -669,6 +699,18 @@ static int __init dmi_sysfs_init(void)
>>>>    		goto err;
>>>>    	}
>>>>    
>>>> +	smbios_raw_header = dmi_get_smbios_entry_area(&size);
>>>> +	if (!smbios_raw_header) {
>>>> +		pr_debug("dmi-sysfs: SMBIOS raw data is not available.\n");
>>>> +		error = -EINVAL;
>>>> +		goto err;
>>>> +	}
>>> I don't think this should have been a fatal error. Just because for
>>> some reason dmi_get_smbios_entry_area() returned NULL is no good reason
>>> for nor exposing /sys/firmware/dmi/entries as we used to.
>> It issues an error only in case of when entry table is not available,
>> if entry point is absent then dmi table is not available a fortiori.
>> So there is no reason to continue from that point.
> Well it could also fail because of an error in the code ;-) But OK, I
> agree with you here.
>
>>> But anyway this is no longer relevant if the code is moved to dmi_scan
>>> as I suggested.
>>>
>>>> +
>>>> +	/* Create the raw binary file to access the entry area */
>>>> +	smbios_raw_area_attr.size = size;
>>>> +	if (sysfs_create_bin_file(dmi_kobj, &smbios_raw_area_attr))
>>>> +		goto err;
>>> I think this should have had a corresponding call to
>>> sysfs_remove_bin_file() in dmi_sysfs_exit(). (Again no longer relevant
>>> if the code is moved.)
>> The removing is done in kobject_del().
>> Doesn't it? In another way it should be done for
>> dmi/entries/*/raw attributes also.
> It _is_ done for other attributes already:
>
> 	kset_unregister(dmi_kset);
>
> Which is exactly why I believe it should be done for
> smbios_raw_area_attr too. All other kernel drivers are calling
> sysfs_create_bin_file() or equivalent in their cleanup function and I
> see no reason why this driver would be an exception.

kset_unregister() uses kobject_del()
no see difference.

>
>>> There's one thing I do not understand. I seem to understand that the
>>> goal behind this patch is to be able to run dmidecode without /dev/mem.
>>> Dmidecode currently reads 2 areas from /dev/mem: the 0xF0000-0xFFFFF
>>> area in search of the entry point, and the DMI data table itself. With
>>> this patch you make the entry point available through sysfs. But
>>> dmidecode will still need to access /dev/mem to access the DMI data
>>> table. So that does not really solve anything, does it?
>> It's supposed to read DMI table via entries presented by dmi-sysfs.
>> It contains raw attributes that can be used for these purposes.
>> No need to use /dev/mem.
> Yes, I understood this meanwhile, sorry.
>
>> Another case if you want to add binary of whole dmi table to be able to
>> read it directly in order to parse in dmidecode w/o any additional headache
>> with open/close. Well, it partly dupes currently present dmi-sysfs.
> In fact dmi-sysfs already duplicates a lot of code which was already
> present in dmidecode and libsmbios. And exporting the raw table will
> require way less code than the implementation you proposed originally.
> So the code duplication argument doesn't hold, sorry.
>
>> But it simplifies dmi table parsing for dmidecode, and who wants to use
>> dmi-sysfs, let them use it, but dmidecode will be reading raw entry.
>> Well let it be. Why not.
> Yes, this is exactly my point.
>
>> If others are OK, for dmidecode, and probably others tools also,
>> kernel can constantly expose two tables under /sys/firmware/dmi/tables/
>> smbios_entry_point and dmi_table. Independently on dmi-sysfs.
> That's what I would like to see implemented, yes, thank you very much.
>

-- 
Regards,
Ivan Khoronzhuk


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
@ 2015-03-19 17:33         ` Ivan.khoronzhuk
  0 siblings, 0 replies; 19+ messages in thread
From: Ivan.khoronzhuk @ 2015-03-19 17:33 UTC (permalink / raw)
  To: Jean Delvare
  Cc: Ivan Khoronzhuk, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
	grant.likely-QSEj5FYQhm4dnm+yROfE0A,
	matt.fleming-ral2JQCrhuEAvxtiuMwx3w,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	dmidecode-devel-qX2TKyscuCcdnm+yROfE0A,
	leif.lindholm-QSEj5FYQhm4dnm+yROfE0A,
	msalter-H+wXaHxf7aLQT0dZR+AlfA



On 10.03.15 11:13, Jean Delvare wrote:
> Hi Ivan,
>
> Sorry for the late reply. I think I addressed some points in other
> replies already, but for completeness let me reply to this post too.
>
> Le Wednesday 04 March 2015 à 14:30 +0200, Ivan.khoronzhuk a écrit :
>> On 02/26/2015 11:36 AM, Jean Delvare wrote:
>>> On Wed,  4 Feb 2015 19:06:03 +0200, Ivan Khoronzhuk wrote:
>>>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
>>>> index c78f9ab..3a9ffe8 100644
>>>> --- a/Documentation/ABI/testing/sysfs-firmware-dmi
>>>> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi
>>>> @@ -12,6 +12,16 @@ Description:
>>>>    		cannot ensure that the data as exported to userland is
>>>>    		without error either.
>>>>    
>>>> +		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_raw_header. The format of SMBIOS
>>> As mentioned before, I don't like the name "smbios_raw_header". I think
>>> it should be "smbios_entry_point" or similar.
>> If Matt is OK to get another version,
>> Let it be smbios_entry_point.
>> If it's more convenient, it should be changed while it's possible.
> Great, thanks.
>
>>>> @@ -669,6 +699,18 @@ static int __init dmi_sysfs_init(void)
>>>>    		goto err;
>>>>    	}
>>>>    
>>>> +	smbios_raw_header = dmi_get_smbios_entry_area(&size);
>>>> +	if (!smbios_raw_header) {
>>>> +		pr_debug("dmi-sysfs: SMBIOS raw data is not available.\n");
>>>> +		error = -EINVAL;
>>>> +		goto err;
>>>> +	}
>>> I don't think this should have been a fatal error. Just because for
>>> some reason dmi_get_smbios_entry_area() returned NULL is no good reason
>>> for nor exposing /sys/firmware/dmi/entries as we used to.
>> It issues an error only in case of when entry table is not available,
>> if entry point is absent then dmi table is not available a fortiori.
>> So there is no reason to continue from that point.
> Well it could also fail because of an error in the code ;-) But OK, I
> agree with you here.
>
>>> But anyway this is no longer relevant if the code is moved to dmi_scan
>>> as I suggested.
>>>
>>>> +
>>>> +	/* Create the raw binary file to access the entry area */
>>>> +	smbios_raw_area_attr.size = size;
>>>> +	if (sysfs_create_bin_file(dmi_kobj, &smbios_raw_area_attr))
>>>> +		goto err;
>>> I think this should have had a corresponding call to
>>> sysfs_remove_bin_file() in dmi_sysfs_exit(). (Again no longer relevant
>>> if the code is moved.)
>> The removing is done in kobject_del().
>> Doesn't it? In another way it should be done for
>> dmi/entries/*/raw attributes also.
> It _is_ done for other attributes already:
>
> 	kset_unregister(dmi_kset);
>
> Which is exactly why I believe it should be done for
> smbios_raw_area_attr too. All other kernel drivers are calling
> sysfs_create_bin_file() or equivalent in their cleanup function and I
> see no reason why this driver would be an exception.

kset_unregister() uses kobject_del()
no see difference.

>
>>> There's one thing I do not understand. I seem to understand that the
>>> goal behind this patch is to be able to run dmidecode without /dev/mem.
>>> Dmidecode currently reads 2 areas from /dev/mem: the 0xF0000-0xFFFFF
>>> area in search of the entry point, and the DMI data table itself. With
>>> this patch you make the entry point available through sysfs. But
>>> dmidecode will still need to access /dev/mem to access the DMI data
>>> table. So that does not really solve anything, does it?
>> It's supposed to read DMI table via entries presented by dmi-sysfs.
>> It contains raw attributes that can be used for these purposes.
>> No need to use /dev/mem.
> Yes, I understood this meanwhile, sorry.
>
>> Another case if you want to add binary of whole dmi table to be able to
>> read it directly in order to parse in dmidecode w/o any additional headache
>> with open/close. Well, it partly dupes currently present dmi-sysfs.
> In fact dmi-sysfs already duplicates a lot of code which was already
> present in dmidecode and libsmbios. And exporting the raw table will
> require way less code than the implementation you proposed originally.
> So the code duplication argument doesn't hold, sorry.
>
>> But it simplifies dmi table parsing for dmidecode, and who wants to use
>> dmi-sysfs, let them use it, but dmidecode will be reading raw entry.
>> Well let it be. Why not.
> Yes, this is exactly my point.
>
>> If others are OK, for dmidecode, and probably others tools also,
>> kernel can constantly expose two tables under /sys/firmware/dmi/tables/
>> smbios_entry_point and dmi_table. Independently on dmi-sysfs.
> That's what I would like to see implemented, yes, thank you very much.
>

-- 
Regards,
Ivan Khoronzhuk

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
  2015-03-16 21:02           ` Ivan.khoronzhuk
  (?)
@ 2015-03-26 13:05           ` Matt Fleming
  2015-03-26 13:06             ` Ivan.khoronzhuk
  -1 siblings, 1 reply; 19+ messages in thread
From: Matt Fleming @ 2015-03-26 13:05 UTC (permalink / raw)
  To: Ivan.khoronzhuk
  Cc: Matt Fleming, Jean Delvare, Ivan Khoronzhuk, linux-kernel,
	ard.biesheuvel, grant.likely, linux-api, linux-doc,
	dmidecode-devel, leif.lindholm, msalter

On Mon, 16 Mar, at 11:02:29PM, Ivan.khoronzhuk wrote:
> 
> Matt, I've sent new patch that replaces this one.
> "[Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs"
> It can take a while to go via review.

OK, thanks Ivan. I've dropped commit ("firmware: dmi-sysfs: Add SMBIOS
entry point area attribute").

The two remaining DMI commits I have are,

  59d7e3c02860 firmware: dmi_scan: Use direct access to static vars
  d759d96eb5cc firmware: dmi_scan: Use full dmi version for SMBIOS3

Am I still sending those for v4.1?

-- 
Matt Fleming, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
  2015-03-26 13:05           ` Matt Fleming
@ 2015-03-26 13:06             ` Ivan.khoronzhuk
  2015-03-26 14:23                 ` Jean Delvare
  0 siblings, 1 reply; 19+ messages in thread
From: Ivan.khoronzhuk @ 2015-03-26 13:06 UTC (permalink / raw)
  To: Matt Fleming
  Cc: Matt Fleming, Jean Delvare, Ivan Khoronzhuk, linux-kernel,
	ard.biesheuvel, grant.likely, linux-api, linux-doc,
	dmidecode-devel, leif.lindholm, msalter



On 26.03.15 15:05, Matt Fleming wrote:
> On Mon, 16 Mar, at 11:02:29PM, Ivan.khoronzhuk wrote:
>> Matt, I've sent new patch that replaces this one.
>> "[Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs"
>> It can take a while to go via review.
> OK, thanks Ivan. I've dropped commit ("firmware: dmi-sysfs: Add SMBIOS
> entry point area attribute").
>
> The two remaining DMI commits I have are,
>
>    59d7e3c02860 firmware: dmi_scan: Use direct access to static vars
>    d759d96eb5cc firmware: dmi_scan: Use full dmi version for SMBIOS3
>
> Am I still sending those for v4.1?
>

Yes

-- 
Regards,
Ivan Khoronzhuk


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
@ 2015-03-26 14:23                 ` Jean Delvare
  0 siblings, 0 replies; 19+ messages in thread
From: Jean Delvare @ 2015-03-26 14:23 UTC (permalink / raw)
  To: Ivan.khoronzhuk
  Cc: Matt Fleming, Matt Fleming, Ivan Khoronzhuk, linux-kernel,
	ard.biesheuvel, grant.likely, linux-api, linux-doc,
	dmidecode-devel, leif.lindholm, msalter

Le Thursday 26 March 2015 à 15:06 +0200, Ivan.khoronzhuk a écrit :
> 
> On 26.03.15 15:05, Matt Fleming wrote:
> > On Mon, 16 Mar, at 11:02:29PM, Ivan.khoronzhuk wrote:
> >> Matt, I've sent new patch that replaces this one.
> >> "[Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs"
> >> It can take a while to go via review.
> > OK, thanks Ivan. I've dropped commit ("firmware: dmi-sysfs: Add SMBIOS
> > entry point area attribute").
> >
> > The two remaining DMI commits I have are,
> >
> >    59d7e3c02860 firmware: dmi_scan: Use direct access to static vars
> >    d759d96eb5cc firmware: dmi_scan: Use full dmi version for SMBIOS3
> >
> > Am I still sending those for v4.1?

FWIW I have still not reviewed these, I would like to, but my current
health condition make my progress slower than usual :/

-- 
Jean Delvare
SUSE L3 Support


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
@ 2015-03-26 14:23                 ` Jean Delvare
  0 siblings, 0 replies; 19+ messages in thread
From: Jean Delvare @ 2015-03-26 14:23 UTC (permalink / raw)
  To: Ivan.khoronzhuk
  Cc: Matt Fleming, Matt Fleming, Ivan Khoronzhuk,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
	grant.likely-QSEj5FYQhm4dnm+yROfE0A,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	dmidecode-devel-qX2TKyscuCcdnm+yROfE0A,
	leif.lindholm-QSEj5FYQhm4dnm+yROfE0A,
	msalter-H+wXaHxf7aLQT0dZR+AlfA

Le Thursday 26 March 2015 à 15:06 +0200, Ivan.khoronzhuk a écrit :
> 
> On 26.03.15 15:05, Matt Fleming wrote:
> > On Mon, 16 Mar, at 11:02:29PM, Ivan.khoronzhuk wrote:
> >> Matt, I've sent new patch that replaces this one.
> >> "[Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs"
> >> It can take a while to go via review.
> > OK, thanks Ivan. I've dropped commit ("firmware: dmi-sysfs: Add SMBIOS
> > entry point area attribute").
> >
> > The two remaining DMI commits I have are,
> >
> >    59d7e3c02860 firmware: dmi_scan: Use direct access to static vars
> >    d759d96eb5cc firmware: dmi_scan: Use full dmi version for SMBIOS3
> >
> > Am I still sending those for v4.1?

FWIW I have still not reviewed these, I would like to, but my current
health condition make my progress slower than usual :/

-- 
Jean Delvare
SUSE L3 Support

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2015-03-26 14:23 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-04 17:06 [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute Ivan Khoronzhuk
2015-02-04 17:06 ` Ivan Khoronzhuk
2015-02-10  9:51 ` Ivan Khoronzhuk
2015-02-11 14:17   ` Matt Fleming
2015-02-11 14:43     ` Matt Fleming
2015-02-12  2:33       ` Ivan Khoronzhuk
2015-02-12  2:33         ` Ivan Khoronzhuk
2015-02-26  9:36 ` [dmidecode] " Jean Delvare
2015-03-04 12:30   ` Ivan.khoronzhuk
2015-03-10  9:13     ` Jean Delvare
2015-03-10  9:13       ` Jean Delvare
     [not found]       ` <1426162975.2784.31.camel@mfleming-mobl1.ger.corp.intel.com>
2015-03-16 21:02         ` Ivan.khoronzhuk
2015-03-16 21:02           ` Ivan.khoronzhuk
2015-03-26 13:05           ` Matt Fleming
2015-03-26 13:06             ` Ivan.khoronzhuk
2015-03-26 14:23               ` Jean Delvare
2015-03-26 14:23                 ` Jean Delvare
2015-03-19 17:33       ` Ivan.khoronzhuk
2015-03-19 17:33         ` Ivan.khoronzhuk

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.