Re: [PATCH v4 02/15] PCI Hotplug: serialize pci_hp_register/deregister

[Matthew Wilcox <matthew@wil.cx>]

On Fri, Oct 03, 2008 at 05:17:37PM -0600, Alex Chiang wrote:
> Add explicit locking to pci_hp_register/deregister. For a given pci_slot,
> we want to make sure that the pci_slot->hotplug pointer isn't being
> changed from underneath us.

I'm torn between saying this goes too far and this doesn't go far
enough.  If pci_hp_register() and pci_hp_deregister() can race against
each other, then I don't like the way we grab, release, grab, release
the pci_hotplug_slot_list_lock in pci_hp_deregister()/get_slot_from_name().
I think it would be smarter to grab the pci_hotplug_slot_list_lock
before calling get_slot_from_name() and release it after the list_del().

Mind you, I can't see a race there, but I'm also not at my best today
and it's possible there's one there I can't see.

As it currently stands, I don't think it needs to be a mutex -- do we do
anything sleeping under it?

Another part of me says "Just slap a mutex around both functions and
have done with it".

So, in summary, more eyes on this please.  I don't think I can do a good
job today at spotting races.

> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
> index 0e7a511..e00266b 100644
> --- a/drivers/pci/hotplug/pci_hotplug_core.c
> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
> @@ -37,6 +37,7 @@
>  #include <linux/init.h>
>  #include <linux/mount.h>
>  #include <linux/namei.h>
> +#include <linux/mutex.h>
>  #include <linux/pci.h>
>  #include <linux/pci_hotplug.h>
>  #include <asm/uaccess.h>
> @@ -554,6 +555,7 @@ out:
>   *
>   * Returns 0 if successful, anything else for an error.
>   */
> +static DEFINE_MUTEX(pci_hp_mutex);
>  int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
>  			const char *name)
>  {
> @@ -583,7 +585,9 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
>  	if (IS_ERR(pci_slot))
>  		return PTR_ERR(pci_slot);
>  
> +	mutex_lock(&pci_hp_mutex);
>  	if (pci_slot->hotplug) {
> +		mutex_unlock(&pci_hp_mutex);
>  		dbg("%s: already claimed\n", __func__);
>  		pci_destroy_slot(pci_slot);
>  		return -EBUSY;
> @@ -591,6 +595,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
>  
>  	slot->pci_slot = pci_slot;
>  	pci_slot->hotplug = slot;
> +	mutex_unlock(&pci_hp_mutex);
>  
>  	/*
>  	 * Allow pcihp drivers to override the ACPI_PCI_SLOT name.
> @@ -635,6 +640,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug)
>  	if (temp != hotplug)
>  		return -ENODEV;
>  
> +	mutex_lock(&pci_hp_mutex);
>  	spin_lock(&pci_hotplug_slot_list_lock);
>  	list_del(&hotplug->slot_list);
>  	spin_unlock(&pci_hotplug_slot_list_lock);
> @@ -645,6 +651,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug)
>  
>  	hotplug->release(hotplug);
>  	slot->hotplug = NULL;
> +	mutex_unlock(&pci_hp_mutex);
>  	pci_destroy_slot(slot);
>  
>  	return 0;
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 0c6db03..1fffb27 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -164,10 +164,10 @@ placeholder:
>  	pr_debug("%s: created pci_slot on %04x:%02x:%02x\n",
>  		 __func__, pci_domain_nr(parent), parent->number, slot_nr);
>  
> - out:
> +out:
>  	up_write(&pci_bus_sem);
>  	return slot;
> - err:
> +err:
>  	kfree(slot);
>  	slot = ERR_PTR(err);
>  	goto out;

Gratuitous whitespace change?

-- 
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."