Date: Tue, 28 Apr 2015 10:15:46 +0200
From: Jean Delvare <jdelvare@suse.de>
To: "Ivan.khoronzhuk" <ivan.khoronzhuk@globallogic.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
        Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Subject: Re: [PATCH 1/2] firmware: dmi_scan: Simplified displayed version
Message-ID: <20150428101546.3a27a119@endymion.delvare>
In-Reply-To: <553E5F5D.60302@gmail.com>
References: <20150421144555.35e2828d@endymion.delvare>
	<553E5F5D.60302@gmail.com>
Organization: SUSE Linux
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5032
Lines: 105

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/