[PATCH]: SMBIOS: Add initial code and export version via sysfs

[PATCH]: SMBIOS: Add initial code and export version via sysfs

[Prarit Bhargava]

The existing DMI code, drivers/firmware/dmi.c, is not really parsing DMI.  It
is actually SMBIOS code that is using the old DMI infrastructure to expose
SMBIOS entries.

We should be doing the following:

1.  Looking for SMBIOS (either EFI or mapped to it's standard location,
    0xF0000)
2.  If found, look for the SMBIOS' Intermediate Structure (aka "DMI" entry)
3.  If not found, look for an "old" DMI structure.

DMI is a predecessor of SMBIOS, so we should start treating it as such.

For this patch I have introduced drivers/firmware/smbios.c, exported the
SMBIOS version through sysfs, and modified the DMI code such that

a) dmi_scan_machine() is now called from init_smbios(), and
b) if an SMBIOS is found, the values in the SMBIOS STEP structure are used
in dmi_scan_machine().

Right now, removing the DMI code is not an option.  It is used by common
initscripts, etc., to bringup the system.  Later modifications in this area will
include additional parsing of the SMBIOS structs and a simultaneous cleanup
of the DMI code.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index fcf3b43..ff2f1a3 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -94,6 +94,11 @@ config HAVE_SETUP_PER_CPU_AREA
 
 config DMI
 	bool
+	depends on SMBIOS
+	default y
+
+config SMBIOS
+	bool
 	default y
 
 config EFI
diff --git a/arch/ia64/include/asm/smbios.h b/arch/ia64/include/asm/smbios.h
new file mode 100644
index 0000000..cf04e8a
--- /dev/null
+++ b/arch/ia64/include/asm/smbios.h
@@ -0,0 +1,12 @@
+#ifndef _ASM_SMBIOS_H
+#define _ASM_SMBIOS_H
+
+#include <linux/slab.h>
+#include <asm/io.h>
+
+/* Use normal IO mappings for SMBIOS */
+#define smbios_ioremap ioremap
+#define smbios_iounmap(x, l) iounmap(x)
+#define smbios_alloc(l) kmalloc(l, GFP_ATOMIC)
+
+#endif
diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
index 911cf97..1bdb320 100644
--- a/arch/ia64/kernel/setup.c
+++ b/arch/ia64/kernel/setup.c
@@ -1061,9 +1061,9 @@ check_bugs (void)
 			       (unsigned long) __end___mckinley_e9_bundles);
 }
 
-static int __init run_dmi_scan(void)
+static int __init run_smbios_scan(void)
 {
-	dmi_scan_machine();
+	init_smbios();
 	return 0;
 }
-core_initcall(run_dmi_scan);
+core_initcall(run_smbios_scan);
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f8958b0..9d70dcd 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -637,6 +637,7 @@ config APB_TIMER
 # The code disables itself when not needed.
 config DMI
 	default y
+	depends on SMBIOS
 	bool "Enable DMI scanning" if EXPERT
 	---help---
 	  Enabled scanning of DMI to identify machine quirks. Say Y
@@ -644,6 +645,17 @@ config DMI
 	  affected by entries in the DMI blacklist. Required by PNP
 	  BIOS code.
 
+# Mark as expert because too many people got it wrong.
+# The code disables itself when not needed.
+config SMBIOS
+	default y
+	bool "Enable SMBIOS scan" if EXPERT
+	---help---
+	  Enables scanning of SMBIOS to identify machine quirks.  Say Y
+	  here unless you have verified that your setup is not
+	  affected by entries in the DMI and SMBIOS blacklists.  Required by
+	  PNP BIOS code
+
 config GART_IOMMU
 	bool "GART IOMMU support" if EXPERT
 	default y
diff --git a/arch/x86/include/asm/smbios.h b/arch/x86/include/asm/smbios.h
new file mode 100644
index 0000000..c5819ce
--- /dev/null
+++ b/arch/x86/include/asm/smbios.h
@@ -0,0 +1,19 @@
+#ifndef _ASM_X86_SMBIOS_H
+#define _ASM_X86_SMBIOS_H
+
+#include <linux/compiler.h>
+#include <linux/init.h>
+
+#include <asm/io.h>
+#include <asm/setup.h>
+
+static __always_inline __init void *smbios_alloc(unsigned len)
+{
+	return extend_brk(len, sizeof(int));
+}
+
+/* Use early IO mappings for SMIBIOS because it's initialized early */
+#define smbios_ioremap early_ioremap
+#define smbios_iounmap early_iounmap
+
+#endif /* _ASM_X86_SMBIOS_H */
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index b176f2b..a84930f 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -68,6 +68,7 @@
 #include <linux/percpu.h>
 #include <linux/crash_dump.h>
 #include <linux/tboot.h>
+#include <linux/smbios.h>
 
 #include <video/edid.h>
 
@@ -126,6 +127,9 @@ unsigned long max_pfn_mapped;
 #ifdef CONFIG_DMI
 RESERVE_BRK(dmi_alloc, 65536);
 #endif
+#ifdef CONFIG_SMBIOS
+RESERVE_BRK(smbios_alloc, 65536);
+#endif
 
 
 static __initdata unsigned long _brk_start = (unsigned long)__brk_base;
@@ -879,11 +883,11 @@ void __init setup_arch(char **cmdline_p)
 	if (efi_enabled)
 		efi_init();
 
