[PATCH 1/3] firmware/dmi_scan: Add dmi_save_release to save releases fields

[PATCH 1/3] firmware/dmi_scan: Add dmi_save_release to save releases fields

[Erwan Velu]

In DMI type 0, there is several fields that encodes a release.
The dmi_save_release() function have the logic to check if the field is valid.
If so, it reports the actual value.

Signed-off-by: Erwan Velu <e.velu@criteo.com>
---
 drivers/firmware/dmi_scan.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 35ed56b9c34f..202bd2c69d0f 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -181,6 +181,32 @@ static void __init dmi_save_ident(const struct dmi_header *dm, int slot,
 	dmi_ident[slot] = p;
 }
 
+static void __init dmi_save_release(const struct dmi_header *dm, int slot,
+		int index)
+{
+	const u8 *d;
+	char *s;
+
+	// If the table doesn't have the field, let's return
+	if (dmi_ident[slot] || dm->length < index)
+		return;
+
+	d = (u8 *) dm + index;
+
+	// As per the specification,
+	// if the system doesn't have the field, the value is FF
+	if (d[0] == 0xFF)
+		return;
+
+	s = dmi_alloc(4);
+	if (!s)
+		return;
+
+	sprintf(s, "%u", d[0]);
+
+	dmi_ident[slot] = s;
+}
+
 static void __init dmi_save_uuid(const struct dmi_header *dm, int slot,
 		int index)
 {
-- 
2.21.0

[PATCH 2/3] firmware/dmi: Report DMI Bios release

[Erwan Velu]

Some vendors like HPe or Dell, encodes the release version of their BIOS
in the "System BIOS {Major|Minor} Release" fields of Type 0.

This information is useful to know which release of the bios is actually running.
It could be used for some quirks, debugging sessions or inventory tasks.

This patch extract these 2 fields in DMI_BIOS_MAJOR_RELEASE & DMI_BIOS_MINOR_RELEASE.

A typical output for a Dell system running the 65.27 bios is :

	[root@t1700 ~]# cat /sys/devices/virtual/dmi/id/bios_release_major
	65
	[root@t1700 ~]# cat /sys/devices/virtual/dmi/id/bios_release_minor
	27
	[root@t1700 ~]#

Signed-off-by: Erwan Velu <e.velu@criteo.com>
---
 drivers/firmware/dmi-id.c       | 6 ++++++
 drivers/firmware/dmi_scan.c     | 2 ++
 include/linux/mod_devicetable.h | 2 ++
 scripts/mod/file2alias.c        | 2 ++
 4 files changed, 12 insertions(+)

diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
index ff39f64f2aae..3248c2837a4d 100644
--- a/drivers/firmware/dmi-id.c
+++ b/drivers/firmware/dmi-id.c
@@ -42,6 +42,8 @@ DEFINE_DMI_ATTR_WITH_SHOW(bios_vendor,		0444, DMI_BIOS_VENDOR);
 DEFINE_DMI_ATTR_WITH_SHOW(bios_version,		0444, DMI_BIOS_VERSION);
 DEFINE_DMI_ATTR_WITH_SHOW(bios_date,		0444, DMI_BIOS_DATE);
 DEFINE_DMI_ATTR_WITH_SHOW(sys_vendor,		0444, DMI_SYS_VENDOR);
+DEFINE_DMI_ATTR_WITH_SHOW(bios_release_major, 0444, DMI_BIOS_MAJOR_RELEASE);
+DEFINE_DMI_ATTR_WITH_SHOW(bios_release_minor, 0444, DMI_BIOS_MINOR_RELEASE);
 DEFINE_DMI_ATTR_WITH_SHOW(product_name,		0444, DMI_PRODUCT_NAME);
 DEFINE_DMI_ATTR_WITH_SHOW(product_version,	0444, DMI_PRODUCT_VERSION);
 DEFINE_DMI_ATTR_WITH_SHOW(product_serial,	0400, DMI_PRODUCT_SERIAL);
@@ -78,6 +80,8 @@ static ssize_t get_modalias(char *buffer, size_t buffer_size)
 		{ "bvn", DMI_BIOS_VENDOR },
 		{ "bvr", DMI_BIOS_VERSION },
 		{ "bd",  DMI_BIOS_DATE },
+		{ "bjr", DMI_BIOS_MAJOR_RELEASE },
+		{ "bmr", DMI_BIOS_MINOR_RELEASE },
 		{ "svn", DMI_SYS_VENDOR },
 		{ "pn",  DMI_PRODUCT_NAME },
 		{ "pvr", DMI_PRODUCT_VERSION },
@@ -187,6 +191,8 @@ static void __init dmi_id_init_attr_table(void)
 	ADD_DMI_ATTR(bios_vendor,       DMI_BIOS_VENDOR);
 	ADD_DMI_ATTR(bios_version,      DMI_BIOS_VERSION);
 	ADD_DMI_ATTR(bios_date,         DMI_BIOS_DATE);
+	ADD_DMI_ATTR(bios_release_major, DMI_BIOS_MAJOR_RELEASE);
+	ADD_DMI_ATTR(bios_release_minor, DMI_BIOS_MINOR_RELEASE);
 	ADD_DMI_ATTR(sys_vendor,        DMI_SYS_VENDOR);
 	ADD_DMI_ATTR(product_name,      DMI_PRODUCT_NAME);
 	ADD_DMI_ATTR(product_version,   DMI_PRODUCT_VERSION);
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 202bd2c69d0f..886ace54e527 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -464,6 +464,8 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
 		dmi_save_ident(dm, DMI_BIOS_VENDOR, 4);
 		dmi_save_ident(dm, DMI_BIOS_VERSION, 5);
 		dmi_save_ident(dm, DMI_BIOS_DATE, 8);
+		dmi_save_release(dm, DMI_BIOS_MAJOR_RELEASE, 20);
+		dmi_save_release(dm, DMI_BIOS_MINOR_RELEASE, 21);
 		break;
 	case 1:		/* System Information */
 		dmi_save_ident(dm, DMI_SYS_VENDOR, 4);
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5714fd35a83c..2471de601bd6 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -532,6 +532,8 @@ enum dmi_field {
 	DMI_BIOS_VENDOR,
 	DMI_BIOS_VERSION,
 	DMI_BIOS_DATE,
+	DMI_BIOS_MAJOR_RELEASE,
+	DMI_BIOS_MINOR_RELEASE,
 	DMI_SYS_VENDOR,
 	DMI_PRODUCT_NAME,
 	DMI_PRODUCT_VERSION,
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index e17a29ae2e97..1b4f9bc3b06c 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -931,6 +931,8 @@ static const struct dmifield {
 	{ "bvn", DMI_BIOS_VENDOR },
 	{ "bvr", DMI_BIOS_VERSION },
 	{ "bd",  DMI_BIOS_DATE },
+	{ "bjr", DMI_BIOS_MAJOR_RELEASE },
+	{ "bmr", DMI_BIOS_MINOR_RELEASE },
 	{ "svn", DMI_SYS_VENDOR },
 	{ "pn",  DMI_PRODUCT_NAME },
 	{ "pvr", DMI_PRODUCT_VERSION },
-- 
2.21.0

Re: [PATCH 2/3] firmware/dmi: Report DMI Bios release

[Jean Delvare]

On Wed, 18 Sep 2019 11:43:20 +0200, Erwan Velu wrote:
> Some vendors like HPe or Dell, encodes the release version of their BIOS

encodes -> encode

> in the "System BIOS {Major|Minor} Release" fields of Type 0.
> 
> This information is useful to know which release of the bios is actually running.
> It could be used for some quirks, debugging sessions or inventory tasks.
> 
> This patch extract these 2 fields in DMI_BIOS_MAJOR_RELEASE & DMI_BIOS_MINOR_RELEASE.
> 
> A typical output for a Dell system running the 65.27 bios is :
> 
> 	[root@t1700 ~]# cat /sys/devices/virtual/dmi/id/bios_release_major
> 	65
> 	[root@t1700 ~]# cat /sys/devices/virtual/dmi/id/bios_release_minor
> 	27
> 	[root@t1700 ~]#

I don't think we want two fields. This adds quite some overhead, and
they are not independent from each other anyway. I'd rather have one
field with the values combined:

	[root@t1700 ~]# cat /sys/devices/virtual/dmi/id/bios_release
	65.27
	[root@t1700 ~]#

This would also be in line with how it was implemented in dmidecode. Is
there any reason to NOT go that route?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/3] firmware/dmi: Report DMI Bios release

[Erwan Velu]

On 21/10/2019 16:53, Jean Delvare wrote:
> This would also be in line with how it was implemented in dmidecode. Is
> there any reason to NOT go that route?

The main reason was I though it would be easier to compare releases by 
splitting major & minor.

That is probably overkill, so I'll stick to the usual way to represent a 
release in V2.

Thanks for the review,

Erwan,

[PATCH 3/3] firmware/dmi: Report DMI Embedded Firmware release

[Erwan Velu]

Servers that have a BMC encodes the release version of their firmware
in the "Embedded Controller Firmware {Major|Minor} Release" fields of Type 0.

This information is useful to know which release of the BMC is actually running.
It could be used for some quirks, debugging sessions or inventory tasks.

This patch extract these 2 fields in DMI_EMBEDDED_FW_MAJOR_RELEASE & DMI_EMBEDDED_FW_MINOR_RELEASE

A typical output for a Dell system running the 3.75 bios is :

    [root@t1700 ~]# cat /sys/devices/virtual/dmi/id/fw_release_major
    3
    [root@t1700 ~]# cat /sys/devices/virtual/dmi/id/fw_release_minor
    75
    [root@t1700 ~]#

Signed-off-by: Erwan Velu <e.velu@criteo.com>
---
 drivers/firmware/dmi-id.c       | 10 ++++++++--
 drivers/firmware/dmi_scan.c     |  2 ++
 include/linux/mod_devicetable.h |  2 ++
 scripts/mod/file2alias.c        |  2 ++
 4 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
index 3248c2837a4d..5262626bf9f1 100644
--- a/drivers/firmware/dmi-id.c
+++ b/drivers/firmware/dmi-id.c
@@ -42,8 +42,10 @@ DEFINE_DMI_ATTR_WITH_SHOW(bios_vendor,		0444, DMI_BIOS_VENDOR);
 DEFINE_DMI_ATTR_WITH_SHOW(bios_version,		0444, DMI_BIOS_VERSION);
 DEFINE_DMI_ATTR_WITH_SHOW(bios_date,		0444, DMI_BIOS_DATE);
 DEFINE_DMI_ATTR_WITH_SHOW(sys_vendor,		0444, DMI_SYS_VENDOR);
-DEFINE_DMI_ATTR_WITH_SHOW(bios_release_major, 0444, DMI_BIOS_MAJOR_RELEASE);
-DEFINE_DMI_ATTR_WITH_SHOW(bios_release_minor, 0444, DMI_BIOS_MINOR_RELEASE);
+DEFINE_DMI_ATTR_WITH_SHOW(bios_release_major,	0444, DMI_BIOS_MAJOR_RELEASE);
+DEFINE_DMI_ATTR_WITH_SHOW(bios_release_minor,	0444, DMI_BIOS_MINOR_RELEASE);
+DEFINE_DMI_ATTR_WITH_SHOW(fw_release_major,	0444, DMI_EMBEDDED_FW_MAJOR_RELEASE);
+DEFINE_DMI_ATTR_WITH_SHOW(fw_release_minor,	0444, DMI_EMBEDDED_FW_MINOR_RELEASE);
 DEFINE_DMI_ATTR_WITH_SHOW(product_name,		0444, DMI_PRODUCT_NAME);
 DEFINE_DMI_ATTR_WITH_SHOW(product_version,	0444, DMI_PRODUCT_VERSION);
 DEFINE_DMI_ATTR_WITH_SHOW(product_serial,	0400, DMI_PRODUCT_SERIAL);
@@ -82,6 +84,8 @@ static ssize_t get_modalias(char *buffer, size_t buffer_size)
 		{ "bd",  DMI_BIOS_DATE },
 		{ "bjr", DMI_BIOS_MAJOR_RELEASE },
 		{ "bmr", DMI_BIOS_MINOR_RELEASE },
+		{ "efj", DMI_EMBEDDED_FW_MAJOR_RELEASE },
+		{ "efm", DMI_EMBEDDED_FW_MINOR_RELEASE },
 		{ "svn", DMI_SYS_VENDOR },
 		{ "pn",  DMI_PRODUCT_NAME },
 		{ "pvr", DMI_PRODUCT_VERSION },
@@ -193,6 +197,8 @@ static void __init dmi_id_init_attr_table(void)
 	ADD_DMI_ATTR(bios_date,         DMI_BIOS_DATE);
 	ADD_DMI_ATTR(bios_release_major, DMI_BIOS_MAJOR_RELEASE);
 	ADD_DMI_ATTR(bios_release_minor, DMI_BIOS_MINOR_RELEASE);
+	ADD_DMI_ATTR(fw_release_major,  DMI_EMBEDDED_FW_MAJOR_RELEASE);
+	ADD_DMI_ATTR(fw_release_minor,  DMI_EMBEDDED_FW_MINOR_RELEASE);
 	ADD_DMI_ATTR(sys_vendor,        DMI_SYS_VENDOR);
 	ADD_DMI_ATTR(product_name,      DMI_PRODUCT_NAME);
 	ADD_DMI_ATTR(product_version,   DMI_PRODUCT_VERSION);
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 886ace54e527..3beec6896a58 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -466,6 +466,8 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
 		dmi_save_ident(dm, DMI_BIOS_DATE, 8);
 		dmi_save_release(dm, DMI_BIOS_MAJOR_RELEASE, 20);
 		dmi_save_release(dm, DMI_BIOS_MINOR_RELEASE, 21);
+		dmi_save_release(dm, DMI_EMBEDDED_FW_MAJOR_RELEASE, 22);
+		dmi_save_release(dm, DMI_EMBEDDED_FW_MINOR_RELEASE, 23);
 		break;
 	case 1:		/* System Information */
 		dmi_save_ident(dm, DMI_SYS_VENDOR, 4);
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 2471de601bd6..e6482fd94bfd 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -534,6 +534,8 @@ enum dmi_field {
 	DMI_BIOS_DATE,
 	DMI_BIOS_MAJOR_RELEASE,
 	DMI_BIOS_MINOR_RELEASE,
+	DMI_EMBEDDED_FW_MAJOR_RELEASE,
+	DMI_EMBEDDED_FW_MINOR_RELEASE,
 	DMI_SYS_VENDOR,
 	DMI_PRODUCT_NAME,
 	DMI_PRODUCT_VERSION,
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 1b4f9bc3b06c..ce03040271cd 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -933,6 +933,8 @@ static const struct dmifield {
 	{ "bd",  DMI_BIOS_DATE },
 	{ "bjr", DMI_BIOS_MAJOR_RELEASE },
 	{ "bmr", DMI_BIOS_MINOR_RELEASE },
+	{ "efj", DMI_EMBEDDED_FW_MAJOR_RELEASE },
+	{ "efm", DMI_EMBEDDED_FW_MINOR_RELEASE },
 	{ "svn", DMI_SYS_VENDOR },
 	{ "pn",  DMI_PRODUCT_NAME },
 	{ "pvr", DMI_PRODUCT_VERSION },
-- 
2.21.0

Re: [PATCH 3/3] firmware/dmi: Report DMI Embedded Firmware release

[Jean Delvare]

On Wed, 18 Sep 2019 11:43:21 +0200, Erwan Velu wrote:
> Servers that have a BMC encodes the release version of their firmware
> in the "Embedded Controller Firmware {Major|Minor} Release" fields of Type 0.
> 
> This information is useful to know which release of the BMC is actually running.
> It could be used for some quirks, debugging sessions or inventory tasks.
> 
> This patch extract these 2 fields in DMI_EMBEDDED_FW_MAJOR_RELEASE & DMI_EMBEDDED_FW_MINOR_RELEASE
> 
> A typical output for a Dell system running the 3.75 bios is :
> 
>     [root@t1700 ~]# cat /sys/devices/virtual/dmi/id/fw_release_major
>     3
>     [root@t1700 ~]# cat /sys/devices/virtual/dmi/id/fw_release_minor
>     75
>     [root@t1700 ~]#

Same comment here as for previous patch, obviously.

Additionally, please run scripts/checkpatch.pl on your patch before you
resubmit, and address all the problems reported.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/3] firmware/dmi_scan: Add dmi_save_release to save releases fields

[Jean Delvare]

Hi Erwan,

Sorry for the late answer.

On Wed, 18 Sep 2019 11:43:19 +0200, Erwan Velu wrote:
> In DMI type 0, there is several fields that encodes a release.

is -> are
encodes -> encode

> The dmi_save_release() function have the logic to check if the field is valid.
> If so, it reports the actual value.
> 
> Signed-off-by: Erwan Velu <e.velu@criteo.com>
> ---
>  drivers/firmware/dmi_scan.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)

This patch introduces a warning:

drivers/firmware/dmi_scan.c:185:20: warning: ‘dmi_save_release’ defined but not used [-Wunused-function]
 static void __init dmi_save_release(const struct dmi_header *dm, int slot,
                    ^~~~~~~~~~~~~~~~

because you add a static function with no user. I understand that you
add a use later in the series, but it's not OK to introduce a warning
even if temporary. It also makes little sense to split the changes that
way as there is no way to cherry-pick one of the patches without the
rest. And it makes things more difficult to review too, as I can't
possibly judge if this function if right without also seeing where is
will be called and how.

So, please merge all the changes into a single patch.

> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 35ed56b9c34f..202bd2c69d0f 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -181,6 +181,32 @@ static void __init dmi_save_ident(const struct dmi_header *dm, int slot,
>  	dmi_ident[slot] = p;
>  }
>  
> +static void __init dmi_save_release(const struct dmi_header *dm, int slot,
> +		int index)
> +{
> +	const u8 *d;
> +	char *s;
> +
> +	// If the table doesn't have the field, let's return

Please stick to C89-style comments (/* */) as used everywhere else in
this file.

> +	if (dmi_ident[slot] || dm->length < index)
> +		return;
> +
> +	d = (u8 *) dm + index;
> +
> +	// As per the specification,
> +	// if the system doesn't have the field, the value is FF
> +	if (d[0] == 0xFF)
> +		return;

That's not exactly what the specification says. It says:

"If the system does not support the use of [the System BIOS Major
Release] field, the value is 0FFh for both this field and the System
BIOS Minor Release field." So unused is when both fields are 0xFF. You
can't test them independently.

Same goes for the Embedded Controller Firmware Release fields, even if
it is worded differently, the logic is the same.

> +
> +	s = dmi_alloc(4);
> +	if (!s)
> +		return;
> +
> +	sprintf(s, "%u", d[0]);
> +
> +	dmi_ident[slot] = s;
> +}
> +
>  static void __init dmi_save_uuid(const struct dmi_header *dm, int slot,
>  		int index)
>  {


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/3] firmware/dmi_scan: Add dmi_save_release to save releases fields

[Erwan Velu]

Got all your points, sending a V2 with the fixes you asked.

On 21/10/2019 16:32, Jean Delvare wrote:
> Hi Erwan,
>
> Sorry for the late answer.
>
> On Wed, 18 Sep 2019 11:43:19 +0200, Erwan Velu wrote:
>> In DMI type 0, there is several fields that encodes a release.
> is -> are
> encodes -> encode
>
>> The dmi_save_release() function have the logic to check if the field is valid.
>> If so, it reports the actual value.
>>
>> Signed-off-by: Erwan Velu <e.velu@criteo.com>
>> ---
>>   drivers/firmware/dmi_scan.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
> This patch introduces a warning:
>
> drivers/firmware/dmi_scan.c:185:20: warning: ‘dmi_save_release’ defined but not used [-Wunused-function]
>   static void __init dmi_save_release(const struct dmi_header *dm, int slot,
>                      ^~~~~~~~~~~~~~~~
>
> because you add a static function with no user. I understand that you
> add a use later in the series, but it's not OK to introduce a warning
> even if temporary. It also makes little sense to split the changes that
> way as there is no way to cherry-pick one of the patches without the
> rest. And it makes things more difficult to review too, as I can't
> possibly judge if this function if right without also seeing where is
> will be called and how.
>
> So, please merge all the changes into a single patch.
>
>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>> index 35ed56b9c34f..202bd2c69d0f 100644
>> --- a/drivers/firmware/dmi_scan.c
>> +++ b/drivers/firmware/dmi_scan.c
>> @@ -181,6 +181,32 @@ static void __init dmi_save_ident(const struct dmi_header *dm, int slot,
>>   	dmi_ident[slot] = p;
>>   }
>>   
>> +static void __init dmi_save_release(const struct dmi_header *dm, int slot,
>> +		int index)
>> +{
>> +	const u8 *d;
>> +	char *s;
>> +
>> +	// If the table doesn't have the field, let's return
> Please stick to C89-style comments (/* */) as used everywhere else in
> this file.
>
>> +	if (dmi_ident[slot] || dm->length < index)
>> +		return;
>> +
>> +	d = (u8 *) dm + index;
>> +
>> +	// As per the specification,
>> +	// if the system doesn't have the field, the value is FF
>> +	if (d[0] == 0xFF)
>> +		return;
> That's not exactly what the specification says. It says:
>
> "If the system does not support the use of [the System BIOS Major
> Release] field, the value is 0FFh for both this field and the System
> BIOS Minor Release field." So unused is when both fields are 0xFF. You
> can't test them independently.
>
> Same goes for the Embedded Controller Firmware Release fields, even if
> it is worded differently, the logic is the same.
>
>> +
>> +	s = dmi_alloc(4);
>> +	if (!s)
>> +		return;
>> +
>> +	sprintf(s, "%u", d[0]);
>> +
>> +	dmi_ident[slot] = s;
>> +}
>> +
>>   static void __init dmi_save_uuid(const struct dmi_header *dm, int slot,
>>   		int index)
>>   {
>

[PATCH 1/2] firmware/dmi: Report DMI Bios release

[Erwan Velu]

Some vendors like HPe or Dell, encode the release version of their BIOS
in the "System BIOS {Major|Minor} Release" fields of Type 0.

This information is used to know which bios release actually runs.
It could be used for some quirks, debugging sessions or inventory tasks.

A typical output for a Dell system running the 65.27 bios is :

	[root@t1700 ~]# cat /sys/devices/virtual/dmi/id/bios_release
	65.27
	[root@t1700 ~]#

This commit add  dmi_save_release() function have the logic to
check if the field is valid. If so, it reports the actual value.

Signed-off-by: Erwan Velu <e.velu@criteo.com>
---
 drivers/firmware/dmi-id.c       |  3 +++
 drivers/firmware/dmi_scan.c     | 29 +++++++++++++++++++++++++++++
 include/linux/mod_devicetable.h |  1 +
 scripts/mod/file2alias.c        |  1 +
 4 files changed, 34 insertions(+)

diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
index ff39f64f2aae..a2aac65ff771 100644
--- a/drivers/firmware/dmi-id.c
+++ b/drivers/firmware/dmi-id.c
@@ -42,6 +42,7 @@ DEFINE_DMI_ATTR_WITH_SHOW(bios_vendor,		0444, DMI_BIOS_VENDOR);
 DEFINE_DMI_ATTR_WITH_SHOW(bios_version,		0444, DMI_BIOS_VERSION);
 DEFINE_DMI_ATTR_WITH_SHOW(bios_date,		0444, DMI_BIOS_DATE);
 DEFINE_DMI_ATTR_WITH_SHOW(sys_vendor,		0444, DMI_SYS_VENDOR);
+DEFINE_DMI_ATTR_WITH_SHOW(bios_release,         0444, DMI_BIOS_RELEASE);
 DEFINE_DMI_ATTR_WITH_SHOW(product_name,		0444, DMI_PRODUCT_NAME);
 DEFINE_DMI_ATTR_WITH_SHOW(product_version,	0444, DMI_PRODUCT_VERSION);
 DEFINE_DMI_ATTR_WITH_SHOW(product_serial,	0400, DMI_PRODUCT_SERIAL);
@@ -78,6 +79,7 @@ static ssize_t get_modalias(char *buffer, size_t buffer_size)
 		{ "bvn", DMI_BIOS_VENDOR },
 		{ "bvr", DMI_BIOS_VERSION },
 		{ "bd",  DMI_BIOS_DATE },
+		{ "br",  DMI_BIOS_RELEASE },
 		{ "svn", DMI_SYS_VENDOR },
 		{ "pn",  DMI_PRODUCT_NAME },
 		{ "pvr", DMI_PRODUCT_VERSION },
@@ -187,6 +189,7 @@ static void __init dmi_id_init_attr_table(void)
 	ADD_DMI_ATTR(bios_vendor,       DMI_BIOS_VENDOR);
 	ADD_DMI_ATTR(bios_version,      DMI_BIOS_VERSION);
 	ADD_DMI_ATTR(bios_date,         DMI_BIOS_DATE);
+	ADD_DMI_ATTR(bios_release,      DMI_BIOS_RELEASE);
 	ADD_DMI_ATTR(sys_vendor,        DMI_SYS_VENDOR);
 	ADD_DMI_ATTR(product_name,      DMI_PRODUCT_NAME);
 	ADD_DMI_ATTR(product_version,   DMI_PRODUCT_VERSION);
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 1e21fc3e9851..d010c915c1ab 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -181,6 +181,34 @@ static void __init dmi_save_ident(const struct dmi_header *dm, int slot,
 	dmi_ident[slot] = p;
 }
 
+static void __init dmi_save_release(const struct dmi_header *dm, int slot,
+		int index)
+{
+	const u8 *minor, *major;
+	char *s;
+
+	/* If the table doesn't have the field, let's return */
+	if (dmi_ident[slot] || dm->length < index)
+		return;
+
+	minor = (u8 *) dm + index;
+	major = (u8 *) dm + index - 1;
+
+	/* As per the spec, if the system doesn't support this field,
+	 * the value is FF
+	 */
+	if (major[0] == 0xFF && minor[0] == 0xFF)
+		return;
+
+	s = dmi_alloc(4);
+	if (!s)
+		return;
+
+	sprintf(s, "%u.%u", major[0], minor[0]);
+
+	dmi_ident[slot] = s;
+}
+
 static void __init dmi_save_uuid(const struct dmi_header *dm, int slot,
 		int index)
 {
@@ -438,6 +466,7 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
 		dmi_save_ident(dm, DMI_BIOS_VENDOR, 4);
 		dmi_save_ident(dm, DMI_BIOS_VERSION, 5);
 		dmi_save_ident(dm, DMI_BIOS_DATE, 8);
+		dmi_save_release(dm, DMI_BIOS_RELEASE, 21);
 		break;
 	case 1:		/* System Information */
 		dmi_save_ident(dm, DMI_SYS_VENDOR, 4);
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5714fd35a83c..618933d770e6 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -532,6 +532,7 @@ enum dmi_field {
 	DMI_BIOS_VENDOR,
 	DMI_BIOS_VERSION,
 	DMI_BIOS_DATE,
+	DMI_BIOS_RELEASE,
 	DMI_SYS_VENDOR,
 	DMI_PRODUCT_NAME,
 	DMI_PRODUCT_VERSION,
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index c91eba751804..cc48930cc02a 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -936,6 +936,7 @@ static const struct dmifield {
 	{ "bvn", DMI_BIOS_VENDOR },
 	{ "bvr", DMI_BIOS_VERSION },
 	{ "bd",  DMI_BIOS_DATE },
+	{ "br",  DMI_BIOS_RELEASE },
 	{ "svn", DMI_SYS_VENDOR },
 	{ "pn",  DMI_PRODUCT_NAME },
 	{ "pvr", DMI_PRODUCT_VERSION },
-- 
2.23.0

[PATCH 2/2] firmware/dmi: Report DMI Embedded Firmware release

[Erwan Velu]

Servers that have a BMC encodes the release version of their firmwarein the
 "Embedded Controller Firmware {Major|Minor} Release" fields of Type 0.

This information is used to know which BMC release actually runs.
It could be used for some quirks, debugging sessions or inventory tasks.

A typical output for a Dell system running the 3.75 bios is :

    [root@t1700 ~]# cat /sys/devices/virtual/dmi/id/fw_release
    3.75
    [root@t1700 ~]#

Signed-off-by: Erwan Velu <e.velu@criteo.com>
---
 drivers/firmware/dmi-id.c       | 3 +++
 drivers/firmware/dmi_scan.c     | 1 +
 include/linux/mod_devicetable.h | 1 +
 scripts/mod/file2alias.c        | 1 +
 4 files changed, 6 insertions(+)

diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
index a2aac65ff771..93eca3222fb0 100644
--- a/drivers/firmware/dmi-id.c
+++ b/drivers/firmware/dmi-id.c
@@ -43,6 +43,7 @@ DEFINE_DMI_ATTR_WITH_SHOW(bios_version,		0444, DMI_BIOS_VERSION);
 DEFINE_DMI_ATTR_WITH_SHOW(bios_date,		0444, DMI_BIOS_DATE);
 DEFINE_DMI_ATTR_WITH_SHOW(sys_vendor,		0444, DMI_SYS_VENDOR);
 DEFINE_DMI_ATTR_WITH_SHOW(bios_release,         0444, DMI_BIOS_RELEASE);
+DEFINE_DMI_ATTR_WITH_SHOW(fw_release,           0444, DMI_EMBEDDED_FW_RELEASE);
 DEFINE_DMI_ATTR_WITH_SHOW(product_name,		0444, DMI_PRODUCT_NAME);
 DEFINE_DMI_ATTR_WITH_SHOW(product_version,	0444, DMI_PRODUCT_VERSION);
 DEFINE_DMI_ATTR_WITH_SHOW(product_serial,	0400, DMI_PRODUCT_SERIAL);
@@ -80,6 +81,7 @@ static ssize_t get_modalias(char *buffer, size_t buffer_size)
 		{ "bvr", DMI_BIOS_VERSION },
 		{ "bd",  DMI_BIOS_DATE },
 		{ "br",  DMI_BIOS_RELEASE },
+		{ "efr", DMI_EMBEDDED_FW_RELEASE },
 		{ "svn", DMI_SYS_VENDOR },
 		{ "pn",  DMI_PRODUCT_NAME },
 		{ "pvr", DMI_PRODUCT_VERSION },
@@ -190,6 +192,7 @@ static void __init dmi_id_init_attr_table(void)
 	ADD_DMI_ATTR(bios_version,      DMI_BIOS_VERSION);
 	ADD_DMI_ATTR(bios_date,         DMI_BIOS_DATE);
 	ADD_DMI_ATTR(bios_release,      DMI_BIOS_RELEASE);
+	ADD_DMI_ATTR(fw_release,        DMI_EMBEDDED_FW_RELEASE);
 	ADD_DMI_ATTR(sys_vendor,        DMI_SYS_VENDOR);
 	ADD_DMI_ATTR(product_name,      DMI_PRODUCT_NAME);
 	ADD_DMI_ATTR(product_version,   DMI_PRODUCT_VERSION);
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index d010c915c1ab..5394aa553140 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -467,6 +467,7 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
 		dmi_save_ident(dm, DMI_BIOS_VERSION, 5);
 		dmi_save_ident(dm, DMI_BIOS_DATE, 8);
 		dmi_save_release(dm, DMI_BIOS_RELEASE, 21);
+		dmi_save_release(dm, DMI_EMBEDDED_FW_RELEASE, 23);
 		break;
 	case 1:		/* System Information */
 		dmi_save_ident(dm, DMI_SYS_VENDOR, 4);
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 618933d770e6..ad64a101676d 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -533,6 +533,7 @@ enum dmi_field {
 	DMI_BIOS_VERSION,
 	DMI_BIOS_DATE,
 	DMI_BIOS_RELEASE,
+	DMI_EMBEDDED_FW_RELEASE,
 	DMI_SYS_VENDOR,
 	DMI_PRODUCT_NAME,
 	DMI_PRODUCT_VERSION,
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index cc48930cc02a..c0e1d379d9df 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -937,6 +937,7 @@ static const struct dmifield {
 	{ "bvr", DMI_BIOS_VERSION },
 	{ "bd",  DMI_BIOS_DATE },
 	{ "br",  DMI_BIOS_RELEASE },
+	{ "efr", DMI_EMBEDDED_FW_RELEASE },
 	{ "svn", DMI_SYS_VENDOR },
 	{ "pn",  DMI_PRODUCT_NAME },
 	{ "pvr", DMI_PRODUCT_VERSION },
-- 
2.23.0

Re: [PATCH 2/2] firmware/dmi: Report DMI Embedded Firmware release

[Jean Delvare]

On Wed, 27 Nov 2019 16:07:26 +0100, Erwan Velu wrote:
> Servers that have a BMC encodes the release version of their firmwarein the

encodes -> encode
firmwarein -> firmware in

>  "Embedded Controller Firmware {Major|Minor} Release" fields of Type 0.
> 
> This information is used to know which BMC release actually runs.
> It could be used for some quirks, debugging sessions or inventory tasks.
> 
> A typical output for a Dell system running the 3.75 bios is :
> 
>     [root@t1700 ~]# cat /sys/devices/virtual/dmi/id/fw_release
>     3.75
>     [root@t1700 ~]#
> 
> Signed-off-by: Erwan Velu <e.velu@criteo.com>
> ---
>  drivers/firmware/dmi-id.c       | 3 +++
>  drivers/firmware/dmi_scan.c     | 1 +
>  include/linux/mod_devicetable.h | 1 +
>  scripts/mod/file2alias.c        | 1 +
>  4 files changed, 6 insertions(+)
> 
> diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> index a2aac65ff771..93eca3222fb0 100644
> --- a/drivers/firmware/dmi-id.c
> +++ b/drivers/firmware/dmi-id.c
> @@ -43,6 +43,7 @@ DEFINE_DMI_ATTR_WITH_SHOW(bios_version,		0444, DMI_BIOS_VERSION);
>  DEFINE_DMI_ATTR_WITH_SHOW(bios_date,		0444, DMI_BIOS_DATE);
>  DEFINE_DMI_ATTR_WITH_SHOW(sys_vendor,		0444, DMI_SYS_VENDOR);
>  DEFINE_DMI_ATTR_WITH_SHOW(bios_release,         0444, DMI_BIOS_RELEASE);
> +DEFINE_DMI_ATTR_WITH_SHOW(fw_release,           0444, DMI_EMBEDDED_FW_RELEASE);

I suggest naming the define DMI_EC_FIRMWARE_RELEASE. "EC" is the
commonly accepted abbreviation for "embedded controller". "FW" on the
other hand is ambiguous as it can stand for firmware but also firewire
or firewall.

Likewise I think the sysfs attribute should be named
"ec_firmware_release".

>  DEFINE_DMI_ATTR_WITH_SHOW(product_name,		0444, DMI_PRODUCT_NAME);
>  DEFINE_DMI_ATTR_WITH_SHOW(product_version,	0444, DMI_PRODUCT_VERSION);
>  DEFINE_DMI_ATTR_WITH_SHOW(product_serial,	0400, DMI_PRODUCT_SERIAL);
> @@ -80,6 +81,7 @@ static ssize_t get_modalias(char *buffer, size_t buffer_size)
>  		{ "bvr", DMI_BIOS_VERSION },
>  		{ "bd",  DMI_BIOS_DATE },
>  		{ "br",  DMI_BIOS_RELEASE },
> +		{ "efr", DMI_EMBEDDED_FW_RELEASE },
>  		{ "svn", DMI_SYS_VENDOR },
>  		{ "pn",  DMI_PRODUCT_NAME },
>  		{ "pvr", DMI_PRODUCT_VERSION },
> @@ -190,6 +192,7 @@ static void __init dmi_id_init_attr_table(void)
>  	ADD_DMI_ATTR(bios_version,      DMI_BIOS_VERSION);
>  	ADD_DMI_ATTR(bios_date,         DMI_BIOS_DATE);
>  	ADD_DMI_ATTR(bios_release,      DMI_BIOS_RELEASE);
> +	ADD_DMI_ATTR(fw_release,        DMI_EMBEDDED_FW_RELEASE);
>  	ADD_DMI_ATTR(sys_vendor,        DMI_SYS_VENDOR);
>  	ADD_DMI_ATTR(product_name,      DMI_PRODUCT_NAME);
>  	ADD_DMI_ATTR(product_version,   DMI_PRODUCT_VERSION);
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index d010c915c1ab..5394aa553140 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -467,6 +467,7 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
>  		dmi_save_ident(dm, DMI_BIOS_VERSION, 5);
>  		dmi_save_ident(dm, DMI_BIOS_DATE, 8);
>  		dmi_save_release(dm, DMI_BIOS_RELEASE, 21);
> +		dmi_save_release(dm, DMI_EMBEDDED_FW_RELEASE, 23);
>  		break;
>  	case 1:		/* System Information */
>  		dmi_save_ident(dm, DMI_SYS_VENDOR, 4);
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 618933d770e6..ad64a101676d 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -533,6 +533,7 @@ enum dmi_field {
>  	DMI_BIOS_VERSION,
>  	DMI_BIOS_DATE,
>  	DMI_BIOS_RELEASE,
> +	DMI_EMBEDDED_FW_RELEASE,
>  	DMI_SYS_VENDOR,
>  	DMI_PRODUCT_NAME,
>  	DMI_PRODUCT_VERSION,
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index cc48930cc02a..c0e1d379d9df 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -937,6 +937,7 @@ static const struct dmifield {
>  	{ "bvr", DMI_BIOS_VERSION },
>  	{ "bd",  DMI_BIOS_DATE },
>  	{ "br",  DMI_BIOS_RELEASE },
> +	{ "efr", DMI_EMBEDDED_FW_RELEASE },
>  	{ "svn", DMI_SYS_VENDOR },
>  	{ "pn",  DMI_PRODUCT_NAME },
>  	{ "pvr", DMI_PRODUCT_VERSION },

Also, as already mentioned during my first review, please merge both
patches together. They touch exactly the same areas, they are doing
basically the same thing, and will never be backported individually, so
splitting them creates more paperwork with no benefit.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] firmware/dmi: Report DMI Embedded Firmware release

[Erwan Velu]

Thanks for the feedback Jean.
I will update the patch accordingly.

Le jeu. 6 févr. 2020 à 13:25, Jean Delvare <jdelvare@suse.de> a écrit :
>
> On Wed, 27 Nov 2019 16:07:26 +0100, Erwan Velu wrote:
> > Servers that have a BMC encodes the release version of their firmwarein the
>
> encodes -> encode
> firmwarein -> firmware in
>
> >  "Embedded Controller Firmware {Major|Minor} Release" fields of Type 0.
> >
> > This information is used to know which BMC release actually runs.
> > It could be used for some quirks, debugging sessions or inventory tasks.
> >
> > A typical output for a Dell system running the 3.75 bios is :
> >
> >     [root@t1700 ~]# cat /sys/devices/virtual/dmi/id/fw_release
> >     3.75
> >     [root@t1700 ~]#
> >
> > Signed-off-by: Erwan Velu <e.velu@criteo.com>
> > ---
> >  drivers/firmware/dmi-id.c       | 3 +++
> >  drivers/firmware/dmi_scan.c     | 1 +
> >  include/linux/mod_devicetable.h | 1 +
> >  scripts/mod/file2alias.c        | 1 +
> >  4 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> > index a2aac65ff771..93eca3222fb0 100644
> > --- a/drivers/firmware/dmi-id.c
> > +++ b/drivers/firmware/dmi-id.c
> > @@ -43,6 +43,7 @@ DEFINE_DMI_ATTR_WITH_SHOW(bios_version,             0444, DMI_BIOS_VERSION);
> >  DEFINE_DMI_ATTR_WITH_SHOW(bios_date,         0444, DMI_BIOS_DATE);
> >  DEFINE_DMI_ATTR_WITH_SHOW(sys_vendor,                0444, DMI_SYS_VENDOR);
> >  DEFINE_DMI_ATTR_WITH_SHOW(bios_release,         0444, DMI_BIOS_RELEASE);
> > +DEFINE_DMI_ATTR_WITH_SHOW(fw_release,           0444, DMI_EMBEDDED_FW_RELEASE);
>
> I suggest naming the define DMI_EC_FIRMWARE_RELEASE. "EC" is the
> commonly accepted abbreviation for "embedded controller". "FW" on the
> other hand is ambiguous as it can stand for firmware but also firewire
> or firewall.
>
> Likewise I think the sysfs attribute should be named
> "ec_firmware_release".
>
> >  DEFINE_DMI_ATTR_WITH_SHOW(product_name,              0444, DMI_PRODUCT_NAME);
> >  DEFINE_DMI_ATTR_WITH_SHOW(product_version,   0444, DMI_PRODUCT_VERSION);
> >  DEFINE_DMI_ATTR_WITH_SHOW(product_serial,    0400, DMI_PRODUCT_SERIAL);
> > @@ -80,6 +81,7 @@ static ssize_t get_modalias(char *buffer, size_t buffer_size)
> >               { "bvr", DMI_BIOS_VERSION },
> >               { "bd",  DMI_BIOS_DATE },
> >               { "br",  DMI_BIOS_RELEASE },
> > +             { "efr", DMI_EMBEDDED_FW_RELEASE },
> >               { "svn", DMI_SYS_VENDOR },
> >               { "pn",  DMI_PRODUCT_NAME },
> >               { "pvr", DMI_PRODUCT_VERSION },
> > @@ -190,6 +192,7 @@ static void __init dmi_id_init_attr_table(void)
> >       ADD_DMI_ATTR(bios_version,      DMI_BIOS_VERSION);
> >       ADD_DMI_ATTR(bios_date,         DMI_BIOS_DATE);
> >       ADD_DMI_ATTR(bios_release,      DMI_BIOS_RELEASE);
> > +     ADD_DMI_ATTR(fw_release,        DMI_EMBEDDED_FW_RELEASE);
> >       ADD_DMI_ATTR(sys_vendor,        DMI_SYS_VENDOR);
> >       ADD_DMI_ATTR(product_name,      DMI_PRODUCT_NAME);
> >       ADD_DMI_ATTR(product_version,   DMI_PRODUCT_VERSION);
> > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> > index d010c915c1ab..5394aa553140 100644
> > --- a/drivers/firmware/dmi_scan.c
> > +++ b/drivers/firmware/dmi_scan.c
> > @@ -467,6 +467,7 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
> >               dmi_save_ident(dm, DMI_BIOS_VERSION, 5);
> >               dmi_save_ident(dm, DMI_BIOS_DATE, 8);
> >               dmi_save_release(dm, DMI_BIOS_RELEASE, 21);
> > +             dmi_save_release(dm, DMI_EMBEDDED_FW_RELEASE, 23);
> >               break;
> >       case 1:         /* System Information */
> >               dmi_save_ident(dm, DMI_SYS_VENDOR, 4);
> > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > index 618933d770e6..ad64a101676d 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -533,6 +533,7 @@ enum dmi_field {
> >       DMI_BIOS_VERSION,
> >       DMI_BIOS_DATE,
> >       DMI_BIOS_RELEASE,
> > +     DMI_EMBEDDED_FW_RELEASE,
> >       DMI_SYS_VENDOR,
> >       DMI_PRODUCT_NAME,
> >       DMI_PRODUCT_VERSION,
> > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> > index cc48930cc02a..c0e1d379d9df 100644
> > --- a/scripts/mod/file2alias.c
> > +++ b/scripts/mod/file2alias.c
> > @@ -937,6 +937,7 @@ static const struct dmifield {
> >       { "bvr", DMI_BIOS_VERSION },
> >       { "bd",  DMI_BIOS_DATE },
> >       { "br",  DMI_BIOS_RELEASE },
> > +     { "efr", DMI_EMBEDDED_FW_RELEASE },
> >       { "svn", DMI_SYS_VENDOR },
> >       { "pn",  DMI_PRODUCT_NAME },
> >       { "pvr", DMI_PRODUCT_VERSION },
>
> Also, as already mentioned during my first review, please merge both
> patches together. They touch exactly the same areas, they are doing
> basically the same thing, and will never be backported individually, so
> splitting them creates more paperwork with no benefit.
>
> --
> Jean Delvare
> SUSE L3 Support

Re: [PATCH 2/2] firmware/dmi: Report DMI Embedded Firmware release

[Erwan Velu]

On 06/02/2020 13:24, Jean Delvare wrote:
[...]
> Also, as already mentioned during my first review, please merge both
> patches together. They touch exactly the same areas, they are doing
> basically the same thing, and will never be backported individually, so
> splitting them creates more paperwork with no benefit.

Jean,

I applied all your recommendations and submitted a new version of the patch.

Hope you'll like it this way.

Erwan,

Re: [PATCH 1/2] firmware/dmi: Report DMI Bios release

[Jean Delvare]

Hi Erwan,

Once again, sorry for the late answer.

On Wed, 27 Nov 2019 16:07:25 +0100, Erwan Velu wrote:
> Some vendors like HPe or Dell, encode the release version of their BIOS
> in the "System BIOS {Major|Minor} Release" fields of Type 0.
> 
> This information is used to know which bios release actually runs.
> It could be used for some quirks, debugging sessions or inventory tasks.
> 
> A typical output for a Dell system running the 65.27 bios is :
> 
> 	[root@t1700 ~]# cat /sys/devices/virtual/dmi/id/bios_release
> 	65.27
> 	[root@t1700 ~]#
> 
> This commit add  dmi_save_release() function have the logic to
> check if the field is valid. If so, it reports the actual value.
> 
> Signed-off-by: Erwan Velu <e.velu@criteo.com>
> ---
>  drivers/firmware/dmi-id.c       |  3 +++
>  drivers/firmware/dmi_scan.c     | 29 +++++++++++++++++++++++++++++
>  include/linux/mod_devicetable.h |  1 +
>  scripts/mod/file2alias.c        |  1 +
>  4 files changed, 34 insertions(+)
> 
> diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> index ff39f64f2aae..a2aac65ff771 100644
> --- a/drivers/firmware/dmi-id.c
> +++ b/drivers/firmware/dmi-id.c
> @@ -42,6 +42,7 @@ DEFINE_DMI_ATTR_WITH_SHOW(bios_vendor,		0444, DMI_BIOS_VENDOR);
>  DEFINE_DMI_ATTR_WITH_SHOW(bios_version,		0444, DMI_BIOS_VERSION);
>  DEFINE_DMI_ATTR_WITH_SHOW(bios_date,		0444, DMI_BIOS_DATE);
>  DEFINE_DMI_ATTR_WITH_SHOW(sys_vendor,		0444, DMI_SYS_VENDOR);
> +DEFINE_DMI_ATTR_WITH_SHOW(bios_release,         0444, DMI_BIOS_RELEASE);
>  DEFINE_DMI_ATTR_WITH_SHOW(product_name,		0444, DMI_PRODUCT_NAME);
>  DEFINE_DMI_ATTR_WITH_SHOW(product_version,	0444, DMI_PRODUCT_VERSION);
>  DEFINE_DMI_ATTR_WITH_SHOW(product_serial,	0400, DMI_PRODUCT_SERIAL);
> @@ -78,6 +79,7 @@ static ssize_t get_modalias(char *buffer, size_t buffer_size)
>  		{ "bvn", DMI_BIOS_VENDOR },
>  		{ "bvr", DMI_BIOS_VERSION },
>  		{ "bd",  DMI_BIOS_DATE },
> +		{ "br",  DMI_BIOS_RELEASE },
>  		{ "svn", DMI_SYS_VENDOR },
>  		{ "pn",  DMI_PRODUCT_NAME },
>  		{ "pvr", DMI_PRODUCT_VERSION },
> @@ -187,6 +189,7 @@ static void __init dmi_id_init_attr_table(void)
>  	ADD_DMI_ATTR(bios_vendor,       DMI_BIOS_VENDOR);
>  	ADD_DMI_ATTR(bios_version,      DMI_BIOS_VERSION);
>  	ADD_DMI_ATTR(bios_date,         DMI_BIOS_DATE);
> +	ADD_DMI_ATTR(bios_release,      DMI_BIOS_RELEASE);
>  	ADD_DMI_ATTR(sys_vendor,        DMI_SYS_VENDOR);
>  	ADD_DMI_ATTR(product_name,      DMI_PRODUCT_NAME);
>  	ADD_DMI_ATTR(product_version,   DMI_PRODUCT_VERSION);
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 1e21fc3e9851..d010c915c1ab 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -181,6 +181,34 @@ static void __init dmi_save_ident(const struct dmi_header *dm, int slot,
>  	dmi_ident[slot] = p;
>  }
>  
> +static void __init dmi_save_release(const struct dmi_header *dm, int slot,
> +		int index)
> +{
> +	const u8 *minor, *major;
> +	char *s;
> +
> +	/* If the table doesn't have the field, let's return */
> +	if (dmi_ident[slot] || dm->length < index)
> +		return;
> +
> +	minor = (u8 *) dm + index;
> +	major = (u8 *) dm + index - 1;
> +
> +	/* As per the spec, if the system doesn't support this field,
> +	 * the value is FF
> +	 */
> +	if (major[0] == 0xFF && minor[0] == 0xFF)

When using a pointer to a single entity, the common practice is to use
*major rather than major[0].

> +		return;
> +
> +	s = dmi_alloc(4);

4 bytes (3 + 1) were enough when you encoded a single byte. Now that you
encode 2 bytes separates by a dot, you need 8 (3 + 1 + 3 + 1).

> +	if (!s)
> +		return;
> +
> +	sprintf(s, "%u.%u", major[0], minor[0]);

Here too, *major would be preferred.

> +
> +	dmi_ident[slot] = s;
> +}
> +
>  static void __init dmi_save_uuid(const struct dmi_header *dm, int slot,
>  		int index)
>  {
> @@ -438,6 +466,7 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
>  		dmi_save_ident(dm, DMI_BIOS_VENDOR, 4);
>  		dmi_save_ident(dm, DMI_BIOS_VERSION, 5);
>  		dmi_save_ident(dm, DMI_BIOS_DATE, 8);
> +		dmi_save_release(dm, DMI_BIOS_RELEASE, 21);
>  		break;
>  	case 1:		/* System Information */
>  		dmi_save_ident(dm, DMI_SYS_VENDOR, 4);
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 5714fd35a83c..618933d770e6 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -532,6 +532,7 @@ enum dmi_field {
>  	DMI_BIOS_VENDOR,
>  	DMI_BIOS_VERSION,
>  	DMI_BIOS_DATE,
> +	DMI_BIOS_RELEASE,
>  	DMI_SYS_VENDOR,
>  	DMI_PRODUCT_NAME,
>  	DMI_PRODUCT_VERSION,
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index c91eba751804..cc48930cc02a 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -936,6 +936,7 @@ static const struct dmifield {
>  	{ "bvn", DMI_BIOS_VENDOR },
>  	{ "bvr", DMI_BIOS_VERSION },
>  	{ "bd",  DMI_BIOS_DATE },
> +	{ "br",  DMI_BIOS_RELEASE },
>  	{ "svn", DMI_SYS_VENDOR },
>  	{ "pn",  DMI_PRODUCT_NAME },
>  	{ "pvr", DMI_PRODUCT_VERSION },


-- 
Jean Delvare
SUSE L3 Support