| Message ID | 20210610093316.833557-1-ilias.apalodimas@linaro.org |
|---|---|
| State | Accepted |
| Commit | 70e80666f26a516096f3787e884d42818d8b4087 |
| Headers | show |
| Series | smbios: Fix SMBIOS tables | expand |
On 6/10/21 11:33 AM, Ilias Apalodimas wrote: > Commit e4f8e543f1a9("smbios: Drop the unused Kconfig options") > break SMBIOS tables. The reason is that the patch drops the Kconfig > options *after* removing the code using them, but that changes the semantics > of the code completely. Prior to the change a non NULL value was used in > the 'product' and 'manufacturer ' fields. > > Chapter 6.2 of the DMTF spec requires Manufacturer and Product Name to be > non-null on some of the tables. So let's add sane defaults for Type1/2/3. > > * Before the patchset: @Ilias, Simon: Simon should take this patch as he has been the main contributor for SMBIOS. Acked-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > <snip> > Handle 0x0002, DMI type 2, 14 bytes > Base Board Information > Manufacturer: Not Specified > Product Name: Not Specified > Version: Not Specified > Serial Number: Not Specified > Asset Tag: Not Specified > Features: > Board is a hosting board > Location In Chassis: Not Specified > Chassis Handle: 0x0000 > Type: Motherboard > > Invalid entry length (0). DMI table is broken! Stop. > > * After the patchset: > > <snip> > Handle 0x0005, DMI type 32, 11 bytes > System Boot Information > Status: No errors detected > > Handle 0x0006, DMI type 127, 4 bytes > End Of Table > > Fixes: e4f8e543f1a9 ("smbios: Drop the unused Kconfig options") > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > This depends on https://lists.denx.de/pipermail/u-boot/2021-June/451761.html > lib/smbios.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/lib/smbios.c b/lib/smbios.c > index abdd157a7084..e2c6b1a44ee3 100644 > --- a/lib/smbios.c > +++ b/lib/smbios.c > @@ -259,7 +259,11 @@ static int smbios_write_type1(ulong *current, int handle, > fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle); > smbios_set_eos(ctx, t->eos); > t->manufacturer = smbios_add_prop(ctx, "manufacturer"); > + if (!t->manufacturer) > + t->manufacturer = smbios_add_string(ctx, "Unknown"); > t->product_name = smbios_add_prop(ctx, "product"); > + if (!t->product_name) > + t->product_name = smbios_add_string(ctx, "Unknown Product"); > t->version = smbios_add_prop_si(ctx, "version", > SYSINFO_ID_SMBIOS_SYSTEM_VERSION); > if (serial_str) { > @@ -289,7 +293,11 @@ static int smbios_write_type2(ulong *current, int handle, > fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle); > smbios_set_eos(ctx, t->eos); > t->manufacturer = smbios_add_prop(ctx, "manufacturer"); > + if (!t->manufacturer) > + t->manufacturer = smbios_add_string(ctx, "Unknown"); > t->product_name = smbios_add_prop(ctx, "product"); > + if (!t->product_name) > + t->product_name = smbios_add_string(ctx, "Unknown Product"); > t->version = smbios_add_prop_si(ctx, "version", > SYSINFO_ID_SMBIOS_BASEBOARD_VERSION); > t->asset_tag_number = smbios_add_prop(ctx, "asset-tag"); > @@ -314,6 +322,8 @@ static int smbios_write_type3(ulong *current, int handle, > fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle); > smbios_set_eos(ctx, t->eos); > t->manufacturer = smbios_add_prop(ctx, "manufacturer"); > + if (!t->manufacturer) > + t->manufacturer = smbios_add_string(ctx, "Unknown"); > t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP; > t->bootup_state = SMBIOS_STATE_SAFE; > t->power_supply_state = SMBIOS_STATE_SAFE; >
Hi Ilias, On Thu, 10 Jun 2021 at 03:33, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Commit e4f8e543f1a9("smbios: Drop the unused Kconfig options") > break SMBIOS tables. The reason is that the patch drops the Kconfig > options *after* removing the code using them, but that changes the semantics > of the code completely. Prior to the change a non NULL value was used in > the 'product' and 'manufacturer ' fields. > > Chapter 6.2 of the DMTF spec requires Manufacturer and Product Name to be > non-null on some of the tables. So let's add sane defaults for Type1/2/3. > > * Before the patchset: > > <snip> > Handle 0x0002, DMI type 2, 14 bytes > Base Board Information > Manufacturer: Not Specified > Product Name: Not Specified > Version: Not Specified > Serial Number: Not Specified > Asset Tag: Not Specified > Features: > Board is a hosting board > Location In Chassis: Not Specified > Chassis Handle: 0x0000 > Type: Motherboard > > Invalid entry length (0). DMI table is broken! Stop. > > * After the patchset: > > <snip> > Handle 0x0005, DMI type 32, 11 bytes > System Boot Information > Status: No errors detected > > Handle 0x0006, DMI type 127, 4 bytes > End Of Table > > Fixes: e4f8e543f1a9 ("smbios: Drop the unused Kconfig options") > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > This depends on https://lists.denx.de/pipermail/u-boot/2021-June/451761.html > lib/smbios.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) It is strange that all the boards that defined the old CONFIG set it to "", meaning that an empty string is used. Was that just wrong? In your patch you are using 'Unknown' and 'Unknown Product'. Should it use CONFIG_SYS_VENDOR and CONFIG_SYS_BOARD instead? Or should we actually fail (and return an error code), and require the properties to be set by the board? It does not seem very useful to have a meaningless string. Are these SMBIOS values ignored in Linux? > > diff --git a/lib/smbios.c b/lib/smbios.c > index abdd157a7084..e2c6b1a44ee3 100644 > --- a/lib/smbios.c > +++ b/lib/smbios.c > @@ -259,7 +259,11 @@ static int smbios_write_type1(ulong *current, int handle, > fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle); > smbios_set_eos(ctx, t->eos); > t->manufacturer = smbios_add_prop(ctx, "manufacturer"); > + if (!t->manufacturer) > + t->manufacturer = smbios_add_string(ctx, "Unknown"); > t->product_name = smbios_add_prop(ctx, "product"); > + if (!t->product_name) > + t->product_name = smbios_add_string(ctx, "Unknown Product"); > t->version = smbios_add_prop_si(ctx, "version", > SYSINFO_ID_SMBIOS_SYSTEM_VERSION); > if (serial_str) { > @@ -289,7 +293,11 @@ static int smbios_write_type2(ulong *current, int handle, > fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle); > smbios_set_eos(ctx, t->eos); > t->manufacturer = smbios_add_prop(ctx, "manufacturer"); > + if (!t->manufacturer) > + t->manufacturer = smbios_add_string(ctx, "Unknown"); > t->product_name = smbios_add_prop(ctx, "product"); > + if (!t->product_name) > + t->product_name = smbios_add_string(ctx, "Unknown Product"); > t->version = smbios_add_prop_si(ctx, "version", > SYSINFO_ID_SMBIOS_BASEBOARD_VERSION); > t->asset_tag_number = smbios_add_prop(ctx, "asset-tag"); > @@ -314,6 +322,8 @@ static int smbios_write_type3(ulong *current, int handle, > fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle); > smbios_set_eos(ctx, t->eos); > t->manufacturer = smbios_add_prop(ctx, "manufacturer"); > + if (!t->manufacturer) > + t->manufacturer = smbios_add_string(ctx, "Unknown"); > t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP; > t->bootup_state = SMBIOS_STATE_SAFE; > t->power_supply_state = SMBIOS_STATE_SAFE; > -- > 2.32.0.rc0 > Regrads, Simon
Hi Simon, > > --- [...] > > This depends on https://lists.denx.de/pipermail/u-boot/2021-June/451761.html > > lib/smbios.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > It is strange that all the boards that defined the old CONFIG set it > to "", meaning that an empty string is used. Was that just wrong? Yea I think so. In fact Heinrich fixed an identical error due to that on 00a871d34e2f > > In your patch you are using 'Unknown' and 'Unknown Product'. Should it > use CONFIG_SYS_VENDOR and CONFIG_SYS_BOARD instead? I don't have an issue with that, as long as they are defined for every board. I think already Tom pulled this though, but I can send a follow up. > > Or should we actually fail (and return an error code), and require the > properties to be set by the board? It does not seem very useful to > have a meaningless string. Are these SMBIOS values ignored in Linux? > Well the problem is that those tables are marked as mandatory (section 6.2) So failing one would mean disable the entire thing. Why dont we do something less intrusive? I can send a follow up, popping a warning 'fix your dts and/or CONFIG_SYS_VENDOR/CONFIG_SYS_BOARD'. Then we can rid of the fallback but keep the warning for future boards. I am not aware of all the cases linux uses those. I found the problem trying to enable fwupd and specifically the EFI capsule updates for the firmware. In that case, fwupd is trying to find the 'EFI bit' in 'BIOS Characteristics Extension Byte 2'. There were two things wrong, the bit was wrong and the tables were not installed at all. With the two patches applied fwupd seems happy. Cheers /Ilias > > > > diff --git a/lib/smbios.c b/lib/smbios.c > > index abdd157a7084..e2c6b1a44ee3 100644 > > --- a/lib/smbios.c > > +++ b/lib/smbios.c > > @@ -259,7 +259,11 @@ static int smbios_write_type1(ulong *current, int handle, > > fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle); > > smbios_set_eos(ctx, t->eos); > > t->manufacturer = smbios_add_prop(ctx, "manufacturer"); > > + if (!t->manufacturer) > > + t->manufacturer = smbios_add_string(ctx, "Unknown"); > > t->product_name = smbios_add_prop(ctx, "product"); > > + if (!t->product_name) > > + t->product_name = smbios_add_string(ctx, "Unknown Product"); > > t->version = smbios_add_prop_si(ctx, "version", > > SYSINFO_ID_SMBIOS_SYSTEM_VERSION); > > if (serial_str) { > > @@ -289,7 +293,11 @@ static int smbios_write_type2(ulong *current, int handle, > > fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle); > > smbios_set_eos(ctx, t->eos); > > t->manufacturer = smbios_add_prop(ctx, "manufacturer"); > > + if (!t->manufacturer) > > + t->manufacturer = smbios_add_string(ctx, "Unknown"); > > t->product_name = smbios_add_prop(ctx, "product"); > > + if (!t->product_name) > > + t->product_name = smbios_add_string(ctx, "Unknown Product"); > > t->version = smbios_add_prop_si(ctx, "version", > > SYSINFO_ID_SMBIOS_BASEBOARD_VERSION); > > t->asset_tag_number = smbios_add_prop(ctx, "asset-tag"); > > @@ -314,6 +322,8 @@ static int smbios_write_type3(ulong *current, int handle, > > fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle); > > smbios_set_eos(ctx, t->eos); > > t->manufacturer = smbios_add_prop(ctx, "manufacturer"); > > + if (!t->manufacturer) > > + t->manufacturer = smbios_add_string(ctx, "Unknown"); > > t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP; > > t->bootup_state = SMBIOS_STATE_SAFE; > > t->power_supply_state = SMBIOS_STATE_SAFE; > > -- > > 2.32.0.rc0 > > > > Regrads, > Simon
Hi Ilias, On Sat, 26 Jun 2021 at 12:51, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > > > --- > [...] > > > This depends on https://lists.denx.de/pipermail/u-boot/2021-June/451761.html > > > lib/smbios.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > It is strange that all the boards that defined the old CONFIG set it > > to "", meaning that an empty string is used. Was that just wrong? > > Yea I think so. In fact Heinrich fixed an identical error due to that on > 00a871d34e2f OK I see. > > > > > In your patch you are using 'Unknown' and 'Unknown Product'. Should it > > use CONFIG_SYS_VENDOR and CONFIG_SYS_BOARD instead? > > I don't have an issue with that, as long as they are defined for every board. > I think already Tom pulled this though, but I can send a follow up. I don't they are both defined for every board (the vast majority though), but probably for every board that uses SMBIOS. > > > > > Or should we actually fail (and return an error code), and require the > > properties to be set by the board? It does not seem very useful to > > have a meaningless string. Are these SMBIOS values ignored in Linux? > > > > Well the problem is that those tables are marked as mandatory (section 6.2) > So failing one would mean disable the entire thing. Why dont we do something > less intrusive? I can send a follow up, popping a warning 'fix your dts > and/or CONFIG_SYS_VENDOR/CONFIG_SYS_BOARD'. Then we can rid of the fallback > but keep the warning for future boards. OK, I suppose that would have to be a run-time warning. I just worry that no one will notice / fix it if the code builds and runs without it. > > I am not aware of all the cases linux uses those. I found the problem trying > to enable fwupd and specifically the EFI capsule updates for the firmware. In > that case, fwupd is trying to find the 'EFI bit' in 'BIOS Characteristics > Extension Byte 2'. There were two things wrong, the bit was wrong and the > tables were not installed at all. With the two patches applied fwupd seems > happy. OK. Regards, Simon
diff --git a/lib/smbios.c b/lib/smbios.c index abdd157a7084..e2c6b1a44ee3 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -259,7 +259,11 @@ static int smbios_write_type1(ulong *current, int handle, fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle); smbios_set_eos(ctx, t->eos); t->manufacturer = smbios_add_prop(ctx, "manufacturer"); + if (!t->manufacturer) + t->manufacturer = smbios_add_string(ctx, "Unknown"); t->product_name = smbios_add_prop(ctx, "product"); + if (!t->product_name) + t->product_name = smbios_add_string(ctx, "Unknown Product"); t->version = smbios_add_prop_si(ctx, "version", SYSINFO_ID_SMBIOS_SYSTEM_VERSION); if (serial_str) { @@ -289,7 +293,11 @@ static int smbios_write_type2(ulong *current, int handle, fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle); smbios_set_eos(ctx, t->eos); t->manufacturer = smbios_add_prop(ctx, "manufacturer"); + if (!t->manufacturer) + t->manufacturer = smbios_add_string(ctx, "Unknown"); t->product_name = smbios_add_prop(ctx, "product"); + if (!t->product_name) + t->product_name = smbios_add_string(ctx, "Unknown Product"); t->version = smbios_add_prop_si(ctx, "version", SYSINFO_ID_SMBIOS_BASEBOARD_VERSION); t->asset_tag_number = smbios_add_prop(ctx, "asset-tag"); @@ -314,6 +322,8 @@ static int smbios_write_type3(ulong *current, int handle, fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle); smbios_set_eos(ctx, t->eos); t->manufacturer = smbios_add_prop(ctx, "manufacturer"); + if (!t->manufacturer) + t->manufacturer = smbios_add_string(ctx, "Unknown"); t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP; t->bootup_state = SMBIOS_STATE_SAFE; t->power_supply_state = SMBIOS_STATE_SAFE;
Commit e4f8e543f1a9("smbios: Drop the unused Kconfig options") break SMBIOS tables. The reason is that the patch drops the Kconfig options *after* removing the code using them, but that changes the semantics of the code completely. Prior to the change a non NULL value was used in the 'product' and 'manufacturer ' fields. Chapter 6.2 of the DMTF spec requires Manufacturer and Product Name to be non-null on some of the tables. So let's add sane defaults for Type1/2/3. * Before the patchset: <snip> Handle 0x0002, DMI type 2, 14 bytes Base Board Information Manufacturer: Not Specified Product Name: Not Specified Version: Not Specified Serial Number: Not Specified Asset Tag: Not Specified Features: Board is a hosting board Location In Chassis: Not Specified Chassis Handle: 0x0000 Type: Motherboard Invalid entry length (0). DMI table is broken! Stop. * After the patchset: <snip> Handle 0x0005, DMI type 32, 11 bytes System Boot Information Status: No errors detected Handle 0x0006, DMI type 127, 4 bytes End Of Table Fixes: e4f8e543f1a9 ("smbios: Drop the unused Kconfig options") Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- This depends on https://lists.denx.de/pipermail/u-boot/2021-June/451761.html lib/smbios.c | 10 ++++++++++ 1 file changed, 10 insertions(+) -- 2.32.0.rc0