* [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-02 12:57 ` Ivan Khoronzhuk
0 siblings, 0 replies; 41+ messages in thread
From: Ivan Khoronzhuk @ 2015-04-02 12:57 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
matt.fleming-ral2JQCrhuEAvxtiuMwx3w, jdelvare-l3A5Bk7waGM,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, mikew-hpIqsD4AKlfQT0dZR+AlfA
Cc: dmidecode-devel-qX2TKyscuCcdnm+yROfE0A,
leif.lindholm-QSEj5FYQhm4dnm+yROfE0A,
msalter-H+wXaHxf7aLQT0dZR+AlfA, roy.franz-QSEj5FYQhm4dnm+yROfE0A,
Ivan Khoronzhuk
Some utils, like dmidecode and smbios, need to access SMBIOS entry
table area in order to get information like SMBIOS version, size, etc.
Currently it's done via /dev/mem. But for situation when /dev/mem
usage is disabled, the utils have to use dmi sysfs instead, which
doesn't represent SMBIOS entry and adds code/delay redundancy when direct
access for table is needed.
So this patch creates dmi/tables and adds SMBIOS entry point to allow
utils in question to work correctly without /dev/mem. Also patch adds
raw dmi table to simplify dmi table processing in user space, as
proposed by Jean Delvare.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
---
.../ABI/testing/sysfs-firmware-dmi-tables | 22 ++++++
drivers/firmware/dmi-sysfs.c | 11 ++-
drivers/firmware/dmi_scan.c | 80 ++++++++++++++++++++++
include/linux/dmi.h | 1 +
4 files changed, 107 insertions(+), 7 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
new file mode 100644
index 0000000..f46158c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
@@ -0,0 +1,22 @@
+What: /sys/firmware/dmi/tables/
+Date: April 2015
+Contact: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
+Description:
+ The firmware provides DMI structures as a packed list of
+ data referenced by a SMBIOS table entry point. The SMBIOS
+ entry point contains general information, like SMBIOS
+ version, DMI table size, etc. The structure, content and
+ size of SMBIOS entry point is dependent on SMBIOS version.
+ The format of SMBIOS entry point, equal as DMI structures
+ can be read in SMBIOS specification.
+
+ The dmi/tables provides raw SMBIOS entry point and DMI tables
+ through sysfs as an alternative to utilities reading them
+ from /dev/mem. The raw SMBIOS entry point and DMI table are
+ presented as raw attributes and are accessible via:
+
+ /sys/firmware/dmi/tables/smbios_entry_point
+ /sys/firmware/dmi/tables/DMI
+
+ The complete DMI information can be taken using these two
+ tables.
diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index e0f1cb3..8e1a411 100644
--- a/drivers/firmware/dmi-sysfs.c
+++ b/drivers/firmware/dmi-sysfs.c
@@ -566,7 +566,6 @@ static struct kobj_type dmi_sysfs_entry_ktype = {
.default_attrs = dmi_sysfs_entry_attrs,
};
-static struct kobject *dmi_kobj;
static struct kset *dmi_kset;
/* Global count of all instances seen. Only for setup */
@@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
int error = -ENOMEM;
int val;
- /* Set up our directory */
- dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
- if (!dmi_kobj)
+ if (!dmi_kobj) {
+ pr_err("dmi-sysfs: dmi entry is absent.\n");
+ error = -ENOSYS;
goto err;
+ }
dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
if (!dmi_kset)
@@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
err:
cleanup_entry_list();
kset_unregister(dmi_kset);
- kobject_put(dmi_kobj);
return error;
}
@@ -685,8 +684,6 @@ static void __exit dmi_sysfs_exit(void)
pr_debug("dmi-sysfs: unloading.\n");
cleanup_entry_list();
kset_unregister(dmi_kset);
- kobject_del(dmi_kobj);
- kobject_put(dmi_kobj);
}
module_init(dmi_sysfs_init);
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index d3aae09..bb19f8b 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -10,6 +10,9 @@
#include <asm/dmi.h>
#include <asm/unaligned.h>
+struct kobject *dmi_kobj;
+EXPORT_SYMBOL_GPL(dmi_kobj);
+
/*
* DMI stands for "Desktop Management Interface". It is part
* of and an antecedent to, SMBIOS, which stands for System
@@ -20,6 +23,9 @@ static const char dmi_empty_string[] = " ";
static u32 dmi_ver __initdata;
static u32 dmi_len;
static u16 dmi_num;
+static u8 smbios_entry_point[32];
+static int smbios_entry_point_size;
+
/*
* Catch too early calls to dmi_check_system():
*/
@@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
}
static phys_addr_t dmi_base;
+static u8 *dmi_table;
static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
void *))
@@ -476,6 +483,8 @@ static int __init dmi_present(const u8 *buf)
if (memcmp(buf, "_SM_", 4) == 0 &&
buf[5] < 32 && dmi_checksum(buf, buf[5])) {
smbios_ver = get_unaligned_be16(buf + 6);
+ smbios_entry_point_size = buf[5];
+ memcpy(smbios_entry_point, buf, smbios_entry_point_size);
/* Some BIOS report weird SMBIOS version, fix that up */
switch (smbios_ver) {
@@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
dmi_ver >> 8, dmi_ver & 0xFF,
(dmi_ver < 0x0300) ? "" : ".x");
} else {
+ smbios_entry_point_size = 15;
+ memcpy(smbios_entry_point, buf,
+ smbios_entry_point_size);
dmi_ver = (buf[14] & 0xF0) << 4 |
(buf[14] & 0x0F);
pr_info("Legacy DMI %d.%d present.\n",
@@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
dmi_ver &= 0xFFFFFF;
dmi_len = get_unaligned_le32(buf + 12);
dmi_base = get_unaligned_le64(buf + 16);
+ smbios_entry_point_size = buf[6];
+ memcpy(smbios_entry_point, buf, smbios_entry_point_size);
/*
* The 64-bit SMBIOS 3.0 entry point no longer has a field
@@ -638,6 +652,72 @@ void __init dmi_scan_machine(void)
dmi_initialized = 1;
}
+static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf,
+ loff_t pos, size_t count)
+{
+ memcpy(buf, attr->private + pos, count);
+ return count;
+}
+
+static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
+struct bin_attribute bin_attr_dmi_table =
+ __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
+
+static int __init dmi_init(void)
+{
+ int ret = -ENOMEM;
+ struct kobject *tables_kobj = NULL;
+
+ if (!dmi_available) {
+ ret = -ENOSYS;
+ goto err;
+ }
+
+ /* Set up dmi directory at /sys/firmware/dmi */
+ dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
+ if (!dmi_kobj)
+ goto err;
+
+ tables_kobj = kobject_create_and_add("tables", dmi_kobj);
+ if (!tables_kobj)
+ goto err;
+
+ bin_attr_smbios_entry_point.size = smbios_entry_point_size;
+ bin_attr_smbios_entry_point.private = smbios_entry_point;
+ ret = sysfs_create_bin_file(tables_kobj, &bin_attr_smbios_entry_point);
+ if (ret)
+ goto err;
+
+ dmi_table = dmi_remap(dmi_base, dmi_len);
+ if (!dmi_table)
+ goto err;
+
+ bin_attr_dmi_table.size = dmi_len;
+ bin_attr_dmi_table.private = dmi_table;
+ ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
+ if (ret) {
+ dmi_unmap(dmi_table);
+ goto err;
+ }
+
+ return 0;
+err:
+ pr_err("dmi: Firmware registration failed.\n");
+
+ if (tables_kobj)
+ sysfs_remove_bin_file(tables_kobj,
+ &bin_attr_smbios_entry_point);
+ kobject_del(tables_kobj);
+ kobject_put(tables_kobj);
+ kobject_del(dmi_kobj);
+ kobject_put(dmi_kobj);
+ dmi_kobj = NULL;
+
+ return ret;
+}
+subsys_initcall(dmi_init);
+
/**
* dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
*
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index f820f0a..9f55f46 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -93,6 +93,7 @@ struct dmi_dev_onboard {
int devfn;
};
+extern struct kobject *dmi_kobj;
extern int dmi_check_system(const struct dmi_system_id *list);
const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
extern const char * dmi_get_system_info(int field);
--
1.9.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
2015-04-02 12:57 ` Ivan Khoronzhuk
(?)
@ 2015-04-03 9:36 ` Ivan.khoronzhuk
2015-04-15 4:19 ` Roy Franz
-1 siblings, 1 reply; 41+ messages in thread
From: Ivan.khoronzhuk @ 2015-04-03 9:36 UTC (permalink / raw)
To: linux-kernel, matt.fleming, jdelvare, ard.biesheuvel,
grant.likely, linux-api, linux-doc, mikew
Cc: dmidecode-devel, leif.lindholm, msalter, roy.franz
On 02.04.15 15:57, Ivan Khoronzhuk wrote:
> Some utils, like dmidecode and smbios, need to access SMBIOS entry
> table area in order to get information like SMBIOS version, size, etc.
> Currently it's done via /dev/mem. But for situation when /dev/mem
> usage is disabled, the utils have to use dmi sysfs instead, which
> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
> access for table is needed.
>
> So this patch creates dmi/tables and adds SMBIOS entry point to allow
> utils in question to work correctly without /dev/mem. Also patch adds
> raw dmi table to simplify dmi table processing in user space, as
> proposed by Jean Delvare.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
> ---
> .../ABI/testing/sysfs-firmware-dmi-tables | 22 ++++++
> drivers/firmware/dmi-sysfs.c | 11 ++-
> drivers/firmware/dmi_scan.c | 80 ++++++++++++++++++++++
> include/linux/dmi.h | 1 +
> 4 files changed, 107 insertions(+), 7 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
>
> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
> new file mode 100644
> index 0000000..f46158c
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
> @@ -0,0 +1,22 @@
> +What: /sys/firmware/dmi/tables/
> +Date: April 2015
> +Contact: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
> +Description:
> + The firmware provides DMI structures as a packed list of
> + data referenced by a SMBIOS table entry point. The SMBIOS
> + entry point contains general information, like SMBIOS
> + version, DMI table size, etc. The structure, content and
> + size of SMBIOS entry point is dependent on SMBIOS version.
> + The format of SMBIOS entry point, equal as DMI structures
> + can be read in SMBIOS specification.
> +
> + The dmi/tables provides raw SMBIOS entry point and DMI tables
> + through sysfs as an alternative to utilities reading them
> + from /dev/mem. The raw SMBIOS entry point and DMI table are
> + presented as raw attributes and are accessible via:
> +
> + /sys/firmware/dmi/tables/smbios_entry_point
> + /sys/firmware/dmi/tables/DMI
> +
> + The complete DMI information can be taken using these two
> + tables.
> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
> index e0f1cb3..8e1a411 100644
> --- a/drivers/firmware/dmi-sysfs.c
> +++ b/drivers/firmware/dmi-sysfs.c
> @@ -566,7 +566,6 @@ static struct kobj_type dmi_sysfs_entry_ktype = {
> .default_attrs = dmi_sysfs_entry_attrs,
> };
>
> -static struct kobject *dmi_kobj;
> static struct kset *dmi_kset;
>
> /* Global count of all instances seen. Only for setup */
> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
> int error = -ENOMEM;
> int val;
>
> - /* Set up our directory */
> - dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> - if (!dmi_kobj)
> + if (!dmi_kobj) {
> + pr_err("dmi-sysfs: dmi entry is absent.\n");
> + error = -ENOSYS;
> goto err;
> + }
>
> dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
> if (!dmi_kset)
> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
> err:
> cleanup_entry_list();
> kset_unregister(dmi_kset);
> - kobject_put(dmi_kobj);
> return error;
> }
>
> @@ -685,8 +684,6 @@ static void __exit dmi_sysfs_exit(void)
> pr_debug("dmi-sysfs: unloading.\n");
> cleanup_entry_list();
> kset_unregister(dmi_kset);
> - kobject_del(dmi_kobj);
> - kobject_put(dmi_kobj);
> }
>
> module_init(dmi_sysfs_init);
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index d3aae09..bb19f8b 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -10,6 +10,9 @@
> #include <asm/dmi.h>
> #include <asm/unaligned.h>
>
> +struct kobject *dmi_kobj;
> +EXPORT_SYMBOL_GPL(dmi_kobj);
> +
> /*
> * DMI stands for "Desktop Management Interface". It is part
> * of and an antecedent to, SMBIOS, which stands for System
> @@ -20,6 +23,9 @@ static const char dmi_empty_string[] = " ";
> static u32 dmi_ver __initdata;
> static u32 dmi_len;
> static u16 dmi_num;
> +static u8 smbios_entry_point[32];
> +static int smbios_entry_point_size;
> +
> /*
> * Catch too early calls to dmi_check_system():
> */
> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
> }
>
> static phys_addr_t dmi_base;
> +static u8 *dmi_table;
>
> static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
> void *))
> @@ -476,6 +483,8 @@ static int __init dmi_present(const u8 *buf)
> if (memcmp(buf, "_SM_", 4) == 0 &&
> buf[5] < 32 && dmi_checksum(buf, buf[5])) {
> smbios_ver = get_unaligned_be16(buf + 6);
> + smbios_entry_point_size = buf[5];
> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>
> /* Some BIOS report weird SMBIOS version, fix that up */
> switch (smbios_ver) {
> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
> dmi_ver >> 8, dmi_ver & 0xFF,
> (dmi_ver < 0x0300) ? "" : ".x");
> } else {
> + smbios_entry_point_size = 15;
> + memcpy(smbios_entry_point, buf,
> + smbios_entry_point_size);
> dmi_ver = (buf[14] & 0xF0) << 4 |
> (buf[14] & 0x0F);
> pr_info("Legacy DMI %d.%d present.\n",
> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
> dmi_ver &= 0xFFFFFF;
> dmi_len = get_unaligned_le32(buf + 12);
> dmi_base = get_unaligned_le64(buf + 16);
> + smbios_entry_point_size = buf[6];
> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>
> /*
> * The 64-bit SMBIOS 3.0 entry point no longer has a field
> @@ -638,6 +652,72 @@ void __init dmi_scan_machine(void)
> dmi_initialized = 1;
> }
>
> +static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
> + struct bin_attribute *attr, char *buf,
> + loff_t pos, size_t count)
> +{
> + memcpy(buf, attr->private + pos, count);
> + return count;
> +}
> +
> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
> +struct bin_attribute bin_attr_dmi_table =
> + __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
missed static here
> +
> +static int __init dmi_init(void)
> +{
> + int ret = -ENOMEM;
> + struct kobject *tables_kobj = NULL;
> +
> + if (!dmi_available) {
> + ret = -ENOSYS;
> + goto err;
> + }
> +
> + /* Set up dmi directory at /sys/firmware/dmi */
> + dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> + if (!dmi_kobj)
> + goto err;
> +
> + tables_kobj = kobject_create_and_add("tables", dmi_kobj);
> + if (!tables_kobj)
> + goto err;
> +
> + bin_attr_smbios_entry_point.size = smbios_entry_point_size;
> + bin_attr_smbios_entry_point.private = smbios_entry_point;
> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_smbios_entry_point);
> + if (ret)
> + goto err;
> +
> + dmi_table = dmi_remap(dmi_base, dmi_len);
> + if (!dmi_table)
> + goto err;
> +
> + bin_attr_dmi_table.size = dmi_len;
> + bin_attr_dmi_table.private = dmi_table;
> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
> + if (ret) {
> + dmi_unmap(dmi_table);
> + goto err;
> + }
> +
> + return 0;
> +err:
> + pr_err("dmi: Firmware registration failed.\n");
> +
> + if (tables_kobj)
> + sysfs_remove_bin_file(tables_kobj,
> + &bin_attr_smbios_entry_point);
> + kobject_del(tables_kobj);
> + kobject_put(tables_kobj);
> + kobject_del(dmi_kobj);
> + kobject_put(dmi_kobj);
> + dmi_kobj = NULL;
> +
> + return ret;
> +}
> +subsys_initcall(dmi_init);
> +
> /**
> * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
> *
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index f820f0a..9f55f46 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -93,6 +93,7 @@ struct dmi_dev_onboard {
> int devfn;
> };
>
> +extern struct kobject *dmi_kobj;
> extern int dmi_check_system(const struct dmi_system_id *list);
> const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
> extern const char * dmi_get_system_info(int field);
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-15 4:19 ` Roy Franz
0 siblings, 0 replies; 41+ messages in thread
From: Roy Franz @ 2015-04-15 4:19 UTC (permalink / raw)
To: Ivan.khoronzhuk
Cc: Linux Kernel Mailing List, Matt Fleming, Jean Delvare,
Ard Biesheuvel, Grant Likely, linux-api, linux-doc, mikew,
dmidecode-devel, Leif Lindholm, Mark Salter
On Fri, Apr 3, 2015 at 2:36 AM, Ivan.khoronzhuk
<ivan.khoronzhuk@globallogic.com> wrote:
>
>
> On 02.04.15 15:57, Ivan Khoronzhuk wrote:
>>
>> Some utils, like dmidecode and smbios, need to access SMBIOS entry
>> table area in order to get information like SMBIOS version, size, etc.
>> Currently it's done via /dev/mem. But for situation when /dev/mem
>> usage is disabled, the utils have to use dmi sysfs instead, which
>> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
>> access for table is needed.
>>
>> So this patch creates dmi/tables and adds SMBIOS entry point to allow
>> utils in question to work correctly without /dev/mem. Also patch adds
>> raw dmi table to simplify dmi table processing in user space, as
>> proposed by Jean Delvare.
>>
I have made modifications to dmidecode to support this interface, and it
works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
The only open issue I am aware of is how the SMBIOS v3 entry point
will be handled,
especially in cases where there is a v2 and a v3 entry point.
In principal I think this a good change - are there any other open issues?
Thanks,
Roy
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
>> ---
>> .../ABI/testing/sysfs-firmware-dmi-tables | 22 ++++++
>> drivers/firmware/dmi-sysfs.c | 11 ++-
>> drivers/firmware/dmi_scan.c | 80
>> ++++++++++++++++++++++
>> include/linux/dmi.h | 1 +
>> 4 files changed, 107 insertions(+), 7 deletions(-)
>> create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
>>
>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> new file mode 100644
>> index 0000000..f46158c
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> @@ -0,0 +1,22 @@
>> +What: /sys/firmware/dmi/tables/
>> +Date: April 2015
>> +Contact: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
>> +Description:
>> + The firmware provides DMI structures as a packed list of
>> + data referenced by a SMBIOS table entry point. The SMBIOS
>> + entry point contains general information, like SMBIOS
>> + version, DMI table size, etc. The structure, content and
>> + size of SMBIOS entry point is dependent on SMBIOS version.
>> + The format of SMBIOS entry point, equal as DMI structures
>> + can be read in SMBIOS specification.
>> +
>> + The dmi/tables provides raw SMBIOS entry point and DMI
>> tables
>> + through sysfs as an alternative to utilities reading them
>> + from /dev/mem. The raw SMBIOS entry point and DMI table
>> are
>> + presented as raw attributes and are accessible via:
>> +
>> + /sys/firmware/dmi/tables/smbios_entry_point
>> + /sys/firmware/dmi/tables/DMI
>> +
>> + The complete DMI information can be taken using these two
>> + tables.
>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>> index e0f1cb3..8e1a411 100644
>> --- a/drivers/firmware/dmi-sysfs.c
>> +++ b/drivers/firmware/dmi-sysfs.c
>> @@ -566,7 +566,6 @@ static struct kobj_type dmi_sysfs_entry_ktype = {
>> .default_attrs = dmi_sysfs_entry_attrs,
>> };
>> -static struct kobject *dmi_kobj;
>> static struct kset *dmi_kset;
>> /* Global count of all instances seen. Only for setup */
>> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>> int error = -ENOMEM;
>> int val;
>> - /* Set up our directory */
>> - dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> - if (!dmi_kobj)
>> + if (!dmi_kobj) {
>> + pr_err("dmi-sysfs: dmi entry is absent.\n");
>> + error = -ENOSYS;
>> goto err;
>> + }
>> dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>> if (!dmi_kset)
>> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>> err:
>> cleanup_entry_list();
>> kset_unregister(dmi_kset);
>> - kobject_put(dmi_kobj);
>> return error;
>> }
>> @@ -685,8 +684,6 @@ static void __exit dmi_sysfs_exit(void)
>> pr_debug("dmi-sysfs: unloading.\n");
>> cleanup_entry_list();
>> kset_unregister(dmi_kset);
>> - kobject_del(dmi_kobj);
>> - kobject_put(dmi_kobj);
>> }
>> module_init(dmi_sysfs_init);
>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>> index d3aae09..bb19f8b 100644
>> --- a/drivers/firmware/dmi_scan.c
>> +++ b/drivers/firmware/dmi_scan.c
>> @@ -10,6 +10,9 @@
>> #include <asm/dmi.h>
>> #include <asm/unaligned.h>
>> +struct kobject *dmi_kobj;
>> +EXPORT_SYMBOL_GPL(dmi_kobj);
>> +
>> /*
>> * DMI stands for "Desktop Management Interface". It is part
>> * of and an antecedent to, SMBIOS, which stands for System
>> @@ -20,6 +23,9 @@ static const char dmi_empty_string[] = " ";
>> static u32 dmi_ver __initdata;
>> static u32 dmi_len;
>> static u16 dmi_num;
>> +static u8 smbios_entry_point[32];
>> +static int smbios_entry_point_size;
>> +
>> /*
>> * Catch too early calls to dmi_check_system():
>> */
>> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>> }
>> static phys_addr_t dmi_base;
>> +static u8 *dmi_table;
>> static int __init dmi_walk_early(void (*decode)(const struct
>> dmi_header *,
>> void *))
>> @@ -476,6 +483,8 @@ static int __init dmi_present(const u8 *buf)
>> if (memcmp(buf, "_SM_", 4) == 0 &&
>> buf[5] < 32 && dmi_checksum(buf, buf[5])) {
>> smbios_ver = get_unaligned_be16(buf + 6);
>> + smbios_entry_point_size = buf[5];
>> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>> /* Some BIOS report weird SMBIOS version, fix that up */
>> switch (smbios_ver) {
>> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>> dmi_ver >> 8, dmi_ver & 0xFF,
>> (dmi_ver < 0x0300) ? "" : ".x");
>> } else {
>> + smbios_entry_point_size = 15;
>> + memcpy(smbios_entry_point, buf,
>> + smbios_entry_point_size);
>> dmi_ver = (buf[14] & 0xF0) << 4 |
>> (buf[14] & 0x0F);
>> pr_info("Legacy DMI %d.%d present.\n",
>> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>> dmi_ver &= 0xFFFFFF;
>> dmi_len = get_unaligned_le32(buf + 12);
>> dmi_base = get_unaligned_le64(buf + 16);
>> + smbios_entry_point_size = buf[6];
>> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>> /*
>> * The 64-bit SMBIOS 3.0 entry point no longer has a field
>> @@ -638,6 +652,72 @@ void __init dmi_scan_machine(void)
>> dmi_initialized = 1;
>> }
>> +static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
>> + struct bin_attribute *attr, char *buf,
>> + loff_t pos, size_t count)
>> +{
>> + memcpy(buf, attr->private + pos, count);
>> + return count;
>> +}
>> +
>> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
>> +struct bin_attribute bin_attr_dmi_table =
>> + __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>
>
> missed static here
>
>
>> +
>> +static int __init dmi_init(void)
>> +{
>> + int ret = -ENOMEM;
>> + struct kobject *tables_kobj = NULL;
>> +
>> + if (!dmi_available) {
>> + ret = -ENOSYS;
>> + goto err;
>> + }
>> +
>> + /* Set up dmi directory at /sys/firmware/dmi */
>> + dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> + if (!dmi_kobj)
>> + goto err;
>> +
>> + tables_kobj = kobject_create_and_add("tables", dmi_kobj);
>> + if (!tables_kobj)
>> + goto err;
>> +
>> + bin_attr_smbios_entry_point.size = smbios_entry_point_size;
>> + bin_attr_smbios_entry_point.private = smbios_entry_point;
>> + ret = sysfs_create_bin_file(tables_kobj,
>> &bin_attr_smbios_entry_point);
>> + if (ret)
>> + goto err;
>> +
>> + dmi_table = dmi_remap(dmi_base, dmi_len);
>> + if (!dmi_table)
>> + goto err;
>> +
>> + bin_attr_dmi_table.size = dmi_len;
>> + bin_attr_dmi_table.private = dmi_table;
>> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
>> + if (ret) {
>> + dmi_unmap(dmi_table);
>> + goto err;
>> + }
>> +
>> + return 0;
>> +err:
>> + pr_err("dmi: Firmware registration failed.\n");
>> +
>> + if (tables_kobj)
>> + sysfs_remove_bin_file(tables_kobj,
>> + &bin_attr_smbios_entry_point);
>> + kobject_del(tables_kobj);
>> + kobject_put(tables_kobj);
>> + kobject_del(dmi_kobj);
>> + kobject_put(dmi_kobj);
>> + dmi_kobj = NULL;
>> +
>> + return ret;
>> +}
>> +subsys_initcall(dmi_init);
>> +
>> /**
>> * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
>> *
>> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
>> index f820f0a..9f55f46 100644
>> --- a/include/linux/dmi.h
>> +++ b/include/linux/dmi.h
>> @@ -93,6 +93,7 @@ struct dmi_dev_onboard {
>> int devfn;
>> };
>> +extern struct kobject *dmi_kobj;
>> extern int dmi_check_system(const struct dmi_system_id *list);
>> const struct dmi_system_id *dmi_first_match(const struct dmi_system_id
>> *list);
>> extern const char * dmi_get_system_info(int field);
>
>
> --
> Regards,
> Ivan Khoronzhuk
>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-15 4:19 ` Roy Franz
0 siblings, 0 replies; 41+ messages in thread
From: Roy Franz @ 2015-04-15 4:19 UTC (permalink / raw)
To: Ivan.khoronzhuk
Cc: Linux Kernel Mailing List, Matt Fleming, Jean Delvare,
Ard Biesheuvel, Grant Likely, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, mikew-hpIqsD4AKlfQT0dZR+AlfA,
dmidecode-devel-qX2TKyscuCcdnm+yROfE0A, Leif Lindholm,
Mark Salter
On Fri, Apr 3, 2015 at 2:36 AM, Ivan.khoronzhuk
<ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org> wrote:
>
>
> On 02.04.15 15:57, Ivan Khoronzhuk wrote:
>>
>> Some utils, like dmidecode and smbios, need to access SMBIOS entry
>> table area in order to get information like SMBIOS version, size, etc.
>> Currently it's done via /dev/mem. But for situation when /dev/mem
>> usage is disabled, the utils have to use dmi sysfs instead, which
>> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
>> access for table is needed.
>>
>> So this patch creates dmi/tables and adds SMBIOS entry point to allow
>> utils in question to work correctly without /dev/mem. Also patch adds
>> raw dmi table to simplify dmi table processing in user space, as
>> proposed by Jean Delvare.
>>
I have made modifications to dmidecode to support this interface, and it
works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
The only open issue I am aware of is how the SMBIOS v3 entry point
will be handled,
especially in cases where there is a v2 and a v3 entry point.
In principal I think this a good change - are there any other open issues?
Thanks,
Roy
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
>> ---
>> .../ABI/testing/sysfs-firmware-dmi-tables | 22 ++++++
>> drivers/firmware/dmi-sysfs.c | 11 ++-
>> drivers/firmware/dmi_scan.c | 80
>> ++++++++++++++++++++++
>> include/linux/dmi.h | 1 +
>> 4 files changed, 107 insertions(+), 7 deletions(-)
>> create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
>>
>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> new file mode 100644
>> index 0000000..f46158c
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> @@ -0,0 +1,22 @@
>> +What: /sys/firmware/dmi/tables/
>> +Date: April 2015
>> +Contact: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
>> +Description:
>> + The firmware provides DMI structures as a packed list of
>> + data referenced by a SMBIOS table entry point. The SMBIOS
>> + entry point contains general information, like SMBIOS
>> + version, DMI table size, etc. The structure, content and
>> + size of SMBIOS entry point is dependent on SMBIOS version.
>> + The format of SMBIOS entry point, equal as DMI structures
>> + can be read in SMBIOS specification.
>> +
>> + The dmi/tables provides raw SMBIOS entry point and DMI
>> tables
>> + through sysfs as an alternative to utilities reading them
>> + from /dev/mem. The raw SMBIOS entry point and DMI table
>> are
>> + presented as raw attributes and are accessible via:
>> +
>> + /sys/firmware/dmi/tables/smbios_entry_point
>> + /sys/firmware/dmi/tables/DMI
>> +
>> + The complete DMI information can be taken using these two
>> + tables.
>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>> index e0f1cb3..8e1a411 100644
>> --- a/drivers/firmware/dmi-sysfs.c
>> +++ b/drivers/firmware/dmi-sysfs.c
>> @@ -566,7 +566,6 @@ static struct kobj_type dmi_sysfs_entry_ktype = {
>> .default_attrs = dmi_sysfs_entry_attrs,
>> };
>> -static struct kobject *dmi_kobj;
>> static struct kset *dmi_kset;
>> /* Global count of all instances seen. Only for setup */
>> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>> int error = -ENOMEM;
>> int val;
>> - /* Set up our directory */
>> - dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> - if (!dmi_kobj)
>> + if (!dmi_kobj) {
>> + pr_err("dmi-sysfs: dmi entry is absent.\n");
>> + error = -ENOSYS;
>> goto err;
>> + }
>> dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>> if (!dmi_kset)
>> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>> err:
>> cleanup_entry_list();
>> kset_unregister(dmi_kset);
>> - kobject_put(dmi_kobj);
>> return error;
>> }
>> @@ -685,8 +684,6 @@ static void __exit dmi_sysfs_exit(void)
>> pr_debug("dmi-sysfs: unloading.\n");
>> cleanup_entry_list();
>> kset_unregister(dmi_kset);
>> - kobject_del(dmi_kobj);
>> - kobject_put(dmi_kobj);
>> }
>> module_init(dmi_sysfs_init);
>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>> index d3aae09..bb19f8b 100644
>> --- a/drivers/firmware/dmi_scan.c
>> +++ b/drivers/firmware/dmi_scan.c
>> @@ -10,6 +10,9 @@
>> #include <asm/dmi.h>
>> #include <asm/unaligned.h>
>> +struct kobject *dmi_kobj;
>> +EXPORT_SYMBOL_GPL(dmi_kobj);
>> +
>> /*
>> * DMI stands for "Desktop Management Interface". It is part
>> * of and an antecedent to, SMBIOS, which stands for System
>> @@ -20,6 +23,9 @@ static const char dmi_empty_string[] = " ";
>> static u32 dmi_ver __initdata;
>> static u32 dmi_len;
>> static u16 dmi_num;
>> +static u8 smbios_entry_point[32];
>> +static int smbios_entry_point_size;
>> +
>> /*
>> * Catch too early calls to dmi_check_system():
>> */
>> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>> }
>> static phys_addr_t dmi_base;
>> +static u8 *dmi_table;
>> static int __init dmi_walk_early(void (*decode)(const struct
>> dmi_header *,
>> void *))
>> @@ -476,6 +483,8 @@ static int __init dmi_present(const u8 *buf)
>> if (memcmp(buf, "_SM_", 4) == 0 &&
>> buf[5] < 32 && dmi_checksum(buf, buf[5])) {
>> smbios_ver = get_unaligned_be16(buf + 6);
>> + smbios_entry_point_size = buf[5];
>> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>> /* Some BIOS report weird SMBIOS version, fix that up */
>> switch (smbios_ver) {
>> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>> dmi_ver >> 8, dmi_ver & 0xFF,
>> (dmi_ver < 0x0300) ? "" : ".x");
>> } else {
>> + smbios_entry_point_size = 15;
>> + memcpy(smbios_entry_point, buf,
>> + smbios_entry_point_size);
>> dmi_ver = (buf[14] & 0xF0) << 4 |
>> (buf[14] & 0x0F);
>> pr_info("Legacy DMI %d.%d present.\n",
>> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>> dmi_ver &= 0xFFFFFF;
>> dmi_len = get_unaligned_le32(buf + 12);
>> dmi_base = get_unaligned_le64(buf + 16);
>> + smbios_entry_point_size = buf[6];
>> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>> /*
>> * The 64-bit SMBIOS 3.0 entry point no longer has a field
>> @@ -638,6 +652,72 @@ void __init dmi_scan_machine(void)
>> dmi_initialized = 1;
>> }
>> +static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
>> + struct bin_attribute *attr, char *buf,
>> + loff_t pos, size_t count)
>> +{
>> + memcpy(buf, attr->private + pos, count);
>> + return count;
>> +}
>> +
>> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
>> +struct bin_attribute bin_attr_dmi_table =
>> + __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>
>
> missed static here
>
>
>> +
>> +static int __init dmi_init(void)
>> +{
>> + int ret = -ENOMEM;
>> + struct kobject *tables_kobj = NULL;
>> +
>> + if (!dmi_available) {
>> + ret = -ENOSYS;
>> + goto err;
>> + }
>> +
>> + /* Set up dmi directory at /sys/firmware/dmi */
>> + dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> + if (!dmi_kobj)
>> + goto err;
>> +
>> + tables_kobj = kobject_create_and_add("tables", dmi_kobj);
>> + if (!tables_kobj)
>> + goto err;
>> +
>> + bin_attr_smbios_entry_point.size = smbios_entry_point_size;
>> + bin_attr_smbios_entry_point.private = smbios_entry_point;
>> + ret = sysfs_create_bin_file(tables_kobj,
>> &bin_attr_smbios_entry_point);
>> + if (ret)
>> + goto err;
>> +
>> + dmi_table = dmi_remap(dmi_base, dmi_len);
>> + if (!dmi_table)
>> + goto err;
>> +
>> + bin_attr_dmi_table.size = dmi_len;
>> + bin_attr_dmi_table.private = dmi_table;
>> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
>> + if (ret) {
>> + dmi_unmap(dmi_table);
>> + goto err;
>> + }
>> +
>> + return 0;
>> +err:
>> + pr_err("dmi: Firmware registration failed.\n");
>> +
>> + if (tables_kobj)
>> + sysfs_remove_bin_file(tables_kobj,
>> + &bin_attr_smbios_entry_point);
>> + kobject_del(tables_kobj);
>> + kobject_put(tables_kobj);
>> + kobject_del(dmi_kobj);
>> + kobject_put(dmi_kobj);
>> + dmi_kobj = NULL;
>> +
>> + return ret;
>> +}
>> +subsys_initcall(dmi_init);
>> +
>> /**
>> * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
>> *
>> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
>> index f820f0a..9f55f46 100644
>> --- a/include/linux/dmi.h
>> +++ b/include/linux/dmi.h
>> @@ -93,6 +93,7 @@ struct dmi_dev_onboard {
>> int devfn;
>> };
>> +extern struct kobject *dmi_kobj;
>> extern int dmi_check_system(const struct dmi_system_id *list);
>> const struct dmi_system_id *dmi_first_match(const struct dmi_system_id
>> *list);
>> extern const char * dmi_get_system_info(int field);
>
>
> --
> Regards,
> Ivan Khoronzhuk
>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-16 0:54 ` Roy Franz
0 siblings, 0 replies; 41+ messages in thread
From: Roy Franz @ 2015-04-16 0:54 UTC (permalink / raw)
To: Ivan.khoronzhuk
Cc: Linux Kernel Mailing List, Matt Fleming, Jean Delvare,
Ard Biesheuvel, Grant Likely, linux-api, linux-doc,
Mike Waychison, dmidecode-devel, Leif Lindholm, Mark Salter
On Tue, Apr 14, 2015 at 9:19 PM, Roy Franz <roy.franz@linaro.org> wrote:
> On Fri, Apr 3, 2015 at 2:36 AM, Ivan.khoronzhuk
> <ivan.khoronzhuk@globallogic.com> wrote:
>>
>>
>> On 02.04.15 15:57, Ivan Khoronzhuk wrote:
>>>
>>> Some utils, like dmidecode and smbios, need to access SMBIOS entry
>>> table area in order to get information like SMBIOS version, size, etc.
>>> Currently it's done via /dev/mem. But for situation when /dev/mem
>>> usage is disabled, the utils have to use dmi sysfs instead, which
>>> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
>>> access for table is needed.
>>>
>>> So this patch creates dmi/tables and adds SMBIOS entry point to allow
>>> utils in question to work correctly without /dev/mem. Also patch adds
>>> raw dmi table to simplify dmi table processing in user space, as
>>> proposed by Jean Delvare.
>>>
>
> I have made modifications to dmidecode to support this interface, and it
> works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
> The only open issue I am aware of is how the SMBIOS v3 entry point
> will be handled,
> especially in cases where there is a v2 and a v3 entry point.
> In principal I think this a good change - are there any other open issues?
>
> Thanks,
> Roy
>
I looked at the SMBIOS spec again, and the platform can provide either or
both of the 32-bit and 64-bit entry points. The spec says that an
implementation
"should" provide a 32-bit entry point for compatibility.
These 2 entry point structures can both point to the same SMBIOS
structure table,
or two distinct ones. If distinct, the one referenced by the 32-bit
entry point must be
a consistent subset of the 64-bit one.
There does not seem to be any prohibition from using new SMBIOS v3 structures
in a table referenced by a 32-bit entry point, so backwards
compatibility is completely
up to the implementation.
Since the point of this patchset (and related changes to dmidecode) is
to provide the
SMBIOS information without using /dev/mem, I think the interface we
define should
support all the cases outlined in the specification. I would like to
avoid a case where
we're back to using /dev/mem to deal with the dual entry/dual table
case if that becomes
important down the road.
Here's a proposal for files in /sys/firmware/dmi/tables:
smbios_entry_point32 - 32 bit (existing entry point type), if it exists.
smbios_entry_point64 - 64 bit entry, new in SMBIOS v3.0
DMI32 - smbios structure tables referenced by
32 bit entry point, if it exists
DMI64 - smbios structure tables referenced by
64 bit entry point, if it exists.
symlink to DMI32 if identical
smbios_entry_point - symlink to smbios_entry_point64 if it exists,
otherwise symlink to smbios_entry_point64
DMI - symlink to DMI64 if it exists,
otherwise symlink to DMI32
These last two would provide names to the 'preferred' tables, and
names that would always exist on all systems, which
I think is a nice property to have.
Thoughts?
Thanks,
Roy
>
>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
>>> ---
>>> .../ABI/testing/sysfs-firmware-dmi-tables | 22 ++++++
>>> drivers/firmware/dmi-sysfs.c | 11 ++-
>>> drivers/firmware/dmi_scan.c | 80
>>> ++++++++++++++++++++++
>>> include/linux/dmi.h | 1 +
>>> 4 files changed, 107 insertions(+), 7 deletions(-)
>>> create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>>> b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>>> new file mode 100644
>>> index 0000000..f46158c
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>>> @@ -0,0 +1,22 @@
>>> +What: /sys/firmware/dmi/tables/
>>> +Date: April 2015
>>> +Contact: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
>>> +Description:
>>> + The firmware provides DMI structures as a packed list of
>>> + data referenced by a SMBIOS table entry point. The SMBIOS
>>> + entry point contains general information, like SMBIOS
>>> + version, DMI table size, etc. The structure, content and
>>> + size of SMBIOS entry point is dependent on SMBIOS version.
>>> + The format of SMBIOS entry point, equal as DMI structures
>>> + can be read in SMBIOS specification.
>>> +
>>> + The dmi/tables provides raw SMBIOS entry point and DMI
>>> tables
>>> + through sysfs as an alternative to utilities reading them
>>> + from /dev/mem. The raw SMBIOS entry point and DMI table
>>> are
>>> + presented as raw attributes and are accessible via:
>>> +
>>> + /sys/firmware/dmi/tables/smbios_entry_point
>>> + /sys/firmware/dmi/tables/DMI
>>> +
>>> + The complete DMI information can be taken using these two
>>> + tables.
>>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>>> index e0f1cb3..8e1a411 100644
>>> --- a/drivers/firmware/dmi-sysfs.c
>>> +++ b/drivers/firmware/dmi-sysfs.c
>>> @@ -566,7 +566,6 @@ static struct kobj_type dmi_sysfs_entry_ktype = {
>>> .default_attrs = dmi_sysfs_entry_attrs,
>>> };
>>> -static struct kobject *dmi_kobj;
>>> static struct kset *dmi_kset;
>>> /* Global count of all instances seen. Only for setup */
>>> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>>> int error = -ENOMEM;
>>> int val;
>>> - /* Set up our directory */
>>> - dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>>> - if (!dmi_kobj)
>>> + if (!dmi_kobj) {
>>> + pr_err("dmi-sysfs: dmi entry is absent.\n");
>>> + error = -ENOSYS;
>>> goto err;
>>> + }
>>> dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>>> if (!dmi_kset)
>>> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>>> err:
>>> cleanup_entry_list();
>>> kset_unregister(dmi_kset);
>>> - kobject_put(dmi_kobj);
>>> return error;
>>> }
>>> @@ -685,8 +684,6 @@ static void __exit dmi_sysfs_exit(void)
>>> pr_debug("dmi-sysfs: unloading.\n");
>>> cleanup_entry_list();
>>> kset_unregister(dmi_kset);
>>> - kobject_del(dmi_kobj);
>>> - kobject_put(dmi_kobj);
>>> }
>>> module_init(dmi_sysfs_init);
>>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>>> index d3aae09..bb19f8b 100644
>>> --- a/drivers/firmware/dmi_scan.c
>>> +++ b/drivers/firmware/dmi_scan.c
>>> @@ -10,6 +10,9 @@
>>> #include <asm/dmi.h>
>>> #include <asm/unaligned.h>
>>> +struct kobject *dmi_kobj;
>>> +EXPORT_SYMBOL_GPL(dmi_kobj);
>>> +
>>> /*
>>> * DMI stands for "Desktop Management Interface". It is part
>>> * of and an antecedent to, SMBIOS, which stands for System
>>> @@ -20,6 +23,9 @@ static const char dmi_empty_string[] = " ";
>>> static u32 dmi_ver __initdata;
>>> static u32 dmi_len;
>>> static u16 dmi_num;
>>> +static u8 smbios_entry_point[32];
>>> +static int smbios_entry_point_size;
>>> +
>>> /*
>>> * Catch too early calls to dmi_check_system():
>>> */
>>> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>>> }
>>> static phys_addr_t dmi_base;
>>> +static u8 *dmi_table;
>>> static int __init dmi_walk_early(void (*decode)(const struct
>>> dmi_header *,
>>> void *))
>>> @@ -476,6 +483,8 @@ static int __init dmi_present(const u8 *buf)
>>> if (memcmp(buf, "_SM_", 4) == 0 &&
>>> buf[5] < 32 && dmi_checksum(buf, buf[5])) {
>>> smbios_ver = get_unaligned_be16(buf + 6);
>>> + smbios_entry_point_size = buf[5];
>>> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>>> /* Some BIOS report weird SMBIOS version, fix that up */
>>> switch (smbios_ver) {
>>> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>>> dmi_ver >> 8, dmi_ver & 0xFF,
>>> (dmi_ver < 0x0300) ? "" : ".x");
>>> } else {
>>> + smbios_entry_point_size = 15;
>>> + memcpy(smbios_entry_point, buf,
>>> + smbios_entry_point_size);
>>> dmi_ver = (buf[14] & 0xF0) << 4 |
>>> (buf[14] & 0x0F);
>>> pr_info("Legacy DMI %d.%d present.\n",
>>> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>>> dmi_ver &= 0xFFFFFF;
>>> dmi_len = get_unaligned_le32(buf + 12);
>>> dmi_base = get_unaligned_le64(buf + 16);
>>> + smbios_entry_point_size = buf[6];
>>> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>>> /*
>>> * The 64-bit SMBIOS 3.0 entry point no longer has a field
>>> @@ -638,6 +652,72 @@ void __init dmi_scan_machine(void)
>>> dmi_initialized = 1;
>>> }
>>> +static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
>>> + struct bin_attribute *attr, char *buf,
>>> + loff_t pos, size_t count)
>>> +{
>>> + memcpy(buf, attr->private + pos, count);
>>> + return count;
>>> +}
>>> +
>>> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
>>> +struct bin_attribute bin_attr_dmi_table =
>>> + __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>>
>>
>> missed static here
>>
>>
>>> +
>>> +static int __init dmi_init(void)
>>> +{
>>> + int ret = -ENOMEM;
>>> + struct kobject *tables_kobj = NULL;
>>> +
>>> + if (!dmi_available) {
>>> + ret = -ENOSYS;
>>> + goto err;
>>> + }
>>> +
>>> + /* Set up dmi directory at /sys/firmware/dmi */
>>> + dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>>> + if (!dmi_kobj)
>>> + goto err;
>>> +
>>> + tables_kobj = kobject_create_and_add("tables", dmi_kobj);
>>> + if (!tables_kobj)
>>> + goto err;
>>> +
>>> + bin_attr_smbios_entry_point.size = smbios_entry_point_size;
>>> + bin_attr_smbios_entry_point.private = smbios_entry_point;
>>> + ret = sysfs_create_bin_file(tables_kobj,
>>> &bin_attr_smbios_entry_point);
>>> + if (ret)
>>> + goto err;
>>> +
>>> + dmi_table = dmi_remap(dmi_base, dmi_len);
>>> + if (!dmi_table)
>>> + goto err;
>>> +
>>> + bin_attr_dmi_table.size = dmi_len;
>>> + bin_attr_dmi_table.private = dmi_table;
>>> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
>>> + if (ret) {
>>> + dmi_unmap(dmi_table);
>>> + goto err;
>>> + }
>>> +
>>> + return 0;
>>> +err:
>>> + pr_err("dmi: Firmware registration failed.\n");
>>> +
>>> + if (tables_kobj)
>>> + sysfs_remove_bin_file(tables_kobj,
>>> + &bin_attr_smbios_entry_point);
>>> + kobject_del(tables_kobj);
>>> + kobject_put(tables_kobj);
>>> + kobject_del(dmi_kobj);
>>> + kobject_put(dmi_kobj);
>>> + dmi_kobj = NULL;
>>> +
>>> + return ret;
>>> +}
>>> +subsys_initcall(dmi_init);
>>> +
>>> /**
>>> * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
>>> *
>>> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
>>> index f820f0a..9f55f46 100644
>>> --- a/include/linux/dmi.h
>>> +++ b/include/linux/dmi.h
>>> @@ -93,6 +93,7 @@ struct dmi_dev_onboard {
>>> int devfn;
>>> };
>>> +extern struct kobject *dmi_kobj;
>>> extern int dmi_check_system(const struct dmi_system_id *list);
>>> const struct dmi_system_id *dmi_first_match(const struct dmi_system_id
>>> *list);
>>> extern const char * dmi_get_system_info(int field);
>>
>>
>> --
>> Regards,
>> Ivan Khoronzhuk
>>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-16 0:54 ` Roy Franz
0 siblings, 0 replies; 41+ messages in thread
From: Roy Franz @ 2015-04-16 0:54 UTC (permalink / raw)
To: Ivan.khoronzhuk
Cc: Linux Kernel Mailing List, Matt Fleming, Jean Delvare,
Ard Biesheuvel, Grant Likely, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, Mike Waychison,
dmidecode-devel-qX2TKyscuCcdnm+yROfE0A, Leif Lindholm,
Mark Salter
On Tue, Apr 14, 2015 at 9:19 PM, Roy Franz <roy.franz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Apr 3, 2015 at 2:36 AM, Ivan.khoronzhuk
> <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org> wrote:
>>
>>
>> On 02.04.15 15:57, Ivan Khoronzhuk wrote:
>>>
>>> Some utils, like dmidecode and smbios, need to access SMBIOS entry
>>> table area in order to get information like SMBIOS version, size, etc.
>>> Currently it's done via /dev/mem. But for situation when /dev/mem
>>> usage is disabled, the utils have to use dmi sysfs instead, which
>>> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
>>> access for table is needed.
>>>
>>> So this patch creates dmi/tables and adds SMBIOS entry point to allow
>>> utils in question to work correctly without /dev/mem. Also patch adds
>>> raw dmi table to simplify dmi table processing in user space, as
>>> proposed by Jean Delvare.
>>>
>
> I have made modifications to dmidecode to support this interface, and it
> works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
> The only open issue I am aware of is how the SMBIOS v3 entry point
> will be handled,
> especially in cases where there is a v2 and a v3 entry point.
> In principal I think this a good change - are there any other open issues?
>
> Thanks,
> Roy
>
I looked at the SMBIOS spec again, and the platform can provide either or
both of the 32-bit and 64-bit entry points. The spec says that an
implementation
"should" provide a 32-bit entry point for compatibility.
These 2 entry point structures can both point to the same SMBIOS
structure table,
or two distinct ones. If distinct, the one referenced by the 32-bit
entry point must be
a consistent subset of the 64-bit one.
There does not seem to be any prohibition from using new SMBIOS v3 structures
in a table referenced by a 32-bit entry point, so backwards
compatibility is completely
up to the implementation.
Since the point of this patchset (and related changes to dmidecode) is
to provide the
SMBIOS information without using /dev/mem, I think the interface we
define should
support all the cases outlined in the specification. I would like to
avoid a case where
we're back to using /dev/mem to deal with the dual entry/dual table
case if that becomes
important down the road.
Here's a proposal for files in /sys/firmware/dmi/tables:
smbios_entry_point32 - 32 bit (existing entry point type), if it exists.
smbios_entry_point64 - 64 bit entry, new in SMBIOS v3.0
DMI32 - smbios structure tables referenced by
32 bit entry point, if it exists
DMI64 - smbios structure tables referenced by
64 bit entry point, if it exists.
symlink to DMI32 if identical
smbios_entry_point - symlink to smbios_entry_point64 if it exists,
otherwise symlink to smbios_entry_point64
DMI - symlink to DMI64 if it exists,
otherwise symlink to DMI32
These last two would provide names to the 'preferred' tables, and
names that would always exist on all systems, which
I think is a nice property to have.
Thoughts?
Thanks,
Roy
>
>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
>>> ---
>>> .../ABI/testing/sysfs-firmware-dmi-tables | 22 ++++++
>>> drivers/firmware/dmi-sysfs.c | 11 ++-
>>> drivers/firmware/dmi_scan.c | 80
>>> ++++++++++++++++++++++
>>> include/linux/dmi.h | 1 +
>>> 4 files changed, 107 insertions(+), 7 deletions(-)
>>> create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>>> b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>>> new file mode 100644
>>> index 0000000..f46158c
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>>> @@ -0,0 +1,22 @@
>>> +What: /sys/firmware/dmi/tables/
>>> +Date: April 2015
>>> +Contact: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
>>> +Description:
>>> + The firmware provides DMI structures as a packed list of
>>> + data referenced by a SMBIOS table entry point. The SMBIOS
>>> + entry point contains general information, like SMBIOS
>>> + version, DMI table size, etc. The structure, content and
>>> + size of SMBIOS entry point is dependent on SMBIOS version.
>>> + The format of SMBIOS entry point, equal as DMI structures
>>> + can be read in SMBIOS specification.
>>> +
>>> + The dmi/tables provides raw SMBIOS entry point and DMI
>>> tables
>>> + through sysfs as an alternative to utilities reading them
>>> + from /dev/mem. The raw SMBIOS entry point and DMI table
>>> are
>>> + presented as raw attributes and are accessible via:
>>> +
>>> + /sys/firmware/dmi/tables/smbios_entry_point
>>> + /sys/firmware/dmi/tables/DMI
>>> +
>>> + The complete DMI information can be taken using these two
>>> + tables.
>>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>>> index e0f1cb3..8e1a411 100644
>>> --- a/drivers/firmware/dmi-sysfs.c
>>> +++ b/drivers/firmware/dmi-sysfs.c
>>> @@ -566,7 +566,6 @@ static struct kobj_type dmi_sysfs_entry_ktype = {
>>> .default_attrs = dmi_sysfs_entry_attrs,
>>> };
>>> -static struct kobject *dmi_kobj;
>>> static struct kset *dmi_kset;
>>> /* Global count of all instances seen. Only for setup */
>>> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>>> int error = -ENOMEM;
>>> int val;
>>> - /* Set up our directory */
>>> - dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>>> - if (!dmi_kobj)
>>> + if (!dmi_kobj) {
>>> + pr_err("dmi-sysfs: dmi entry is absent.\n");
>>> + error = -ENOSYS;
>>> goto err;
>>> + }
>>> dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>>> if (!dmi_kset)
>>> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>>> err:
>>> cleanup_entry_list();
>>> kset_unregister(dmi_kset);
>>> - kobject_put(dmi_kobj);
>>> return error;
>>> }
>>> @@ -685,8 +684,6 @@ static void __exit dmi_sysfs_exit(void)
>>> pr_debug("dmi-sysfs: unloading.\n");
>>> cleanup_entry_list();
>>> kset_unregister(dmi_kset);
>>> - kobject_del(dmi_kobj);
>>> - kobject_put(dmi_kobj);
>>> }
>>> module_init(dmi_sysfs_init);
>>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>>> index d3aae09..bb19f8b 100644
>>> --- a/drivers/firmware/dmi_scan.c
>>> +++ b/drivers/firmware/dmi_scan.c
>>> @@ -10,6 +10,9 @@
>>> #include <asm/dmi.h>
>>> #include <asm/unaligned.h>
>>> +struct kobject *dmi_kobj;
>>> +EXPORT_SYMBOL_GPL(dmi_kobj);
>>> +
>>> /*
>>> * DMI stands for "Desktop Management Interface". It is part
>>> * of and an antecedent to, SMBIOS, which stands for System
>>> @@ -20,6 +23,9 @@ static const char dmi_empty_string[] = " ";
>>> static u32 dmi_ver __initdata;
>>> static u32 dmi_len;
>>> static u16 dmi_num;
>>> +static u8 smbios_entry_point[32];
>>> +static int smbios_entry_point_size;
>>> +
>>> /*
>>> * Catch too early calls to dmi_check_system():
>>> */
>>> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>>> }
>>> static phys_addr_t dmi_base;
>>> +static u8 *dmi_table;
>>> static int __init dmi_walk_early(void (*decode)(const struct
>>> dmi_header *,
>>> void *))
>>> @@ -476,6 +483,8 @@ static int __init dmi_present(const u8 *buf)
>>> if (memcmp(buf, "_SM_", 4) == 0 &&
>>> buf[5] < 32 && dmi_checksum(buf, buf[5])) {
>>> smbios_ver = get_unaligned_be16(buf + 6);
>>> + smbios_entry_point_size = buf[5];
>>> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>>> /* Some BIOS report weird SMBIOS version, fix that up */
>>> switch (smbios_ver) {
>>> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>>> dmi_ver >> 8, dmi_ver & 0xFF,
>>> (dmi_ver < 0x0300) ? "" : ".x");
>>> } else {
>>> + smbios_entry_point_size = 15;
>>> + memcpy(smbios_entry_point, buf,
>>> + smbios_entry_point_size);
>>> dmi_ver = (buf[14] & 0xF0) << 4 |
>>> (buf[14] & 0x0F);
>>> pr_info("Legacy DMI %d.%d present.\n",
>>> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>>> dmi_ver &= 0xFFFFFF;
>>> dmi_len = get_unaligned_le32(buf + 12);
>>> dmi_base = get_unaligned_le64(buf + 16);
>>> + smbios_entry_point_size = buf[6];
>>> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>>> /*
>>> * The 64-bit SMBIOS 3.0 entry point no longer has a field
>>> @@ -638,6 +652,72 @@ void __init dmi_scan_machine(void)
>>> dmi_initialized = 1;
>>> }
>>> +static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
>>> + struct bin_attribute *attr, char *buf,
>>> + loff_t pos, size_t count)
>>> +{
>>> + memcpy(buf, attr->private + pos, count);
>>> + return count;
>>> +}
>>> +
>>> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
>>> +struct bin_attribute bin_attr_dmi_table =
>>> + __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>>
>>
>> missed static here
>>
>>
>>> +
>>> +static int __init dmi_init(void)
>>> +{
>>> + int ret = -ENOMEM;
>>> + struct kobject *tables_kobj = NULL;
>>> +
>>> + if (!dmi_available) {
>>> + ret = -ENOSYS;
>>> + goto err;
>>> + }
>>> +
>>> + /* Set up dmi directory at /sys/firmware/dmi */
>>> + dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>>> + if (!dmi_kobj)
>>> + goto err;
>>> +
>>> + tables_kobj = kobject_create_and_add("tables", dmi_kobj);
>>> + if (!tables_kobj)
>>> + goto err;
>>> +
>>> + bin_attr_smbios_entry_point.size = smbios_entry_point_size;
>>> + bin_attr_smbios_entry_point.private = smbios_entry_point;
>>> + ret = sysfs_create_bin_file(tables_kobj,
>>> &bin_attr_smbios_entry_point);
>>> + if (ret)
>>> + goto err;
>>> +
>>> + dmi_table = dmi_remap(dmi_base, dmi_len);
>>> + if (!dmi_table)
>>> + goto err;
>>> +
>>> + bin_attr_dmi_table.size = dmi_len;
>>> + bin_attr_dmi_table.private = dmi_table;
>>> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
>>> + if (ret) {
>>> + dmi_unmap(dmi_table);
>>> + goto err;
>>> + }
>>> +
>>> + return 0;
>>> +err:
>>> + pr_err("dmi: Firmware registration failed.\n");
>>> +
>>> + if (tables_kobj)
>>> + sysfs_remove_bin_file(tables_kobj,
>>> + &bin_attr_smbios_entry_point);
>>> + kobject_del(tables_kobj);
>>> + kobject_put(tables_kobj);
>>> + kobject_del(dmi_kobj);
>>> + kobject_put(dmi_kobj);
>>> + dmi_kobj = NULL;
>>> +
>>> + return ret;
>>> +}
>>> +subsys_initcall(dmi_init);
>>> +
>>> /**
>>> * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
>>> *
>>> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
>>> index f820f0a..9f55f46 100644
>>> --- a/include/linux/dmi.h
>>> +++ b/include/linux/dmi.h
>>> @@ -93,6 +93,7 @@ struct dmi_dev_onboard {
>>> int devfn;
>>> };
>>> +extern struct kobject *dmi_kobj;
>>> extern int dmi_check_system(const struct dmi_system_id *list);
>>> const struct dmi_system_id *dmi_first_match(const struct dmi_system_id
>>> *list);
>>> extern const char * dmi_get_system_info(int field);
>>
>>
>> --
>> Regards,
>> Ivan Khoronzhuk
>>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-16 6:48 ` Jean Delvare
0 siblings, 0 replies; 41+ messages in thread
From: Jean Delvare @ 2015-04-16 6:48 UTC (permalink / raw)
To: Roy Franz
Cc: Ivan.khoronzhuk, Linux Kernel Mailing List, Matt Fleming,
Ard Biesheuvel, Grant Likely, linux-api, linux-doc,
Mike Waychison, dmidecode-devel, Leif Lindholm, Mark Salter
Hi Roy,
On Wed, 15 Apr 2015 17:54:51 -0700, Roy Franz wrote:
> On Tue, Apr 14, 2015 at 9:19 PM, Roy Franz <roy.franz@linaro.org> wrote:
> > I have made modifications to dmidecode to support this interface, and it
> > works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
> > The only open issue I am aware of is how the SMBIOS v3 entry point
> > will be handled,
> > especially in cases where there is a v2 and a v3 entry point.
> > In principal I think this a good change - are there any other open issues?
>
> I looked at the SMBIOS spec again, and the platform can provide either or
> both of the 32-bit and 64-bit entry points. The spec says that an
> implementation
> "should" provide a 32-bit entry point for compatibility.
>
> These 2 entry point structures can both point to the same SMBIOS
> structure table,
> or two distinct ones. If distinct, the one referenced by the 32-bit
> entry point must be
> a consistent subset of the 64-bit one.
> There does not seem to be any prohibition from using new SMBIOS v3 structures
> in a table referenced by a 32-bit entry point, so backwards
> compatibility is completely
> up to the implementation.
What do you mean with "new SMBIOS v3 structures"? I took a brief look
at the change log at the end of the SMBIOS 3.0.0 specification, and all
I can see are new enumerated values for existing fields, as well as 3
new fields in the type 4 structure. I can't see any new structure type.
Either way this is all backwards compatible, which is why both entry
points can point to the same table.
> Since the point of this patchset (and related changes to dmidecode) is
> to provide the
> SMBIOS information without using /dev/mem, I think the interface we
> define should
> support all the cases outlined in the specification. I would like to
> avoid a case where
> we're back to using /dev/mem to deal with the dual entry/dual table
> case if that becomes
> important down the road.
>
> Here's a proposal for files in /sys/firmware/dmi/tables:
>
> smbios_entry_point32 - 32 bit (existing entry point type), if it exists.
> smbios_entry_point64 - 64 bit entry, new in SMBIOS v3.0
> DMI32 - smbios structure tables referenced by
> 32 bit entry point, if it exists
> DMI64 - smbios structure tables referenced by
> 64 bit entry point, if it exists.
> symlink to DMI32 if identical
> smbios_entry_point - symlink to smbios_entry_point64 if it exists,
> otherwise symlink to smbios_entry_point64
> DMI - symlink to DMI64 if it exists,
> otherwise symlink to DMI32
>
> These last two would provide names to the 'preferred' tables, and
> names that would always exist on all systems, which
> I think is a nice property to have.
>
> Thoughts?
The idea was discussed before, and discarded. The question is, what use
cases do you envision from a user-space application perspective? As you
found out, the table pointed to by the _SM3_ entry point must be a
super-set of the one pointed to by the _SM_ entry point, so for a
compliant implementation there is no reason to follow the _SM_ entry
point when both exist. And if only one entry point exists, there is
nothing to choose from.
The kernel itself will have to choose one of the entry points when it
comes to dmi_scan and dmi-id. It can't provide two sets of DMI strings.
So it seems reasonable to only expose though sysfs that one entry point
and table that the kernel itself used.
The only case where it would make some sense to expose everything to
user-space is for BIOS debugging purpose: if both entry points exist
and point to different tables, and if the _SM3_ table is broken and
the _SM_ table is correct, then you may want to be able to temporarily
read the _SM_ table instead of the _SM3_ table, to check what needs to
be fixed in the latter. But I would argue that this is beyond the scope
of our code: neither the kernel nor dmidecode are DMI table or BIOS
authoring tools.
If there really ever is a need to ignore the _SM3_ entry point, I'd
rather have a kernel boot parameter for this. This would guarantee that
the kernel and user-space applications always operate on the same data.
So I would keep the sysfs interface simple, as Ivan implemented it.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-16 6:48 ` Jean Delvare
0 siblings, 0 replies; 41+ messages in thread
From: Jean Delvare @ 2015-04-16 6:48 UTC (permalink / raw)
To: Roy Franz
Cc: Ivan.khoronzhuk, Linux Kernel Mailing List, Matt Fleming,
Ard Biesheuvel, Grant Likely, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, Mike Waychison,
dmidecode-devel-qX2TKyscuCcdnm+yROfE0A, Leif Lindholm,
Mark Salter
Hi Roy,
On Wed, 15 Apr 2015 17:54:51 -0700, Roy Franz wrote:
> On Tue, Apr 14, 2015 at 9:19 PM, Roy Franz <roy.franz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > I have made modifications to dmidecode to support this interface, and it
> > works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
> > The only open issue I am aware of is how the SMBIOS v3 entry point
> > will be handled,
> > especially in cases where there is a v2 and a v3 entry point.
> > In principal I think this a good change - are there any other open issues?
>
> I looked at the SMBIOS spec again, and the platform can provide either or
> both of the 32-bit and 64-bit entry points. The spec says that an
> implementation
> "should" provide a 32-bit entry point for compatibility.
>
> These 2 entry point structures can both point to the same SMBIOS
> structure table,
> or two distinct ones. If distinct, the one referenced by the 32-bit
> entry point must be
> a consistent subset of the 64-bit one.
> There does not seem to be any prohibition from using new SMBIOS v3 structures
> in a table referenced by a 32-bit entry point, so backwards
> compatibility is completely
> up to the implementation.
What do you mean with "new SMBIOS v3 structures"? I took a brief look
at the change log at the end of the SMBIOS 3.0.0 specification, and all
I can see are new enumerated values for existing fields, as well as 3
new fields in the type 4 structure. I can't see any new structure type.
Either way this is all backwards compatible, which is why both entry
points can point to the same table.
> Since the point of this patchset (and related changes to dmidecode) is
> to provide the
> SMBIOS information without using /dev/mem, I think the interface we
> define should
> support all the cases outlined in the specification. I would like to
> avoid a case where
> we're back to using /dev/mem to deal with the dual entry/dual table
> case if that becomes
> important down the road.
>
> Here's a proposal for files in /sys/firmware/dmi/tables:
>
> smbios_entry_point32 - 32 bit (existing entry point type), if it exists.
> smbios_entry_point64 - 64 bit entry, new in SMBIOS v3.0
> DMI32 - smbios structure tables referenced by
> 32 bit entry point, if it exists
> DMI64 - smbios structure tables referenced by
> 64 bit entry point, if it exists.
> symlink to DMI32 if identical
> smbios_entry_point - symlink to smbios_entry_point64 if it exists,
> otherwise symlink to smbios_entry_point64
> DMI - symlink to DMI64 if it exists,
> otherwise symlink to DMI32
>
> These last two would provide names to the 'preferred' tables, and
> names that would always exist on all systems, which
> I think is a nice property to have.
>
> Thoughts?
The idea was discussed before, and discarded. The question is, what use
cases do you envision from a user-space application perspective? As you
found out, the table pointed to by the _SM3_ entry point must be a
super-set of the one pointed to by the _SM_ entry point, so for a
compliant implementation there is no reason to follow the _SM_ entry
point when both exist. And if only one entry point exists, there is
nothing to choose from.
The kernel itself will have to choose one of the entry points when it
comes to dmi_scan and dmi-id. It can't provide two sets of DMI strings.
So it seems reasonable to only expose though sysfs that one entry point
and table that the kernel itself used.
The only case where it would make some sense to expose everything to
user-space is for BIOS debugging purpose: if both entry points exist
and point to different tables, and if the _SM3_ table is broken and
the _SM_ table is correct, then you may want to be able to temporarily
read the _SM_ table instead of the _SM3_ table, to check what needs to
be fixed in the latter. But I would argue that this is beyond the scope
of our code: neither the kernel nor dmidecode are DMI table or BIOS
authoring tools.
If there really ever is a need to ignore the _SM3_ entry point, I'd
rather have a kernel boot parameter for this. This would guarantee that
the kernel and user-space applications always operate on the same data.
So I would keep the sysfs interface simple, as Ivan implemented it.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
2015-04-16 6:48 ` Jean Delvare
(?)
@ 2015-04-16 17:08 ` Roy Franz
-1 siblings, 0 replies; 41+ messages in thread
From: Roy Franz @ 2015-04-16 17:08 UTC (permalink / raw)
To: Jean Delvare
Cc: Ivan.khoronzhuk, Linux Kernel Mailing List, Matt Fleming,
Ard Biesheuvel, Grant Likely, linux-api, linux-doc,
Mike Waychison, dmidecode-devel, Leif Lindholm, Mark Salter
On Wed, Apr 15, 2015 at 11:48 PM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Roy,
>
> On Wed, 15 Apr 2015 17:54:51 -0700, Roy Franz wrote:
>> On Tue, Apr 14, 2015 at 9:19 PM, Roy Franz <roy.franz@linaro.org> wrote:
>> > I have made modifications to dmidecode to support this interface, and it
>> > works quite nicely for dmidecode. (changes posted to dmidecode-devel list)
>> > The only open issue I am aware of is how the SMBIOS v3 entry point
>> > will be handled,
>> > especially in cases where there is a v2 and a v3 entry point.
>> > In principal I think this a good change - are there any other open issues?
>>
>> I looked at the SMBIOS spec again, and the platform can provide either or
>> both of the 32-bit and 64-bit entry points. The spec says that an
>> implementation
>> "should" provide a 32-bit entry point for compatibility.
>>
>> These 2 entry point structures can both point to the same SMBIOS
>> structure table,
>> or two distinct ones. If distinct, the one referenced by the 32-bit
>> entry point must be
>> a consistent subset of the 64-bit one.
>> There does not seem to be any prohibition from using new SMBIOS v3 structures
>> in a table referenced by a 32-bit entry point, so backwards
>> compatibility is completely
>> up to the implementation.
>
> What do you mean with "new SMBIOS v3 structures"? I took a brief look
> at the change log at the end of the SMBIOS 3.0.0 specification, and all
> I can see are new enumerated values for existing fields, as well as 3
> new fields in the type 4 structure. I can't see any new structure type.
> Either way this is all backwards compatible, which is why both entry
> points can point to the same table.
Sorry, I used that imprecisely - I was referring to the new things defined
in the 3.0 specification in addition to the new entry point. It sounds like
there is not much.
>
>> Since the point of this patchset (and related changes to dmidecode) is
>> to provide the
>> SMBIOS information without using /dev/mem, I think the interface we
>> define should
>> support all the cases outlined in the specification. I would like to
>> avoid a case where
>> we're back to using /dev/mem to deal with the dual entry/dual table
>> case if that becomes
>> important down the road.
>>
>> Here's a proposal for files in /sys/firmware/dmi/tables:
>>
>> smbios_entry_point32 - 32 bit (existing entry point type), if it exists.
>> smbios_entry_point64 - 64 bit entry, new in SMBIOS v3.0
>> DMI32 - smbios structure tables referenced by
>> 32 bit entry point, if it exists
>> DMI64 - smbios structure tables referenced by
>> 64 bit entry point, if it exists.
>> symlink to DMI32 if identical
>> smbios_entry_point - symlink to smbios_entry_point64 if it exists,
>> otherwise symlink to smbios_entry_point64
>> DMI - symlink to DMI64 if it exists,
>> otherwise symlink to DMI32
>>
>> These last two would provide names to the 'preferred' tables, and
>> names that would always exist on all systems, which
>> I think is a nice property to have.
>>
>> Thoughts?
>
> The idea was discussed before, and discarded. The question is, what use
> cases do you envision from a user-space application perspective? As you
> found out, the table pointed to by the _SM3_ entry point must be a
> super-set of the one pointed to by the _SM_ entry point, so for a
> compliant implementation there is no reason to follow the _SM_ entry
> point when both exist. And if only one entry point exists, there is
> nothing to choose from.
>
> The kernel itself will have to choose one of the entry points when it
> comes to dmi_scan and dmi-id. It can't provide two sets of DMI strings.
> So it seems reasonable to only expose though sysfs that one entry point
> and table that the kernel itself used.
>
> The only case where it would make some sense to expose everything to
> user-space is for BIOS debugging purpose: if both entry points exist
> and point to different tables, and if the _SM3_ table is broken and
> the _SM_ table is correct, then you may want to be able to temporarily
> read the _SM_ table instead of the _SM3_ table, to check what needs to
> be fixed in the latter. But I would argue that this is beyond the scope
> of our code: neither the kernel nor dmidecode are DMI table or BIOS
> authoring tools.
>
> If there really ever is a need to ignore the _SM3_ entry point, I'd
> rather have a kernel boot parameter for this. This would guarantee that
> the kernel and user-space applications always operate on the same data.
>
> So I would keep the sysfs interface simple, as Ivan implemented it.
Apologies for missing that previous discussion - I was intentionally
bringing an old topic up.
I don't feel strongly, and I think the main use case is debugging
something in the future,
as it doesn't address any current problems. My main motivation for
proposing this was to
avoid a future case where the answer is "just map /dev/mem". This is
acceptable if the only
people doing it are developers debugging a BIOS/UEFI problem, but I
don't think this is the
right answer for general use case. If a general use case emerges in
the future that requires
both tables exposed, we can extend the sysfs interface then (and this
may well never happen.)
Thanks,
Roy
>
> --
> Jean Delvare
> SUSE L3 Support
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
2015-04-02 12:57 ` Ivan Khoronzhuk
(?)
(?)
@ 2015-04-16 9:52 ` Jean Delvare
2015-04-16 12:56 ` Ivan.khoronzhuk
-1 siblings, 1 reply; 41+ messages in thread
From: Jean Delvare @ 2015-04-16 9:52 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: linux-kernel, matt.fleming, ard.biesheuvel, grant.likely,
linux-api, linux-doc, mikew, dmidecode-devel, leif.lindholm,
msalter, roy.franz
Hi Ivan,
On Thu, 2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
> Some utils, like dmidecode and smbios, need to access SMBIOS entry
> table area in order to get information like SMBIOS version, size, etc.
> Currently it's done via /dev/mem. But for situation when /dev/mem
> usage is disabled, the utils have to use dmi sysfs instead, which
> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
> access for table is needed.
>
> So this patch creates dmi/tables and adds SMBIOS entry point to allow
> utils in question to work correctly without /dev/mem. Also patch adds
> raw dmi table to simplify dmi table processing in user space, as
> proposed by Jean Delvare.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
> ---
> .../ABI/testing/sysfs-firmware-dmi-tables | 22 ++++++
> drivers/firmware/dmi-sysfs.c | 11 ++-
> drivers/firmware/dmi_scan.c | 80 ++++++++++++++++++++++
> include/linux/dmi.h | 1 +
> 4 files changed, 107 insertions(+), 7 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
First of all: thanks for doing this, it looks mostly good now, and I've
tested it successfully on my own system.
> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
> new file mode 100644
> index 0000000..f46158c
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
> @@ -0,0 +1,22 @@
> +What: /sys/firmware/dmi/tables/
> +Date: April 2015
> +Contact: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
> +Description:
> + The firmware provides DMI structures as a packed list of
> + data referenced by a SMBIOS table entry point. The SMBIOS
> + entry point contains general information, like SMBIOS
> + version, DMI table size, etc. The structure, content and
> + size of SMBIOS entry point is dependent on SMBIOS version.
> + The format of SMBIOS entry point, equal as DMI structures
> + can be read in SMBIOS specification.
"equal as" sounds strange, I think a simple "and" would be better.
> +
> + The dmi/tables provides raw SMBIOS entry point and DMI tables
> + through sysfs as an alternative to utilities reading them
> + from /dev/mem. The raw SMBIOS entry point and DMI table are
> + presented as raw attributes and are accessible via:
"binary attributes" rather than "raw attributes"? The "raw" nature is
already mentioned earlier in the sentence.
> +
> + /sys/firmware/dmi/tables/smbios_entry_point
> + /sys/firmware/dmi/tables/DMI
> +
> + The complete DMI information can be taken using these two
"obtained" or "decoded" would sound better than "taken" IMHO.
> + tables.
> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
> index e0f1cb3..8e1a411 100644
> --- a/drivers/firmware/dmi-sysfs.c
> +++ b/drivers/firmware/dmi-sysfs.c
> @@ -566,7 +566,6 @@ static struct kobj_type dmi_sysfs_entry_ktype = {
> .default_attrs = dmi_sysfs_entry_attrs,
> };
>
> -static struct kobject *dmi_kobj;
> static struct kset *dmi_kset;
>
> /* Global count of all instances seen. Only for setup */
> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
> int error = -ENOMEM;
This initialization can be moved to the single error path left that
needs it. (I can do it myself in a separate patch if you don't want to
do it here.)
> int val;
>
> - /* Set up our directory */
> - dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> - if (!dmi_kobj)
> + if (!dmi_kobj) {
> + pr_err("dmi-sysfs: dmi entry is absent.\n");
> + error = -ENOSYS;
> goto err;
> + }
>
> dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
> if (!dmi_kset)
> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
> err:
> cleanup_entry_list();
> kset_unregister(dmi_kset);
> - kobject_put(dmi_kobj);
> return error;
> }
>
> @@ -685,8 +684,6 @@ static void __exit dmi_sysfs_exit(void)
> pr_debug("dmi-sysfs: unloading.\n");
> cleanup_entry_list();
> kset_unregister(dmi_kset);
> - kobject_del(dmi_kobj);
> - kobject_put(dmi_kobj);
> }
>
> module_init(dmi_sysfs_init);
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index d3aae09..bb19f8b 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -10,6 +10,9 @@
> #include <asm/dmi.h>
> #include <asm/unaligned.h>
>
> +struct kobject *dmi_kobj;
> +EXPORT_SYMBOL_GPL(dmi_kobj);
> +
> /*
> * DMI stands for "Desktop Management Interface". It is part
> * of and an antecedent to, SMBIOS, which stands for System
> @@ -20,6 +23,9 @@ static const char dmi_empty_string[] = " ";
> static u32 dmi_ver __initdata;
> static u32 dmi_len;
> static u16 dmi_num;
> +static u8 smbios_entry_point[32];
> +static int smbios_entry_point_size;
> +
> /*
> * Catch too early calls to dmi_check_system():
> */
> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
> }
>
> static phys_addr_t dmi_base;
> +static u8 *dmi_table;
This variable is only ever used in a single function (dmi_init). Does
it actually need to be a global variable? I suspect not.
>
> static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
> void *))
> @@ -476,6 +483,8 @@ static int __init dmi_present(const u8 *buf)
> if (memcmp(buf, "_SM_", 4) == 0 &&
> buf[5] < 32 && dmi_checksum(buf, buf[5])) {
> smbios_ver = get_unaligned_be16(buf + 6);
> + smbios_entry_point_size = buf[5];
> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>
> /* Some BIOS report weird SMBIOS version, fix that up */
> switch (smbios_ver) {
> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
> dmi_ver >> 8, dmi_ver & 0xFF,
> (dmi_ver < 0x0300) ? "" : ".x");
> } else {
> + smbios_entry_point_size = 15;
> + memcpy(smbios_entry_point, buf,
> + smbios_entry_point_size);
> dmi_ver = (buf[14] & 0xF0) << 4 |
> (buf[14] & 0x0F);
> pr_info("Legacy DMI %d.%d present.\n",
> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
> dmi_ver &= 0xFFFFFF;
> dmi_len = get_unaligned_le32(buf + 12);
> dmi_base = get_unaligned_le64(buf + 16);
> + smbios_entry_point_size = buf[6];
> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>
> /*
> * The 64-bit SMBIOS 3.0 entry point no longer has a field
> @@ -638,6 +652,72 @@ void __init dmi_scan_machine(void)
> dmi_initialized = 1;
> }
>
> +static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
> + struct bin_attribute *attr, char *buf,
> + loff_t pos, size_t count)
> +{
> + memcpy(buf, attr->private + pos, count);
> + return count;
> +}
> +
> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
This one could be world-readable as it contains no sensitive
information.
> +struct bin_attribute bin_attr_dmi_table =
> + __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
I do not understand why you don't use BIN_ATTR here too? I tried naming
the attribute bin_attr_DMI and it seems to work just fine, checkpatch
doesn't even complain!
> +
> +static int __init dmi_init(void)
> +{
> + int ret = -ENOMEM;
> + struct kobject *tables_kobj = NULL;
> +
> + if (!dmi_available) {
> + ret = -ENOSYS;
> + goto err;
> + }
This is weird. Can this actually happen?
We currently have two ways to enter this module: dmi_scan_machine(),
which is called by the architecture code, and dmi_init(), which is
called at subsys_initcall time. As far as I can see,
core/arch_initcalls are guaranteed to be always called before
subsys_initcalls. If we can rely on that, the test above is not needed.
If for any reason we can't, that means that dmi_init() should not be a
subsys_initcall, but should instead be called explicitly at the end of
dmi_scan_machine().
> +
> + /* Set up dmi directory at /sys/firmware/dmi */
> + dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
> + if (!dmi_kobj)
> + goto err;
> +
> + tables_kobj = kobject_create_and_add("tables", dmi_kobj);
> + if (!tables_kobj)
> + goto err;
> +
> + bin_attr_smbios_entry_point.size = smbios_entry_point_size;
> + bin_attr_smbios_entry_point.private = smbios_entry_point;
> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_smbios_entry_point);
> + if (ret)
> + goto err;
> +
> + dmi_table = dmi_remap(dmi_base, dmi_len);
> + if (!dmi_table)
> + goto err;
At this point "ret" has value 0 so you will take the error path but
return with success. No good. I suggest that you move the call to
dmi_remap before the first call to sysfs_create_bin_file above, so that
"ret" still has value -ENOMEM.
> +
> + bin_attr_dmi_table.size = dmi_len;
> + bin_attr_dmi_table.private = dmi_table;
> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
> + if (ret) {
> + dmi_unmap(dmi_table);
Doing this here goes against the logic of having a single error path.
Instead you should have an additional error label before err: and jump
there.
> + goto err;
> + }
> +
> + return 0;
> +err:
> + pr_err("dmi: Firmware registration failed.\n");
> +
> + if (tables_kobj)
> + sysfs_remove_bin_file(tables_kobj,
> + &bin_attr_smbios_entry_point);
> + kobject_del(tables_kobj);
> + kobject_put(tables_kobj);
These last two calls could be moved inside the conditional above. Even
better would be a separate error label so that you know exactly what
needs to be undone.
> + kobject_del(dmi_kobj);
> + kobject_put(dmi_kobj);
> + dmi_kobj = NULL;
I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
appropriate comment), so that dmi-sysfs has a chance to load? As it is
now, a bug or some unexpected behavior in this new code could cause a
regression for dmi-sysfs users. Just because I don't like dmi_sysfs
doesn't mean we can break it ;-)
> +
> + return ret;
> +}
> +subsys_initcall(dmi_init);
> +
> /**
> * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
> *
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index f820f0a..9f55f46 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -93,6 +93,7 @@ struct dmi_dev_onboard {
> int devfn;
> };
>
> +extern struct kobject *dmi_kobj;
struct kobject is defined in <linux/kobject.h> so I think you should
include that file to avoid random build failures in the future.
> extern int dmi_check_system(const struct dmi_system_id *list);
> const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
> extern const char * dmi_get_system_info(int field);
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-16 12:56 ` Ivan.khoronzhuk
0 siblings, 0 replies; 41+ messages in thread
From: Ivan.khoronzhuk @ 2015-04-16 12:56 UTC (permalink / raw)
To: Jean Delvare
Cc: linux-kernel, matt.fleming, ard.biesheuvel, grant.likely,
linux-api, linux-doc, mikew, dmidecode-devel, leif.lindholm,
msalter, roy.franz
Hi Jean,
On 16.04.15 12:52, Jean Delvare wrote:
> Hi Ivan,
>
> On Thu, 2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
>> Some utils, like dmidecode and smbios, need to access SMBIOS entry
>> table area in order to get information like SMBIOS version, size, etc.
>> Currently it's done via /dev/mem. But for situation when /dev/mem
>> usage is disabled, the utils have to use dmi sysfs instead, which
>> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
>> access for table is needed.
>>
>> So this patch creates dmi/tables and adds SMBIOS entry point to allow
>> utils in question to work correctly without /dev/mem. Also patch adds
>> raw dmi table to simplify dmi table processing in user space, as
>> proposed by Jean Delvare.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
>> ---
>> .../ABI/testing/sysfs-firmware-dmi-tables | 22 ++++++
>> drivers/firmware/dmi-sysfs.c | 11 ++-
>> drivers/firmware/dmi_scan.c | 80 ++++++++++++++++++++++
>> include/linux/dmi.h | 1 +
>> 4 files changed, 107 insertions(+), 7 deletions(-)
>> create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
> First of all: thanks for doing this, it looks mostly good now, and I've
> tested it successfully on my own system.
>
>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> new file mode 100644
>> index 0000000..f46158c
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> @@ -0,0 +1,22 @@
>> +What: /sys/firmware/dmi/tables/
>> +Date: April 2015
>> +Contact: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
>> +Description:
>> + The firmware provides DMI structures as a packed list of
>> + data referenced by a SMBIOS table entry point. The SMBIOS
>> + entry point contains general information, like SMBIOS
>> + version, DMI table size, etc. The structure, content and
>> + size of SMBIOS entry point is dependent on SMBIOS version.
>> + The format of SMBIOS entry point, equal as DMI structures
>> + can be read in SMBIOS specification.
> "equal as" sounds strange, I think a simple "and" would be better.
Ok
>
>> +
>> + The dmi/tables provides raw SMBIOS entry point and DMI tables
>> + through sysfs as an alternative to utilities reading them
>> + from /dev/mem. The raw SMBIOS entry point and DMI table are
>> + presented as raw attributes and are accessible via:
> "binary attributes" rather than "raw attributes"? The "raw" nature is
> already mentioned earlier in the sentence.
Ok
>
>> +
>> + /sys/firmware/dmi/tables/smbios_entry_point
>> + /sys/firmware/dmi/tables/DMI
>> +
>> + The complete DMI information can be taken using these two
> "obtained" or "decoded" would sound better than "taken" IMHO.
Ok
>
>> + tables.
>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>> index e0f1cb3..8e1a411 100644
>> --- a/drivers/firmware/dmi-sysfs.c
>> +++ b/drivers/firmware/dmi-sysfs.c
>> @@ -566,7 +566,6 @@ static struct kobj_type dmi_sysfs_entry_ktype = {
>> .default_attrs = dmi_sysfs_entry_attrs,
>> };
>>
>> -static struct kobject *dmi_kobj;
>> static struct kset *dmi_kset;
>>
>> /* Global count of all instances seen. Only for setup */
>> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>> int error = -ENOMEM;
> This initialization can be moved to the single error path left that
> needs it. (I can do it myself in a separate patch if you don't want to
> do it here.)
Let it be in this patch.
>> int val;
>>
>> - /* Set up our directory */
>> - dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> - if (!dmi_kobj)
>> + if (!dmi_kobj) {
>> + pr_err("dmi-sysfs: dmi entry is absent.\n");
>> + error = -ENOSYS;
>> goto err;
>> + }
>>
>> dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>> if (!dmi_kset)
>> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>> err:
>> cleanup_entry_list();
>> kset_unregister(dmi_kset);
>> - kobject_put(dmi_kobj);
>> return error;
>> }
>>
>> @@ -685,8 +684,6 @@ static void __exit dmi_sysfs_exit(void)
>> pr_debug("dmi-sysfs: unloading.\n");
>> cleanup_entry_list();
>> kset_unregister(dmi_kset);
>> - kobject_del(dmi_kobj);
>> - kobject_put(dmi_kobj);
>> }
>>
>> module_init(dmi_sysfs_init);
>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>> index d3aae09..bb19f8b 100644
>> --- a/drivers/firmware/dmi_scan.c
>> +++ b/drivers/firmware/dmi_scan.c
>> @@ -10,6 +10,9 @@
>> #include <asm/dmi.h>
>> #include <asm/unaligned.h>
>>
>> +struct kobject *dmi_kobj;
>> +EXPORT_SYMBOL_GPL(dmi_kobj);
>> +
>> /*
>> * DMI stands for "Desktop Management Interface". It is part
>> * of and an antecedent to, SMBIOS, which stands for System
>> @@ -20,6 +23,9 @@ static const char dmi_empty_string[] = " ";
>> static u32 dmi_ver __initdata;
>> static u32 dmi_len;
>> static u16 dmi_num;
>> +static u8 smbios_entry_point[32];
>> +static int smbios_entry_point_size;
>> +
>> /*
>> * Catch too early calls to dmi_check_system():
>> */
>> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>> }
>>
>> static phys_addr_t dmi_base;
>> +static u8 *dmi_table;
> This variable is only ever used in a single function (dmi_init). Does
> it actually need to be a global variable? I suspect not.
, Yes.
>
>>
>> static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>> void *))
>> @@ -476,6 +483,8 @@ static int __init dmi_present(const u8 *buf)
>> if (memcmp(buf, "_SM_", 4) == 0 &&
>> buf[5] < 32 && dmi_checksum(buf, buf[5])) {
>> smbios_ver = get_unaligned_be16(buf + 6);
>> + smbios_entry_point_size = buf[5];
>> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>>
>> /* Some BIOS report weird SMBIOS version, fix that up */
>> switch (smbios_ver) {
>> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>> dmi_ver >> 8, dmi_ver & 0xFF,
>> (dmi_ver < 0x0300) ? "" : ".x");
>> } else {
>> + smbios_entry_point_size = 15;
>> + memcpy(smbios_entry_point, buf,
>> + smbios_entry_point_size);
>> dmi_ver = (buf[14] & 0xF0) << 4 |
>> (buf[14] & 0x0F);
>> pr_info("Legacy DMI %d.%d present.\n",
>> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>> dmi_ver &= 0xFFFFFF;
>> dmi_len = get_unaligned_le32(buf + 12);
>> dmi_base = get_unaligned_le64(buf + 16);
>> + smbios_entry_point_size = buf[6];
>> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>>
>> /*
>> * The 64-bit SMBIOS 3.0 entry point no longer has a field
>> @@ -638,6 +652,72 @@ void __init dmi_scan_machine(void)
>> dmi_initialized = 1;
>> }
>>
>> +static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
>> + struct bin_attribute *attr, char *buf,
>> + loff_t pos, size_t count)
>> +{
>> + memcpy(buf, attr->private + pos, count);
>> + return count;
>> +}
>> +
>> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
> This one could be world-readable as it contains no sensitive
> information.
It contains the address of DMI table containing sensitive information.
Who knows which ways can be used to take it. Anyway, no see reasons in this
w/o DMI table. But if you insist I can do it "world-readable".
>
>> +struct bin_attribute bin_attr_dmi_table =
>> + __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
> I do not understand why you don't use BIN_ATTR here too? I tried naming
> the attribute bin_attr_DMI and it seems to work just fine, checkpatch
> doesn't even complain!
I dislike upper case in names, at least in such simple names.
It makes me using bin_attr_DMI lower in the code. That's why.
But if you like it, I will name it "bin_attr_DMI"
>
>> +
>> +static int __init dmi_init(void)
>> +{
>> + int ret = -ENOMEM;
>> + struct kobject *tables_kobj = NULL;
>> +
>> + if (!dmi_available) {
>> + ret = -ENOSYS;
>> + goto err;
>> + }
> This is weird. Can this actually happen?
>
> We currently have two ways to enter this module: dmi_scan_machine(),
> which is called by the architecture code, and dmi_init(), which is
> called at subsys_initcall time. As far as I can see,
> core/arch_initcalls are guaranteed to be always called before
> subsys_initcalls. If we can rely on that, the test above is not needed.
> If for any reason we can't, that means that dmi_init() should not be a
> subsys_initcall, but should instead be called explicitly at the end of
> dmi_scan_machine().
We cannot be sure that firmware_kobj created at time of dmi_init().
The sources don't oblige you to call it at core level,
for instance like it was done for arm64. For x86, dmi_init() can be called
before firmware_kobj is created. And if I call it from dmi_init() I suppose
I would face an error. As I can't call it in dmi_init I can't be sure that
DMI is available at all. So, no, we have to check dmi_available here and
call
it at subsys layer, where it's supposed to be.
>> +
>> + /* Set up dmi directory at /sys/firmware/dmi */
>> + dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> + if (!dmi_kobj)
>> + goto err;
>> +
>> + tables_kobj = kobject_create_and_add("tables", dmi_kobj);
>> + if (!tables_kobj)
>> + goto err;
>> +
>> + bin_attr_smbios_entry_point.size = smbios_entry_point_size;
>> + bin_attr_smbios_entry_point.private = smbios_entry_point;
>> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_smbios_entry_point);
>> + if (ret)
>> + goto err;
>> +
>> + dmi_table = dmi_remap(dmi_base, dmi_len);
>> + if (!dmi_table)
>> + goto err;
> At this point "ret" has value 0 so you will take the error path but
> return with success. No good. I suggest that you move the call to
> dmi_remap before the first call to sysfs_create_bin_file above, so that
> "ret" still has value -ENOMEM.
Yes.
>
>> +
>> + bin_attr_dmi_table.size = dmi_len;
>> + bin_attr_dmi_table.private = dmi_table;
>> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
>> + if (ret) {
>> + dmi_unmap(dmi_table);
> Doing this here goes against the logic of having a single error path.
> Instead you should have an additional error label before err: and jump
> there.
I will redo this.
>
>> + goto err;
>> + }
>> +
>> + return 0;
>> +err:
>> + pr_err("dmi: Firmware registration failed.\n");
>> +
>> + if (tables_kobj)
>> + sysfs_remove_bin_file(tables_kobj,
>> + &bin_attr_smbios_entry_point);
>> + kobject_del(tables_kobj);
>> + kobject_put(tables_kobj);
> These last two calls could be moved inside the conditional above. Even
> better would be a separate error label so that you know exactly what
> needs to be undone.
Let it be.
>
>> + kobject_del(dmi_kobj);
>> + kobject_put(dmi_kobj);
>> + dmi_kobj = NULL;
> I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
> appropriate comment), so that dmi-sysfs has a chance to load? As it is
> now, a bug or some unexpected behavior in this new code could cause a
> regression for dmi-sysfs users. Just because I don't like dmi_sysfs
> doesn't mean we can break it ;-)
As I remember it was not so critical for you last time.
"I don't care which way you choose". And I've explained my position.
But it's not very hard to me to change it. Anyway patch requires re-push.
>
>> +
>> + return ret;
>> +}
>> +subsys_initcall(dmi_init);
>> +
>> /**
>> * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
>> *
>> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
>> index f820f0a..9f55f46 100644
>> --- a/include/linux/dmi.h
>> +++ b/include/linux/dmi.h
>> @@ -93,6 +93,7 @@ struct dmi_dev_onboard {
>> int devfn;
>> };
>>
>> +extern struct kobject *dmi_kobj;
> struct kobject is defined in <linux/kobject.h> so I think you should
> include that file to avoid random build failures in the future.
Ok
>
>> extern int dmi_check_system(const struct dmi_system_id *list);
>> const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
>> extern const char * dmi_get_system_info(int field);
>
Thanks Jean.
I will add you propositions and re-push the series.
If you are OK with my answer of-course.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-16 12:56 ` Ivan.khoronzhuk
0 siblings, 0 replies; 41+ messages in thread
From: Ivan.khoronzhuk @ 2015-04-16 12:56 UTC (permalink / raw)
To: Jean Delvare
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
matt.fleming-ral2JQCrhuEAvxtiuMwx3w,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, mikew-hpIqsD4AKlfQT0dZR+AlfA,
dmidecode-devel-qX2TKyscuCcdnm+yROfE0A,
leif.lindholm-QSEj5FYQhm4dnm+yROfE0A,
msalter-H+wXaHxf7aLQT0dZR+AlfA, roy.franz-QSEj5FYQhm4dnm+yROfE0A
Hi Jean,
On 16.04.15 12:52, Jean Delvare wrote:
> Hi Ivan,
>
> On Thu, 2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
>> Some utils, like dmidecode and smbios, need to access SMBIOS entry
>> table area in order to get information like SMBIOS version, size, etc.
>> Currently it's done via /dev/mem. But for situation when /dev/mem
>> usage is disabled, the utils have to use dmi sysfs instead, which
>> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
>> access for table is needed.
>>
>> So this patch creates dmi/tables and adds SMBIOS entry point to allow
>> utils in question to work correctly without /dev/mem. Also patch adds
>> raw dmi table to simplify dmi table processing in user space, as
>> proposed by Jean Delvare.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
>> ---
>> .../ABI/testing/sysfs-firmware-dmi-tables | 22 ++++++
>> drivers/firmware/dmi-sysfs.c | 11 ++-
>> drivers/firmware/dmi_scan.c | 80 ++++++++++++++++++++++
>> include/linux/dmi.h | 1 +
>> 4 files changed, 107 insertions(+), 7 deletions(-)
>> create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-tables
> First of all: thanks for doing this, it looks mostly good now, and I've
> tested it successfully on my own system.
>
>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi-tables b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> new file mode 100644
>> index 0000000..f46158c
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi-tables
>> @@ -0,0 +1,22 @@
>> +What: /sys/firmware/dmi/tables/
>> +Date: April 2015
>> +Contact: Ivan Khoronzhuk <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
>> +Description:
>> + The firmware provides DMI structures as a packed list of
>> + data referenced by a SMBIOS table entry point. The SMBIOS
>> + entry point contains general information, like SMBIOS
>> + version, DMI table size, etc. The structure, content and
>> + size of SMBIOS entry point is dependent on SMBIOS version.
>> + The format of SMBIOS entry point, equal as DMI structures
>> + can be read in SMBIOS specification.
> "equal as" sounds strange, I think a simple "and" would be better.
Ok
>
>> +
>> + The dmi/tables provides raw SMBIOS entry point and DMI tables
>> + through sysfs as an alternative to utilities reading them
>> + from /dev/mem. The raw SMBIOS entry point and DMI table are
>> + presented as raw attributes and are accessible via:
> "binary attributes" rather than "raw attributes"? The "raw" nature is
> already mentioned earlier in the sentence.
Ok
>
>> +
>> + /sys/firmware/dmi/tables/smbios_entry_point
>> + /sys/firmware/dmi/tables/DMI
>> +
>> + The complete DMI information can be taken using these two
> "obtained" or "decoded" would sound better than "taken" IMHO.
Ok
>
>> + tables.
>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>> index e0f1cb3..8e1a411 100644
>> --- a/drivers/firmware/dmi-sysfs.c
>> +++ b/drivers/firmware/dmi-sysfs.c
>> @@ -566,7 +566,6 @@ static struct kobj_type dmi_sysfs_entry_ktype = {
>> .default_attrs = dmi_sysfs_entry_attrs,
>> };
>>
>> -static struct kobject *dmi_kobj;
>> static struct kset *dmi_kset;
>>
>> /* Global count of all instances seen. Only for setup */
>> @@ -651,10 +650,11 @@ static int __init dmi_sysfs_init(void)
>> int error = -ENOMEM;
> This initialization can be moved to the single error path left that
> needs it. (I can do it myself in a separate patch if you don't want to
> do it here.)
Let it be in this patch.
>> int val;
>>
>> - /* Set up our directory */
>> - dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> - if (!dmi_kobj)
>> + if (!dmi_kobj) {
>> + pr_err("dmi-sysfs: dmi entry is absent.\n");
>> + error = -ENOSYS;
>> goto err;
>> + }
>>
>> dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>> if (!dmi_kset)
>> @@ -675,7 +675,6 @@ static int __init dmi_sysfs_init(void)
>> err:
>> cleanup_entry_list();
>> kset_unregister(dmi_kset);
>> - kobject_put(dmi_kobj);
>> return error;
>> }
>>
>> @@ -685,8 +684,6 @@ static void __exit dmi_sysfs_exit(void)
>> pr_debug("dmi-sysfs: unloading.\n");
>> cleanup_entry_list();
>> kset_unregister(dmi_kset);
>> - kobject_del(dmi_kobj);
>> - kobject_put(dmi_kobj);
>> }
>>
>> module_init(dmi_sysfs_init);
>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>> index d3aae09..bb19f8b 100644
>> --- a/drivers/firmware/dmi_scan.c
>> +++ b/drivers/firmware/dmi_scan.c
>> @@ -10,6 +10,9 @@
>> #include <asm/dmi.h>
>> #include <asm/unaligned.h>
>>
>> +struct kobject *dmi_kobj;
>> +EXPORT_SYMBOL_GPL(dmi_kobj);
>> +
>> /*
>> * DMI stands for "Desktop Management Interface". It is part
>> * of and an antecedent to, SMBIOS, which stands for System
>> @@ -20,6 +23,9 @@ static const char dmi_empty_string[] = " ";
>> static u32 dmi_ver __initdata;
>> static u32 dmi_len;
>> static u16 dmi_num;
>> +static u8 smbios_entry_point[32];
>> +static int smbios_entry_point_size;
>> +
>> /*
>> * Catch too early calls to dmi_check_system():
>> */
>> @@ -118,6 +124,7 @@ static void dmi_decode_table(u8 *buf,
>> }
>>
>> static phys_addr_t dmi_base;
>> +static u8 *dmi_table;
> This variable is only ever used in a single function (dmi_init). Does
> it actually need to be a global variable? I suspect not.
, Yes.
>
>>
>> static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>> void *))
>> @@ -476,6 +483,8 @@ static int __init dmi_present(const u8 *buf)
>> if (memcmp(buf, "_SM_", 4) == 0 &&
>> buf[5] < 32 && dmi_checksum(buf, buf[5])) {
>> smbios_ver = get_unaligned_be16(buf + 6);
>> + smbios_entry_point_size = buf[5];
>> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>>
>> /* Some BIOS report weird SMBIOS version, fix that up */
>> switch (smbios_ver) {
>> @@ -508,6 +517,9 @@ static int __init dmi_present(const u8 *buf)
>> dmi_ver >> 8, dmi_ver & 0xFF,
>> (dmi_ver < 0x0300) ? "" : ".x");
>> } else {
>> + smbios_entry_point_size = 15;
>> + memcpy(smbios_entry_point, buf,
>> + smbios_entry_point_size);
>> dmi_ver = (buf[14] & 0xF0) << 4 |
>> (buf[14] & 0x0F);
>> pr_info("Legacy DMI %d.%d present.\n",
>> @@ -535,6 +547,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>> dmi_ver &= 0xFFFFFF;
>> dmi_len = get_unaligned_le32(buf + 12);
>> dmi_base = get_unaligned_le64(buf + 16);
>> + smbios_entry_point_size = buf[6];
>> + memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>>
>> /*
>> * The 64-bit SMBIOS 3.0 entry point no longer has a field
>> @@ -638,6 +652,72 @@ void __init dmi_scan_machine(void)
>> dmi_initialized = 1;
>> }
>>
>> +static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
>> + struct bin_attribute *attr, char *buf,
>> + loff_t pos, size_t count)
>> +{
>> + memcpy(buf, attr->private + pos, count);
>> + return count;
>> +}
>> +
>> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
> This one could be world-readable as it contains no sensitive
> information.
It contains the address of DMI table containing sensitive information.
Who knows which ways can be used to take it. Anyway, no see reasons in this
w/o DMI table. But if you insist I can do it "world-readable".
>
>> +struct bin_attribute bin_attr_dmi_table =
>> + __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
> I do not understand why you don't use BIN_ATTR here too? I tried naming
> the attribute bin_attr_DMI and it seems to work just fine, checkpatch
> doesn't even complain!
I dislike upper case in names, at least in such simple names.
It makes me using bin_attr_DMI lower in the code. That's why.
But if you like it, I will name it "bin_attr_DMI"
>
>> +
>> +static int __init dmi_init(void)
>> +{
>> + int ret = -ENOMEM;
>> + struct kobject *tables_kobj = NULL;
>> +
>> + if (!dmi_available) {
>> + ret = -ENOSYS;
>> + goto err;
>> + }
> This is weird. Can this actually happen?
>
> We currently have two ways to enter this module: dmi_scan_machine(),
> which is called by the architecture code, and dmi_init(), which is
> called at subsys_initcall time. As far as I can see,
> core/arch_initcalls are guaranteed to be always called before
> subsys_initcalls. If we can rely on that, the test above is not needed.
> If for any reason we can't, that means that dmi_init() should not be a
> subsys_initcall, but should instead be called explicitly at the end of
> dmi_scan_machine().
We cannot be sure that firmware_kobj created at time of dmi_init().
The sources don't oblige you to call it at core level,
for instance like it was done for arm64. For x86, dmi_init() can be called
before firmware_kobj is created. And if I call it from dmi_init() I suppose
I would face an error. As I can't call it in dmi_init I can't be sure that
DMI is available at all. So, no, we have to check dmi_available here and
call
it at subsys layer, where it's supposed to be.
>> +
>> + /* Set up dmi directory at /sys/firmware/dmi */
>> + dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> + if (!dmi_kobj)
>> + goto err;
>> +
>> + tables_kobj = kobject_create_and_add("tables", dmi_kobj);
>> + if (!tables_kobj)
>> + goto err;
>> +
>> + bin_attr_smbios_entry_point.size = smbios_entry_point_size;
>> + bin_attr_smbios_entry_point.private = smbios_entry_point;
>> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_smbios_entry_point);
>> + if (ret)
>> + goto err;
>> +
>> + dmi_table = dmi_remap(dmi_base, dmi_len);
>> + if (!dmi_table)
>> + goto err;
> At this point "ret" has value 0 so you will take the error path but
> return with success. No good. I suggest that you move the call to
> dmi_remap before the first call to sysfs_create_bin_file above, so that
> "ret" still has value -ENOMEM.
Yes.
>
>> +
>> + bin_attr_dmi_table.size = dmi_len;
>> + bin_attr_dmi_table.private = dmi_table;
>> + ret = sysfs_create_bin_file(tables_kobj, &bin_attr_dmi_table);
>> + if (ret) {
>> + dmi_unmap(dmi_table);
> Doing this here goes against the logic of having a single error path.
> Instead you should have an additional error label before err: and jump
> there.
I will redo this.
>
>> + goto err;
>> + }
>> +
>> + return 0;
>> +err:
>> + pr_err("dmi: Firmware registration failed.\n");
>> +
>> + if (tables_kobj)
>> + sysfs_remove_bin_file(tables_kobj,
>> + &bin_attr_smbios_entry_point);
>> + kobject_del(tables_kobj);
>> + kobject_put(tables_kobj);
> These last two calls could be moved inside the conditional above. Even
> better would be a separate error label so that you know exactly what
> needs to be undone.
Let it be.
>
>> + kobject_del(dmi_kobj);
>> + kobject_put(dmi_kobj);
>> + dmi_kobj = NULL;
> I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
> appropriate comment), so that dmi-sysfs has a chance to load? As it is
> now, a bug or some unexpected behavior in this new code could cause a
> regression for dmi-sysfs users. Just because I don't like dmi_sysfs
> doesn't mean we can break it ;-)
As I remember it was not so critical for you last time.
"I don't care which way you choose". And I've explained my position.
But it's not very hard to me to change it. Anyway patch requires re-push.
>
>> +
>> + return ret;
>> +}
>> +subsys_initcall(dmi_init);
>> +
>> /**
>> * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
>> *
>> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
>> index f820f0a..9f55f46 100644
>> --- a/include/linux/dmi.h
>> +++ b/include/linux/dmi.h
>> @@ -93,6 +93,7 @@ struct dmi_dev_onboard {
>> int devfn;
>> };
>>
>> +extern struct kobject *dmi_kobj;
> struct kobject is defined in <linux/kobject.h> so I think you should
> include that file to avoid random build failures in the future.
Ok
>
>> extern int dmi_check_system(const struct dmi_system_id *list);
>> const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
>> extern const char * dmi_get_system_info(int field);
>
Thanks Jean.
I will add you propositions and re-push the series.
If you are OK with my answer of-course.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
2015-04-16 12:56 ` Ivan.khoronzhuk
@ 2015-04-16 15:44 ` Jean Delvare
-1 siblings, 0 replies; 41+ messages in thread
From: Jean Delvare @ 2015-04-16 15:44 UTC (permalink / raw)
To: Ivan.khoronzhuk
Cc: linux-kernel, matt.fleming, ard.biesheuvel, grant.likely,
linux-api, linux-doc, mikew, dmidecode-devel, leif.lindholm,
msalter, roy.franz
Hi Ivan,
Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
> On 16.04.15 12:52, Jean Delvare wrote:
> > On Thu, 2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
> >> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
> > This one could be world-readable as it contains no sensitive
> > information.
>
> It contains the address of DMI table containing sensitive information.
> Who knows which ways can be used to take it. Anyway, no see reasons in this
> w/o DMI table. But if you insist I can do it "world-readable".
OK, you convinced me.
> >
> >> +struct bin_attribute bin_attr_dmi_table =
> >> + __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
> > I do not understand why you don't use BIN_ATTR here too? I tried naming
> > the attribute bin_attr_DMI and it seems to work just fine, checkpatch
> > doesn't even complain!
>
> I dislike upper case in names, at least in such simple names.
> It makes me using bin_attr_DMI lower in the code. That's why.
> But if you like it, I will name it "bin_attr_DMI"
I don't like upper case in names either, but in this specific case, I'd
do it for consistency. As you wish though, I really only wanted to know
the reason.
> >> +
> >> +static int __init dmi_init(void)
> >> +{
> >> + int ret = -ENOMEM;
> >> + struct kobject *tables_kobj = NULL;
> >> +
> >> + if (!dmi_available) {
> >> + ret = -ENOSYS;
> >> + goto err;
> >> + }
> > This is weird. Can this actually happen?
> >
> > We currently have two ways to enter this module: dmi_scan_machine(),
> > which is called by the architecture code, and dmi_init(), which is
> > called at subsys_initcall time. As far as I can see,
> > core/arch_initcalls are guaranteed to be always called before
> > subsys_initcalls. If we can rely on that, the test above is not needed.
> > If for any reason we can't, that means that dmi_init() should not be a
> > subsys_initcall, but should instead be called explicitly at the end of
> > dmi_scan_machine().
>
> We cannot be sure that firmware_kobj created at time of dmi_init().
> The sources don't oblige you to call it at core level,
> for instance like it was done for arm64. For x86, dmi_init() can be called
> before firmware_kobj is created.
Looking at the code, it seems that firmware_kobj is created very, very
early in the boot process. In do_basic_setup(), you can see that
driver_init() (which in turn calls firmware_init(), creating
firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
defined before dmi_scan_machine() or dmi_init() is called.
Oh, and this wasn't even my point ;-) I'm fine with you checking if
firmware_kobj is defined. My question was about the dmi_available check
above. But that question was silly anyway, sorry. I confused
dmi_available with dmi_initialized. Checking for dmi_available is
perfectly reasonable, please scratch my objection.
> And if I call it from dmi_init() I suppose
> I would face an error. As I can't call it in dmi_init I can't be sure that
> DMI is available at all. So, no, we have to check dmi_available here and
> call it at subsys layer, where it's supposed to be.
I can't parse that, I suspect you wrote dmi_init where you actually
meant dmi_scan_machine? Given how early firmware_kobj is created, I
think the code currently in dmi_init could in fact go at the end of
dmi_scan_machine. But it's not important for the time being, this can be
attempted later.
> >> (...)
> >> + kobject_del(dmi_kobj);
> >> + kobject_put(dmi_kobj);
> >> + dmi_kobj = NULL;
> > I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
> > appropriate comment), so that dmi-sysfs has a chance to load? As it is
> > now, a bug or some unexpected behavior in this new code could cause a
> > regression for dmi-sysfs users. Just because I don't like dmi_sysfs
> > doesn't mean we can break it ;-)
>
> As I remember it was not so critical for you last time.
> "I don't care which way you choose". And I've explained my position.
> But it's not very hard to me to change it. Anyway patch requires re-push.
You're right, I did not remember we had discussed this already,
sorry :-(
Well, I still agree that it doesn't really matter, but of two acceptable
solutions for an event which will most likely never happen, why not go
for the ones with the fewer lines of code? ;-)
> (...)
> Thanks Jean.
> I will add you propositions and re-push the series.
> If you are OK with my answer of-course.
You're welcome. Yes I think we have clarified everything now so you can
submit the next (hopefully last) iteration.
Thank you,
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [dmidecode] [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-16 15:44 ` Jean Delvare
0 siblings, 0 replies; 41+ messages in thread
From: Jean Delvare @ 2015-04-16 15:44 UTC (permalink / raw)
To: Ivan.khoronzhuk
Cc: dmidecode-devel, matt.fleming, linux-doc, linux-api,
ard.biesheuvel, linux-kernel, leif.lindholm, mikew, roy.franz,
msalter, grant.likely
Hi Ivan,
Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
> On 16.04.15 12:52, Jean Delvare wrote:
> > On Thu, 2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
> >> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
> > This one could be world-readable as it contains no sensitive
> > information.
>
> It contains the address of DMI table containing sensitive information.
> Who knows which ways can be used to take it. Anyway, no see reasons in this
> w/o DMI table. But if you insist I can do it "world-readable".
OK, you convinced me.
> >
> >> +struct bin_attribute bin_attr_dmi_table =
> >> + __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
> > I do not understand why you don't use BIN_ATTR here too? I tried naming
> > the attribute bin_attr_DMI and it seems to work just fine, checkpatch
> > doesn't even complain!
>
> I dislike upper case in names, at least in such simple names.
> It makes me using bin_attr_DMI lower in the code. That's why.
> But if you like it, I will name it "bin_attr_DMI"
I don't like upper case in names either, but in this specific case, I'd
do it for consistency. As you wish though, I really only wanted to know
the reason.
> >> +
> >> +static int __init dmi_init(void)
> >> +{
> >> + int ret = -ENOMEM;
> >> + struct kobject *tables_kobj = NULL;
> >> +
> >> + if (!dmi_available) {
> >> + ret = -ENOSYS;
> >> + goto err;
> >> + }
> > This is weird. Can this actually happen?
> >
> > We currently have two ways to enter this module: dmi_scan_machine(),
> > which is called by the architecture code, and dmi_init(), which is
> > called at subsys_initcall time. As far as I can see,
> > core/arch_initcalls are guaranteed to be always called before
> > subsys_initcalls. If we can rely on that, the test above is not needed.
> > If for any reason we can't, that means that dmi_init() should not be a
> > subsys_initcall, but should instead be called explicitly at the end of
> > dmi_scan_machine().
>
> We cannot be sure that firmware_kobj created at time of dmi_init().
> The sources don't oblige you to call it at core level,
> for instance like it was done for arm64. For x86, dmi_init() can be called
> before firmware_kobj is created.
Looking at the code, it seems that firmware_kobj is created very, very
early in the boot process. In do_basic_setup(), you can see that
driver_init() (which in turn calls firmware_init(), creating
firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
defined before dmi_scan_machine() or dmi_init() is called.
Oh, and this wasn't even my point ;-) I'm fine with you checking if
firmware_kobj is defined. My question was about the dmi_available check
above. But that question was silly anyway, sorry. I confused
dmi_available with dmi_initialized. Checking for dmi_available is
perfectly reasonable, please scratch my objection.
> And if I call it from dmi_init() I suppose
> I would face an error. As I can't call it in dmi_init I can't be sure that
> DMI is available at all. So, no, we have to check dmi_available here and
> call it at subsys layer, where it's supposed to be.
I can't parse that, I suspect you wrote dmi_init where you actually
meant dmi_scan_machine? Given how early firmware_kobj is created, I
think the code currently in dmi_init could in fact go at the end of
dmi_scan_machine. But it's not important for the time being, this can be
attempted later.
> >> (...)
> >> + kobject_del(dmi_kobj);
> >> + kobject_put(dmi_kobj);
> >> + dmi_kobj = NULL;
> > I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
> > appropriate comment), so that dmi-sysfs has a chance to load? As it is
> > now, a bug or some unexpected behavior in this new code could cause a
> > regression for dmi-sysfs users. Just because I don't like dmi_sysfs
> > doesn't mean we can break it ;-)
>
> As I remember it was not so critical for you last time.
> "I don't care which way you choose". And I've explained my position.
> But it's not very hard to me to change it. Anyway patch requires re-push.
You're right, I did not remember we had discussed this already,
sorry :-(
Well, I still agree that it doesn't really matter, but of two acceptable
solutions for an event which will most likely never happen, why not go
for the ones with the fewer lines of code? ;-)
> (...)
> Thanks Jean.
> I will add you propositions and re-push the series.
> If you are OK with my answer of-course.
You're welcome. Yes I think we have clarified everything now so you can
submit the next (hopefully last) iteration.
Thank you,
--
Jean Delvare
SUSE L3 Support
_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
2015-04-16 15:44 ` [dmidecode] " Jean Delvare
(?)
@ 2015-04-16 17:27 ` subscivan
2015-04-16 17:32 ` Ivan.khoronzhuk
2015-04-17 13:02 ` Jean Delvare
-1 siblings, 2 replies; 41+ messages in thread
From: subscivan @ 2015-04-16 17:27 UTC (permalink / raw)
To: Jean Delvare, Ivan.khoronzhuk
Cc: linux-kernel, matt.fleming, ard.biesheuvel, grant.likely,
linux-api, linux-doc, mikew, dmidecode-devel, leif.lindholm,
msalter, roy.franz
Jean,
On 16.04.15 18:44, Jean Delvare wrote:
> Hi Ivan,
>
> Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
>> On 16.04.15 12:52, Jean Delvare wrote:
>>> On Thu, 2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
>>>> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
>>> This one could be world-readable as it contains no sensitive
>>> information.
>> It contains the address of DMI table containing sensitive information.
>> Who knows which ways can be used to take it. Anyway, no see reasons in this
>> w/o DMI table. But if you insist I can do it "world-readable".
> OK, you convinced me.
>
>>>> +struct bin_attribute bin_attr_dmi_table =
>>>> + __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>>> I do not understand why you don't use BIN_ATTR here too? I tried naming
>>> the attribute bin_attr_DMI and it seems to work just fine, checkpatch
>>> doesn't even complain!
>> I dislike upper case in names, at least in such simple names.
>> It makes me using bin_attr_DMI lower in the code. That's why.
>> But if you like it, I will name it "bin_attr_DMI"
> I don't like upper case in names either, but in this specific case, I'd
> do it for consistency. As you wish though, I really only wanted to know
> the reason.
>
>>>> +
>>>> +static int __init dmi_init(void)
>>>> +{
>>>> + int ret = -ENOMEM;
>>>> + struct kobject *tables_kobj = NULL;
>>>> +
>>>> + if (!dmi_available) {
>>>> + ret = -ENOSYS;
>>>> + goto err;
>>>> + }
>>> This is weird. Can this actually happen?
>>>
>>> We currently have two ways to enter this module: dmi_scan_machine(),
>>> which is called by the architecture code, and dmi_init(), which is
>>> called at subsys_initcall time. As far as I can see,
>>> core/arch_initcalls are guaranteed to be always called before
>>> subsys_initcalls. If we can rely on that, the test above is not needed.
>>> If for any reason we can't, that means that dmi_init() should not be a
>>> subsys_initcall, but should instead be called explicitly at the end of
>>> dmi_scan_machine().
>> We cannot be sure that firmware_kobj created at time of dmi_init().
>> The sources don't oblige you to call it at core level,
>> for instance like it was done for arm64. For x86, dmi_init() can be called
>> before firmware_kobj is created.
> Looking at the code, it seems that firmware_kobj is created very, very
> early in the boot process. In do_basic_setup(), you can see that
> driver_init() (which in turn calls firmware_init(), creating
> firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
> defined before dmi_scan_machine() or dmi_init() is called.
No. Not must, rather should. See below.
>
> Oh, and this wasn't even my point ;-) I'm fine with you checking if
> firmware_kobj is defined. My question was about the dmi_available check
> above. But that question was silly anyway, sorry. I confused
> dmi_available with dmi_initialized. Checking for dmi_available is
> perfectly reasonable, please scratch my objection.
>
>> And if I call it from dmi_init() I suppose
>> I would face an error. As I can't call it in dmi_init I can't be sure that
>> DMI is available at all. So, no, we have to check dmi_available here and
>> call it at subsys layer, where it's supposed to be.
> I can't parse that, I suspect you wrote dmi_init where you actually
> meant dmi_scan_machine? Given how early firmware_kobj is created, I
> think the code currently in dmi_init could in fact go at the end of
> dmi_scan_machine.
Actually, dmi_scan_machine can be called even earlier.
As I've sad, for x86, it's called before firmware_kobj is created.
kernel_start()
setup_arch()
dmi_scan_machine()
And for firmware_init(), as you noticed already:
start_kernel()
rest_init()
kernel_init()
kernel_init_freeable()
do_basic_setup()
driver_init()
firmware_init()
Pay attentions that setup_arch() is called much earlier than rest_init().
So dmi_init couldn't in fact go at the end of dmi_scan_machine.
> But it's not important for the time being, this can be
> attempted later.
>
>>>> (...)
>>>> + kobject_del(dmi_kobj);
>>>> + kobject_put(dmi_kobj);
>>>> + dmi_kobj = NULL;
>>> I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
>>> appropriate comment), so that dmi-sysfs has a chance to load? As it is
>>> now, a bug or some unexpected behavior in this new code could cause a
>>> regression for dmi-sysfs users. Just because I don't like dmi_sysfs
>>> doesn't mean we can break it ;-)
>> As I remember it was not so critical for you last time.
>> "I don't care which way you choose". And I've explained my position.
>> But it's not very hard to me to change it. Anyway patch requires re-push.
> You're right, I did not remember we had discussed this already,
> sorry :-(
>
> Well, I still agree that it doesn't really matter, but of two acceptable
> solutions for an event which will most likely never happen, why not go
> for the ones with the fewer lines of code? ;-)
Ok.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-16 17:32 ` Ivan.khoronzhuk
0 siblings, 0 replies; 41+ messages in thread
From: Ivan.khoronzhuk @ 2015-04-16 17:32 UTC (permalink / raw)
To: subscivan, Jean Delvare
Cc: linux-kernel, matt.fleming, ard.biesheuvel, grant.likely,
linux-api, linux-doc, mikew, dmidecode-devel, leif.lindholm,
msalter, roy.franz
Sorry, sent it from not original mail.
On 16.04.15 20:27, subscivan wrote:
> Jean,
>
> On 16.04.15 18:44, Jean Delvare wrote:
>> Hi Ivan,
>>
>> Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
>>> On 16.04.15 12:52, Jean Delvare wrote:
>>>> On Thu, 2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
>>>>> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read,
>>>>> NULL, 0);
>>>> This one could be world-readable as it contains no sensitive
>>>> information.
>>> It contains the address of DMI table containing sensitive information.
>>> Who knows which ways can be used to take it. Anyway, no see reasons
>>> in this
>>> w/o DMI table. But if you insist I can do it "world-readable".
>> OK, you convinced me.
>>
>>>>> +struct bin_attribute bin_attr_dmi_table =
>>>>> + __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>>>> I do not understand why you don't use BIN_ATTR here too? I tried
>>>> naming
>>>> the attribute bin_attr_DMI and it seems to work just fine, checkpatch
>>>> doesn't even complain!
>>> I dislike upper case in names, at least in such simple names.
>>> It makes me using bin_attr_DMI lower in the code. That's why.
>>> But if you like it, I will name it "bin_attr_DMI"
>> I don't like upper case in names either, but in this specific case, I'd
>> do it for consistency. As you wish though, I really only wanted to know
>> the reason.
>>
>>>>> +
>>>>> +static int __init dmi_init(void)
>>>>> +{
>>>>> + int ret = -ENOMEM;
>>>>> + struct kobject *tables_kobj = NULL;
>>>>> +
>>>>> + if (!dmi_available) {
>>>>> + ret = -ENOSYS;
>>>>> + goto err;
>>>>> + }
>>>> This is weird. Can this actually happen?
>>>>
>>>> We currently have two ways to enter this module: dmi_scan_machine(),
>>>> which is called by the architecture code, and dmi_init(), which is
>>>> called at subsys_initcall time. As far as I can see,
>>>> core/arch_initcalls are guaranteed to be always called before
>>>> subsys_initcalls. If we can rely on that, the test above is not
>>>> needed.
>>>> If for any reason we can't, that means that dmi_init() should not be a
>>>> subsys_initcall, but should instead be called explicitly at the end of
>>>> dmi_scan_machine().
>>> We cannot be sure that firmware_kobj created at time of dmi_init().
>>> The sources don't oblige you to call it at core level,
>>> for instance like it was done for arm64. For x86, dmi_init() can be
>>> called
>>> before firmware_kobj is created.
>> Looking at the code, it seems that firmware_kobj is created very, very
>> early in the boot process. In do_basic_setup(), you can see that
>> driver_init() (which in turn calls firmware_init(), creating
>> firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
>> defined before dmi_scan_machine() or dmi_init() is called.
>
> No. Not must, rather should. See below.
>
>>
>> Oh, and this wasn't even my point ;-) I'm fine with you checking if
>> firmware_kobj is defined. My question was about the dmi_available check
>> above. But that question was silly anyway, sorry. I confused
>> dmi_available with dmi_initialized. Checking for dmi_available is
>> perfectly reasonable, please scratch my objection.
>>
>>> And if I call it from dmi_init() I suppose
>>> I would face an error. As I can't call it in dmi_init I can't be
>>> sure that
>>> DMI is available at all. So, no, we have to check dmi_available here
>>> and
>>> call it at subsys layer, where it's supposed to be.
>> I can't parse that, I suspect you wrote dmi_init where you actually
>> meant dmi_scan_machine? Given how early firmware_kobj is created, I
>> think the code currently in dmi_init could in fact go at the end of
>> dmi_scan_machine.
>
> Actually, dmi_scan_machine can be called even earlier.
> As I've sad, for x86, it's called before firmware_kobj is created.
>
> kernel_start()
> setup_arch()
> dmi_scan_machine()
>
> And for firmware_init(), as you noticed already:
>
> start_kernel()
> rest_init()
> kernel_init()
> kernel_init_freeable()
> do_basic_setup()
> driver_init()
> firmware_init()
>
> Pay attentions that setup_arch() is called much earlier than rest_init().
> So dmi_init couldn't in fact go at the end of dmi_scan_machine.
>
>> But it's not important for the time being, this can be
>> attempted later.
>>
>>>>> (...)
>>>>> + kobject_del(dmi_kobj);
>>>>> + kobject_put(dmi_kobj);
>>>>> + dmi_kobj = NULL;
>>>> I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
>>>> appropriate comment), so that dmi-sysfs has a chance to load? As it is
>>>> now, a bug or some unexpected behavior in this new code could cause a
>>>> regression for dmi-sysfs users. Just because I don't like dmi_sysfs
>>>> doesn't mean we can break it ;-)
>>> As I remember it was not so critical for you last time.
>>> "I don't care which way you choose". And I've explained my position.
>>> But it's not very hard to me to change it. Anyway patch requires
>>> re-push.
>> You're right, I did not remember we had discussed this already,
>> sorry :-(
>>
>> Well, I still agree that it doesn't really matter, but of two acceptable
>> solutions for an event which will most likely never happen, why not go
>> for the ones with the fewer lines of code? ;-)
>
> Ok.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-16 17:32 ` Ivan.khoronzhuk
0 siblings, 0 replies; 41+ messages in thread
From: Ivan.khoronzhuk @ 2015-04-16 17:32 UTC (permalink / raw)
To: subscivan, Jean Delvare
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
matt.fleming-ral2JQCrhuEAvxtiuMwx3w,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, mikew-hpIqsD4AKlfQT0dZR+AlfA,
dmidecode-devel-qX2TKyscuCcdnm+yROfE0A,
leif.lindholm-QSEj5FYQhm4dnm+yROfE0A,
msalter-H+wXaHxf7aLQT0dZR+AlfA, roy.franz-QSEj5FYQhm4dnm+yROfE0A
Sorry, sent it from not original mail.
On 16.04.15 20:27, subscivan wrote:
> Jean,
>
> On 16.04.15 18:44, Jean Delvare wrote:
>> Hi Ivan,
>>
>> Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
>>> On 16.04.15 12:52, Jean Delvare wrote:
>>>> On Thu, 2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
>>>>> +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read,
>>>>> NULL, 0);
>>>> This one could be world-readable as it contains no sensitive
>>>> information.
>>> It contains the address of DMI table containing sensitive information.
>>> Who knows which ways can be used to take it. Anyway, no see reasons
>>> in this
>>> w/o DMI table. But if you insist I can do it "world-readable".
>> OK, you convinced me.
>>
>>>>> +struct bin_attribute bin_attr_dmi_table =
>>>>> + __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
>>>> I do not understand why you don't use BIN_ATTR here too? I tried
>>>> naming
>>>> the attribute bin_attr_DMI and it seems to work just fine, checkpatch
>>>> doesn't even complain!
>>> I dislike upper case in names, at least in such simple names.
>>> It makes me using bin_attr_DMI lower in the code. That's why.
>>> But if you like it, I will name it "bin_attr_DMI"
>> I don't like upper case in names either, but in this specific case, I'd
>> do it for consistency. As you wish though, I really only wanted to know
>> the reason.
>>
>>>>> +
>>>>> +static int __init dmi_init(void)
>>>>> +{
>>>>> + int ret = -ENOMEM;
>>>>> + struct kobject *tables_kobj = NULL;
>>>>> +
>>>>> + if (!dmi_available) {
>>>>> + ret = -ENOSYS;
>>>>> + goto err;
>>>>> + }
>>>> This is weird. Can this actually happen?
>>>>
>>>> We currently have two ways to enter this module: dmi_scan_machine(),
>>>> which is called by the architecture code, and dmi_init(), which is
>>>> called at subsys_initcall time. As far as I can see,
>>>> core/arch_initcalls are guaranteed to be always called before
>>>> subsys_initcalls. If we can rely on that, the test above is not
>>>> needed.
>>>> If for any reason we can't, that means that dmi_init() should not be a
>>>> subsys_initcall, but should instead be called explicitly at the end of
>>>> dmi_scan_machine().
>>> We cannot be sure that firmware_kobj created at time of dmi_init().
>>> The sources don't oblige you to call it at core level,
>>> for instance like it was done for arm64. For x86, dmi_init() can be
>>> called
>>> before firmware_kobj is created.
>> Looking at the code, it seems that firmware_kobj is created very, very
>> early in the boot process. In do_basic_setup(), you can see that
>> driver_init() (which in turn calls firmware_init(), creating
>> firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
>> defined before dmi_scan_machine() or dmi_init() is called.
>
> No. Not must, rather should. See below.
>
>>
>> Oh, and this wasn't even my point ;-) I'm fine with you checking if
>> firmware_kobj is defined. My question was about the dmi_available check
>> above. But that question was silly anyway, sorry. I confused
>> dmi_available with dmi_initialized. Checking for dmi_available is
>> perfectly reasonable, please scratch my objection.
>>
>>> And if I call it from dmi_init() I suppose
>>> I would face an error. As I can't call it in dmi_init I can't be
>>> sure that
>>> DMI is available at all. So, no, we have to check dmi_available here
>>> and
>>> call it at subsys layer, where it's supposed to be.
>> I can't parse that, I suspect you wrote dmi_init where you actually
>> meant dmi_scan_machine? Given how early firmware_kobj is created, I
>> think the code currently in dmi_init could in fact go at the end of
>> dmi_scan_machine.
>
> Actually, dmi_scan_machine can be called even earlier.
> As I've sad, for x86, it's called before firmware_kobj is created.
>
> kernel_start()
> setup_arch()
> dmi_scan_machine()
>
> And for firmware_init(), as you noticed already:
>
> start_kernel()
> rest_init()
> kernel_init()
> kernel_init_freeable()
> do_basic_setup()
> driver_init()
> firmware_init()
>
> Pay attentions that setup_arch() is called much earlier than rest_init().
> So dmi_init couldn't in fact go at the end of dmi_scan_machine.
>
>> But it's not important for the time being, this can be
>> attempted later.
>>
>>>>> (...)
>>>>> + kobject_del(dmi_kobj);
>>>>> + kobject_put(dmi_kobj);
>>>>> + dmi_kobj = NULL;
>>>> I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
>>>> appropriate comment), so that dmi-sysfs has a chance to load? As it is
>>>> now, a bug or some unexpected behavior in this new code could cause a
>>>> regression for dmi-sysfs users. Just because I don't like dmi_sysfs
>>>> doesn't mean we can break it ;-)
>>> As I remember it was not so critical for you last time.
>>> "I don't care which way you choose". And I've explained my position.
>>> But it's not very hard to me to change it. Anyway patch requires
>>> re-push.
>> You're right, I did not remember we had discussed this already,
>> sorry :-(
>>
>> Well, I still agree that it doesn't really matter, but of two acceptable
>> solutions for an event which will most likely never happen, why not go
>> for the ones with the fewer lines of code? ;-)
>
> Ok.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-17 13:02 ` Jean Delvare
0 siblings, 0 replies; 41+ messages in thread
From: Jean Delvare @ 2015-04-17 13:02 UTC (permalink / raw)
To: Ivan.khoronzhuk
Cc: linux-kernel, matt.fleming, ard.biesheuvel, grant.likely,
linux-api, linux-doc, mikew, dmidecode-devel, leif.lindholm,
msalter, roy.franz
On Thu, 16 Apr 2015 20:27:00 +0300, subscivan wrote:
> On 16.04.15 18:44, Jean Delvare wrote:
> > Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
> >> We cannot be sure that firmware_kobj created at time of dmi_init().
> >> The sources don't oblige you to call it at core level,
> >> for instance like it was done for arm64. For x86, dmi_init() can be called
> >> before firmware_kobj is created.
> > Looking at the code, it seems that firmware_kobj is created very, very
> > early in the boot process. In do_basic_setup(), you can see that
> > driver_init() (which in turn calls firmware_init(), creating
> > firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
> > defined before dmi_scan_machine() or dmi_init() is called.
>
> No. Not must, rather should. See below.
>
> > Oh, and this wasn't even my point ;-) I'm fine with you checking if
> > firmware_kobj is defined. My question was about the dmi_available check
> > above. But that question was silly anyway, sorry. I confused
> > dmi_available with dmi_initialized. Checking for dmi_available is
> > perfectly reasonable, please scratch my objection.
> >
> >> And if I call it from dmi_init() I suppose
> >> I would face an error. As I can't call it in dmi_init I can't be sure that
> >> DMI is available at all. So, no, we have to check dmi_available here and
> >> call it at subsys layer, where it's supposed to be.
> > I can't parse that, I suspect you wrote dmi_init where you actually
> > meant dmi_scan_machine? Given how early firmware_kobj is created, I
> > think the code currently in dmi_init could in fact go at the end of
> > dmi_scan_machine.
>
> Actually, dmi_scan_machine can be called even earlier.
> As I've sad, for x86, it's called before firmware_kobj is created.
>
> kernel_start()
> setup_arch()
> dmi_scan_machine()
>
> And for firmware_init(), as you noticed already:
>
> start_kernel()
> rest_init()
> kernel_init()
> kernel_init_freeable()
> do_basic_setup()
> driver_init()
> firmware_init()
>
> Pay attentions that setup_arch() is called much earlier than rest_init().
> So dmi_init couldn't in fact go at the end of dmi_scan_machine.
Yeah, you're right, sorry. Somehow I thought that setup_arch was an
arch_initcall, but it is not, so I got the order all wrong.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
@ 2015-04-17 13:02 ` Jean Delvare
0 siblings, 0 replies; 41+ messages in thread
From: Jean Delvare @ 2015-04-17 13:02 UTC (permalink / raw)
To: Ivan.khoronzhuk
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
matt.fleming-ral2JQCrhuEAvxtiuMwx3w,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, mikew-hpIqsD4AKlfQT0dZR+AlfA,
dmidecode-devel-qX2TKyscuCcdnm+yROfE0A,
leif.lindholm-QSEj5FYQhm4dnm+yROfE0A,
msalter-H+wXaHxf7aLQT0dZR+AlfA, roy.franz-QSEj5FYQhm4dnm+yROfE0A
On Thu, 16 Apr 2015 20:27:00 +0300, subscivan wrote:
> On 16.04.15 18:44, Jean Delvare wrote:
> > Le Thursday 16 April 2015 à 15:56 +0300, Ivan.khoronzhuk a écrit :
> >> We cannot be sure that firmware_kobj created at time of dmi_init().
> >> The sources don't oblige you to call it at core level,
> >> for instance like it was done for arm64. For x86, dmi_init() can be called
> >> before firmware_kobj is created.
> > Looking at the code, it seems that firmware_kobj is created very, very
> > early in the boot process. In do_basic_setup(), you can see that
> > driver_init() (which in turn calls firmware_init(), creating
> > firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
> > defined before dmi_scan_machine() or dmi_init() is called.
>
> No. Not must, rather should. See below.
>
> > Oh, and this wasn't even my point ;-) I'm fine with you checking if
> > firmware_kobj is defined. My question was about the dmi_available check
> > above. But that question was silly anyway, sorry. I confused
> > dmi_available with dmi_initialized. Checking for dmi_available is
> > perfectly reasonable, please scratch my objection.
> >
> >> And if I call it from dmi_init() I suppose
> >> I would face an error. As I can't call it in dmi_init I can't be sure that
> >> DMI is available at all. So, no, we have to check dmi_available here and
> >> call it at subsys layer, where it's supposed to be.
> > I can't parse that, I suspect you wrote dmi_init where you actually
> > meant dmi_scan_machine? Given how early firmware_kobj is created, I
> > think the code currently in dmi_init could in fact go at the end of
> > dmi_scan_machine.
>
> Actually, dmi_scan_machine can be called even earlier.
> As I've sad, for x86, it's called before firmware_kobj is created.
>
> kernel_start()
> setup_arch()
> dmi_scan_machine()
>
> And for firmware_init(), as you noticed already:
>
> start_kernel()
> rest_init()
> kernel_init()
> kernel_init_freeable()
> do_basic_setup()
> driver_init()
> firmware_init()
>
> Pay attentions that setup_arch() is called much earlier than rest_init().
> So dmi_init couldn't in fact go at the end of dmi_scan_machine.
Yeah, you're right, sorry. Somehow I thought that setup_arch was an
arch_initcall, but it is not, so I got the order all wrong.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 41+ messages in thread