RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

From: Mario.Limonciello
Date: Mon May 01 2017 - 16:58:34 EST

> -----Original Message-----
> From: Jean Delvare [mailto:jdelvare@xxxxxxx]
> Sent: Monday, May 1, 2017 6:39 AM
> To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; Hung, Allen <Allen_Hung@xxxxxxxx>;
> rmk+kernel@xxxxxxxxxxxxxxxx; somlo@xxxxxxx; jens.wiklander@xxxxxxxxxx;
> agross@xxxxxxxxxxxxxx; arnd@xxxxxxxx; sudeep.holla@xxxxxxx;
> eric@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
>
> Hi Mario, Allen,
>
> Sorry for taking so long to get back to you. No excuses really :-(

Well thanks for eventually responding.

>
> On Thu, 1 Sep 2016 19:14:54 +0000, Mario_Limonciello@xxxxxxxx wrote:
> > > -----Original Message-----
> > > From: Jean Delvare [mailto:jdelvare@xxxxxxx]
> > > Sent: Thursday, September 1, 2016 1:01 PM
> > > To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> > > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; Hung, Allen <Allen_Hung@xxxxxxxx>;
> > > rmk+kernel@xxxxxxxxxxxxxxxx; somlo@xxxxxxx; jens.wiklander@xxxxxxxxxx;
> > > agross@xxxxxxxxxxxxxx; arnd@xxxxxxxx; sudeep.holla@xxxxxxx;
> > > eric@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem
> > > strings to sysfs
> > >
> > > On Wed, 31 Aug 2016 21:51:22 +0000, Mario_Limonciello@xxxxxxxx wrote:
> > > > The main downside I see from this approach versus what Allen did in
> > > > the kernel is you don't know in advance how many OEM strings will
> > > > exist.
> > > >
> > > > Allen's kernel approach you knew how many would be there by the
> > > > number of sysfs items that were created. Your userspace approach I
> > > > can only really see working by trial and error based upon the
> > > > argument you give it.
> > > >
> > > > For example on a Precision 5510 I see 7 OEM strings, but on a T5810 I
> > > > only have 4.
> > >
> > > My expectation was that whoever needs some OEM string would know its
> > > index as well. There is no description attached to each string anyway,
> > > so the index is the only key to figure out what is what. The vendor is
> > > responsible for getting it right (that is, be consistent where it
> > > matters.)
> > >
> > > > Maybe one way to solve this would be if no arguments were given to
> > > > --oem-string return the number of OEM strings rather than an error.
> > >
> > > Can you explain why you need to know? If I knew your use case I would
> > > feel more motivated to come up with a solution ;-)
> > >
> > > (I am also not a fan of parameters with optional arguments in general,
> > > as this can make the command lines ambiguous, but this is an
> > > implementation detail really.)
> >
> > At least on Dell systems the number and contents of OEM strings is
> > dynamic. You won't be able to know in advance how many strings
> > will exist on a given box since some strings may only be present
> > based upon configuration settings.
>
> That I understand. What I do not understand is how software is supposed
> to figure out which string represents what, even if it knows how many
> strings there are in total.

At least for Dell, not all of these are useful to general purpose userspace
software but more so to internal tools.

The most useful one is one that indicates "Dell System" and would be
always indexed to "String 1" on Dell systems.

If *any* kernel implementation that exports to userspace would be
acceptable, recognizing that particular OEM string would be the most
valuable.

This might seem silly, why not just read /sys/class/dmi/id/sys_vendor
right?

Most business client products at Dell have the ability to be OEM'ed.
When this happens the standard SMBIOS strings will be wiped and
filled with data related to the OEM that will be re-selling them.

This unfortunately breaks some presumptions that kernel makes
about when quirks are applied to particular systems by DMI data.
Modules that look for "Dell Inc." won't load, quirks that are matched
on a "Latitude 7140" wouldn't apply etc.

Userspace similarly breaks when presumptions are made. For example
fwupd in displaying the value of the ESRT properly. It's quirked
to display how Dell encodes it based upon SMBIOS data in userspace.
This causes UEFI capsules to not apply on OEM'ed systems.

Not many systems that are OEM'ed are sold with Linux today (or used
with to our knowledge), but we're anticipating some changes in this,
and that's why we raised this in the first place.

So we would really like a way in the kernel to be able to recognize
this situation and both tell kernel modules and tell userspace.


