Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759028AbYLDWLb (ORCPT ); Thu, 4 Dec 2008 17:11:31 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1756438AbYLDWKo (ORCPT ); Thu, 4 Dec 2008 17:10:44 -0500 Received: from g1t0027.austin.hp.com ([15.216.28.34]:1993 "EHLO g1t0027.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758922AbYLDWKm (ORCPT ); Thu, 4 Dec 2008 17:10:42 -0500 Date: Thu, 4 Dec 2008 15:10:39 -0700 From: Alex Chiang To: Greg KH Cc: linux-kernel@vger.kernel.org, stable@kernel.org, Jake Edge , jbarnes@virtuousgeek.org, kristen.c.accardi@intel.com, matthew@wil.cx, Kenji Kaneshige Subject: Re: [patch 031/104] PCI: prevent duplicate slot names Message-ID: <20081204221039.GJ31468@ldl.fc.hp.com> Mail-Followup-To: Alex Chiang , Greg KH , linux-kernel@vger.kernel.org, stable@kernel.org, Jake Edge , jbarnes@virtuousgeek.org, kristen.c.accardi@intel.com, matthew@wil.cx, Kenji Kaneshige References: <20081203193901.715896543@mini.kroah.org> <20081203195050.GF8950@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20081203195050.GF8950@kroah.com> User-Agent: Mutt/1.5.17+20080114 (2008-01-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14094 Lines: 440 Hi Greg, I found a memory leak that I introduced with the below patch. I sent the patch to Jesse a few days ago, but he hasn't pushed it upstream yet. http://article.gmane.org/gmane.linux.kernel.pci/2187/match=stop+leaking I did Cc: stable@kernel.org on it, but I'm guessing that since it's not upstream yet, you guys never saw it. Anyhow, please pick it up for this round of .27-stable, else we'll get a memory leak (while trying to work-around the duplicate slot name issue). Thanks. /ac * Greg KH : > 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/ > -- 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/