[PATCH] Export smbios strings associated with onboard devices to sysfs

[PATCH] Export smbios strings associated with onboard devices to sysfs

[Narendra K]

Hello,

[Please let me know if any other list needs to be copied]

* We have been having discussions in the netdev list about creating multiple names for the network interfaces to bring determinism into the way network interfaces are named in the OSes. In specific, "eth0 in the OS does not always map to the integrated NIC Gb1 as labelled on the chassis".  

http://marc.info/?l=linux-netdev&m=125510301513312&w=2 - (Re: PATCH: Network Device Naming mechanism and policy)
http://marc.info/?l=linux-netdev&m=125619338904322&w=2 - ([PATCH] udev: create empty regular files to represent net)

This was not favored.

* We also had proposed an installer based approach where installers would provide options to rename kernel eth names to namespaces based on different policies like smbios names etc. That was also not favored.

As a result of these discussions we propose another solution - 

1.Export smbios strings of onboard devices, to sysfs. For example -

cat /sys/class/net/eth0/device/smbiosname
Embedded NIC 2

cat  /sys/bus/pci/devices/0000\:03\:00.0/smbiosname
Embedded NIC 2

2. User space library like libnetdevname would map these smbiosname names to eth names and eth names to smbiosnames

3. User space tools would use the smbios alias names in addition to the eth names they already support. This ensures that, right interface is referred to, irrespective of how the interface is named by the kernel (eth0 or eth1..).

For example:

/sbin/ethtool -p "Embedded NIC 1" (Might be eth0 or eth1)
/sbin/ip link show "Embedded NIC 2"

Please refer to - 

* http://linux.dell.com/wiki/index.php/Oss/libnetdevname#Linux_Enumeration_of_NICs_in_the_Enterprise -  for more information for more information on the issue we are trying to address
* http://linux.dell.com/wiki/index.php/Oss/libnetdevname#Proposal_2:  - for more information on the current proposal.

Submitting the patch which addresses point 1 mentioned above for review. 


Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
Signed-off-by: Narendra K <Narendra_K@dell.com>
---
 drivers/base/bus.c          |    5 +++++
 drivers/firmware/dmi_scan.c |   23 +++++++++++++++++++++++
 drivers/pci/pci-sysfs.c     |   38 ++++++++++++++++++++++++++++++++++++++
 include/linux/device.h      |    4 ++++
 include/linux/dmi.h         |    9 +++++++++
 5 files changed, 79 insertions(+), 0 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index c0c5a43..ad0fcfb 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -419,6 +419,11 @@ static int device_add_attrs(struct bus_type *bus, struct device *dev)
 		return 0;
 
 	for (i = 0; attr_name(bus->dev_attrs[i]); i++) {
+		/* if the device does not have an associated smbios string in the smbios table, do not create this attribute */ 
+		if (!(strcmp(attr_name(bus->dev_attrs[i]), "smbiosname"))) {
+			if (!smbiosname_string_is_valid(dev, NULL)) 
+				continue;
+		}
 		error = device_create_file(dev, &bus->dev_attrs[i]);
 		if (error) {
 			while (--i >= 0)
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 31b983d..1d10663 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -278,6 +278,28 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
 	list_add_tail(&dev->list, &dmi_devices);
 }
 
+static void __init dmi_save_devslot(int id, int seg, int bus, int devfn, const char *name)
+{
+	struct dmi_devslot *slot;
+
+	slot = dmi_alloc(sizeof(*slot) + strlen(name) + 1);
+	if (!slot) {
+		printk(KERN_ERR "dmi_save_devslot: out of memory.\n");
+		return;
+	}	
+	slot->id = id;
+	slot->seg = seg;
+	slot->bus = bus;
+	slot->devfn = devfn;
+
+	strcpy((char *)&slot[1], name);
+	slot->dev.type = DMI_DEV_TYPE_DEVSLOT;
+	slot->dev.name = &slot[1];
+	slot->dev.device_data = slot;
+
+	list_add(&slot->dev.list, &dmi_devices);
+}
+
 static void __init dmi_save_extended_devices(const struct dmi_header *dm)
 {
 	const u8 *d = (u8*) dm + 5;
@@ -286,6 +308,7 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
 	if ((*d & 0x80) == 0)
 		return;
 
+	dmi_save_devslot(-1, *(u16 *)(d+2), *(d+4), *(d+5), dmi_string_nosave(dm, *(d-1)));
 	dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
 }
 
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index c5df94e..a3b9bf9 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -140,6 +140,41 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
 		       (u8)(pci_dev->class));
 }
 
+#ifdef CONFIG_DMI
+#include <linux/dmi.h>
+ssize_t
+smbiosname_string_is_valid(struct device *dev, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+  	struct dmi_device *dmi;
+  	struct dmi_devslot *dslot;
+  	int bus;
+  	int devfn;
+
+  	bus = pdev->bus->number;
+  	devfn = pdev->devfn;
+
+  	dmi = NULL;
+  	while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEVSLOT, NULL, dmi)) != NULL) {
+    		dslot = dmi->device_data;
+    		if (dslot && dslot->bus == bus && dslot->devfn == devfn) {
+			if (buf)
+      				return scnprintf(buf, PAGE_SIZE, "%s\n", dmi->name);
+			return strlen(dmi->name);
+		}
+		
+	}
+}
+EXPORT_SYMBOL(smbiosname_string_is_valid);
+
+static ssize_t
+smbiosname_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return smbiosname_string_is_valid(dev, buf);
+
+}
+#endif
+
 static ssize_t is_enabled_store(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)
@@ -324,6 +359,9 @@ struct device_attribute pci_dev_attrs[] = {
 	__ATTR_RO(class),
 	__ATTR_RO(irq),
 	__ATTR_RO(local_cpus),
+#ifdef CONFIG_DMI
+	__ATTR_RO(smbiosname),
+#endif
 	__ATTR_RO(local_cpulist),
 	__ATTR_RO(modalias),
 #ifdef CONFIG_NUMA
diff --git a/include/linux/device.h b/include/linux/device.h
index a62799f..647246c 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -427,6 +427,10 @@ struct device {
 	void	(*release)(struct device *dev);
 };
 
+#ifdef CONFIG_DMI
+extern ssize_t smbiosname_string_is_valid(struct device *, char *);
+#endif
+
 /* Get the wakeup routines, which depend on struct device */
 #include <linux/pm_wakeup.h>
 
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index a8a3e1a..cc57c3a 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -20,6 +20,7 @@ enum dmi_device_type {
 	DMI_DEV_TYPE_SAS,
 	DMI_DEV_TYPE_IPMI = -1,
 	DMI_DEV_TYPE_OEM_STRING = -2,
+	DMI_DEV_TYPE_DEVSLOT = -3,
 };
 
 struct dmi_header {
@@ -37,6 +38,14 @@ struct dmi_device {
 
 #ifdef CONFIG_DMI
 
+struct dmi_devslot {
+	struct dmi_device dev;
+	int id;
+	int seg;
+	int bus;
+	int devfn;
+};
+
 extern int dmi_check_system(const struct dmi_system_id *list);
 const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
 extern const char * dmi_get_system_info(int field);
-- 
1.6.5.2

With regards,
Narendra K

[PATCH] Export smbios strings associated with onboard devices to

[Narendra K]

Hello,

[Please let me know if any other list needs to be copied]

* We have been having discussions in the netdev list about creating multiple names for the network interfaces to bring determinism into the way network interfaces are named in the OSes. In specific, "eth0 in the OS does not always map to the integrated NIC Gb1 as labelled on the chassis".  

http://marc.info/?l=linux-netdev&m\x125510301513312&w=2 - (Re: PATCH: Network Device Naming mechanism and policy)
http://marc.info/?l=linux-netdev&m\x125619338904322&w=2 - ([PATCH] udev: create empty regular files to represent net)

This was not favored.

* We also had proposed an installer based approach where installers would provide options to rename kernel eth names to namespaces based on different policies like smbios names etc. That was also not favored.

As a result of these discussions we propose another solution - 

1.Export smbios strings of onboard devices, to sysfs. For example -

cat /sys/class/net/eth0/device/smbiosname
Embedded NIC 2

cat  /sys/bus/pci/devices/0000\:03\:00.0/smbiosname
Embedded NIC 2

2. User space library like libnetdevname would map these smbiosname names to eth names and eth names to smbiosnames

3. User space tools would use the smbios alias names in addition to the eth names they already support. This ensures that, right interface is referred to, irrespective of how the interface is named by the kernel (eth0 or eth1..).

For example:

/sbin/ethtool -p "Embedded NIC 1" (Might be eth0 or eth1)
/sbin/ip link show "Embedded NIC 2"

Please refer to - 

* http://linux.dell.com/wiki/index.php/Oss/libnetdevname#Linux_Enumeration_of_NICs_in_the_Enterprise -  for more information for more information on the issue we are trying to address
* http://linux.dell.com/wiki/index.php/Oss/libnetdevname#Proposal_2:  - for more information on the current proposal.

Submitting the patch which addresses point 1 mentioned above for review. 


Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
Signed-off-by: Narendra K <Narendra_K@dell.com>
---
 drivers/base/bus.c          |    5 +++++
 drivers/firmware/dmi_scan.c |   23 +++++++++++++++++++++++
 drivers/pci/pci-sysfs.c     |   38 ++++++++++++++++++++++++++++++++++++++
 include/linux/device.h      |    4 ++++
 include/linux/dmi.h         |    9 +++++++++
 5 files changed, 79 insertions(+), 0 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index c0c5a43..ad0fcfb 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -419,6 +419,11 @@ static int device_add_attrs(struct bus_type *bus, struct device *dev)
 		return 0;
 
 	for (i = 0; attr_name(bus->dev_attrs[i]); i++) {
+		/* if the device does not have an associated smbios string in the smbios table, do not create this attribute */ 
+		if (!(strcmp(attr_name(bus->dev_attrs[i]), "smbiosname"))) {
+			if (!smbiosname_string_is_valid(dev, NULL)) 
+				continue;
+		}
 		error = device_create_file(dev, &bus->dev_attrs[i]);
 		if (error) {
 			while (--i >= 0)
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 31b983d..1d10663 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -278,6 +278,28 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
 	list_add_tail(&dev->list, &dmi_devices);
 }
 
+static void __init dmi_save_devslot(int id, int seg, int bus, int devfn, const char *name)
+{
+	struct dmi_devslot *slot;
+
+	slot = dmi_alloc(sizeof(*slot) + strlen(name) + 1);
+	if (!slot) {
+		printk(KERN_ERR "dmi_save_devslot: out of memory.\n");
+		return;
+	}	
+	slot->id = id;
+	slot->seg = seg;
+	slot->bus = bus;
+	slot->devfn = devfn;
+
+	strcpy((char *)&slot[1], name);
+	slot->dev.type = DMI_DEV_TYPE_DEVSLOT;
+	slot->dev.name = &slot[1];
+	slot->dev.device_data = slot;
+
+	list_add(&slot->dev.list, &dmi_devices);
+}
+
 static void __init dmi_save_extended_devices(const struct dmi_header *dm)
 {
 	const u8 *d = (u8*) dm + 5;
@@ -286,6 +308,7 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
 	if ((*d & 0x80) = 0)
 		return;
 
+	dmi_save_devslot(-1, *(u16 *)(d+2), *(d+4), *(d+5), dmi_string_nosave(dm, *(d-1)));
 	dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
 }
 
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index c5df94e..a3b9bf9 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -140,6 +140,41 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
 		       (u8)(pci_dev->class));
 }
 
+#ifdef CONFIG_DMI
+#include <linux/dmi.h>
+ssize_t
+smbiosname_string_is_valid(struct device *dev, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+  	struct dmi_device *dmi;
+  	struct dmi_devslot *dslot;
+  	int bus;
+  	int devfn;
+
+  	bus = pdev->bus->number;
+  	devfn = pdev->devfn;
+
+  	dmi = NULL;
+  	while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEVSLOT, NULL, dmi)) != NULL) {
+    		dslot = dmi->device_data;
+    		if (dslot && dslot->bus = bus && dslot->devfn = devfn) {
+			if (buf)
+      				return scnprintf(buf, PAGE_SIZE, "%s\n", dmi->name);
+			return strlen(dmi->name);
+		}
+		
+	}
+}
+EXPORT_SYMBOL(smbiosname_string_is_valid);
+
+static ssize_t
+smbiosname_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return smbiosname_string_is_valid(dev, buf);
+
+}
+#endif
+
 static ssize_t is_enabled_store(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)
