On Sat, Jun 16, 2018 at 10:37 PM Lukas Wunner <lukas@xxxxxxxxx> 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@xxxxxxxxxxxxx> > Cc: Len Brown <lenb@xxxxxxxxxx> > Cc: Scott Murray <scott@xxxxxxxxxxxx> > Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> > Cc: Paul Mackerras <paulus@xxxxxxxxx> > Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx> > Cc: Gavin Shan <gwshan@xxxxxxxxxxxxxxxxxx> > Cc: Sebastian Ott <sebott@xxxxxxxxxxxxxxxxxx> > Cc: Gerald Schaefer <gerald.schaefer@xxxxxxxxxx> > Cc: Corentin Chary <corentin.chary@xxxxxxxxx> > Cc: Darren Hart <dvhart@xxxxxxxxxxxxx> > Cc: Andy Shevchenko <andy@xxxxxxxxxxxxx> > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx> > --- > 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@xxxxxxxxx> 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 > -- With Best Regards, Andy Shevchenko
