[PATCH 1/2] firmware: dmi_scan: Simplified displayed version

[PATCH 1/2] firmware: dmi_scan: Simplified displayed version

[Jean Delvare]

The trailing .x adds no information for the reader, and if anyone
tries to parse that line, this is more work as they have 3 different
formats to handle instead of 2. Plus, this makes backporting fixes
harder.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Fixes: 95be58df74a5 ("firmware: dmi_scan: Use full dmi version for SMBIOS3")
Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
It doesn't actually "fix" the mentioned commit, as there is no bug, but
if anyone backports dmi-related commits, picking this one will make
his/her life easier.

 drivers/firmware/dmi_scan.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

--- linux-4.0.orig/drivers/firmware/dmi_scan.c	2015-04-17 10:35:56.959512401 +0200
+++ linux-4.0/drivers/firmware/dmi_scan.c	2015-04-17 10:38:02.090156803 +0200
@@ -506,9 +506,8 @@ static int __init dmi_present(const u8 *
 		if (dmi_walk_early(dmi_decode) == 0) {
 			if (smbios_ver) {
 				dmi_ver = smbios_ver;
-				pr_info("SMBIOS %d.%d%s present.\n",
-					dmi_ver >> 8, dmi_ver & 0xFF,
-					(dmi_ver < 0x0300) ? "" : ".x");
+				pr_info("SMBIOS %d.%d present.\n",
+				       dmi_ver >> 8, dmi_ver & 0xFF);
 			} else {
 				dmi_ver = (buf[14] & 0xF0) << 4 |
 					   (buf[14] & 0x0F);


-- 
Jean Delvare
SUSE L3 Support

[PATCH 2/2] firmware: dmi_scan: Fix ordering of product_uuid

[Jean Delvare]

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")
Fixes: 79bae42d51a5 ("dmi_scan: refactor dmi_scan_machine(), {smbios,dmi}_present()")
Acked-by: 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);
 			}

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] firmware: dmi_scan: Simplified displayed version

[subscivan]

Hi, Jean