-	dmi_scan_machine();
+	init_smbios();
 
 	/*
 	 * VMware detection requires dmi to be available, so this
-	 * needs to be done after dmi_scan_machine, for the BP.
+	 * needs to be done after init_smbios(), for the BP.
 	 */
 	init_hypervisor_platform();
 
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 1c3c173..fe3ade4 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_DMIID)		+= dmi-id.o
 obj-$(CONFIG_ISCSI_IBFT_FIND)	+= iscsi_ibft_find.o
 obj-$(CONFIG_ISCSI_IBFT)	+= iscsi_ibft.o
 obj-$(CONFIG_FIRMWARE_MEMMAP)	+= memmap.o
+obj-$(CONFIG_SMBIOS)		+= smbios.o
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index bcb1126..6080034 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -6,6 +6,7 @@
 #include <linux/dmi.h>
 #include <linux/efi.h>
 #include <linux/bootmem.h>
+#include <linux/smbios.h>
 #include <asm/dmi.h>
 
 /*
@@ -429,49 +430,37 @@ void __init dmi_scan_machine(void)
 	char __iomem *p, *q;
 	int rc;
 
-	if (efi_enabled) {
-		if (efi.smbios == EFI_INVALID_TABLE_ADDR)
-			goto error;
+	if (smbios_present) {
+		/* use values from SMBIOS STEP struct */
+		dmi_num = smbios_step->num_structs;
+		dmi_len = smbios_step->struct_len;
+		dmi_base = smbios_step->struct_addr;
 
-		/* This is called as a core_initcall() because it isn't
-		 * needed during early boot.  This also means we can
-		 * iounmap the space when we're done with it.
-		 */
-		p = dmi_ioremap(efi.smbios, 32);
-		if (p == NULL)
-			goto error;
-
-		rc = dmi_present(p + 0x10); /* offset of _DMI_ string */
-		dmi_iounmap(p, 32);
-		if (!rc) {
+		printk(KERN_INFO "DMI present.\n");
+		if (dmi_walk_early(dmi_decode) == 0) {
+			dmi_dump_ids();
 			dmi_available = 1;
-			goto out;
 		}
-	}
-	else {
-		/*
-		 * no iounmap() for that ioremap(); it would be a no-op, but
-		 * it's so early in setup that sucker gets confused into doing
-		 * what it shouldn't if we actually call it.
-		 */
+
+		dmi_initialized = 1;
+	} else {
+		/* Old school DMI table */
 		p = dmi_ioremap(0xF0000, 0x10000);
-		if (p == NULL)
-			goto error;
+		if (!p) {
+			printk(KERN_INFO "DMI not present.\n");
+			return;
+		}
 
 		for (q = p; q < p + 0x10000; q += 16) {
 			rc = dmi_present(q);
 			if (!rc) {
 				dmi_available = 1;
 				dmi_iounmap(p, 0x10000);
-				goto out;
+				return;
 			}
 		}
 		dmi_iounmap(p, 0x10000);
 	}
