Messages in this thread Patch in this message |  | | Date | Thu, 16 Apr 2015 14:22:45 +0800 | From | Zhenzhong Duan <> | Subject | Re: [PATCH] firmware: dmi_scan: Fix ordering of product_uuid |
| |
On 2015/4/15 17:02, Jean Delvare wrote: > In function dmi_present(), dmi_walk_early() calls dmi_table(), which > calls dmi_decode(), which ultimately calls dmi_save_uuid(). This last > function makes a decision based on the value of global variable > dmi_ver. The problem is that this variable is set right _after_ > dmi_walk_early() returns. So dmi_save_uuid() always sees dmi_ver == 0 > regardless of the actual version implemented. > > This causes /sys/class/dmi/id/product_uuid to always use the old > ordering even on systems implementing DMI/SMBIOS 2.6 or later, which > should use the new ordering. > > This is broken since kernel v3.8 for legacy DMI implementations and > since kernel v3.10 for SMBIOS 2 implementations. SMBIOS 3 > implementations with the 64-bit entry point are not affected. > > The first breakage does not matter much as in practice legacy DMI > implementations are always for versions older than 2.6, which is when > the UUID ordering changed. The second breakage is more problematic as > it affects the vast majority of x86 systems manufactured since 2009. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Fixes: 9f9c9cbb6057 ("drivers/firmware/dmi_scan.c: fetch dmi version from SMBIOS if it exists") I think above line should be removed as dmi_ver is set before dmi_walk_early with the commit, see below clip. We did get right UUID order with SMBIOS 2.6 per customer test.
+static int __init smbios_present(const char __iomem *p) +{ + u8 buf[32]; + int offset = 0; + + memcpy_fromio(buf, p, 32); + if ((buf[5] < 32) && dmi_checksum(buf, buf[5])) { + dmi_ver = (buf[6] << 8) + buf[7]; > Fixes: 79bae42d51a5 ("dmi_scan: refactor dmi_scan_machine(), {smbios,dmi}_present()") > Cc: Zhenzhong Duan <zhenzhong.duan@oracle.com> > Cc: Ben Hutchings <ben@decadent.org.uk> > Cc: Artem Savkov <artem.savkov@gmail.com> > Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> > Cc: Matt Fleming <matt.fleming@intel.com> > Cc: stable@vger.kernel.org [v3.10+] > --- > drivers/firmware/dmi_scan.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > --- linux-4.0.orig/drivers/firmware/dmi_scan.c 2015-04-13 00:12:50.000000000 +0200 > +++ linux-4.0/drivers/firmware/dmi_scan.c 2015-04-15 10:24:37.556994240 +0200 > @@ -499,18 +499,19 @@ static int __init dmi_present(const u8 * > buf += 16; > > if (memcmp(buf, "_DMI_", 5) == 0 && dmi_checksum(buf, 15)) { > + if (smbios_ver) > + dmi_ver = smbios_ver; > + else > + dmi_ver = (buf[14] & 0xF0) << 4 | (buf[14] & 0x0F); > dmi_num = get_unaligned_le16(buf + 12); > dmi_len = get_unaligned_le16(buf + 6); > dmi_base = get_unaligned_le32(buf + 8); > > if (dmi_walk_early(dmi_decode) == 0) { > if (smbios_ver) { > - dmi_ver = smbios_ver; > pr_info("SMBIOS %d.%d present.\n", > dmi_ver >> 8, dmi_ver & 0xFF); > } else { > - dmi_ver = (buf[14] & 0xF0) << 4 | > - (buf[14] & 0x0F); > pr_info("Legacy DMI %d.%d present.\n", > dmi_ver >> 8, dmi_ver & 0xFF); > } > > The basic idea is right, but you ignore the case dmi_walk_early may fail, though looks impossible when bootup.
Better to add below for robust.
@@ -521,6 +521,6 @@ static int __init dmi_present(const u8 *
return 0; } } + dmi_ver = 0; return 1; } zduan
|  |