>
> Note that I am not blaming Dell here, the SMBIOS specification is
> really weak in this respect, it would have better defined string pairs,
> so that the BIOS can specify both string names and values; or defined a
> standard format to include both in one string (simply defining a
> standard separator character such as ";" or "=" would have done the
> trick.) Leaving it to each vendor to sort it out was the best way to
> ensure there wouldn't be any consistency. Sigh.
>

It the DMTF had something better defined, absolutely we would follow
it for the future. Do you sit on DMTF?

> > The original kernel patch this wasn't a concern because you could
> > count number of sysfs files to determine how many strings were
> > present.
> >
> > With the current PoC implementation of yours, an app would need
> > to just keep calling with monotonically increasing OEM string index
> > values until an empty output was returned to find the number of
> > strings present.
>
> Alternatively the software could call:
> $ dmidecode -q -t 11 | grep -c '^[[:space:]][[:space:]]*String'
>
> But I agree we can make it easier. My proposal is that "--oem-string
> count" will return the number of OEM strings.
>
> > > (...)
> > > > 2) There is testing for some invalid arguments, but if you put a
> > > > larger number than number of OEM strings no error is displayed.
> > >
> > > That was on purpose. People kept complaining to me over the past years
> > > when option --string returned an error message. I finally "fixed" it for
> > > --string so I thought --oem-string should behave the same for
> > > consistency. The assumption is that the caller would test if it gets an
> > > empty string. Empty strings are not allowed in DMI data so if you get
> > > an empty string it means there was no string by that index.
> > >
> > > But I can print an error message on invalid index if you think it makes
> > > sense, that's easy.
> >
> > I don't know the context of those complaints, but as long as you return
> > an error code from dmidecode and output to stderr rather than stdout this
> > makes sense to me.
> >
> > Apps should only be looking at the output of stdout anyhow.
>
> You are correct. At the time of these complaints, there was also some
> confusion about what went to stdout and what when to stderr. As it has
> been sorted out meanwhile, spitting an error message on bad OEM string
> index shouldn't be a problem. I have implemented it.
>
> Below is my second proposal of a dmidecode patch implementing what you
> need. I believe I have addressed all your concerns, but please report
> if there is anything left to fix [1]. If you are happy with this patch,
> I will include it in the upcoming release 3.1 of dmidecode.
>

I'll do some testing with this later, but I would still really like a way to
take care of the situation I raised above. We can set up tools like
fwupd to have a dependency of dmidecode 3.1+ and then shell out to
dmidecode and use this functionality to parse the strings and pass up
whether it's a Dell system, but is that really the best way?

It does still leave the kernel modules matching to be problematic too.