@@ -324,6 +359,9 @@ struct device_attribute pci_dev_attrs[] = {
 	__ATTR_RO(class),
 	__ATTR_RO(irq),
 	__ATTR_RO(local_cpus),
+#ifdef CONFIG_DMI
+	__ATTR_RO(smbiosname),
+#endif
 	__ATTR_RO(local_cpulist),
 	__ATTR_RO(modalias),
 #ifdef CONFIG_NUMA
diff --git a/include/linux/device.h b/include/linux/device.h
index a62799f..647246c 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -427,6 +427,10 @@ struct device {
 	void	(*release)(struct device *dev);
 };
 
+#ifdef CONFIG_DMI
+extern ssize_t smbiosname_string_is_valid(struct device *, char *);
+#endif
+
 /* Get the wakeup routines, which depend on struct device */
 #include <linux/pm_wakeup.h>
 
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index a8a3e1a..cc57c3a 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -20,6 +20,7 @@ enum dmi_device_type {
 	DMI_DEV_TYPE_SAS,
 	DMI_DEV_TYPE_IPMI = -1,
 	DMI_DEV_TYPE_OEM_STRING = -2,
+	DMI_DEV_TYPE_DEVSLOT = -3,
 };
 
 struct dmi_header {
@@ -37,6 +38,14 @@ struct dmi_device {
 
 #ifdef CONFIG_DMI
 
+struct dmi_devslot {
+	struct dmi_device dev;
+	int id;
+	int seg;
+	int bus;
+	int devfn;
+};
+
 extern int dmi_check_system(const struct dmi_system_id *list);
 const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
 extern const char * dmi_get_system_info(int field);
-- 
1.6.5.2

With regards,
Narendra K

Re: [PATCH] Export smbios strings associated with onboard devices to sysfs

[Domsch, Matt]

On Thu, Feb 25, 2010 at 02:29:42PM -0600, K, Narendra wrote:
> 1.Export smbios strings of onboard devices, to sysfs. For example -

I like the idea in this patch, which exports this additional
smbios-provided string in sysfs.  This removes the need to parse the
SMBIOS table in userspace, which requires root privs.

The concept is also extensible to other methods (e.g. ACPI) that I
expect will appear for the platform BIOS to provide naming hints to
the OS - I want those to appear in sysfs also.

now, for the patch:

> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index c0c5a43..ad0fcfb 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -419,6 +419,11 @@ static int device_add_attrs(struct bus_type *bus, struct device *dev)
>  		return 0;
>  
>  	for (i = 0; attr_name(bus->dev_attrs[i]); i++) {
> +		/* if the device does not have an associated smbios string in the smbios table, do not create this attribute */ 
> +		if (!(strcmp(attr_name(bus->dev_attrs[i]), "smbiosname"))) {
> +			if (!smbiosname_string_is_valid(dev, NULL)) 
> +				continue;
> +		}
>  		error = device_create_file(dev, &bus->dev_attrs[i]);
>  		if (error) {
>  			while (--i >= 0)

This cannot go in bus.c.  It needs to go in pci-sysfs.c.

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index c5df94e..a3b9bf9 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -324,6 +359,9 @@ struct device_attribute pci_dev_attrs[] = {
>  	__ATTR_RO(class),
>  	__ATTR_RO(irq),
>  	__ATTR_RO(local_cpus),
> +#ifdef CONFIG_DMI
> +	__ATTR_RO(smbiosname),
> +#endif
>  	__ATTR_RO(local_cpulist),

Here, instead of __ATTR_RO(smbiosname), and then finding and ignoring
it in the device core, you need to make the addition of this dynamic.
drivers/firmware/edd.c does something quite like this:

#define __ATTR_RO_TEST(_name, _test) { \
        .attr   = { .name = __stringify(_name), .mode = 0444 }, \
        .show   = _name##_show,                                 \
	.test   = _test,                                        \
}

so it defines a test function to apply before creating the attribute.
See edd.c:edd_populate_dir() for how the .test is evaluated before
calling sysfs_create_file().

Can this be genericized?  I'm sure these two places aren't the only
places where attributes may or may not exist on an object.

Thanks,
Matt

-- 
Matt Domsch
Technology Strategist, Dell Office of the CTO
linux.dell.com & www.dell.com/linux

Re: [PATCH] Export smbios strings associated with onboard devices to

[Domsch, Matt]

On Thu, Feb 25, 2010 at 02:29:42PM -0600, K, Narendra wrote:
> 1.Export smbios strings of onboard devices, to sysfs. For example -

I like the idea in this patch, which exports this additional
smbios-provided string in sysfs.  This removes the need to parse the
SMBIOS table in userspace, which requires root privs.

The concept is also extensible to other methods (e.g. ACPI) that I
expect will appear for the platform BIOS to provide naming hints to
the OS - I want those to appear in sysfs also.

now, for the patch:

> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index c0c5a43..ad0fcfb 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -419,6 +419,11 @@ static int device_add_attrs(struct bus_type *bus, struct device *dev)
>  		return 0;
>  
>  	for (i = 0; attr_name(bus->dev_attrs[i]); i++) {
> +		/* if the device does not have an associated smbios string in the smbios table, do not create this attribute */ 
> +		if (!(strcmp(attr_name(bus->dev_attrs[i]), "smbiosname"))) {
> +			if (!smbiosname_string_is_valid(dev, NULL)) 
> +				continue;
> +		}
>  		error = device_create_file(dev, &bus->dev_attrs[i]);
>  		if (error) {
>  			while (--i >= 0)

This cannot go in bus.c.  It needs to go in pci-sysfs.c.

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index c5df94e..a3b9bf9 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -324,6 +359,9 @@ struct device_attribute pci_dev_attrs[] = {
>  	__ATTR_RO(class),
>  	__ATTR_RO(irq),
>  	__ATTR_RO(local_cpus),
> +#ifdef CONFIG_DMI
> +	__ATTR_RO(smbiosname),
> +#endif
>  	__ATTR_RO(local_cpulist),

Here, instead of __ATTR_RO(smbiosname), and then finding and ignoring
it in the device core, you need to make the addition of this dynamic.
drivers/firmware/edd.c does something quite like this:

#define __ATTR_RO_TEST(_name, _test) { \
        .attr   = { .name = __stringify(_name), .mode = 0444 }, \
        .show   = _name##_show,                                 \
	.test   = _test,                                        \
}

so it defines a test function to apply before creating the attribute.
See edd.c:edd_populate_dir() for how the .test is evaluated before
calling sysfs_create_file().

Can this be genericized?  I'm sure these two places aren't the only
places where attributes may or may not exist on an object.

Thanks,
Matt

-- 
Matt Domsch
Technology Strategist, Dell Office of the CTO
linux.dell.com & www.dell.com/linux

Re: [PATCH] Export smbios strings associated with onboard devices to sysfs

[Narendra K]

> > -----Original Message-----
> > From: Domsch, Matt [mailto:Matt_Domsch@Dell.com]
> > Sent: Friday, February 26, 2010 2:19 AM
> > To: K, Narendra
> > Cc: netdev@vger.kernel.org; linux-hotplug@vger.kernel.org; linux-
> > pci@vger.kernel.org; Hargrave, Jordan; Shandilya, Sandeep K; Rose,
> > Charles; Iyer, Shyam
> > Subject: Re: [PATCH] Export smbios strings associated with onboard
> > devices to sysfs
> > 
> > On Thu, Feb 25, 2010 at 02:29:42PM -0600, K, Narendra wrote:
> > > 1.Export smbios strings of onboard devices, to sysfs. For example -
> > 
> > I like the idea in this patch, which exports this additional
> > smbios-provided string in sysfs.  This removes the need to parse the
> > SMBIOS table in userspace, which requires root privs.
> > 
> > The concept is also extensible to other methods (e.g. ACPI) that I
> > expect will appear for the platform BIOS to provide naming hints to
> > the OS - I want those to appear in sysfs also.
> > 
> > now, for the patch:
> > 
> > > +		if (!(strcmp(attr_name(bus->dev_attrs[i]),
> "smbiosname")))
> > {
> > > +			if (!smbiosname_string_is_valid(dev, NULL))
> > > +				continue;
> > > +		}
> > >  		error = device_create_file(dev, &bus->dev_attrs[i]);
> > >  		if (error) {
> > >  			while (--i >= 0)
> > 
> > This cannot go in bus.c.  It needs to go in pci-sysfs.c.
> > 

Resending the patch with review comments incorporated.

Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
Signed-off-by: Narendra K <Narendra_K@dell.com>
---
 drivers/firmware/dmi_scan.c |   23 ++++++++++++++++++
 drivers/pci/pci-sysfs.c     |   54 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/dmi.h         |    9 +++++++
 3 files changed, 86 insertions(+), 0 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 31b983d..1d10663 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -278,6 +278,28 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
 	list_add_tail(&dev->list, &dmi_devices);
 }
 
+static void __init dmi_save_devslot(int id, int seg, int bus, int devfn, const char *name)
+{
+	struct dmi_devslot *slot;
+
+	slot = dmi_alloc(sizeof(*slot) + strlen(name) + 1);
+	if (!slot) {
+		printk(KERN_ERR "dmi_save_devslot: out of memory.\n");
+		return;
+	}	
+	slot->id = id;
+	slot->seg = seg;
+	slot->bus = bus;
+	slot->devfn = devfn;
+
+	strcpy((char *)&slot[1], name);
+	slot->dev.type = DMI_DEV_TYPE_DEVSLOT;
+	slot->dev.name = &slot[1];
+	slot->dev.device_data = slot;
+
+	list_add(&slot->dev.list, &dmi_devices);
+}
+
 static void __init dmi_save_extended_devices(const struct dmi_header *dm)
 {
 	const u8 *d = (u8*) dm + 5;
@@ -286,6 +308,7 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
 	if ((*d & 0x80) == 0)
 		return;
 
+	dmi_save_devslot(-1, *(u16 *)(d+2), *(d+4), *(d+5), dmi_string_nosave(dm, *(d-1)));
 	dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
 }
 
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 807224e..85d5d79 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -142,6 +142,54 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
 		       (u8)(pci_dev->class));
 }
 
+#ifdef CONFIG_DMI
+#include <linux/dmi.h>
+static ssize_t
+smbiosname_string_is_valid(struct device *dev, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+  	struct dmi_device *dmi;
+  	struct dmi_devslot *dslot;
+  	int bus;
+  	int devfn;
+
+  	bus = pdev->bus->number;
+  	devfn = pdev->devfn;
+
+  	dmi = NULL;
+  	while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEVSLOT, NULL, dmi)) != NULL) {
+    		dslot = dmi->device_data;
+    		if (dslot && dslot->bus == bus && dslot->devfn == devfn) {
+			if (buf)
+      				return scnprintf(buf, PAGE_SIZE, "%s\n", dmi->name);
+			return strlen(dmi->name);
+		}
+	}
+}
+
+static ssize_t
+smbiosname_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return smbiosname_string_is_valid(dev, buf);
+
+}
+
+struct smbios_attribute {
+	struct attribute attr;
+	ssize_t(*show) (struct device * edev, char *buf);
+	int (*test) (struct device * edev, char *buf);
+};
+
+#define SMBIOSNAME_DEVICE_ATTR(_name, _mode, _show, _test) \
+struct smbios_attribute smbios_attr_##_name = { 	\
+	.attr = {.name = __stringify(_name), .mode = _mode },	\
+	.show	= _show,				\
+	.test	= _test,				\
+};
+
+static SMBIOSNAME_DEVICE_ATTR(smbiosname, 0444, smbiosname_show, smbiosname_string_is_valid);
+#endif
+
 static ssize_t is_enabled_store(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)
