Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1473428 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem  strings to sysfs Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-08-31 14:50 +0200
    RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem  strings to sysfs <Mario_Limonciello@Dell.com> - 2016-08-31 16:10 +0200
      Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem  strings to sysfs Greg KH <gregkh@linuxfoundation.org> - 2016-08-31 16:50 +0200
        Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem  strings to sysfs Jean Delvare <jdelvare@suse.de> - 2016-08-31 17:50 +0200
          RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem  strings to sysfs <Mario_Limonciello@Dell.com> - 2016-09-01 00:00 +0200
            Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem  strings to sysfs Jean Delvare <jdelvare@suse.de> - 2016-09-01 23:40 +0200
              RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem  strings to sysfs <Mario_Limonciello@Dell.com> - 2016-09-01 23:50 +0200

#1473428 — Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

On Mon, Aug 15, 2016 at 05:22:05PM +0800, Allen Hung wrote:
> The oem strings in DMI system identification information of the BIOS have
> been parsed and stored as dmi devices in dmi_scan.c but they are not
> exported to userspace via sysfs.
> 
> The patch intends to export oem strings to sysfs device /sys/class/dmi/id.
> As the number of oem strings are dynamic, a group "oem" is added to the
> device and the strings will be added to the group as string1, string2, ...,
> and stringN.
> 
> Signed-off-by: Allen Hung <allen_hung@dell.com>
> ---
>  drivers/firmware/Kconfig  |   9 ++++
>  drivers/firmware/dmi-id.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 125 insertions(+)
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 6664f11..885a6c9 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -119,6 +119,15 @@ config DMIID
>  	  information from userspace through /sys/class/dmi/id/ or if you want
>  	  DMI-based module auto-loading.
>  
> +config DMIID_OEM_STRINGS
> +	bool "Export OEM strings in SMBIOS/DMI via sysfs to userspace"
> +	depends on DMIID
> +	default n
> +	help
> +	  Say Y here if you want to query OEM strings (as part of the information
> +	  contained in SMBIOS/DMI system identification) from userspace through
> +	  /sys/class/dmi/id/oem/.

Why wouldn't you want these?

> +
>  config DMI_SYSFS
>  	tristate "DMI table support in sysfs"
>  	depends on SYSFS && DMI

Shouldn't the new option, if you really want it, be below this one?

But again, why not just always provide these values, if they are in the
DMI tables, and you want sysfs DMI support?

thanks,

greg k-h

[toc] | [next] | [standalone]

#1473516

Hi Greg,

> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, August 31, 2016 7:40 AM
> To: Hung, Allen <Allen_Hung@Dell.com>
> Cc: Jean Delvare <jdelvare@suse.com>; Russell King
> <rmk+kernel@arm.linux.org.uk>; Gabriel Somlo <somlo@cmu.edu>; Bjorn
> Andersson <bjorn.andersson@sonymobile.com>; Jens Wiklander
> <jens.wiklander@linaro.org>; Andy Gross <agross@codeaurora.org>; Arnd
> Bergmann <arnd@arndb.de>; Sudeep Holla <sudeep.holla@arm.com>; Eric
> Anholt <eric@anholt.net>; linux-kernel@vger.kernel.org; Limonciello, Mario
> <Mario_Limonciello@Dell.com>
> Subject: Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> On Mon, Aug 15, 2016 at 05:22:05PM +0800, Allen Hung wrote:
> > The oem strings in DMI system identification information of the BIOS
> > have been parsed and stored as dmi devices in dmi_scan.c but they are
> > not exported to userspace via sysfs.
> >
> > The patch intends to export oem strings to sysfs device /sys/class/dmi/id.
> > As the number of oem strings are dynamic, a group "oem" is added to
> > the device and the strings will be added to the group as string1,
> > string2, ..., and stringN.
> >
> > Signed-off-by: Allen Hung <allen_hung@dell.com>
> > ---
> >  drivers/firmware/Kconfig  |   9 ++++
> >  drivers/firmware/dmi-id.c | 116
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 125 insertions(+)
> >
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index
> > 6664f11..885a6c9 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -119,6 +119,15 @@ config DMIID
> >  	  information from userspace through /sys/class/dmi/id/ or if you want
> >  	  DMI-based module auto-loading.
> >
> > +config DMIID_OEM_STRINGS
> > +	bool "Export OEM strings in SMBIOS/DMI via sysfs to userspace"
> > +	depends on DMIID
> > +	default n
> > +	help
> > +	  Say Y here if you want to query OEM strings (as part of the information
> > +	  contained in SMBIOS/DMI system identification) from userspace
> through
> > +	  /sys/class/dmi/id/oem/.
> 
> Why wouldn't you want these?
> 

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).

