[Patch v2 0/3] firmware: dmi_scan: add some SMBIOSv3 corrections

[Patch v2 0/3] firmware: dmi_scan: add some SMBIOSv3 corrections

[Ivan Khoronzhuk]

This series adds corrections to dmi_scan:
- extends version to be like 3.4.5 format
- fix dmi_len to be 32 bit wide

v1: https://lkml.org/lkml/2015/2/11/124

v2..v1:
  firmware: dmi_scan: fix dmi_len type
  	- now as a first patch in the series
  firmware: dmi_scan: use full dmi version for SMBIOS3
  	- use shifted dmi_ver var for all versions
	- display docrev version for 32 bit 3+ versions like 3.2.x
	- don't display docrev for versions < 3
  firmware: dmi_scan: use direct access to static vars
  	- move as the last patch and leave only corrections

Ivan Khoronzhuk (3):
  firmware: dmi_scan: fix dmi_len type
  firmware: dmi_scan: use full dmi version for SMBIOS3
  firmware: dmi_scan: use direct access to static vars

 drivers/firmware/dmi_scan.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

-- 
1.9.1

[Patch v2 1/3] firmware: dmi_scan: fix dmi_len type

[Ivan Khoronzhuk]

According to SMBIOSv3 specification the length of DMI table can be
up to 32bits wide. So use appropriate type to avoid overflow.

It's obvious that dmi_num theoretically can be more than u16 also,
so it's can be changed to u32 or at least it's better to use int
instead of u16, but on that moment I cannot imagine dmi structure
count more than 65535 and it can require changing type of vars that
work with it. So I didn't correct it.

Acked-by: Ard Biesheuvel <ard@linaro.org>
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 drivers/firmware/dmi_scan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 66fda58..07d2960 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -78,7 +78,7 @@ static const char * __init dmi_string(const struct dmi_header *dm, u8 s)
  *	We have to be cautious here. We have seen BIOSes with DMI pointers
  *	pointing to completely the wrong place for example
  */
