| Message ID | 20210705084915.32008-1-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | smbios: Try CONFIG_SYS_ options before using "Unknow" as a value | expand |
On Mon, Jul 5, 2021 at 4:49 PM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > SMBIOS entries for manufacturer and board can't be empty, as the spec > explicitly requires a value. > Instead of passing "Unknown" and "Unknown product" for the manufacturer and > product name if the .dts options are missing, try to use CONFIG_SYS_VENDOR > and CONFIG_SYS_BOARD respectively. If those are not found either warn the > user at runtime and use "Unknown" for both entries. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > lib/smbios.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
On 7/5/21 10:49 AM, Ilias Apalodimas wrote: > SMBIOS entries for manufacturer and board can't be empty, as the spec > explicitly requires a value. > Instead of passing "Unknown" and "Unknown product" for the manufacturer and > product name if the .dts options are missing, try to use CONFIG_SYS_VENDOR > and CONFIG_SYS_BOARD respectively. If those are not found either warn the > user at runtime and use "Unknown" for both entries. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> These defaults don't make any sense to me: Hardkernel Odroid C2 CONFIG_SYS_VENDOR="amlogic" CONFIG_SYS_BOARD="p200" Orange Pi PC CONFIG_SYS_BOARD="sunxi" CONFIG_SYS_CONFIG_NAME="sun8i" Solidrun MacchiatoBIN CONFIG_SYS_VENDOR="Marvell" CONFIG_SYS_BOARD="mvebu_armada-8k" None of these CONFIG_SYS_VENDOR and CONFIG_SYS_CONFIG_NAME values match the manufacturer nor the board name. If you want a default for the board name, use the 'model' property of the devicetree. Hardkernel Odroid C2: model = "Hardkernel ODROID-C2" Orange Pi PC model = "Xunlong Orange Pi PC" Solidrun MacchiatoBIN: model = "MACCHIATOBin-8040" Best regards Heinrich > --- > lib/smbios.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/lib/smbios.c b/lib/smbios.c > index b52e125eeb14..d1997ce70d19 100644 > --- a/lib/smbios.c > +++ b/lib/smbios.c > @@ -79,8 +79,10 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > int i = 1; > char *p = ctx->eos; > > - if (!*str) > + if (!*str) { > str = "Unknown"; > + log_warning("Empty string found. Please fix your DTS and/or CONFIG_SYS_* options"); > + } > > for (;;) { > if (!*p) { > @@ -259,10 +261,10 @@ static int smbios_write_type1(ulong *current, int 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->manufacturer = smbios_add_string(ctx, CONFIG_SYS_VENDOR); > t->product_name = smbios_add_prop(ctx, "product"); > if (!t->product_name) > - t->product_name = smbios_add_string(ctx, "Unknown Product"); > + t->product_name = smbios_add_string(ctx, CONFIG_SYS_BOARD); > t->version = smbios_add_prop_si(ctx, "version", > SYSINFO_ID_SMBIOS_SYSTEM_VERSION); > if (serial_str) { > @@ -293,10 +295,10 @@ static int smbios_write_type2(ulong *current, int 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->manufacturer = smbios_add_string(ctx, CONFIG_SYS_VENDOR); > t->product_name = smbios_add_prop(ctx, "product"); > if (!t->product_name) > - t->product_name = smbios_add_string(ctx, "Unknown Product"); > + t->product_name = smbios_add_string(ctx, CONFIG_SYS_BOARD); > t->version = smbios_add_prop_si(ctx, "version", > SYSINFO_ID_SMBIOS_BASEBOARD_VERSION); > t->asset_tag_number = smbios_add_prop(ctx, "asset-tag"); > @@ -322,7 +324,7 @@ static int smbios_write_type3(ulong *current, int 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->manufacturer = smbios_add_string(ctx, CONFIG_SYS_VENDOR); > t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP; > t->bootup_state = SMBIOS_STATE_SAFE; > t->power_supply_state = SMBIOS_STATE_SAFE; >
Hi Heinrich, On Mon, Jul 05, 2021 at 12:25:47PM +0200, Heinrich Schuchardt wrote: > On 7/5/21 10:49 AM, Ilias Apalodimas wrote: > > SMBIOS entries for manufacturer and board can't be empty, as the spec > > explicitly requires a value. > > Instead of passing "Unknown" and "Unknown product" for the manufacturer and > > product name if the .dts options are missing, try to use CONFIG_SYS_VENDOR > > and CONFIG_SYS_BOARD respectively. If those are not found either warn the > > user at runtime and use "Unknown" for both entries. > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > These defaults don't make any sense to me: > > Hardkernel Odroid C2 > CONFIG_SYS_VENDOR="amlogic" > CONFIG_SYS_BOARD="p200" > > Orange Pi PC > CONFIG_SYS_BOARD="sunxi" > CONFIG_SYS_CONFIG_NAME="sun8i" > > Solidrun MacchiatoBIN > CONFIG_SYS_VENDOR="Marvell" > CONFIG_SYS_BOARD="mvebu_armada-8k" > > None of these CONFIG_SYS_VENDOR and CONFIG_SYS_CONFIG_NAME values match > the manufacturer nor the board name. The point here is to provide a viable fallback in case the .dts options are missing and define the semantics we expect from U-Boot. I do think Simon did the right thing by deleting the specific SMBIOS Kconfig options for these values. We should have a common place keeping those instead of scattering around config options. If the .config values don't make sense then the SMBIOS wont either. Perhaps we should just fix the .config options properly? > > If you want a default for the board name, use the 'model' property of > the devicetree. That would be require a different patch. Right now the code *already* uses 'product=' from the device tree. If everyone is fine I can send a different patch, using 'model=' instead of that. I don't think parsing multiple .dts values makes sense here. Cheers /Ilias > > Hardkernel Odroid C2: > model = "Hardkernel ODROID-C2" > > Orange Pi PC > model = "Xunlong Orange Pi PC" > > Solidrun MacchiatoBIN: > model = "MACCHIATOBin-8040" > > Best regards > > Heinrich > > > --- > > lib/smbios.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/lib/smbios.c b/lib/smbios.c > > index b52e125eeb14..d1997ce70d19 100644 > > --- a/lib/smbios.c > > +++ b/lib/smbios.c > > @@ -79,8 +79,10 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > > int i = 1; > > char *p = ctx->eos; > > > > - if (!*str) > > + if (!*str) { > > str = "Unknown"; > > + log_warning("Empty string found. Please fix your DTS and/or CONFIG_SYS_* options"); > > + } > > > > for (;;) { > > if (!*p) { > > @@ -259,10 +261,10 @@ static int smbios_write_type1(ulong *current, int 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->manufacturer = smbios_add_string(ctx, CONFIG_SYS_VENDOR); > > t->product_name = smbios_add_prop(ctx, "product"); > > if (!t->product_name) > > - t->product_name = smbios_add_string(ctx, "Unknown Product"); > > + t->product_name = smbios_add_string(ctx, CONFIG_SYS_BOARD); > > t->version = smbios_add_prop_si(ctx, "version", > > SYSINFO_ID_SMBIOS_SYSTEM_VERSION); > > if (serial_str) { > > @@ -293,10 +295,10 @@ static int smbios_write_type2(ulong *current, int 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->manufacturer = smbios_add_string(ctx, CONFIG_SYS_VENDOR); > > t->product_name = smbios_add_prop(ctx, "product"); > > if (!t->product_name) > > - t->product_name = smbios_add_string(ctx, "Unknown Product"); > > + t->product_name = smbios_add_string(ctx, CONFIG_SYS_BOARD); > > t->version = smbios_add_prop_si(ctx, "version", > > SYSINFO_ID_SMBIOS_BASEBOARD_VERSION); > > t->asset_tag_number = smbios_add_prop(ctx, "asset-tag"); > > @@ -322,7 +324,7 @@ static int smbios_write_type3(ulong *current, int 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->manufacturer = smbios_add_string(ctx, CONFIG_SYS_VENDOR); > > t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP; > > t->bootup_state = SMBIOS_STATE_SAFE; > > t->power_supply_state = SMBIOS_STATE_SAFE; > > >
On Mon, 5 Jul 2021 at 02:49, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > SMBIOS entries for manufacturer and board can't be empty, as the spec > explicitly requires a value. > Instead of passing "Unknown" and "Unknown product" for the manufacturer and > product name if the .dts options are missing, try to use CONFIG_SYS_VENDOR > and CONFIG_SYS_BOARD respectively. If those are not found either warn the > user at runtime and use "Unknown" for both entries. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > lib/smbios.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org> I'm not quite sure what this is based on, but perhaps the code changed without me noticing. How about adding some tests for this code?
Simon, Bin, As Heinrich points out some of the CONFIG_SYS_* values are not representing the devices properly. Maybe it's a better idea to only check the .dts? If the values are missing we can pop a runtime warning and keep using 'Unknown'. On Mon, Jul 05, 2021 at 09:29:57AM -0600, Simon Glass wrote: > On Mon, 5 Jul 2021 at 02:49, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > SMBIOS entries for manufacturer and board can't be empty, as the spec > > explicitly requires a value. > > Instead of passing "Unknown" and "Unknown product" for the manufacturer and > > product name if the .dts options are missing, try to use CONFIG_SYS_VENDOR > > and CONFIG_SYS_BOARD respectively. If those are not found either warn the > > user at runtime and use "Unknown" for both entries. > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > lib/smbios.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > Reviewed-by: Simon Glass <sjg@chromium.org> > > I'm not quite sure what this is based on, but perhaps the code changed > without me noticing. > > How about adding some tests for this code? How? The only valid test we can do is that the values we expect are not "". But as the code is right now it will always replace "" with "Unknown" Thanks /Ilias
Hi Ilias, On Mon, 5 Jul 2021 at 11:31, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Simon, Bin, > > As Heinrich points out some of the CONFIG_SYS_* values are not representing > the devices properly. Maybe it's a better idea to only check the .dts? > If the values are missing we can pop a runtime warning and keep using > 'Unknown'. I think we should pick one of two options: 1. Fail if we don't have the info 2. Try to use CONFIG_SYS_... as the old code did As mentioned earlier, I actually prefer 1, since then it means boards must add the info in the DT (probably) which indicates that someone has actually thought about it. But if we go with 2, we may as well go all the way, and use all the info at our disposal. > > On Mon, Jul 05, 2021 at 09:29:57AM -0600, Simon Glass wrote: > > On Mon, 5 Jul 2021 at 02:49, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > SMBIOS entries for manufacturer and board can't be empty, as the spec > > > explicitly requires a value. > > > Instead of passing "Unknown" and "Unknown product" for the manufacturer and > > > product name if the .dts options are missing, try to use CONFIG_SYS_VENDOR > > > and CONFIG_SYS_BOARD respectively. If those are not found either warn the > > > user at runtime and use "Unknown" for both entries. > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > --- > > > lib/smbios.c | 14 ++++++++------ > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > I'm not quite sure what this is based on, but perhaps the code changed > > without me noticing. > > > > How about adding some tests for this code? > > How? The only valid test we can do is that the values we expect are not "". > But as the code is right now it will always replace "" with "Unknown" I just mean we need unit tests for generation of smbios tables....it is complicated enough. Regards, Simon
diff --git a/lib/smbios.c b/lib/smbios.c index b52e125eeb14..d1997ce70d19 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -79,8 +79,10 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) int i = 1; char *p = ctx->eos; - if (!*str) + if (!*str) { str = "Unknown"; + log_warning("Empty string found. Please fix your DTS and/or CONFIG_SYS_* options"); + } for (;;) { if (!*p) { @@ -259,10 +261,10 @@ static int smbios_write_type1(ulong *current, int 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->manufacturer = smbios_add_string(ctx, CONFIG_SYS_VENDOR); t->product_name = smbios_add_prop(ctx, "product"); if (!t->product_name) - t->product_name = smbios_add_string(ctx, "Unknown Product"); + t->product_name = smbios_add_string(ctx, CONFIG_SYS_BOARD); t->version = smbios_add_prop_si(ctx, "version", SYSINFO_ID_SMBIOS_SYSTEM_VERSION); if (serial_str) { @@ -293,10 +295,10 @@ static int smbios_write_type2(ulong *current, int 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->manufacturer = smbios_add_string(ctx, CONFIG_SYS_VENDOR); t->product_name = smbios_add_prop(ctx, "product"); if (!t->product_name) - t->product_name = smbios_add_string(ctx, "Unknown Product"); + t->product_name = smbios_add_string(ctx, CONFIG_SYS_BOARD); t->version = smbios_add_prop_si(ctx, "version", SYSINFO_ID_SMBIOS_BASEBOARD_VERSION); t->asset_tag_number = smbios_add_prop(ctx, "asset-tag"); @@ -322,7 +324,7 @@ static int smbios_write_type3(ulong *current, int 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->manufacturer = smbios_add_string(ctx, CONFIG_SYS_VENDOR); t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP; t->bootup_state = SMBIOS_STATE_SAFE; t->power_supply_state = SMBIOS_STATE_SAFE;
SMBIOS entries for manufacturer and board can't be empty, as the spec explicitly requires a value. Instead of passing "Unknown" and "Unknown product" for the manufacturer and product name if the .dts options are missing, try to use CONFIG_SYS_VENDOR and CONFIG_SYS_BOARD respectively. If those are not found either warn the user at runtime and use "Unknown" for both entries. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- lib/smbios.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) -- 2.32.0.rc0