All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs
@ 2016-08-15  9:22 Allen Hung
  2016-08-31 12:40 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 12+ messages in thread
From: Allen Hung @ 2016-08-15  9:22 UTC (permalink / raw)
  To: Jean Delvare, Greg Kroah-Hartman, Russell King, Gabriel Somlo,
	Bjorn Andersson, Jens Wiklander, Andy Gross, Arnd Bergmann,
	Sudeep Holla, Eric Anholt, linux-kernel, Mario Limonciello
  Cc: Allen Hung

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/.
+
 config DMI_SYSFS
 	tristate "DMI table support in sysfs"
 	depends on SYSFS && DMI
diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
index 44c0139..4467044 100644
--- a/drivers/firmware/dmi-id.c
+++ b/drivers/firmware/dmi-id.c
@@ -58,6 +58,46 @@ DEFINE_DMI_ATTR_WITH_SHOW(chassis_version,	0444, DMI_CHASSIS_VERSION);
 DEFINE_DMI_ATTR_WITH_SHOW(chassis_serial,	0400, DMI_CHASSIS_SERIAL);
 DEFINE_DMI_ATTR_WITH_SHOW(chassis_asset_tag,	0444, DMI_CHASSIS_ASSET_TAG);
 
+#ifdef CONFIG_DMIID_OEM_STRINGS
+static struct attribute *dmi_oem_attrs[] = {
+	NULL,
+};
+
+static const char oem_group[] = "oem";
+
+static struct attribute_group dmi_oem_attr_group = {
+	.attrs = dmi_oem_attrs,
+	.name  = oem_group,
+};
+
+static LIST_HEAD(dmi_oem_attrs_list);
+
+struct dmi_oem_attribute {
+	struct device_attribute dev_attr;
+	const char *oem_string;
+	char buf[32];
+	bool is_added:1;
+	struct list_head list;
+};
+
+#define to_dmi_oem_attr(_dev_attr) \
+	container_of(_dev_attr, struct dmi_oem_attribute, dev_attr)
+
+static ssize_t sys_dmi_oem_show(struct device *dev,
+				  struct device_attribute *attr,
+				  char *page)
+{
+	struct dmi_oem_attribute *oa = to_dmi_oem_attr(attr);
+	ssize_t len;
+
+	strlcpy(page, oa->oem_string, PAGE_SIZE-1);
+	len = strlen(page);
+	page[len++] = '\n';
+	page[len] = 0;
+	return len;
+}
+#endif
+
 static void ascii_filter(char *d, const char *s)
 {
 	/* Filter out characters we don't want to see in the modalias string */
@@ -204,6 +244,72 @@ static void __init dmi_id_init_attr_table(void)
 	sys_dmi_attributes[i++] = &sys_dmi_modalias_attr.attr;
 }
 