@@ -1140,6 +1188,7 @@ static int __init pci_sysfs_init(void)
 {
 	struct pci_dev *pdev = NULL;
 	int retval;
+	struct smbios_attribute *attr = &smbios_attr_smbiosname;
 
 	sysfs_initialized = 1;
 	for_each_pci_dev(pdev) {
@@ -1148,6 +1197,11 @@ static int __init pci_sysfs_init(void)
 			pci_dev_put(pdev);
 			return retval;
 		}
+#ifdef CONFIG_DMI
+		/* if the device does not have an associated smbios string in the smbios table, do not create this attribute */ 
+		if (attr->test && attr->test(&pdev->dev, NULL))
+			sysfs_create_file(&pdev->dev.kobj, &attr->attr);
+#endif
 	}
 
 	return 0;
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index a8a3e1a..cc57c3a 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -20,6 +20,7 @@ enum dmi_device_type {
 	DMI_DEV_TYPE_SAS,
 	DMI_DEV_TYPE_IPMI = -1,
 	DMI_DEV_TYPE_OEM_STRING = -2,
+	DMI_DEV_TYPE_DEVSLOT = -3,
 };
 
 struct dmi_header {
@@ -37,6 +38,14 @@ struct dmi_device {
 
 #ifdef CONFIG_DMI
 
+struct dmi_devslot {
+	struct dmi_device dev;
+	int id;
+	int seg;
+	int bus;
+	int devfn;
+};
+
 extern int dmi_check_system(const struct dmi_system_id *list);
 const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
 extern const char * dmi_get_system_info(int field);
-- 
1.6.5.2

With regards,
Narendra K

Re: [PATCH] Export smbios strings associated with onboard devices

[Narendra K]

> > -----Original Message-----
> > From: Domsch, Matt [mailto:Matt_Domsch@Dell.com]
> > Sent: Friday, February 26, 2010 2:19 AM
> > To: K, Narendra
> > Cc: netdev@vger.kernel.org; linux-hotplug@vger.kernel.org; linux-
> > pci@vger.kernel.org; Hargrave, Jordan; Shandilya, Sandeep K; Rose,
> > Charles; Iyer, Shyam
> > Subject: Re: [PATCH] Export smbios strings associated with onboard
> > devices to sysfs
> > 
> > On Thu, Feb 25, 2010 at 02:29:42PM -0600, K, Narendra wrote:
> > > 1.Export smbios strings of onboard devices, to sysfs. For example -
> > 
> > I like the idea in this patch, which exports this additional
> > smbios-provided string in sysfs.  This removes the need to parse the
> > SMBIOS table in userspace, which requires root privs.
> > 
> > The concept is also extensible to other methods (e.g. ACPI) that I
> > expect will appear for the platform BIOS to provide naming hints to
> > the OS - I want those to appear in sysfs also.
> > 
> > now, for the patch:
> > 
> > > +		if (!(strcmp(attr_name(bus->dev_attrs[i]),
> "smbiosname")))
> > {
> > > +			if (!smbiosname_string_is_valid(dev, NULL))
> > > +				continue;
> > > +		}
> > >  		error = device_create_file(dev, &bus->dev_attrs[i]);
> > >  		if (error) {
> > >  			while (--i >= 0)
> > 
> > This cannot go in bus.c.  It needs to go in pci-sysfs.c.
> > 

Resending the patch with review comments incorporated.

Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
Signed-off-by: Narendra K <Narendra_K@dell.com>
---
 drivers/firmware/dmi_scan.c |   23 ++++++++++++++++++
 drivers/pci/pci-sysfs.c     |   54 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/dmi.h         |    9 +++++++
 3 files changed, 86 insertions(+), 0 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 31b983d..1d10663 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -278,6 +278,28 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
 	list_add_tail(&dev->list, &dmi_devices);
 }
 
+static void __init dmi_save_devslot(int id, int seg, int bus, int devfn, const char *name)
+{
+	struct dmi_devslot *slot;
+
+	slot = dmi_alloc(sizeof(*slot) + strlen(name) + 1);
+	if (!slot) {
+		printk(KERN_ERR "dmi_save_devslot: out of memory.\n");
+		return;
+	}	
+	slot->id = id;
+	slot->seg = seg;
+	slot->bus = bus;
+	slot->devfn = devfn;
+
+	strcpy((char *)&slot[1], name);
+	slot->dev.type = DMI_DEV_TYPE_DEVSLOT;
+	slot->dev.name = &slot[1];
+	slot->dev.device_data = slot;
+
+	list_add(&slot->dev.list, &dmi_devices);
+}
+
 static void __init dmi_save_extended_devices(const struct dmi_header *dm)
 {
 	const u8 *d = (u8*) dm + 5;
@@ -286,6 +308,7 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
 	if ((*d & 0x80) = 0)
 		return;
 
+	dmi_save_devslot(-1, *(u16 *)(d+2), *(d+4), *(d+5), dmi_string_nosave(dm, *(d-1)));
 	dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
 }
 
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 807224e..85d5d79 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -142,6 +142,54 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
 		       (u8)(pci_dev->class));
 }
 
