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

Groups > linux.kernel > #1443175 > unrolled thread

[PATCH 0/2] dmi-id: export oem strings to sysfs

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] dmi-id: export oem strings to sysfs Allen Hung <allen_hung@dell.com> - 2016-07-14 10:20 +0200
    [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs Allen Hung <allen_hung@dell.com> - 2016-07-14 10:20 +0200
      Re: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem  strings to sysfs kbuild test robot <lkp@intel.com> - 2016-07-14 11:20 +0200
      Re: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem  strings to sysfs Jean Delvare <jdelvare@suse.de> - 2016-07-19 11:10 +0200
        RE: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem  strings to sysfs <Mario_Limonciello@Dell.com> - 2016-07-19 17:00 +0200

#1443175 — [PATCH 0/2] dmi-id: export oem strings to sysfs

*** BLURB HERE ***

Allen Hung (2):
  dmi-id: don't free dev structure after calling device_register
  dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

 drivers/firmware/dmi-id.c | 116 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 112 insertions(+), 4 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

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

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/dmi-id.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
index 44c0139..f284a07 100644
--- a/drivers/firmware/dmi-id.c
+++ b/drivers/firmware/dmi-id.c
@@ -58,6 +58,107 @@ 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);
 
+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;
+}
+
+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;
+		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;
+}
+
 static void ascii_filter(char *d, const char *s)
 {
 	/* Filter out characters we don't want to see in the modalias string */
@@ -231,8 +332,15 @@ static int __init dmi_id_init(void)
 	if (ret)
 		goto fail_put_dmi_dev;
 
+	ret = dmi_id_init_oem_attr_group();
+	if (ret)
+		goto fail_dev_unregister;
+
 	return 0;
 
+fail_dev_unregister:
+	device_unregister(dmi_dev);
+
 fail_put_dmi_dev:
 	put_device(dmi_dev);
 
-- 
2.7.4

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

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

[Multipart message — attachments visible in raw view] — view raw

Hi,

[auto build test ERROR on v4.7-rc7]
[also build test ERROR on next-20160713]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Allen-Hung/dmi-id-export-oem-strings-to-sysfs/20160714-161631
config: i386-defconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/firmware/dmi-id.c: In function 'dmi_id_init_oem_attr_group':
>> drivers/firmware/dmi-id.c:107:28: error: 'dmi_dev' undeclared (first use in this function)
     ret = sysfs_create_group(&dmi_dev->kobj, &dmi_oem_attr_group);
                               ^~~~~~~
   drivers/firmware/dmi-id.c:107:28: note: each undeclared identifier is reported only once for each function it appears in

vim +/dmi_dev +107 drivers/firmware/dmi-id.c

   101		int i, ret;
   102		const struct dmi_device *dev;
   103		struct dmi_oem_attribute *oa, *tmp;
   104		struct device_attribute dev_attr_tmpl =
   105			__ATTR(, 0444, sys_dmi_oem_show, NULL);
   106	
 > 107		ret = sysfs_create_group(&dmi_dev->kobj, &dmi_oem_attr_group);
   108		if (ret)
   109			return ret;
   110	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

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

Hello Allen,

On Thu, 14 Jul 2016 16:01:23 +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.

They are intended for internal consumption by the kernel drivers.

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

What is the use case? You can already get these strings easily using
dmidecode:

# dmidecode -qt 11
OEM Strings
	String 1: Dell System
	String 2: 1[05A4]
	String 3: 3[1.0]
	String 4: 12[www.dell.com]
	String 5: 14[1]
	String 6: 15[3]
	String 7:  

If needed, a dedicated option could be added to dmidecode to extract
specific OEM strings. Or existing option -s could be extended for that
purpose.

Also your code doesn't even build. I won't review this patch until I
know why it is needed, and it builds (without warning.)

One comment below though:

> 
> Signed-off-by: Allen Hung <allen_hung@dell.com>
> ---
>  drivers/firmware/dmi-id.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 108 insertions(+)
> 
> diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> index 44c0139..f284a07 100644
> --- a/drivers/firmware/dmi-id.c
> +++ b/drivers/firmware/dmi-id.c
> (...)
> +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);

I'd be very careful about permissions. OEM strings could contain pretty
much everything, including serial numbers or passwords. Making these
files world-readable doesn't strike me as the best of the ideas.

-- 
Jean Delvare
SUSE L3 Support

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

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

Hi Jean,

I worked with Allen on this concept, so I've got some comments below.

> -----Original Message-----
> From: Jean Delvare [mailto:jdelvare@suse.de]
> Sent: Tuesday, July 19, 2016 4:03 AM
> To: Hung, Allen <Allen_Hung@Dell.com>
> Cc: Jean Delvare <jdelvare@suse.com>; linux-kernel@vger.kernel.org;
> Limonciello, Mario <Mario_Limonciello@Dell.com>
> Subject: Re: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> Hello Allen,
> 
> On Thu, 14 Jul 2016 16:01:23 +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.
> 
> They are intended for internal consumption by the kernel drivers.
> 
> > 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.
> 
> What is the use case? You can already get these strings easily using
> dmidecode:
> 
> # dmidecode -qt 11
> OEM Strings
> 	String 1: Dell System
> 	String 2: 1[05A4]
> 	String 3: 3[1.0]
> 	String 4: 12[www.dell.com]
> 	String 5: 14[1]
> 	String 6: 15[3]
> 	String 7:
> 
> If needed, a dedicated option could be added to dmidecode to extract
> specific OEM strings. Or existing option -s could be extended for that
> purpose.

The main purpose was to be able to parse these easily from userspace
without needing dmidecode installed and handling its output 
(with tools such as grep, sed, and awk).

For example in an initramfs, typically dmidecode isn't included, but there
is value to being able to make decisions on things related to the values of 
those OEM strings.

Instead this allows userspace to iterate the oem/ directory and directly
look at the values of these strings.

> 
> Also your code doesn't even build. I won't review this patch until I
> know why it is needed, and it builds (without warning.)
> 

Allen had a mistake in that submission when he was refactoring it prior to 
LKML submission.  
He resubmitted it the next day fixing that mistake:
https://patchwork.kernel.org/patch/9231473/

> One comment below though:
> 
> >
> > Signed-off-by: Allen Hung <allen_hung@dell.com>
> > ---
> >  drivers/firmware/dmi-id.c | 108
> ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 108 insertions(+)
> >
> > diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> > index 44c0139..f284a07 100644
> > --- a/drivers/firmware/dmi-id.c
> > +++ b/drivers/firmware/dmi-id.c
> > (...)
> > +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);
> 
> I'd be very careful about permissions. OEM strings could contain pretty
> much everything, including serial numbers or passwords. Making these
> files world-readable doesn't strike me as the best of the ideas.
> 

At least on Dell systems, the values in these strings are OK to be world
readable, but I understand this concern and agree that Allen should adjust
these permissions in the next version if you agree with the concept of this
patch.

Thanks,

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web