- error:
-	printk(KERN_INFO "DMI not present or invalid.\n");
- out:
-	dmi_initialized = 1;
 }
 
 /**
diff --git a/drivers/firmware/smbios.c b/drivers/firmware/smbios.c
new file mode 100644
index 0000000..19e59e1
--- /dev/null
+++ b/drivers/firmware/smbios.c
@@ -0,0 +1,175 @@
+#include <linux/dmi.h>
+#include <linux/efi.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/smbios.h>
+
+#include <asm/setup.h>
+
+int smbios_present = 0;
+struct smbios_step *smbios_step;
+static struct device *smbios_dev;
+/* The actual address of the SMBIOS */
+static unsigned long smbios_addr = 0;
+
+#ifdef CONFIG_SYSFS
+static ssize_t version_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%u.%u\n", smbios_step->major,
+		       smbios_step->minor);
+}
+
+/* add additional sysfs files here */
+static struct device_attribute smbios_attrs[] = {
+	__ATTR_RO(version),
+};
+
+static struct class smbios_class = {
+	.name = "smbios",
+	.dev_release = (void(*)(struct device *)) kfree,
+};
+
+static struct device *smbios_dev;
+
+static int __init smbios_setup_sysfs(void)
+{
+	int attr, i, ret = 0;
+
+	ret = class_register(&smbios_class);
+	if (ret)
+		goto out;
+
+	smbios_dev = kzalloc(sizeof(*smbios_dev), GFP_KERNEL);
+	if (!smbios_dev) {
+		ret = -ENOMEM;
+		goto out_cleanup_class;
+	}
+
+	smbios_dev->class = &smbios_class;
+	dev_set_name(smbios_dev, "id");
+
+	ret = device_register(smbios_dev);
+	if (ret)
+		goto out_cleanup_smbios_dev;
+
+	for (attr = 0; attr < ARRAY_SIZE(smbios_attrs); attr++) {
+		ret = device_create_file(smbios_dev, &smbios_attrs[attr]);
+		if (ret)
+			goto out_cleanup_files;
+	}
+
+	return 0;
+
+out_cleanup_files:
+	for (i = 0; i < attr; i++)
+		device_remove_file(smbios_dev, &smbios_attrs[attr]);
+	device_unregister(smbios_dev);
+out_cleanup_smbios_dev:
+	kfree(smbios_dev);
+out_cleanup_class:
+	class_unregister(&smbios_class);
+out:
+	return ret;
+}
+#endif /* CONFIG_SYSFS */
+
+static int __init smbios_decode_step(void)
+{
+	u8 chksum1, chksum2, *buf, fp;
+
+	if (!smbios_addr)
+		return -ENODEV;
+
+	smbios_step = smbios_alloc(sizeof(*smbios_step));
+	if (!smbios_step)
+		return -ENOMEM;
+
+	buf = smbios_ioremap(smbios_addr, sizeof(*smbios_step));
+	if (!buf)
+		return -ENODEV;
+
+	memcpy(smbios_step, buf, sizeof(*smbios_step));
+
+	/* checksum the entire STEP structure */
+	for (fp = 0; fp < smbios_step->length; fp++)
+		chksum1 += buf[fp];
+	if (chksum1) {
+		smbios_iounmap(buf, sizeof(*smbios_step));
+		printk(KERN_INFO "SMBIOS: Invalid STEP table checksum = %u\n",
+		       chksum1);
+		return -EINVAL;
+	}
+
+	/* checksum the Intermediate structure */
+	for (fp = 0x10; fp < smbios_step->length; fp++)
+		chksum2 += buf[fp];
+	if (chksum2) {
+		smbios_iounmap(buf, sizeof(*smbios_step));
+		printk(KERN_INFO "SMBIOS: Invalid Intermediate checksum = %u\n",
+		       chksum2);
+		return -EINVAL;
+	}
+
+	smbios_iounmap(buf, sizeof(*smbios_step));
+
+	printk(KERN_INFO "SMBIOS: version %u.%u @ 0x%lX\n",
+	       smbios_step->major, smbios_step->minor, smbios_addr);
+
+	/* is there a _DMI_ table? */
+	if (!(memcmp(smbios_step->dmi_string, "_DMI_", 5)))
+		dmi_scan_machine();
+	else
+		printk(KERN_INFO "SMBIOS: No DMI Table.\n");
+
+	return 0;
+}
+
+/* Should only be called from x86 system setup code */
+void __init init_smbios(void)
+{
+	u8 *buf;
+	int fp = 0, rc;
+
+	if (smbios_present)
+		return;
+
+	if (efi_enabled) {
+		if (efi.smbios == EFI_INVALID_TABLE_ADDR)
+			goto error;
+		smbios_addr = efi.smbios;
+	} else {
+		/* "Legacy" SMBIOS is mapped @ 0xF0000 */
+		smbios_addr = 0xF0000;
+	}
+	buf = smbios_ioremap(smbios_addr, 0x10000);
+	if (!buf)
+		goto error;
+
+	/* Find the entry point */
+	for (fp = 0; fp <= 0xFFF0; fp += 0x10)
+		if (!memcmp(buf + fp, "_SM_", 4)) {
+			smbios_addr += fp;
+			smbios_present = 1;
+			break;
+		}
+	smbios_iounmap(buf, 0x10000);
+
+	/* decode calls */
+	if (smbios_present) {
+		rc = smbios_decode_step();
+		if (rc)
+			return;
+	}
+error:
+	if (!smbios_present) {
+		printk(KERN_INFO "SMBIOS not present.");
+		/* call dmi_scan_table in case there is a "old" DMI table */
+		dmi_scan_machine();
+	}
+}
+
+#ifdef CONFIG_SYSFS
+arch_initcall(smbios_setup_sysfs);
+#endif
diff --git a/include/linux/smbios.h b/include/linux/smbios.h
new file mode 100644
index 0000000..774caaf
--- /dev/null
+++ b/include/linux/smbios.h
@@ -0,0 +1,51 @@
+#ifndef _SMBIOS_H
+#define _SMBIOS_H
+
+#include <linux/kernel.h>
+#include <asm/smbios.h>
+/*
+ * "The SMBIOS Specification addresses how motherboard and system vendors
+ * present management information about their products in a standard format by
+ * extending the BIOS interface on x86 architecture systems. The information is
+ * intended to allow generic instrumentation to deliver this information to
+ * management applications that use DMI, CIM or direct access, eliminating the
+ * need for error prone operations like probing system hardware for presence
+ * detection."
+ *
+ * From http://www.dmtf.org/standards/smbios
+ */
+
+/*
+ * SMBIOS Structure Table Entry Point, STEP
+ */
+
+#ifdef CONFIG_SMBIOS
+
+struct smbios_step {
+	char	anchor_string[4];	/* 0x0, always is "_SM_" */
+	u8	checksum;
+	u8	length;
+	u8	major;
+	u8	minor;
+	u16	max_size;
+	u8	eps_revision;
+	u8	reserved[5];
+	u8	dmi_string[5];	/* 0x10, should be "_DMI_" if available */
+	u8	intermediate_chksum;
+	u16	struct_len;
+	u32	struct_addr;
+	u16	num_structs;
+	u8	bcd_revision;
+} __packed;
+
+extern struct smbios_step *smbios_step;
+extern int smbios_present;
+
+extern void init_smbios(void);
+
+#else /* ifdef CONFIG_SMBIOS */
+
+void init_smbios(void) { return; }
+
+#endif
+#endif /* _SMBIOS_H */

Re: [PATCH]: SMBIOS: Add initial code and export version via sysfs

[Alan Cox]

> +static struct class smbios_class = {
> +	.name = "smbios",
> +	.dev_release = (void(*)(struct device *)) kfree,
> +};

I thought the policy was no new classes- Greg ?

> +		smbios_iounmap(buf, sizeof(*smbios_step));
> +		printk(KERN_INFO "SMBIOS: Invalid STEP table checksum = %u\n",
> +		       chksum1);