+#ifdef CONFIG_DMI
+#include <linux/dmi.h>
+static ssize_t
+smbiosname_string_is_valid(struct device *dev, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+  	struct dmi_device *dmi;
+  	struct dmi_devslot *dslot;
+  	int bus;
+  	int devfn;
+
+  	bus = pdev->bus->number;
+  	devfn = pdev->devfn;
+
+  	dmi = NULL;
+  	while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEVSLOT, NULL, dmi)) != NULL) {
+    		dslot = dmi->device_data;
+    		if (dslot && dslot->bus = bus && dslot->devfn = devfn) {
+			if (buf)
+      				return scnprintf(buf, PAGE_SIZE, "%s\n", dmi->name);
+			return strlen(dmi->name);
+		}
+	}
+}
+
+static ssize_t
+smbiosname_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return smbiosname_string_is_valid(dev, buf);
+
+}
+
+struct smbios_attribute {
+	struct attribute attr;
+	ssize_t(*show) (struct device * edev, char *buf);
+	int (*test) (struct device * edev, char *buf);
+};
+
+#define SMBIOSNAME_DEVICE_ATTR(_name, _mode, _show, _test) \
+struct smbios_attribute smbios_attr_##_name = { 	\
+	.attr = {.name = __stringify(_name), .mode = _mode },	\
+	.show	= _show,				\
+	.test	= _test,				\
+};
+
+static SMBIOSNAME_DEVICE_ATTR(smbiosname, 0444, smbiosname_show, smbiosname_string_is_valid);
+#endif
+
 static ssize_t is_enabled_store(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)
@@ -1140,6 +1188,7 @@ static int __init pci_sysfs_init(void)
 {
 	struct pci_dev *pdev = NULL;
 	int retval;
+	struct smbios_attribute *attr = &smbios_attr_smbiosname;
 
 	sysfs_initialized = 1;
 	for_each_pci_dev(pdev) {
@@ -1148,6 +1197,11 @@ static int __init pci_sysfs_init(void)
 			pci_dev_put(pdev);
 			return retval;
 		}
+#ifdef CONFIG_DMI
+		/* if the device does not have an associated smbios string in the smbios table, do not create this attribute */ 
+		if (attr->test && attr->test(&pdev->dev, NULL))
+			sysfs_create_file(&pdev->dev.kobj, &attr->attr);
+#endif
 	}
 
 	return 0;
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index a8a3e1a..cc57c3a 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -20,6 +20,7 @@ enum dmi_device_type {
 	DMI_DEV_TYPE_SAS,
 	DMI_DEV_TYPE_IPMI = -1,
 	DMI_DEV_TYPE_OEM_STRING = -2,
+	DMI_DEV_TYPE_DEVSLOT = -3,
 };
 
 struct dmi_header {
@@ -37,6 +38,14 @@ struct dmi_device {
 
 #ifdef CONFIG_DMI
 
+struct dmi_devslot {
+	struct dmi_device dev;
+	int id;
+	int seg;
+	int bus;
+	int devfn;
+};
+
 extern int dmi_check_system(const struct dmi_system_id *list);
 const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
 extern const char * dmi_get_system_info(int field);
-- 
1.6.5.2

With regards,
Narendra K

Re: [PATCH] Export smbios strings associated with onboard devices to sysfs

[Greg KH]

On Tue, Mar 02, 2010 at 11:33:04AM -0600, Narendra K wrote:
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 807224e..85d5d79 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -142,6 +142,54 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
>  		       (u8)(pci_dev->class));
>  }
>  
> +#ifdef CONFIG_DMI
> +#include <linux/dmi.h>

#includes go at the top of the file.

Can you just create a pci-dmi.c and put this in that .c that way you
don't need any #ifdefs in a .c file?

> +static ssize_t
> +smbiosname_string_is_valid(struct device *dev, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +  	struct dmi_device *dmi;
> +  	struct dmi_devslot *dslot;
> +  	int bus;
> +  	int devfn;
> +
> +  	bus = pdev->bus->number;
> +  	devfn = pdev->devfn;
> +
> +  	dmi = NULL;
> +  	while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEVSLOT, NULL, dmi)) != NULL) {
> +    		dslot = dmi->device_data;
> +    		if (dslot && dslot->bus == bus && dslot->devfn == devfn) {
> +			if (buf)
> +      				return scnprintf(buf, PAGE_SIZE, "%s\n", dmi->name);
> +			return strlen(dmi->name);
> +		}
> +	}
> +}
> +
> +static ssize_t
> +smbiosname_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return smbiosname_string_is_valid(dev, buf);
> +
> +}
> +
> +struct smbios_attribute {
> +	struct attribute attr;
> +	ssize_t(*show) (struct device * edev, char *buf);
> +	int (*test) (struct device * edev, char *buf);
> +};

Why are you creating your own attribute type?

> +
> +#define SMBIOSNAME_DEVICE_ATTR(_name, _mode, _show, _test) \
> +struct smbios_attribute smbios_attr_##_name = { 	\
> +	.attr = {.name = __stringify(_name), .mode = _mode },	\
> +	.show	= _show,				\
> +	.test	= _test,				\
> +};
> +
> +static SMBIOSNAME_DEVICE_ATTR(smbiosname, 0444, smbiosname_show, smbiosname_string_is_valid);

Why are you creating a #define that is only used once?

What is wrong with a "normal" device attribute that you can not use it
here?

> +#endif
> +
>  static ssize_t is_enabled_store(struct device *dev,
>  				struct device_attribute *attr, const char *buf,
>  				size_t count)
> @@ -1140,6 +1188,7 @@ static int __init pci_sysfs_init(void)
>  {
>  	struct pci_dev *pdev = NULL;
>  	int retval;
> +	struct smbios_attribute *attr = &smbios_attr_smbiosname;
>  
>  	sysfs_initialized = 1;
>  	for_each_pci_dev(pdev) {
> @@ -1148,6 +1197,11 @@ static int __init pci_sysfs_init(void)
>  			pci_dev_put(pdev);
>  			return retval;
>  		}
> +#ifdef CONFIG_DMI
> +		/* if the device does not have an associated smbios string in the smbios table, do not create this attribute */ 
> +		if (attr->test && attr->test(&pdev->dev, NULL))
> +			sysfs_create_file(&pdev->dev.kobj, &attr->attr);
> +#endif

Put this in a new function and call it in a different file.

And how are you handling pci devices that are added after the pci
subsystem is initialized (think pci hotplug).

And where are you removing this sysfs file?

Why not put this in the pci_create_sysfs_dev_files function?

thanks,

greg k-h

Re: [PATCH] Export smbios strings associated with onboard devices

[Greg KH]

On Tue, Mar 02, 2010 at 11:33:04AM -0600, Narendra K wrote:
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 807224e..85d5d79 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -142,6 +142,54 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
>  		       (u8)(pci_dev->class));
>  }
>  
> +#ifdef CONFIG_DMI
> +#include <linux/dmi.h>

#includes go at the top of the file.

Can you just create a pci-dmi.c and put this in that .c that way you
don't need any #ifdefs in a .c file?

> +static ssize_t
> +smbiosname_string_is_valid(struct device *dev, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +  	struct dmi_device *dmi;
> +  	struct dmi_devslot *dslot;
> +  	int bus;
> +  	int devfn;
> +
> +  	bus = pdev->bus->number;
> +  	devfn = pdev->devfn;
> +
> +  	dmi = NULL;
> +  	while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEVSLOT, NULL, dmi)) != NULL) {
> +    		dslot = dmi->device_data;
> +    		if (dslot && dslot->bus = bus && dslot->devfn = devfn) {
> +			if (buf)
> +      				return scnprintf(buf, PAGE_SIZE, "%s\n", dmi->name);
> +			return strlen(dmi->name);
> +		}
> +	}
> +}
> +
> +static ssize_t
> +smbiosname_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return smbiosname_string_is_valid(dev, buf);
> +
> +}
> +
> +struct smbios_attribute {
> +	struct attribute attr;
> +	ssize_t(*show) (struct device * edev, char *buf);
> +	int (*test) (struct device * edev, char *buf);
> +};

Why are you creating your own attribute type?

