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