+#ifdef CONFIG_DMIID_OEM_STRINGS
+static int __init dmi_id_init_oem_attr_group(void)
+{
+	int i, ret;
+	const struct dmi_device *dev;
+	struct dmi_oem_attribute *oa, *tmp;
+	struct device_attribute dev_attr_tmpl =
+		__ATTR(, 0444, sys_dmi_oem_show, NULL);
+
+	ret = sysfs_create_group(&dmi_dev->kobj, &dmi_oem_attr_group);
+	if (ret)
+		return ret;
+
+	/* All devices with type=DMI_DEV_TYPE_OEM_STRING will be found in
+	 * the reverse order of what they were parsed in dmi_scan.c. However,
+	 * we do want to expose the OEM strings to sysfs in the same order as
+	 * what they were originally parsed. A linked list with 2-pass method
+	 * is used here to reverse the reserved order.
+	 *
+	 * Pass 1: find out all "OEM string" devices and add each "oem string"
+	 * to a linked list.
+	 */
+	dev = dmi_find_device(DMI_DEV_TYPE_OEM_STRING, NULL, NULL);
+	while (dev)  {
+		oa = kzalloc(sizeof(*oa), GFP_KERNEL);
+		if (!oa) {
+			ret = -ENOMEM;
+			goto failed;
+		}
+		oa->dev_attr = dev_attr_tmpl;
+		oa->oem_string = dev->name;
+		list_add(&oa->list, &dmi_oem_attrs_list);
+		dev = dmi_find_device(DMI_DEV_TYPE_OEM_STRING, NULL, dev);
+	}
+
+	/* Pass 2: traverse the list and add each string as a file to "oem"
+	 * group
+	 */
+	i = 0;
+	list_for_each_entry(oa, &dmi_oem_attrs_list, list) {
+		snprintf(oa->buf, sizeof(oa->buf), "string%d", ++i);
+		oa->dev_attr.attr.name = oa->buf;
+		oa->dev_attr.attr.mode = 0400;
+		ret = sysfs_add_file_to_group(
+			&dmi_dev->kobj, &oa->dev_attr.attr, oem_group);
+		if (ret)
+			goto failed;
+		oa->is_added = 1;
+	}
+
+	return 0;
+
+failed:
+	list_for_each_entry_safe(oa, tmp, &dmi_oem_attrs_list, list) {
+		if (oa->is_added)
+			sysfs_remove_file_from_group(
+				&dmi_dev->kobj,	&oa->dev_attr.attr, oem_group);
+		list_del(&oa->list);
+		kfree(oa);
+	}
+	sysfs_remove_group(&dmi_dev->kobj, &dmi_oem_attr_group);
+
+	return ret;
+}
+#endif
+
 static int __init dmi_id_init(void)
 {
 	int ret;
@@ -231,8 +337,18 @@ static int __init dmi_id_init(void)
 	if (ret)
 		goto fail_put_dmi_dev;
 
+#ifdef CONFIG_DMIID_OEM_STRINGS
+	ret = dmi_id_init_oem_attr_group();
+	if (ret)
+		goto fail_dev_unregister;
+#endif
 	return 0;
 
+#ifdef CONFIG_DMIID_OEM_STRINGS
+fail_dev_unregister:
+	device_unregister(dmi_dev);
+#endif
+
 fail_put_dmi_dev:
 	put_device(dmi_dev);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs
  2016-08-15  9:22 [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs Allen Hung
@ 2016-08-31 12:40 ` Greg Kroah-Hartman
  2016-08-31 14:01   ` Mario_Limonciello
  0 siblings, 1 reply; 12+ messages in thread
From: Greg Kroah-Hartman @ 2016-08-31 12:40 UTC (permalink / raw)
  To: Allen Hung
  Cc: Jean Delvare, Russell King, Gabriel Somlo, Bjorn Andersson,
	Jens Wiklander, Andy Gross, Arnd Bergmann, Sudeep Holla,
	Eric Anholt, linux-kernel, Mario Limonciello

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs
  2016-08-31 12:40 ` Greg Kroah-Hartman
@ 2016-08-31 14:01   ` Mario_Limonciello
  2016-08-31 14:43     ` Greg KH
  0 siblings, 1 reply; 12+ messages in thread
From: Mario_Limonciello @ 2016-08-31 14:01 UTC (permalink / raw)
  To: gregkh, Allen_Hung
  Cc: jdelvare, rmk+kernel, somlo, bjorn.andersson, jens.wiklander,
	agross, arnd, sudeep.holla, eric, linux-kernel

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs
  2016-08-31 14:01   ` Mario_Limonciello
@ 2016-08-31 14:43     ` Greg KH
  2016-08-31 15:47       ` Jean Delvare
  0 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2016-08-31 14:43 UTC (permalink / raw)
  To: Mario_Limonciello
  Cc: Allen_Hung, jdelvare, rmk+kernel, somlo, bjorn.andersson,
	jens.wiklander, agross, arnd, sudeep.holla, eric, linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs
  2016-08-31 14:43     ` Greg KH
@ 2016-08-31 15:47       ` Jean Delvare
  2016-08-31 21:51         ` Mario_Limonciello
  0 siblings, 1 reply; 12+ messages in thread
From: Jean Delvare @ 2016-08-31 15:47 UTC (permalink / raw)
  To: Greg KH
  Cc: Mario_Limonciello, Allen_Hung, rmk+kernel, somlo,
	bjorn.andersson, jens.wiklander, agross, arnd, sudeep.holla,
	eric, linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs
  2016-08-31 15:47       ` Jean Delvare
@ 2016-08-31 21:51         ` Mario_Limonciello
  2016-09-01 18:01           ` Jean Delvare
  0 siblings, 1 reply; 12+ messages in thread
From: Mario_Limonciello @ 2016-08-31 21:51 UTC (permalink / raw)
  To: jdelvare, gregkh
  Cc: Allen_Hung, rmk+kernel, somlo, bjorn.andersson, jens.wiklander,
	agross, arnd, sudeep.holla, eric, linux-kernel

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs
  2016-08-31 21:51         ` Mario_Limonciello
@ 2016-09-01 18:01           ` Jean Delvare
  2016-09-01 19:14             ` Mario_Limonciello
  0 siblings, 1 reply; 12+ messages in thread
From: Jean Delvare @ 2016-09-01 18:01 UTC (permalink / raw)
  To: Mario_Limonciello
  Cc: gregkh, Allen_Hung, rmk+kernel, somlo, jens.wiklander, agross,
	arnd, sudeep.holla, eric, linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs
  2016-09-01 18:01           ` Jean Delvare
@ 2016-09-01 19:14             ` Mario_Limonciello
  2017-05-01 11:39               ` Jean Delvare
  0 siblings, 1 reply; 12+ messages in thread
From: Mario_Limonciello @ 2016-09-01 19:14 UTC (permalink / raw)
  To: jdelvare
  Cc: gregkh, Allen_Hung, rmk+kernel, somlo, jens.wiklander, agross,
	arnd, sudeep.holla, eric, linux-kernel

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs
  2016-09-01 19:14             ` Mario_Limonciello
@ 2017-05-01 11:39               ` Jean Delvare
  2017-05-01 20:58                 ` Mario.Limonciello
  0 siblings, 1 reply; 12+ messages in thread
From: Jean Delvare @ 2017-05-01 11:39 UTC (permalink / raw)
  To: Mario_Limonciello
  Cc: gregkh, Allen_Hung, rmk+kernel, somlo, jens.wiklander, agross,
	arnd, sudeep.holla, eric, linux-kernel

Hi Mario, Allen,

Sorry for taking so long to get back to you. No excuses really :-(

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

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.

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

[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@suse.de>
---
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs
  2017-05-01 11:39               ` Jean Delvare
@ 2017-05-01 20:58                 ` Mario.Limonciello
  2017-05-22  7:47                   ` Jean Delvare
  0 siblings, 1 reply; 12+ messages in thread
From: Mario.Limonciello @ 2017-05-01 20:58 UTC (permalink / raw)
  To: jdelvare
  Cc: gregkh, Allen.Hung, rmk+kernel, somlo, jens.wiklander, agross,
	arnd, sudeep.holla, eric, linux-kernel

> -----Original Message-----
> From: Jean Delvare [mailto:jdelvare@suse.de]
> Sent: Monday, May 1, 2017 6:39 AM
> 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, 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@Dell.com wrote:
> > > -----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
> > >
> > > On Wed, 31 Aug 2016 21:51:22 +0000, Mario_Limonciello@Dell.com 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@suse.de>
> ---
> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs
  2017-05-01 20:58                 ` Mario.Limonciello
@ 2017-05-22  7:47                   ` Jean Delvare
  2017-05-22 19:53                     ` Mario.Limonciello
  0 siblings, 1 reply; 12+ messages in thread
From: Jean Delvare @ 2017-05-22  7:47 UTC (permalink / raw)
  To: Mario.Limonciello
  Cc: gregkh, Allen.Hung, rmk+kernel, somlo, jens.wiklander, agross,
	arnd, sudeep.holla, eric, linux-kernel

Hi Mario,

On Mon, 1 May 2017 20:58:13 +0000, Mario.Limonciello@dell.com wrote:
> > 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.

Well I think it is in the very nature of OEM strings to be only useful
for vendor-specific tools. Nothing Dell-specific here.

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

Understood, more on this below.

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

This is rather good news :-)

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

OK, I get the idea. It should be noted that the patch you initially
proposed, and I declined, did not fully solve this actual problem.
Making the OEM strings available to user-space through sysfs does not
allow for modalias-based auto-loading of kernel drivers based on these
OEM strings. It would only allow user-space tools or scripts to
manually poke at OEM strings to make decisions. Only the data included
in /sys/devices/virtual/dmi/id/modalias can be used to load drivers
automatically, and OEM strings aren't part of that. Nor do I think they
should, as they are non-standard by nature.

>From a kernel driver perspective, your candidate patch did not change
anything, as the OEM strings are already available to kernel drivers
today, using dmi_find_device(DMI_DEV_TYPE_OEM_STRING, ...).

Overall it looks like an XY problem to me. You came to us with a
candidate solution to problem Y (making OEM strings available to
user-space), and we have spent some time debating it (you doing it in
the kernel, me in dmidecode) while in the end neither implementation
will address problem X (exposing the name of the original hardware
manufacturer behind the OEM vendor, ideally in a way which allows
module auto-loading.)

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

No, I do not. It would probably make sense, but I have no idea how to
make it happen, nor the requirements and implications.

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

Did you test? I would like to release dmidecode 3.1 by the end of the
month, and it would be great if this feature could be included.

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

I think it is, yes.

You would have a dependency on something anyway, as the feature doesn't
exist today. If it was implemented in the kernel as you wanted, you
would depend on a specific kernel version (or backport of your commit.)
Most people will find it a lot easier to have to install dmidecode 3.1
(or 3.0 with my commit backported) than to upgrade their kernel.

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

I agree, as my own conclusions above indicate.

I think we need to start thinking again from scratch. As I understand
it, what you really need is a way to include the original maker of the
hardware in some modalias string, so that udev can trigger the loading
of the appropriate kernel modules. Is that correct?

In the case of Dell, the information is present in the DMI OEM string
#1. However this is a Dell-specific decision, so just always including
this string in /sys/devices/virtual/dmi/id/modalias would only work for
Dell and not other hardware makers. Therefore I think we need one level
of indirection.

One way to implement this would be to add another key at the end
of /sys/devices/virtual/dmi/id/modalias, maybe "osvn" for "original
system vendor" (tell me if you have a better idea.) The value of this
key would be computed by sequentially testing a number of hypotheses,
starting with checking if OEM string #1 is "Dell System". In which case
osvn would be set to "DellInc." or whatever you think is appropriate.
For over OEM providers, other tests could be added later. If no test
succeeds then the extra key is left empty.

Kernel drivers which should be loaded on every system originally
designed by some hardware maker could then just match the extra key
(which will thus need an internal name and ID as well, something like
DMI_ORIGINAL_SYS_VENDOR, added to enum dmi_field.)

The value could be additionally exposed
as /sys/devices/virtual/dmi/id/original_sys_vendor or some such if you
think that could be useful.

What do you think?

Are we good with one original maker string, or should we envision the
possibility that different parts of the system could be from different
original makers?

Still... Before going down that road, can you convince me that the
above is really needed? I mean, let's assume vendor Magicfoo decides to
base their upcoming laptops on an original Dell design. Why can't we
just add:

	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Magicfoo"),
			DMI_MATCH(DMI_CHASSIS_TYPE, "9"), /*Laptop*/
		},
	},

to drivers/platform/x86/dell-laptop.c and be done with it? The driver
can simply bail out if loaded on a system where it isn't actually
needed. Isn't it good enough?

-- 
Jean Delvare
SUSE L3 Support

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs
  2017-05-22  7:47                   ` Jean Delvare
@ 2017-05-22 19:53                     ` Mario.Limonciello
  0 siblings, 0 replies; 12+ messages in thread
From: Mario.Limonciello @ 2017-05-22 19:53 UTC (permalink / raw)
  To: jdelvare
  Cc: gregkh, Allen.Hung, rmk+kernel, somlo, jens.wiklander, agross,
	arnd, sudeep.holla, eric, linux-kernel

> > 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.
> 
> OK, I get the idea. It should be noted that the patch you initially
> proposed, and I declined, did not fully solve this actual problem.
> Making the OEM strings available to user-space through sysfs does not
> allow for modalias-based auto-loading of kernel drivers based on these
> OEM strings. It would only allow user-space tools or scripts to
> manually poke at OEM strings to make decisions. Only the data included
> in /sys/devices/virtual/dmi/id/modalias can be used to load drivers
> automatically, and OEM strings aren't part of that. Nor do I think they
> should, as they are non-standard by nature.
> 
> From a kernel driver perspective, your candidate patch did not change
> anything, as the OEM strings are already available to kernel drivers
> today, using dmi_find_device(DMI_DEV_TYPE_OEM_STRING, ...).
> 
> Overall it looks like an XY problem to me. You came to us with a
> candidate solution to problem Y (making OEM strings available to
> user-space), and we have spent some time debating it (you doing it in
> the kernel, me in dmidecode) while in the end neither implementation
> will address problem X (exposing the name of the original hardware
> manufacturer behind the OEM vendor, ideally in a way which allows
> module auto-loading.)

So far we've been lucky and systems that we know get OEM'ed
have only needed to be quirked one time that I know of.  That quirk
ended up being done off of the PCI host controller 
subsystem vendor and product ID.

More on that as an option later.  You're entirely right though.  
We were only solving one half of the problem with the solution 
we proposed so far.

> 
> > > 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?
> 
> No, I do not. It would probably make sense, but I have no idea how to
> make it happen, nor the requirements and implications.

I agree that pushing a requirement in this area will probably break "someone".
I guess let's table that for now and figure out the right solution for "current".

> 
> > > (...)
> > > 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
> 
> Did you test? I would like to release dmidecode 3.1 by the end of the
> month, and it would be great if this feature could be included.

Yes, sorry I did test this and it works properly.  I think this is good for
dmidecode. 

> 
> > 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?
> 
> I think it is, yes.
> 
> You would have a dependency on something anyway, as the feature doesn't
> exist today. If it was implemented in the kernel as you wanted, you
> would depend on a specific kernel version (or backport of your commit.)
> Most people will find it a lot easier to have to install dmidecode 3.1
> (or 3.0 with my commit backported) than to upgrade their kernel.
> 

This is true.  It's subjective where it's "easy" since for factory installed stuff
we work with our various Linux OS vendors to build images with backported
stuff.

For the general use case, yeah I agree installing a newer userspace application
is easier than backporting a kernel patch and rebuilding a kernel.

> > It does still leave the kernel modules matching to be problematic too.
> 
> I agree, as my own conclusions above indicate.
> 
> I think we need to start thinking again from scratch. As I understand
> it, what you really need is a way to include the original maker of the
> hardware in some modalias string, so that udev can trigger the loading
> of the appropriate kernel modules. Is that correct?

Not just for udev triggering, but also internal use within the kernel.
There's plenty of places that drivers special case on DMI data using stuff like this:
DMI_MATCH(DMI_PRODUCT_NAME,

> 
> In the case of Dell, the information is present in the DMI OEM string
> #1. However this is a Dell-specific decision, so just always including
> this string in /sys/devices/virtual/dmi/id/modalias would only work for
> Dell and not other hardware makers. Therefore I think we need one level
> of indirection.
>
Yeah I agree.
 
> One way to implement this would be to add another key at the end
> of /sys/devices/virtual/dmi/id/modalias, maybe "osvn" for "original
> system vendor" (tell me if you have a better idea.) The value of this
> key would be computed by sequentially testing a number of hypotheses,
> starting with checking if OEM string #1 is "Dell System". In which case
> osvn would be set to "DellInc." or whatever you think is appropriate.
> For over OEM providers, other tests could be added later. If no test
> succeeds then the extra key is left empty.
> 
> Kernel drivers which should be loaded on every system originally
> designed by some hardware maker could then just match the extra key
> (which will thus need an internal name and ID as well, something like
> DMI_ORIGINAL_SYS_VENDOR, added to enum dmi_field.)
> 
> The value could be additionally exposed
> as /sys/devices/virtual/dmi/id/original_sys_vendor or some such if you
> think that could be useful.
> 
> What do you think?
> 

I think this is actually a pretty good proposal.  Once extension I'd like
to add though is "ospn" for "Original System Product Name".

There is a hexadecimal string used to indicate the system identifier.  
It's the same thing you would see if you looked at PCI subsystem
device ID across the various PCI devices in the box.

Between that combination of both keys you can do modaliasing or quirking
accurately and to the same granularity you can today with 
DMI_SYS_VENDOR/DMI_PRODUCT_NAME.

> Are we good with one original maker string, or should we envision the
> possibility that different parts of the system could be from different
> original makers?

The hardware will be identical to a Dell branded machine, but as I said
above the model number will be changed.

> 
> Still... Before going down that road, can you convince me that the
> above is really needed? I mean, let's assume vendor Magicfoo decides to
> base their upcoming laptops on an original Dell design. Why can't we
> just add:
> 
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Magicfoo"),
> 			DMI_MATCH(DMI_CHASSIS_TYPE, "9"), /*Laptop*/
> 		},
> 	},
> 
> to drivers/platform/x86/dell-laptop.c and be done with it? The driver
> can simply bail out if loaded on a system where it isn't actually
> needed. Isn't it good enough?

Dell internal teams and partners are given direction not to depend 
anything upon the marketing model ID (DMI_PRODUCT_NAME) because
this rebranding can be applied to any product.  They instead depend upon
the value that I mentioned above that would be that original system
produce name.
For example on my  Precision T7600 this would be "0495":
        String 1: Dell System
        String 2: 1[0495]

Of course you don't really have an idea of what scale this happens, but
in some business segments more machines are sold as OEM'ed than Dell
branded.  Newly branded machines can be introduced at any time.

As a result, it's not really a manageable approach to document every
single machine that has been OEM'ed.  (I suspect the team that works on 
OEM'ing these would consider it proprietary information too.)


So all that said, maybe is the simpler approach to export the information
from PCI subsystem vendor and device out to the system as a pair of new
DMI match variables?   I think both X & Y could be solved in the same solution
to get this information if exported to the right places.

Example, my same system you'll get:
00:00.0 Host bridge [0600]: Intel Corporation Xeon E5/Core i7 DMI2 [8086:3c00] (rev 05)
        Subsystem: Dell Xeon E5/Core i7 DMI2 [1028:0495]

1028 is Dell
0495 is the system product ID I referred to above.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-05-22 19:53 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-15  9:22 [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs Allen Hung
2016-08-31 12:40 ` Greg Kroah-Hartman
2016-08-31 14:01   ` Mario_Limonciello
2016-08-31 14:43     ` Greg KH
2016-08-31 15:47       ` Jean Delvare
2016-08-31 21:51         ` Mario_Limonciello
2016-09-01 18:01           ` Jean Delvare
2016-09-01 19:14             ` Mario_Limonciello
2017-05-01 11:39               ` Jean Delvare
2017-05-01 20:58                 ` Mario.Limonciello
2017-05-22  7:47                   ` Jean Delvare
2017-05-22 19:53                     ` Mario.Limonciello

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.