| Message ID | 6ef59b1fd3aa05f9989d29d0988152f18e3b824e.1529173804.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
On Sat, Jun 16, 2018 at 10:37 PM Lukas Wunner <lukas@wunner.de> wrote: > > When a hotplug driver calls pci_hp_register(), all steps necessary for > registration are carried out in one go, including creation of a kobject > and addition to sysfs. That's a problem for pciehp once it's converted > to enable/disable the slot exclusively from the IRQ thread: The thread > needs to be spawned after creation of the kobject (because it uses the > kobject's name), but before addition to sysfs (because it will handle > enable/disable requests submitted via sysfs). > > pci_hp_deregister() does offer a ->release callback that's invoked > after deletion from sysfs and before destruction of the kobject. But > because pci_hp_register() doesn't offer a counterpart, hotplug drivers' > ->probe and ->remove code becomes asymmetric, which is error prone > as recently discovered use-after-free bugs in pciehp's ->remove hook > have shown. > > In a sense, this appears to be a case of the midlayer antipattern: > > "The core thesis of the "midlayer mistake" is that midlayers are > bad and should not exist. That common functionality which it is > so tempting to put in a midlayer should instead be provided as > library routines which can [be] used, augmented, or ignored by > each bottom level driver independently. Thus every subsystem > that supports multiple implementations (or drivers) should > provide a very thin top layer which calls directly into the > bottom layer drivers, and a rich library of support code that > eases the implementation of those drivers. This library is > available to, but not forced upon, those drivers." > -- Neil Brown (2009), https://lwn.net/Articles/336262/ > > The presence of midlayer traits in the PCI hotplug core might be ascribed > to its age: When it was introduced in February 2002, the blessings of a > library approach might not have been well known: > https://git.kernel.org/tglx/history/c/a8a2069f432c > > For comparison, the driver core does offer split functions for creating > a kobject (device_initialize()) and addition to sysfs (device_add()) as > an alternative to carrying out everything at once (device_register()). > This was introduced in October 2002: > https://git.kernel.org/tglx/history/c/8b290eb19962 > > The odd ->release callback in the PCI hotplug core was added in 2003: > https://git.kernel.org/tglx/history/c/69f8d663b595 > > Clearly, a library approach would not force every hotplug driver to > implement a ->release callback, but rather allow the driver to remove > the sysfs files, release its data structures and finally destroy the > kobject. Alternatively, a driver may choose to remove everything with > pci_hp_deregister(), then release its data structures. > > To this end, offer drivers pci_hp_initialize() and pci_hp_add() as a > split-up version of pci_hp_register(). Likewise, offer pci_hp_del() > and pci_hp_destroy() as a split-up version of pci_hp_deregister(). > > Eliminate the ->release callback and move its code into each driver's > teardown routine. > > Declare pci_hp_deregister() void, in keeping with the usual kernel > pattern that enablement can fail, but disablement cannot. It only > returned an error if the caller passed in a NULL pointer or a slot which > has never or is no longer registered or is sharing its name with another > slot. Those would be bugs, so WARN about them. Few hotplug drivers > actually checked the return value and those that did only printed a > useless error message to dmesg. Remove that. > > For most drivers the conversion was straightforward since it doesn't > matter whether the code in the ->release callback is executed before or > after destruction of the kobject. But in the case of ibmphp, it was > unclear to me whether setting slot_cur->ctrl and slot_cur->bus_on to > NULL needs to happen before the kobject is destroyed, so I erred on > the side of caution and ensured that the order stays the same. Another > nontrivial case is pnv_php, I've found the list and kref logic difficult > to understand, however my impression was that it is safe to delete the > list element and drop the references until after the kobject is > destroyed. > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Len Brown <lenb@kernel.org> > Cc: Scott Murray <scott@spiteful.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Gavin Shan <gwshan@linux.vnet.ibm.com> > Cc: Sebastian Ott <sebott@linux.vnet.ibm.com> > Cc: Gerald Schaefer <gerald.schaefer@de.ibm.com> > Cc: Corentin Chary <corentin.chary@gmail.com> > Cc: Darren Hart <dvhart@infradead.org> > Cc: Andy Shevchenko <andy@infradead.org> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > drivers/pci/hotplug/acpiphp_core.c | 22 +--- > drivers/pci/hotplug/cpci_hotplug_core.c | 14 +-- > drivers/pci/hotplug/cpqphp_core.c | 16 +-- > drivers/pci/hotplug/ibmphp_core.c | 15 ++- > drivers/pci/hotplug/ibmphp_ebda.c | 20 ---- > drivers/pci/hotplug/pci_hotplug_core.c | 139 +++++++++++++++++------- > drivers/pci/hotplug/pciehp_core.c | 19 +--- > drivers/pci/hotplug/pcihp_skeleton.c | 16 +-- > drivers/pci/hotplug/pnv_php.c | 5 +- > drivers/pci/hotplug/rpaphp_core.c | 2 +- > drivers/pci/hotplug/rpaphp_slot.c | 13 +-- > drivers/pci/hotplug/s390_pci_hpc.c | 13 +-- > drivers/pci/hotplug/sgi_hotplug.c | 9 +- > drivers/pci/hotplug/shpchp_core.c | 20 +--- > drivers/platform/x86/asus-wmi.c | 12 +- > drivers/platform/x86/eeepc-laptop.c | 12 +- So, as far as I can see these are just mechanical changes due to internal API change. Thus, Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> for PDx86 bits only. > include/linux/pci_hotplug.h | 15 ++- > 17 files changed, 171 insertions(+), 191 deletions(-) > > diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c > index 12b5655fd390..ad32ffbc4b91 100644 > --- a/drivers/pci/hotplug/acpiphp_core.c > +++ b/drivers/pci/hotplug/acpiphp_core.c > @@ -254,20 +254,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value) > return 0; > } > > -/** > - * release_slot - free up the memory used by a slot > - * @hotplug_slot: slot to free > - */ > -static void release_slot(struct hotplug_slot *hotplug_slot) > -{ > - struct slot *slot = hotplug_slot->private; > - > - pr_debug("%s - physical_slot = %s\n", __func__, slot_name(slot)); > - > - kfree(slot->hotplug_slot); > - kfree(slot); > -} > - > /* callback routine to initialize 'struct slot' for each slot */ > int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot, > unsigned int sun) > @@ -287,7 +273,6 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot, > slot->hotplug_slot->info = &slot->info; > > slot->hotplug_slot->private = slot; > - slot->hotplug_slot->release = &release_slot; > slot->hotplug_slot->ops = &acpi_hotplug_slot_ops; > > slot->acpi_slot = acpiphp_slot; > @@ -324,13 +309,12 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot, > void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot) > { > struct slot *slot = acpiphp_slot->slot; > - int retval = 0; > > pr_info("Slot [%s] unregistered\n", slot_name(slot)); > > - retval = pci_hp_deregister(slot->hotplug_slot); > - if (retval) > - pr_err("pci_hp_deregister failed with error %d\n", retval); > + pci_hp_deregister(slot->hotplug_slot); > + kfree(slot->hotplug_slot); > + kfree(slot); > } > > > diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c > index 07b533adc9df..52a339baf06c 100644 > --- a/drivers/pci/hotplug/cpci_hotplug_core.c > +++ b/drivers/pci/hotplug/cpci_hotplug_core.c > @@ -195,10 +195,8 @@ get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value) > return 0; > } > > -static void release_slot(struct hotplug_slot *hotplug_slot) > +static void release_slot(struct slot *slot) > { > - struct slot *slot = hotplug_slot->private; > - > kfree(slot->hotplug_slot->info); > kfree(slot->hotplug_slot); > pci_dev_put(slot->dev); > @@ -253,7 +251,6 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last) > snprintf(name, SLOT_NAME_SIZE, "%02x:%02x", bus->number, i); > > hotplug_slot->private = slot; > - hotplug_slot->release = &release_slot; > hotplug_slot->ops = &cpci_hotplug_slot_ops; > > /* > @@ -308,12 +305,8 @@ cpci_hp_unregister_bus(struct pci_bus *bus) > slots--; > > dbg("deregistering slot %s", slot_name(slot)); > - status = pci_hp_deregister(slot->hotplug_slot); > - if (status) { > - err("pci_hp_deregister failed with error %d", > - status); > - break; > - } > + pci_hp_deregister(slot->hotplug_slot); > + release_slot(slot); > } > } > up_write(&list_rwsem); > @@ -623,6 +616,7 @@ cleanup_slots(void) > list_for_each_entry_safe(slot, tmp, &slot_list, slot_list) { > list_del(&slot->slot_list); > pci_hp_deregister(slot->hotplug_slot); > + release_slot(slot); > } > cleanup_null: > up_write(&list_rwsem); > diff --git a/drivers/pci/hotplug/cpqphp_core.c b/drivers/pci/hotplug/cpqphp_core.c > index 1797e36ec586..5a06636e910a 100644 > --- a/drivers/pci/hotplug/cpqphp_core.c > +++ b/drivers/pci/hotplug/cpqphp_core.c > @@ -266,17 +266,6 @@ static void __iomem *get_SMBIOS_entry(void __iomem *smbios_start, > return previous; > } > > -static void release_slot(struct hotplug_slot *hotplug_slot) > -{ > - struct slot *slot = hotplug_slot->private; > - > - dbg("%s - physical_slot = %s\n", __func__, slot_name(slot)); > - > - kfree(slot->hotplug_slot->info); > - kfree(slot->hotplug_slot); > - kfree(slot); > -} > - > static int ctrl_slot_cleanup(struct controller *ctrl) > { > struct slot *old_slot, *next_slot; > @@ -285,9 +274,11 @@ static int ctrl_slot_cleanup(struct controller *ctrl) > ctrl->slot = NULL; > > while (old_slot) { > - /* memory will be freed by the release_slot callback */ > next_slot = old_slot->next; > pci_hp_deregister(old_slot->hotplug_slot); > + kfree(old_slot->hotplug_slot->info); > + kfree(old_slot->hotplug_slot); > + kfree(old_slot); > old_slot = next_slot; > } > > @@ -678,7 +669,6 @@ static int ctrl_slot_setup(struct controller *ctrl, > ((read_slot_enable(ctrl) << 2) >> ctrl_slot) & 0x04; > > /* register this slot with the hotplug pci core */ > - hotplug_slot->release = &release_slot; > hotplug_slot->private = slot; > snprintf(name, SLOT_NAME_SIZE, "%u", slot->number); > hotplug_slot->ops = &cpqphp_hotplug_slot_ops; > diff --git a/drivers/pci/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c > index 1869b0411ce0..4ea57e9019f1 100644 > --- a/drivers/pci/hotplug/ibmphp_core.c > +++ b/drivers/pci/hotplug/ibmphp_core.c > @@ -673,7 +673,20 @@ static void free_slots(void) > > list_for_each_entry_safe(slot_cur, next, &ibmphp_slot_head, > ibm_slot_list) { > - pci_hp_deregister(slot_cur->hotplug_slot); > + pci_hp_del(slot_cur->hotplug_slot); > + slot_cur->ctrl = NULL; > + slot_cur->bus_on = NULL; > + > + /* > + * We don't want to actually remove the resources, > + * since ibmphp_free_resources() will do just that. > + */ > + ibmphp_unconfigure_card(&slot_cur, -1); > + > + pci_hp_destroy(slot_cur->hotplug_slot); > + kfree(slot_cur->hotplug_slot->info); > + kfree(slot_cur->hotplug_slot); > + kfree(slot_cur); > } > debug("%s -- exit\n", __func__); > } > diff --git a/drivers/pci/hotplug/ibmphp_ebda.c b/drivers/pci/hotplug/ibmphp_ebda.c > index 64549aa24c0f..6f8e90e3ec08 100644 > --- a/drivers/pci/hotplug/ibmphp_ebda.c > +++ b/drivers/pci/hotplug/ibmphp_ebda.c > @@ -699,25 +699,6 @@ static int fillslotinfo(struct hotplug_slot *hotplug_slot) > return rc; > } > > -static void release_slot(struct hotplug_slot *hotplug_slot) > -{ > - struct slot *slot; > - > - if (!hotplug_slot || !hotplug_slot->private) > - return; > - > - slot = hotplug_slot->private; > - kfree(slot->hotplug_slot->info); > - kfree(slot->hotplug_slot); > - slot->ctrl = NULL; > - slot->bus_on = NULL; > - > - /* we don't want to actually remove the resources, since free_resources will do just that */ > - ibmphp_unconfigure_card(&slot, -1); > - > - kfree(slot); > -} > - > static struct pci_driver ibmphp_driver; > > /* > @@ -941,7 +922,6 @@ static int __init ebda_rsrc_controller(void) > tmp_slot->hotplug_slot = hp_slot_ptr; > > hp_slot_ptr->private = tmp_slot; > - hp_slot_ptr->release = release_slot; > > rc = fillslotinfo(hp_slot_ptr); > if (rc) > diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c > index fd93783a87b0..90fde5f106d8 100644 > --- a/drivers/pci/hotplug/pci_hotplug_core.c > +++ b/drivers/pci/hotplug/pci_hotplug_core.c > @@ -396,8 +396,9 @@ static struct hotplug_slot *get_slot_from_name(const char *name) > * @owner: caller module owner > * @mod_name: caller module name > * > - * Registers a hotplug slot with the pci hotplug subsystem, which will allow > - * userspace interaction to the slot. > + * Prepares a hotplug slot for in-kernel use and immediately publishes it to > + * user space in one go. Drivers may alternatively carry out the two steps > + * separately by invoking pci_hp_initialize() and pci_hp_add(). > * > * Returns 0 if successful, anything else for an error. > */ > @@ -406,54 +407,91 @@ int __pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, > struct module *owner, const char *mod_name) > { > int result; > + > + result = __pci_hp_initialize(slot, bus, devnr, name, owner, mod_name); > + if (result) > + return result; > + > + result = pci_hp_add(slot); > + if (result) > + pci_hp_destroy(slot); > + > + return result; > +} > +EXPORT_SYMBOL_GPL(__pci_hp_register); > + > +/** > + * __pci_hp_initialize - prepare hotplug slot for in-kernel use > + * @slot: pointer to the &struct hotplug_slot to initialize > + * @bus: bus this slot is on > + * @devnr: slot number > + * @name: name registered with kobject core > + * @owner: caller module owner > + * @mod_name: caller module name > + * > + * Allocate and fill in a PCI slot for use by a hotplug driver. Once this has > + * been called, the driver may invoke hotplug_slot_name() to get the slot's > + * unique name. The driver must be prepared to handle a ->reset_slot callback > + * from this point on. > + * > + * Returns 0 on success or a negative int on error. > + */ > +int __pci_hp_initialize(struct hotplug_slot *slot, struct pci_bus *bus, > + int devnr, const char *name, struct module *owner, > + const char *mod_name) > +{ > struct pci_slot *pci_slot; > > if (slot == NULL) > return -ENODEV; > if ((slot->info == NULL) || (slot->ops == NULL)) > return -EINVAL; > - if (slot->release == NULL) { > - dbg("Why are you trying to register a hotplug slot without a proper release function?\n"); > - return -EINVAL; > - } > > slot->ops->owner = owner; > slot->ops->mod_name = mod_name; > > - mutex_lock(&pci_hp_mutex); > /* > * No problems if we call this interface from both ACPI_PCI_SLOT > * driver and call it here again. If we've already created the > * pci_slot, the interface will simply bump the refcount. > */ > pci_slot = pci_create_slot(bus, devnr, name, slot); > - if (IS_ERR(pci_slot)) { > - result = PTR_ERR(pci_slot); > - goto out; > - } > + if (IS_ERR(pci_slot)) > + return PTR_ERR(pci_slot); > > slot->pci_slot = pci_slot; > pci_slot->hotplug = slot; > + return 0; > +} > +EXPORT_SYMBOL_GPL(__pci_hp_initialize); > > - list_add(&slot->slot_list, &pci_hotplug_slot_list); > +/** > + * pci_hp_add - publish hotplug slot to user space > + * @slot: pointer to the &struct hotplug_slot to publish > + * > + * Make a hotplug slot's sysfs interface available and inform user space of its > + * addition by sending a uevent. The hotplug driver must be prepared to handle > + * all &struct hotplug_slot_ops callbacks from this point on. > + * > + * Returns 0 on success or a negative int on error. > + */ > +int pci_hp_add(struct hotplug_slot *slot) > +{ > + struct pci_slot *pci_slot = slot->pci_slot; > + int result; > > result = fs_add_slot(pci_slot); > if (result) > - goto err_list_del; > + return result; > > kobject_uevent(&pci_slot->kobj, KOBJ_ADD); > - dbg("Added slot %s to the list\n", name); > - goto out; > - > -err_list_del: > - list_del(&slot->slot_list); > - pci_slot->hotplug = NULL; > - pci_destroy_slot(pci_slot); > -out: > + mutex_lock(&pci_hp_mutex); > + list_add(&slot->slot_list, &pci_hotplug_slot_list); > mutex_unlock(&pci_hp_mutex); > - return result; > + dbg("Added slot %s to the list\n", hotplug_slot_name(slot)); > + return 0; > } > -EXPORT_SYMBOL_GPL(__pci_hp_register); > +EXPORT_SYMBOL_GPL(pci_hp_add); > > /** > * pci_hp_deregister - deregister a hotplug_slot with the PCI hotplug subsystem > @@ -464,35 +502,62 @@ EXPORT_SYMBOL_GPL(__pci_hp_register); > * > * Returns 0 if successful, anything else for an error. > */ > -int pci_hp_deregister(struct hotplug_slot *slot) > +void pci_hp_deregister(struct hotplug_slot *slot) > +{ > + pci_hp_del(slot); > + pci_hp_destroy(slot); > +} > +EXPORT_SYMBOL_GPL(pci_hp_deregister); > + > +/** > + * pci_hp_del - unpublish hotplug slot from user space > + * @slot: pointer to the &struct hotplug_slot to unpublish > + * > + * Remove a hotplug slot's sysfs interface. > + * > + * Returns 0 on success or a negative int on error. > + */ > +void pci_hp_del(struct hotplug_slot *slot) > { > struct hotplug_slot *temp; > - struct pci_slot *pci_slot; > > - if (!slot) > - return -ENODEV; > + if (WARN_ON(!slot)) > + return; > > mutex_lock(&pci_hp_mutex); > temp = get_slot_from_name(hotplug_slot_name(slot)); > - if (temp != slot) { > + if (WARN_ON(temp != slot)) { > mutex_unlock(&pci_hp_mutex); > - return -ENODEV; > + return; > } > > list_del(&slot->slot_list); > - > - pci_slot = slot->pci_slot; > - fs_remove_slot(pci_slot); > + mutex_unlock(&pci_hp_mutex); > dbg("Removed slot %s from the list\n", hotplug_slot_name(slot)); > + fs_remove_slot(slot->pci_slot); > +} > +EXPORT_SYMBOL_GPL(pci_hp_del); > + > +/** > + * pci_hp_destroy - remove hotplug slot from in-kernel use > + * @slot: pointer to the &struct hotplug_slot to destroy > + * > + * Destroy a PCI slot used by a hotplug driver. Once this has been called, > + * the driver may no longer invoke hotplug_slot_name() to get the slot's > + * unique name. The driver no longer needs to handle a ->reset_slot callback > + * from this point on. > + * > + * Returns 0 on success or a negative int on error. > + */ > +void pci_hp_destroy(struct hotplug_slot *slot) > +{ > + struct pci_slot *pci_slot = slot->pci_slot; > > - slot->release(slot); > + slot->pci_slot = NULL; > pci_slot->hotplug = NULL; > pci_destroy_slot(pci_slot); > - mutex_unlock(&pci_hp_mutex); > - > - return 0; > } > -EXPORT_SYMBOL_GPL(pci_hp_deregister); > +EXPORT_SYMBOL_GPL(pci_hp_destroy); > > /** > * pci_hp_change_slot_info - changes the slot's information structure in the core > diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c > index b360645377c2..37d8f81e548f 100644 > --- a/drivers/pci/hotplug/pciehp_core.c > +++ b/drivers/pci/hotplug/pciehp_core.c > @@ -56,17 +56,6 @@ static int get_latch_status(struct hotplug_slot *slot, u8 *value); > static int get_adapter_status(struct hotplug_slot *slot, u8 *value); > static int reset_slot(struct hotplug_slot *slot, int probe); > > -/** > - * release_slot - free up the memory used by a slot > - * @hotplug_slot: slot to free > - */ > -static void release_slot(struct hotplug_slot *hotplug_slot) > -{ > - kfree(hotplug_slot->ops); > - kfree(hotplug_slot->info); > - kfree(hotplug_slot); > -} > - > static int init_slot(struct controller *ctrl) > { > struct slot *slot = ctrl->slot; > @@ -107,7 +96,6 @@ static int init_slot(struct controller *ctrl) > /* register this slot with the hotplug pci core */ > hotplug->info = info; > hotplug->private = slot; > - hotplug->release = &release_slot; > hotplug->ops = ops; > slot->hotplug_slot = hotplug; > snprintf(name, SLOT_NAME_SIZE, "%u", PSN(ctrl)); > @@ -127,7 +115,12 @@ static int init_slot(struct controller *ctrl) > > static void cleanup_slot(struct controller *ctrl) > { > - pci_hp_deregister(ctrl->slot->hotplug_slot); > + struct hotplug_slot *hotplug_slot = ctrl->slot->hotplug_slot; > + > + pci_hp_deregister(hotplug_slot); > + kfree(hotplug_slot->ops); > + kfree(hotplug_slot->info); > + kfree(hotplug_slot); > } > > /* > diff --git a/drivers/pci/hotplug/pcihp_skeleton.c b/drivers/pci/hotplug/pcihp_skeleton.c > index c19694a04d2c..94ad7cd72878 100644 > --- a/drivers/pci/hotplug/pcihp_skeleton.c > +++ b/drivers/pci/hotplug/pcihp_skeleton.c > @@ -210,16 +210,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value) > return retval; > } > > -static void release_slot(struct hotplug_slot *hotplug_slot) > -{ > - struct slot *slot = hotplug_slot->private; > - > - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name); > - kfree(slot->hotplug_slot->info); > - kfree(slot->hotplug_slot); > - kfree(slot); > -} > - > static void make_slot_name(struct slot *slot) > { > /* > @@ -270,7 +260,6 @@ static int __init init_slots(void) > > hotplug_slot->name = slot->name; > hotplug_slot->private = slot; > - hotplug_slot->release = &release_slot; > make_slot_name(slot); > hotplug_slot->ops = &skel_hotplug_slot_ops; > > @@ -311,12 +300,13 @@ static void __exit cleanup_slots(void) > > /* > * Unregister all of our slots with the pci_hotplug subsystem. > - * Memory will be freed in release_slot() callback after slot's > - * lifespan is finished. > */ > list_for_each_entry_safe(slot, next, &slot_list, slot_list) { > list_del(&slot->slot_list); > pci_hp_deregister(slot->hotplug_slot); > + kfree(slot->hotplug_slot->info); > + kfree(slot->hotplug_slot); > + kfree(slot); > } > } > > diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c > index 6c2e8d7307c6..3276a5e4c430 100644 > --- a/drivers/pci/hotplug/pnv_php.c > +++ b/drivers/pci/hotplug/pnv_php.c > @@ -538,9 +538,8 @@ static struct hotplug_slot_ops php_slot_ops = { > .disable_slot = pnv_php_disable_slot, > }; > > -static void pnv_php_release(struct hotplug_slot *slot) > +static void pnv_php_release(struct pnv_php_slot *php_slot) > { > - struct pnv_php_slot *php_slot = slot->private; > unsigned long flags; > > /* Remove from global or child list */ > @@ -596,7 +595,6 @@ static struct pnv_php_slot *pnv_php_alloc_slot(struct device_node *dn) > php_slot->power_state_check = false; > php_slot->slot.ops = &php_slot_ops; > php_slot->slot.info = &php_slot->slot_info; > - php_slot->slot.release = pnv_php_release; > php_slot->slot.private = php_slot; > > INIT_LIST_HEAD(&php_slot->children); > @@ -924,6 +922,7 @@ static void pnv_php_unregister_one(struct device_node *dn) > > php_slot->state = PNV_PHP_STATE_OFFLINE; > pci_hp_deregister(&php_slot->slot); > + pnv_php_release(php_slot); > pnv_php_put_slot(php_slot); > } > > diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c > index fb5e0845429d..857c358b727b 100644 > --- a/drivers/pci/hotplug/rpaphp_core.c > +++ b/drivers/pci/hotplug/rpaphp_core.c > @@ -404,13 +404,13 @@ static void __exit cleanup_slots(void) > /* > * Unregister all of our slots with the pci_hotplug subsystem, > * and free up all memory that we had allocated. > - * memory will be freed in release_slot callback. > */ > > list_for_each_entry_safe(slot, next, &rpaphp_slot_head, > rpaphp_slot_list) { > list_del(&slot->rpaphp_slot_list); > pci_hp_deregister(slot->hotplug_slot); > + dealloc_slot_struct(slot); > } > return; > } > diff --git a/drivers/pci/hotplug/rpaphp_slot.c b/drivers/pci/hotplug/rpaphp_slot.c > index 3840a2075e6a..b916c8e4372d 100644 > --- a/drivers/pci/hotplug/rpaphp_slot.c > +++ b/drivers/pci/hotplug/rpaphp_slot.c > @@ -19,12 +19,6 @@ > #include "rpaphp.h" > > /* free up the memory used by a slot */ > -static void rpaphp_release_slot(struct hotplug_slot *hotplug_slot) > -{ > - struct slot *slot = (struct slot *) hotplug_slot->private; > - dealloc_slot_struct(slot); > -} > - > void dealloc_slot_struct(struct slot *slot) > { > kfree(slot->hotplug_slot->info); > @@ -56,7 +50,6 @@ struct slot *alloc_slot_struct(struct device_node *dn, > slot->power_domain = power_domain; > slot->hotplug_slot->private = slot; > slot->hotplug_slot->ops = &rpaphp_hotplug_slot_ops; > - slot->hotplug_slot->release = &rpaphp_release_slot; > > return (slot); > > @@ -90,10 +83,8 @@ int rpaphp_deregister_slot(struct slot *slot) > __func__, slot->name); > > list_del(&slot->rpaphp_slot_list); > - > - retval = pci_hp_deregister(php_slot); > - if (retval) > - err("Problem unregistering a slot %s\n", slot->name); > + pci_hp_deregister(php_slot); > + dealloc_slot_struct(slot); > > dbg("%s - Exit: rc[%d]\n", __func__, retval); > return retval; > diff --git a/drivers/pci/hotplug/s390_pci_hpc.c b/drivers/pci/hotplug/s390_pci_hpc.c > index ffdc2977395d..93b5341d282c 100644 > --- a/drivers/pci/hotplug/s390_pci_hpc.c > +++ b/drivers/pci/hotplug/s390_pci_hpc.c > @@ -130,15 +130,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value) > return 0; > } > > -static void release_slot(struct hotplug_slot *hotplug_slot) > -{ > - struct slot *slot = hotplug_slot->private; > - > - kfree(slot->hotplug_slot->info); > - kfree(slot->hotplug_slot); > - kfree(slot); > -} > - > static struct hotplug_slot_ops s390_hotplug_slot_ops = { > .enable_slot = enable_slot, > .disable_slot = disable_slot, > @@ -175,7 +166,6 @@ int zpci_init_slot(struct zpci_dev *zdev) > hotplug_slot->info = info; > > hotplug_slot->ops = &s390_hotplug_slot_ops; > - hotplug_slot->release = &release_slot; > > get_power_status(hotplug_slot, &info->power_status); > get_adapter_status(hotplug_slot, &info->adapter_status); > @@ -209,5 +199,8 @@ void zpci_exit_slot(struct zpci_dev *zdev) > continue; > list_del(&slot->slot_list); > pci_hp_deregister(slot->hotplug_slot); > + kfree(slot->hotplug_slot->info); > + kfree(slot->hotplug_slot); > + kfree(slot); > } > } > diff --git a/drivers/pci/hotplug/sgi_hotplug.c b/drivers/pci/hotplug/sgi_hotplug.c > index 78b6bdbb3a39..babd23409f61 100644 > --- a/drivers/pci/hotplug/sgi_hotplug.c > +++ b/drivers/pci/hotplug/sgi_hotplug.c > @@ -628,7 +628,6 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus) > goto alloc_err; > } > bss_hotplug_slot->ops = &sn_hotplug_slot_ops; > - bss_hotplug_slot->release = &sn_release_slot; > > rc = pci_hp_register(bss_hotplug_slot, pci_bus, device, name); > if (rc) > @@ -656,8 +655,10 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus) > sn_release_slot(bss_hotplug_slot); > > /* destroy anything else on the list */ > - while ((bss_hotplug_slot = sn_hp_destroy())) > + while ((bss_hotplug_slot = sn_hp_destroy())) { > pci_hp_deregister(bss_hotplug_slot); > + sn_release_slot(bss_hotplug_slot); > + } > > return rc; > } > @@ -703,8 +704,10 @@ static void __exit sn_pci_hotplug_exit(void) > { > struct hotplug_slot *bss_hotplug_slot; > > - while ((bss_hotplug_slot = sn_hp_destroy())) > + while ((bss_hotplug_slot = sn_hp_destroy())) { > pci_hp_deregister(bss_hotplug_slot); > + sn_release_slot(bss_hotplug_slot); > + } > > if (!list_empty(&sn_hp_list)) > printk(KERN_ERR "%s: internal list is not empty\n", __FILE__); > diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c > index e91be287f292..5990695d1737 100644 > --- a/drivers/pci/hotplug/shpchp_core.c > +++ b/drivers/pci/hotplug/shpchp_core.c > @@ -61,22 +61,6 @@ static struct hotplug_slot_ops shpchp_hotplug_slot_ops = { > .get_adapter_status = get_adapter_status, > }; > > -/** > - * release_slot - free up the memory used by a slot > - * @hotplug_slot: slot to free > - */ > -static void release_slot(struct hotplug_slot *hotplug_slot) > -{ > - struct slot *slot = hotplug_slot->private; > - > - ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n", > - __func__, slot_name(slot)); > - > - kfree(slot->hotplug_slot->info); > - kfree(slot->hotplug_slot); > - kfree(slot); > -} > - > static int init_slots(struct controller *ctrl) > { > struct slot *slot; > @@ -125,7 +109,6 @@ static int init_slots(struct controller *ctrl) > > /* register this slot with the hotplug pci core */ > hotplug_slot->private = slot; > - hotplug_slot->release = &release_slot; > snprintf(name, SLOT_NAME_SIZE, "%d", slot->number); > hotplug_slot->ops = &shpchp_hotplug_slot_ops; > > @@ -171,6 +154,9 @@ void cleanup_slots(struct controller *ctrl) > cancel_delayed_work(&slot->work); > destroy_workqueue(slot->wq); > pci_hp_deregister(slot->hotplug_slot); > + kfree(slot->hotplug_slot->info); > + kfree(slot->hotplug_slot); > + kfree(slot); > } > } > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > index 3d523ca64694..d67f32a29bb4 100644 > --- a/drivers/platform/x86/asus-wmi.c > +++ b/drivers/platform/x86/asus-wmi.c > @@ -858,12 +858,6 @@ static int asus_get_adapter_status(struct hotplug_slot *hotplug_slot, > return 0; > } > > -static void asus_cleanup_pci_hotplug(struct hotplug_slot *hotplug_slot) > -{ > - kfree(hotplug_slot->info); > - kfree(hotplug_slot); > -} > - > static struct hotplug_slot_ops asus_hotplug_slot_ops = { > .owner = THIS_MODULE, > .get_adapter_status = asus_get_adapter_status, > @@ -905,7 +899,6 @@ static int asus_setup_pci_hotplug(struct asus_wmi *asus) > goto error_info; > > asus->hotplug_slot->private = asus; > - asus->hotplug_slot->release = &asus_cleanup_pci_hotplug; > asus->hotplug_slot->ops = &asus_hotplug_slot_ops; > asus_get_adapter_status(asus->hotplug_slot, > &asus->hotplug_slot->info->adapter_status); > @@ -1051,8 +1044,11 @@ static void asus_wmi_rfkill_exit(struct asus_wmi *asus) > * asus_unregister_rfkill_notifier() > */ > asus_rfkill_hotplug(asus); > - if (asus->hotplug_slot) > + if (asus->hotplug_slot) { > pci_hp_deregister(asus->hotplug_slot); > + kfree(asus->hotplug_slot->info); > + kfree(asus->hotplug_slot); > + } > if (asus->hotplug_workqueue) > destroy_workqueue(asus->hotplug_workqueue); > > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c > index 4c38904a8a32..a4bbf6ecd1f0 100644 > --- a/drivers/platform/x86/eeepc-laptop.c > +++ b/drivers/platform/x86/eeepc-laptop.c > @@ -726,12 +726,6 @@ static int eeepc_get_adapter_status(struct hotplug_slot *hotplug_slot, > return 0; > } > > -static void eeepc_cleanup_pci_hotplug(struct hotplug_slot *hotplug_slot) > -{ > - kfree(hotplug_slot->info); > - kfree(hotplug_slot); > -} > - > static struct hotplug_slot_ops eeepc_hotplug_slot_ops = { > .owner = THIS_MODULE, > .get_adapter_status = eeepc_get_adapter_status, > @@ -758,7 +752,6 @@ static int eeepc_setup_pci_hotplug(struct eeepc_laptop *eeepc) > goto error_info; > > eeepc->hotplug_slot->private = eeepc; > - eeepc->hotplug_slot->release = &eeepc_cleanup_pci_hotplug; > eeepc->hotplug_slot->ops = &eeepc_hotplug_slot_ops; > eeepc_get_adapter_status(eeepc->hotplug_slot, > &eeepc->hotplug_slot->info->adapter_status); > @@ -837,8 +830,11 @@ static void eeepc_rfkill_exit(struct eeepc_laptop *eeepc) > eeepc->wlan_rfkill = NULL; > } > > - if (eeepc->hotplug_slot) > + if (eeepc->hotplug_slot) { > pci_hp_deregister(eeepc->hotplug_slot); > + kfree(eeepc->hotplug_slot->info); > + kfree(eeepc->hotplug_slot); > + } > > if (eeepc->bluetooth_rfkill) { > rfkill_unregister(eeepc->bluetooth_rfkill); > diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h > index cf5e22103f68..a6d6650a0490 100644 > --- a/include/linux/pci_hotplug.h > +++ b/include/linux/pci_hotplug.h > @@ -80,15 +80,12 @@ struct hotplug_slot_info { > * @ops: pointer to the &struct hotplug_slot_ops to be used for this slot > * @info: pointer to the &struct hotplug_slot_info for the initial values for > * this slot. > - * @release: called during pci_hp_deregister to free memory allocated in a > - * hotplug_slot structure. > * @private: used by the hotplug pci controller driver to store whatever it > * needs. > */ > struct hotplug_slot { > struct hotplug_slot_ops *ops; > struct hotplug_slot_info *info; > - void (*release) (struct hotplug_slot *slot); > void *private; > > /* Variables below this are for use only by the hotplug pci core. */ > @@ -104,13 +101,23 @@ static inline const char *hotplug_slot_name(const struct hotplug_slot *slot) > int __pci_hp_register(struct hotplug_slot *slot, struct pci_bus *pbus, int nr, > const char *name, struct module *owner, > const char *mod_name); > -int pci_hp_deregister(struct hotplug_slot *slot); > +int __pci_hp_initialize(struct hotplug_slot *slot, struct pci_bus *bus, int nr, > + const char *name, struct module *owner, > + const char *mod_name); > +int pci_hp_add(struct hotplug_slot *slot); > + > +void pci_hp_del(struct hotplug_slot *slot); > +void pci_hp_destroy(struct hotplug_slot *slot); > +void pci_hp_deregister(struct hotplug_slot *slot); > + > int __must_check pci_hp_change_slot_info(struct hotplug_slot *slot, > struct hotplug_slot_info *info); > > /* use a define to avoid include chaining to get THIS_MODULE & friends */ > #define pci_hp_register(slot, pbus, devnr, name) \ > __pci_hp_register(slot, pbus, devnr, name, THIS_MODULE, KBUILD_MODNAME) > +#define pci_hp_initialize(slot, bus, nr, name) \ > + __pci_hp_initialize(slot, bus, nr, name, THIS_MODULE, KBUILD_MODNAME) > > /* PCI Setting Record (Type 0) */ > struct hpp_type0 { > -- > 2.17.1 >
On Sun, Jun 17, 2018 at 07:44:03PM +0300, Andy Shevchenko wrote: > On Sat, Jun 16, 2018 at 10:37 PM Lukas Wunner <lukas@wunner.de> wrote: > > > > When a hotplug driver calls pci_hp_register(), all steps necessary for > > registration are carried out in one go, including creation of a kobject > > and addition to sysfs. That's a problem for pciehp once it's converted > > to enable/disable the slot exclusively from the IRQ thread: The thread > > needs to be spawned after creation of the kobject (because it uses the > > kobject's name), but before addition to sysfs (because it will handle > > enable/disable requests submitted via sysfs). > > > > pci_hp_deregister() does offer a ->release callback that's invoked > > after deletion from sysfs and before destruction of the kobject. But > > because pci_hp_register() doesn't offer a counterpart, hotplug drivers' > > ->probe and ->remove code becomes asymmetric, which is error prone > > as recently discovered use-after-free bugs in pciehp's ->remove hook > > have shown. > > > > In a sense, this appears to be a case of the midlayer antipattern: > > > > "The core thesis of the "midlayer mistake" is that midlayers are > > bad and should not exist. That common functionality which it is > > so tempting to put in a midlayer should instead be provided as > > library routines which can [be] used, augmented, or ignored by > > each bottom level driver independently. Thus every subsystem > > that supports multiple implementations (or drivers) should > > provide a very thin top layer which calls directly into the > > bottom layer drivers, and a rich library of support code that > > eases the implementation of those drivers. This library is > > available to, but not forced upon, those drivers." > > -- Neil Brown (2009), https://lwn.net/Articles/336262/ > > > > The presence of midlayer traits in the PCI hotplug core might be ascribed > > to its age: When it was introduced in February 2002, the blessings of a > > library approach might not have been well known: > > https://git.kernel.org/tglx/history/c/a8a2069f432c > > > > For comparison, the driver core does offer split functions for creating > > a kobject (device_initialize()) and addition to sysfs (device_add()) as > > an alternative to carrying out everything at once (device_register()). > > This was introduced in October 2002: > > https://git.kernel.org/tglx/history/c/8b290eb19962 > > > > The odd ->release callback in the PCI hotplug core was added in 2003: > > https://git.kernel.org/tglx/history/c/69f8d663b595 > > > > Clearly, a library approach would not force every hotplug driver to > > implement a ->release callback, but rather allow the driver to remove > > the sysfs files, release its data structures and finally destroy the > > kobject. Alternatively, a driver may choose to remove everything with > > pci_hp_deregister(), then release its data structures. > > > > To this end, offer drivers pci_hp_initialize() and pci_hp_add() as a > > split-up version of pci_hp_register(). Likewise, offer pci_hp_del() > > and pci_hp_destroy() as a split-up version of pci_hp_deregister(). > > > > Eliminate the ->release callback and move its code into each driver's > > teardown routine. > > > > Declare pci_hp_deregister() void, in keeping with the usual kernel > > pattern that enablement can fail, but disablement cannot. It only > > returned an error if the caller passed in a NULL pointer or a slot which > > has never or is no longer registered or is sharing its name with another > > slot. Those would be bugs, so WARN about them. Few hotplug drivers > > actually checked the return value and those that did only printed a > > useless error message to dmesg. Remove that. > > > > For most drivers the conversion was straightforward since it doesn't > > matter whether the code in the ->release callback is executed before or > > after destruction of the kobject. But in the case of ibmphp, it was > > unclear to me whether setting slot_cur->ctrl and slot_cur->bus_on to > > NULL needs to happen before the kobject is destroyed, so I erred on > > the side of caution and ensured that the order stays the same. Another > > nontrivial case is pnv_php, I've found the list and kref logic difficult > > to understand, however my impression was that it is safe to delete the > > list element and drop the references until after the kobject is > > destroyed. > > > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > > Cc: Len Brown <lenb@kernel.org> > > Cc: Scott Murray <scott@spiteful.org> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Cc: Paul Mackerras <paulus@samba.org> > > Cc: Michael Ellerman <mpe@ellerman.id.au> > > Cc: Gavin Shan <gwshan@linux.vnet.ibm.com> > > Cc: Sebastian Ott <sebott@linux.vnet.ibm.com> > > Cc: Gerald Schaefer <gerald.schaefer@de.ibm.com> > > Cc: Corentin Chary <corentin.chary@gmail.com> > > Cc: Darren Hart <dvhart@infradead.org> > > Cc: Andy Shevchenko <andy@infradead.org> > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > --- > > drivers/pci/hotplug/acpiphp_core.c | 22 +--- > > drivers/pci/hotplug/cpci_hotplug_core.c | 14 +-- > > drivers/pci/hotplug/cpqphp_core.c | 16 +-- > > drivers/pci/hotplug/ibmphp_core.c | 15 ++- > > drivers/pci/hotplug/ibmphp_ebda.c | 20 ---- > > drivers/pci/hotplug/pci_hotplug_core.c | 139 +++++++++++++++++------- > > drivers/pci/hotplug/pciehp_core.c | 19 +--- > > drivers/pci/hotplug/pcihp_skeleton.c | 16 +-- > > drivers/pci/hotplug/pnv_php.c | 5 +- > > drivers/pci/hotplug/rpaphp_core.c | 2 +- > > drivers/pci/hotplug/rpaphp_slot.c | 13 +-- > > drivers/pci/hotplug/s390_pci_hpc.c | 13 +-- > > drivers/pci/hotplug/sgi_hotplug.c | 9 +- > > drivers/pci/hotplug/shpchp_core.c | 20 +--- > > > drivers/platform/x86/asus-wmi.c | 12 +- > > drivers/platform/x86/eeepc-laptop.c | 12 +- > > So, as far as I can see these are just mechanical changes due to > internal API change. > Thus, > > Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > for PDx86 bits only. Hi Andy, I'm not sure what you intend by "PDx86". I'm happy to add a note so your ack looks like Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> # drivers/platform/x86 or something similar. Is that what you have in mind? > > include/linux/pci_hotplug.h | 15 ++- > > 17 files changed, 171 insertions(+), 191 deletions(-)
On Mon, Jul 16, 2018 at 3:46 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Sun, Jun 17, 2018 at 07:44:03PM +0300, Andy Shevchenko wrote: >> On Sat, Jun 16, 2018 at 10:37 PM Lukas Wunner <lukas@wunner.de> wrote: >> Thus, >> >> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> >> for PDx86 bits only. > > Hi Andy, I'm not sure what you intend by "PDx86". I'm happy to add a note > so your ack looks like > > Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> # drivers/platform/x86 > > or something similar. Is that what you have in mind? Ah, sorry, not everyone gets that abbreviation. It stands for Platform Drivers x86. What you crafted above is quite good fit to what I meant. Thanks!
diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c index 12b5655fd390..ad32ffbc4b91 100644 --- a/drivers/pci/hotplug/acpiphp_core.c +++ b/drivers/pci/hotplug/acpiphp_core.c @@ -254,20 +254,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value) return 0; } -/** - * release_slot - free up the memory used by a slot - * @hotplug_slot: slot to free - */ -static void release_slot(struct hotplug_slot *hotplug_slot) -{ - struct slot *slot = hotplug_slot->private; - - pr_debug("%s - physical_slot = %s\n", __func__, slot_name(slot)); - - kfree(slot->hotplug_slot); - kfree(slot); -} - /* callback routine to initialize 'struct slot' for each slot */ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot, unsigned int sun) @@ -287,7 +273,6 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot, slot->hotplug_slot->info = &slot->info; slot->hotplug_slot->private = slot; - slot->hotplug_slot->release = &release_slot; slot->hotplug_slot->ops = &acpi_hotplug_slot_ops; slot->acpi_slot = acpiphp_slot; @@ -324,13 +309,12 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot, void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot) { struct slot *slot = acpiphp_slot->slot; - int retval = 0; pr_info("Slot [%s] unregistered\n", slot_name(slot)); - retval = pci_hp_deregister(slot->hotplug_slot); - if (retval) - pr_err("pci_hp_deregister failed with error %d\n", retval); + pci_hp_deregister(slot->hotplug_slot); + kfree(slot->hotplug_slot); + kfree(slot); } diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c index 07b533adc9df..52a339baf06c 100644 --- a/drivers/pci/hotplug/cpci_hotplug_core.c +++ b/drivers/pci/hotplug/cpci_hotplug_core.c @@ -195,10 +195,8 @@ get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value) return 0; } -static void release_slot(struct hotplug_slot *hotplug_slot) +static void release_slot(struct slot *slot) { - struct slot *slot = hotplug_slot->private; - kfree(slot->hotplug_slot->info); kfree(slot->hotplug_slot); pci_dev_put(slot->dev); @@ -253,7 +251,6 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last) snprintf(name, SLOT_NAME_SIZE, "%02x:%02x", bus->number, i); hotplug_slot->private = slot; - hotplug_slot->release = &release_slot; hotplug_slot->ops = &cpci_hotplug_slot_ops; /* @@ -308,12 +305,8 @@ cpci_hp_unregister_bus(struct pci_bus *bus) slots--; dbg("deregistering slot %s", slot_name(slot)); - status = pci_hp_deregister(slot->hotplug_slot); - if (status) { - err("pci_hp_deregister failed with error %d", - status); - break; - } + pci_hp_deregister(slot->hotplug_slot); + release_slot(slot); } } up_write(&list_rwsem); @@ -623,6 +616,7 @@ cleanup_slots(void) list_for_each_entry_safe(slot, tmp, &slot_list, slot_list) { list_del(&slot->slot_list); pci_hp_deregister(slot->hotplug_slot); + release_slot(slot); } cleanup_null: up_write(&list_rwsem); diff --git a/drivers/pci/hotplug/cpqphp_core.c b/drivers/pci/hotplug/cpqphp_core.c index 1797e36ec586..5a06636e910a 100644 --- a/drivers/pci/hotplug/cpqphp_core.c +++ b/drivers/pci/hotplug/cpqphp_core.c @@ -266,17 +266,6 @@ static void __iomem *get_SMBIOS_entry(void __iomem *smbios_start, return previous; } -static void release_slot(struct hotplug_slot *hotplug_slot) -{ - struct slot *slot = hotplug_slot->private; - - dbg("%s - physical_slot = %s\n", __func__, slot_name(slot)); - - kfree(slot->hotplug_slot->info); - kfree(slot->hotplug_slot); - kfree(slot); -} - static int ctrl_slot_cleanup(struct controller *ctrl) { struct slot *old_slot, *next_slot; @@ -285,9 +274,11 @@ static int ctrl_slot_cleanup(struct controller *ctrl) ctrl->slot = NULL; while (old_slot) { - /* memory will be freed by the release_slot callback */ next_slot = old_slot->next; pci_hp_deregister(old_slot->hotplug_slot); + kfree(old_slot->hotplug_slot->info); + kfree(old_slot->hotplug_slot); + kfree(old_slot); old_slot = next_slot; } @@ -678,7 +669,6 @@ static int ctrl_slot_setup(struct controller *ctrl, ((read_slot_enable(ctrl) << 2) >> ctrl_slot) & 0x04; /* register this slot with the hotplug pci core */ - hotplug_slot->release = &release_slot; hotplug_slot->private = slot; snprintf(name, SLOT_NAME_SIZE, "%u", slot->number); hotplug_slot->ops = &cpqphp_hotplug_slot_ops; diff --git a/drivers/pci/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c index 1869b0411ce0..4ea57e9019f1 100644 --- a/drivers/pci/hotplug/ibmphp_core.c +++ b/drivers/pci/hotplug/ibmphp_core.c @@ -673,7 +673,20 @@ static void free_slots(void) list_for_each_entry_safe(slot_cur, next, &ibmphp_slot_head, ibm_slot_list) { - pci_hp_deregister(slot_cur->hotplug_slot); + pci_hp_del(slot_cur->hotplug_slot); + slot_cur->ctrl = NULL; + slot_cur->bus_on = NULL; + + /* + * We don't want to actually remove the resources, + * since ibmphp_free_resources() will do just that. + */ + ibmphp_unconfigure_card(&slot_cur, -1); + + pci_hp_destroy(slot_cur->hotplug_slot); + kfree(slot_cur->hotplug_slot->info); + kfree(slot_cur->hotplug_slot); + kfree(slot_cur); } debug("%s -- exit\n", __func__); } diff --git a/drivers/pci/hotplug/ibmphp_ebda.c b/drivers/pci/hotplug/ibmphp_ebda.c index 64549aa24c0f..6f8e90e3ec08 100644 --- a/drivers/pci/hotplug/ibmphp_ebda.c +++ b/drivers/pci/hotplug/ibmphp_ebda.c @@ -699,25 +699,6 @@ static int fillslotinfo(struct hotplug_slot *hotplug_slot) return rc; } -static void release_slot(struct hotplug_slot *hotplug_slot) -{ - struct slot *slot; - - if (!hotplug_slot || !hotplug_slot->private) - return; - - slot = hotplug_slot->private; - kfree(slot->hotplug_slot->info); - kfree(slot->hotplug_slot); - slot->ctrl = NULL; - slot->bus_on = NULL; - - /* we don't want to actually remove the resources, since free_resources will do just that */ - ibmphp_unconfigure_card(&slot, -1); - - kfree(slot); -} - static struct pci_driver ibmphp_driver; /* @@ -941,7 +922,6 @@ static int __init ebda_rsrc_controller(void) tmp_slot->hotplug_slot = hp_slot_ptr; hp_slot_ptr->private = tmp_slot; - hp_slot_ptr->release = release_slot; rc = fillslotinfo(hp_slot_ptr); if (rc) diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c index fd93783a87b0..90fde5f106d8 100644 --- a/drivers/pci/hotplug/pci_hotplug_core.c +++ b/drivers/pci/hotplug/pci_hotplug_core.c @@ -396,8 +396,9 @@ static struct hotplug_slot *get_slot_from_name(const char *name) * @owner: caller module owner * @mod_name: caller module name * - * Registers a hotplug slot with the pci hotplug subsystem, which will allow - * userspace interaction to the slot. + * Prepares a hotplug slot for in-kernel use and immediately publishes it to + * user space in one go. Drivers may alternatively carry out the two steps + * separately by invoking pci_hp_initialize() and pci_hp_add(). * * Returns 0 if successful, anything else for an error. */ @@ -406,54 +407,91 @@ int __pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, struct module *owner, const char *mod_name) { int result; + + result = __pci_hp_initialize(slot, bus, devnr, name, owner, mod_name); + if (result) + return result; + + result = pci_hp_add(slot); + if (result) + pci_hp_destroy(slot); + + return result; +} +EXPORT_SYMBOL_GPL(__pci_hp_register); + +/** + * __pci_hp_initialize - prepare hotplug slot for in-kernel use + * @slot: pointer to the &struct hotplug_slot to initialize + * @bus: bus this slot is on + * @devnr: slot number + * @name: name registered with kobject core + * @owner: caller module owner + * @mod_name: caller module name + * + * Allocate and fill in a PCI slot for use by a hotplug driver. Once this has + * been called, the driver may invoke hotplug_slot_name() to get the slot's + * unique name. The driver must be prepared to handle a ->reset_slot callback + * from this point on. + * + * Returns 0 on success or a negative int on error. + */ +int __pci_hp_initialize(struct hotplug_slot *slot, struct pci_bus *bus, + int devnr, const char *name, struct module *owner, + const char *mod_name) +{ struct pci_slot *pci_slot; if (slot == NULL) return -ENODEV; if ((slot->info == NULL) || (slot->ops == NULL)) return -EINVAL; - if (slot->release == NULL) { - dbg("Why are you trying to register a hotplug slot without a proper release function?\n"); - return -EINVAL; - } slot->ops->owner = owner; slot->ops->mod_name = mod_name; - mutex_lock(&pci_hp_mutex); /* * No problems if we call this interface from both ACPI_PCI_SLOT * driver and call it here again. If we've already created the * pci_slot, the interface will simply bump the refcount. */ pci_slot = pci_create_slot(bus, devnr, name, slot); - if (IS_ERR(pci_slot)) { - result = PTR_ERR(pci_slot); - goto out; - } + if (IS_ERR(pci_slot)) + return PTR_ERR(pci_slot); slot->pci_slot = pci_slot; pci_slot->hotplug = slot; + return 0; +} +EXPORT_SYMBOL_GPL(__pci_hp_initialize); - list_add(&slot->slot_list, &pci_hotplug_slot_list); +/** + * pci_hp_add - publish hotplug slot to user space + * @slot: pointer to the &struct hotplug_slot to publish + * + * Make a hotplug slot's sysfs interface available and inform user space of its + * addition by sending a uevent. The hotplug driver must be prepared to handle + * all &struct hotplug_slot_ops callbacks from this point on. + * + * Returns 0 on success or a negative int on error. + */ +int pci_hp_add(struct hotplug_slot *slot) +{ + struct pci_slot *pci_slot = slot->pci_slot; + int result; result = fs_add_slot(pci_slot); if (result) - goto err_list_del; + return result; kobject_uevent(&pci_slot->kobj, KOBJ_ADD); - dbg("Added slot %s to the list\n", name); - goto out; - -err_list_del: - list_del(&slot->slot_list); - pci_slot->hotplug = NULL; - pci_destroy_slot(pci_slot); -out: + mutex_lock(&pci_hp_mutex); + list_add(&slot->slot_list, &pci_hotplug_slot_list); mutex_unlock(&pci_hp_mutex); - return result; + dbg("Added slot %s to the list\n", hotplug_slot_name(slot)); + return 0; } -EXPORT_SYMBOL_GPL(__pci_hp_register); +EXPORT_SYMBOL_GPL(pci_hp_add); /** * pci_hp_deregister - deregister a hotplug_slot with the PCI hotplug subsystem @@ -464,35 +502,62 @@ EXPORT_SYMBOL_GPL(__pci_hp_register); * * Returns 0 if successful, anything else for an error. */ -int pci_hp_deregister(struct hotplug_slot *slot) +void pci_hp_deregister(struct hotplug_slot *slot) +{ + pci_hp_del(slot); + pci_hp_destroy(slot); +} +EXPORT_SYMBOL_GPL(pci_hp_deregister); + +/** + * pci_hp_del - unpublish hotplug slot from user space + * @slot: pointer to the &struct hotplug_slot to unpublish + * + * Remove a hotplug slot's sysfs interface. + * + * Returns 0 on success or a negative int on error. + */ +void pci_hp_del(struct hotplug_slot *slot) { struct hotplug_slot *temp; - struct pci_slot *pci_slot; - if (!slot) - return -ENODEV; + if (WARN_ON(!slot)) + return; mutex_lock(&pci_hp_mutex); temp = get_slot_from_name(hotplug_slot_name(slot)); - if (temp != slot) { + if (WARN_ON(temp != slot)) { mutex_unlock(&pci_hp_mutex); - return -ENODEV; + return; } list_del(&slot->slot_list); - - pci_slot = slot->pci_slot; - fs_remove_slot(pci_slot); + mutex_unlock(&pci_hp_mutex); dbg("Removed slot %s from the list\n", hotplug_slot_name(slot)); + fs_remove_slot(slot->pci_slot); +} +EXPORT_SYMBOL_GPL(pci_hp_del); + +/** + * pci_hp_destroy - remove hotplug slot from in-kernel use + * @slot: pointer to the &struct hotplug_slot to destroy + * + * Destroy a PCI slot used by a hotplug driver. Once this has been called, + * the driver may no longer invoke hotplug_slot_name() to get the slot's + * unique name. The driver no longer needs to handle a ->reset_slot callback + * from this point on. + * + * Returns 0 on success or a negative int on error. + */ +void pci_hp_destroy(struct hotplug_slot *slot) +{ + struct pci_slot *pci_slot = slot->pci_slot; - slot->release(slot); + slot->pci_slot = NULL; pci_slot->hotplug = NULL; pci_destroy_slot(pci_slot); - mutex_unlock(&pci_hp_mutex); - - return 0; } -EXPORT_SYMBOL_GPL(pci_hp_deregister); +EXPORT_SYMBOL_GPL(pci_hp_destroy); /** * pci_hp_change_slot_info - changes the slot's information structure in the core diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c index b360645377c2..37d8f81e548f 100644 --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -56,17 +56,6 @@ static int get_latch_status(struct hotplug_slot *slot, u8 *value); static int get_adapter_status(struct hotplug_slot *slot, u8 *value); static int reset_slot(struct hotplug_slot *slot, int probe); -/** - * release_slot - free up the memory used by a slot - * @hotplug_slot: slot to free - */ -static void release_slot(struct hotplug_slot *hotplug_slot) -{ - kfree(hotplug_slot->ops); - kfree(hotplug_slot->info); - kfree(hotplug_slot); -} - static int init_slot(struct controller *ctrl) { struct slot *slot = ctrl->slot; @@ -107,7 +96,6 @@ static int init_slot(struct controller *ctrl) /* register this slot with the hotplug pci core */ hotplug->info = info; hotplug->private = slot; - hotplug->release = &release_slot; hotplug->ops = ops; slot->hotplug_slot = hotplug; snprintf(name, SLOT_NAME_SIZE, "%u", PSN(ctrl)); @@ -127,7 +115,12 @@ static int init_slot(struct controller *ctrl) static void cleanup_slot(struct controller *ctrl) { - pci_hp_deregister(ctrl->slot->hotplug_slot); + struct hotplug_slot *hotplug_slot = ctrl->slot->hotplug_slot; + + pci_hp_deregister(hotplug_slot); + kfree(hotplug_slot->ops); + kfree(hotplug_slot->info); + kfree(hotplug_slot); } /* diff --git a/drivers/pci/hotplug/pcihp_skeleton.c b/drivers/pci/hotplug/pcihp_skeleton.c index c19694a04d2c..94ad7cd72878 100644 --- a/drivers/pci/hotplug/pcihp_skeleton.c +++ b/drivers/pci/hotplug/pcihp_skeleton.c @@ -210,16 +210,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value) return retval; } -static void release_slot(struct hotplug_slot *hotplug_slot) -{ - struct slot *slot = hotplug_slot->private; - - dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name); - kfree(slot->hotplug_slot->info); - kfree(slot->hotplug_slot); - kfree(slot); -} - static void make_slot_name(struct slot *slot) { /* @@ -270,7 +260,6 @@ static int __init init_slots(void) hotplug_slot->name = slot->name; hotplug_slot->private = slot; - hotplug_slot->release = &release_slot; make_slot_name(slot); hotplug_slot->ops = &skel_hotplug_slot_ops; @@ -311,12 +300,13 @@ static void __exit cleanup_slots(void) /* * Unregister all of our slots with the pci_hotplug subsystem. - * Memory will be freed in release_slot() callback after slot's - * lifespan is finished. */ list_for_each_entry_safe(slot, next, &slot_list, slot_list) { list_del(&slot->slot_list); pci_hp_deregister(slot->hotplug_slot); + kfree(slot->hotplug_slot->info); + kfree(slot->hotplug_slot); + kfree(slot); } } diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c index 6c2e8d7307c6..3276a5e4c430 100644 --- a/drivers/pci/hotplug/pnv_php.c +++ b/drivers/pci/hotplug/pnv_php.c @@ -538,9 +538,8 @@ static struct hotplug_slot_ops php_slot_ops = { .disable_slot = pnv_php_disable_slot, }; -static void pnv_php_release(struct hotplug_slot *slot) +static void pnv_php_release(struct pnv_php_slot *php_slot) { - struct pnv_php_slot *php_slot = slot->private; unsigned long flags; /* Remove from global or child list */ @@ -596,7 +595,6 @@ static struct pnv_php_slot *pnv_php_alloc_slot(struct device_node *dn) php_slot->power_state_check = false; php_slot->slot.ops = &php_slot_ops; php_slot->slot.info = &php_slot->slot_info; - php_slot->slot.release = pnv_php_release; php_slot->slot.private = php_slot; INIT_LIST_HEAD(&php_slot->children); @@ -924,6 +922,7 @@ static void pnv_php_unregister_one(struct device_node *dn) php_slot->state = PNV_PHP_STATE_OFFLINE; pci_hp_deregister(&php_slot->slot); + pnv_php_release(php_slot); pnv_php_put_slot(php_slot); } diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c index fb5e0845429d..857c358b727b 100644 --- a/drivers/pci/hotplug/rpaphp_core.c +++ b/drivers/pci/hotplug/rpaphp_core.c @@ -404,13 +404,13 @@ static void __exit cleanup_slots(void) /* * Unregister all of our slots with the pci_hotplug subsystem, * and free up all memory that we had allocated. - * memory will be freed in release_slot callback. */ list_for_each_entry_safe(slot, next, &rpaphp_slot_head, rpaphp_slot_list) { list_del(&slot->rpaphp_slot_list); pci_hp_deregister(slot->hotplug_slot); + dealloc_slot_struct(slot); } return; } diff --git a/drivers/pci/hotplug/rpaphp_slot.c b/drivers/pci/hotplug/rpaphp_slot.c index 3840a2075e6a..b916c8e4372d 100644 --- a/drivers/pci/hotplug/rpaphp_slot.c +++ b/drivers/pci/hotplug/rpaphp_slot.c @@ -19,12 +19,6 @@ #include "rpaphp.h" /* free up the memory used by a slot */ -static void rpaphp_release_slot(struct hotplug_slot *hotplug_slot) -{ - struct slot *slot = (struct slot *) hotplug_slot->private; - dealloc_slot_struct(slot); -} - void dealloc_slot_struct(struct slot *slot) { kfree(slot->hotplug_slot->info); @@ -56,7 +50,6 @@ struct slot *alloc_slot_struct(struct device_node *dn, slot->power_domain = power_domain; slot->hotplug_slot->private = slot; slot->hotplug_slot->ops = &rpaphp_hotplug_slot_ops; - slot->hotplug_slot->release = &rpaphp_release_slot; return (slot); @@ -90,10 +83,8 @@ int rpaphp_deregister_slot(struct slot *slot) __func__, slot->name); list_del(&slot->rpaphp_slot_list); - - retval = pci_hp_deregister(php_slot); - if (retval) - err("Problem unregistering a slot %s\n", slot->name); + pci_hp_deregister(php_slot); + dealloc_slot_struct(slot); dbg("%s - Exit: rc[%d]\n", __func__, retval); return retval; diff --git a/drivers/pci/hotplug/s390_pci_hpc.c b/drivers/pci/hotplug/s390_pci_hpc.c index ffdc2977395d..93b5341d282c 100644 --- a/drivers/pci/hotplug/s390_pci_hpc.c +++ b/drivers/pci/hotplug/s390_pci_hpc.c @@ -130,15 +130,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value) return 0; } -static void release_slot(struct hotplug_slot *hotplug_slot) -{ - struct slot *slot = hotplug_slot->private; - - kfree(slot->hotplug_slot->info); - kfree(slot->hotplug_slot); - kfree(slot); -} - static struct hotplug_slot_ops s390_hotplug_slot_ops = { .enable_slot = enable_slot, .disable_slot = disable_slot, @@ -175,7 +166,6 @@ int zpci_init_slot(struct zpci_dev *zdev) hotplug_slot->info = info; hotplug_slot->ops = &s390_hotplug_slot_ops; - hotplug_slot->release = &release_slot; get_power_status(hotplug_slot, &info->power_status); get_adapter_status(hotplug_slot, &info->adapter_status); @@ -209,5 +199,8 @@ void zpci_exit_slot(struct zpci_dev *zdev) continue; list_del(&slot->slot_list); pci_hp_deregister(slot->hotplug_slot); + kfree(slot->hotplug_slot->info); + kfree(slot->hotplug_slot); + kfree(slot); } } diff --git a/drivers/pci/hotplug/sgi_hotplug.c b/drivers/pci/hotplug/sgi_hotplug.c index 78b6bdbb3a39..babd23409f61 100644 --- a/drivers/pci/hotplug/sgi_hotplug.c +++ b/drivers/pci/hotplug/sgi_hotplug.c @@ -628,7 +628,6 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus) goto alloc_err; } bss_hotplug_slot->ops = &sn_hotplug_slot_ops; - bss_hotplug_slot->release = &sn_release_slot; rc = pci_hp_register(bss_hotplug_slot, pci_bus, device, name); if (rc) @@ -656,8 +655,10 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus) sn_release_slot(bss_hotplug_slot); /* destroy anything else on the list */ - while ((bss_hotplug_slot = sn_hp_destroy())) + while ((bss_hotplug_slot = sn_hp_destroy())) { pci_hp_deregister(bss_hotplug_slot); + sn_release_slot(bss_hotplug_slot); + } return rc; } @@ -703,8 +704,10 @@ static void __exit sn_pci_hotplug_exit(void) { struct hotplug_slot *bss_hotplug_slot; - while ((bss_hotplug_slot = sn_hp_destroy())) + while ((bss_hotplug_slot = sn_hp_destroy())) { pci_hp_deregister(bss_hotplug_slot); + sn_release_slot(bss_hotplug_slot); + } if (!list_empty(&sn_hp_list)) printk(KERN_ERR "%s: internal list is not empty\n", __FILE__); diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c index e91be287f292..5990695d1737 100644 --- a/drivers/pci/hotplug/shpchp_core.c +++ b/drivers/pci/hotplug/shpchp_core.c @@ -61,22 +61,6 @@ static struct hotplug_slot_ops shpchp_hotplug_slot_ops = { .get_adapter_status = get_adapter_status, }; -/** - * release_slot - free up the memory used by a slot - * @hotplug_slot: slot to free - */ -static void release_slot(struct hotplug_slot *hotplug_slot) -{ - struct slot *slot = hotplug_slot->private; - - ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n", - __func__, slot_name(slot)); - - kfree(slot->hotplug_slot->info); - kfree(slot->hotplug_slot); - kfree(slot); -} - static int init_slots(struct controller *ctrl) { struct slot *slot; @@ -125,7 +109,6 @@ static int init_slots(struct controller *ctrl) /* register this slot with the hotplug pci core */ hotplug_slot->private = slot; - hotplug_slot->release = &release_slot; snprintf(name, SLOT_NAME_SIZE, "%d", slot->number); hotplug_slot->ops = &shpchp_hotplug_slot_ops; @@ -171,6 +154,9 @@ void cleanup_slots(struct controller *ctrl) cancel_delayed_work(&slot->work); destroy_workqueue(slot->wq); pci_hp_deregister(slot->hotplug_slot); + kfree(slot->hotplug_slot->info); + kfree(slot->hotplug_slot); + kfree(slot); } } diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 3d523ca64694..d67f32a29bb4 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -858,12 +858,6 @@ static int asus_get_adapter_status(struct hotplug_slot *hotplug_slot, return 0; } -static void asus_cleanup_pci_hotplug(struct hotplug_slot *hotplug_slot) -{ - kfree(hotplug_slot->info); - kfree(hotplug_slot); -} - static struct hotplug_slot_ops asus_hotplug_slot_ops = { .owner = THIS_MODULE, .get_adapter_status = asus_get_adapter_status, @@ -905,7 +899,6 @@ static int asus_setup_pci_hotplug(struct asus_wmi *asus) goto error_info; asus->hotplug_slot->private = asus; - asus->hotplug_slot->release = &asus_cleanup_pci_hotplug; asus->hotplug_slot->ops = &asus_hotplug_slot_ops; asus_get_adapter_status(asus->hotplug_slot, &asus->hotplug_slot->info->adapter_status); @@ -1051,8 +1044,11 @@ static void asus_wmi_rfkill_exit(struct asus_wmi *asus) * asus_unregister_rfkill_notifier() */ asus_rfkill_hotplug(asus); - if (asus->hotplug_slot) + if (asus->hotplug_slot) { pci_hp_deregister(asus->hotplug_slot); + kfree(asus->hotplug_slot->info); + kfree(asus->hotplug_slot); + } if (asus->hotplug_workqueue) destroy_workqueue(asus->hotplug_workqueue); diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c index 4c38904a8a32..a4bbf6ecd1f0 100644 --- a/drivers/platform/x86/eeepc-laptop.c +++ b/drivers/platform/x86/eeepc-laptop.c @@ -726,12 +726,6 @@ static int eeepc_get_adapter_status(struct hotplug_slot *hotplug_slot, return 0; } -static void eeepc_cleanup_pci_hotplug(struct hotplug_slot *hotplug_slot) -{ - kfree(hotplug_slot->info); - kfree(hotplug_slot); -} - static struct hotplug_slot_ops eeepc_hotplug_slot_ops = { .owner = THIS_MODULE, .get_adapter_status = eeepc_get_adapter_status, @@ -758,7 +752,6 @@ static int eeepc_setup_pci_hotplug(struct eeepc_laptop *eeepc) goto error_info; eeepc->hotplug_slot->private = eeepc; - eeepc->hotplug_slot->release = &eeepc_cleanup_pci_hotplug; eeepc->hotplug_slot->ops = &eeepc_hotplug_slot_ops; eeepc_get_adapter_status(eeepc->hotplug_slot, &eeepc->hotplug_slot->info->adapter_status); @@ -837,8 +830,11 @@ static void eeepc_rfkill_exit(struct eeepc_laptop *eeepc) eeepc->wlan_rfkill = NULL; } - if (eeepc->hotplug_slot) + if (eeepc->hotplug_slot) { pci_hp_deregister(eeepc->hotplug_slot); + kfree(eeepc->hotplug_slot->info); + kfree(eeepc->hotplug_slot); + } if (eeepc->bluetooth_rfkill) { rfkill_unregister(eeepc->bluetooth_rfkill); diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h index cf5e22103f68..a6d6650a0490 100644 --- a/include/linux/pci_hotplug.h +++ b/include/linux/pci_hotplug.h @@ -80,15 +80,12 @@ struct hotplug_slot_info { * @ops: pointer to the &struct hotplug_slot_ops to be used for this slot * @info: pointer to the &struct hotplug_slot_info for the initial values for * this slot. - * @release: called during pci_hp_deregister to free memory allocated in a - * hotplug_slot structure. * @private: used by the hotplug pci controller driver to store whatever it * needs. */ struct hotplug_slot { struct hotplug_slot_ops *ops; struct hotplug_slot_info *info; - void (*release) (struct hotplug_slot *slot); void *private; /* Variables below this are for use only by the hotplug pci core. */ @@ -104,13 +101,23 @@ static inline const char *hotplug_slot_name(const struct hotplug_slot *slot) int __pci_hp_register(struct hotplug_slot *slot, struct pci_bus *pbus, int nr, const char *name, struct module *owner, const char *mod_name); -int pci_hp_deregister(struct hotplug_slot *slot); +int __pci_hp_initialize(struct hotplug_slot *slot, struct pci_bus *bus, int nr, + const char *name, struct module *owner, + const char *mod_name); +int pci_hp_add(struct hotplug_slot *slot); + +void pci_hp_del(struct hotplug_slot *slot); +void pci_hp_destroy(struct hotplug_slot *slot); +void pci_hp_deregister(struct hotplug_slot *slot); + int __must_check pci_hp_change_slot_info(struct hotplug_slot *slot, struct hotplug_slot_info *info); /* use a define to avoid include chaining to get THIS_MODULE & friends */ #define pci_hp_register(slot, pbus, devnr, name) \ __pci_hp_register(slot, pbus, devnr, name, THIS_MODULE, KBUILD_MODNAME) +#define pci_hp_initialize(slot, bus, nr, name) \ + __pci_hp_initialize(slot, bus, nr, name, THIS_MODULE, KBUILD_MODNAME) /* PCI Setting Record (Type 0) */ struct hpp_type0 {
When a hotplug driver calls pci_hp_register(), all steps necessary for registration are carried out in one go, including creation of a kobject and addition to sysfs. That's a problem for pciehp once it's converted to enable/disable the slot exclusively from the IRQ thread: The thread needs to be spawned after creation of the kobject (because it uses the kobject's name), but before addition to sysfs (because it will handle enable/disable requests submitted via sysfs). pci_hp_deregister() does offer a ->release callback that's invoked after deletion from sysfs and before destruction of the kobject. But because pci_hp_register() doesn't offer a counterpart, hotplug drivers' ->probe and ->remove code becomes asymmetric, which is error prone as recently discovered use-after-free bugs in pciehp's ->remove hook have shown. In a sense, this appears to be a case of the midlayer antipattern: "The core thesis of the "midlayer mistake" is that midlayers are bad and should not exist. That common functionality which it is so tempting to put in a midlayer should instead be provided as library routines which can [be] used, augmented, or ignored by each bottom level driver independently. Thus every subsystem that supports multiple implementations (or drivers) should provide a very thin top layer which calls directly into the bottom layer drivers, and a rich library of support code that eases the implementation of those drivers. This library is available to, but not forced upon, those drivers." -- Neil Brown (2009), https://lwn.net/Articles/336262/ The presence of midlayer traits in the PCI hotplug core might be ascribed to its age: When it was introduced in February 2002, the blessings of a library approach might not have been well known: https://git.kernel.org/tglx/history/c/a8a2069f432c For comparison, the driver core does offer split functions for creating a kobject (device_initialize()) and addition to sysfs (device_add()) as an alternative to carrying out everything at once (device_register()). This was introduced in October 2002: https://git.kernel.org/tglx/history/c/8b290eb19962 The odd ->release callback in the PCI hotplug core was added in 2003: https://git.kernel.org/tglx/history/c/69f8d663b595 Clearly, a library approach would not force every hotplug driver to implement a ->release callback, but rather allow the driver to remove the sysfs files, release its data structures and finally destroy the kobject. Alternatively, a driver may choose to remove everything with pci_hp_deregister(), then release its data structures. To this end, offer drivers pci_hp_initialize() and pci_hp_add() as a split-up version of pci_hp_register(). Likewise, offer pci_hp_del() and pci_hp_destroy() as a split-up version of pci_hp_deregister(). Eliminate the ->release callback and move its code into each driver's teardown routine. Declare pci_hp_deregister() void, in keeping with the usual kernel pattern that enablement can fail, but disablement cannot. It only returned an error if the caller passed in a NULL pointer or a slot which has never or is no longer registered or is sharing its name with another slot. Those would be bugs, so WARN about them. Few hotplug drivers actually checked the return value and those that did only printed a useless error message to dmesg. Remove that. For most drivers the conversion was straightforward since it doesn't matter whether the code in the ->release callback is executed before or after destruction of the kobject. But in the case of ibmphp, it was unclear to me whether setting slot_cur->ctrl and slot_cur->bus_on to NULL needs to happen before the kobject is destroyed, so I erred on the side of caution and ensured that the order stays the same. Another nontrivial case is pnv_php, I've found the list and kref logic difficult to understand, however my impression was that it is safe to delete the list element and drop the references until after the kobject is destroyed. Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Len Brown <lenb@kernel.org> Cc: Scott Murray <scott@spiteful.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Paul Mackerras <paulus@samba.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Gavin Shan <gwshan@linux.vnet.ibm.com> Cc: Sebastian Ott <sebott@linux.vnet.ibm.com> Cc: Gerald Schaefer <gerald.schaefer@de.ibm.com> Cc: Corentin Chary <corentin.chary@gmail.com> Cc: Darren Hart <dvhart@infradead.org> Cc: Andy Shevchenko <andy@infradead.org> Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/pci/hotplug/acpiphp_core.c | 22 +--- drivers/pci/hotplug/cpci_hotplug_core.c | 14 +-- drivers/pci/hotplug/cpqphp_core.c | 16 +-- drivers/pci/hotplug/ibmphp_core.c | 15 ++- drivers/pci/hotplug/ibmphp_ebda.c | 20 ---- drivers/pci/hotplug/pci_hotplug_core.c | 139 +++++++++++++++++------- drivers/pci/hotplug/pciehp_core.c | 19 +--- drivers/pci/hotplug/pcihp_skeleton.c | 16 +-- drivers/pci/hotplug/pnv_php.c | 5 +- drivers/pci/hotplug/rpaphp_core.c | 2 +- drivers/pci/hotplug/rpaphp_slot.c | 13 +-- drivers/pci/hotplug/s390_pci_hpc.c | 13 +-- drivers/pci/hotplug/sgi_hotplug.c | 9 +- drivers/pci/hotplug/shpchp_core.c | 20 +--- drivers/platform/x86/asus-wmi.c | 12 +- drivers/platform/x86/eeepc-laptop.c | 12 +- include/linux/pci_hotplug.h | 15 ++- 17 files changed, 171 insertions(+), 191 deletions(-)