> [1] One thing I am not sure of is how to deal with the case where a DMI
> table has multiple type 11 records. While this doesn't make much sense,
> the DMI specification does note mandate uniqueness of this record type,
> and I have seen several systems from one vendor with multiple 1-string
> type 11 records instead of a single multiple-string type 11 record.
> With my implementation, "dmidecode --oem-string 1" will return all of
> them, in record order (which may seem strange, but is how option
> --string works for other records, so it is at least consistent.)
> Allen's kernel implementation would number all OEM strings linearly
> instead, giving them a unique number (which is good) but losing their
> origin (not good.) All I can think of at the moment is either a generic
> --handle command line option that would restrict the output of
> dmidecode to the specified record, or something like --oem-string
> [handle,]number, that is, optionally restricting the output of
> --oem-string to a specific handle (or maybe instance number.) Opinions?
>
> Anyway, here's the patch:
>
> Subject: dmidecode: New option --oem-string
>
> Add a new option to extract OEM strings, like we already have for
> many other strings.
>
> Signed-off-by: Jean Delvare <jdelvare@xxxxxxx>
> ---
> Changes since v1:
> * Document the new option in --help and man page.
> * Return an error message if index is out of range.
> * Special value "count" returns the number of OEM strings.
>
> dmidecode.c | 15 +++++++++++++++
> dmiopt.c | 40 ++++++++++++++++++++++++++++++++++++++++
> man/dmidecode.8 | 7 ++++++-
> 3 files changed, 61 insertions(+), 1 deletion(-)
>
> --- dmidecode.orig/dmiopt.c 2017-04-04 10:03:43.885412547 +0200
> +++ dmidecode/dmiopt.c 2017-05-01 12:23:44.207581011 +0200
> @@ -20,6 +20,7 @@
> */
>
> #include <stdio.h>
> +#include <string.h>
> #include <strings.h>
> #include <stdlib.h>
> #include <getopt.h>
> @@ -171,6 +172,10 @@ static const struct string_keyword opt_s
> { "processor-frequency", 4, 0x16 }, /* dmi_processor_frequency() */
> };
>
> +/* This is a template, 3rd field is set at runtime. */
> +static struct string_keyword opt_oem_string_keyword =
> + { NULL, 11, 0x00 };
> +
> static void print_opt_string_list(void)
> {
> unsigned int i;
> @@ -206,6 +211,34 @@ static int parse_opt_string(const char *
> return -1;
> }
>
> +static int parse_opt_oem_string(const char *arg)
> +{
> + unsigned long val;
> + char *next;
> +
> + if (opt.string)
> + {
> + fprintf(stderr, "Only one string can be specified\n");
> + return -1;
> + }
> +
> + /* Return the number of OEM strings */
> + if (strcmp(arg, "count") == 0)
> + goto done;
> +
> + val = strtoul(arg, &next, 10);
> + if (next == arg || val == 0x00 || val > 0xff)
> + {
> + fprintf(stderr, "Invalid OEM string number: %s\n", arg);
> + return -1;
> + }
> +
> + opt_oem_string_keyword.offset = val;
> +done:
> + opt.string = &opt_oem_string_keyword;
> + return 0;
> +}
> +
>
> /*
> * Command line options handling
> @@ -225,6 +258,7 @@ int parse_command_line(int argc, char *
> { "dump", no_argument, NULL, 'u' },
> { "dump-bin", required_argument, NULL, 'B' },
> { "from-dump", required_argument, NULL, 'F' },
> + { "oem-string", required_argument, NULL, 'O' },
> { "no-sysfs", no_argument, NULL, 'S' },
> { "version", no_argument, NULL, 'V' },
> { NULL, 0, NULL, 0 }
> @@ -255,6 +289,11 @@ int parse_command_line(int argc, char *
> return -1;
> opt.flags |= FLAG_QUIET;
> break;
> + case 'O':
> + if (parse_opt_oem_string(optarg) < 0)
> + return -1;
> + opt.flags |= FLAG_QUIET;
> + break;
> case 't':
> opt.type = parse_opt_type(opt.type, optarg);
> if (opt.type == NULL)
> @@ -315,6 +354,7 @@ void print_help(void)
> " --dump-bin FILE Dump the DMI data to a binary file\n"
> " --from-dump FILE Read the DMI data from a binary file\n"
> " --no-sysfs Do not attempt to read DMI data from sysfs
> files\n"
> + " --oem-string N Only display the value of the given OEM
> string\n"
> " -V, --version Display the version and exit\n";
>
> printf("%s", help);
> --- dmidecode.orig/dmidecode.c 2017-04-27 16:55:26.011215158 +0200
> +++ dmidecode/dmidecode.c 2017-05-01 12:25:35.269804990 +0200
> @@ -4555,6 +4555,21 @@ static void dmi_table_string(const struc
> int key;
> u8 offset = opt.string->offset;
>
> + if (opt.string->type == 11) /* OEM strings */
> + {
> + if (h->length < 5 || offset > data[4])
> + {
> + fprintf(stderr, "No OEM string number %u\n", offset);
> + return;
> + }
> +
> + if (offset)
> + printf("%s\n", dmi_string(h, offset));
> + else
> + printf("%u\n", data[4]); /* count */
> + return;
> + }
> +
> if (offset >= h->length)
> return;
>
> --- dmidecode.orig/man/dmidecode.8 2015-08-06 12:49:52.339237585 +0200
> +++ dmidecode/man/dmidecode.8 2017-05-01 12:07:26.188735324 +0200
> @@ -134,13 +134,18 @@ Read the DMI data from a binary file pre
> Do not attempt to read DMI data from sysfs files. This is mainly useful for
> debugging.
> .TP
> +.BR " " " " "--oem-string N"
> +Only display the value of the \s-1OEM\s0 string number \fBN\fR. The first
> +\s-1OEM\s0 string has number 1. With special value "count", return the
> +number of OEM strings instead.
> +.TP
> .BR "-h" ", " "--help"
> Display usage information and exit
> .TP
> .BR "-V" ", " "--version"
> Display the version and exit
> .P
> -Options --string, --type and --dump-bin
> +Options --string, --type, --dump-bin and --oem-string
> determine the output format and are mutually exclusive.
> .P
> Please note in case of
>
>
> --
> Jean Delvare
> SUSE L3 Support