Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
On Thu, Sep 29, 2022 at 10:02:48AM -0400, Sean Anderson wrote:
> On 9/29/22 05:59, Simon Glass wrote:
> > Hi,
> >
> > On Wed, 28 Sept 2022 at 22:34, Sean Anderson wrote:
> > >
> > > On 9/26/22 06:56, Ilias Apalodimas wrote:
> > > > Hi Sean
> > > >
> > > > On Sat, 17 Sept 2022 at 19:55, Sean Anderson wrote:
> > > > >
> > > > > On 9/16/22 16:30, Ilias Apalodimas wrote:
> > > > > > Hi Simon,
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > Signed-off-by: Ilias Apalodimas
> > > > > > > > ---
> > > > > > > > lib/smbios.c | 17 +++--
> > > > > > > > 1 file changed, 3 insertions(+), 14 deletions(-)
> > > > > > >
> > > > > > > Perhaps a better fix is to drop the smbios info?
> > > > > >
> > > > > > Unfortunately there's a ton of userspace tools still using it. So
> > > > > > I think
> > > > > > we still need it
> > > > > >
> > > > > > >
> > > > > > > What upstream projects use this information to show things to the
> > > > > > > user? You showed a screenshot of some sort of system-info app. We
> > > > > > > could teach it about falling back to the device tree. That way we
> > > > > > > are
> > > > > > > not adding fake information to SMBIOS.
> > > > > > >
> > > > > >
> > > > > > What's fake here? The model and compatible are taken directly from
> > > > > > the DT
> > > > > > and that should be accurate. I'd rather fix the DT if that's
> > > > > > problematic.
> > > > > > What would make sense for me to change is take the first token of
> > > > > > the
> > > > > > compatible node instead of the entire string as it's format is
> > > > > > expected to
> > > > > > be anyway.
> > > > >
> > > > > > Manufacturer: socionext,developer-box
> > > > > > Product Name: Socionext Developer Box
> > > > >
> > > > > Well, firstly, the manufacturer is "Socionext", not
> > > > > "socionext,developer-box". Compatibles are not suitable for
> > > > > user-visible identifiers. The product name should also be something
> > > > > like
> > > > > "Socionext Developerbox" or maybe "SynQuacer E-series", but this more
> > > > > of
> > > > > a "bug" in the devicetree model property.
> > > >
> > > > Yea as I said we can get rid of the everything after the ',' on the
> > > > compatible node. Ideally if vendors followed the DT spec, we could
> > > > also just use manufacturer node, the reality is that we can't though.
> > >
> > > This is another one of the problems with this approach. There's no
> > > consistency in existing device trees, because at most this info is
> > > printed in the boot log.
> > >
> > > > The whole point of the patchset is provide something reasonable
> > > > without having to add a .dtsi smbios node for all our devices. We can
> > > > then go back to fixing the DT with proper values if it's a DT "bug".
> > > > >
> > > > > Second, these identifiers are not suitable for all structures you want
> > > > > to use it for. For example, the chassis is really a "INWIN industrial
> > > > > PC
> > > > > case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with
> > > > > 300W
> > > > > SFX power supply" [1]. I would describe this as something like
> > > >
> > > > The chassis isn't even addressed in the series. IIRC it's currently
> > > > hardcoded in smbios.c.
> > >
> > > You showed it as different in the commit message.
> > >
> > > > >
> > > > > Handle 0x0003, DMI type 3, 21 bytes
> > > > > Chassis Information
> > > > >Manufacturer: INWIN
> > > > >Type: Mini Tower
> > > > >Lock: Not Present
> > > > >Version: Unknown
> > > > >Serial Number: Not Specified
> > > > >Asset Tag: Not Specified
> > > > >Boot-up State: Safe
> > > > >Power Supply State: Safe
> > > > >Thermal State: Safe
> > > > >Security Status: None
> > > > >OEM Information: 0x
> > > > >Height: Unspecified
> > > > >Number Of Power Cords: 1
> > > > >Contained Elements: 0
> > > > >
> > > > > The exact values are not particularly important, but I would certainly
> > > > > classify a manufacturer of "socionext,developer-box" as fake. We might
> > > > > not even know what the chassis is; what's to stop a user from using a
> > > > > different case?
> > > >
> > > > But the chassis isn't even addressed in the series? Again I am mostly
> > > > interested in a sane fallback for device and manufacturer.
> > >
> > > ditto
> > >
> > > > >
> > > > > [1]
> > > > > https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-2-3E.pdf
> > > > >
> > > > > > > Also, SMBIOS is a legacy thing and a PITA to work with. How about
> > > > > > > we
> > > > > > > use the device tree binding for the same info:
> > > > > > >
> > > > > > >smbios {
> > > > > > >compatible = "u-boot,sysinfo-smbios";
> > > > > > >
> > > > > > >smbios {
> > > > > > >system {
Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
On 9/29/22 05:59, Simon Glass wrote:
Hi,
On Wed, 28 Sept 2022 at 22:34, Sean Anderson wrote:
On 9/26/22 06:56, Ilias Apalodimas wrote:
Hi Sean
On Sat, 17 Sept 2022 at 19:55, Sean Anderson wrote:
On 9/16/22 16:30, Ilias Apalodimas wrote:
Hi Simon,
[...]
Signed-off-by: Ilias Apalodimas
---
lib/smbios.c | 17 +++--
1 file changed, 3 insertions(+), 14 deletions(-)
Perhaps a better fix is to drop the smbios info?
Unfortunately there's a ton of userspace tools still using it. So I think
we still need it
What upstream projects use this information to show things to the
user? You showed a screenshot of some sort of system-info app. We
could teach it about falling back to the device tree. That way we are
not adding fake information to SMBIOS.
What's fake here? The model and compatible are taken directly from the DT
and that should be accurate. I'd rather fix the DT if that's problematic.
What would make sense for me to change is take the first token of the
compatible node instead of the entire string as it's format is expected to
be anyway.
Manufacturer: socionext,developer-box
Product Name: Socionext Developer Box
Well, firstly, the manufacturer is "Socionext", not
"socionext,developer-box". Compatibles are not suitable for
user-visible identifiers. The product name should also be something like
"Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
a "bug" in the devicetree model property.
Yea as I said we can get rid of the everything after the ',' on the
compatible node. Ideally if vendors followed the DT spec, we could
also just use manufacturer node, the reality is that we can't though.
This is another one of the problems with this approach. There's no
consistency in existing device trees, because at most this info is
printed in the boot log.
The whole point of the patchset is provide something reasonable
without having to add a .dtsi smbios node for all our devices. We can
then go back to fixing the DT with proper values if it's a DT "bug".
Second, these identifiers are not suitable for all structures you want
to use it for. For example, the chassis is really a "INWIN industrial PC
case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
SFX power supply" [1]. I would describe this as something like
The chassis isn't even addressed in the series. IIRC it's currently
hardcoded in smbios.c.
You showed it as different in the commit message.
Handle 0x0003, DMI type 3, 21 bytes
Chassis Information
Manufacturer: INWIN
Type: Mini Tower
Lock: Not Present
Version: Unknown
Serial Number: Not Specified
Asset Tag: Not Specified
Boot-up State: Safe
Power Supply State: Safe
Thermal State: Safe
Security Status: None
OEM Information: 0x
Height: Unspecified
Number Of Power Cords: 1
Contained Elements: 0
The exact values are not particularly important, but I would certainly
classify a manufacturer of "socionext,developer-box" as fake. We might
not even know what the chassis is; what's to stop a user from using a
different case?
But the chassis isn't even addressed in the series? Again I am mostly
interested in a sane fallback for device and manufacturer.
ditto
[1]
https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-2-3E.pdf
Also, SMBIOS is a legacy thing and a PITA to work with. How about we
use the device tree binding for the same info:
smbios {
compatible = "u-boot,sysinfo-smbios";
smbios {
system {
manufacturer = "pine64";
product = "rock64_rk3328";
};
baseboard {
manufacturer = "pine64";
product = "rock64_rk3328";
};
chassis {
manufacturer = "pine64";
product = "rock64_rk3328";
};
};
};
This is easy to parse and gets us away from all this legacy junk that
we don't need.
That's the exact opposite of the patch description. Most of these info are
already included in the DT in it's standard properties. So if U-Boot ends
up with a DT without these we get a usable smbios table. For example a DT
handed over by the previous stage bootloader would not include these nodes.
I agree. I think a better example would fill in these fields with
descriptive values.
We are off to a chicken and egg problem now. Can you provide U-Boot
with a 'configuration' DT, which would be disjoint from the DT that
describes hardware?
Sorry, I misread the context there.
I still don't think this is the right approach for this... better to fix
the prior stage's devicetree.
As far as sysinfo-smbios node is concerned, it's only present in 13
boards, so it's not l
Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
Hi Sean,
On Thu, Sep 29, 2022 at 12:34:37AM -0400, Sean Anderson wrote:
> On 9/26/22 06:56, Ilias Apalodimas wrote:
> > Hi Sean
> >
> > On Sat, 17 Sept 2022 at 19:55, Sean Anderson wrote:
> > >
> > > On 9/16/22 16:30, Ilias Apalodimas wrote:
> > > > Hi Simon,
> > > >
> > > > [...]
> > > >
> > > > > > Signed-off-by: Ilias Apalodimas
> > > > > > ---
> > > > > >lib/smbios.c | 17 +++--
> > > > > >1 file changed, 3 insertions(+), 14 deletions(-)
> > > > >
> > > > > Perhaps a better fix is to drop the smbios info?
> > > >
> > > > Unfortunately there's a ton of userspace tools still using it. So I
> > > > think
> > > > we still need it
> > > >
> > > > >
> > > > > What upstream projects use this information to show things to the
> > > > > user? You showed a screenshot of some sort of system-info app. We
> > > > > could teach it about falling back to the device tree. That way we are
> > > > > not adding fake information to SMBIOS.
> > > > >
> > > >
> > > > What's fake here? The model and compatible are taken directly from the
> > > > DT
> > > > and that should be accurate. I'd rather fix the DT if that's
> > > > problematic.
> > > > What would make sense for me to change is take the first token of the
> > > > compatible node instead of the entire string as it's format is expected
> > > > to
> > > > be anyway.
> > >
> > > > Manufacturer: socionext,developer-box
> > > > Product Name: Socionext Developer Box
> > >
> > > Well, firstly, the manufacturer is "Socionext", not
> > > "socionext,developer-box". Compatibles are not suitable for
> > > user-visible identifiers. The product name should also be something like
> > > "Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
> > > a "bug" in the devicetree model property.
> >
> > Yea as I said we can get rid of the everything after the ',' on the
> > compatible node. Ideally if vendors followed the DT spec, we could
> > also just use manufacturer node, the reality is that we can't though.
>
> This is another one of the problems with this approach. There's no
> consistency in existing device trees, because at most this info is
> printed in the boot log.
I see these 2 as completely disjoint problems tbh. The approach says
"Let's use what the DT spec suggests to derive values that make sense as a
last resort". The fact that some DTs decide to do differently is a side
effect. But greping into DT's a bit all of the 'model' seems sane and all
the values before the first ',' as well.
>
> > The whole point of the patchset is provide something reasonable
> > without having to add a .dtsi smbios node for all our devices. We can
> > then go back to fixing the DT with proper values if it's a DT "bug".
> > >
> > > Second, these identifiers are not suitable for all structures you want
> > > to use it for. For example, the chassis is really a "INWIN industrial PC
> > > case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
> > > SFX power supply" [1]. I would describe this as something like
> >
> > The chassis isn't even addressed in the series. IIRC it's currently
> > hardcoded in smbios.c.
>
> You showed it as different in the commit message.
Not on the commit message. Maybe you remember a discussion over IRC?
In any case I think this is a moot argument. Whether we parse the chassis
from the DT or the sysinfo-smbios in the DT someone still has to change the
*text* if he changes the enclosure. So I don't really see any big
difference here.
>
> > >
> > > Handle 0x0003, DMI type 3, 21 bytes
> > > Chassis Information
> > > Manufacturer: INWIN
> > > Type: Mini Tower
> > > Lock: Not Present
> > > Version: Unknown
> > > Serial Number: Not Specified
> > > Asset Tag: Not Specified
> > > Boot-up State: Safe
> > > Power Supply State: Safe
> > > Thermal State: Safe
> > > Security Status: None
> > > OEM Information: 0x
> > > Height: Unspecified
> > > Number Of Power Cords: 1
> > > Contained Elements: 0
> > >
> > > The exact values are not particularly important, but I would certainly
> > > classify a manufacturer of "socionext,developer-box" as fake. We might
> > > not even know what the chassis is; what's to stop a user from using a
> > > different case?
> >
> > But the chassis isn't even addressed in the series? Again I am mostly
> > interested in a sane fallback for device and manufacturer.
>
> ditto
>
> > >
> > > [1]
> > > https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-2-3E.pdf
> > >
> > > > > Also, SMBIOS is a legacy thing and a PITA to work with. How about we
> > > > > use the device tree binding for the same info:
> > > > >
> > > > > smbios {
> > > > > compatible = "u-boot,sysinfo-smbios";
> > > > >
> > > > > smbios {
> > > > > system {
>
Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
Hi,
On Wed, 28 Sept 2022 at 22:34, Sean Anderson wrote:
>
> On 9/26/22 06:56, Ilias Apalodimas wrote:
> > Hi Sean
> >
> > On Sat, 17 Sept 2022 at 19:55, Sean Anderson wrote:
> >>
> >> On 9/16/22 16:30, Ilias Apalodimas wrote:
> >>> Hi Simon,
> >>>
> >>> [...]
> >>>
> > Signed-off-by: Ilias Apalodimas
> > ---
> >lib/smbios.c | 17 +++--
> >1 file changed, 3 insertions(+), 14 deletions(-)
>
> Perhaps a better fix is to drop the smbios info?
> >>>
> >>> Unfortunately there's a ton of userspace tools still using it. So I think
> >>> we still need it
> >>>
>
> What upstream projects use this information to show things to the
> user? You showed a screenshot of some sort of system-info app. We
> could teach it about falling back to the device tree. That way we are
> not adding fake information to SMBIOS.
>
> >>>
> >>> What's fake here? The model and compatible are taken directly from the DT
> >>> and that should be accurate. I'd rather fix the DT if that's problematic.
> >>> What would make sense for me to change is take the first token of the
> >>> compatible node instead of the entire string as it's format is expected to
> >>> be anyway.
> >>
> >>> Manufacturer: socionext,developer-box
> >>> Product Name: Socionext Developer Box
> >>
> >> Well, firstly, the manufacturer is "Socionext", not
> >> "socionext,developer-box". Compatibles are not suitable for
> >> user-visible identifiers. The product name should also be something like
> >> "Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
> >> a "bug" in the devicetree model property.
> >
> > Yea as I said we can get rid of the everything after the ',' on the
> > compatible node. Ideally if vendors followed the DT spec, we could
> > also just use manufacturer node, the reality is that we can't though.
>
> This is another one of the problems with this approach. There's no
> consistency in existing device trees, because at most this info is
> printed in the boot log.
>
> > The whole point of the patchset is provide something reasonable
> > without having to add a .dtsi smbios node for all our devices. We can
> > then go back to fixing the DT with proper values if it's a DT "bug".
> >>
> >> Second, these identifiers are not suitable for all structures you want
> >> to use it for. For example, the chassis is really a "INWIN industrial PC
> >> case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
> >> SFX power supply" [1]. I would describe this as something like
> >
> > The chassis isn't even addressed in the series. IIRC it's currently
> > hardcoded in smbios.c.
>
> You showed it as different in the commit message.
>
> >>
> >> Handle 0x0003, DMI type 3, 21 bytes
> >> Chassis Information
> >> Manufacturer: INWIN
> >> Type: Mini Tower
> >> Lock: Not Present
> >> Version: Unknown
> >> Serial Number: Not Specified
> >> Asset Tag: Not Specified
> >> Boot-up State: Safe
> >> Power Supply State: Safe
> >> Thermal State: Safe
> >> Security Status: None
> >> OEM Information: 0x
> >> Height: Unspecified
> >> Number Of Power Cords: 1
> >> Contained Elements: 0
> >>
> >> The exact values are not particularly important, but I would certainly
> >> classify a manufacturer of "socionext,developer-box" as fake. We might
> >> not even know what the chassis is; what's to stop a user from using a
> >> different case?
> >
> > But the chassis isn't even addressed in the series? Again I am mostly
> > interested in a sane fallback for device and manufacturer.
>
> ditto
>
> >>
> >> [1]
> >> https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-2-3E.pdf
> >>
> Also, SMBIOS is a legacy thing and a PITA to work with. How about we
> use the device tree binding for the same info:
>
> smbios {
> compatible = "u-boot,sysinfo-smbios";
>
> smbios {
> system {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
>
> baseboard {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
>
> chassis {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
> };
> };
>
> This is easy to parse and gets us away from all this legacy junk that
> we don't need.
> >>>
> >>> That's the exact opposite of the patch description. Most of these info
> >>> are
> >>> already included in the DT in it's standard properties. So if U-Boot ends
> >>> up with a DT without these we get a usable smbios table. For example a DT
> >>
Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
On 9/26/22 06:56, Ilias Apalodimas wrote:
Hi Sean
On Sat, 17 Sept 2022 at 19:55, Sean Anderson wrote:
On 9/16/22 16:30, Ilias Apalodimas wrote:
Hi Simon,
[...]
Signed-off-by: Ilias Apalodimas
---
lib/smbios.c | 17 +++--
1 file changed, 3 insertions(+), 14 deletions(-)
Perhaps a better fix is to drop the smbios info?
Unfortunately there's a ton of userspace tools still using it. So I think
we still need it
What upstream projects use this information to show things to the
user? You showed a screenshot of some sort of system-info app. We
could teach it about falling back to the device tree. That way we are
not adding fake information to SMBIOS.
What's fake here? The model and compatible are taken directly from the DT
and that should be accurate. I'd rather fix the DT if that's problematic.
What would make sense for me to change is take the first token of the
compatible node instead of the entire string as it's format is expected to
be anyway.
Manufacturer: socionext,developer-box
Product Name: Socionext Developer Box
Well, firstly, the manufacturer is "Socionext", not
"socionext,developer-box". Compatibles are not suitable for
user-visible identifiers. The product name should also be something like
"Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
a "bug" in the devicetree model property.
Yea as I said we can get rid of the everything after the ',' on the
compatible node. Ideally if vendors followed the DT spec, we could
also just use manufacturer node, the reality is that we can't though.
This is another one of the problems with this approach. There's no
consistency in existing device trees, because at most this info is
printed in the boot log.
The whole point of the patchset is provide something reasonable
without having to add a .dtsi smbios node for all our devices. We can
then go back to fixing the DT with proper values if it's a DT "bug".
Second, these identifiers are not suitable for all structures you want
to use it for. For example, the chassis is really a "INWIN industrial PC
case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
SFX power supply" [1]. I would describe this as something like
The chassis isn't even addressed in the series. IIRC it's currently
hardcoded in smbios.c.
You showed it as different in the commit message.
Handle 0x0003, DMI type 3, 21 bytes
Chassis Information
Manufacturer: INWIN
Type: Mini Tower
Lock: Not Present
Version: Unknown
Serial Number: Not Specified
Asset Tag: Not Specified
Boot-up State: Safe
Power Supply State: Safe
Thermal State: Safe
Security Status: None
OEM Information: 0x
Height: Unspecified
Number Of Power Cords: 1
Contained Elements: 0
The exact values are not particularly important, but I would certainly
classify a manufacturer of "socionext,developer-box" as fake. We might
not even know what the chassis is; what's to stop a user from using a
different case?
But the chassis isn't even addressed in the series? Again I am mostly
interested in a sane fallback for device and manufacturer.
ditto
[1]
https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-2-3E.pdf
Also, SMBIOS is a legacy thing and a PITA to work with. How about we
use the device tree binding for the same info:
smbios {
compatible = "u-boot,sysinfo-smbios";
smbios {
system {
manufacturer = "pine64";
product = "rock64_rk3328";
};
baseboard {
manufacturer = "pine64";
product = "rock64_rk3328";
};
chassis {
manufacturer = "pine64";
product = "rock64_rk3328";
};
};
};
This is easy to parse and gets us away from all this legacy junk that
we don't need.
That's the exact opposite of the patch description. Most of these info are
already included in the DT in it's standard properties. So if U-Boot ends
up with a DT without these we get a usable smbios table. For example a DT
handed over by the previous stage bootloader would not include these nodes.
I agree. I think a better example would fill in these fields with
descriptive values.
We are off to a chicken and egg problem now. Can you provide U-Boot
with a 'configuration' DT, which would be disjoint from the DT that
describes hardware?
Sorry, I misread the context there.
I still don't think this is the right approach for this... better to fix
the prior stage's devicetree.
As far as sysinfo-smbios node is concerned, it's only present in 13
boards, so it's not like it's used by the majority of boards. Yes we
could fix them, but imho we are better off re-using what's already there
and define
Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
Hi Sean
On Sat, 17 Sept 2022 at 19:55, Sean Anderson wrote:
>
> On 9/16/22 16:30, Ilias Apalodimas wrote:
> > Hi Simon,
> >
> > [...]
> >
> >>> Signed-off-by: Ilias Apalodimas
> >>> ---
> >>> lib/smbios.c | 17 +++--
> >>> 1 file changed, 3 insertions(+), 14 deletions(-)
> >>
> >> Perhaps a better fix is to drop the smbios info?
> >
> > Unfortunately there's a ton of userspace tools still using it. So I think
> > we still need it
> >
> >>
> >> What upstream projects use this information to show things to the
> >> user? You showed a screenshot of some sort of system-info app. We
> >> could teach it about falling back to the device tree. That way we are
> >> not adding fake information to SMBIOS.
> >>
> >
> > What's fake here? The model and compatible are taken directly from the DT
> > and that should be accurate. I'd rather fix the DT if that's problematic.
> > What would make sense for me to change is take the first token of the
> > compatible node instead of the entire string as it's format is expected to
> > be anyway.
>
> > Manufacturer: socionext,developer-box
> > Product Name: Socionext Developer Box
>
> Well, firstly, the manufacturer is "Socionext", not
> "socionext,developer-box". Compatibles are not suitable for
> user-visible identifiers. The product name should also be something like
> "Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
> a "bug" in the devicetree model property.
Yea as I said we can get rid of the everything after the ',' on the
compatible node. Ideally if vendors followed the DT spec, we could
also just use manufacturer node, the reality is that we can't though.
The whole point of the patchset is provide something reasonable
without having to add a .dtsi smbios node for all our devices. We can
then go back to fixing the DT with proper values if it's a DT "bug".
>
> Second, these identifiers are not suitable for all structures you want
> to use it for. For example, the chassis is really a "INWIN industrial PC
> case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
> SFX power supply" [1]. I would describe this as something like
The chassis isn't even addressed in the series. IIRC it's currently
hardcoded in smbios.c.
>
> Handle 0x0003, DMI type 3, 21 bytes
> Chassis Information
> Manufacturer: INWIN
> Type: Mini Tower
> Lock: Not Present
> Version: Unknown
> Serial Number: Not Specified
> Asset Tag: Not Specified
> Boot-up State: Safe
> Power Supply State: Safe
> Thermal State: Safe
> Security Status: None
> OEM Information: 0x
> Height: Unspecified
> Number Of Power Cords: 1
> Contained Elements: 0
>
> The exact values are not particularly important, but I would certainly
> classify a manufacturer of "socionext,developer-box" as fake. We might
> not even know what the chassis is; what's to stop a user from using a
> different case?
But the chassis isn't even addressed in the series? Again I am mostly
interested in a sane fallback for device and manufacturer.
>
> [1]
> https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-2-3E.pdf
>
> >> Also, SMBIOS is a legacy thing and a PITA to work with. How about we
> >> use the device tree binding for the same info:
> >>
> >> smbios {
> >> compatible = "u-boot,sysinfo-smbios";
> >>
> >> smbios {
> >> system {
> >> manufacturer = "pine64";
> >> product = "rock64_rk3328";
> >> };
> >>
> >> baseboard {
> >> manufacturer = "pine64";
> >> product = "rock64_rk3328";
> >> };
> >>
> >> chassis {
> >> manufacturer = "pine64";
> >> product = "rock64_rk3328";
> >> };
> >> };
> >> };
> >>
> >> This is easy to parse and gets us away from all this legacy junk that
> >> we don't need.
> >
> > That's the exact opposite of the patch description. Most of these info are
> > already included in the DT in it's standard properties. So if U-Boot ends
> > up with a DT without these we get a usable smbios table. For example a DT
> > handed over by the previous stage bootloader would not include these nodes.
>
> I agree. I think a better example would fill in these fields with
> descriptive values.
We are off to a chicken and egg problem now. Can you provide U-Boot
with a 'configuration' DT, which would be disjoint from the DT that
describes hardware?
>
> > As far as sysinfo-smbios node is concerned, it's only present in 13
> > boards, so it's not like it's used by the majority of boards. Yes we
> > could fix them, but imho we are better off re-using what's already there
> > and defined on the DT spec at least for the simplistic values.
>
> IMO SYS_VENDOR and SYS_BOARD are more descriptive
Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
On Tue, Sep 6, 2022 at 2:44 PM Ilias Apalodimas
wrote:
>
> If a value is not valid during the DT or SYSINFO parsing, we explicitly
> set that to "Unknown Product" and "Unknown" for the product and
> manufacturer respectively. It's cleaner if we move the checks insisde
> smbios_add_string() and always report "Unknown" regardless of the missing
> field.
The data below probably can go below the --- as I'm not sure it's
useful in the final commit.
> pre-patch dmidecode
>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
> Manufacturer: Unknown
> Product Name: Unknown Product
> Version: Not Specified
> Serial Number: Not Specified
> UUID: Not Settable
> Wake-up Type: Reserved
> SKU Number: Not Specified
> Family: Not Specified
>
> Handle 0x0002, DMI type 2, 14 bytes
> Base Board Information
> Manufacturer: Unknown
> Product Name: Unknown Product
> Version: Not Specified
> Serial Number: Not Specified
> Asset Tag: Not Specified
> Features:
> Board is a hosting board
> Location In Chassis: Not Specified
> Chassis Handle: 0x
> Type: Motherboard
>
>
> post-patch dmidecode:
>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
> Manufacturer: Unknown
> Product Name: Unknown
> Version: Unknown
> Serial Number: Unknown
> UUID: Not Settable
> Wake-up Type: Reserved
> SKU Number: Unknown
> Family: Unknown
>
> Handle 0x0002, DMI type 2, 14 bytes
> Base Board Information
> Manufacturer: Unknown
> Product Name: Unknown
> Version: Unknown
> Serial Number: Not Specified
> Asset Tag: Unknown
> Features:
> Board is a hosting board
> Location In Chassis: Not Specified
> Chassis Handle: 0x
> Type: Motherboard
>
> Signed-off-by: Ilias Apalodimas
Reviewed-by: Peter Robinson
Tested-by: Peter Robinson
> ---
> lib/smbios.c | 17 +++--
> 1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/lib/smbios.c b/lib/smbios.c
> index d7f4999e8b2a..fcc8686993ef 100644
> --- a/lib/smbios.c
> +++ b/lib/smbios.c
> @@ -102,7 +102,7 @@ static int smbios_add_string(struct smbios_ctx *ctx,
> const char *str)
> int i = 1;
> char *p = ctx->eos;
>
> - if (!*str)
> + if (!str || !*str)
> str = "Unknown";
>
> for (;;) {
> @@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx,
> const char *prop,
> const char *str;
>
> str = ofnode_read_string(ctx->node, prop);
> - if (str)
> - return smbios_add_string(ctx, str);
> + return smbios_add_string(ctx, str);
> }
>
> return 0;
> @@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle,
> t->vendor = smbios_add_string(ctx, "U-Boot");
>
> t->bios_ver = smbios_add_prop(ctx, "version");
> - if (!t->bios_ver)
> + if (!strcmp(ctx->last_str, "Unknown"))
> t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION);
> if (t->bios_ver)
> gd->smbios_version = ctx->last_str;
> @@ -281,11 +280,7 @@ 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) {
> @@ -315,11 +310,7 @@ 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");
> @@ -344,8 +335,6 @@ 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, "manu
Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
Hi Simon,
Adding Rob for information around putting things in device tree.
> > If a value is not valid during the DT or SYSINFO parsing, we explicitly
> > set that to "Unknown Product" and "Unknown" for the product and
> > manufacturer respectively. It's cleaner if we move the checks insisde
> > smbios_add_string() and always report "Unknown" regardless of the missing
> > field.
> >
> > pre-patch dmidecode
> >
> > Handle 0x0001, DMI type 1, 27 bytes
> > System Information
> > Manufacturer: Unknown
> > Product Name: Unknown Product
> > Version: Not Specified
> > Serial Number: Not Specified
> > UUID: Not Settable
> > Wake-up Type: Reserved
> > SKU Number: Not Specified
> > Family: Not Specified
> >
> > Handle 0x0002, DMI type 2, 14 bytes
> > Base Board Information
> > Manufacturer: Unknown
> > Product Name: Unknown Product
> > Version: Not Specified
> > Serial Number: Not Specified
> > Asset Tag: Not Specified
> > Features:
> > Board is a hosting board
> > Location In Chassis: Not Specified
> > Type: Motherboard
> >
> >
> > post-patch dmidecode:
> >
> > Handle 0x0001, DMI type 1, 27 bytes
> > System Information
> > Manufacturer: Unknown
> > Product Name: Unknown
> > Version: Unknown
> > Serial Number: Unknown
> > UUID: Not Settable
> > Wake-up Type: Reserved
> > SKU Number: Unknown
> > Family: Unknown
> >
> > Handle 0x0002, DMI type 2, 14 bytes
> > Base Board Information
> > Manufacturer: Unknown
> > Product Name: Unknown
> > Version: Unknown
> > Serial Number: Not Specified
> > Asset Tag: Unknown
> > Features:
> > Board is a hosting board
> > Location In Chassis: Not Specified
> > Chassis Handle: 0x
> > Type: Motherboard
> >
> > Signed-off-by: Ilias Apalodimas
> > ---
> > lib/smbios.c | 17 +++--
> > 1 file changed, 3 insertions(+), 14 deletions(-)
>
> Perhaps a better fix is to drop the smbios info?
>
> What upstream projects use this information to show things to the
> user? You showed a screenshot of some sort of system-info app. We
> could teach it about falling back to the device tree. That way we are
> not adding fake information to SMBIOS.
Lots of things use it, particularly for enterprise support platforms.
The system-info apps you mention above are the GNOME about dialog box
and a tool called neofetch (which I personally don't particularly care
for but people like it for some reason).
There's general tools like dmidecode that use the information, but
there's a LOT of open source tools that use the SMBIOS information,
most of the Desktop UX About or HW tools, a lot of support tools like
sosreport (https://github.com/sosreport/sos), and server management
tools like cockpit (https://cockpit-project.org/) and uncountable
proprietary tools.
The problem with putting these things into Device Tree is that they're
not really describing the hardware explicitly and it's U-Boot specific
and Rob has mentioned in the past we absolutely shouldn't be making
things up that don't belong in Device Tree.
> Also, SMBIOS is a legacy thing and a PITA to work with. How about we
> use the device tree binding for the same info:
You say it's legacy, yet it released a new 3.6.0 version in June this
year and is used extensively across x86 and is required for some of
the Arm SystemReady standards. Doesn't appear particularly legacy to
me.
While I don't particularly like SMBIOS either it's the standard and
widely used, I don't see it going anywhere soon. The advantage of it
from someone who had to deal with end to end I feel it's better to
support this because patching all the various projects to support
other things and getting them deployed and enterprises and third
parties to upgrade and integrate into their platforms and processes.
Which from experience takes an extremely long time!
> smbios {
> compatible = "u-boot,sysinfo-smbios";
>
> smbios {
> system {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
>
> baseboard {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
>
> chassis {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
> };
> };
>
> This is easy to parse and gets us away from all this legacy junk that
> we don't need.
I don't see this is any more pretty or any better than getting the
information from Device Tree, moving forward I think it would also be
useful to populate things like serial numbers from the fuse/otp blocks
which we often already display either on the console during boot for
via various commands.
Overall I think we need to be practical, this
Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
On 9/16/22 16:30, Ilias Apalodimas wrote:
Hi Simon,
[...]
Signed-off-by: Ilias Apalodimas
---
lib/smbios.c | 17 +++--
1 file changed, 3 insertions(+), 14 deletions(-)
Perhaps a better fix is to drop the smbios info?
Unfortunately there's a ton of userspace tools still using it. So I think
we still need it
What upstream projects use this information to show things to the
user? You showed a screenshot of some sort of system-info app. We
could teach it about falling back to the device tree. That way we are
not adding fake information to SMBIOS.
What's fake here? The model and compatible are taken directly from the DT
and that should be accurate. I'd rather fix the DT if that's problematic.
What would make sense for me to change is take the first token of the
compatible node instead of the entire string as it's format is expected to
be anyway.
Manufacturer: socionext,developer-box
Product Name: Socionext Developer Box
Well, firstly, the manufacturer is "Socionext", not
"socionext,developer-box". Compatibles are not suitable for
user-visible identifiers. The product name should also be something like
"Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
a "bug" in the devicetree model property.
Second, these identifiers are not suitable for all structures you want
to use it for. For example, the chassis is really a "INWIN industrial PC
case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
SFX power supply" [1]. I would describe this as something like
Handle 0x0003, DMI type 3, 21 bytes
Chassis Information
Manufacturer: INWIN
Type: Mini Tower
Lock: Not Present
Version: Unknown
Serial Number: Not Specified
Asset Tag: Not Specified
Boot-up State: Safe
Power Supply State: Safe
Thermal State: Safe
Security Status: None
OEM Information: 0x
Height: Unspecified
Number Of Power Cords: 1
Contained Elements: 0
The exact values are not particularly important, but I would certainly
classify a manufacturer of "socionext,developer-box" as fake. We might
not even know what the chassis is; what's to stop a user from using a
different case?
[1]
https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-2-3E.pdf
Also, SMBIOS is a legacy thing and a PITA to work with. How about we
use the device tree binding for the same info:
smbios {
compatible = "u-boot,sysinfo-smbios";
smbios {
system {
manufacturer = "pine64";
product = "rock64_rk3328";
};
baseboard {
manufacturer = "pine64";
product = "rock64_rk3328";
};
chassis {
manufacturer = "pine64";
product = "rock64_rk3328";
};
};
};
This is easy to parse and gets us away from all this legacy junk that
we don't need.
That's the exact opposite of the patch description. Most of these info are
already included in the DT in it's standard properties. So if U-Boot ends
up with a DT without these we get a usable smbios table. For example a DT
handed over by the previous stage bootloader would not include these nodes.
I agree. I think a better example would fill in these fields with
descriptive values.
As far as sysinfo-smbios node is concerned, it's only present in 13
boards, so it's not like it's used by the majority of boards. Yes we
could fix them, but imho we are better off re-using what's already there
and defined on the DT spec at least for the simplistic values.
IMO SYS_VENDOR and SYS_BOARD are more descriptive than the devicetree
values, but neither is good...
How many boards do we have which actually use the SMBIOS tables? There
are a lot of boards with EFI_LOADER enabled by default, but I suspect
most never boot anything EFI.
--Sean
Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
Hi Simon,
[...]
> > Signed-off-by: Ilias Apalodimas
> > ---
> > lib/smbios.c | 17 +++--
> > 1 file changed, 3 insertions(+), 14 deletions(-)
>
> Perhaps a better fix is to drop the smbios info?
Unfortunately there's a ton of userspace tools still using it. So I think
we still need it
>
> What upstream projects use this information to show things to the
> user? You showed a screenshot of some sort of system-info app. We
> could teach it about falling back to the device tree. That way we are
> not adding fake information to SMBIOS.
>
What's fake here? The model and compatible are taken directly from the DT
and that should be accurate. I'd rather fix the DT if that's problematic.
What would make sense for me to change is take the first token of the
compatible node instead of the entire string as it's format is expected to
be anyway.
> Also, SMBIOS is a legacy thing and a PITA to work with. How about we
> use the device tree binding for the same info:
>
> smbios {
> compatible = "u-boot,sysinfo-smbios";
>
> smbios {
> system {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
>
> baseboard {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
>
> chassis {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
> };
> };
>
> This is easy to parse and gets us away from all this legacy junk that
> we don't need.
That's the exact opposite of the patch description. Most of these info are
already included in the DT in it's standard properties. So if U-Boot ends
up with a DT without these we get a usable smbios table. For example a DT
handed over by the previous stage bootloader would not include these nodes.
As far as sysinfo-smbios node is concerned, it's only present in 13
boards, so it's not like it's used by the majority of boards. Yes we
could fix them, but imho we are better off re-using what's already there
and defined on the DT spec at least for the simplistic values.
Thanks
/Ilias
>
> Regards,
> Simon
Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
Hi Ilias,
On Tue, 6 Sept 2022 at 07:44, Ilias Apalodimas
wrote:
>
> If a value is not valid during the DT or SYSINFO parsing, we explicitly
> set that to "Unknown Product" and "Unknown" for the product and
> manufacturer respectively. It's cleaner if we move the checks insisde
> smbios_add_string() and always report "Unknown" regardless of the missing
> field.
>
> pre-patch dmidecode
>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
> Manufacturer: Unknown
> Product Name: Unknown Product
> Version: Not Specified
> Serial Number: Not Specified
> UUID: Not Settable
> Wake-up Type: Reserved
> SKU Number: Not Specified
> Family: Not Specified
>
> Handle 0x0002, DMI type 2, 14 bytes
> Base Board Information
> Manufacturer: Unknown
> Product Name: Unknown Product
> Version: Not Specified
> Serial Number: Not Specified
> Asset Tag: Not Specified
> Features:
> Board is a hosting board
> Location In Chassis: Not Specified
> Chassis Handle: 0x
> Type: Motherboard
>
>
> post-patch dmidecode:
>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
> Manufacturer: Unknown
> Product Name: Unknown
> Version: Unknown
> Serial Number: Unknown
> UUID: Not Settable
> Wake-up Type: Reserved
> SKU Number: Unknown
> Family: Unknown
>
> Handle 0x0002, DMI type 2, 14 bytes
> Base Board Information
> Manufacturer: Unknown
> Product Name: Unknown
> Version: Unknown
> Serial Number: Not Specified
> Asset Tag: Unknown
> Features:
> Board is a hosting board
> Location In Chassis: Not Specified
> Chassis Handle: 0x
> Type: Motherboard
>
> Signed-off-by: Ilias Apalodimas
> ---
> lib/smbios.c | 17 +++--
> 1 file changed, 3 insertions(+), 14 deletions(-)
Perhaps a better fix is to drop the smbios info?
What upstream projects use this information to show things to the
user? You showed a screenshot of some sort of system-info app. We
could teach it about falling back to the device tree. That way we are
not adding fake information to SMBIOS.
Also, SMBIOS is a legacy thing and a PITA to work with. How about we
use the device tree binding for the same info:
smbios {
compatible = "u-boot,sysinfo-smbios";
smbios {
system {
manufacturer = "pine64";
product = "rock64_rk3328";
};
baseboard {
manufacturer = "pine64";
product = "rock64_rk3328";
};
chassis {
manufacturer = "pine64";
product = "rock64_rk3328";
};
};
};
This is easy to parse and gets us away from all this legacy junk that
we don't need.
Regards,
Simon
[PATCH 1/2] smbios: Simplify reporting of unknown values
If a value is not valid during the DT or SYSINFO parsing, we explicitly
set that to "Unknown Product" and "Unknown" for the product and
manufacturer respectively. It's cleaner if we move the checks insisde
smbios_add_string() and always report "Unknown" regardless of the missing
field.
pre-patch dmidecode
Handle 0x0001, DMI type 1, 27 bytes
System Information
Manufacturer: Unknown
Product Name: Unknown Product
Version: Not Specified
Serial Number: Not Specified
UUID: Not Settable
Wake-up Type: Reserved
SKU Number: Not Specified
Family: Not Specified
Handle 0x0002, DMI type 2, 14 bytes
Base Board Information
Manufacturer: Unknown
Product Name: Unknown Product
Version: Not Specified
Serial Number: Not Specified
Asset Tag: Not Specified
Features:
Board is a hosting board
Location In Chassis: Not Specified
Chassis Handle: 0x
Type: Motherboard
post-patch dmidecode:
Handle 0x0001, DMI type 1, 27 bytes
System Information
Manufacturer: Unknown
Product Name: Unknown
Version: Unknown
Serial Number: Unknown
UUID: Not Settable
Wake-up Type: Reserved
SKU Number: Unknown
Family: Unknown
Handle 0x0002, DMI type 2, 14 bytes
Base Board Information
Manufacturer: Unknown
Product Name: Unknown
Version: Unknown
Serial Number: Not Specified
Asset Tag: Unknown
Features:
Board is a hosting board
Location In Chassis: Not Specified
Chassis Handle: 0x
Type: Motherboard
Signed-off-by: Ilias Apalodimas
---
lib/smbios.c | 17 +++--
1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/lib/smbios.c b/lib/smbios.c
index d7f4999e8b2a..fcc8686993ef 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -102,7 +102,7 @@ static int smbios_add_string(struct smbios_ctx *ctx, const
char *str)
int i = 1;
char *p = ctx->eos;
- if (!*str)
+ if (!str || !*str)
str = "Unknown";
for (;;) {
@@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const
char *prop,
const char *str;
str = ofnode_read_string(ctx->node, prop);
- if (str)
- return smbios_add_string(ctx, str);
+ return smbios_add_string(ctx, str);
}
return 0;
@@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle,
t->vendor = smbios_add_string(ctx, "U-Boot");
t->bios_ver = smbios_add_prop(ctx, "version");
- if (!t->bios_ver)
+ if (!strcmp(ctx->last_str, "Unknown"))
t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION);
if (t->bios_ver)
gd->smbios_version = ctx->last_str;
@@ -281,11 +280,7 @@ 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) {
@@ -315,11 +310,7 @@ 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");
@@ -344,8 +335,6 @@ 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.37.2
