From: Greg KH <greg@kroah•com>
To: Matt Domsch <Matt_Domsch@Dell•com>
Cc: Stephen Hemminger <shemminger@vyatta•com>,
Narendra_K@Dell•com, netdev@vger•kernel.org,
Charles_Rose@Dell•com, Jordan_Hargrave@Dell•com,
linux-pci@vger•kernel.org
Subject: Re: [PATCH] Add firmware label support to iproute2
Date: Wed, 25 Aug 2010 15:16:28 -0700 [thread overview]
Message-ID: <20100825221628.GA24502@kroah.com> (raw)
In-Reply-To: <20100825220323.GA12671@auslistsprd01.us.dell.com>
On Wed, Aug 25, 2010 at 05:03:23PM -0500, Matt Domsch wrote:
> On Thu, Aug 19, 2010 at 02:53:08PM -0700, Stephen Hemminger wrote:
> > On Thu, 19 Aug 2010 16:33:14 -0500
> > Matt Domsch <Matt_Domsch@Dell•com> wrote:
> >
> > > On Wed, Aug 18, 2010 at 02:41:24PM -0700, Stephen Hemminger wrote:
> > > > The netdev_alias_to_kernelname should only happen after normal lookup failed.
> > >
> > > Stephen, can you enlighten me as to the "right" way to do interface
> > > name lookups? While I can still find examples of parsing
> > > /proc/net/dev, or globbing /sys/class/net/*, I expect these aren't the
> > > preferred method anymore. Your own iproute2 suite uses RTM_GETLINK
> > > netlink calls, though for the seeming simple case of "give me a list of all
> > > interfaces", your path through there is far more capable (and
> > > complex) than I would hope to need.
> >
> > There is no magic right way. We have to support multiple interfaces.
> > I am really concerned that all this alias stuff will turn into a
> > disaster when there are 10,000 interfaces (Vlans). The kernel has
> > lots of tables and hashes to handle this but if the utilities
> > are doing a dumb scan of all names it will not work.
>
> We can remove the dumb scan of names if we expose the labels in a
> second way. For consideration of the approach:
>
>
> >From 86b8ab8d24df722073d1118659c1eb269c5d9dd7 Mon Sep 17 00:00:00 2001
> From: Matt Domsch <Matt_Domsch@dell•com>
> Date: Wed, 25 Aug 2010 15:31:01 -0500
> Subject: [PATCH] create /sys/firmware/pci for firmware-exposed label lookup
>
> libnetdevname needs to be able to look up a (PCI) device by label. It
> currently does this by glob()ing /sys/class/net/*/device/label to find
> all network interfaces that have a PCI device label exposed. There can
> be arbitrarily many network interfaces, making this a very expensive
> operation.
So you want to add code to the kernel to make userspace have an easier
'find' path? I'm all for making stuff easier, but just how "expensive"
is such an operation today?
> This patch creates a new directory /sys/firmware/pci. This directory
> contains symlinks, named for the PCI device labels that are exposed in
> the 'label' file of each PCI device already. Here is an example on a
> Dell PowerEdge R610 server:
>
> $ ls -l /sys/firmware/pci/
> total 0
> lrwxrwxrwx. 1 root root 0 2010-08-25 15:28 Embedded NIC 1 -> ../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/
> lrwxrwxrwx. 1 root root 0 2010-08-25 15:28 Embedded NIC 2 -> ../../devices/pci0000:00/0000:00:01.0/0000:01:00.1/
> lrwxrwxrwx. 1 root root 0 2010-08-25 15:28 Embedded NIC 3 -> ../../devices/pci0000:00/0000:00:03.0/0000:02:00.0/
> lrwxrwxrwx. 1 root root 0 2010-08-25 15:28 Embedded NIC 4 -> ../../devices/pci0000:00/0000:00:03.0/0000:02:00.1/
> lrwxrwxrwx. 1 root root 0 2010-08-25 15:28 Embedded Video -> ../../devices/pci0000:00/0000:00:1e.0/0000:09:03.0/
> lrwxrwxrwx. 1 root root 0 2010-08-25 15:28 Integrated SAS -> ../../devices/pci0000:00/0000:00:1c.0/0000:03:00.0/
>
> $ cat /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/label
> Embedded NIC 1
>
> Using this, libnetdevname can remove it's glob() call, and instead can
> look up the PCI device corresponding to a given label directly. It
> can then find the network interface name for that PCI device by
> looking in the net/ directory of that device.
>
> This makes no attempt to deal with SMBIOS that reports the same label
> for two different PCI devices (if such were to exist).
I'm sure it does, and I will end up getting the bugs reported to me as
it shows a sysfs WARN_ON() when that happens.
So please, handle this case to properly fail if there are duplicate
names, as we all know it will happen (look at all of the duplicate slot
names that have been found already as proof of this...)
>
> Signed-off-by: Matt Domsch <Matt_Domsch@dell•com>
> ---
> drivers/pci/pci-label.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 66 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
> index 111500e..a48eedb 100644
> --- a/drivers/pci/pci-label.c
> +++ b/drivers/pci/pci-label.c
> @@ -19,6 +19,7 @@
> #include <linux/pci_ids.h>
> #include <linux/module.h>
> #include <linux/device.h>
> +#include <linux/string.h>
> #include "pci.h"
>
> enum smbios_attr_enum {
> @@ -131,13 +132,78 @@ pci_remove_smbiosname_file(struct pci_dev *pdev)
> sysfs_remove_group(&pdev->dev.kobj, &smbios_attr_group);
> }
>
> +static struct kobject *pci_label_kobj;
> +
> +static int pci_label_kobj_init(void)
> +{
> + pci_label_kobj = kobject_create_and_add("pci", firmware_kobj);
> + if (pci_label_kobj == NULL)
> + return -ENOMEM;
> + return 0;
> +}
> +
> +static int
> +pci_create_smbios_label_symlink(struct pci_dev *pdev)
> +{
> + char *label, *p;
> + mode_t mode;
> + int ret=-ENOMEM;
> +
> + if (!pdev)
> + return -ENODEV;
> + if (pci_label_kobj == NULL)
> + if (pci_label_kobj_init())
> + return -ENOMEM;
> + label = kmalloc(PAGE_SIZE, GFP_KERNEL);
> + if (label == NULL)
> + return -ENOMEM;
> + mode = find_smbios_instance_string(pdev, label, SMBIOS_ATTR_LABEL_SHOW);
> + if (mode == 0) {
> + ret = -ENODEV;
> + goto out_free;
> + }
> + p = strim(label);
> + ret = sysfs_create_link_nowarn(pci_label_kobj, &pdev->dev.kobj, p);
> +out_free:
> + kfree(label);
> + return ret;
> +}
> +
> +static void
> +pci_delete_smbios_label_symlink(struct pci_dev *pdev)
> +{
> + char *label, *p;
> + mode_t mode;
> +
> + if (!pdev)
> + return;
> + if (pci_label_kobj == NULL)
> + return;
> + label = kmalloc(PAGE_SIZE, GFP_KERNEL);
> + if (label == NULL)
> + return;
> + mode = find_smbios_instance_string(pdev, label, SMBIOS_ATTR_LABEL_SHOW);
> + if (mode == 0) {
> + goto out_free;
> + }
> + p = strim(label);
> + sysfs_delete_link(pci_label_kobj, &pdev->dev.kobj, p);
> +
> +out_free:
> + kfree(label);
> +}
> +
> +
> void pci_create_firmware_label_files(struct pci_dev *pdev)
> {
> if (!pci_create_smbiosname_file(pdev))
> ;
> + if (!pci_create_smbios_label_symlink(pdev))
> + ;
Why even have this check at all, if nothing is going to be done with it?
I missed this last time around for the smbios name stuff, but come on
people, that's horrible code.
thanks,
greg k-h
next prev parent reply other threads:[~2010-08-25 22:16 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-08-12 17:35 [PATCH] Add firmware label support to iproute2 Narendra K
2010-08-12 18:10 ` Stephen Hemminger
2010-08-12 18:12 ` Stephen Hemminger
2010-08-13 12:36 ` Narendra_K
2010-08-18 21:41 ` Stephen Hemminger
2010-08-19 21:33 ` Matt Domsch
2010-08-19 21:53 ` Stephen Hemminger
2010-08-19 22:18 ` Matt Domsch
2010-08-25 22:03 ` Matt Domsch
2010-08-25 22:16 ` Greg KH [this message]
2010-08-26 14:10 ` Matt Domsch
2010-08-26 0:16 ` Stephen Hemminger
2010-08-26 15:01 ` Matt Domsch
2010-08-26 15:17 ` Loke, Chetan
2010-08-26 15:21 ` Stephen Hemminger
2010-08-26 15:38 ` Loke, Chetan
2010-08-27 7:54 ` Kay Sievers
2010-08-26 16:38 ` Matt Domsch
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100825221628.GA24502@kroah.com \
--to=greg@kroah$(echo .)com \
--cc=Charles_Rose@Dell$(echo .)com \
--cc=Jordan_Hargrave@Dell$(echo .)com \
--cc=Matt_Domsch@Dell$(echo .)com \
--cc=Narendra_K@Dell$(echo .)com \
--cc=linux-pci@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=shemminger@vyatta$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox