lkml.org 
[lkml]   [2016]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs
Date
Hi Jean,

> -----Original Message-----
> From: Jean Delvare [mailto:jdelvare@suse.de]
> Sent: Wednesday, August 31, 2016 10:48 AM
> To: Greg KH <gregkh@linuxfoundation.org>
> Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; Hung, Allen
> <Allen_Hung@Dell.com>; rmk+kernel@arm.linux.org.uk; somlo@cmu.edu;
> bjorn.andersson@sonymobile.com; jens.wiklander@linaro.org;
> agross@codeaurora.org; arnd@arndb.de; sudeep.holla@arm.com;
> eric@anholt.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
>
> Hi all,
>
> On Wed, 31 Aug 2016 16:43:26 +0200, Greg KH wrote:
> > On Wed, Aug 31, 2016 at 02:01:23PM +0000, Mario_Limonciello@Dell.com
> wrote:
> > > Jean Delvare would rather see this implemented in userspace
> dmidecode.
> > > Jean raised a concern in an earlier submission that this runs on every
> > > machine (https://lkml.org/lkml/2016/8/2/799).
> >
> > Ah, yeah, just use dmidecode, much simpler, keeps the kernel smaller, I
> > like it.
>

The main fundamental difference between kernel and userspace
will be that applications will need to run dmidecode multiple times
to get at all this data rather than read in a handful of files from sysfs.

I'd like to ask more on the history of why *any* SMBIOS data was
exposed to sysfs in the first place rather than making all of
userspace do this same exercise of calling dmidecode to get at data?

Why are the strings already exposed by the kernel in sysfs any more
valuable than OEM strings?

> I wrote a proof of concept patch for dmidecode before my vacation, I
> can't remember if I sent it out or not, so I guess it did not happen.
> Here it is:
>
> From: Jean Delvare <jdelvare@suse.de>
> 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@suse.de>
> ---
> dmidecode.c | 7 +++++++
> dmiopt.c | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 40 insertions(+)
>
> --- dmidecode.orig/dmiopt.c 2015-10-01 08:41:43.533806256 +0200
> +++ dmidecode/dmiopt.c 2016-08-05 10:32:44.907196966 +0200
> @@ -171,6 +171,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 +210,29 @@ 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;
> + }
> +
> + 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;
> + opt.string = &opt_oem_string_keyword;
> + return 0;
> +}
> +
>
> /*
> * Command line options handling
> @@ -225,6 +252,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 +283,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)
> --- dmidecode.orig/dmidecode.c 2016-07-22 10:26:50.190119889 +0200
> +++ dmidecode/dmidecode.c 2016-08-05 10:41:53.746645533 +0200
> @@ -4370,6 +4370,13 @@ 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])
> + printf("%s\n", dmi_string(h, offset));
> + return;
> + }
> +
> if (offset >= h->length)
> return;
>
> I know it's not a universal way to decide where to put the code, but
> note how it's half the side of your kernel-side implementation proposal.
>

Thanks for doing that.

I applied your patch locally and looked a little bit at it.

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.

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.

I know it was just a PoC, but if you do end up including this in dmidecode some
other functional comments:
1) --help would need to be updated too for the new option.
2) There is testing for some invalid arguments, but if you put a larger number than
number of OEM strings no error is displayed.
3) -O didn't seem to work for me, only --oem-string.

\
 
 \ /
  Last update: 2016-09-17 09:58    [W:0.148 / U:0.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site