| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 28 May 2010 08:40:41 -0700
From: Greg KH <greg@...ah.com>
To: "K, Narendra" <Narendra_K@...l.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-hotplug@...r.kernel.org" <linux-hotplug@...r.kernel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"Domsch, Matt" <Matt_Domsch@...l.com>,
"Hargrave, Jordan" <Jordan_Hargrave@...l.com>,
"Rose, Charles" <Charles_Rose@...l.com>,
"Nijhawan, Vijay" <Vijay_Nijhawan@...l.com>
Subject: Re: [PATCH 1/2] Export firmware assigned labels of network devices
to sysfs
On Fri, May 28, 2010 at 06:55:21AM -0500, K, Narendra wrote:
> Please refer to the PCI-SIG Draft ECN
> "PCIe Device Labeling under Operating Systems Draft ECN" at this link -
> http://www.pcisig.com/specifications/pciexpress/review_zone/.
>
> It would be great to know your views on this ECN. Please let us know if you have
> have any suggestions or changes.
Note that only members of the PCI-SIG can do this, which pretty much
rules out any "normal" Linux kernel developer :(
Care to post a public version of this for us to review?
> --- /dev/null
> +++ b/drivers/pci/pci-label.c
> @@ -0,0 +1,242 @@
> +/*
> + * File: drivers/pci/pci-label.c
This line is not needed, we know the file name :)
> + * Purpose: Export the firmware label associated with a pci network interface
> + * device to sysfs
> + * Copyright (C) 2010 Dell Inc.
> + * by Narendra K <Narendra_K@...l.com>, Jordan Hargrave <Jordan_Hargrave@...l.com>
> + *
> + * This code checks if the pci network device has a related ACPI _DSM. If
> + * available, the code calls the _DSM to retrieve the index and string and
> + * exports them to sysfs. If the ACPI _DSM is not available, it falls back on
> + * SMBIOS. SMBIOS defines type 41 for onboard pci devices. This code retrieves
> + * strings associated with the type 41 and exports it to sysfs.
> + *
> + * Please see http://linux.dell.com/wiki/index.php/Oss/libnetdevname for more
> + * information.
> + */
> +
> +#include <linux/pci-label.h>
Why is this file in include/linux/ ? Who needs it there? Can't it just
be in in the drivers/pci/ directory? Actually all you need is 2
functions in there, so it could go into the internal pci.h file in that
directory without a problem, right?
> +
> +static ssize_t
> +smbiosname_string_exists(struct device *dev, char *buf)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + const 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);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static ssize_t
> +smbiosname_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + return smbiosname_string_exists(dev, buf);
> +}
> +
> +struct smbios_attribute smbios_attr_label = {
> + .attr = {.name = __stringify(label), .mode = 0444, .owner = THIS_MODULE},
Can't you just put "label" as the name?
> + .show = smbiosname_show,
> + .test = smbiosname_string_exists,
> +};
> +
> +static int
> +pci_create_smbiosname_file(struct pci_dev *pdev)
> +{
> + if (smbios_attr_label.test && smbios_attr_label.test(&pdev->dev, NULL)) {
> + sysfs_create_file(&pdev->dev.kobj, &smbios_attr_label.attr);
> + return 0;
> + }
> + return -1;
> +}
> +
> +static int
> +pci_remove_smbiosname_file(struct pci_dev *pdev)
> +{
> + if (smbios_attr_label.test && smbios_attr_label.test(&pdev->dev, NULL)) {
> + sysfs_remove_file(&pdev->dev.kobj, &smbios_attr_label.attr);
> + return 0;
> + }
> + return -1;
> +}
> +
> +static const char dell_dsm_uuid[] = {
Um, a dell specific uuid in a generic file? What happens when we need
to support another manufacturer?
> + 0xD0, 0x37, 0xC9, 0xE5, 0x53, 0x35, 0x7A, 0x4D,
> + 0x91, 0x17, 0xEA, 0x4D, 0x19, 0xC3, 0x43, 0x4D
> +};
> +
> +
> +static int
> +dsm_get_label(acpi_handle handle, int func,
> + struct acpi_buffer *output,
> + char *buf, char *attribute)
> +{
> + struct acpi_object_list input;
> + union acpi_object params[4];
> + union acpi_object *obj;
> + int len = 0;
> +
> + int err;
> +
> + input.count = 4;
> + input.pointer = params;
> + params[0].type = ACPI_TYPE_BUFFER;
> + params[0].buffer.length = sizeof(dell_dsm_uuid);
> + params[0].buffer.pointer = (char *)dell_dsm_uuid;
> + params[1].type = ACPI_TYPE_INTEGER;
> + params[1].integer.value = 0x02;
> + params[2].type = ACPI_TYPE_INTEGER;
> + params[2].integer.value = func;
> + params[3].type = ACPI_TYPE_PACKAGE;
> + params[3].package.count = 0;
> + params[3].package.elements = NULL;
> +
> + err = acpi_evaluate_object(handle, "_DSM", &input, output);
> + if (err) {
> + printk(KERN_INFO "failed to evaulate _DSM\n");
> + return -1;
> + }
> +
> + obj = (union acpi_object *)output->pointer;
> +
> + switch (obj->type) {
> + case ACPI_TYPE_PACKAGE:
> + if (obj->package.count == 2) {
> + len = obj->package.elements[0].integer.value;
> + if (buf) {
> + if (!strncmp(attribute, "index", strlen(attribute)))
> + scnprintf(buf, PAGE_SIZE, "%lu\n",
> + obj->package.elements[0].integer.value);
> + else
> + scnprintf(buf, PAGE_SIZE, "%s\n",
> + obj->package.elements[1].string.pointer);
> + kfree(output->pointer);
> + return strlen(buf);
> + }
> + }
> + kfree(output->pointer);
> + return len;
> + break;
> + default:
> + return -1;
> + }
> +}
> +
> +static ssize_t
> +acpi_index_string_exist(struct device *dev, char *buf, char *attribute)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> +
> + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> + acpi_handle handle;
> + int length;
> + int is_addin_card = 0;
> +
> + if ((pdev->class >> 16) != PCI_BASE_CLASS_NETWORK)
> + return -1;
> +
> + handle = DEVICE_ACPI_HANDLE(dev);
> +
> + if (!handle) {
> + /*
> + * The device is an add-in network controller and does have
> + * a valid handle. Try until we get the handle for the parent
> + * bridge
> + */
> + struct pci_bus *pbus;
> + for (pbus = pdev->bus; pbus; pbus = pbus->parent) {
> + handle = DEVICE_ACPI_HANDLE(&(pbus->self->dev));
> + if (handle)
> + break;
> +
> + }
> + }
> +
> + if ((length = dsm_get_label(handle, DELL_DSM_NETWORK,
> + &output, buf, attribute)) < 0)
> + return -1;
> +
> + return length;
> +}
> +
> +static ssize_t
> +acpilabel_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + return acpi_index_string_exist(dev, buf, "label");
> +}
> +
> +static ssize_t
> +acpiindex_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + return acpi_index_string_exist(dev, buf, "index");
> +}
> +
> +struct acpi_attribute acpi_attr_label = {
> + .attr = {.name = __stringify(label), .mode = 0444, .owner = THIS_MODULE},
> + .show = acpilabel_show,
> + .test = acpi_index_string_exist,
> +};
> +
> +struct acpi_attribute acpi_attr_index = {
> + .attr = {.name = __stringify(index), .mode = 0444, .owner = THIS_MODULE},
> + .show = acpiindex_show,
> + .test = acpi_index_string_exist,
> +};
> +
> +static int
> +pci_create_acpi_index_label_files(struct pci_dev *pdev)
> +{
> + if (acpi_attr_label.test && acpi_attr_label.test(&pdev->dev, NULL) > 0) {
> + sysfs_create_file(&pdev->dev.kobj, &acpi_attr_label.attr);
> + sysfs_create_file(&pdev->dev.kobj, &acpi_attr_index.attr);
> + return 0;
> + }
> + return -1;
> +}
> +
> +static int
> +pci_remove_acpi_index_label_files(struct pci_dev *pdev)
> +{
> + if (acpi_attr_label.test && acpi_attr_label.test(&pdev->dev, NULL) > 0) {
> + sysfs_remove_file(&pdev->dev.kobj, &acpi_attr_label.attr);
> + sysfs_remove_file(&pdev->dev.kobj, &acpi_attr_index.attr);
> + return 0;
> + }
> + return -1;
> +}
> +
> +int pci_create_acpi_attr_files(struct pci_dev *pdev)
> +{
> + if (!pci_create_acpi_index_label_files(pdev))
> + return 0;
> + if (!pci_create_smbiosname_file(pdev))
> + return 0;
> + return -ENODEV;
> +}
> +EXPORT_SYMBOL(pci_create_acpi_attr_files);
EXPORT_SYMBOL_GPL?
Wait, why does this need to be exported at all? What module is ever
going to call this function?
> +int pci_remove_acpi_attr_files(struct pci_dev *pdev)
> +{
> + if (!pci_remove_acpi_index_label_files(pdev))
> + return 0;
> + if (!pci_remove_smbiosname_file(pdev))
> + return 0;
> + return -ENODEV;
> +
> +}
> +EXPORT_SYMBOL(pci_remove_acpi_attr_files);
Same here, what module will call this?
> +++ b/include/linux/pci-label.h
As discussed above, this whole file does not need to exist.
> +extern int pci_create_acpi_attr_files(struct pci_dev *pdev);
> +extern int pci_remove_acpi_attr_files(struct pci_dev *pdev);
Just put these two functions in the drivers/pci/pci.h file.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists