From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753144AbcGSJDW (ORCPT ); Tue, 19 Jul 2016 05:03:22 -0400 Received: from mx2.suse.de ([195.135.220.15]:60810 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753001AbcGSJDS (ORCPT ); Tue, 19 Jul 2016 05:03:18 -0400 Date: Tue, 19 Jul 2016 11:03:14 +0200 From: Jean Delvare To: Allen Hung Cc: Jean Delvare , linux-kernel@vger.kernel.org, Mario Limonciello Subject: Re: [PATCH 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs Message-ID: <20160719110314.05d74b84@endymion> In-Reply-To: <1468483283-84766-3-git-send-email-allen_hung@dell.com> References: <1468483283-84766-1-git-send-email-allen_hung@dell.com> <1468483283-84766-3-git-send-email-allen_hung@dell.com> Organization: SUSE Linux X-Mailer: Claws Mail 3.12.0 (GTK+ 2.24.30; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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