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