Patchwork [2/30] ACPI / hotplug / PCI: Consolidate acpiphp_enumerate_slots()

login
register
mail settings
Submitter Rafael J. Wysocki
Date July 17, 2013, 11:16 p.m.
Message ID <10121200.vQGTlJqFtd@vostro.rjw.lan>
Download mbox | patch
Permalink /patch/259940/
State Not Applicable
Headers show

Comments

Rafael J. Wysocki - July 17, 2013, 11:16 p.m.
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The acpiphp_enumerate_slots() function is now split into two parts,
acpiphp_enumerate_slots() proper and init_bridge_misc() which is
only called by the former.  If these functions are combined,
it is possible to make the code easier to follow and to clean up
the error handling (to prevent memory leaks on error from
happening in particular), so do that.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   92 ++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 47 deletions(-)


--
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
Yinghai Lu - July 18, 2013, 1:40 a.m.
On Wed, Jul 17, 2013 at 4:16 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The acpiphp_enumerate_slots() function is now split into two parts,
> acpiphp_enumerate_slots() proper and init_bridge_misc() which is
> only called by the former.  If these functions are combined,
> it is possible to make the code easier to follow and to clean up
> the error handling (to prevent memory leaks on error from
> happening in particular), so do that.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c |   92 ++++++++++++++++++-------------------
>  1 file changed, 45 insertions(+), 47 deletions(-)
>
> 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
> @@ -353,46 +353,6 @@ static int detect_ejectable_slots(acpi_h
>         return found;
>  }
>
> -/* initialize miscellaneous stuff for both root and PCI-to-PCI bridge */
> -static void init_bridge_misc(struct acpiphp_bridge *bridge)
> -{
> -       acpi_status status;
> -
> -       /* must be added to the list prior to calling register_slot */
> -       mutex_lock(&bridge_mutex);
> -       list_add(&bridge->list, &bridge_list);
> -       mutex_unlock(&bridge_mutex);
> -
> -       /* register all slot objects under this bridge */
> -       status = acpi_walk_namespace(ACPI_TYPE_DEVICE, bridge->handle, (u32)1,
> -                                    register_slot, NULL, bridge, NULL);
> -       if (ACPI_FAILURE(status)) {
> -               mutex_lock(&bridge_mutex);
> -               list_del(&bridge->list);
> -               mutex_unlock(&bridge_mutex);
> -               return;
> -       }
> -
> -       /* install notify handler for P2P bridges */
> -       if (!pci_is_root_bus(bridge->pci_bus)) {
> -               if ((bridge->flags & BRIDGE_HAS_EJ0) && bridge->func) {
> -                       status = acpi_remove_notify_handler(bridge->func->handle,
> -                                               ACPI_SYSTEM_NOTIFY,
> -                                               handle_hotplug_event_func);
> -                       if (ACPI_FAILURE(status))
> -                               err("failed to remove notify handler\n");
> -               }
> -               status = acpi_install_notify_handler(bridge->handle,
> -                                            ACPI_SYSTEM_NOTIFY,
> -                                            handle_hotplug_event_bridge,
> -                                            bridge);
> -
> -               if (ACPI_FAILURE(status)) {
> -                       err("failed to register interrupt notify handler\n");
> -               }
> -       }
> -}
> -
>
>  /* find acpiphp_func from acpiphp_bridge */
>  static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle)
> @@ -1149,8 +1109,9 @@ static void handle_hotplug_event_func(ac
>   */
>  void acpiphp_enumerate_slots(struct pci_bus *bus)
>  {
> -       acpi_handle handle;
>         struct acpiphp_bridge *bridge;
> +       acpi_handle handle;
> +       acpi_status status;
>
>         if (acpiphp_disabled)
>                 return;
> @@ -1178,14 +1139,51 @@ void acpiphp_enumerate_slots(struct pci_
>          */
>         get_device(&bus->dev);
>
> -       if (!pci_is_root_bus(bridge->pci_bus) &&
> -           acpi_has_method(bridge->handle, "_EJ0")) {
> -               dbg("found ejectable p2p bridge\n");
> -               bridge->flags |= BRIDGE_HAS_EJ0;
> -               bridge->func = acpiphp_bridge_handle_to_function(handle);
> +       /* must be added to the list prior to calling register_slot */
> +       mutex_lock(&bridge_mutex);
> +       list_add(&bridge->list, &bridge_list);
> +       mutex_unlock(&bridge_mutex);
> +
> +       /* register all slot objects under this bridge */
> +       status = acpi_walk_namespace(ACPI_TYPE_DEVICE, bridge->handle, 1,
> +                                    register_slot, NULL, bridge, NULL);
> +       if (ACPI_FAILURE(status)) {
> +               acpi_handle_err(bridge->handle, "failed to register slots\n");
> +               goto err;
> +       }
> +
> +       if (pci_is_root_bus(bridge->pci_bus))
> +               return;
> +
> +       /* install notify handler for P2P bridges */
> +       status = acpi_install_notify_handler(bridge->handle, ACPI_SYSTEM_NOTIFY,
> +                                            handle_hotplug_event_bridge,
> +                                            bridge);
> +       if (ACPI_FAILURE(status)) {
> +               acpi_handle_err(bridge->handle,
> +                               "failed to register notify handler\n");
> +               goto err;
>         }
>
> -       init_bridge_misc(bridge);
> +       if (!acpi_has_method(bridge->handle, "_EJ0"))
> +               return;
> +
> +       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,
> +                                                   handle_hotplug_event_func);
> +               if (ACPI_FAILURE(status))
> +                       acpi_handle_err(bridge->func->handle,
> +                                       "failed to remove notify handler\n");
> +       }

looks little weird that you change to install bridge notifier handler and remove
bridge func notifier handler.
actually these two handles is the same one.

I would prefer to keeping the old sequence.

Yinghai
--
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
Rafael J. Wysocki - July 18, 2013, 7:09 p.m.
On Wednesday, July 17, 2013 06:40:10 PM Yinghai Lu wrote:
> On Wed, Jul 17, 2013 at 4:16 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The acpiphp_enumerate_slots() function is now split into two parts,
> > acpiphp_enumerate_slots() proper and init_bridge_misc() which is
> > only called by the former.  If these functions are combined,
> > it is possible to make the code easier to follow and to clean up
> > the error handling (to prevent memory leaks on error from
> > happening in particular), so do that.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/pci/hotplug/acpiphp_glue.c |   92 ++++++++++++++++++-------------------
> >  1 file changed, 45 insertions(+), 47 deletions(-)
> >
> > 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
> > @@ -353,46 +353,6 @@ static int detect_ejectable_slots(acpi_h
> >         return found;
> >  }
> >
> > -/* initialize miscellaneous stuff for both root and PCI-to-PCI bridge */
> > -static void init_bridge_misc(struct acpiphp_bridge *bridge)
> > -{
> > -       acpi_status status;
> > -
> > -       /* must be added to the list prior to calling register_slot */
> > -       mutex_lock(&bridge_mutex);
> > -       list_add(&bridge->list, &bridge_list);
> > -       mutex_unlock(&bridge_mutex);
> > -
> > -       /* register all slot objects under this bridge */
> > -       status = acpi_walk_namespace(ACPI_TYPE_DEVICE, bridge->handle, (u32)1,
> > -                                    register_slot, NULL, bridge, NULL);
> > -       if (ACPI_FAILURE(status)) {
> > -               mutex_lock(&bridge_mutex);
> > -               list_del(&bridge->list);
> > -               mutex_unlock(&bridge_mutex);
> > -               return;
> > -       }
> > -
> > -       /* install notify handler for P2P bridges */
> > -       if (!pci_is_root_bus(bridge->pci_bus)) {
> > -               if ((bridge->flags & BRIDGE_HAS_EJ0) && bridge->func) {
> > -                       status = acpi_remove_notify_handler(bridge->func->handle,
> > -                                               ACPI_SYSTEM_NOTIFY,
> > -                                               handle_hotplug_event_func);
> > -                       if (ACPI_FAILURE(status))
> > -                               err("failed to remove notify handler\n");
> > -               }
> > -               status = acpi_install_notify_handler(bridge->handle,
> > -                                            ACPI_SYSTEM_NOTIFY,
> > -                                            handle_hotplug_event_bridge,
> > -                                            bridge);
> > -
> > -               if (ACPI_FAILURE(status)) {
> > -                       err("failed to register interrupt notify handler\n");
> > -               }
> > -       }
> > -}
> > -
> >
> >  /* find acpiphp_func from acpiphp_bridge */
> >  static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle)
> > @@ -1149,8 +1109,9 @@ static void handle_hotplug_event_func(ac
> >   */
> >  void acpiphp_enumerate_slots(struct pci_bus *bus)
> >  {
> > -       acpi_handle handle;
> >         struct acpiphp_bridge *bridge;
> > +       acpi_handle handle;
> > +       acpi_status status;
> >
> >         if (acpiphp_disabled)
> >                 return;
> > @@ -1178,14 +1139,51 @@ void acpiphp_enumerate_slots(struct pci_
> >          */
> >         get_device(&bus->dev);
> >
> > -       if (!pci_is_root_bus(bridge->pci_bus) &&
> > -           acpi_has_method(bridge->handle, "_EJ0")) {
> > -               dbg("found ejectable p2p bridge\n");
> > -               bridge->flags |= BRIDGE_HAS_EJ0;
> > -               bridge->func = acpiphp_bridge_handle_to_function(handle);
> > +       /* must be added to the list prior to calling register_slot */
> > +       mutex_lock(&bridge_mutex);
> > +       list_add(&bridge->list, &bridge_list);
> > +       mutex_unlock(&bridge_mutex);
> > +
> > +       /* register all slot objects under this bridge */
> > +       status = acpi_walk_namespace(ACPI_TYPE_DEVICE, bridge->handle, 1,
> > +                                    register_slot, NULL, bridge, NULL);
> > +       if (ACPI_FAILURE(status)) {
> > +               acpi_handle_err(bridge->handle, "failed to register slots\n");
> > +               goto err;
> > +       }
> > +
> > +       if (pci_is_root_bus(bridge->pci_bus))
> > +               return;
> > +
> > +       /* install notify handler for P2P bridges */
> > +       status = acpi_install_notify_handler(bridge->handle, ACPI_SYSTEM_NOTIFY,
> > +                                            handle_hotplug_event_bridge,
> > +                                            bridge);
> > +       if (ACPI_FAILURE(status)) {
> > +               acpi_handle_err(bridge->handle,
> > +                               "failed to register notify handler\n");
> > +               goto err;
> >         }
> >
> > -       init_bridge_misc(bridge);
> > +       if (!acpi_has_method(bridge->handle, "_EJ0"))
> > +               return;
> > +
> > +       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,
> > +                                                   handle_hotplug_event_func);
> > +               if (ACPI_FAILURE(status))
> > +                       acpi_handle_err(bridge->func->handle,
> > +                                       "failed to remove notify handler\n");
> > +       }
> 
> looks little weird that you change to install bridge notifier handler and remove
> bridge func notifier handler.
> actually these two handles is the same one.
> 
> I would prefer to keeping the old sequence.

First, it doesn't matter because one of the notifiers goes away in the
subsequent patches and it would complicate those patches without any real
reason if I did that.

Second, it doesn't matter because there can be multiple notify handlers for the
same handle and if one of the above two triggers, the second one won't really
do anything.

So no, I'm not going to do that, sorry.

Thanks,
Rafael

Patch

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
@@ -353,46 +353,6 @@  static int detect_ejectable_slots(acpi_h
 	return found;
 }
 
-/* initialize miscellaneous stuff for both root and PCI-to-PCI bridge */
-static void init_bridge_misc(struct acpiphp_bridge *bridge)
-{
-	acpi_status status;
-
-	/* must be added to the list prior to calling register_slot */
-	mutex_lock(&bridge_mutex);
-	list_add(&bridge->list, &bridge_list);
-	mutex_unlock(&bridge_mutex);
-
-	/* register all slot objects under this bridge */
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, bridge->handle, (u32)1,
-				     register_slot, NULL, bridge, NULL);
-	if (ACPI_FAILURE(status)) {
-		mutex_lock(&bridge_mutex);
-		list_del(&bridge->list);
-		mutex_unlock(&bridge_mutex);
-		return;
-	}
-
-	/* install notify handler for P2P bridges */
-	if (!pci_is_root_bus(bridge->pci_bus)) {
-		if ((bridge->flags & BRIDGE_HAS_EJ0) && bridge->func) {
-			status = acpi_remove_notify_handler(bridge->func->handle,
-						ACPI_SYSTEM_NOTIFY,
-						handle_hotplug_event_func);
-			if (ACPI_FAILURE(status))
-				err("failed to remove notify handler\n");
-		}
-		status = acpi_install_notify_handler(bridge->handle,
-					     ACPI_SYSTEM_NOTIFY,
-					     handle_hotplug_event_bridge,
-					     bridge);
-
-		if (ACPI_FAILURE(status)) {
-			err("failed to register interrupt notify handler\n");
-		}
-	}
-}
-
 
 /* find acpiphp_func from acpiphp_bridge */
 static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle)
@@ -1149,8 +1109,9 @@  static void handle_hotplug_event_func(ac
  */
 void acpiphp_enumerate_slots(struct pci_bus *bus)
 {
-	acpi_handle handle;
 	struct acpiphp_bridge *bridge;
+	acpi_handle handle;
+	acpi_status status;
 
 	if (acpiphp_disabled)
 		return;
@@ -1178,14 +1139,51 @@  void acpiphp_enumerate_slots(struct pci_
 	 */
 	get_device(&bus->dev);
 
-	if (!pci_is_root_bus(bridge->pci_bus) &&
-	    acpi_has_method(bridge->handle, "_EJ0")) {
-		dbg("found ejectable p2p bridge\n");
-		bridge->flags |= BRIDGE_HAS_EJ0;
-		bridge->func = acpiphp_bridge_handle_to_function(handle);
+	/* must be added to the list prior to calling register_slot */
+	mutex_lock(&bridge_mutex);
+	list_add(&bridge->list, &bridge_list);
+	mutex_unlock(&bridge_mutex);
+
+	/* register all slot objects under this bridge */
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, bridge->handle, 1,
+				     register_slot, NULL, bridge, NULL);
+	if (ACPI_FAILURE(status)) {
+		acpi_handle_err(bridge->handle, "failed to register slots\n");
+		goto err;
+	}
+
+	if (pci_is_root_bus(bridge->pci_bus))
+		return;
+
+	/* install notify handler for P2P bridges */
+	status = acpi_install_notify_handler(bridge->handle, ACPI_SYSTEM_NOTIFY,
+					     handle_hotplug_event_bridge,
+					     bridge);
+	if (ACPI_FAILURE(status)) {
+		acpi_handle_err(bridge->handle,
+				"failed to register notify handler\n");
+		goto err;
 	}
 
-	init_bridge_misc(bridge);
+	if (!acpi_has_method(bridge->handle, "_EJ0"))
+		return;
+
+	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,
+						    handle_hotplug_event_func);
+		if (ACPI_FAILURE(status))
+			acpi_handle_err(bridge->func->handle,
+					"failed to remove notify handler\n");
+	}
+	return;
+
+ err:
+	cleanup_bridge(bridge);
+	put_bridge(bridge);
 }
 
 /* Destroy hotplug slots associated with the PCI bus */