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
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 <[email protected]>
Signed-off-by: Ivan Khoronzhuk <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Ivan Khoronzhuk <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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