Re: [dmidecode] [Patch v2 4/4] dmidecode: add dmi sysfs support

[Ivan Khoronzhuk]

Hi Jean,

On 02/26/2015 02:09 PM, Jean Delvare wrote:
> Hi Ivan,
>
> On Wed, 25 Feb 2015 17:56:46 +0200, Ivan Khoronzhuk wrote:
> > In cases when access to /dev/mem is forbidden the dmi sysfs can
> > be used. This patch adds dmi sysfs support.
> >
> > To add support of dmi sysfs the newly introduced dmifs library were
> > used - libdmifs. Currently it's situated at
> > https://git.linaro.org/people/ivan.khoronzhuk/libdmifs.git
> >
> > The library is needed to simplify working with DMI sysfs.
> >
> > Reported-by: Leif Lindholm <address@hidden>
> > Signed-off-by: Ivan Khoronzhuk <address@hidden>
> > ---
> >   Makefile    |  2 +-
> >   dmidecode.c | 31 ++++++++++++++++++++++++++++++-
> >   version.h   |  2 +-
> >   3 files changed, 32 insertions(+), 3 deletions(-)
> > (...)
> Out of curiosity, I fetched the latest version of libdmifs and tried
> this patch set. I compared the new output with what dmidecode would
> output before and I get the following difference:

I've compared it also, and didn't find difference.
I'd used kernel 3.13.
So I have reasonable questions about how it was checked
in order to find out the issue, if such presents.

> --- /tmp/dmidecode.old  2015-02-26 12:23:17.624968398 +0100
> +++ /tmp/dmidecode.new  2015-02-26 12:25:05.643322338 +0100
> @@ -813,6 +813,7 @@
>         Base Address: 0x0000000000000CA2 (Memory-mapped)
>         Register Spacing: Successive Byte Boundaries
>   
> -Handle 0x0042, DMI type 127, 4 bytes
> -End Of Table
> +Handle 0xBABA, DMI type 186, 186 bytes
> +       <TRUNCATED>
>   
> +Wrong DMI structures length: 2500 bytes announced, structures occupy 2682 bytes.


what kernel did you use? I'm asking because such kind of issue can happen
in case of incorrect format of /sys/firmware/dmi/entries.
FYI, beginning from k3.18, dmi sysfs contains regression.
Look at:
https://lkml.org/lkml/2015/2/18/224
"firmware: dmi_scan: fix dmi scan to handle "End of Table" structure"

It's currently applied on efi/urgent, no need to review it.
I's obvious that the problem can be in it. But, to be sure,
could you please present me the output of:
"ls /sys/firmware/dmi/entries/"

> This doesn't look good. Running dmidecode under valgrind reveals errors:
>
> ==5298== Conditional jump or move depends on uninitialised value(s)
> ==5298==    at 0x409513: dmi_table (dmidecode.c:4410)
> ==5298==    by 0x409A3B: dmifs_smbios_decode (dmidecode.c:4552)
> ==5298==    by 0x409F17: main (dmidecode.c:4721)
> ==5298==
> ==5298== Use of uninitialised value of size 8
> ==5298==    at 0x507C84B: _itoa_word (in /lib64/libc-2.18.so)
> ==5298==    by 0x508091C: vfprintf (in /lib64/libc-2.18.so)
> ==5298==    by 0x5086F58: printf (in /lib64/libc-2.18.so)
> ==5298==    by 0x40952C: dmi_table (dmidecode.c:4412)
> ==5298==    by 0x409A3B: dmifs_smbios_decode (dmidecode.c:4552)
> ==5298==    by 0x409F17: main (dmidecode.c:4721)
> ==5298==
> ==5298== Conditional jump or move depends on uninitialised value(s)
> ==5298==    at 0x507C855: _itoa_word (in /lib64/libc-2.18.so)
> ==5298==    by 0x508091C: vfprintf (in /lib64/libc-2.18.so)
> ==5298==    by 0x5086F58: printf (in /lib64/libc-2.18.so)
> ==5298==    by 0x40952C: dmi_table (dmidecode.c:4412)
> ==5298==    by 0x409A3B: dmifs_smbios_decode (dmidecode.c:4552)
> ==5298==    by 0x409F17: main (dmidecode.c:4721)
> ==5298==
> ==5298== Conditional jump or move depends on uninitialised value(s)
> ==5298==    at 0x5080968: vfprintf (in /lib64/libc-2.18.so)
> ==5298==    by 0x5086F58: printf (in /lib64/libc-2.18.so)
> ==5298==    by 0x40952C: dmi_table (dmidecode.c:4412)
> ==5298==    by 0x409A3B: dmifs_smbios_decode (dmidecode.c:4552)
> ==5298==    by 0x409F17: main (dmidecode.c:4721)
> ==5298==
> ==5298== Conditional jump or move depends on uninitialised value(s)
> ==5298==    at 0x50801C5: vfprintf (in /lib64/libc-2.18.so)
> ==5298==    by 0x5086F58: printf (in /lib64/libc-2.18.so)
> ==5298==    by 0x40952C: dmi_table (dmidecode.c:4412)
> ==5298==    by 0x409A3B: dmifs_smbios_decode (dmidecode.c:4552)
> ==5298==    by 0x409F17: main (dmidecode.c:4721)
> ==5298==
> ==5298== Conditional jump or move depends on uninitialised value(s)
> ==5298==    at 0x5080248: vfprintf (in /lib64/libc-2.18.so)
> ==5298==    by 0x5086F58: printf (in /lib64/libc-2.18.so)
> ==5298==    by 0x40952C: dmi_table (dmidecode.c:4412)
> ==5298==    by 0x409A3B: dmifs_smbios_decode (dmidecode.c:4552)
> ==5298==    by 0x409F17: main (dmidecode.c:4721)
> ==5298==
> (...)
> ==5298== ERROR SUMMARY: 6 errors from 6 contexts (suppressed: 2 from 2)
>
> This only confirms my initial feeling that the complexity added by the
> extra layer is going to cause more trouble than is worth.

I didn't change anything in interpreting of dmi table by dmidecode.
There is no reasons to worry about. So this only confirms that something
wrong in kernel dmi sysfs or libdmifs current implementation. It not proves
that using extra layer is bad idea.

> I also made some performance measurements. As I expected, the new
> libdmifs implementation is 3 times slower than the /dev/mem access
> (from 1.56 ms to 4.71 ms on average on my workstation.) The system CPU
> time grows even more (0.74 ms to 3.41 ms, 4.6 times slower).

Sorry, but your comparison is not correct.
You have to compare ti with own dmidecode implementation
of dmi sysfs reading. I'm sure that results will be not so impressive.
The circumstances are not comparable.

PS: and let check it when "End of Table" issue will gone.

> The only good news is that the entries are in the right order, which I
> did not think the dmi-sysfs interface would allow (I now see the
> "position" attribute makes that possible.) But that alone won't be
> enough to change my position.