This is a second attempt at creating some handy symlinks in
/sys/bus/pci/ between slots and devices.
It addresses the following concerns from last time:
- does not create superfluous 'device' link
- correctly adds and removes links after hotplug ops
- adds a bunch of documentation ;)
It does not address Willy's concerns about not needing the
functionN back-links. I kinda thought they were useful, no one
else seemed to express an opinion...
This patch is against linux-next, but do note that you'll need
gregkh's "pci_get_dev_by_id refleak" patch:
http://lkml.org/lkml/2008/8/21/492
for this patch to work properly.
Thanks!
/ac
From: Alex Chiang <[email protected]>
Create convenience symlinks in sysfs, linking slots to device
functions, and vice versa. These links make it easier for users to
figure out which devices actually live in what slots.
For example:
sapphire:/sys/bus/pci/slots # ls
1 10 2 3 4 5 6 7 8 9
sapphire:/sys/bus/pci/slots # ls -l 3
total 0
-r--r--r-- 1 root root 65536 Aug 18 14:10 address
lrwxrwxrwx 1 root root 0 Aug 18 14:10 function0 ->
../../../../devices/pci0000:23/0000:23:01.0
lrwxrwxrwx 1 root root 0 Aug 18 14:10 function1 ->
../../../../devices/pci0000:23/0000:23:01.1
sapphire:/sys/bus/pci/slots # ls -l 3/function0/slot
lrwxrwxrwx 1 root root 0 Aug 18 14:13 3/function0/slot ->
../../../bus/pci/slots/3
The original form of this patch was written by Matthew Wilcox,
but did not have links from the sysfs slots/ directory pointing
back at the device functions.
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-pci | 39 +++++++++++++++++++++++++
drivers/pci/pci-sysfs.c | 37 +++++++++++++++++++++++
drivers/pci/slot.c | 48 +++++++++++++++++++++++++++++++
3 files changed, 124 insertions(+), 0 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index ceddcff..122490f 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -9,3 +9,42 @@ Description:
that some devices may have malformatted data. If the
underlying VPD has a writable section then the
corresponding section of this file will be writable.
+
+What: /sys/bus/pci/slots/...
+Date: April 2005 (possibly older)
+KernelVersion: 2.6.12 (possibly older)
+Contact: [email protected]
+Description:
+ When the appropriate driver is loaded, it will create a
+ directory per claimed physical PCI slot in
+ /sys/bus/pci/slots/. The names of these directories are
+ specific to the driver, which in turn, are specific to the
+ platform, but in general, should match the label on the
+ machine's physical chassis.
+
+ The drivers that can create slot directories include the
+ PCI hotplug drivers, and as of 2.6.27, the pci_slot driver.
+
+ The slot directories contain, at a minimum, a file named
+ 'address' which contains the PCI bus:device:function tuple.
+ Other files may appear as well, but are specific to the
+ driver.
+
+What: /sys/bus/pci/slots/.../function[0-7]
+Date: August 2008
+KernelVersion: 2.6.28
+Contact: [email protected]
+Description:
+ If PCI slot directories (as described above) are created,
+ and the physical slot is actually populated with a device,
+ symbolic links in the slot directory pointing to the
+ device's PCI functions are created as well.
+
+What: /sys/bus/pci/devices/.../slot
+Date: August 2008
+KernelVersion: 2.6.28
+Contact: [email protected]
+Description:
+ If PCI slot directories (as described above) are created,
+ a symbolic link pointing to the slot directory will be
+ created as well.
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 9c71858..c162c61 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -716,6 +716,39 @@ int __attribute__ ((weak)) pcibios_add_platform_entries(struct pci_dev *dev)
return 0;
}
+static void pci_remove_slot_links(struct pci_dev *dev)
+{
+ char func[10];
+ struct pci_slot *slot;
+
+ sysfs_remove_link(&dev->dev.kobj, "slot");
+ list_for_each_entry(slot, &dev->bus->slots, list) {
+ if (slot->number != PCI_SLOT(dev->devfn))
+ continue;
+ snprintf(func, 10, "function%d", PCI_FUNC(dev->devfn));
+ sysfs_remove_link(&slot->kobj, func);
+ }
+}
+
+static int pci_create_slot_links(struct pci_dev *dev)
+{
+ int result = 0;
+ char func[10];
+ struct pci_slot *slot;
+
+ list_for_each_entry(slot, &dev->bus->slots, list) {
+ if (slot->number != PCI_SLOT(dev->devfn))
+ continue;
+ result = sysfs_create_link(&dev->dev.kobj, &slot->kobj, "slot");
+ if (result)
+ goto out;
+ snprintf(func, 10, "function%d", PCI_FUNC(dev->devfn));
+ result = sysfs_create_link(&slot->kobj, &dev->dev.kobj, func);
+ }
+out:
+ return result;
+}
+
int __must_check pci_create_sysfs_dev_files (struct pci_dev *pdev)
{
struct bin_attribute *attr = NULL;
@@ -779,6 +812,8 @@ int __must_check pci_create_sysfs_dev_files (struct pci_dev *pdev)
pcie_aspm_create_sysfs_dev_files(pdev);
+ pci_create_slot_links(pdev);
+
return 0;
err_rom_file:
@@ -814,6 +849,8 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
if (!sysfs_initialized)
return;
+ pci_remove_slot_links(pdev);
+
pcie_aspm_remove_sysfs_dev_files(pdev);
if (pdev->vpd) {
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 7e5b85c..438c448 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -47,6 +47,50 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)
slot->number);
}
+static void remove_sysfs_files(struct pci_slot *slot)
+{
+ char func[10];
+ struct list_head *tmp;
+
+ list_for_each(tmp, &slot->bus->devices) {
+ struct pci_dev *dev = pci_dev_b(tmp);
+ if (PCI_SLOT(dev->devfn) != slot->number)
+ continue;
+ sysfs_remove_link(&dev->dev.kobj, "slot");
+
+ snprintf(func, 10, "function%d", PCI_FUNC(dev->devfn));
+ sysfs_remove_link(&slot->kobj, func);
+ }
+}
+
+static int create_sysfs_files(struct pci_slot *slot)
+{
+ int result;
+ char func[10];
+ struct list_head *tmp;
+
+ list_for_each(tmp, &slot->bus->devices) {
+ struct pci_dev *dev = pci_dev_b(tmp);
+ if (PCI_SLOT(dev->devfn) != slot->number)
+ continue;
+
+ result = sysfs_create_link(&dev->dev.kobj, &slot->kobj, "slot");
+ if (result)
+ goto fail;
+
+ snprintf(func, 10, "function%d", PCI_FUNC(dev->devfn));
+ result = sysfs_create_link(&slot->kobj, &dev->dev.kobj, func);
+ if (result)
+ goto fail;
+ }
+
+ return 0;
+
+fail:
+ remove_sysfs_files(slot);
+ return result;
+}
+
static void pci_slot_release(struct kobject *kobj)
{
struct pci_slot *slot = to_pci_slot(kobj);
@@ -54,6 +98,8 @@ static void pci_slot_release(struct kobject *kobj)
pr_debug("%s: releasing pci_slot on %x:%d\n", __func__,
slot->bus->number, slot->number);
+ remove_sysfs_files(slot);
+
list_del(&slot->list);
kfree(slot);
@@ -150,6 +196,8 @@ placeholder:
INIT_LIST_HEAD(&slot->list);
list_add(&slot->list, &parent->slots);
+ create_sysfs_files(slot);
+
/* Don't care if debug printk has a -1 for slot_nr */
pr_debug("%s: created pci_slot on %04x:%02x:%02x\n",
__func__, pci_domain_nr(parent), parent->number, slot_nr);
--
1.5.3.1.gbed62
On Fri, Aug 22, 2008 at 10:20:49AM -0600, Alex Chiang wrote:
> This is a second attempt at creating some handy symlinks in
> /sys/bus/pci/ between slots and devices.
>
> It addresses the following concerns from last time:
>
> - does not create superfluous 'device' link
> - correctly adds and removes links after hotplug ops
> - adds a bunch of documentation ;)
>
> It does not address Willy's concerns about not needing the
> functionN back-links. I kinda thought they were useful, no one
> else seemed to express an opinion...
I was just explaining why I didn't create them when I did my version of
this patch. I don't have an objection to adding them; they make logical
sense. The only concern might be the additional memory usage.
> +++ b/drivers/pci/pci-sysfs.c
> @@ -716,6 +716,39 @@ int __attribute__ ((weak)) pcibios_add_platform_entries(struct pci_dev *dev)
> return 0;
> }
>
> +static void pci_remove_slot_links(struct pci_dev *dev)
> +{
> + char func[10];
> + struct pci_slot *slot;
> +
> + sysfs_remove_link(&dev->dev.kobj, "slot");
> + list_for_each_entry(slot, &dev->bus->slots, list) {
> + if (slot->number != PCI_SLOT(dev->devfn))
> + continue;
> + snprintf(func, 10, "function%d", PCI_FUNC(dev->devfn));
> + sysfs_remove_link(&slot->kobj, func);
> + }
Rather than this loop, why not not use dev->slot?
> +static int pci_create_slot_links(struct pci_dev *dev)
> +{
Likewise in this function.
> +++ b/drivers/pci/slot.c
> +static void remove_sysfs_files(struct pci_slot *slot)
> +{
> + char func[10];
> + struct list_head *tmp;
> +
> + list_for_each(tmp, &slot->bus->devices) {
> + struct pci_dev *dev = pci_dev_b(tmp);
> + if (PCI_SLOT(dev->devfn) != slot->number)
> + continue;
> + sysfs_remove_link(&dev->dev.kobj, "slot");
> +
> + snprintf(func, 10, "function%d", PCI_FUNC(dev->devfn));
> + sysfs_remove_link(&slot->kobj, func);
> + }
> +}
It feels a bit strange to be doing this in two different files. I
understand why -- you've got a slot to remove or you've got a device to
remove, and in either case you have to get rid of the links.
Did you try putting all the logic in one of the two files and calling it
from the other?
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
* Matthew Wilcox <[email protected]>:
> On Fri, Aug 22, 2008 at 10:20:49AM -0600, Alex Chiang wrote:
> > This is a second attempt at creating some handy symlinks in
> > /sys/bus/pci/ between slots and devices.
> >
> > It addresses the following concerns from last time:
> >
> > - does not create superfluous 'device' link
> > - correctly adds and removes links after hotplug ops
> > - adds a bunch of documentation ;)
> >
> > It does not address Willy's concerns about not needing the
> > functionN back-links. I kinda thought they were useful, no one
> > else seemed to express an opinion...
>
> I was just explaining why I didn't create them when I did my version of
> this patch. I don't have an objection to adding them; they make logical
> sense. The only concern might be the additional memory usage.
Yes, agree about the memory usage, although I wonder what the
actual overhead is.
Does anyone have numbers for how much it costs to create a new
symlink? I could try and figure this out but it will take a few
days (busy with other stuff).
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -716,6 +716,39 @@ int __attribute__ ((weak)) pcibios_add_platform_entries(struct pci_dev *dev)
> > return 0;
> > }
> >
> > +static void pci_remove_slot_links(struct pci_dev *dev)
> > +{
> > + char func[10];
> > + struct pci_slot *slot;
> > +
> > + sysfs_remove_link(&dev->dev.kobj, "slot");
> > + list_for_each_entry(slot, &dev->bus->slots, list) {
> > + if (slot->number != PCI_SLOT(dev->devfn))
> > + continue;
> > + snprintf(func, 10, "function%d", PCI_FUNC(dev->devfn));
> > + sysfs_remove_link(&slot->kobj, func);
> > + }
>
> Rather than this loop, why not not use dev->slot?
>
> > +static int pci_create_slot_links(struct pci_dev *dev)
> > +{
>
> Likewise in this function.
Good points, both. I'll try that for v3.
> > +++ b/drivers/pci/slot.c
> > +static void remove_sysfs_files(struct pci_slot *slot)
> > +{
> > + char func[10];
> > + struct list_head *tmp;
> > +
> > + list_for_each(tmp, &slot->bus->devices) {
> > + struct pci_dev *dev = pci_dev_b(tmp);
> > + if (PCI_SLOT(dev->devfn) != slot->number)
> > + continue;
> > + sysfs_remove_link(&dev->dev.kobj, "slot");
> > +
> > + snprintf(func, 10, "function%d", PCI_FUNC(dev->devfn));
> > + sysfs_remove_link(&slot->kobj, func);
> > + }
> > +}
>
> It feels a bit strange to be doing this in two different files. I
> understand why -- you've got a slot to remove or you've got a device to
> remove, and in either case you have to get rid of the links.
>
> Did you try putting all the logic in one of the two files and calling it
> from the other?
I did actually try that at first, albeit as a single function to
create links for either type of caller and another function to
remove links for either type of caller.
It was possible, but the looping I had to do was ugly. I could
try again, and possible be smarter about detecting what was
passed in... or making the interface more complicated... or at
least just patching the same file and keeping the two different
flavours of add/remove links.
Thanks.
/ac
On Fri, Aug 22, 2008 at 01:53:58PM -0600, Alex Chiang wrote:
> * Matthew Wilcox <[email protected]>:
> > On Fri, Aug 22, 2008 at 10:20:49AM -0600, Alex Chiang wrote:
> > > This is a second attempt at creating some handy symlinks in
> > > /sys/bus/pci/ between slots and devices.
> > >
> > > It addresses the following concerns from last time:
> > >
> > > - does not create superfluous 'device' link
> > > - correctly adds and removes links after hotplug ops
> > > - adds a bunch of documentation ;)
> > >
> > > It does not address Willy's concerns about not needing the
> > > functionN back-links. I kinda thought they were useful, no one
> > > else seemed to express an opinion...
> >
> > I was just explaining why I didn't create them when I did my version of
> > this patch. I don't have an objection to adding them; they make logical
> > sense. The only concern might be the additional memory usage.
>
> Yes, agree about the memory usage, although I wonder what the
> actual overhead is.
>
> Does anyone have numbers for how much it costs to create a new
> symlink? I could try and figure this out but it will take a few
> days (busy with other stuff).
Almost nothing.
sysfs creates these things on the fly as they are accessed, and if
memory pressure on the machine happens, they are freed up properly and
then created again if a user asks to see them in the tree.
So don't worry about memory issues when adding new files or symlinks in
sysfs, it just isn't a problem (we handle 20000 disks easily on low
memory 31bit s390 systems.)
thanks,
greg k-h
On Sat, Aug 23, 2008 at 02:04:46PM -0600, Alex Chiang wrote:
> * Greg KH <[email protected]>:
> > On Fri, Aug 22, 2008 at 01:53:58PM -0600, Alex Chiang wrote:
> > >
> > > Does anyone have numbers for how much it costs to create a new
> > > symlink? I could try and figure this out but it will take a few
> > > days (busy with other stuff).
> >
> > Almost nothing.
> >
> > sysfs creates these things on the fly as they are accessed, and if
> > memory pressure on the machine happens, they are freed up properly and
> > then created again if a user asks to see them in the tree.
> >
> > So don't worry about memory issues when adding new files or symlinks in
> > sysfs, it just isn't a problem (we handle 20000 disks easily on low
> > memory 31bit s390 systems.)
>
> Great, thanks for the explanation. I've heard the "memory
> overhead" argument before for not wanting to create other sysfs
> files/links, so this will be good to debunk that bogeyman if it
> pops up again in the future.
Yes, please do.
> Did you get a chance to take a look at the documentation I wrote
> for these new symlinks? [I also went and documented the existing
> slots/ directory as well...]
>
> Was it what you had in mind?
Yes, it looked very good.
And thanks for the other documentation as well, if you want, you could
split that out as a different patch and odds are Jesse could get that
into the tree before 2.6.27 comes out :)
thanks,
greg k-h
* Greg KH <[email protected]>:
> On Fri, Aug 22, 2008 at 01:53:58PM -0600, Alex Chiang wrote:
> >
> > Does anyone have numbers for how much it costs to create a new
> > symlink? I could try and figure this out but it will take a few
> > days (busy with other stuff).
>
> Almost nothing.
>
> sysfs creates these things on the fly as they are accessed, and if
> memory pressure on the machine happens, they are freed up properly and
> then created again if a user asks to see them in the tree.
>
> So don't worry about memory issues when adding new files or symlinks in
> sysfs, it just isn't a problem (we handle 20000 disks easily on low
> memory 31bit s390 systems.)
Great, thanks for the explanation. I've heard the "memory
overhead" argument before for not wanting to create other sysfs
files/links, so this will be good to debunk that bogeyman if it
pops up again in the future.
Did you get a chance to take a look at the documentation I wrote
for these new symlinks? [I also went and documented the existing
slots/ directory as well...]
Was it what you had in mind?
Thanks.
/ac
* Greg KH <[email protected]>:
> On Sat, Aug 23, 2008 at 02:04:46PM -0600, Alex Chiang wrote:
>
> > Did you get a chance to take a look at the documentation I wrote
> > for these new symlinks? [I also went and documented the existing
> > slots/ directory as well...]
> >
> > Was it what you had in mind?
>
> Yes, it looked very good.
Thanks.
> And thanks for the other documentation as well, if you want,
> you could split that out as a different patch and odds are
> Jesse could get that into the tree before 2.6.27 comes out :)
Hrm, given the harder line Linus has taken against "regression
only" patches lately, it can probably wait another release cycle.
After all, it's been un-documented since pre-git history anyway.
;)
Thanks.
/ac
On Tue, Aug 26, 2008 at 09:50:03PM -0600, Alex Chiang wrote:
> * Greg KH <[email protected]>:
> > And thanks for the other documentation as well, if you want,
> > you could split that out as a different patch and odds are
> > Jesse could get that into the tree before 2.6.27 comes out :)
>
> Hrm, given the harder line Linus has taken against "regression
> only" patches lately, it can probably wait another release cycle.
Documentation updates and new stand-alone drivers are ok at all times :)
thanks,
greg k-h
On Tue, Aug 26, 2008 at 09:01:35PM -0700, Greg KH wrote:
> On Tue, Aug 26, 2008 at 09:50:03PM -0600, Alex Chiang wrote:
> > * Greg KH <[email protected]>:
> > > And thanks for the other documentation as well, if you want,
> > > you could split that out as a different patch and odds are
> > > Jesse could get that into the tree before 2.6.27 comes out :)
> >
> > Hrm, given the harder line Linus has taken against "regression
> > only" patches lately, it can probably wait another release cycle.
>
> Documentation updates and new stand-alone drivers are ok at all times :)
You're clearly not hanging on every word of our Dear Leader:
http://marc.info/?l=linux-kernel&m=121924636529367&w=2
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Wed, Aug 27, 2008 at 08:21:39AM -0600, Matthew Wilcox wrote:
> On Tue, Aug 26, 2008 at 09:01:35PM -0700, Greg KH wrote:
> > On Tue, Aug 26, 2008 at 09:50:03PM -0600, Alex Chiang wrote:
> > > * Greg KH <[email protected]>:
> > > > And thanks for the other documentation as well, if you want,
> > > > you could split that out as a different patch and odds are
> > > > Jesse could get that into the tree before 2.6.27 comes out :)
> > >
> > > Hrm, given the harder line Linus has taken against "regression
> > > only" patches lately, it can probably wait another release cycle.
> >
> > Documentation updates and new stand-alone drivers are ok at all times :)
>
> You're clearly not hanging on every word of our Dear Leader:
>
> http://marc.info/?l=linux-kernel&m=121924636529367&w=2
I read that, and parsed:
I generally won't complain about them, but I also don't see the
point.
as being up to the maintainer's judgement.
So in this case, it's up to Jesse :)
thanks,
greg k-h
On Wednesday, August 27, 2008 8:04 am Greg KH wrote:
> On Wed, Aug 27, 2008 at 08:21:39AM -0600, Matthew Wilcox wrote:
> > On Tue, Aug 26, 2008 at 09:01:35PM -0700, Greg KH wrote:
> > > On Tue, Aug 26, 2008 at 09:50:03PM -0600, Alex Chiang wrote:
> > > > * Greg KH <[email protected]>:
> > > > > And thanks for the other documentation as well, if you want,
> > > > > you could split that out as a different patch and odds are
> > > > > Jesse could get that into the tree before 2.6.27 comes out :)
> > > >
> > > > Hrm, given the harder line Linus has taken against "regression
> > > > only" patches lately, it can probably wait another release cycle.
> > >
> > > Documentation updates and new stand-alone drivers are ok at all times
> > > :)
> >
> > You're clearly not hanging on every word of our Dear Leader:
> >
> > http://marc.info/?l=linux-kernel&m=121924636529367&w=2
>
> I read that, and parsed:
> I generally won't complain about them, but I also don't see the
> point.
>
> as being up to the maintainer's judgement.
>
> So in this case, it's up to Jesse :)
Linus seems cranky lately, no need to tempt him to flame me I think (besides I
don't have any other critical stuff queued up atm; I'm definitely not going
to ask him to pull just a doc update this late in the cycle).
--
Jesse Barnes, Intel Open Source Technology Center