[PATCH] PCI: create device, function symlinks in /sys/bus/pci/slots/N/

[PATCH] PCI: create device, function symlinks in /sys/bus/pci/slots/N/

[Alex Chiang]

Create convenience symlinks in sysfs, linking slots to devices
and functions, and vice versa. These links make it easier for
users to figure out which devices actually live in what slots.

The device symlink points to the device's first function.

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 device -> ../../../../devices/pci0000:23/0000:23:01.0
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/device/slot
lrwxrwxrwx 1 root root 0 Aug 18 14:13 3/device/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 devices and functions.

Cc: matthew@wil.cx
Signed-off-by: Alex Chiang <achiang@hp.com>
---
 slot.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 7e5b85c..76095ac 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -47,6 +47,60 @@ 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);
+	}
+	sysfs_remove_link(&slot->kobj, "device");
+}
+
+static int create_sysfs_files(struct pci_slot *slot)
+{
+	int result, lastdev = -1;
+	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;
+
+		if (PCI_SLOT(dev->devfn) != lastdev) {
+			lastdev = PCI_SLOT(dev->devfn);
+			result = sysfs_create_link(&slot->kobj,
+						   &dev->dev.kobj,
+						   "device");
+			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 +108,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 +206,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);

Re: [PATCH] PCI: create device, function symlinks in /sys/bus/pci/slots/N/

[Greg KH]

On Mon, Aug 18, 2008 at 02:21:00PM -0600, Alex Chiang wrote:
> Create convenience symlinks in sysfs, linking slots to devices
> and functions, and vice versa. These links make it easier for
> users to figure out which devices actually live in what slots.
> 
> The device symlink points to the device's first function.
> 
> 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 device -> ../../../../devices/pci0000:23/0000:23:01.0
> 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/device/slot
> lrwxrwxrwx 1 root root 0 Aug 18 14:13 3/device/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 devices and functions.

As you are adding new sysfs files/symlinks, please document them in
Documentation/ABI.

thanks,

greg k-h

Re: [PATCH] PCI: create device, function symlinks in /sys/bus/pci/slots/N/

[H. Peter Anvin]

Alex Chiang wrote:
> Create convenience symlinks in sysfs, linking slots to devices
> and functions, and vice versa. These links make it easier for
> users to figure out which devices actually live in what slots.
> 
> The device symlink points to the device's first function.
> 
> 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 device -> ../../../../devices/pci0000:23/0000:23:01.0
> 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
> 

What's the point with "device" and "function0"?  They appear to be the 
same thing by definition.

	-hpa

Re: [PATCH] PCI: create device, function symlinks in /sys/bus/pci/slots/N/

[Gary Hade]

On Mon, Aug 18, 2008 at 02:21:00PM -0600, Alex Chiang wrote:
> Create convenience symlinks in sysfs, linking slots to devices
> and functions, and vice versa. These links make it easier for
> users to figure out which devices actually live in what slots.

This looks it would be quite useful but the symlinks are not adjusted
during hot-add and hot-remove operations (e.g. stale symlinks persist
after hot-remove and new symlinks are not created in response to hot-add)
so it could confuse more than help on systems with PCI hotplug support.

Gary

-- 
Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
garyhade@us.ibm.com
http://www.ibm.com/linux/ltc

Re: [PATCH] PCI: create device, function symlinks in /sys/bus/pci/slots/N/

[Alex Chiang]

* H. Peter Anvin <hpa@zytor.com>:
> Alex Chiang wrote:
>> Create convenience symlinks in sysfs, linking slots to devices
>> and functions, and vice versa. These links make it easier for
>> users to figure out which devices actually live in what slots.
>>
>> The device symlink points to the device's first function.
>>
>> 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 device -> ../../../../devices/pci0000:23/0000:23:01.0
>> 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
>
> What's the point with "device" and "function0"?  They appear to be the  
> same thing by definition.

Good point, I'll remove 'device'.

Thanks.

/ac

Re: [PATCH] PCI: create device, function symlinks in /sys/bus/pci/slots/N/

[Matthew Wilcox]

On Mon, Aug 18, 2008 at 02:21:00PM -0600, Alex Chiang wrote:
> The original form of this patch was written by Matthew Wilcox,
> but did not have links from the sysfs slots/ directory pointing
> back at devices and functions.

I think the reason I didn't bother was that you could already get this
information from the 'address' file.  ie:

$ ls -l /sys/bus/pci/devices/`cat /sys/bus/pci/slots/3/address`*

But I don't think we had a way to go from a device to the slot it's in,
without searching through all the slots for matching address.

> +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);
> +	}
> +	sysfs_remove_link(&slot->kobj, "device");
> +}
> +
> +static int create_sysfs_files(struct pci_slot *slot)
> +{
> +	int result, lastdev = -1;
> +	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;

Why not use pci_get_slot()?

> +		result = sysfs_create_link(&dev->dev.kobj, &slot->kobj, "slot");
> +		if (result)
> +			goto fail;
> +
> +		if (PCI_SLOT(dev->devfn) != lastdev) {
> +			lastdev = PCI_SLOT(dev->devfn);
> +			result = sysfs_create_link(&slot->kobj,
> +						   &dev->dev.kobj,
> +						   "device");
> +			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 +108,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 +206,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);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Intel are signing my paycheques ... these opinions are still mine
"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."

Re: [PATCH] PCI: create device, function symlinks in /sys/bus/pci/slots/N/

[Alex Chiang]

* Matthew Wilcox <matthew@wil.cx>:
> On Mon, Aug 18, 2008 at 02:21:00PM -0600, Alex Chiang wrote:
> > The original form of this patch was written by Matthew Wilcox,
> > but did not have links from the sysfs slots/ directory pointing
> > back at devices and functions.
> 
> I think the reason I didn't bother was that you could already get this
> information from the 'address' file.  ie:
> 
> $ ls -l /sys/bus/pci/devices/`cat /sys/bus/pci/slots/3/address`*
> 
> But I don't think we had a way to go from a device to the slot it's in,
> without searching through all the slots for matching address.

Hm, ok. So I guess the tradeoff here is convenience vs. memory.

If others are opposed to a 'functionN' backlink, I don't have
very strong feelings, but I thought it was useful.

> > +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);
> > +	}
> > +	sysfs_remove_link(&slot->kobj, "device");
> > +}
> > +
> > +static int create_sysfs_files(struct pci_slot *slot)
> > +{
> > +	int result, lastdev = -1;
> > +	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;
> 
> Why not use pci_get_slot()?

This will deadlock, because we're already holding pci_bus_sem as
a writer, taken during pci_create_slot():

	down_write(&pci_bus_sem);

Also, it doesn't really help us get rid of a loop, since
slot->number doesn't encode the entire devfn; it only has the
device number. So we would still have to do something like this:

	for (i = 0; i < 8; i++) {
		/* XXX: deadlock! */
		dev = pci_get_slot(slot->bus, PCI_DEVFN(slot->number, i));
		if (!dev)
			break;

Of course, it is entirely possible that I misconstrued what you
were trying to suggest, so if I missed your point, please let me
know. :)

Thanks.

/ac

> 
> > +		result = sysfs_create_link(&dev->dev.kobj, &slot->kobj, "slot");
> > +		if (result)
> > +			goto fail;
> > +
> > +		if (PCI_SLOT(dev->devfn) != lastdev) {
> > +			lastdev = PCI_SLOT(dev->devfn);
> > +			result = sysfs_create_link(&slot->kobj,
> > +						   &dev->dev.kobj,
> > +						   "device");
> > +			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 +108,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 +206,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);
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Intel are signing my paycheques ... these opinions are still mine
> "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."
> 

Re: [PATCH] PCI: create device, function symlinks in /sys/bus/pci/slots/N/

[Alex Chiang]

* Gary Hade <garyhade@us.ibm.com>:
> On Mon, Aug 18, 2008 at 02:21:00PM -0600, Alex Chiang wrote:
> > Create convenience symlinks in sysfs, linking slots to
> > devices and functions, and vice versa. These links make it
> > easier for users to figure out which devices actually live in
> > what slots.
> 
> This looks it would be quite useful but the symlinks are not
> adjusted during hot-add and hot-remove operations (e.g. stale
> symlinks persist after hot-remove and new symlinks are not
> created in response to hot-add) so it could confuse more than
> help on systems with PCI hotplug support.

Hi Gary,

Yes, you're right. Willy's original version of the patch had
symmetric functions in pci_release_dev/pci_scan_device, which I
didn't include because I didn't understand why we needed that
code.

Now I understand. :)

Thanks for pointing it out. I'll work on this for v2.

/ac