> > +
> >  config DMI_SYSFS
> >  	tristate "DMI table support in sysfs"
> >  	depends on SYSFS && DMI
> 
> Shouldn't the new option, if you really want it, be below this one?
> 

Ah yes, I think so.  If this ends up being the right approach Allen
will need to adjust and resubmit it.

> But again, why not just always provide these values, if they are in the DMI
> tables, and you want sysfs DMI support?

From our (Allen and myself) perspective this makes the most sense too,
Jean had pushed back on this, so Allen re-submitted as making it optional.

[toc] | [prev] | [next] | [standalone]

#1473561

On Wed, Aug 31, 2016 at 02:01:23PM +0000, Mario_Limonciello@Dell.com wrote:
> Hi Greg,
> 
> > -----Original Message-----
> > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> > Sent: Wednesday, August 31, 2016 7:40 AM
> > To: Hung, Allen <Allen_Hung@Dell.com>
> > Cc: Jean Delvare <jdelvare@suse.com>; Russell King
> > <rmk+kernel@arm.linux.org.uk>; Gabriel Somlo <somlo@cmu.edu>; Bjorn
> > Andersson <bjorn.andersson@sonymobile.com>; Jens Wiklander
> > <jens.wiklander@linaro.org>; Andy Gross <agross@codeaurora.org>; Arnd
> > Bergmann <arnd@arndb.de>; Sudeep Holla <sudeep.holla@arm.com>; Eric
> > Anholt <eric@anholt.net>; linux-kernel@vger.kernel.org; Limonciello, Mario
> > <Mario_Limonciello@Dell.com>
> > Subject: Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem
> > strings to sysfs
> > 
> > On Mon, Aug 15, 2016 at 05:22:05PM +0800, Allen Hung wrote:
> > > The oem strings in DMI system identification information of the BIOS
> > > have been parsed and stored as dmi devices in dmi_scan.c but they are
> > > not exported to userspace via sysfs.
> > >
> > > The patch intends to export oem strings to sysfs device /sys/class/dmi/id.
> > > As the number of oem strings are dynamic, a group "oem" is added to
> > > the device and the strings will be added to the group as string1,
> > > string2, ..., and stringN.
> > >
> > > Signed-off-by: Allen Hung <allen_hung@dell.com>
> > > ---
> > >  drivers/firmware/Kconfig  |   9 ++++
> > >  drivers/firmware/dmi-id.c | 116
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 125 insertions(+)
> > >
> > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index
> > > 6664f11..885a6c9 100644
> > > --- a/drivers/firmware/Kconfig
> > > +++ b/drivers/firmware/Kconfig
> > > @@ -119,6 +119,15 @@ config DMIID
> > >  	  information from userspace through /sys/class/dmi/id/ or if you want
> > >  	  DMI-based module auto-loading.
> > >
> > > +config DMIID_OEM_STRINGS
> > > +	bool "Export OEM strings in SMBIOS/DMI via sysfs to userspace"
> > > +	depends on DMIID
> > > +	default n
> > > +	help
> > > +	  Say Y here if you want to query OEM strings (as part of the information
> > > +	  contained in SMBIOS/DMI system identification) from userspace
> > through
> > > +	  /sys/class/dmi/id/oem/.
> > 
> > Why wouldn't you want these?
> > 
> 
> 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.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1473625

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.

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.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1473915

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.

