Patchwork [6/6] ACPI: Change the ordering of acpi_bus_check_add()

login
register
mail settings
Submitter Rafael J. Wysocki
Date Dec. 9, 2012, 11:04 p.m.
Message ID <2268672.Pe9OEEVsRc@vostro.rjw.lan>
Download mbox | patch
Permalink /patch/204794/
State Superseded
Headers show

Comments

Rafael J. Wysocki - Dec. 9, 2012, 11:04 p.m.
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If acpi_bus_check_add() is called for a handle already having an
existing struct acpi_device object attached, it is not necessary to
check the type and status of the device correspondig to it, so
change the ordering of acpi_bus_check_add() to avoid that.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 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
Bjorn Helgaas - Dec. 13, 2012, 1 a.m.
On Sun, Dec 9, 2012 at 4:04 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> If acpi_bus_check_add() is called for a handle already having an
> existing struct acpi_device object attached, it is not necessary to
> check the type and status of the device correspondig to it, so
> change the ordering of acpi_bus_check_add() to avoid that.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/scan.c |   18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> Index: linux/drivers/acpi/scan.c
> ===================================================================
> --- linux.orig/drivers/acpi/scan.c
> +++ linux/drivers/acpi/scan.c
> @@ -1582,6 +1582,10 @@ static acpi_status acpi_bus_check_add(ac
>         acpi_status status;
>         int result;
>
> +       acpi_bus_get_device(handle, &device);
> +       if (device)
> +               goto out;
> +
>         result = acpi_bus_type_and_status(handle, &type, &sta);
>         if (result)
>                 return AE_OK;
> @@ -1602,17 +1606,13 @@ static acpi_status acpi_bus_check_add(ac
>          * We may already have an acpi_device from a previous enumeration.  If
>          * so, we needn't add it again, but we may still have to start it.

It looks like this comment might need updating, since we no longer
even get here if we already have an acpi_device.  Presumably you take
care of the "start" it mentions elsewhere now.

>          */
> -       acpi_bus_get_device(handle, &device);
> -       if (!device) {
> -               acpi_add_single_object(&device, handle, type, sta,
> -                                      ACPI_BUS_ADD_BASIC);
> -               if (!device)
> -                       return AE_CTRL_DEPTH;
> +       acpi_add_single_object(&device, handle, type, sta, ACPI_BUS_ADD_BASIC);
> +       if (!device)
> +               return AE_CTRL_DEPTH;
>
> -               device->add_type = context ?
> -                                       ACPI_BUS_ADD_START : ACPI_BUS_ADD_MATCH;
> -       }
> +       device->add_type = context ? ACPI_BUS_ADD_START : ACPI_BUS_ADD_MATCH;
>
> + out:
>         if (!*return_value)
>                 *return_value = device;

I think all (both) callers of acpi_bus_check_add() supply a
return_value pointer, so you could just remove the test.
--
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 - Dec. 13, 2012, 12:20 p.m.
On Wednesday, December 12, 2012 06:00:28 PM Bjorn Helgaas wrote:
> On Sun, Dec 9, 2012 at 4:04 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > If acpi_bus_check_add() is called for a handle already having an
> > existing struct acpi_device object attached, it is not necessary to
> > check the type and status of the device correspondig to it, so
> > change the ordering of acpi_bus_check_add() to avoid that.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/scan.c |   18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > Index: linux/drivers/acpi/scan.c
> > ===================================================================
> > --- linux.orig/drivers/acpi/scan.c
> > +++ linux/drivers/acpi/scan.c
> > @@ -1582,6 +1582,10 @@ static acpi_status acpi_bus_check_add(ac
> >         acpi_status status;
> >         int result;
> >
> > +       acpi_bus_get_device(handle, &device);
> > +       if (device)
> > +               goto out;
> > +
> >         result = acpi_bus_type_and_status(handle, &type, &sta);
> >         if (result)
> >                 return AE_OK;
> > @@ -1602,17 +1606,13 @@ static acpi_status acpi_bus_check_add(ac
> >          * We may already have an acpi_device from a previous enumeration.  If
> >          * so, we needn't add it again, but we may still have to start it.
> 
> It looks like this comment might need updating, since we no longer
> even get here if we already have an acpi_device.  Presumably you take
> care of the "start" it mentions elsewhere now.

I think you're right, will update.

Thanks,
Rafael


> >          */
> > -       acpi_bus_get_device(handle, &device);
> > -       if (!device) {
> > -               acpi_add_single_object(&device, handle, type, sta,
> > -                                      ACPI_BUS_ADD_BASIC);
> > -               if (!device)
> > -                       return AE_CTRL_DEPTH;
> > +       acpi_add_single_object(&device, handle, type, sta, ACPI_BUS_ADD_BASIC);
> > +       if (!device)
> > +               return AE_CTRL_DEPTH;
> >
> > -               device->add_type = context ?
> > -                                       ACPI_BUS_ADD_START : ACPI_BUS_ADD_MATCH;
> > -       }
> > +       device->add_type = context ? ACPI_BUS_ADD_START : ACPI_BUS_ADD_MATCH;
> >
> > + out:
> >         if (!*return_value)
> >                 *return_value = device;
> 
> I think all (both) callers of acpi_bus_check_add() supply a
> return_value pointer, so you could just remove the test.

Patch

Index: linux/drivers/acpi/scan.c
===================================================================
--- linux.orig/drivers/acpi/scan.c
+++ linux/drivers/acpi/scan.c
@@ -1582,6 +1582,10 @@  static acpi_status acpi_bus_check_add(ac
 	acpi_status status;
 	int result;
 
+	acpi_bus_get_device(handle, &device);
+	if (device)
+		goto out;
+
 	result = acpi_bus_type_and_status(handle, &type, &sta);
 	if (result)
 		return AE_OK;
@@ -1602,17 +1606,13 @@  static acpi_status acpi_bus_check_add(ac
 	 * We may already have an acpi_device from a previous enumeration.  If
 	 * so, we needn't add it again, but we may still have to start it.
 	 */
-	acpi_bus_get_device(handle, &device);
-	if (!device) {
-		acpi_add_single_object(&device, handle, type, sta,
-				       ACPI_BUS_ADD_BASIC);
-		if (!device)
-			return AE_CTRL_DEPTH;
+	acpi_add_single_object(&device, handle, type, sta, ACPI_BUS_ADD_BASIC);
+	if (!device)
+		return AE_CTRL_DEPTH;
 
-		device->add_type = context ?
-					ACPI_BUS_ADD_START : ACPI_BUS_ADD_MATCH;
-	}
+	device->add_type = context ? ACPI_BUS_ADD_START : ACPI_BUS_ADD_MATCH;
 
+ out:
 	if (!*return_value)
 		*return_value = device;