Surely this isn't INFO ?

Re: [PATCH]: SMBIOS: Add initial code and export version via sysfs

[Prarit Bhargava]

On 03/17/2011 03:09 PM, Alan Cox wrote:
>> +static struct class smbios_class = {
>> +	.name = "smbios",
>> +	.dev_release = (void(*)(struct device *)) kfree,
>> +};
>>     
> I thought the policy was no new classes- Greg ?
>   

Greg, eventually the SMBIOS code could replace the mixed DMI/SMBIOS code
we have right now.  If there is a "no new classes" policy, could you
give me an idea of what I should do/use instead?  The DMI stuff is
currently in its own class ...

>   
>> +		smbios_iounmap(buf, sizeof(*smbios_step));
>> +		printk(KERN_INFO "SMBIOS: Invalid STEP table checksum = %u\n",
>> +		       chksum1);
>>     
> Surely this isn't INFO ?
>
>   

Good point :)  I can certainly make that higher or a WARN_ON in the next
version.

P.

Re: [PATCH]: SMBIOS: Add initial code and export version via sysfs

[Greg KH]

On Thu, Mar 17, 2011 at 03:12:50PM -0400, Prarit Bhargava wrote:
> 
> 
> On 03/17/2011 03:09 PM, Alan Cox wrote:
> >> +static struct class smbios_class = {
> >> +	.name = "smbios",
> >> +	.dev_release = (void(*)(struct device *)) kfree,

Wait, what?  Heh, nice try, you can't do this.  If you can, your code is
doing something really wrong.  Or hacky like trying to put the device
structure as the first thing in your structure.  If that gets moved,
this will break badly.  Don't.

> >> +};
> >>     
> > I thought the policy was no new classes- Greg ?
> >   
> 
> Greg, eventually the SMBIOS code could replace the mixed DMI/SMBIOS code
> we have right now.  If there is a "no new classes" policy, could you
> give me an idea of what I should do/use instead?  The DMI stuff is
> currently in its own class ...

Ideally you should use a bus instead.  Let me go look at your original
patch and get back to you on this...

thanks,

greg k-h

Re: [PATCH]: SMBIOS: Add initial code and export version via sysfs

[Greg KH]

On Thu, Mar 17, 2011 at 09:57:54AM -0400, Prarit Bhargava wrote:
> The existing DMI code, drivers/firmware/dmi.c, is not really parsing DMI.  It
> is actually SMBIOS code that is using the old DMI infrastructure to expose
> SMBIOS entries.
> 
> We should be doing the following:
> 
> 1.  Looking for SMBIOS (either EFI or mapped to it's standard location,
>     0xF0000)
> 2.  If found, look for the SMBIOS' Intermediate Structure (aka "DMI" entry)
> 3.  If not found, look for an "old" DMI structure.
> 
> DMI is a predecessor of SMBIOS, so we should start treating it as such.

Why?  What is this change going to buy us in the end?  Heck, right now,
what's the benefit?

> For this patch I have introduced drivers/firmware/smbios.c, exported the
> SMBIOS version through sysfs, and modified the DMI code such that
> 
> a) dmi_scan_machine() is now called from init_smbios(), and
> b) if an SMBIOS is found, the values in the SMBIOS STEP structure are used
> in dmi_scan_machine().
> 
> Right now, removing the DMI code is not an option.  It is used by common
> initscripts, etc., to bringup the system.  Later modifications in this area will
> include additional parsing of the SMBIOS structs and a simultaneous cleanup
> of the DMI code.

Care to show follow-on patches that convert things to a manner which you
think it should look like?

> --- /dev/null
> +++ b/drivers/firmware/smbios.c
> @@ -0,0 +1,175 @@
> +#include <linux/dmi.h>
> +#include <linux/efi.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/smbios.h>
> +
> +#include <asm/setup.h>
> +
> +int smbios_present = 0;
> +struct smbios_step *smbios_step;
> +static struct device *smbios_dev;

A "raw" struct device?  Why?  You should never do this, that's not the
way to use a 'struct device'.

Let's back up here, what exactly are you trying to show in sysfs?  Where
are your Documentation/ABI/ entries that show these new files, what they
contain, and how to use them?

> +/* The actual address of the SMBIOS */
> +static unsigned long smbios_addr = 0;
> +
> +#ifdef CONFIG_SYSFS

You should never need this in your .c file.

> +static ssize_t version_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%u.%u\n", smbios_step->major,
> +		       smbios_step->minor);

As you "know" you are only returning a simple value, why the snprintf()?
Hint, don't, it's overkill and how do you "know" that the buffer is
PAGE_SIZE big?  Hint, you don't, so stick to a small buffer like you
are, and all is fine.

> +}
> +
> +/* add additional sysfs files here */
> +static struct device_attribute smbios_attrs[] = {
> +	__ATTR_RO(version),
> +};

So a whole new class just for a single version file in sysfs?

I'm confused, what exactly are you trying to show here that you can't
show in the existing device/class structure?

> +
> +static struct class smbios_class = {
> +	.name = "smbios",
> +	.dev_release = (void(*)(struct device *)) kfree,

As stated before, nice try, but no. :)


> +};
> +
> +static struct device *smbios_dev;
> +
> +static int __init smbios_setup_sysfs(void)
> +{
> +	int attr, i, ret = 0;
> +
> +	ret = class_register(&smbios_class);
> +	if (ret)
> +		goto out;
> +
> +	smbios_dev = kzalloc(sizeof(*smbios_dev), GFP_KERNEL);
> +	if (!smbios_dev) {
> +		ret = -ENOMEM;
> +		goto out_cleanup_class;
> +	}
> +
> +	smbios_dev->class = &smbios_class;
> +	dev_set_name(smbios_dev, "id");
> +
> +	ret = device_register(smbios_dev);
> +	if (ret)
> +		goto out_cleanup_smbios_dev;
> +
> +	for (attr = 0; attr < ARRAY_SIZE(smbios_attrs); attr++) {
> +		ret = device_create_file(smbios_dev, &smbios_attrs[attr]);

You just raced userspace when you created this file, causing it massive
problems if it was expecting when the hotplug event happened that this
file would be present.  Please don't do that, we have the infrastructure
to do this properly, please use it.

thanks,

greg k-h

Re: [PATCH]: SMBIOS: Add initial code and export version via sysfs

[Prarit Bhargava]

On 03/17/2011 03:30 PM, Greg KH wrote:
> On Thu, Mar 17, 2011 at 09:57:54AM -0400, Prarit Bhargava wrote:
>   
>> The existing DMI code, drivers/firmware/dmi.c, is not really parsing DMI.  It
>> is actually SMBIOS code that is using the old DMI infrastructure to expose
>> SMBIOS entries.
>>
>> We should be doing the following:
>>
>> 1.  Looking for SMBIOS (either EFI or mapped to it's standard location,
>>     0xF0000)
>> 2.  If found, look for the SMBIOS' Intermediate Structure (aka "DMI" entry)
>> 3.  If not found, look for an "old" DMI structure.
>>
>> DMI is a predecessor of SMBIOS, so we should start treating it as such.
>>     
> Why?  What is this change going to buy us in the end?  Heck, right now,
> what's the benefit?
>   

It's wrong.  DMI doesn't give us SMBIOS information -- it's the other
way around.  There's also a bunch of other stuff from SMBIOS being used
in the kernel (see pci slots and hotplug drivers).

It would be nice to have a single place for this, no?

>   
>> For this patch I have introduced drivers/firmware/smbios.c, exported the
>> SMBIOS version through sysfs, and modified the DMI code such that
>>
>> a) dmi_scan_machine() is now called from init_smbios(), and
>> b) if an SMBIOS is found, the values in the SMBIOS STEP structure are used
>> in dmi_scan_machine().
>>
>> Right now, removing the DMI code is not an option.  It is used by common
>> initscripts, etc., to bringup the system.  Later modifications in this area will
>> include additional parsing of the SMBIOS structs and a simultaneous cleanup
>> of the DMI code.
>>     
> Care to show follow-on patches that convert things to a manner which you
> think it should look like?
>
>   

I haven't written anything yet :) -- I was planning on exporting the
SMBIOS structs (all types) though.

>> --- /dev/null
>> +++ b/drivers/firmware/smbios.c
>> @@ -0,0 +1,175 @@
>> +#include <linux/dmi.h>
>> +#include <linux/efi.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/smbios.h>
>> +
>> +#include <asm/setup.h>
>> +
>> +int smbios_present = 0;
>> +struct smbios_step *smbios_step;
>> +static struct device *smbios_dev;
>>     
> A "raw" struct device?  Why?  You should never do this, that's not the
> way to use a 'struct device'.
>
>   

Ah, okay -- my familiarity with sysfs is very minimal.  I'll ask on
kernel-mentors for a pointer to "good" code.

> Let's back up here, what exactly are you trying to show in sysfs?  Where
> are your Documentation/ABI/ entries that show these new files, what they
> contain, and how to use them?
>   

Obviously missing -- I will add this on my next patch.

>   
>> +/* The actual address of the SMBIOS */
>> +static unsigned long smbios_addr = 0;
>> +
>> +#ifdef CONFIG_SYSFS
>>     
> You should never need this in your .c file.
>
>   

Will remove.

>> +static ssize_t version_show(struct device *dev,
>> +			    struct device_attribute *attr, char *buf)
>> +{
>> +	return snprintf(buf, PAGE_SIZE, "%u.%u\n", smbios_step->major,
>> +		       smbios_step->minor);
>>     
> As you "know" you are only returning a simple value, why the snprintf()?
> Hint, don't, it's overkill and how do you "know" that the buffer is
> PAGE_SIZE big?  Hint, you don't, so stick to a small buffer like you
> are, and all is fine.
>   

:)  Will do.  Thanks :)

>   
>> +}
>> +
>> +/* add additional sysfs files here */
>> +static struct device_attribute smbios_attrs[] = {
>> +	__ATTR_RO(version),
>> +};
>>     
> So a whole new class just for a single version file in sysfs?
>
> I'm confused, what exactly are you trying to show here that you can't
> show in the existing device/class structure?
>
>   
>> +
>> +static struct class smbios_class = {
>> +	.name = "smbios",
>> +	.dev_release = (void(*)(struct device *)) kfree,
>>     
> As stated before, nice try, but no. :)
>
>   

Right -- I'll go off and try to figure out the proper way to do this. 
Sorry ... I was following along with what the existing DMI code does.

>   
>> +};
>> +
>> +static struct device *smbios_dev;
>> +
>> +static int __init smbios_setup_sysfs(void)
>> +{
>> +	int attr, i, ret = 0;
>> +
>> +	ret = class_register(&smbios_class);
>> +	if (ret)
>> +		goto out;
>> +
>> +	smbios_dev = kzalloc(sizeof(*smbios_dev), GFP_KERNEL);
>> +	if (!smbios_dev) {
>> +		ret = -ENOMEM;
>> +		goto out_cleanup_class;
>> +	}
>> +
>> +	smbios_dev->class = &smbios_class;
>> +	dev_set_name(smbios_dev, "id");
>> +
>> +	ret = device_register(smbios_dev);
>> +	if (ret)
>> +		goto out_cleanup_smbios_dev;
>> +
>> +	for (attr = 0; attr < ARRAY_SIZE(smbios_attrs); attr++) {
>> +		ret = device_create_file(smbios_dev, &smbios_attrs[attr]);
>>     
> You just raced userspace when you created this file, causing it massive
> problems if it was expecting when the hotplug event happened that this
> file would be present.  Please don't do that, we have the infrastructure
> to do this properly, please use it.
>
>   

np -- thanks for the info Greg.

P.

> thanks,
>
> greg k-h
>   

Re: [PATCH]: SMBIOS: Add initial code and export version via sysfs

[Greg KH]

On Thu, Mar 17, 2011 at 03:55:04PM -0400, Prarit Bhargava wrote:
> 
> 
> On 03/17/2011 03:30 PM, Greg KH wrote:
> > On Thu, Mar 17, 2011 at 09:57:54AM -0400, Prarit Bhargava wrote:
> >   
> >> The existing DMI code, drivers/firmware/dmi.c, is not really parsing DMI.  It
> >> is actually SMBIOS code that is using the old DMI infrastructure to expose
> >> SMBIOS entries.
> >>
> >> We should be doing the following:
> >>
> >> 1.  Looking for SMBIOS (either EFI or mapped to it's standard location,
> >>     0xF0000)
> >> 2.  If found, look for the SMBIOS' Intermediate Structure (aka "DMI" entry)
> >> 3.  If not found, look for an "old" DMI structure.
> >>
> >> DMI is a predecessor of SMBIOS, so we should start treating it as such.
> >>     
> > Why?  What is this change going to buy us in the end?  Heck, right now,
> > what's the benefit?
> >   
> 
> It's wrong.  DMI doesn't give us SMBIOS information -- it's the other
> way around.  There's also a bunch of other stuff from SMBIOS being used
> in the kernel (see pci slots and hotplug drivers).
> 
> It would be nice to have a single place for this, no?

Ok, yes it would.

> >> For this patch I have introduced drivers/firmware/smbios.c, exported the
> >> SMBIOS version through sysfs, and modified the DMI code such that
> >>
> >> a) dmi_scan_machine() is now called from init_smbios(), and
> >> b) if an SMBIOS is found, the values in the SMBIOS STEP structure are used
> >> in dmi_scan_machine().
> >>
> >> Right now, removing the DMI code is not an option.  It is used by common
> >> initscripts, etc., to bringup the system.  Later modifications in this area will
> >> include additional parsing of the SMBIOS structs and a simultaneous cleanup
> >> of the DMI code.
> >>     
> > Care to show follow-on patches that convert things to a manner which you
> > think it should look like?
> >
> >   
> 
> I haven't written anything yet :) -- I was planning on exporting the
> SMBIOS structs (all types) though.

Sounds good, how are you going to do this?  In binary way (like efivars
does it) or in text files like the current DMI code does?

> >> --- /dev/null
> >> +++ b/drivers/firmware/smbios.c
> >> @@ -0,0 +1,175 @@
> >> +#include <linux/dmi.h>
> >> +#include <linux/efi.h>
> >> +#include <linux/init.h>
> >> +#include <linux/io.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/smbios.h>
> >> +
> >> +#include <asm/setup.h>
> >> +
> >> +int smbios_present = 0;
> >> +struct smbios_step *smbios_step;
> >> +static struct device *smbios_dev;
> >>     
> > A "raw" struct device?  Why?  You should never do this, that's not the
> > way to use a 'struct device'.
> >
> >   
> 
> Ah, okay -- my familiarity with sysfs is very minimal.  I'll ask on
> kernel-mentors for a pointer to "good" code.

It all depends on what you are trying to do.  What exactly are you
trying to achive?  What do you want to show in sysfs and how do you
expect it to look?

>From there we can then work backwards and create the code to do this.

thanks,

greg k-h

Re: [PATCH]: SMBIOS: Add initial code and export version via sysfs

[Prarit Bhargava]

Sorry Greg, I missed replying to one of your questions.

>>   
>>     
>>> +}
>>> +
>>> +/* add additional sysfs files here */
>>> +static struct device_attribute smbios_attrs[] = {
>>> +	__ATTR_RO(version),
>>> +};
>>>     
>>>       
>> So a whole new class just for a single version file in sysfs?
>>
>>     

Right now it is only the version file.  Eventually I was thinking that
this would grow to include sysfs entries for the BIOS info (date,
vendor, etc.) and Product and Chassis info, not to mention all the
various SMBIOS structs.  We have at least one driver that goes through a
lot of trouble to get this info, and IIRC, the PCI code now outputs
type41 and type9 fields for the network device naming stuff.

I thought that meant it needed a new class but, as you pointed out, that
isn't the case.  I'll ping kernel-mentors and see if anyone there might
point me to the right class for this data.

>> I'm confused, what exactly are you trying to show here that you can't
>> show in the existing device/class structure?
>>
>>     

I want to show the version of the SMBIOS on the system.  But it doesn't
seem to make sense to me to expose it in the DMI sysfs code.  The SMBIOS
provides the DMI code, not the other way around.  We've become used to
the DMI code, and we're adding SMBIOS decoding to it but the other parts
of the kernel are doing their own decoding of the SMBIOS to get
additional info.  To me, at least, this means we should have a central
place for it.

Thanks again,

P.

Re: [PATCH]: SMBIOS: Add initial code and export version via sysfs

[Greg KH]

On Thu, Mar 17, 2011 at 04:08:47PM -0400, Prarit Bhargava wrote:
> >>   
> >>     
> >>> +}
> >>> +
> >>> +/* add additional sysfs files here */
> >>> +static struct device_attribute smbios_attrs[] = {
> >>> +	__ATTR_RO(version),
> >>> +};
> >>>     
> >>>       
> >> So a whole new class just for a single version file in sysfs?
> >>
> >>     
> 
> Right now it is only the version file.  Eventually I was thinking that
> this would grow to include sysfs entries for the BIOS info (date,
> vendor, etc.) and Product and Chassis info, not to mention all the
> various SMBIOS structs.  We have at least one driver that goes through a
> lot of trouble to get this info, and IIRC, the PCI code now outputs
> type41 and type9 fields for the network device naming stuff.
> 
> I thought that meant it needed a new class but, as you pointed out, that
> isn't the case.  I'll ping kernel-mentors and see if anyone there might
> point me to the right class for this data.

See my other response for how to go about this.

> >> I'm confused, what exactly are you trying to show here that you can't
> >> show in the existing device/class structure?
> >>
> >>     
> 
> I want to show the version of the SMBIOS on the system.  But it doesn't
> seem to make sense to me to expose it in the DMI sysfs code.  The SMBIOS
> provides the DMI code, not the other way around.  We've become used to
> the DMI code, and we're adding SMBIOS decoding to it but the other parts
> of the kernel are doing their own decoding of the SMBIOS to get
> additional info.  To me, at least, this means we should have a central
> place for it.

Ok, that's fine.  But you can't remove the existing DMI files and
symlinks, so how are you anticipating handling all of that?

This seems like a lot of work at the moment just for a version file,
let's see some more use of this which would justify it being added to
the kernel first.

thanks,

greg k-h

Re: [PATCH]: SMBIOS: Add initial code and export version via sysfs

[Prarit Bhargava]

gregkh wrote:
>Sounds good, how are you going to do this?  In binary way (like efivars
>does it) or in text files like the current DMI code does?

gregkh wrote:
>What exactly are you
>trying to achive?  What do you want to show in sysfs and how do you
>expect it to look?

Greg, sorry for repeating myself in some of the below.  You're right in your
implication that I was not explaining myself very well.  I figured the
best thing to do was to start my argument from the beginning...

The last update to the DMI specification was in 2003.  It has since been
replaced by SMBIOS.  A new SMBIOS specification was released in February
2011.

While looking at the SMBIOS related code in the kernel, I noticed that the
DMI code was actually being overloaded to include SMBIOS structures and
that other sections of code (ia64 setup, x86 setup & pci, Dell platform
driver(s), the new PCI sysfs labe codel, hotplug drivers, the HP watchdog
driver, and the sigmatel sound driver, all used a method to derive values
from the SMBIOS structs.  Please note, that this list is very likely
incomplete.  I only did a search on 'smbios' in the kernel.

As I previously said, it would be nice if we did this in a single location
instead of throughout the kernel, and fix the overloading of the DMI code
currently in the kernel.

For now, my intention is to leave the code as similar to the DMI code as
possible.  That is, export the SMBIOS information via text files in
sysfs (although the layout of the files will obviously change, see below).

In this way we would limit the amount of change to external packages
(ie, initscripts, network init, etc.) to hopefully only filename changes.

I will create a new sysfs class, to temporarily exist next to the
current /sys/devices/virtual/dmi entry, which would export values from
SMBIOS.  I want to introduce this slowly and in several stages:

1.  Introduce the new SMBIOS infrastructure in a [relatively] simple patch,
and export the smbios version # to the kernel.

The model I had in mind was:

/sys/devices/virtual/smbios/id/

/sys/devices/virtual/smbios/id/[type#]/...

ie) a straight mapping of the smbios tables.  It should be up to the
individual
drivers & subsystems to link back to the files.  Of course, the SMBIOS code
would provide lookup functions similar to the ones provided by the existing
DMI code to aid drivers in their lookups.

2.  Export all of the SMBIOS information in the current (Feb 2011) spec.  If
all goes well I think the correct thing to do is to update
feature-removal-schedule.txt with a well-defined timeline at this point.

3.  Modify drivers to use the new SMBIOS table lookups, etc, instead of
DMI table lookups.  This is relatively easy, IMO.  I will try to keep
dmi* functions identical to the new smbios* lookup functions.  That code
works
well, and isn't in need of change.

4.  Notify external userspace programs of the change.  To be honest, I
haven't
thought much about this other than to take a couple of systems and see what
is using those values and let them know of the impending change.

[This next part is wishful thinking ;)] ... I suppose at some point it
would be
possible to put in a printk into the *existing* DMI sysfs files to indicate
that they were being deprecated.  I can certainly see how that would be
frowned upon; while it would be a loud nasty warning, it also might be one
of those warnings people just learn to ignore.