[toc] | [prev] | [next] | [standalone]

#1474684

Hi Mario,

On Wed, 31 Aug 2016 21:51:22 +0000, Mario_Limonciello@Dell.com wrote:
> > > 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.

That's correct, and I agree it is slightly less efficient. But the
number of strings being small I don't think it's really a problem in
practice.

> 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?

If I recall properly it was introduced for kernel module auto-loading
based on DMI data, through udev.
Specifically /sys/devices/virtual/dmi/id/uevent was for this purpose.
The other attributes must have been added because it was cheap at that
point.

I suppose it could have been implemented using dmidecode as well, but
doing it in sysfs was more natural because this is where "real" devices
are also declared. So I guess there was almost no code to add to udev
to make it work.

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

Because OEM strings are not standard so generic tools have no use for
them. As a matter of fact the other attributes were added 9 years ago
and only now someone (you) is asking for OEM strings.

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

Thanks for testing.

> 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.)

> 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.

Thanks for the review, I appreciate it.

You are right, I forgot to update --help, and the manual page too.

> 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.

> 3) -O didn't seem to work for me, only --oem-string.

On purpose as well. There is no short option for --dump-bin,
--from-dump or --no-sysfs either, because I do not expect them to be
used frequently. You still have to give each option a one-char
identifier to make getopt_long() happy, even if you don't support the
corresponding short option. I chose "O" but it is arbitrary and
internal only at this point.

Maybe it's me getting old, but I tend to prefer using long options in
scripts now. I find it easier to read later, no need to remember what
the short options do nor look it up in the manual page.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1474716

Jean,

> -----Original Message-----
> From: Jean Delvare [mailto:jdelvare@suse.de]
> Sent: Thursday, September 1, 2016 1:01 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: gregkh@linuxfoundation.org; Hung, Allen <Allen_Hung@Dell.com>;
> rmk+kernel@arm.linux.org.uk; somlo@cmu.edu; 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 Mario,
> 
> On Wed, 31 Aug 2016 21:51:22 +0000, Mario_Limonciello@Dell.com wrote:
> > > > 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.
> 
> That's correct, and I agree it is slightly less efficient. But the
> number of strings being small I don't think it's really a problem in
> practice.
> 
> > 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?
> 
> If I recall properly it was introduced for kernel module auto-loading
> based on DMI data, through udev.
> Specifically /sys/devices/virtual/dmi/id/uevent was for this purpose.
> The other attributes must have been added because it was cheap at that
> point.

Yeah that's sorta what I was thinking it was for too.  Considering it was 
cheap to create those sysfs nodes without a clear standard consumer
is what was making me think that exposing OEM strings in kernel space 
made sense too.

> 
> I suppose it could have been implemented using dmidecode as well, but
> doing it in sysfs was more natural because this is where "real" devices
> are also declared. So I guess there was almost no code to add to udev
> to make it work.
> 
> > Why are the strings already exposed by the kernel in sysfs any more
> > valuable than OEM strings?
> 
> Because OEM strings are not standard so generic tools have no use for
> them. As a matter of fact the other attributes were added 9 years ago
> and only now someone (you) is asking for OEM strings.
> 
> > (...)
> > I applied your patch locally and looked a little bit at it.
> 
> Thanks for testing.

Sure

> 
> > 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.

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.

> (...)
> > 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.
 
> > 3) -O didn't seem to work for me, only --oem-string.
> 
> On purpose as well. There is no short option for --dump-bin,
> --from-dump or --no-sysfs either, because I do not expect them to be
> used frequently. You still have to give each option a one-char
> identifier to make getopt_long() happy, even if you don't support the
> corresponding short option. I chose "O" but it is arbitrary and
> internal only at this point.
> 
> Maybe it's me getting old, but I tend to prefer using long options in
> scripts now. I find it easier to read later, no need to remember what
> the short options do nor look it up in the manual page.

I do too for the same reasons.  I didn't look close enough at the code
to realize it was intended behavior on your behalf.  No concerns here.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web