diff mbox series

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

Message ID 20191127150729.860625-2-e.velu@criteo.com (mailing list archive)
State New, archived
Headers show
Series [1/2] firmware/dmi: Report DMI Bios release | expand

Commit Message

Erwan Velu Nov. 27, 2019, 3:07 p.m. UTC 
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(+)

Comments

Jean Delvare Feb. 6, 2020, 12:24 p.m. UTC | #1 
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.
Erwan Velu Feb. 6, 2020, 12:25 p.m. UTC | #2 
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
Erwan Velu Feb. 7, 2020, 8:38 a.m. UTC | #3 
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,
diff mbox series

Patch

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 },