linux-kernel - Re: [PATCH, v2] PCI: create function symlinks in /sys/bus/pci/slots/N/

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080822182317.GW8318@parisc-linux.org>
Date:	Fri, 22 Aug 2008 12:23:17 -0600
From:	Matthew Wilcox <matthew@....cx>
To:	Alex Chiang <achiang@...com>, jbarnes@...tuousgeek.org,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH, v2] PCI: create function symlinks in /sys/bus/pci/slots/N/

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."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/