Re: [PATCH 14/32] PCI: hotplug: Demidlayer registration with the core

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux