Received-SPF: pass (google.com: domain of linux-kernel+bounces-110030-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 21 Mar 2024 13:27:18 +0100
From: Jean Delvare <jdelvare@suse.de>
To: Sean Rhodes <sean@starlabs.systems>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] dmi: Adjust the format of EC versions to match edk2 and
 Windows
Message-ID: <20240321132718.59d1ad36@endymion.delvare>
In-Reply-To: <a2b3d8253c5915619cd70ff884e1b57e4b0bbead.1708700494.git.sean@starlabs.systems>
References: <a2b3d8253c5915619cd70ff884e1b57e4b0bbead.1708700494.git.sean@starlabs.systems>
Organization: SUSE Linux
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

Hi Sean,

On Fri, 23 Feb 2024 15:01:58 +0000, Sean Rhodes wrote:
> Currently, Linux displays the raw bytes for EC firmware versions, which can
> lead to confusion due to formatting mismatches with other platforms like edk2
> and Windows.
> 
> These platforms format EC versions as `%d.%02d`, ensuring that the minor
> version is zero-padded to two digits. This discrepancy becomes particularly
> noticeable with version numbers where the minor version could be
> misinterpreted, such as interpreting `1.02` as `1.2`, which does not clearly
> distinguish it from `1.20`.

Software version strings are not mathematical numbers. The use of a dot
between the major and minor version numbers is only a convention, and
as a matter of fact, versions with more than 2 fields (and thus more
than 1 dot) are frequently used. Therefore, nobody with even basic
knowledge about software should think that versions "1.2" and "1.20"
are the same.

Padding the minor version number with a leading 0 is indeed seen
sometimes, but I can't see a major trend in that direction. This is
essentially a vendor habit, and even then, not all vendors are
consistent in doing or not doing it.

> To align Linux's presentation of EC firmware versions with these platforms
> and to minimize confusion, this commit adjusts the format to `%d.%02d`,
> matching the convention used by edk2 and Windows.

It turns out that the Linux (and dmidecode [1]) representation matches
the SMBIOS specification [2]. See in particular Chapter 7 (Structure
definitions), section 7.1 (BIOS Information (Type 0)), description of
field 17h (Embedded Controller Firmware Minor Release) :

"(...) for example, the value is 16h for revision 10.22 and 01h for
revision 2.1."

As you can see, the example given in the specification itself is
"revision 2.1", not "revision 2.01". So I would question why Linux and
dmidecode would have to align on edk2 and Windows and not the other way
around.

[1] https://savannah.nongnu.org/projects/dmidecode/
[2] https://www.dmtf.org/standards/smbios

Technical review follows for completeness (but I do not intend to
accept the change anyway).

> Cc: Jean Delvare <jdelvare@suse.com>
> Signed-off-by: Sean Rhodes <sean@starlabs.systems>
> ---
>  drivers/firmware/dmi-id.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> index 5f3a3e913d28..5bb921c4f62d 100644
> --- a/drivers/firmware/dmi-id.c
> +++ b/drivers/firmware/dmi-id.c
> @@ -114,12 +114,20 @@ static ssize_t get_modalias(char *buffer, size_t buffer_size)
>  		if (!c)
>  			continue;
>  
> -		t = kmalloc(strlen(c) + 1, GFP_KERNEL);
> -		if (!t)
> -			break;
> -		ascii_filter(t, c);
> -		l = scnprintf(p, left, ":%s%s", f->prefix, t);
> -		kfree(t);
> +		if (f->field == DMI_EC_FIRMWARE_RELEASE) {
> +			int major, minor;
> +			if (sscanf(c, "%d.%d", &major, &minor) == 2)
> +				l = scnprintf(p, left, ":%s%d.%02d", f->prefix, major, minor);
> +			else
> +				l = scnprintf(p, left, ":%s%s", f->prefix, c);
> +		} else {
> +			t = kmalloc(strlen(c) + 1, GFP_KERNEL);
> +			if (!t)
> +				break;
> +			ascii_filter(t, c);
> +			l = scnprintf(p, left, ":%s%s", f->prefix, t);
> +			kfree(t);
> +		}
>  
>  		p += l;
>  		left -= l;

The implementation is technically poor. Having to resort to scanf to
parse a string which we have encoded ourselves from fields which
contained exactly the numbers we need, is quite inefficient.

Furthermore, it adds more inconsistency than it intends to solve, as it
only enforces the padding of the minor EC firmware version in the
modalias string. This would make this field inconsistent with the
ec_firmware_release sysfs attribute, and would also create inconsistency
with the and the "br" modalias field (BIOS release) which does not get
the same zero-padding.

-- 
Jean Delvare
SUSE L3 Support