| Message ID | 5282264.ht9aNfCcNl@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Jul 17, 2013 at 4:17 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > When either a new hotplug bridge or a new hotplug function is added > by the ACPI-based PCI hotplug (ACPIPHP) code, attach a context object > to its ACPI handle to store hotplug-related information in it. To > start with, put the handle's bridge and function pointers into that > object. Count references to the context objects and drop them when > they are not needed any more. > > First of all, this makes it possible to find out if the given bridge > has been registered as a function already in a much more > straightforward way and acpiphp_bridge_handle_to_function() can be > dropped (Yay!). > > This also will allow some more simplifications to be made going > forward. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/pci/hotplug/acpiphp.h | 10 ++ > drivers/pci/hotplug/acpiphp_glue.c | 179 ++++++++++++++++++++++++++++--------- > 2 files changed, 146 insertions(+), 43 deletions(-) no, more lines are added. > > Index: linux-pm/drivers/pci/hotplug/acpiphp.h > =================================================================== > --- linux-pm.orig/drivers/pci/hotplug/acpiphp.h > +++ linux-pm/drivers/pci/hotplug/acpiphp.h > @@ -49,6 +49,7 @@ > #define info(format, arg...) printk(KERN_INFO "%s: " format, MY_NAME , ## arg) > #define warn(format, arg...) printk(KERN_WARNING "%s: " format, MY_NAME , ## arg) > > +struct acpiphp_context; > struct acpiphp_bridge; > struct acpiphp_slot; > > @@ -77,6 +78,7 @@ struct acpiphp_bridge { > struct kref ref; > acpi_handle handle; > > + struct acpiphp_context *context; > /* Ejectable PCI-to-PCI bridge (PCI bridge and PCI function) */ > struct acpiphp_func *func; > > @@ -119,6 +121,7 @@ struct acpiphp_slot { > * typically 8 objects per slot (i.e. for each PCI function) > */ > struct acpiphp_func { > + struct acpiphp_context *context; > struct acpiphp_slot *slot; /* parent */ > > struct list_head sibling; > @@ -128,6 +131,13 @@ struct acpiphp_func { > u32 flags; /* see below */ > }; > > +struct acpiphp_context { > + acpi_handle handle; > + struct acpiphp_func *func; > + struct acpiphp_bridge *bridge; > + unsigned int refcount; > +}; > + > /* > * struct acpiphp_attention_info - device specific attention registration > * > Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c > =================================================================== > --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c > +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c > @@ -55,6 +55,7 @@ > > static LIST_HEAD(bridge_list); > static DEFINE_MUTEX(bridge_mutex); > +static DEFINE_MUTEX(acpiphp_context_lock); > > #define MY_NAME "acpiphp_glue" > > @@ -79,6 +80,74 @@ is_pci_dock_device(acpi_handle handle, u > } > } > > +static void acpiphp_context_handler(acpi_handle handle, void *context) > +{ > + /* Intentionally empty. */ > +} > + > +/** > + * acpiphp_init_context - Create hotplug context and grab a reference to it. > + * @handle: ACPI object handle to create the context for. > + * > + * Call under acpiphp_context_lock. > + */ > +static struct acpiphp_context *acpiphp_init_context(acpi_handle handle) > +{ > + struct acpiphp_context *context; > + acpi_status status; > + > + context = kzalloc(sizeof(*context), GFP_KERNEL); > + if (!context) > + return NULL; > + > + context->handle = handle; > + context->refcount = 1; > + status = acpi_attach_data(handle, acpiphp_context_handler, context); > + if (ACPI_FAILURE(status)) { > + kfree(context); > + return NULL; > + } > + return context; > +} > + > +/** > + * acpiphp_get_context - Get hotplug context and grab a reference to it. > + * @handle: ACPI object handle to get the context for. > + * > + * Call under acpiphp_context_lock. > + */ > +static struct acpiphp_context *acpiphp_get_context(acpi_handle handle) > +{ > + struct acpiphp_context *context = NULL; > + acpi_status status; > + void *data; > + > + status = acpi_get_data(handle, acpiphp_context_handler, &data); > + if (ACPI_SUCCESS(status)) { > + context = data; > + context->refcount++; > + } > + return context; > +} > + > +/** > + * acpiphp_put_context - Drop a reference to ACPI hotplug context. > + * @handle: ACPI object handle to put the context for. > + * > + * The context object is removed if there are no more references to it. > + * > + * Call under acpiphp_context_lock. > + */ > +static void acpiphp_put_context(struct acpiphp_context *context) > +{ > + if (--context->refcount) > + return; > + > + WARN_ON(context->func || context->bridge); > + acpi_detach_data(context->handle, acpiphp_context_handler); > + kfree(context); > +} > + > static inline void get_bridge(struct acpiphp_bridge *bridge) > { > kref_get(&bridge->ref); > @@ -91,25 +160,37 @@ static inline void put_bridge(struct acp > > static void free_bridge(struct kref *kref) > { > + struct acpiphp_context *context; > struct acpiphp_bridge *bridge; > struct acpiphp_slot *slot, *next; > struct acpiphp_func *func, *tmp; > > + mutex_lock(&acpiphp_context_lock); > + > bridge = container_of(kref, struct acpiphp_bridge, ref); > > list_for_each_entry_safe(slot, next, &bridge->slots, node) { > list_for_each_entry_safe(func, tmp, &slot->funcs, sibling) { > + context = func->context; > + context->func = NULL; > + acpiphp_put_context(context); > kfree(func); > } > kfree(slot); > } > > - /* Release reference acquired by acpiphp_bridge_handle_to_function() */ > + /* Release the reference acquired by acpiphp_enumerate_slots(). */ > if ((bridge->flags & BRIDGE_HAS_EJ0) && bridge->func) > put_bridge(bridge->func->slot->bridge); > + > put_device(&bridge->pci_bus->dev); > pci_dev_put(bridge->pci_dev); > + context = bridge->context; > + context->bridge = NULL; > + acpiphp_put_context(context); > kfree(bridge); > + > + mutex_unlock(&acpiphp_context_lock); > } > > /* > @@ -194,10 +275,11 @@ static void acpiphp_dock_release(void *d > } > > /* callback routine to register each ACPI PCI slot object */ > -static acpi_status > -register_slot(acpi_handle handle, u32 lvl, void *context, void **rv) > +static acpi_status register_slot(acpi_handle handle, u32 lvl, void *data, > + void **rv) > { > - struct acpiphp_bridge *bridge = (struct acpiphp_bridge *)context; > + struct acpiphp_bridge *bridge = data; > + struct acpiphp_context *context; > struct acpiphp_slot *slot; > struct acpiphp_func *newfunc; > acpi_status status = AE_OK; > @@ -230,6 +312,18 @@ register_slot(acpi_handle handle, u32 lv > newfunc->handle = handle; > newfunc->function = function; > > + mutex_lock(&acpiphp_context_lock); > + context = acpiphp_init_context(handle); > + if (!context) { > + mutex_unlock(&acpiphp_context_lock); > + acpi_handle_err(handle, "No hotplug context\n"); > + kfree(newfunc); > + return AE_NOT_EXIST; > + } > + newfunc->context = context; > + context->func = newfunc; > + mutex_unlock(&acpiphp_context_lock); > + > if (acpi_has_method(handle, "_EJ0")) > newfunc->flags = FUNC_HAS_EJ0; > > @@ -266,8 +360,8 @@ register_slot(acpi_handle handle, u32 lv > if (!found) { > slot = kzalloc(sizeof(struct acpiphp_slot), GFP_KERNEL); > if (!slot) { > - kfree(newfunc); > - return AE_NO_MEMORY; > + status = AE_NO_MEMORY; > + goto err_out; > } > > slot->bridge = bridge; > @@ -291,7 +385,9 @@ register_slot(acpi_handle handle, u32 lv > else > warn("acpiphp_register_hotplug_slot failed " > "(err code = 0x%x)\n", retval); > - goto err_exit; > + > + status = AE_OK; > + goto err; > } > } > > @@ -329,15 +425,20 @@ register_slot(acpi_handle handle, u32 lv > > return AE_OK; > > - err_exit: > + err: > bridge->nr_slots--; > mutex_lock(&bridge_mutex); > list_del(&slot->node); > mutex_unlock(&bridge_mutex); > kfree(slot); > - kfree(newfunc); > > - return AE_OK; > + err_out: > + mutex_lock(&acpiphp_context_lock); > + context->func = NULL; > + acpiphp_put_context(context); > + mutex_unlock(&acpiphp_context_lock); > + kfree(newfunc); > + return status; > } > > > @@ -352,32 +453,6 @@ static int detect_ejectable_slots(acpi_h > return found; > } > > - > -/* find acpiphp_func from acpiphp_bridge */ > -static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle) > -{ > - struct acpiphp_bridge *bridge; > - struct acpiphp_slot *slot; > - struct acpiphp_func *func = NULL; > - > - mutex_lock(&bridge_mutex); > - list_for_each_entry(bridge, &bridge_list, list) { > - list_for_each_entry(slot, &bridge->slots, node) { > - list_for_each_entry(func, &slot->funcs, sibling) { > - if (func->handle == handle) { > - get_bridge(func->slot->bridge); > - mutex_unlock(&bridge_mutex); > - return func; > - } > - } > - } > - } > - mutex_unlock(&bridge_mutex); > - > - return NULL; > -} > - > - > static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle) > { > struct acpiphp_bridge *bridge; > @@ -1108,6 +1183,7 @@ static void handle_hotplug_event_func(ac > */ > void acpiphp_enumerate_slots(struct pci_bus *bus) > { > + struct acpiphp_context *context; > struct acpiphp_bridge *bridge; > acpi_handle handle; > acpi_status status; > @@ -1120,8 +1196,8 @@ void acpiphp_enumerate_slots(struct pci_ > return; > > bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL); > - if (bridge == NULL) { > - err("out of memory\n"); > + if (!bridge) { > + acpi_handle_err(handle, "No memory for bridge object\n"); > return; > } > can you put this change in another patch? > @@ -1131,6 +1207,21 @@ void acpiphp_enumerate_slots(struct pci_ > bridge->pci_dev = pci_dev_get(bus->self); > bridge->pci_bus = bus; > > + mutex_lock(&acpiphp_context_lock); > + context = acpiphp_get_context(handle); > + if (!context) { > + context = acpiphp_init_context(handle); > + if (!context) { > + mutex_unlock(&acpiphp_context_lock); > + acpi_handle_err(handle, "No hotplug context\n"); > + kfree(bridge); > + return; > + } > + } > + bridge->context = context; > + context->bridge = bridge; > + mutex_unlock(&acpiphp_context_lock); > + > /* > * Grab a ref to the subordinate PCI bus in case the bus is > * removed via PCI core logical hotplug. The ref pins the bus > @@ -1169,13 +1260,15 @@ void acpiphp_enumerate_slots(struct pci_ > > dbg("found ejectable p2p bridge\n"); > bridge->flags |= BRIDGE_HAS_EJ0; > - bridge->func = acpiphp_bridge_handle_to_function(bridge->handle); > - if (bridge->func) { > - status = acpi_remove_notify_handler(bridge->func->handle, > - ACPI_SYSTEM_NOTIFY, > + if (context->func) { > + get_bridge(context->func->slot->bridge); > + bridge->func = context->func; > + handle = context->handle; > + WARN_ON(bridge->handle != handle); > + status = acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY, > handle_hotplug_event_func); > if (ACPI_FAILURE(status)) > - acpi_handle_err(bridge->func->handle, > + acpi_handle_err(handle, > "failed to remove notify handler\n"); > } > return; > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, July 17, 2013 07:00:33 PM Yinghai Lu wrote: > On Wed, Jul 17, 2013 at 4:17 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > When either a new hotplug bridge or a new hotplug function is added > > by the ACPI-based PCI hotplug (ACPIPHP) code, attach a context object > > to its ACPI handle to store hotplug-related information in it. To > > start with, put the handle's bridge and function pointers into that > > object. Count references to the context objects and drop them when > > they are not needed any more. > > > > First of all, this makes it possible to find out if the given bridge > > has been registered as a function already in a much more > > straightforward way and acpiphp_bridge_handle_to_function() can be > > dropped (Yay!). > > > > This also will allow some more simplifications to be made going > > forward. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/pci/hotplug/acpiphp.h | 10 ++ > > drivers/pci/hotplug/acpiphp_glue.c | 179 ++++++++++++++++++++++++++++--------- > > 2 files changed, 146 insertions(+), 43 deletions(-) > > no, more lines are added. Well, that's what 'quilt refresh --diffstat' tells me, quite consistently. Have you actually counted them? > > Index: linux-pm/drivers/pci/hotplug/acpiphp.h > > =================================================================== > > --- linux-pm.orig/drivers/pci/hotplug/acpiphp.h > > +++ linux-pm/drivers/pci/hotplug/acpiphp.h > > @@ -49,6 +49,7 @@ > > #define info(format, arg...) printk(KERN_INFO "%s: " format, MY_NAME , ## arg) > > #define warn(format, arg...) printk(KERN_WARNING "%s: " format, MY_NAME , ## arg) > > > > +struct acpiphp_context; > > struct acpiphp_bridge; > > struct acpiphp_slot; > > > > @@ -77,6 +78,7 @@ struct acpiphp_bridge { > > struct kref ref; > > acpi_handle handle; > > > > + struct acpiphp_context *context; > > /* Ejectable PCI-to-PCI bridge (PCI bridge and PCI function) */ > > struct acpiphp_func *func; > > > > @@ -119,6 +121,7 @@ struct acpiphp_slot { > > * typically 8 objects per slot (i.e. for each PCI function) > > */ > > struct acpiphp_func { > > + struct acpiphp_context *context; > > struct acpiphp_slot *slot; /* parent */ > > > > struct list_head sibling; > > @@ -128,6 +131,13 @@ struct acpiphp_func { > > u32 flags; /* see below */ > > }; > > > > +struct acpiphp_context { > > + acpi_handle handle; > > + struct acpiphp_func *func; > > + struct acpiphp_bridge *bridge; > > + unsigned int refcount; > > +}; > > + > > /* > > * struct acpiphp_attention_info - device specific attention registration > > * > > Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c > > =================================================================== > > --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c > > +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c > > @@ -55,6 +55,7 @@ > > > > static LIST_HEAD(bridge_list); > > static DEFINE_MUTEX(bridge_mutex); > > +static DEFINE_MUTEX(acpiphp_context_lock); > > > > #define MY_NAME "acpiphp_glue" > > > > @@ -79,6 +80,74 @@ is_pci_dock_device(acpi_handle handle, u > > } > > } > > > > +static void acpiphp_context_handler(acpi_handle handle, void *context) > > +{ > > + /* Intentionally empty. */ > > +} > > + > > +/** > > + * acpiphp_init_context - Create hotplug context and grab a reference to it. > > + * @handle: ACPI object handle to create the context for. > > + * > > + * Call under acpiphp_context_lock. > > + */ > > +static struct acpiphp_context *acpiphp_init_context(acpi_handle handle) > > +{ > > + struct acpiphp_context *context; > > + acpi_status status; > > + > > + context = kzalloc(sizeof(*context), GFP_KERNEL); > > + if (!context) > > + return NULL; > > + > > + context->handle = handle; > > + context->refcount = 1; > > + status = acpi_attach_data(handle, acpiphp_context_handler, context); > > + if (ACPI_FAILURE(status)) { > > + kfree(context); > > + return NULL; > > + } > > + return context; > > +} > > + > > +/** > > + * acpiphp_get_context - Get hotplug context and grab a reference to it. > > + * @handle: ACPI object handle to get the context for. > > + * > > + * Call under acpiphp_context_lock. > > + */ > > +static struct acpiphp_context *acpiphp_get_context(acpi_handle handle) > > +{ > > + struct acpiphp_context *context = NULL; > > + acpi_status status; > > + void *data; > > + > > + status = acpi_get_data(handle, acpiphp_context_handler, &data); > > + if (ACPI_SUCCESS(status)) { > > + context = data; > > + context->refcount++; > > + } > > + return context; > > +} > > + > > +/** > > + * acpiphp_put_context - Drop a reference to ACPI hotplug context. > > + * @handle: ACPI object handle to put the context for. > > + * > > + * The context object is removed if there are no more references to it. > > + * > > + * Call under acpiphp_context_lock. > > + */ > > +static void acpiphp_put_context(struct acpiphp_context *context) > > +{ > > + if (--context->refcount) > > + return; > > + > > + WARN_ON(context->func || context->bridge); > > + acpi_detach_data(context->handle, acpiphp_context_handler); > > + kfree(context); > > +} > > + > > static inline void get_bridge(struct acpiphp_bridge *bridge) > > { > > kref_get(&bridge->ref); > > @@ -91,25 +160,37 @@ static inline void put_bridge(struct acp > > > > static void free_bridge(struct kref *kref) > > { > > + struct acpiphp_context *context; > > struct acpiphp_bridge *bridge; > > struct acpiphp_slot *slot, *next; > > struct acpiphp_func *func, *tmp; > > > > + mutex_lock(&acpiphp_context_lock); > > + > > bridge = container_of(kref, struct acpiphp_bridge, ref); > > > > list_for_each_entry_safe(slot, next, &bridge->slots, node) { > > list_for_each_entry_safe(func, tmp, &slot->funcs, sibling) { > > + context = func->context; > > + context->func = NULL; > > + acpiphp_put_context(context); > > kfree(func); > > } > > kfree(slot); > > } > > > > - /* Release reference acquired by acpiphp_bridge_handle_to_function() */ > > + /* Release the reference acquired by acpiphp_enumerate_slots(). */ > > if ((bridge->flags & BRIDGE_HAS_EJ0) && bridge->func) > > put_bridge(bridge->func->slot->bridge); > > + > > put_device(&bridge->pci_bus->dev); > > pci_dev_put(bridge->pci_dev); > > + context = bridge->context; > > + context->bridge = NULL; > > + acpiphp_put_context(context); > > kfree(bridge); > > + > > + mutex_unlock(&acpiphp_context_lock); > > } > > > > /* > > @@ -194,10 +275,11 @@ static void acpiphp_dock_release(void *d > > } > > > > /* callback routine to register each ACPI PCI slot object */ > > -static acpi_status > > -register_slot(acpi_handle handle, u32 lvl, void *context, void **rv) > > +static acpi_status register_slot(acpi_handle handle, u32 lvl, void *data, > > + void **rv) > > { > > - struct acpiphp_bridge *bridge = (struct acpiphp_bridge *)context; > > + struct acpiphp_bridge *bridge = data; > > + struct acpiphp_context *context; > > struct acpiphp_slot *slot; > > struct acpiphp_func *newfunc; > > acpi_status status = AE_OK; > > @@ -230,6 +312,18 @@ register_slot(acpi_handle handle, u32 lv > > newfunc->handle = handle; > > newfunc->function = function; > > > > + mutex_lock(&acpiphp_context_lock); > > + context = acpiphp_init_context(handle); > > + if (!context) { > > + mutex_unlock(&acpiphp_context_lock); > > + acpi_handle_err(handle, "No hotplug context\n"); > > + kfree(newfunc); > > + return AE_NOT_EXIST; > > + } > > + newfunc->context = context; > > + context->func = newfunc; > > + mutex_unlock(&acpiphp_context_lock); > > + > > if (acpi_has_method(handle, "_EJ0")) > > newfunc->flags = FUNC_HAS_EJ0; > > > > @@ -266,8 +360,8 @@ register_slot(acpi_handle handle, u32 lv > > if (!found) { > > slot = kzalloc(sizeof(struct acpiphp_slot), GFP_KERNEL); > > if (!slot) { > > - kfree(newfunc); > > - return AE_NO_MEMORY; > > + status = AE_NO_MEMORY; > > + goto err_out; > > } > > > > slot->bridge = bridge; > > @@ -291,7 +385,9 @@ register_slot(acpi_handle handle, u32 lv > > else > > warn("acpiphp_register_hotplug_slot failed " > > "(err code = 0x%x)\n", retval); > > - goto err_exit; > > + > > + status = AE_OK; > > + goto err; > > } > > } > > > > @@ -329,15 +425,20 @@ register_slot(acpi_handle handle, u32 lv > > > > return AE_OK; > > > > - err_exit: > > + err: > > bridge->nr_slots--; > > mutex_lock(&bridge_mutex); > > list_del(&slot->node); > > mutex_unlock(&bridge_mutex); > > kfree(slot); > > - kfree(newfunc); > > > > - return AE_OK; > > + err_out: > > + mutex_lock(&acpiphp_context_lock); > > + context->func = NULL; > > + acpiphp_put_context(context); > > + mutex_unlock(&acpiphp_context_lock); > > + kfree(newfunc); > > + return status; > > } > > > > > > @@ -352,32 +453,6 @@ static int detect_ejectable_slots(acpi_h > > return found; > > } > > > > - > > -/* find acpiphp_func from acpiphp_bridge */ > > -static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle) > > -{ > > - struct acpiphp_bridge *bridge; > > - struct acpiphp_slot *slot; > > - struct acpiphp_func *func = NULL; > > - > > - mutex_lock(&bridge_mutex); > > - list_for_each_entry(bridge, &bridge_list, list) { > > - list_for_each_entry(slot, &bridge->slots, node) { > > - list_for_each_entry(func, &slot->funcs, sibling) { > > - if (func->handle == handle) { > > - get_bridge(func->slot->bridge); > > - mutex_unlock(&bridge_mutex); > > - return func; > > - } > > - } > > - } > > - } > > - mutex_unlock(&bridge_mutex); > > - > > - return NULL; > > -} > > - > > - > > static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle) > > { > > struct acpiphp_bridge *bridge; > > @@ -1108,6 +1183,7 @@ static void handle_hotplug_event_func(ac > > */ > > void acpiphp_enumerate_slots(struct pci_bus *bus) > > { > > + struct acpiphp_context *context; > > struct acpiphp_bridge *bridge; > > acpi_handle handle; > > acpi_status status; > > @@ -1120,8 +1196,8 @@ void acpiphp_enumerate_slots(struct pci_ > > return; > > > > bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL); > > - if (bridge == NULL) { > > - err("out of memory\n"); > > + if (!bridge) { > > + acpi_handle_err(handle, "No memory for bridge object\n"); > > return; > > } > > > > can you put this change in another patch? Yes, I can, but is it worth it? > > @@ -1131,6 +1207,21 @@ void acpiphp_enumerate_slots(struct pci_ > > bridge->pci_dev = pci_dev_get(bus->self); > > bridge->pci_bus = bus; > > > > + mutex_lock(&acpiphp_context_lock); > > + context = acpiphp_get_context(handle); > > + if (!context) { > > + context = acpiphp_init_context(handle); > > + if (!context) { > > + mutex_unlock(&acpiphp_context_lock); > > + acpi_handle_err(handle, "No hotplug context\n"); > > + kfree(bridge); > > + return; > > + } > > + } > > + bridge->context = context; > > + context->bridge = bridge; > > + mutex_unlock(&acpiphp_context_lock); > > + > > /* > > * Grab a ref to the subordinate PCI bus in case the bus is > > * removed via PCI core logical hotplug. The ref pins the bus > > @@ -1169,13 +1260,15 @@ void acpiphp_enumerate_slots(struct pci_ > > > > dbg("found ejectable p2p bridge\n"); > > bridge->flags |= BRIDGE_HAS_EJ0; > > - bridge->func = acpiphp_bridge_handle_to_function(bridge->handle); > > - if (bridge->func) { > > - status = acpi_remove_notify_handler(bridge->func->handle, > > - ACPI_SYSTEM_NOTIFY, > > + if (context->func) { > > + get_bridge(context->func->slot->bridge); > > + bridge->func = context->func; > > + handle = context->handle; > > + WARN_ON(bridge->handle != handle); > > + status = acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY, > > handle_hotplug_event_func); > > if (ACPI_FAILURE(status)) > > - acpi_handle_err(bridge->func->handle, > > + acpi_handle_err(handle, > > "failed to remove notify handler\n"); > > } > > return; > >
On Thursday, July 18, 2013 09:04:11 PM Rafael J. Wysocki wrote: > On Wednesday, July 17, 2013 07:00:33 PM Yinghai Lu wrote: > > On Wed, Jul 17, 2013 at 4:17 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > When either a new hotplug bridge or a new hotplug function is added > > > by the ACPI-based PCI hotplug (ACPIPHP) code, attach a context object > > > to its ACPI handle to store hotplug-related information in it. To > > > start with, put the handle's bridge and function pointers into that > > > object. Count references to the context objects and drop them when > > > they are not needed any more. > > > > > > First of all, this makes it possible to find out if the given bridge > > > has been registered as a function already in a much more > > > straightforward way and acpiphp_bridge_handle_to_function() can be > > > dropped (Yay!). > > > > > > This also will allow some more simplifications to be made going > > > forward. > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > --- > > > drivers/pci/hotplug/acpiphp.h | 10 ++ > > > drivers/pci/hotplug/acpiphp_glue.c | 179 ++++++++++++++++++++++++++++--------- > > > 2 files changed, 146 insertions(+), 43 deletions(-) > > > > no, more lines are added. > > Well, that's what 'quilt refresh --diffstat' tells me, quite consistently. > > Have you actually counted them? Now I have done that (using "grep '^+' | wc -l") and that indeed turns out ot the right number. What gives? Rafael
Index: linux-pm/drivers/pci/hotplug/acpiphp.h =================================================================== --- linux-pm.orig/drivers/pci/hotplug/acpiphp.h +++ linux-pm/drivers/pci/hotplug/acpiphp.h @@ -49,6 +49,7 @@ #define info(format, arg...) printk(KERN_INFO "%s: " format, MY_NAME , ## arg) #define warn(format, arg...) printk(KERN_WARNING "%s: " format, MY_NAME , ## arg) +struct acpiphp_context; struct acpiphp_bridge; struct acpiphp_slot; @@ -77,6 +78,7 @@ struct acpiphp_bridge { struct kref ref; acpi_handle handle; + struct acpiphp_context *context; /* Ejectable PCI-to-PCI bridge (PCI bridge and PCI function) */ struct acpiphp_func *func; @@ -119,6 +121,7 @@ struct acpiphp_slot { * typically 8 objects per slot (i.e. for each PCI function) */ struct acpiphp_func { + struct acpiphp_context *context; struct acpiphp_slot *slot; /* parent */ struct list_head sibling; @@ -128,6 +131,13 @@ struct acpiphp_func { u32 flags; /* see below */ }; +struct acpiphp_context { + acpi_handle handle; + struct acpiphp_func *func; + struct acpiphp_bridge *bridge; + unsigned int refcount; +}; + /* * struct acpiphp_attention_info - device specific attention registration * Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c =================================================================== --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c @@ -55,6 +55,7 @@ static LIST_HEAD(bridge_list); static DEFINE_MUTEX(bridge_mutex); +static DEFINE_MUTEX(acpiphp_context_lock); #define MY_NAME "acpiphp_glue" @@ -79,6 +80,74 @@ is_pci_dock_device(acpi_handle handle, u } } +static void acpiphp_context_handler(acpi_handle handle, void *context) +{ + /* Intentionally empty. */ +} + +/** + * acpiphp_init_context - Create hotplug context and grab a reference to it. + * @handle: ACPI object handle to create the context for. + * + * Call under acpiphp_context_lock. + */ +static struct acpiphp_context *acpiphp_init_context(acpi_handle handle) +{ + struct acpiphp_context *context; + acpi_status status; + + context = kzalloc(sizeof(*context), GFP_KERNEL); + if (!context) + return NULL; + + context->handle = handle; + context->refcount = 1; + status = acpi_attach_data(handle, acpiphp_context_handler, context); + if (ACPI_FAILURE(status)) { + kfree(context); + return NULL; + } + return context; +} + +/** + * acpiphp_get_context - Get hotplug context and grab a reference to it. + * @handle: ACPI object handle to get the context for. + * + * Call under acpiphp_context_lock. + */ +static struct acpiphp_context *acpiphp_get_context(acpi_handle handle) +{ + struct acpiphp_context *context = NULL; + acpi_status status; + void *data; + + status = acpi_get_data(handle, acpiphp_context_handler, &data); + if (ACPI_SUCCESS(status)) { + context = data; + context->refcount++; + } + return context; +} + +/** + * acpiphp_put_context - Drop a reference to ACPI hotplug context. + * @handle: ACPI object handle to put the context for. + * + * The context object is removed if there are no more references to it. + * + * Call under acpiphp_context_lock. + */ +static void acpiphp_put_context(struct acpiphp_context *context) +{ + if (--context->refcount) + return; + + WARN_ON(context->func || context->bridge); + acpi_detach_data(context->handle, acpiphp_context_handler); + kfree(context); +} + static inline void get_bridge(struct acpiphp_bridge *bridge) { kref_get(&bridge->ref); @@ -91,25 +160,37 @@ static inline void put_bridge(struct acp static void free_bridge(struct kref *kref) { + struct acpiphp_context *context; struct acpiphp_bridge *bridge; struct acpiphp_slot *slot, *next; struct acpiphp_func *func, *tmp; + mutex_lock(&acpiphp_context_lock); + bridge = container_of(kref, struct acpiphp_bridge, ref); list_for_each_entry_safe(slot, next, &bridge->slots, node) { list_for_each_entry_safe(func, tmp, &slot->funcs, sibling) { + context = func->context; + context->func = NULL; + acpiphp_put_context(context); kfree(func); } kfree(slot); } - /* Release reference acquired by acpiphp_bridge_handle_to_function() */ + /* Release the reference acquired by acpiphp_enumerate_slots(). */ if ((bridge->flags & BRIDGE_HAS_EJ0) && bridge->func) put_bridge(bridge->func->slot->bridge); + put_device(&bridge->pci_bus->dev); pci_dev_put(bridge->pci_dev); + context = bridge->context; + context->bridge = NULL; + acpiphp_put_context(context); kfree(bridge); + + mutex_unlock(&acpiphp_context_lock); } /* @@ -194,10 +275,11 @@ static void acpiphp_dock_release(void *d } /* callback routine to register each ACPI PCI slot object */ -static acpi_status -register_slot(acpi_handle handle, u32 lvl, void *context, void **rv) +static acpi_status register_slot(acpi_handle handle, u32 lvl, void *data, + void **rv) { - struct acpiphp_bridge *bridge = (struct acpiphp_bridge *)context; + struct acpiphp_bridge *bridge = data; + struct acpiphp_context *context; struct acpiphp_slot *slot; struct acpiphp_func *newfunc; acpi_status status = AE_OK; @@ -230,6 +312,18 @@ register_slot(acpi_handle handle, u32 lv newfunc->handle = handle; newfunc->function = function; + mutex_lock(&acpiphp_context_lock); + context = acpiphp_init_context(handle); + if (!context) { + mutex_unlock(&acpiphp_context_lock); + acpi_handle_err(handle, "No hotplug context\n"); + kfree(newfunc); + return AE_NOT_EXIST; + } + newfunc->context = context; + context->func = newfunc; + mutex_unlock(&acpiphp_context_lock); + if (acpi_has_method(handle, "_EJ0")) newfunc->flags = FUNC_HAS_EJ0; @@ -266,8 +360,8 @@ register_slot(acpi_handle handle, u32 lv if (!found) { slot = kzalloc(sizeof(struct acpiphp_slot), GFP_KERNEL); if (!slot) { - kfree(newfunc); - return AE_NO_MEMORY; + status = AE_NO_MEMORY; + goto err_out; } slot->bridge = bridge; @@ -291,7 +385,9 @@ register_slot(acpi_handle handle, u32 lv else warn("acpiphp_register_hotplug_slot failed " "(err code = 0x%x)\n", retval); - goto err_exit; + + status = AE_OK; + goto err; } } @@ -329,15 +425,20 @@ register_slot(acpi_handle handle, u32 lv return AE_OK; - err_exit: + err: bridge->nr_slots--; mutex_lock(&bridge_mutex); list_del(&slot->node); mutex_unlock(&bridge_mutex); kfree(slot); - kfree(newfunc); - return AE_OK; + err_out: + mutex_lock(&acpiphp_context_lock); + context->func = NULL; + acpiphp_put_context(context); + mutex_unlock(&acpiphp_context_lock); + kfree(newfunc); + return status; } @@ -352,32 +453,6 @@ static int detect_ejectable_slots(acpi_h return found; } - -/* find acpiphp_func from acpiphp_bridge */ -static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle) -{ - struct acpiphp_bridge *bridge; - struct acpiphp_slot *slot; - struct acpiphp_func *func = NULL; - - mutex_lock(&bridge_mutex); - list_for_each_entry(bridge, &bridge_list, list) { - list_for_each_entry(slot, &bridge->slots, node) { - list_for_each_entry(func, &slot->funcs, sibling) { - if (func->handle == handle) { - get_bridge(func->slot->bridge); - mutex_unlock(&bridge_mutex); - return func; - } - } - } - } - mutex_unlock(&bridge_mutex); - - return NULL; -} - - static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle) { struct acpiphp_bridge *bridge; @@ -1108,6 +1183,7 @@ static void handle_hotplug_event_func(ac */ void acpiphp_enumerate_slots(struct pci_bus *bus) { + struct acpiphp_context *context; struct acpiphp_bridge *bridge; acpi_handle handle; acpi_status status; @@ -1120,8 +1196,8 @@ void acpiphp_enumerate_slots(struct pci_ return; bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL); - if (bridge == NULL) { - err("out of memory\n"); + if (!bridge) { + acpi_handle_err(handle, "No memory for bridge object\n"); return; } @@ -1131,6 +1207,21 @@ void acpiphp_enumerate_slots(struct pci_ bridge->pci_dev = pci_dev_get(bus->self); bridge->pci_bus = bus; + mutex_lock(&acpiphp_context_lock); + context = acpiphp_get_context(handle); + if (!context) { + context = acpiphp_init_context(handle); + if (!context) { + mutex_unlock(&acpiphp_context_lock); + acpi_handle_err(handle, "No hotplug context\n"); + kfree(bridge); + return; + } + } + bridge->context = context; + context->bridge = bridge; + mutex_unlock(&acpiphp_context_lock); + /* * Grab a ref to the subordinate PCI bus in case the bus is * removed via PCI core logical hotplug. The ref pins the bus @@ -1169,13 +1260,15 @@ void acpiphp_enumerate_slots(struct pci_ dbg("found ejectable p2p bridge\n"); bridge->flags |= BRIDGE_HAS_EJ0; - bridge->func = acpiphp_bridge_handle_to_function(bridge->handle); - if (bridge->func) { - status = acpi_remove_notify_handler(bridge->func->handle, - ACPI_SYSTEM_NOTIFY, + if (context->func) { + get_bridge(context->func->slot->bridge); + bridge->func = context->func; + handle = context->handle; + WARN_ON(bridge->handle != handle); + status = acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY, handle_hotplug_event_func); if (ACPI_FAILURE(status)) - acpi_handle_err(bridge->func->handle, + acpi_handle_err(handle, "failed to remove notify handler\n"); } return;