> +
> +#define SMBIOSNAME_DEVICE_ATTR(_name, _mode, _show, _test) \
> +struct smbios_attribute smbios_attr_##_name = { 	\
> +	.attr = {.name = __stringify(_name), .mode = _mode },	\
> +	.show	= _show,				\
> +	.test	= _test,				\
> +};
> +
> +static SMBIOSNAME_DEVICE_ATTR(smbiosname, 0444, smbiosname_show, smbiosname_string_is_valid);

Why are you creating a #define that is only used once?

What is wrong with a "normal" device attribute that you can not use it
here?

> +#endif
> +
>  static ssize_t is_enabled_store(struct device *dev,
>  				struct device_attribute *attr, const char *buf,
>  				size_t count)
> @@ -1140,6 +1188,7 @@ static int __init pci_sysfs_init(void)
>  {
>  	struct pci_dev *pdev = NULL;
>  	int retval;
> +	struct smbios_attribute *attr = &smbios_attr_smbiosname;
>  
>  	sysfs_initialized = 1;
>  	for_each_pci_dev(pdev) {
> @@ -1148,6 +1197,11 @@ static int __init pci_sysfs_init(void)
>  			pci_dev_put(pdev);
>  			return retval;
>  		}
> +#ifdef CONFIG_DMI
> +		/* if the device does not have an associated smbios string in the smbios table, do not create this attribute */ 
> +		if (attr->test && attr->test(&pdev->dev, NULL))
> +			sysfs_create_file(&pdev->dev.kobj, &attr->attr);
> +#endif

Put this in a new function and call it in a different file.

And how are you handling pci devices that are added after the pci
subsystem is initialized (think pci hotplug).

And where are you removing this sysfs file?

Why not put this in the pci_create_sysfs_dev_files function?

thanks,

greg k-h

Re: [PATCH] Export smbios strings associated with onboard devices to sysfs

[Alex Chiang]

* Narendra K <Narendra_K@dell.com>:
> 
> Resending the patch with review comments incorporated.
> 
> Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
> Signed-off-by: Narendra K <Narendra_K@dell.com>
> ---
>  drivers/firmware/dmi_scan.c |   23 ++++++++++++++++++
>  drivers/pci/pci-sysfs.c     |   54 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/dmi.h         |    9 +++++++
>  3 files changed, 86 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 31b983d..1d10663 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -278,6 +278,28 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
>  	list_add_tail(&dev->list, &dmi_devices);
>  }
>  
> +static void __init dmi_save_devslot(int id, int seg, int bus, int devfn, const char *name)
> +{
> +	struct dmi_devslot *slot;
> +
> +	slot = dmi_alloc(sizeof(*slot) + strlen(name) + 1);
> +	if (!slot) {
> +		printk(KERN_ERR "dmi_save_devslot: out of memory.\n");
> +		return;
> +	}	
> +	slot->id = id;
> +	slot->seg = seg;
> +	slot->bus = bus;
> +	slot->devfn = devfn;
> +
> +	strcpy((char *)&slot[1], name);
> +	slot->dev.type = DMI_DEV_TYPE_DEVSLOT;
> +	slot->dev.name = &slot[1];

This needs a cast, else you'll get a warning:

	slot->dev.name = (char *)&slot[1];

Also, when I was playing with your patchset, I noticed that you
should probably add this hunk too:

@@ -334,6 +359,7 @@ static void __init dmi_decode(const struct dmi_header *dm, v
                break;
        case 41:        /* Onboard Devices Extended Information */
                dmi_save_extended_devices(dm);
+               break;
        }
 }
 

> +#ifdef CONFIG_DMI
> +#include <linux/dmi.h>
> +static ssize_t
> +smbiosname_string_is_valid(struct device *dev, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +  	struct dmi_device *dmi;

This needs to be a const struct dmi_device, else you get a
warning.

> +  	struct dmi_devslot *dslot;
> +  	int bus;
> +  	int devfn;
> +
> +  	bus = pdev->bus->number;
> +  	devfn = pdev->devfn;
> +
> +  	dmi = NULL;
> +  	while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEVSLOT, NULL, dmi)) != NULL) {
> +    		dslot = dmi->device_data;
> +    		if (dslot && dslot->bus == bus && dslot->devfn == devfn) {
> +			if (buf)
> +      				return scnprintf(buf, PAGE_SIZE, "%s\n", dmi->name);

Whitespace seems messed up here?

> +			return strlen(dmi->name);
> +		}
> +	}
> +}
> +
> +static ssize_t
> +smbiosname_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return smbiosname_string_is_valid(dev, buf);
> +
> +}
> +
> +struct smbios_attribute {
> +	struct attribute attr;
> +	ssize_t(*show) (struct device * edev, char *buf);
> +	int (*test) (struct device * edev, char *buf);
> +};
> +
> +#define SMBIOSNAME_DEVICE_ATTR(_name, _mode, _show, _test) \
> +struct smbios_attribute smbios_attr_##_name = { 	\
> +	.attr = {.name = __stringify(_name), .mode = _mode },	\
> +	.show	= _show,				\
> +	.test	= _test,				\
> +};
> +
> +static SMBIOSNAME_DEVICE_ATTR(smbiosname, 0444, smbiosname_show, smbiosname_string_is_valid);
> +#endif
> +

After a discussion on irc, some folks thought that changing the
name of this attribute to 'label' would be a much better name
than smbiosname.

/ac

Re: [PATCH] Export smbios strings associated with onboard devices

[Alex Chiang]

* Narendra K <Narendra_K@dell.com>:
> 
> Resending the patch with review comments incorporated.
> 
> Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
> Signed-off-by: Narendra K <Narendra_K@dell.com>
> ---
>  drivers/firmware/dmi_scan.c |   23 ++++++++++++++++++
>  drivers/pci/pci-sysfs.c     |   54 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/dmi.h         |    9 +++++++
>  3 files changed, 86 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 31b983d..1d10663 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -278,6 +278,28 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
>  	list_add_tail(&dev->list, &dmi_devices);
>  }
>  
> +static void __init dmi_save_devslot(int id, int seg, int bus, int devfn, const char *name)
> +{
> +	struct dmi_devslot *slot;
> +
> +	slot = dmi_alloc(sizeof(*slot) + strlen(name) + 1);
> +	if (!slot) {
> +		printk(KERN_ERR "dmi_save_devslot: out of memory.\n");
> +		return;
> +	}	
> +	slot->id = id;
> +	slot->seg = seg;
> +	slot->bus = bus;
> +	slot->devfn = devfn;
> +
> +	strcpy((char *)&slot[1], name);
> +	slot->dev.type = DMI_DEV_TYPE_DEVSLOT;
> +	slot->dev.name = &slot[1];

This needs a cast, else you'll get a warning:

	slot->dev.name = (char *)&slot[1];

Also, when I was playing with your patchset, I noticed that you
should probably add this hunk too:

@@ -334,6 +359,7 @@ static void __init dmi_decode(const struct dmi_header *dm, v
                break;
        case 41:        /* Onboard Devices Extended Information */
                dmi_save_extended_devices(dm);
+               break;
        }
 }
 

> +#ifdef CONFIG_DMI
> +#include <linux/dmi.h>
> +static ssize_t
> +smbiosname_string_is_valid(struct device *dev, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +  	struct dmi_device *dmi;

This needs to be a const struct dmi_device, else you get a
warning.

> +  	struct dmi_devslot *dslot;
> +  	int bus;
> +  	int devfn;
> +
> +  	bus = pdev->bus->number;
> +  	devfn = pdev->devfn;
> +
> +  	dmi = NULL;
> +  	while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEVSLOT, NULL, dmi)) != NULL) {
> +    		dslot = dmi->device_data;
> +    		if (dslot && dslot->bus = bus && dslot->devfn = devfn) {
> +			if (buf)
> +      				return scnprintf(buf, PAGE_SIZE, "%s\n", dmi->name);

Whitespace seems messed up here?

> +			return strlen(dmi->name);
> +		}
> +	}
> +}
> +
> +static ssize_t
> +smbiosname_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return smbiosname_string_is_valid(dev, buf);
> +
> +}
> +
> +struct smbios_attribute {
> +	struct attribute attr;
> +	ssize_t(*show) (struct device * edev, char *buf);
> +	int (*test) (struct device * edev, char *buf);
> +};
> +
> +#define SMBIOSNAME_DEVICE_ATTR(_name, _mode, _show, _test) \
> +struct smbios_attribute smbios_attr_##_name = { 	\
> +	.attr = {.name = __stringify(_name), .mode = _mode },	\
> +	.show	= _show,				\
> +	.test	= _test,				\
> +};
> +
> +static SMBIOSNAME_DEVICE_ATTR(smbiosname, 0444, smbiosname_show, smbiosname_string_is_valid);
> +#endif
> +

After a discussion on irc, some folks thought that changing the
name of this attribute to 'label' would be a much better name
than smbiosname.

/ac

Re: [PATCH] Export smbios strings associated with onboard devices to sysfs

[Alex Chiang]

* Narendra K <Narendra_K@dell.com>:
> 
> Resending the patch with review comments incorporated.
> 
> Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
> Signed-off-by: Narendra K <Narendra_K@dell.com>

Here is a patch I wrote that would be nice if you could
incorporate into your patch series.

It adds support for HP OEM SMBIOS record Type 209, so that you
can populate the 'label' attribute on existing HP platforms.

It needs that hunk I suggested in my previous mail to apply
cleanly.

Thanks,
/ac

From: Alex Chiang <achiang@hp.com>

dmi: save OEM defined slot information

Some legacy platforms provide onboard device information in an SMBIOS
OEM-defined field, notably HP Proliants.

This information can be used to provide information to userspace that
allows correlation between a Linux PCI device and a chassis label.

Save this information so that it can be exposed to userspace. We choose
the string "Embedded NIC %d" since there are known platforms from other
vendors (Dell) that provide a string in this format for their onboard
NICs (although theirs is provided by a Type 41 record). This consistency
will help simplify life for userspace tools.

Only support HP platforms for now. If we need support for another vendor
in the future, we can write a fancier implementation then.

This code was inspired by the implementation in the userspace dmidecode
tool, which was originally written by John Cagle.

Signed-off-by: Alex Chiang <achiang@hp.com>
---
 dmi_scan.c |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
---
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 5a81a8b..cb2a57a 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -314,6 +314,33 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
 	dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
 }
 
+static void __init dmi_save_oem_devices(const struct dmi_header *dm)
+{
+	int bus, devfn, count;
+	const u8 *d = (u8 *)dm + 4;
+	char name[20];
+
+	/* Only handle HP extensions for now */
+	if (strcmp(dmi_ident[DMI_BIOS_VENDOR], "HP"))
+		return;
+
+	count = 1;
+	while ((d + 8) <= ((u8 *)dm + dm->length)) {
+		if ((*d == 0x00 && *(d + 1) == 0x00) ||
+		    (*d == 0xff && *(d + 1) == 0xff))
+			goto next;
+
+		bus = *(d + 1);
+		devfn = *d;
+		sprintf(name, "Embedded NIC %d", count);
+		dmi_save_devslot(-1, 0, bus, devfn, name);
+
+next:
+		count++;
+		d += 8;
+	}
+}
+
 /*
  *	Process a DMI table entry. Right now all we care about are the BIOS
  *	and machine entries. For 2.5 we should pull the smbus controller info
@@ -360,6 +387,9 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
 	case 41:	/* Onboard Devices Extended Information */
 		dmi_save_extended_devices(dm);
 		break;
+	case 209:
+		dmi_save_oem_devices(dm);
+		break;
 	}
 }
 

Re: [PATCH] Export smbios strings associated with onboard devices

[Alex Chiang]

* Narendra K <Narendra_K@dell.com>:
> 
> Resending the patch with review comments incorporated.
> 
> Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
> Signed-off-by: Narendra K <Narendra_K@dell.com>

Here is a patch I wrote that would be nice if you could
incorporate into your patch series.

It adds support for HP OEM SMBIOS record Type 209, so that you
can populate the 'label' attribute on existing HP platforms.

It needs that hunk I suggested in my previous mail to apply
cleanly.

Thanks,
/ac

From: Alex Chiang <achiang@hp.com>

dmi: save OEM defined slot information

Some legacy platforms provide onboard device information in an SMBIOS
OEM-defined field, notably HP Proliants.

This information can be used to provide information to userspace that
allows correlation between a Linux PCI device and a chassis label.

Save this information so that it can be exposed to userspace. We choose
the string "Embedded NIC %d" since there are known platforms from other
vendors (Dell) that provide a string in this format for their onboard
NICs (although theirs is provided by a Type 41 record). This consistency
will help simplify life for userspace tools.

Only support HP platforms for now. If we need support for another vendor
in the future, we can write a fancier implementation then.

This code was inspired by the implementation in the userspace dmidecode
tool, which was originally written by John Cagle.

Signed-off-by: Alex Chiang <achiang@hp.com>
---
 dmi_scan.c |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
---
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 5a81a8b..cb2a57a 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -314,6 +314,33 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
 	dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
 }
 
+static void __init dmi_save_oem_devices(const struct dmi_header *dm)
+{
+	int bus, devfn, count;
+	const u8 *d = (u8 *)dm + 4;
+	char name[20];
+
+	/* Only handle HP extensions for now */
+	if (strcmp(dmi_ident[DMI_BIOS_VENDOR], "HP"))
+		return;
+
+	count = 1;
+	while ((d + 8) <= ((u8 *)dm + dm->length)) {
+		if ((*d = 0x00 && *(d + 1) = 0x00) ||
+		    (*d = 0xff && *(d + 1) = 0xff))
+			goto next;
+
+		bus = *(d + 1);
+		devfn = *d;
+		sprintf(name, "Embedded NIC %d", count);
+		dmi_save_devslot(-1, 0, bus, devfn, name);
+
+next:
+		count++;
+		d += 8;
+	}
+}
+
 /*
  *	Process a DMI table entry. Right now all we care about are the BIOS
  *	and machine entries. For 2.5 we should pull the smbus controller info
@@ -360,6 +387,9 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
 	case 41:	/* Onboard Devices Extended Information */
 		dmi_save_extended_devices(dm);
 		break;
+	case 209:
+		dmi_save_oem_devices(dm);
+		break;
 	}
 }
 

RE: [PATCH] Export smbios strings associated with onboard devicesto sysfs

[Narendra_K]

> -----Original Message-----
> From: Alex Chiang [mailto:achiang@hp.com]
> Sent: Monday, March 08, 2010 11:09 PM
> To: K, Narendra
> Cc: Domsch, Matt; netdev@vger.kernel.org; linux-
> hotplug@vger.kernel.org; linux-pci@vger.kernel.org; Hargrave, Jordan;
> Shandilya, Sandeep K; Rose, Charles; Iyer, Shyam
> Subject: Re: [PATCH] Export smbios strings associated with onboard
> devicesto sysfs
> 
> * Narendra K <Narendra_K@dell.com>:
> >
> > Resending the patch with review comments incorporated.
> >
> > Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
> > Signed-off-by: Narendra K <Narendra_K@dell.com>
> 
> Here is a patch I wrote that would be nice if you could
> incorporate into your patch series.
> 
> It adds support for HP OEM SMBIOS record Type 209, so that you
> can populate the 'label' attribute on existing HP platforms.
> 
> It needs that hunk I suggested in my previous mail to apply
> cleanly.
> 

Sure, thanks.

With regards,
Narendra K

RE: [PATCH] Export smbios strings associated with onboard devicesto sysfs

[Narendra_K]

> -----Original Message-----
> From: Alex Chiang [mailto:achiang@hp.com]
> Sent: Monday, March 08, 2010 11:09 PM
> To: K, Narendra
> Cc: Domsch, Matt; netdev@vger.kernel.org; linux-
> hotplug@vger.kernel.org; linux-pci@vger.kernel.org; Hargrave, Jordan;
> Shandilya, Sandeep K; Rose, Charles; Iyer, Shyam
> Subject: Re: [PATCH] Export smbios strings associated with onboard
> devicesto sysfs
> 
> * Narendra K <Narendra_K@dell.com>:
> >
> > Resending the patch with review comments incorporated.
> >
> > Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
> > Signed-off-by: Narendra K <Narendra_K@dell.com>
> 
> Here is a patch I wrote that would be nice if you could
> incorporate into your patch series.
> 
> It adds support for HP OEM SMBIOS record Type 209, so that you
> can populate the 'label' attribute on existing HP platforms.
> 
> It needs that hunk I suggested in my previous mail to apply
> cleanly.
> 

Sure, thanks.

With regards,
Narendra K

Re: [PATCH] Export smbios strings associated with onboard devices to sysfs

[Alex Chiang]

* Narendra K <Narendra_K@dell.com>:
> 
> * We have been having discussions in the netdev list about
> creating multiple names for the network interfaces to bring
> determinism into the way network interfaces are named in the
> OSes. In specific, "eth0 in the OS does not always map to the
> integrated NIC Gb1 as labelled on the chassis".  

Yes, I agree that this is a real problem that we do not handle
well today.

> 1.Export smbios strings of onboard devices, to sysfs. For example -
> 
> cat /sys/class/net/eth0/device/smbiosname
> Embedded NIC 2
> 
> cat  /sys/bus/pci/devices/0000\:03\:00.0/smbiosname
> Embedded NIC 2

I agree with this concept, but I don't like the interface.

The name "smbiosname" isn't the proper level of abstraction. We
don't want users to care what firmware standard is providing the
name (think smbios vs acpi vs open firmware...).

We learned this lesson with exposing ACPI interfaces. Let's not
make the same mistake here.

Something like "firmwarename", "fwname", "platformname" etc. is
generic, and then the interface will make sense for platforms
that do not implement SMBIOS.

I don't particularly care which name you choose as long as it's
properly generic.

As far as implementation goes, I've been thinking about this
problem for a while now, and I keep wanting to use the
pci_create_slot() API, but am still a little on the fence about
it.

The pros:
	- all you have to do is write a simple little driver that
	  can read SMBIOS to get PCI bus:devfn and the name, and
	  then you call pci_create_slot(). Then you get all sorts
	  of stuff for free, like sysfs exposure, proper
	  refcounting (important given that the PCI logical
	  hotplug interface (/sys/bus/pci/rescan and friends) can
	  be used to remove onboard devices), etc.

	- see drivers/acpi/pci_slot.c for an example of how to
	  detect slots and then register them.

The cons:
	- the user interface is /sys/bus/pci/slots/<name>

	I don't know if that is an appropriate interface, since
	technically an onboard device isn't in a slot. But maybe
	if you did something like:

		/sys/bus/pci/slots/onboard0
		/sys/bus/pci/slots/onboard1
	
	that might make sense.

	Or...
		/sys/bus/pci/onboard/<name>