-static void dmi_table(u8 *buf, int len, int num,
+static void dmi_table(u8 *buf, u32 len, int num,
 		      void (*decode)(const struct dmi_header *, void *),
 		      void *private_data)
 {
@@ -117,7 +117,7 @@ static void dmi_table(u8 *buf, int len, int num,
 static u8 smbios_header[32];
 static int smbios_header_size;
 static phys_addr_t dmi_base;
-static u16 dmi_len;
+static u32 dmi_len;
 static u16 dmi_num;
 
 static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
-- 
1.9.1

Re: [Patch v2 1/3] firmware: dmi_scan: fix dmi_len type

[Matt Fleming]

On Wed, 18 Feb, at 01:33:19PM, Ivan Khoronzhuk wrote:
> According to SMBIOSv3 specification the length of DMI table can be
> up to 32bits wide. So use appropriate type to avoid overflow.
> 
> It's obvious that dmi_num theoretically can be more than u16 also,
> so it's can be changed to u32 or at least it's better to use int
> instead of u16, but on that moment I cannot imagine dmi structure
> count more than 65535 and it can require changing type of vars that
> work with it. So I didn't correct it.
> 
> Acked-by: Ard Biesheuvel <ard@linaro.org>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  drivers/firmware/dmi_scan.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied to the 'urgent' EFI branch and tagged for stable. Thanks Ivan.

-- 
Matt Fleming, Intel Open Source Technology Center

[Patch v2 2/3] firmware: dmi_scan: use full dmi version for SMBIOS3

[Ivan Khoronzhuk]

New SMBIOS3 spec adds additional field for versioning - docrev.
The docrev identifies the revision of a specification implemented in
the table structures, so display SMBIOSv3 versions in format,
like "3.22.1".

In case of only 32 bit entry point for versions > 3 display
dmi version like "3.22.x" as we don't know the docrev.

In other cases display version like it was.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 drivers/firmware/dmi_scan.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 07d2960..3f3470f 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -17,7 +17,7 @@
  */
 static const char dmi_empty_string[] = "        ";
 
-static u16 __initdata dmi_ver;
+static u32 dmi_ver __initdata;
 /*
  * Catch too early calls to dmi_check_system():
  */
@@ -200,7 +200,7 @@ static void __init dmi_save_uuid(const struct dmi_header *dm, int slot,
 	 * the UUID are supposed to be little-endian encoded.  The specification
 	 * says that this is the defacto standard.
 	 */
-	if (dmi_ver >= 0x0206)
+	if (dmi_ver >= 0x020600)
 		sprintf(s, "%pUL", d);
 	else
 		sprintf(s, "%pUB", d);
@@ -472,7 +472,7 @@ static void __init dmi_format_ids(char *buf, size_t len)
  */
 static int __init dmi_present(const u8 *buf)
 {
-	int smbios_ver;
+	u32 smbios_ver;
 
 	if (memcmp(buf, "_SM_", 4) == 0 &&
 	    buf[5] < 32 && dmi_checksum(buf, buf[5])) {
@@ -507,8 +507,9 @@ static int __init dmi_present(const u8 *buf)
 		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);
+				pr_info("SMBIOS %d.%d%s present.\n",
+					dmi_ver >> 8, dmi_ver & 0xFF,
+					(dmi_ver < 0x0300) ? "" : ".x");
 			} else {
 				smbios_header_size = 15;
 				memcpy(smbios_header, buf, smbios_header_size);
@@ -517,6 +518,7 @@ static int __init dmi_present(const u8 *buf)
 				pr_info("Legacy DMI %d.%d present.\n",
 				       dmi_ver >> 8, dmi_ver & 0xFF);
 			}
+			dmi_ver <<= 8;
 			dmi_format_ids(dmi_ids_string, sizeof(dmi_ids_string));
 			printk(KERN_DEBUG "DMI: %s\n", dmi_ids_string);
 			return 0;
@@ -534,7 +536,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
 {
 	if (memcmp(buf, "_SM3_", 5) == 0 &&
 	    buf[6] < 32 && dmi_checksum(buf, buf[6])) {
-		dmi_ver = get_unaligned_be16(buf + 7);
+		dmi_ver = get_unaligned_be32(buf + 6);
+		dmi_ver &= 0xFFFFFF;
 		dmi_len = get_unaligned_le32(buf + 12);
 		dmi_base = get_unaligned_le64(buf + 16);
 		smbios_header_size = buf[6];
@@ -553,8 +556,9 @@ static int __init dmi_smbios3_present(const u8 *buf)
 		dmi_num = dmi_len / 4;
 
 		if (dmi_walk_early(dmi_decode) == 0) {
-			pr_info("SMBIOS %d.%d present.\n",
-				dmi_ver >> 8, dmi_ver & 0xFF);
+			pr_info("SMBIOS %d.%d.%d present.\n",
+				dmi_ver >> 16, (dmi_ver >> 8) & 0xFF,
+				dmi_ver & 0xFF);
 			dmi_format_ids(dmi_ids_string, sizeof(dmi_ids_string));
 			pr_debug("DMI: %s\n", dmi_ids_string);
 			return 0;
-- 
1.9.1

Re: [Patch v2 2/3] firmware: dmi_scan: use full dmi version for SMBIOS3

[Matt Fleming]

On Wed, 18 Feb, at 01:33:20PM, Ivan Khoronzhuk wrote:
> New SMBIOS3 spec adds additional field for versioning - docrev.
> The docrev identifies the revision of a specification implemented in
> the table structures, so display SMBIOSv3 versions in format,
> like "3.22.1".
> 
> In case of only 32 bit entry point for versions > 3 display
> dmi version like "3.22.x" as we don't know the docrev.
> 
> In other cases display version like it was.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  drivers/firmware/dmi_scan.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)

Applied to the EFI 'next' branch, thanks Ivan.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [Patch v2 2/3] firmware: dmi_scan: use full dmi version for SMBIOS3

[Jean Delvare]

Hi Ivan,

Sorry for the very late review.

On Wed, 18 Feb 2015 13:33:20 +0200, Ivan Khoronzhuk wrote:
> New SMBIOS3 spec adds additional field for versioning - docrev.
> The docrev identifies the revision of a specification implemented in
> the table structures, so display SMBIOSv3 versions in format,
> like "3.22.1".

I'm not sure if that will be terribly useful in practice, but it
certainly can't hurt, so why not.

> In case of only 32 bit entry point for versions > 3 display
> dmi version like "3.22.x" as we don't know the docrev.
> 
> In other cases display version like it was.

I don't like this part, see below.

> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  drivers/firmware/dmi_scan.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 07d2960..3f3470f 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -17,7 +17,7 @@
>   */
>  static const char dmi_empty_string[] = "        ";
>  
> -static u16 __initdata dmi_ver;
> +static u32 dmi_ver __initdata;
>  /*
>   * Catch too early calls to dmi_check_system():
>   */
> @@ -200,7 +200,7 @@ static void __init dmi_save_uuid(const struct dmi_header *dm, int slot,
>  	 * the UUID are supposed to be little-endian encoded.  The specification
>  	 * says that this is the defacto standard.
>  	 */
> -	if (dmi_ver >= 0x0206)
> +	if (dmi_ver >= 0x020600)
>  		sprintf(s, "%pUL", d);
>  	else
>  		sprintf(s, "%pUB", d);
> @@ -472,7 +472,7 @@ static void __init dmi_format_ids(char *buf, size_t len)
>   */
>  static int __init dmi_present(const u8 *buf)
>  {
> -	int smbios_ver;
> +	u32 smbios_ver;

This makes little sense, as the code handles smbios_ver as a 16-bit
value. You should either leave this alone (this change is not needed
AFAICS?), or declare it as a u16. It only makes sense to make it a u32
if you change the code to store 24-bit values in it as you ultimately
do in dmi_ver.

>  
>  	if (memcmp(buf, "_SM_", 4) == 0 &&
>  	    buf[5] < 32 && dmi_checksum(buf, buf[5])) {
> @@ -507,8 +507,9 @@ static int __init dmi_present(const u8 *buf)
>  		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);
> +				pr_info("SMBIOS %d.%d%s present.\n",
> +					dmi_ver >> 8, dmi_ver & 0xFF,
> +					(dmi_ver < 0x0300) ? "" : ".x");

I see zero value in this change. 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.

>  			} else {
>  				smbios_header_size = 15;
>  				memcpy(smbios_header, buf, smbios_header_size);
> @@ -517,6 +518,7 @@ static int __init dmi_present(const u8 *buf)
>  				pr_info("Legacy DMI %d.%d present.\n",
>  				       dmi_ver >> 8, dmi_ver & 0xFF);
>  			}
> +			dmi_ver <<= 8;

I understand this was the easiest way to implement the change, but I'm
not really comfortable with dmi_ver having different formats for
different parts of the code. This is rather error prone for future
changes, even if the code is currently correct.

>  			dmi_format_ids(dmi_ids_string, sizeof(dmi_ids_string));
>  			printk(KERN_DEBUG "DMI: %s\n", dmi_ids_string);
>  			return 0;
> @@ -534,7 +536,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>  {
>  	if (memcmp(buf, "_SM3_", 5) == 0 &&
>  	    buf[6] < 32 && dmi_checksum(buf, buf[6])) {
> -		dmi_ver = get_unaligned_be16(buf + 7);
> +		dmi_ver = get_unaligned_be32(buf + 6);
> +		dmi_ver &= 0xFFFFFF;

That would easily fit on a single line.

>  		dmi_len = get_unaligned_le32(buf + 12);
>  		dmi_base = get_unaligned_le64(buf + 16);
>  		smbios_header_size = buf[6];
> @@ -553,8 +556,9 @@ static int __init dmi_smbios3_present(const u8 *buf)
>  		dmi_num = dmi_len / 4;
>  
>  		if (dmi_walk_early(dmi_decode) == 0) {
> -			pr_info("SMBIOS %d.%d present.\n",
> -				dmi_ver >> 8, dmi_ver & 0xFF);
> +			pr_info("SMBIOS %d.%d.%d present.\n",
> +				dmi_ver >> 16, (dmi_ver >> 8) & 0xFF,
> +				dmi_ver & 0xFF);
>  			dmi_format_ids(dmi_ids_string, sizeof(dmi_ids_string));
>  			pr_debug("DMI: %s\n", dmi_ids_string);
>  			return 0;


-- 
Jean Delvare
SUSE L3 Support

[Patch v2 3/3] firmware: dmi_scan: use direct access to static vars

[Ivan Khoronzhuk]

There is no reason to pass static vars to function that can use
only them.

The dmi_table() can use only dmi_len and dmi_num static vars, so use
them directly. In this case we can freely change their type in one
place and slightly decrease redundancy.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 drivers/firmware/dmi_scan.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 3f3470f..8c065f7 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -18,6 +18,8 @@
 static const char dmi_empty_string[] = "        ";
 
 static u32 dmi_ver __initdata;
+static u32 dmi_len;
+static u16 dmi_num;
 /*
  * Catch too early calls to dmi_check_system():
  */
@@ -78,7 +80,7 @@ static const char * __init dmi_string(const struct dmi_header *dm, u8 s)
  *	We have to be cautious here. We have seen BIOSes with DMI pointers
  *	pointing to completely the wrong place for example
  */
-static void dmi_table(u8 *buf, u32 len, int num,
+static void dmi_table(u8 *buf,
 		      void (*decode)(const struct dmi_header *, void *),
 		      void *private_data)
 {
@@ -89,7 +91,8 @@ static void dmi_table(u8 *buf, u32 len, int num,
 	 *	Stop when we see all the items the table claimed to have
 	 *	OR we run off the end of the table (also happens)
 	 */
-	while ((i < num) && (data - buf + sizeof(struct dmi_header)) <= len) {
+	while ((i < dmi_num) && (data - buf + sizeof(struct dmi_header))
+		<= dmi_len) {
 		const struct dmi_header *dm = (const struct dmi_header *)data;
 
 		/*
@@ -98,9 +101,9 @@ static void dmi_table(u8 *buf, u32 len, int num,
 		 *  table in dmi_decode or dmi_string
 		 */
 		data += dm->length;
-		while ((data - buf < len - 1) && (data[0] || data[1]))
+		while ((data - buf < dmi_len - 1) && (data[0] || data[1]))
 			data++;
-		if (data - buf < len - 1)
+		if (data - buf < dmi_len - 1)
 			decode(dm, private_data);
 
 		/*
@@ -117,8 +120,6 @@ static void dmi_table(u8 *buf, u32 len, int num,
 static u8 smbios_header[32];
 static int smbios_header_size;
 static phys_addr_t dmi_base;
-static u32 dmi_len;
-static u16 dmi_num;
 
 static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
 		void *))
@@ -129,7 +130,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
 	if (buf == NULL)
 		return -1;
 
-	dmi_table(buf, dmi_len, dmi_num, decode, NULL);
+	dmi_table(buf, decode, NULL);
 
 	add_device_randomness(buf, dmi_len);
 
@@ -913,7 +914,7 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	if (buf == NULL)
 		return -1;
 
-	dmi_table(buf, dmi_len, dmi_num, decode, private_data);
+	dmi_table(buf, decode, private_data);
 
 	dmi_unmap(buf);
 	return 0;
-- 
1.9.1

Re: [Patch v2 3/3] firmware: dmi_scan: use direct access to static vars

[Matt Fleming]

On Wed, 18 Feb, at 01:33:21PM, Ivan Khoronzhuk wrote:
> There is no reason to pass static vars to function that can use
> only them.
> 
> The dmi_table() can use only dmi_len and dmi_num static vars, so use
> them directly. In this case we can freely change their type in one
> place and slightly decrease redundancy.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  drivers/firmware/dmi_scan.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)

Applied to the EFI 'next' branch, thanks Ivan.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [Patch v2 3/3] firmware: dmi_scan: use direct access to static vars

[Jean Delvare]

On Wed, 18 Feb 2015 13:33:21 +0200, Ivan Khoronzhuk wrote:
> There is no reason to pass static vars to function that can use
> only them.
> 
> The dmi_table() can use only dmi_len and dmi_num static vars, so use
> them directly. In this case we can freely change their type in one
> place and slightly decrease redundancy.

Too bad we changed dmi_len's type already ;-)

I can see the benefit, and even if this certainly makes dmi_table
slightly slower, no objection from me.

> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  drivers/firmware/dmi_scan.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 3f3470f..8c065f7 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -18,6 +18,8 @@
>  static const char dmi_empty_string[] = "        ";
>  
>  static u32 dmi_ver __initdata;
> +static u32 dmi_len;
> +static u16 dmi_num;

A blank line here wouldn't hurt.

>  /*
>   * Catch too early calls to dmi_check_system():
>   */
> @@ -78,7 +80,7 @@ static const char * __init dmi_string(const struct dmi_header *dm, u8 s)
>   *	We have to be cautious here. We have seen BIOSes with DMI pointers
>   *	pointing to completely the wrong place for example
>   */
> -static void dmi_table(u8 *buf, u32 len, int num,
> +static void dmi_table(u8 *buf,
>  		      void (*decode)(const struct dmi_header *, void *),
>  		      void *private_data)
>  {
> @@ -89,7 +91,8 @@ static void dmi_table(u8 *buf, u32 len, int num,
>  	 *	Stop when we see all the items the table claimed to have
>  	 *	OR we run off the end of the table (also happens)
>  	 */
> -	while ((i < num) && (data - buf + sizeof(struct dmi_header)) <= len) {
> +	while ((i < dmi_num) && (data - buf + sizeof(struct dmi_header))
> +		<= dmi_len) {

One condition per line is easier to read:

	while ((i < dmi_num) &&
	       (data - buf + sizeof(struct dmi_header)) <= dmi_len) {

>  		const struct dmi_header *dm = (const struct dmi_header *)data;
>  
>  		/*
> @@ -98,9 +101,9 @@ static void dmi_table(u8 *buf, u32 len, int num,
>  		 *  table in dmi_decode or dmi_string
>  		 */
>  		data += dm->length;
> -		while ((data - buf < len - 1) && (data[0] || data[1]))
> +		while ((data - buf < dmi_len - 1) && (data[0] || data[1]))
>  			data++;
> -		if (data - buf < len - 1)
> +		if (data - buf < dmi_len - 1)
>  			decode(dm, private_data);
>  
>  		/*
> @@ -117,8 +120,6 @@ static void dmi_table(u8 *buf, u32 len, int num,
>  static u8 smbios_header[32];
>  static int smbios_header_size;
>  static phys_addr_t dmi_base;
> -static u32 dmi_len;
> -static u16 dmi_num;
>  
>  static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>  		void *))
> @@ -129,7 +130,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>  	if (buf == NULL)
>  		return -1;
>  
> -	dmi_table(buf, dmi_len, dmi_num, decode, NULL);
> +	dmi_table(buf, decode, NULL);
>  
>  	add_device_randomness(buf, dmi_len);
>  
> @@ -913,7 +914,7 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>  	if (buf == NULL)
>  		return -1;
>  
> -	dmi_table(buf, dmi_len, dmi_num, decode, private_data);
> +	dmi_table(buf, decode, private_data);
>  
>  	dmi_unmap(buf);
>  	return 0;

-- 
Jean Delvare
SUSE L3 Support