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 |
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.
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
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 --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 },
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(+)