5.  After a release (or some other set period of time defined in
feature-removal-schedule.txt), remove the old DMI code.

I'm hoping to lean on what you did for the udev change, Greg.  IIRC, you
kept the old code and the new udev code in place for a while and then made
the final change after some time.  I can see that would be the best idea
here...

P.

Re: [PATCH]: SMBIOS: Add initial code and export version via sysfs

[Alan Cox]

> In this way we would limit the amount of change to external packages
> (ie, initscripts, network init, etc.) to hopefully only filename changes.

Why change them at all. SMBIOS provides everything DMI BIOS interfaces
did, so we should just continue to provide them to userspace exactly as
before, but possibly sourced from other locations. There simply isn't a
need to break anything user side. Maybe in five or ten years time someone
will delete the old stuff as nobody uses it any more but it's not an item
that needs hurrying.

Also distinguish between DMI and the DMI BIOS interfaces, the two are
rather different beasts.

> drivers & subsystems to link back to the files.  Of course, the SMBIOS code
> would provide lookup functions similar to the ones provided by the existing
> DMI code to aid drivers in their lookups.

I'm wondering why put all the SMBIOS complexity into the kernel, but then
I wonder likewise about some of the DMI stuff still 8)

> 3.  Modify drivers to use the new SMBIOS table lookups, etc, instead of
> DMI table lookups.  This is relatively easy, IMO.  I will try to keep
> dmi* functions identical to the new smbios* lookup functions.  That code
> works
> well, and isn't in need of change.

I'd actually suggest one change. In using dmi_ naming and DMI alone the
original code made the assumption it was a PC specific tool for doing
fixups and the like. If we are going to tweak it then we should probably
recognize that want the kernel needs to provide to drivers isn't (for the
most case) SMBIOS lookup it's generic "Platform name" etc and these could
have been populated from things other than SMBIOS (OpenFirmware,
DeviceTree, whatever).

It doesn't really change the interface but if you are going to be
renaming things maybe a better naming would be worth it ?

> 5.  After a release (or some other set period of time defined in
> feature-removal-schedule.txt), remove the old DMI code.

This is a user API so really it wants to hang around for a long time, and
given SMBIOS can just provide the same names as well trivially it might
as well just stay there until no distro is using it, then get a _LEGACY
config, then vanish. Think multiples of years.

Alan

Re: [PATCH]: SMBIOS: Add initial code and export version via sysfs

[Prarit Bhargava]

On 03/21/2011 12:06 PM, Alan Cox wrote:
>> In this way we would limit the amount of change to external packages
>> (ie, initscripts, network init, etc.) to hopefully only filename changes.
>>     
> Why change them at all. SMBIOS provides everything DMI BIOS interfaces
> did, so we should just continue to provide them to userspace exactly as
> before, but possibly sourced from other locations. There simply isn't a
> need to break anything user side. Maybe in five or ten years time someone
> will delete the old stuff as nobody uses it any more but it's not an item
> that needs hurrying.
>   

I'm sorry Alan -- I meant to say that the change would be minimal -- it
is likely with a new sysfs mapping that
/sys/devices/virtual/id/dmi would stay the same, but I'm not willing to
guarantee it.

It *might* turn out to be /sys/devices/virtual/id/smbios/dmi .

I'm really not sure which I prefer.

> Also distinguish between DMI and the DMI BIOS interfaces, the two are
> rather different beasts.
>
>   
>> drivers & subsystems to link back to the files.  Of course, the SMBIOS code
>> would provide lookup functions similar to the ones provided by the existing
>> DMI code to aid drivers in their lookups.
>>     
> I'm wondering why put all the SMBIOS complexity into the kernel, but then
> I wonder likewise about some of the DMI stuff still 8)
>
>   

Well, like I said before, there are several drivers that are each doing
their own thing.  Given the past history of the kernel this should be
centralized to one location.  Having drivers each "dig" through mem is
not the best thing IMO.

>> 3.  Modify drivers to use the new SMBIOS table lookups, etc, instead of
>> DMI table lookups.  This is relatively easy, IMO.  I will try to keep
>> dmi* functions identical to the new smbios* lookup functions.  That code
>> works
>> well, and isn't in need of change.
>>     
> I'd actually suggest one change. In using dmi_ naming and DMI alone the
> original code made the assumption it was a PC specific tool for doing
> fixups and the like. If we are going to tweak it then we should probably
> recognize that want the kernel needs to provide to drivers isn't (for the
> most case) SMBIOS lookup it's generic "Platform name" etc and these could
> have been populated from things other than SMBIOS (OpenFirmware,
> DeviceTree, whatever).
>
>   

Good idea :)  Thanks -- I'll incorporate that into my next proposal. 
IMO that's much better ... and it lends its hand to other arches if they
wish to do it.

> It doesn't really change the interface but if you are going to be
> renaming things maybe a better naming would be worth it ?
>
>   

IMO yes.  The more general the better IMO.  Why have several different
Platform Firmware implementations.

So .. maybe /sys/devices/virtual/platform ?  I dunno ... maybe someone
reading can come up with a better idea.


>> 5.  After a release (or some other set period of time defined in
>> feature-removal-schedule.txt), remove the old DMI code.
>>     
> This is a user API so really it wants to hang around for a long time, and
> given SMBIOS can just provide the same names as well trivially it might
> as well just stay there until no distro is using it, then get a _LEGACY
> config, then vanish. Think multiples of years.
>
>   

Okay.  Will do.

P.
> Alan
>