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

Hi Ivan,

On Thu, 19 Mar 2015 19:35:34 +0200, Ivan.khoronzhuk wrote:

On 19.03.15 17:30, Jean Delvare wrote:

quoted

Le Monday 16 March 2015 à 22:57 +0200, Ivan Khoronzhuk a écrit :

quoted

Some utils, like dmidecode and smbios, need to access SMBIOS entry
table area in order to get information like SMBIOS version, size, etc.
Currently it's done via /dev/mem. But for situation when /dev/mem
usage is disabled, the utils have to use dmi sysfs instead, which
doesn't represent SMBIOS entry and adds code/delay redundancy when direct
access for table is needed.

So this patch creates dmi subsystem and adds SMBIOS entry point to allow
utils in question to work correctly without /dev/mem. Also patch adds
raw dmi table to simplify dmi table processing in user space, as were
proposed by Jean Delvare.

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

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

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

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

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

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

quoted

quoted

(...)

@@ -118,6 +124,7 @@ static void dmi_table(u8 *buf,
  }
  
  static phys_addr_t dmi_base;
+static u8 *dmi_tb;

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

If others are OK, I'll better rename dmi_table function to 
dmi_decode_table()
(or maybe dmidecode_table :), joke)
as it's more appropriate to what it's doing.
And accordingly dmi_tb to dmi_table.

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

quoted

quoted

(...)
  static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
  		void *))

@@ -476,6 +483,8 @@ static int __init dmi_present(const u8 *buf)
  	if (memcmp(buf, "_SM_", 4) == 0 &&
  	    buf[5] < 32 && dmi_checksum(buf, buf[5])) {
  		smbios_ver = get_unaligned_be16(buf + 6);
+		smbios_entry_point_size = buf[5];
+		memcpy(smbios_entry_point, buf, smbios_entry_point_size);
  
  		/* Some BIOS report weird SMBIOS version, fix that up */
  		switch (smbios_ver) {

@@ -508,6 +517,8 @@ static int __init dmi_present(const u8 *buf)
  					dmi_ver >> 8, dmi_ver & 0xFF,
  					(dmi_ver < 0x0300) ? "" : ".x");
  			} else {
+				smbios_entry_point_size = 15;
+				memcpy(smbios_entry_point, buf, 15);
  				dmi_ver = (buf[14] & 0xF0) << 4 |
  					   (buf[14] & 0x0F);
  				pr_info("Legacy DMI %d.%d present.\n",

@@ -535,6 +546,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
  		dmi_ver &= 0xFFFFFF;
  		dmi_len = get_unaligned_le32(buf + 12);
  		dmi_base = get_unaligned_le64(buf + 16);
+		smbios_entry_point_size = buf[6];
+		memcpy(smbios_entry_point, buf, smbios_entry_point_size);
  
  		/*
  		 * The 64-bit SMBIOS 3.0 entry point no longer has a field

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

You probably meant "memcpy(smbios_entry_point, buf, 15)"

Yes.

Just wanted to avoid redundant line break.

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

quoted

quoted

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

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

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

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

quoted

quoted

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

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

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

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

quoted

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

quoted

quoted

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

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

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

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

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

quoted

quoted

(...)
+	kobject_del(dmi_kobj);
+	kobject_put(dmi_kobj);

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

Yes.

quoted

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

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

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

Thanks,
-- 
Jean Delvare
SUSE L3 Support