Patchwork Export smbios strings associated with onboard devices to sysfs

login
register
mail settings
Submitter Narendra K
Date Feb. 25, 2010, 8:29 p.m.
Message ID <20100225202941.GA19404@mock.linuxdev.us.dell.com>
Download mbox | patch
Permalink /patch/46275/
State RFC
Delegated to: David Miller
Headers show

Comments

Narendra K - Feb. 25, 2010, 8:29 p.m.
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(-)
Matt Domsch - Feb. 25, 2010, 8:49 p.m.
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
Alex Chiang - Feb. 25, 2010, 9:40 p.m.
* 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matt Domsch - Feb. 25, 2010, 9:46 p.m.
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
Alex Chiang - Feb. 25, 2010, 10:20 p.m.
* 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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH - Feb. 25, 2010, 10:55 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Narendra K - March 2, 2010, 5:42 p.m.
> -----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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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)
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);