| Message ID | 20100225202941.GA19404@mock.linuxdev.us.dell.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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
* 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
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
* 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
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
> -----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
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);