I read through the patch, but given that the implementation
strategy might change based on my comments, will hold off and see
how the conversation develops.

Thanks,
/ac

Re: [PATCH] Export smbios strings associated with onboard devices

[Alex Chiang]

* Narendra K <Narendra_K@dell.com>:
> 
> * We have been having discussions in the netdev list about
> creating multiple names for the network interfaces to bring
> determinism into the way network interfaces are named in the
> OSes. In specific, "eth0 in the OS does not always map to the
> integrated NIC Gb1 as labelled on the chassis".  

Yes, I agree that this is a real problem that we do not handle
well today.

> 1.Export smbios strings of onboard devices, to sysfs. For example -
> 
> cat /sys/class/net/eth0/device/smbiosname
> Embedded NIC 2
> 
> cat  /sys/bus/pci/devices/0000\:03\:00.0/smbiosname
> Embedded NIC 2

I agree with this concept, but I don't like the interface.

The name "smbiosname" isn't the proper level of abstraction. We
don't want users to care what firmware standard is providing the
name (think smbios vs acpi vs open firmware...).

We learned this lesson with exposing ACPI interfaces. Let's not
make the same mistake here.

Something like "firmwarename", "fwname", "platformname" etc. is
generic, and then the interface will make sense for platforms
that do not implement SMBIOS.

I don't particularly care which name you choose as long as it's
properly generic.

As far as implementation goes, I've been thinking about this
problem for a while now, and I keep wanting to use the
pci_create_slot() API, but am still a little on the fence about
it.

The pros:
	- all you have to do is write a simple little driver that
	  can read SMBIOS to get PCI bus:devfn and the name, and
	  then you call pci_create_slot(). Then you get all sorts
	  of stuff for free, like sysfs exposure, proper
	  refcounting (important given that the PCI logical
	  hotplug interface (/sys/bus/pci/rescan and friends) can
	  be used to remove onboard devices), etc.

	- see drivers/acpi/pci_slot.c for an example of how to
	  detect slots and then register them.

The cons:
	- the user interface is /sys/bus/pci/slots/<name>

	I don't know if that is an appropriate interface, since
	technically an onboard device isn't in a slot. But maybe
	if you did something like:

		/sys/bus/pci/slots/onboard0
		/sys/bus/pci/slots/onboard1
	
	that might make sense.

	Or...
		/sys/bus/pci/onboard/<name>

I read through the patch, but given that the implementation
strategy might change based on my comments, will hold off and see
how the conversation develops.

Thanks,
/ac

Re: [PATCH] Export smbios strings associated with onboard devices to sysfs

[Domsch, Matt]

On Thu, Feb 25, 2010 at 03:40:20PM -0600, Alex Chiang wrote:
> * Narendra K <Narendra_K@dell.com>:
> > 
> > * We have been having discussions in the netdev list about
> > creating multiple names for the network interfaces to bring
> > determinism into the way network interfaces are named in the
> > OSes. In specific, "eth0 in the OS does not always map to the
> > integrated NIC Gb1 as labelled on the chassis".  
> 
> Yes, I agree that this is a real problem that we do not handle
> well today.
> 
> > 1.Export smbios strings of onboard devices, to sysfs. For example -
> > 
> > cat /sys/class/net/eth0/device/smbiosname
> > Embedded NIC 2
> > 
> > cat  /sys/bus/pci/devices/0000\:03\:00.0/smbiosname
> > Embedded NIC 2
> 
> I agree with this concept, but I don't like the interface.
> 
> The name "smbiosname" isn't the proper level of abstraction. We
> don't want users to care what firmware standard is providing the
> name (think smbios vs acpi vs open firmware...).
> 
> We learned this lesson with exposing ACPI interfaces. Let's not
> make the same mistake here.
> 
> Something like "firmwarename", "fwname", "platformname" etc. is
> generic, and then the interface will make sense for platforms
> that do not implement SMBIOS.
> 
> I don't particularly care which name you choose as long as it's
> properly generic.

I'm not sure I like the generic name.  Then the policy of which
provider (if there could be >1, which there will be once this can be
done via ACPI instead of static SMBIOS) gets to use that file name
becomes kernel-dependent, instead of userspace-dependent.

What is wrong with having both "smbiosname" and "acpiname" (for lack
of better names), either, both, or none, as files in the sysfs tree,
and let userspace set the policy of which one to use if there are >1 ?

Thanks,
Matt

-- 
Matt Domsch
Technology Strategist, Dell Office of the CTO
linux.dell.com & www.dell.com/linux

Re: [PATCH] Export smbios strings associated with onboard devices to

[Domsch, Matt]

On Thu, Feb 25, 2010 at 03:40:20PM -0600, Alex Chiang wrote:
> * Narendra K <Narendra_K@dell.com>:
> > 
> > * We have been having discussions in the netdev list about
> > creating multiple names for the network interfaces to bring
> > determinism into the way network interfaces are named in the
> > OSes. In specific, "eth0 in the OS does not always map to the
> > integrated NIC Gb1 as labelled on the chassis".  
> 
> Yes, I agree that this is a real problem that we do not handle
> well today.
> 
> > 1.Export smbios strings of onboard devices, to sysfs. For example -
> > 
> > cat /sys/class/net/eth0/device/smbiosname
> > Embedded NIC 2
> > 
> > cat  /sys/bus/pci/devices/0000\:03\:00.0/smbiosname
> > Embedded NIC 2
> 
> I agree with this concept, but I don't like the interface.
> 
> The name "smbiosname" isn't the proper level of abstraction. We
> don't want users to care what firmware standard is providing the
> name (think smbios vs acpi vs open firmware...).
> 
> We learned this lesson with exposing ACPI interfaces. Let's not
> make the same mistake here.
> 
> Something like "firmwarename", "fwname", "platformname" etc. is
> generic, and then the interface will make sense for platforms
> that do not implement SMBIOS.
> 
> I don't particularly care which name you choose as long as it's
> properly generic.

I'm not sure I like the generic name.  Then the policy of which
provider (if there could be >1, which there will be once this can be
done via ACPI instead of static SMBIOS) gets to use that file name
becomes kernel-dependent, instead of userspace-dependent.

What is wrong with having both "smbiosname" and "acpiname" (for lack
of better names), either, both, or none, as files in the sysfs tree,
and let userspace set the policy of which one to use if there are >1 ?

Thanks,
Matt

-- 
Matt Domsch
Technology Strategist, Dell Office of the CTO
linux.dell.com & www.dell.com/linux

Re: [PATCH] Export smbios strings associated with onboard devices to sysfs

[Alex Chiang]

* Domsch, Matt <Matt_Domsch@Dell.com>:
> On Thu, Feb 25, 2010 at 03:40:20PM -0600, Alex Chiang wrote:
> > 
> > I agree with this concept, but I don't like the interface.
> > 
> > The name "smbiosname" isn't the proper level of abstraction. We
> > don't want users to care what firmware standard is providing the
> > name (think smbios vs acpi vs open firmware...).
> > 
> > We learned this lesson with exposing ACPI interfaces. Let's not
> > make the same mistake here.
> > 
> > Something like "firmwarename", "fwname", "platformname" etc. is
> > generic, and then the interface will make sense for platforms
> > that do not implement SMBIOS.
> > 
> > I don't particularly care which name you choose as long as it's
> > properly generic.
> 
> I'm not sure I like the generic name.  Then the policy of which
> provider (if there could be >1, which there will be once this can be
> done via ACPI instead of static SMBIOS) gets to use that file name
> becomes kernel-dependent, instead of userspace-dependent.
 
I would imagine that an ACPI _DSM would take precedence over
SMBIOS in that example.

The kernel implements policy like that today already, especially
in the hotplug drivers.

If you modprobe pci_slot, it creates files named with ACPI _SUN.
If you then load pciehp, the names from PCI config space take
precedence.

> What is wrong with having both "smbiosname" and "acpiname" (for lack
> of better names), either, both, or none, as files in the sysfs tree,
> and let userspace set the policy of which one to use if there are >1 ?

sysfs is already confusing enough as it is.

If we present multiple names, every piece of software has to
choose which one to use. Not everyone will simply look at what 
udev creates.

And those will be a maintenance burden forever.

If we have a generic name, then all the non-x86 platforms that do
not have SMBIOS nor ACPI can benefit. In the x86/ia64 world, we
also protect ourselves from new, future firmware standards.

ACPI has been trying to dig itself out of this hole for a long
time, and they're probably stuck with legacy interfaces forever.
I'm hoping not to make the same mistake again.

Thanks,
/ac

Re: [PATCH] Export smbios strings associated with onboard devices

[Alex Chiang]

* Domsch, Matt <Matt_Domsch@Dell.com>:
> On Thu, Feb 25, 2010 at 03:40:20PM -0600, Alex Chiang wrote:
> > 
> > I agree with this concept, but I don't like the interface.
> > 
> > The name "smbiosname" isn't the proper level of abstraction. We
> > don't want users to care what firmware standard is providing the
> > name (think smbios vs acpi vs open firmware...).
> > 
> > We learned this lesson with exposing ACPI interfaces. Let's not
> > make the same mistake here.
> > 
> > Something like "firmwarename", "fwname", "platformname" etc. is
> > generic, and then the interface will make sense for platforms
> > that do not implement SMBIOS.
> > 
> > I don't particularly care which name you choose as long as it's
> > properly generic.
> 
> I'm not sure I like the generic name.  Then the policy of which
> provider (if there could be >1, which there will be once this can be
> done via ACPI instead of static SMBIOS) gets to use that file name
> becomes kernel-dependent, instead of userspace-dependent.
 
