Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756864AbYLCUCo (ORCPT ); Wed, 3 Dec 2008 15:02:44 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752703AbYLCTw2 (ORCPT ); Wed, 3 Dec 2008 14:52:28 -0500 Received: from kroah.org ([198.145.64.141]:59844 "EHLO coco.kroah.org" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754140AbYLCTw0 (ORCPT ); Wed, 3 Dec 2008 14:52:26 -0500 Date: Wed, 3 Dec 2008 11:50:50 -0800 From: Greg KH To: linux-kernel@vger.kernel.org, stable@kernel.org Cc: Justin Forbes , Zwane Mwaikambo , "Theodore Ts'o" , Randy Dunlap , Dave Jones , Chuck Wolber , Chris Wedgwood , Michael Krufky , Chuck Ebbert , Domenico Andreoli , Willy Tarreau , Rodrigo Rubira Branco , Jake Edge , Eugene Teo , torvalds@linux-foundation.org, akpm@linux-foundation.org, alan@lxorguk.ukuu.org.uk, jbarnes@virtuousgeek.org, kristen.c.accardi@intel.com, matthew@wil.cx, Kenji Kaneshige , Alex Chiang Subject: [patch 031/104] PCI: prevent duplicate slot names Message-ID: <20081203195050.GF8950@kroah.com> References: <20081203193901.715896543@mini.kroah.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline; filename="pci-prevent-duplicate-slot-names.patch" In-Reply-To: <20081203194725.GA8950@kroah.com> User-Agent: Mutt/1.5.16 (2007-06-09) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12494 Lines: 412 2.6.27-stable review patch. If anyone has any objections, please let us know. ------------------ From: Alex Chiang commit 5fe6cc60680d29740b85278e17a002fa27b7e642 upstream. Prevent callers of pci_create_slot() from registering slots with duplicate names. This condition occurs most often when PCI hotplug drivers are loaded on platforms with broken firmware that assigns identical names to multiple slots. We now rename these duplicate slots on behalf of the user. If firmware assigns the name N to multiple slots, then: The first registered slot is assigned N The second registered slot is assigned N-1 The third registered slot is assigned N-2 etc. This is the permanent fix mentioned in earlier commits d6a9e9b4 and 167e782e (shpchp/pciehp: Rename duplicate slot name...). We take advantage of the new 'hotplug' parameter in pci_create_slot() to prevent a slot create/rename race between hotplug drivers and detection drivers. Scenario A: hotplug driver detection driver -------------- ---------------- pci_create_slot(hotplug=set) pci_create_slot(hotplug=NULL) The hotplug driver creates the slot with its desired name, and then releases the semaphore. Now, the detection driver tries to create the same slot, but it already exists. We don't care about renaming, so return the existing slot. Scenario B: hotplug driver detection driver -------------- ---------------- pci_create_slot(hotplug=NULL) pci_create_slot(hotplug=set) The detection driver creates the slot with name "X". Then the hotplug driver tries to create the same slot, but wants the name "Y" instead. We detect that we're trying to create the same slot and that we also want a rename, so rename the slot to "Y" and return. Scenario C: hotplug driver hotplug driver -------------- ---------------- pci_create_slot(hotplug=set) pci_create_slot(hotplug=set) Two separate hotplug drivers are attempting to claim the slot and are passing valid hotplug_slot args to pci_create_slot(). We detect that the slot already has a ->hotplug callback, prevent a rename, and return -EBUSY. Cc: jbarnes@virtuousgeek.org Cc: kristen.c.accardi@intel.com Cc: matthew@wil.cx Acked-by: Kenji Kaneshige Signed-off-by: Alex Chiang Signed-off-by: Jesse Barnes Signed-off-by: Greg Kroah-Hartman --- drivers/pci/hotplug/pci_hotplug_core.c | 26 ------ drivers/pci/hotplug/pciehp_core.c | 14 --- drivers/pci/hotplug/shpchp_core.c | 15 --- drivers/pci/slot.c | 139 ++++++++++++++++++++++++++------- 4 files changed, 114 insertions(+), 80 deletions(-) --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -191,7 +191,6 @@ static int init_slots(struct controller struct slot *slot; struct hotplug_slot *hotplug_slot; struct hotplug_slot_info *info; - int len, dup = 1; int retval = -ENOMEM; list_for_each_entry(slot, &ctrl->slot_list, slot_list) { @@ -218,24 +217,11 @@ static int init_slots(struct controller dbg("Registering bus=%x dev=%x hp_slot=%x sun=%x " "slot_device_offset=%x\n", slot->bus, slot->device, slot->hp_slot, slot->number, ctrl->slot_device_offset); -duplicate_name: retval = pci_hp_register(hotplug_slot, ctrl->pci_dev->subordinate, slot->device, slot->name); if (retval) { - /* - * If slot N already exists, we'll try to create - * slot N-1, N-2 ... N-M, until we overflow. - */ - if (retval == -EEXIST) { - len = snprintf(slot->name, SLOT_NAME_SIZE, - "%d-%d", slot->number, dup++); - if (len < SLOT_NAME_SIZE) - goto duplicate_name; - else - err("duplicate slot name overflow\n"); - } err("pci_hp_register failed with error %d\n", retval); goto error_info; } --- a/drivers/pci/hotplug/pci_hotplug_core.c +++ b/drivers/pci/hotplug/pci_hotplug_core.c @@ -569,12 +569,6 @@ int pci_hp_register(struct hotplug_slot mutex_lock(&pci_hp_mutex); - /* Check if we have already registered a slot with the same name. */ - if (get_slot_from_name(name)) { - result = -EEXIST; - goto out; - } - /* * No problems if we call this interface from both ACPI_PCI_SLOT * driver and call it here again. If we've already created the @@ -583,27 +577,12 @@ int pci_hp_register(struct hotplug_slot pci_slot = pci_create_slot(bus, slot_nr, name, slot); if (IS_ERR(pci_slot)) { result = PTR_ERR(pci_slot); - goto cleanup; - } - - if (pci_slot->hotplug) { - dbg("%s: already claimed\n", __func__); - result = -EBUSY; - goto cleanup; + goto out; } slot->pci_slot = pci_slot; pci_slot->hotplug = slot; - /* - * Allow pcihp drivers to override the ACPI_PCI_SLOT name. - */ - if (strcmp(kobject_name(&pci_slot->kobj), name)) { - result = kobject_rename(&pci_slot->kobj, name); - if (result) - goto cleanup; - } - list_add(&slot->slot_list, &pci_hotplug_slot_list); result = fs_add_slot(pci_slot); @@ -612,9 +591,6 @@ int pci_hp_register(struct hotplug_slot out: mutex_unlock(&pci_hp_mutex); return result; -cleanup: - pci_destroy_slot(pci_slot); - goto out; } /** --- a/drivers/pci/hotplug/shpchp_core.c +++ b/drivers/pci/hotplug/shpchp_core.c @@ -102,7 +102,7 @@ static int init_slots(struct controller struct hotplug_slot *hotplug_slot; struct hotplug_slot_info *info; int retval = -ENOMEM; - int i, len, dup = 1; + int i; for (i = 0; i < ctrl->num_slots; i++) { slot = kzalloc(sizeof(*slot), GFP_KERNEL); @@ -144,23 +144,10 @@ static int init_slots(struct controller dbg("Registering bus=%x dev=%x hp_slot=%x sun=%x " "slot_device_offset=%x\n", slot->bus, slot->device, slot->hp_slot, slot->number, ctrl->slot_device_offset); -duplicate_name: retval = pci_hp_register(slot->hotplug_slot, ctrl->pci_dev->subordinate, slot->device, hotplug_slot->name); if (retval) { - /* - * If slot N already exists, we'll try to create - * slot N-1, N-2 ... N-M, until we overflow. - */ - if (retval == -EEXIST) { - len = snprintf(slot->name, SLOT_NAME_SIZE, - "%d-%d", slot->number, dup++); - if (len < SLOT_NAME_SIZE) - goto duplicate_name; - else - err("duplicate slot name overflow\n"); - } err("pci_hp_register failed with error %d\n", retval); goto error_info; } --- a/drivers/pci/slot.c +++ b/drivers/pci/slot.c @@ -73,6 +73,77 @@ static struct kobj_type pci_slot_ktype = .default_attrs = pci_slot_default_attrs, }; +static char *make_slot_name(const char *name) +{ + char *new_name; + int len, max, dup; + + new_name = kstrdup(name, GFP_KERNEL); + if (!new_name) + return NULL; + + /* + * Make sure we hit the realloc case the first time through the + * loop. 'len' will be strlen(name) + 3 at that point which is + * enough space for "name-X" and the trailing NUL. + */ + len = strlen(name) + 2; + max = 1; + dup = 1; + + for (;;) { + struct kobject *dup_slot; + dup_slot = kset_find_obj(pci_slots_kset, new_name); + if (!dup_slot) + break; + kobject_put(dup_slot); + if (dup == max) { + len++; + max *= 10; + kfree(new_name); + new_name = kmalloc(len, GFP_KERNEL); + if (!new_name) + break; + } + sprintf(new_name, "%s-%d", name, dup++); + } + + return new_name; +} + +static int rename_slot(struct pci_slot *slot, const char *name) +{ + int result = 0; + char *slot_name; + + if (strcmp(kobject_name(&slot->kobj), name) == 0) + return result; + + slot_name = make_slot_name(name); + if (!slot_name) + return -ENOMEM; + + result = kobject_rename(&slot->kobj, slot_name); + kfree(slot_name); + + return result; +} + +static struct pci_slot *get_slot(struct pci_bus *parent, int slot_nr) +{ + struct pci_slot *slot; + /* + * We already hold pci_bus_sem so don't worry + */ + list_for_each_entry(slot, &parent->slots, list) + if (slot->number == slot_nr) { + kobject_get(&slot->kobj); + return slot; + } + + return NULL; +} + /** * pci_create_slot - create or increment refcount for physical PCI slot * @parent: struct pci_bus of parent bridge @@ -85,7 +156,17 @@ static struct kobj_type pci_slot_ktype = * either return a new &struct pci_slot to the caller, or if the pci_slot * already exists, its refcount will be incremented. * - * Slots are uniquely identified by a @pci_bus, @slot_nr, @name tuple. + * Slots are uniquely identified by a @pci_bus, @slot_nr tuple. + * + * There are known platforms with broken firmware that assign the same + * name to multiple slots. Workaround these broken platforms by renaming + * the slots on behalf of the caller. If firmware assigns name N to + * multiple slots: + * + * The first slot is assigned N + * The second slot is assigned N-1 + * The third slot is assigned N-2 + * etc. * * Placeholder slots: * In most cases, @pci_bus, @slot_nr will be sufficient to uniquely identify @@ -94,12 +175,8 @@ static struct kobj_type pci_slot_ktype = * the slot. In this scenario, the caller may pass -1 for @slot_nr. * * The following semantics are imposed when the caller passes @slot_nr == - * -1. First, the check for existing %struct pci_slot is skipped, as the - * caller may know about several unpopulated slots on a given %struct - * pci_bus, and each slot would have a @slot_nr of -1. Uniqueness for - * these slots is then determined by the @name parameter. We expect - * kobject_init_and_add() to warn us if the caller attempts to create - * multiple slots with the same name. The other change in semantics is + * -1. First, we no longer check for an existing %struct pci_slot, as there + * may be many slots with @slot_nr of -1. The other change in semantics is * user-visible, which is the 'address' parameter presented in sysfs will * consist solely of a dddd:bb tuple, where dddd is the PCI domain of the * %struct pci_bus and bb is the bus number. In other words, the devfn of @@ -111,44 +188,53 @@ struct pci_slot *pci_create_slot(struct struct hotplug_slot *hotplug) { struct pci_slot *slot; - int err; + int err = 0; + char *slot_name = NULL; down_write(&pci_bus_sem); if (slot_nr == -1) goto placeholder; - /* If we've already created this slot, bump refcount and return. */ - list_for_each_entry(slot, &parent->slots, list) { - if (slot->number == slot_nr) { - kobject_get(&slot->kobj); - pr_debug("%s: inc refcount to %d on %04x:%02x:%02x\n", - __func__, - atomic_read(&slot->kobj.kref.refcount), - pci_domain_nr(parent), parent->number, - slot_nr); - goto out; + /* + * Hotplug drivers are allowed to rename an existing slot, + * but only if not already claimed. + */ + slot = get_slot(parent, slot_nr); + if (slot) { + if (hotplug) { + if ((err = slot->hotplug ? -EBUSY : 0) + || (err = rename_slot(slot, name))) { + kobject_put(&slot->kobj); + slot = NULL; + goto err; + } } + goto out; } placeholder: slot = kzalloc(sizeof(*slot), GFP_KERNEL); if (!slot) { - slot = ERR_PTR(-ENOMEM); - goto out; + err = -ENOMEM; + goto err; } slot->bus = parent; slot->number = slot_nr; slot->kobj.kset = pci_slots_kset; - err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL, - "%s", name); - if (err) { - printk(KERN_ERR "Unable to register kobject %s\n", name); + slot_name = make_slot_name(name); + if (!slot_name) { + err = -ENOMEM; goto err; } + err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL, + "%s", slot_name); + if (err) + goto err; + INIT_LIST_HEAD(&slot->list); list_add(&slot->list, &parent->slots); @@ -156,10 +242,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; @@ -205,7 +291,6 @@ EXPORT_SYMBOL_GPL(pci_update_slot_number * just call kobject_put on its kobj and let our release methods do the * rest. */ - void pci_destroy_slot(struct pci_slot *slot) { pr_debug("%s: dec refcount to %d on %04x:%02x:%02x\n", __func__, -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/