On 21.04.15 15:45, Jean Delvare wrote:
> The trailing .x adds no information for the reader, and if anyone
> tries to parse that line, this is more work as they have 3 different
> formats to handle instead of 2. Plus, this makes backporting fixes
> harder.
>
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Fixes: 95be58df74a5 ("firmware: dmi_scan: Use full dmi version for SMBIOS3")
> Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> It doesn't actually "fix" the mentioned commit, as there is no bug, but
> if anyone backports dmi-related commits, picking this one will make
> his/her life easier.
>
>   drivers/firmware/dmi_scan.c |    5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> --- linux-4.0.orig/drivers/firmware/dmi_scan.c	2015-04-17 10:35:56.959512401 +0200
> +++ linux-4.0/drivers/firmware/dmi_scan.c	2015-04-17 10:38:02.090156803 +0200
> @@ -506,9 +506,8 @@ static int __init dmi_present(const u8 *
>   		if (dmi_walk_early(dmi_decode) == 0) {
>   			if (smbios_ver) {
>   				dmi_ver = smbios_ver;
> -				pr_info("SMBIOS %d.%d%s present.\n",
> -					dmi_ver >> 8, dmi_ver & 0xFF,
> -					(dmi_ver < 0x0300) ? "" : ".x");
> +				pr_info("SMBIOS %d.%d present.\n",
> +				       dmi_ver >> 8, dmi_ver & 0xFF);
>   			} else {
>   				dmi_ver = (buf[14] & 0xF0) << 4 |
>   					   (buf[14] & 0x0F);
>
>

The main idea here was that dmi version after 3 is in format x.x.x
And after v3 it's expected to see such format. But in case if (I hope that
will never happen) firmware has 32 bit version of SMBIOS3 the table doesn't
have fields to hold revision number, that's why, to warn user about trimming
of revision the .x was added. IMHO the 3.2.x is more informative then 3.2
3.2 can be wrongly interpreted as 3.2.0. If script (or else) needs to see
version in usual way, it can parse tables recently exposed.

But if you insist on 3.2, maybe it be good to warn user in some way like
printing pr_info("SMBIOS doc revision cannot be accessible");

Re: [PATCH 1/2] firmware: dmi_scan: Simplified displayed version

[Ivan.khoronzhuk]

Sorry sent it from wrong address, just don't have access to linaro mailbox,
at least for now.

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH 1/2] firmware: dmi_scan: Simplified displayed version

[Jean Delvare]

Hi Ivan,

On Mon, 27 Apr 2015 19:10:05 +0300, subscivan wrote:
> On 21.04.15 15:45, Jean Delvare wrote:
> > The trailing .x adds no information for the reader, and if anyone
> > tries to parse that line, this is more work as they have 3 different
> > formats to handle instead of 2. Plus, this makes backporting fixes
> > harder.
> >
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Fixes: 95be58df74a5 ("firmware: dmi_scan: Use full dmi version for SMBIOS3")
> > Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> > ---
> > It doesn't actually "fix" the mentioned commit, as there is no bug, but
> > if anyone backports dmi-related commits, picking this one will make
> > his/her life easier.
> >
> >   drivers/firmware/dmi_scan.c |    5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > --- linux-4.0.orig/drivers/firmware/dmi_scan.c	2015-04-17 10:35:56.959512401 +0200
> > +++ linux-4.0/drivers/firmware/dmi_scan.c	2015-04-17 10:38:02.090156803 +0200
> > @@ -506,9 +506,8 @@ static int __init dmi_present(const u8 *
> >   		if (dmi_walk_early(dmi_decode) == 0) {
> >   			if (smbios_ver) {
> >   				dmi_ver = smbios_ver;
> > -				pr_info("SMBIOS %d.%d%s present.\n",
> > -					dmi_ver >> 8, dmi_ver & 0xFF,
> > -					(dmi_ver < 0x0300) ? "" : ".x");
> > +				pr_info("SMBIOS %d.%d present.\n",
> > +				       dmi_ver >> 8, dmi_ver & 0xFF);
> >   			} else {
> >   				dmi_ver = (buf[14] & 0xF0) << 4 |
> >   					   (buf[14] & 0x0F);
> >
> >
> 
> The main idea here was that dmi version after 3 is in format x.x.x
> And after v3 it's expected to see such format. But in case if (I hope that
> will never happen) firmware has 32 bit version of SMBIOS3 the table doesn't

Oh, it will happen. Given that the v3 entry point format is
incompatible with the v2 entry point format, I expect (at least x86)
vendors to provide both whenever possible for several years to come, for
compatibility reasons. Our code scanning the memory for SMBIOS entry
points will pick the first one it finds (both in the kernel and in
dmidecode). I hope that vendors will be smart enough to place the v3
entry point first, but I expect to be disappointed by some.

> have fields to hold revision number, that's why, to warn user about trimming
> of revision the .x was added. IMHO the 3.2.x is more informative then 3.2
> 3.2 can be wrongly interpreted as 3.2.0. If script (or else) needs to see
> version in usual way, it can parse tables recently exposed.

I don't think so. 3.2.x and 3.2 mean exactly the same, none if more
informative than the other. For example if I say "openSUSE 13.2 is
based on kernel 3.16", that doesn't mean exactly kernel version 3.16.0.
Same here.

> But if you insist on 3.2, maybe it be good to warn user in some way like
> printing pr_info("SMBIOS doc revision cannot be accessible");

That would be replacing a bit of over-engineering with another. No,
thanks.

The doc revision number has been omitted so far because the
specification made no room to carry it. People and tools are used to
that. And to be honest I'm surprised they added it in v3. The revision
number is not so interesting IMHO, I never missed it in dmidecode.
Thankfully the additions to the specification are incremental and
almost always backward compatible so we seldom need to make decoding
decisions based on the version. Whenever a significant change happens,
at least the minor version number should be incremented. Bumps of the
doc revision should only translate to new enumerated values and maybe
new fields, all of which can be implemented unconditionally.

I suspect that they added a field for the doc revision number in the v3
entry point simply to avoid a mistake that has happened a couple times
in the past where vendors would attempt to encode the minor version
_and_ the doc revision in the minor version byte. When the SMBIOS 2.3.1
specification was released, a number of vendors encoded the version as
2.31 instead of 2.3. This was the first time the doc revision number
was used AFAIK and apparently some vendors failed to understand how to
handle it. Maybe the DMTF took note back then that, if the entry point
format ever changed, they should include a separate field for the doc
revision number to clear the confusion.

But what I do expect now is the opposite: the doc revision number
doesn't really matter, so I wouldn't be surprised if in the future some
vendors don't set it or forget to bump it on BIOS update. So we can
report it where available but I don't plan to make any use of it.

Anyway, my point here is: let's keep things simple and just report what
is encoded in the entry point. If it's a v3 entry point, the doc
revision is there, print it; if it's a v2 entry point, it's not, don't
print it. Easy as that.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] firmware: dmi_scan: Simplified displayed version

[subscivan]

On 28.04.15 11:15, Jean Delvare wrote:
> Hi Ivan,
>
> On Mon, 27 Apr 2015 19:10:05 +0300, subscivan wrote:
>> On 21.04.15 15:45, Jean Delvare wrote:
>>> The trailing .x adds no information for the reader, and if anyone
>>> tries to parse that line, this is more work as they have 3 different
>>> formats to handle instead of 2. Plus, this makes backporting fixes
>>> harder.
>>>
>>> Signed-off-by: Jean Delvare <jdelvare@suse.de>
>>> Fixes: 95be58df74a5 ("firmware: dmi_scan: Use full dmi version for SMBIOS3")
>>> Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>>> ---
>>> It doesn't actually "fix" the mentioned commit, as there is no bug, but
>>> if anyone backports dmi-related commits, picking this one will make
>>> his/her life easier.
>>>
>>>    drivers/firmware/dmi_scan.c |    5 ++---
>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> --- linux-4.0.orig/drivers/firmware/dmi_scan.c	2015-04-17 10:35:56.959512401 +0200
>>> +++ linux-4.0/drivers/firmware/dmi_scan.c	2015-04-17 10:38:02.090156803 +0200
>>> @@ -506,9 +506,8 @@ static int __init dmi_present(const u8 *
>>>    		if (dmi_walk_early(dmi_decode) == 0) {
>>>    			if (smbios_ver) {
>>>    				dmi_ver = smbios_ver;
>>> -				pr_info("SMBIOS %d.%d%s present.\n",
>>> -					dmi_ver >> 8, dmi_ver & 0xFF,
>>> -					(dmi_ver < 0x0300) ? "" : ".x");
>>> +				pr_info("SMBIOS %d.%d present.\n",
>>> +				       dmi_ver >> 8, dmi_ver & 0xFF);
>>>    			} else {
>>>    				dmi_ver = (buf[14] & 0xF0) << 4 |
>>>    					   (buf[14] & 0x0F);
>>>
>>>
>> The main idea here was that dmi version after 3 is in format x.x.x
>> And after v3 it's expected to see such format. But in case if (I hope that
>> will never happen) firmware has 32 bit version of SMBIOS3 the table doesn't
> Oh, it will happen. Given that the v3 entry point format is
> incompatible with the v2 entry point format, I expect (at least x86)
> vendors to provide both whenever possible for several years to come, for
> compatibility reasons. Our code scanning the memory for SMBIOS entry
> points will pick the first one it finds (both in the kernel and in
> dmidecode). I hope that vendors will be smart enough to place the v3
> entry point first, but I expect to be disappointed by some.
>
>> have fields to hold revision number, that's why, to warn user about trimming
>> of revision the .x was added. IMHO the 3.2.x is more informative then 3.2
>> 3.2 can be wrongly interpreted as 3.2.0. If script (or else) needs to see
>> version in usual way, it can parse tables recently exposed.
> I don't think so. 3.2.x and 3.2 mean exactly the same, none if more
> informative than the other. For example if I say "openSUSE 13.2 is
> based on kernel 3.16", that doesn't mean exactly kernel version 3.16.0.
> Same here.
>
>> But if you insist on 3.2, maybe it be good to warn user in some way like
>> printing pr_info("SMBIOS doc revision cannot be accessible");
> That would be replacing a bit of over-engineering with another. No,
> thanks.
>
> The doc revision number has been omitted so far because the
> specification made no room to carry it. People and tools are used to
> that. And to be honest I'm surprised they added it in v3. The revision
> number is not so interesting IMHO, I never missed it in dmidecode.
> Thankfully the additions to the specification are incremental and
> almost always backward compatible so we seldom need to make decoding
> decisions based on the version. Whenever a significant change happens,
> at least the minor version number should be incremented. Bumps of the
> doc revision should only translate to new enumerated values and maybe
> new fields, all of which can be implemented unconditionally.
>
> I suspect that they added a field for the doc revision number in the v3
> entry point simply to avoid a mistake that has happened a couple times
> in the past where vendors would attempt to encode the minor version
> _and_ the doc revision in the minor version byte. When the SMBIOS 2.3.1
> specification was released, a number of vendors encoded the version as
> 2.31 instead of 2.3. This was the first time the doc revision number
> was used AFAIK and apparently some vendors failed to understand how to
> handle it. Maybe the DMTF took note back then that, if the entry point
> format ever changed, they should include a separate field for the doc
> revision number to clear the confusion.
>
> But what I do expect now is the opposite: the doc revision number
> doesn't really matter, so I wouldn't be surprised if in the future some
> vendors don't set it or forget to bump it on BIOS update. So we can
> report it where available but I don't plan to make any use of it.
>
> Anyway, my point here is: let's keep things simple and just report what
> is encoded in the entry point. If it's a v3 entry point, the doc
> revision is there, print it; if it's a v2 entry point, it's not, don't
> print it. Easy as that.
>

Sorry, but you probably meant if it's a 64-bit version of v3
print it, if it's a 32-bit v3 don't print it. It's no the same as
with v2. In case of v2 it's printed as usual w/o this patch, like "2.3".
.x is added only for 32-bit version of v3.