I would imagine that an ACPI _DSM would take precedence over
SMBIOS in that example.

The kernel implements policy like that today already, especially
in the hotplug drivers.

If you modprobe pci_slot, it creates files named with ACPI _SUN.
If you then load pciehp, the names from PCI config space take
precedence.

> What is wrong with having both "smbiosname" and "acpiname" (for lack
> of better names), either, both, or none, as files in the sysfs tree,
> and let userspace set the policy of which one to use if there are >1 ?

sysfs is already confusing enough as it is.

If we present multiple names, every piece of software has to
choose which one to use. Not everyone will simply look at what 
udev creates.

And those will be a maintenance burden forever.

If we have a generic name, then all the non-x86 platforms that do
not have SMBIOS nor ACPI can benefit. In the x86/ia64 world, we
also protect ourselves from new, future firmware standards.

ACPI has been trying to dig itself out of this hole for a long
time, and they're probably stuck with legacy interfaces forever.
I'm hoping not to make the same mistake again.

Thanks,
/ac

RE: [PATCH] Export smbios strings associated with onboard devicesto sysfs

[Narendra_K]

> -----Original Message-----
> From: Alex Chiang [mailto:achiang@hp.com]
> Sent: Friday, February 26, 2010 3:10 AM
> To: K, Narendra
> Cc: netdev@vger.kernel.org; linux-hotplug@vger.kernel.org; linux-
> pci@vger.kernel.org; Domsch, Matt; Hargrave, Jordan; Shandilya,
Sandeep
> K; Rose, Charles; Iyer, Shyam
> Subject: Re: [PATCH] Export smbios strings associated with onboard
> devicesto sysfs
> 
> * Narendra K <Narendra_K@dell.com>:
> >
> > * We have been having discussions in the netdev list about
> > creating multiple names for the network interfaces to bring
> > determinism into the way network interfaces are named in the
> > OSes. In specific, "eth0 in the OS does not always map to the
> > integrated NIC Gb1 as labelled on the chassis".
> 
> Yes, I agree that this is a real problem that we do not handle
> well today.
> 
> > 1.Export smbios strings of onboard devices, to sysfs. For example -
> >
> > cat /sys/class/net/eth0/device/smbiosname
> > Embedded NIC 2
> >
> > cat  /sys/bus/pci/devices/0000\:03\:00.0/smbiosname
> > Embedded NIC 2
> 
> I agree with this concept, but I don't like the interface.
> 
> The name "smbiosname" isn't the proper level of abstraction. We
> don't want users to care what firmware standard is providing the
> name (think smbios vs acpi vs open firmware...).
> 
> We learned this lesson with exposing ACPI interfaces. Let's not
> make the same mistake here.
> 
> Something like "firmwarename", "fwname", "platformname" etc. is
> generic, and then the interface will make sense for platforms
> that do not implement SMBIOS.
> 
> I don't particularly care which name you choose as long as it's
> properly generic.
> 
> As far as implementation goes, I've been thinking about this
> problem for a while now, and I keep wanting to use the
> pci_create_slot() API, but am still a little on the fence about
> it.
> 
> The pros:
> 	- all you have to do is write a simple little driver that
> 	  can read SMBIOS to get PCI bus:devfn and the name, and
> 	  then you call pci_create_slot(). Then you get all sorts
> 	  of stuff for free, like sysfs exposure, proper
> 	  refcounting (important given that the PCI logical
> 	  hotplug interface (/sys/bus/pci/rescan and friends) can
> 	  be used to remove onboard devices), etc.
> 
> 	- see drivers/acpi/pci_slot.c for an example of how to
> 	  detect slots and then register them.
> 
> The cons:
> 	- the user interface is /sys/bus/pci/slots/<name>
> 
> 	I don't know if that is an appropriate interface, since
> 	technically an onboard device isn't in a slot. But maybe
> 	if you did something like:
> 
> 		/sys/bus/pci/slots/onboard0
> 		/sys/bus/pci/slots/onboard1
> 
> 	that might make sense.
> 
> 	Or...
> 		/sys/bus/pci/onboard/<name>
> 
> I read through the patch, but given that the implementation
> strategy might change based on my comments, will hold off and see
> how the conversation develops.

Thanks. I will wait too, to see how the discussion develops on this
method of implementation.

With regards,
Narendra K

RE: [PATCH] Export smbios strings associated with onboard devicesto sysfs

[Narendra_K]

> -----Original Message-----
> From: Alex Chiang [mailto:achiang@hp.com]
> Sent: Friday, February 26, 2010 3:10 AM
> To: K, Narendra
> Cc: netdev@vger.kernel.org; linux-hotplug@vger.kernel.org; linux-
> pci@vger.kernel.org; Domsch, Matt; Hargrave, Jordan; Shandilya,
Sandeep
> K; Rose, Charles; Iyer, Shyam
> Subject: Re: [PATCH] Export smbios strings associated with onboard
> devicesto sysfs
> 
> * Narendra K <Narendra_K@dell.com>:
> >
> > * We have been having discussions in the netdev list about
> > creating multiple names for the network interfaces to bring
> > determinism into the way network interfaces are named in the
> > OSes. In specific, "eth0 in the OS does not always map to the
> > integrated NIC Gb1 as labelled on the chassis".
> 
> Yes, I agree that this is a real problem that we do not handle
> well today.
> 
> > 1.Export smbios strings of onboard devices, to sysfs. For example -
> >
> > cat /sys/class/net/eth0/device/smbiosname
> > Embedded NIC 2
> >
> > cat  /sys/bus/pci/devices/0000\:03\:00.0/smbiosname
> > Embedded NIC 2
> 
> I agree with this concept, but I don't like the interface.
> 
> The name "smbiosname" isn't the proper level of abstraction. We
> don't want users to care what firmware standard is providing the
> name (think smbios vs acpi vs open firmware...).
> 
> We learned this lesson with exposing ACPI interfaces. Let's not
> make the same mistake here.
> 
> Something like "firmwarename", "fwname", "platformname" etc. is
> generic, and then the interface will make sense for platforms
> that do not implement SMBIOS.
> 
> I don't particularly care which name you choose as long as it's
> properly generic.
> 
> As far as implementation goes, I've been thinking about this
> problem for a while now, and I keep wanting to use the
> pci_create_slot() API, but am still a little on the fence about
> it.
> 
> The pros:
> 	- all you have to do is write a simple little driver that
> 	  can read SMBIOS to get PCI bus:devfn and the name, and
> 	  then you call pci_create_slot(). Then you get all sorts
> 	  of stuff for free, like sysfs exposure, proper
> 	  refcounting (important given that the PCI logical
> 	  hotplug interface (/sys/bus/pci/rescan and friends) can
> 	  be used to remove onboard devices), etc.
> 
> 	- see drivers/acpi/pci_slot.c for an example of how to
> 	  detect slots and then register them.
> 
> The cons:
> 	- the user interface is /sys/bus/pci/slots/<name>
> 
> 	I don't know if that is an appropriate interface, since
> 	technically an onboard device isn't in a slot. But maybe
> 	if you did something like:
> 
> 		/sys/bus/pci/slots/onboard0
> 		/sys/bus/pci/slots/onboard1
> 
> 	that might make sense.
> 
> 	Or...
> 		/sys/bus/pci/onboard/<name>
> 
> I read through the patch, but given that the implementation
> strategy might change based on my comments, will hold off and see
> how the conversation develops.

Thanks. I will wait too, to see how the discussion develops on this
method of implementation.

With regards,
Narendra K

Re: [PATCH] Export smbios strings associated with onboard devices to sysfs

[Greg KH]

On Thu, Feb 25, 2010 at 02:29:42PM -0600, Narendra K wrote:
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -419,6 +419,11 @@ static int device_add_attrs(struct bus_type *bus, struct device *dev)
>  		return 0;
>  
>  	for (i = 0; attr_name(bus->dev_attrs[i]); i++) {
> +		/* if the device does not have an associated smbios string in the smbios table, do not create this attribute */ 
> +		if (!(strcmp(attr_name(bus->dev_attrs[i]), "smbiosname"))) {
> +			if (!smbiosname_string_is_valid(dev, NULL)) 
> +				continue;
> +		}

Um, no, you can not modify the driver core for stuff like this.  Do it
in your driver or class specific code, as that is where it is supposed
to be.

good luck,

greg k-h

Re: [PATCH] Export smbios strings associated with onboard devices

[Greg KH]

On Thu, Feb 25, 2010 at 02:29:42PM -0600, Narendra K wrote:
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -419,6 +419,11 @@ static int device_add_attrs(struct bus_type *bus, struct device *dev)
>  		return 0;
>  
>  	for (i = 0; attr_name(bus->dev_attrs[i]); i++) {
> +		/* if the device does not have an associated smbios string in the smbios table, do not create this attribute */ 
> +		if (!(strcmp(attr_name(bus->dev_attrs[i]), "smbiosname"))) {
> +			if (!smbiosname_string_is_valid(dev, NULL)) 
> +				continue;
> +		}

Um, no, you can not modify the driver core for stuff like this.  Do it
in your driver or class specific code, as that is where it is supposed
to be.

good luck,

greg k-h