[PATCH 0/2] ACPI / scan: Additional changes

[PATCH 0/2] ACPI / scan: Additional changes

[Rafael J. Wysocki]

Hi All,

The following patches make two more changes to the ACPI namespace scanning
code that I think are more-or-less useful:

[1/2] Introduce lock to prevent acpi_bus_scan() from running in parallel with
      acpi_bus_trim() to avoid removing device nodes while we're setting
      them up.
[2/2] Change the scanning of fixed device nodes to work along the lines of
      acpi_bus_scan().

Patch [1/2] should apply on top of the acpi-scan branch, but [2/2] needs the
entire linux-pm.git/linux-next branch.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 1/2] ACPI / scan: Make namespace scanning and trimming mutually exclusive

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There is no guarantee that acpi_bus_scan() and acpi_bus_trim() will
not be run in parallel for the same scope of the ACPI namespace,
which may lead to a great deal of confusion, so introduce a new mutex
to prevent that from happening.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -52,6 +52,7 @@ static const struct acpi_device_id acpi_
 
 static LIST_HEAD(acpi_device_list);
 static LIST_HEAD(acpi_bus_id_list);
+static DEFINE_MUTEX(acpi_scan_lock);
 DEFINE_MUTEX(acpi_device_lock);
 LIST_HEAD(acpi_wakeup_device_list);
 
@@ -1612,19 +1613,22 @@ static acpi_status acpi_bus_device_attac
 int acpi_bus_scan(acpi_handle handle)
 {
 	void *device = NULL;
+	int error = 0;
+
+	mutex_lock(&acpi_scan_lock);
 
 	if (ACPI_SUCCESS(acpi_bus_check_add(handle, 0, NULL, &device)))
 		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
 				    acpi_bus_check_add, NULL, NULL, &device);
 
 	if (!device)
-		return -ENODEV;
-
-	if (ACPI_SUCCESS(acpi_bus_device_attach(handle, 0, NULL, NULL)))
+		error = -ENODEV;
+	else if (ACPI_SUCCESS(acpi_bus_device_attach(handle, 0, NULL, NULL)))
 		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
 				    acpi_bus_device_attach, NULL, NULL, NULL);
 
-	return 0;
+	mutex_unlock(&acpi_scan_lock);
+	return error;
 }
 EXPORT_SYMBOL(acpi_bus_scan);
 
@@ -1653,6 +1657,8 @@ static acpi_status acpi_bus_remove(acpi_
 
 void acpi_bus_trim(struct acpi_device *start)
 {
+	mutex_lock(&acpi_scan_lock);
+
 	/*
 	 * Execute acpi_bus_device_detach() as a post-order callback to detach
 	 * all ACPI drivers from the device nodes being removed.
@@ -1667,6 +1673,8 @@ void acpi_bus_trim(struct acpi_device *s
 	acpi_walk_namespace(ACPI_TYPE_ANY, start->handle, ACPI_UINT32_MAX, NULL,
 			    acpi_bus_remove, NULL, NULL);
 	acpi_bus_remove(start->handle, 0, NULL, NULL);
+
+	mutex_unlock(&acpi_scan_lock);
 }
 EXPORT_SYMBOL_GPL(acpi_bus_trim);
 

[PATCH 2/2] ACPI / scan: Make scanning of fixed devices follow the general scheme

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make acpi_bus_scan_fixed() use device_attach() directly to attach
drivers, if any, to the fixed devices in analogy with how
acpi_bus_scan() works, which allows the last argument of
acpi_add_single_object() to be dropped and the manipulation of the
flags.match_driver bit to be moved to acpi_init_device_object()
and acpi_device_add_finalize().

After these changes all of the functions for the initialization
and registration of struct acpi_device objects work in the same
way for all of them.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1437,19 +1437,21 @@ void acpi_init_device_object(struct acpi
 	acpi_device_get_busid(device);
 	acpi_device_set_id(device);
 	acpi_bus_get_flags(device);
+	device->flags.match_driver = false;
 	device_initialize(&device->dev);
 	dev_set_uevent_suppress(&device->dev, true);
 }
 
 void acpi_device_add_finalize(struct acpi_device *device)
 {
+	device->flags.match_driver = true;
 	dev_set_uevent_suppress(&device->dev, false);
 	kobject_uevent(&device->dev.kobj, KOBJ_ADD);
 }
 
 static int acpi_add_single_object(struct acpi_device **child,
 				  acpi_handle handle, int type,
-				  unsigned long long sta, bool match_driver)
+				  unsigned long long sta)
 {
 	int result;
 	struct acpi_device *device;
@@ -1465,7 +1467,6 @@ static int acpi_add_single_object(struct
 	acpi_bus_get_power_flags(device);
 	acpi_bus_get_wakeup_device_flags(device);
 
-	device->flags.match_driver = match_driver;
 	result = acpi_device_add(device, acpi_device_release);
 	if (result) {
 		acpi_device_release(&device->dev);
@@ -1558,12 +1559,10 @@ static acpi_status acpi_bus_check_add(ac
 		return AE_CTRL_DEPTH;
 	}
 
-	acpi_add_single_object(&device, handle, type, sta, false);
+	acpi_add_single_object(&device, handle, type, sta);
 	if (!device)
 		return AE_CTRL_DEPTH;
 
-	device->flags.match_driver = true;
-
  out:
 	if (!*return_value)
 		*return_value = device;
@@ -1681,25 +1680,39 @@ EXPORT_SYMBOL_GPL(acpi_bus_trim);
 static int acpi_bus_scan_fixed(void)
 {
 	int result = 0;
-	struct acpi_device *device = NULL;
 
 	/*
 	 * Enumerate all fixed-feature devices.
 	 */
-	if ((acpi_gbl_FADT.flags & ACPI_FADT_POWER_BUTTON) == 0) {
+	if (!(acpi_gbl_FADT.flags & ACPI_FADT_POWER_BUTTON)) {
+		struct acpi_device *device = NULL;
+
 		result = acpi_add_single_object(&device, NULL,
 						ACPI_BUS_TYPE_POWER_BUTTON,
-						ACPI_STA_DEFAULT, true);
+						ACPI_STA_DEFAULT);
+		if (result)
+			return result;
+
+		result = device_attach(&device->dev);
+		if (result < 0)
+			return result;
+
 		device_init_wakeup(&device->dev, true);
 	}
 
-	if ((acpi_gbl_FADT.flags & ACPI_FADT_SLEEP_BUTTON) == 0) {
+	if (!(acpi_gbl_FADT.flags & ACPI_FADT_SLEEP_BUTTON)) {
+		struct acpi_device *device = NULL;
+
 		result = acpi_add_single_object(&device, NULL,
 						ACPI_BUS_TYPE_SLEEP_BUTTON,
-						ACPI_STA_DEFAULT, true);
+						ACPI_STA_DEFAULT);
+		if (result)
+			return result;
+
+		result = device_attach(&device->dev);
 	}
 
-	return result;
+	return result < 0 ? result : 0;
 }
 
 int __init acpi_scan_init(void)

Re: [PATCH 1/2] ACPI / scan: Make namespace scanning and trimming mutually exclusive

[Yinghai Lu]

On Sat, Jan 26, 2013 at 2:41 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> There is no guarantee that acpi_bus_scan() and acpi_bus_trim() will
> not be run in parallel for the same scope of the ACPI namespace,
> which may lead to a great deal of confusion, so introduce a new mutex
> to prevent that from happening.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Yinghai Lu <yinghai@kernel.org>

Steven,

Can you apply this one to for-pci-res-alloc to check if racing with
docking hotplug/eject
still happen?
or wait one or two days after i rebase that branch.

Thanks

Yinghai

> ---
>  drivers/acpi/scan.c |   16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -52,6 +52,7 @@ static const struct acpi_device_id acpi_
>
>  static LIST_HEAD(acpi_device_list);
>  static LIST_HEAD(acpi_bus_id_list);
> +static DEFINE_MUTEX(acpi_scan_lock);
>  DEFINE_MUTEX(acpi_device_lock);
>  LIST_HEAD(acpi_wakeup_device_list);
>
> @@ -1612,19 +1613,22 @@ static acpi_status acpi_bus_device_attac
>  int acpi_bus_scan(acpi_handle handle)
>  {
>         void *device = NULL;
> +       int error = 0;
> +
> +       mutex_lock(&acpi_scan_lock);
>
>         if (ACPI_SUCCESS(acpi_bus_check_add(handle, 0, NULL, &device)))
>                 acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
>                                     acpi_bus_check_add, NULL, NULL, &device);
>
>         if (!device)
> -               return -ENODEV;
> -
> -       if (ACPI_SUCCESS(acpi_bus_device_attach(handle, 0, NULL, NULL)))
> +               error = -ENODEV;
> +       else if (ACPI_SUCCESS(acpi_bus_device_attach(handle, 0, NULL, NULL)))
>                 acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
>                                     acpi_bus_device_attach, NULL, NULL, NULL);
>
> -       return 0;
> +       mutex_unlock(&acpi_scan_lock);
> +       return error;
>  }
>  EXPORT_SYMBOL(acpi_bus_scan);
>
> @@ -1653,6 +1657,8 @@ static acpi_status acpi_bus_remove(acpi_
>
>  void acpi_bus_trim(struct acpi_device *start)
>  {
> +       mutex_lock(&acpi_scan_lock);
> +
>         /*
>          * Execute acpi_bus_device_detach() as a post-order callback to detach
>          * all ACPI drivers from the device nodes being removed.
> @@ -1667,6 +1673,8 @@ void acpi_bus_trim(struct acpi_device *s
>         acpi_walk_namespace(ACPI_TYPE_ANY, start->handle, ACPI_UINT32_MAX, NULL,
>                             acpi_bus_remove, NULL, NULL);
>         acpi_bus_remove(start->handle, 0, NULL, NULL);
> +
> +       mutex_unlock(&acpi_scan_lock);
>  }
>  EXPORT_SYMBOL_GPL(acpi_bus_trim);
>
>

Re: [PATCH 2/2] ACPI / scan: Make scanning of fixed devices follow the general scheme

[Yinghai Lu]

On Sat, Jan 26, 2013 at 2:43 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Make acpi_bus_scan_fixed() use device_attach() directly to attach
> drivers, if any, to the fixed devices in analogy with how
> acpi_bus_scan() works, which allows the last argument of
> acpi_add_single_object() to be dropped and the manipulation of the
> flags.match_driver bit to be moved to acpi_init_device_object()
> and acpi_device_add_finalize().
>
> After these changes all of the functions for the initialization
> and registration of struct acpi_device objects work in the same
> way for all of them.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/scan.c |   35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
>
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -1437,19 +1437,21 @@ void acpi_init_device_object(struct acpi
>         acpi_device_get_busid(device);
>         acpi_device_set_id(device);
>         acpi_bus_get_flags(device);
> +       device->flags.match_driver = false;
>         device_initialize(&device->dev);
>         dev_set_uevent_suppress(&device->dev, true);
>  }
>
>  void acpi_device_add_finalize(struct acpi_device *device)
>  {
> +       device->flags.match_driver = true;
>         dev_set_uevent_suppress(&device->dev, false);
>         kobject_uevent(&device->dev.kobj, KOBJ_ADD);
>  }
>
>  static int acpi_add_single_object(struct acpi_device **child,
>                                   acpi_handle handle, int type,
> -                                 unsigned long long sta, bool match_driver)
> +                                 unsigned long long sta)
>  {
>         int result;
>         struct acpi_device *device;
> @@ -1465,7 +1467,6 @@ static int acpi_add_single_object(struct
>         acpi_bus_get_power_flags(device);
>         acpi_bus_get_wakeup_device_flags(device);
>
> -       device->flags.match_driver = match_driver;
>         result = acpi_device_add(device, acpi_device_release);
>         if (result) {
>                 acpi_device_release(&device->dev);
> @@ -1558,12 +1559,10 @@ static acpi_status acpi_bus_check_add(ac
>                 return AE_CTRL_DEPTH;
>         }
>
> -       acpi_add_single_object(&device, handle, type, sta, false);
> +       acpi_add_single_object(&device, handle, type, sta);
>         if (!device)
>                 return AE_CTRL_DEPTH;
>
> -       device->flags.match_driver = true;
> -
>   out:
>         if (!*return_value)
>                 *return_value = device;

nice cleanup.

Acked-by: Yinghai Lu <yinghai@kernel.org>

> @@ -1681,25 +1680,39 @@ EXPORT_SYMBOL_GPL(acpi_bus_trim);
>  static int acpi_bus_scan_fixed(void)
>  {
>         int result = 0;
> -       struct acpi_device *device = NULL;
>
>         /*
>          * Enumerate all fixed-feature devices.
>          */
> -       if ((acpi_gbl_FADT.flags & ACPI_FADT_POWER_BUTTON) == 0) {
> +       if (!(acpi_gbl_FADT.flags & ACPI_FADT_POWER_BUTTON)) {
> +               struct acpi_device *device = NULL;
> +
>                 result = acpi_add_single_object(&device, NULL,
>                                                 ACPI_BUS_TYPE_POWER_BUTTON,
> -                                               ACPI_STA_DEFAULT, true);
> +                                               ACPI_STA_DEFAULT);
> +               if (result)
> +                       return result;
> +
> +               result = device_attach(&device->dev);
> +               if (result < 0)
> +                       return result;
> +
>                 device_init_wakeup(&device->dev, true);
>         }
>
> -       if ((acpi_gbl_FADT.flags & ACPI_FADT_SLEEP_BUTTON) == 0) {
> +       if (!(acpi_gbl_FADT.flags & ACPI_FADT_SLEEP_BUTTON)) {
> +               struct acpi_device *device = NULL;
> +
>                 result = acpi_add_single_object(&device, NULL,
>                                                 ACPI_BUS_TYPE_SLEEP_BUTTON,
> -                                               ACPI_STA_DEFAULT, true);
> +                                               ACPI_STA_DEFAULT);
> +               if (result)
> +                       return result;
> +
> +               result = device_attach(&device->dev);
>         }
>
> -       return result;
> +       return result < 0 ? result : 0;
>  }
>
>  int __init acpi_scan_init(void)
>

Re: [PATCH 1/2] ACPI / scan: Make namespace scanning and trimming mutually exclusive

[Steven Newbury]

On Sat, 2013-01-26 at 15:19 -0800, Yinghai Lu wrote:
> On Sat, Jan 26, 2013 at 2:41 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > There is no guarantee that acpi_bus_scan() and acpi_bus_trim() will
> > not be run in parallel for the same scope of the ACPI namespace,
> > which may lead to a great deal of confusion, so introduce a new mutex
> > to prevent that from happening.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Acked-by: Yinghai Lu <yinghai@kernel.org>
> 
> Steven,
> 
> Can you apply this one to for-pci-res-alloc to check if racing with
> docking hotplug/eject
> still happen?
> or wait one or two days after i rebase that branch.
> 
Still get the corrupt acpi_handle.

> Thanks
> 
> Yinghai
> 
> > ---
> >  drivers/acpi/scan.c |   16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -52,6 +52,7 @@ static const struct acpi_device_id acpi_
> >
> >  static LIST_HEAD(acpi_device_list);
> >  static LIST_HEAD(acpi_bus_id_list);
> > +static DEFINE_MUTEX(acpi_scan_lock);
> >  DEFINE_MUTEX(acpi_device_lock);
> >  LIST_HEAD(acpi_wakeup_device_list);
> >
> > @@ -1612,19 +1613,22 @@ static acpi_status acpi_bus_device_attac
> >  int acpi_bus_scan(acpi_handle handle)
> >  {
> >         void *device = NULL;
> > +       int error = 0;
> > +
> > +       mutex_lock(&acpi_scan_lock);
> >
> >         if (ACPI_SUCCESS(acpi_bus_check_add(handle, 0, NULL, &device)))
> >                 acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
> >                                     acpi_bus_check_add, NULL, NULL, &device);
> >
> >         if (!device)
> > -               return -ENODEV;
> > -
> > -       if (ACPI_SUCCESS(acpi_bus_device_attach(handle, 0, NULL, NULL)))
> > +               error = -ENODEV;
> > +       else if (ACPI_SUCCESS(acpi_bus_device_attach(handle, 0, NULL, NULL)))
> >                 acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
> >                                     acpi_bus_device_attach, NULL, NULL, NULL);
> >
> > -       return 0;
> > +       mutex_unlock(&acpi_scan_lock);
> > +       return error;
> >  }
> >  EXPORT_SYMBOL(acpi_bus_scan);
> >
> > @@ -1653,6 +1657,8 @@ static acpi_status acpi_bus_remove(acpi_
> >
> >  void acpi_bus_trim(struct acpi_device *start)
> >  {
> > +       mutex_lock(&acpi_scan_lock);
> > +
> >         /*
> >          * Execute acpi_bus_device_detach() as a post-order callback to detach
> >          * all ACPI drivers from the device nodes being removed.
> > @@ -1667,6 +1673,8 @@ void acpi_bus_trim(struct acpi_device *s
> >         acpi_walk_namespace(ACPI_TYPE_ANY, start->handle, ACPI_UINT32_MAX, NULL,
> >                             acpi_bus_remove, NULL, NULL);
> >         acpi_bus_remove(start->handle, 0, NULL, NULL);
> > +
> > +       mutex_unlock(&acpi_scan_lock);
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_bus_trim);
> >
> >

Re: [PATCH 1/2] ACPI / scan: Make namespace scanning and trimming mutually exclusive

[Steven Newbury]

On Sat, 2013-01-26 at 15:19 -0800, Yinghai Lu wrote:
> On Sat, Jan 26, 2013 at 2:41 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > There is no guarantee that acpi_bus_scan() and acpi_bus_trim() will
> > not be run in parallel for the same scope of the ACPI namespace,
> > which may lead to a great deal of confusion, so introduce a new mutex
> > to prevent that from happening.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Acked-by: Yinghai Lu <yinghai@kernel.org>
> 
> Steven,
> 
> Can you apply this one to for-pci-res-alloc to check if racing with
> docking hotplug/eject
> still happen?
> or wait one or two days after i rebase that branch.

Tried merging with linux-pm/bleeding-edge, same behaviour:

[ 3589.013578] ACPI: \_SB_.PCI0.PCIE.GDCK: undocking
[ 3589.585356] vgaarb: device changed decodes: PCI:0000:00:02.0,olddecodes=none,decodes=io+mem:owns=none
[ 3589.585422] ACPI: Delete PCI Interrupt Routing Table for 0000:04
[ 3589.585426] pci 0000:03:08.0: Oops, 'acpi_handle' corrupt
[ 3589.585446] pci_bus 0000:04: busn_res: [bus 04] is released

03:08.0 PCI bridge: PLX Technology, Inc. PEX8112 x1 Lane PCI
Express-to-PCI Bridge (rev aa)
04:00.0 VGA compatible controller: Advanced Micro Devices [AMD] nee ATI
Manhattan [Mobility Radeon HD 5430 Series]
04:00.1 Audio device: Advanced Micro Devices [AMD] nee ATI Cedar HDMI
Audio [Radeon HD 5400/6300 Series]


> > ---
> >  drivers/acpi/scan.c |   16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -52,6 +52,7 @@ static const struct acpi_device_id acpi_
> >
> >  static LIST_HEAD(acpi_device_list);
> >  static LIST_HEAD(acpi_bus_id_list);
> > +static DEFINE_MUTEX(acpi_scan_lock);
> >  DEFINE_MUTEX(acpi_device_lock);
> >  LIST_HEAD(acpi_wakeup_device_list);
> >
> > @@ -1612,19 +1613,22 @@ static acpi_status acpi_bus_device_attac
> >  int acpi_bus_scan(acpi_handle handle)
> >  {
> >         void *device = NULL;
> > +       int error = 0;
> > +
> > +       mutex_lock(&acpi_scan_lock);
> >
> >         if (ACPI_SUCCESS(acpi_bus_check_add(handle, 0, NULL, &device)))
> >                 acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
> >                                     acpi_bus_check_add, NULL, NULL, &device);
> >
> >         if (!device)
> > -               return -ENODEV;
> > -
> > -       if (ACPI_SUCCESS(acpi_bus_device_attach(handle, 0, NULL, NULL)))
> > +               error = -ENODEV;
> > +       else if (ACPI_SUCCESS(acpi_bus_device_attach(handle, 0, NULL, NULL)))
> >                 acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
> >                                     acpi_bus_device_attach, NULL, NULL, NULL);
> >
> > -       return 0;
> > +       mutex_unlock(&acpi_scan_lock);
> > +       return error;
> >  }
> >  EXPORT_SYMBOL(acpi_bus_scan);
> >
> > @@ -1653,6 +1657,8 @@ static acpi_status acpi_bus_remove(acpi_
> >
> >  void acpi_bus_trim(struct acpi_device *start)
> >  {
> > +       mutex_lock(&acpi_scan_lock);
> > +
> >         /*
> >          * Execute acpi_bus_device_detach() as a post-order callback to detach
> >          * all ACPI drivers from the device nodes being removed.
> > @@ -1667,6 +1673,8 @@ void acpi_bus_trim(struct acpi_device *s
> >         acpi_walk_namespace(ACPI_TYPE_ANY, start->handle, ACPI_UINT32_MAX, NULL,
> >                             acpi_bus_remove, NULL, NULL);
> >         acpi_bus_remove(start->handle, 0, NULL, NULL);
> > +
> > +       mutex_unlock(&acpi_scan_lock);
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_bus_trim);
> >
> >

Re: [PATCH 1/2] ACPI / scan: Make namespace scanning and trimming mutually exclusive

[Rafael J. Wysocki]

On Saturday, February 02, 2013 11:58:41 AM Steven Newbury wrote:
> On Sat, 2013-01-26 at 15:19 -0800, Yinghai Lu wrote:
> > On Sat, Jan 26, 2013 at 2:41 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > There is no guarantee that acpi_bus_scan() and acpi_bus_trim() will
> > > not be run in parallel for the same scope of the ACPI namespace,
> > > which may lead to a great deal of confusion, so introduce a new mutex
> > > to prevent that from happening.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Acked-by: Yinghai Lu <yinghai@kernel.org>
> > 
> > Steven,
> > 
> > Can you apply this one to for-pci-res-alloc to check if racing with
> > docking hotplug/eject
> > still happen?
> > or wait one or two days after i rebase that branch.
> 
> Tried merging with linux-pm/bleeding-edge, same behaviour:
> 
> [ 3589.013578] ACPI: \_SB_.PCI0.PCIE.GDCK: undocking
> [ 3589.585356] vgaarb: device changed decodes: PCI:0000:00:02.0,olddecodes=none,decodes=io+mem:owns=none
> [ 3589.585422] ACPI: Delete PCI Interrupt Routing Table for 0000:04
> [ 3589.585426] pci 0000:03:08.0: Oops, 'acpi_handle' corrupt
> [ 3589.585446] pci_bus 0000:04: busn_res: [bus 04] is released
> 
> 03:08.0 PCI bridge: PLX Technology, Inc. PEX8112 x1 Lane PCI
> Express-to-PCI Bridge (rev aa)
> 04:00.0 VGA compatible controller: Advanced Micro Devices [AMD] nee ATI
> Manhattan [Mobility Radeon HD 5430 Series]
> 04:00.1 Audio device: Advanced Micro Devices [AMD] nee ATI Cedar HDMI
> Audio [Radeon HD 5400/6300 Series]

That's because of a bug in the dock code I believe.

If you're willing to test patches, I can try to cook up something to debug/fix
this.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/2] ACPI / scan: Make namespace scanning and trimming mutually exclusive

[Steven Newbury]

On Sat,   2 Feb 2013, 20:18:28 GMT, Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Saturday, February 02, 2013 11:58:41 AM Steven Newbury wrote:
> > On Sat, 2013-01-26 at 15:19 -0800, Yinghai Lu wrote:
> > > On Sat, Jan 26, 2013 at 2:41 PM, Rafael J. Wysocki <rjw@sisk.pl>
> > > wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > > There is no guarantee that acpi_bus_scan() and acpi_bus_trim() will
> > > > not be run in parallel for the same scope of the ACPI namespace,
> > > > which may lead to a great deal of confusion, so introduce a new
> > > > mutex to prevent that from happening.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > Acked-by: Yinghai Lu <yinghai@kernel.org>
> > > 
> > > Steven,
> > > 
> > > Can you apply this one to for-pci-res-alloc to check if racing with
> > > docking hotplug/eject
> > > still happen?
> > > or wait one or two days after i rebase that branch.
> > 
> > Tried merging with linux-pm/bleeding-edge, same behaviour:
> > 
> > [ 3589.013578] ACPI: \_SB_.PCI0.PCIE.GDCK: undocking
> > [ 3589.585356] vgaarb: device changed decodes:
> > PCI:0000:00:02.0,olddecodes=none,decodes=io+mem:owns=none [
> > 3589.585422] ACPI: Delete PCI Interrupt Routing Table for 0000:04 [
> > 3589.585426] pci 0000:03:08.0: Oops, 'acpi_handle' corrupt [
> > 3589.585446] pci_bus 0000:04: busn_res: [bus 04] is released
> > 
> > 03:08.0 PCI bridge: PLX Technology, Inc. PEX8112 x1 Lane PCI
> > Express-to-PCI Bridge (rev aa)
> > 04:00.0 VGA compatible controller: Advanced Micro Devices [AMD] nee ATI
> > Manhattan [Mobility Radeon HD 5430 Series]
> > 04:00.1 Audio device: Advanced Micro Devices [AMD] nee ATI Cedar HDMI
> > Audio [Radeon HD 5400/6300 Series]
> 
> That's because of a bug in the dock code I believe.
> 
> If you're willing to test patches, I can try to cook up something to
> debug/fix this.
Sure, I'm always willing to test patches.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ACPI / scan: Make namespace scanning and trimming mutually exclusive

[Steven Newbury]

On Sat,   2 Feb 2013, 20:18:28 GMT, Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Saturday, February 02, 2013 11:58:41 AM Steven Newbury wrote:
> > On Sat, 2013-01-26 at 15:19 -0800, Yinghai Lu wrote:
> > > On Sat, Jan 26, 2013 at 2:41 PM, Rafael J. Wysocki <rjw@sisk.pl>
> > > wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > > There is no guarantee that acpi_bus_scan() and acpi_bus_trim() will
> > > > not be run in parallel for the same scope of the ACPI namespace,
> > > > which may lead to a great deal of confusion, so introduce a new
> > > > mutex to prevent that from happening.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > Acked-by: Yinghai Lu <yinghai@kernel.org>
> > > 
> > > Steven,
> > > 
> > > Can you apply this one to for-pci-res-alloc to check if racing with
> > > docking hotplug/eject
> > > still happen?
> > > or wait one or two days after i rebase that branch.
> > 
> > Tried merging with linux-pm/bleeding-edge, same behaviour:
> > 
> > [ 3589.013578] ACPI: \_SB_.PCI0.PCIE.GDCK: undocking
> > [ 3589.585356] vgaarb: device changed decodes:
> > PCI:0000:00:02.0,olddecodes=none,decodes=io+mem:owns=none [
> > 3589.585422] ACPI: Delete PCI Interrupt Routing Table for 0000:04 [
> > 3589.585426] pci 0000:03:08.0: Oops, 'acpi_handle' corrupt [
> > 3589.585446] pci_bus 0000:04: busn_res: [bus 04] is released
> > 
> > 03:08.0 PCI bridge: PLX Technology, Inc. PEX8112 x1 Lane PCI
> > Express-to-PCI Bridge (rev aa)
> > 04:00.0 VGA compatible controller: Advanced Micro Devices [AMD] nee ATI
> > Manhattan [Mobility Radeon HD 5430 Series]
> > 04:00.1 Audio device: Advanced Micro Devices [AMD] nee ATI Cedar HDMI
> > Audio [Radeon HD 5400/6300 Series]
> 
> That's because of a bug in the dock code I believe.
> 
> If you're willing to test patches, I can try to cook up something to
> debug/fix this.
Sure, I'm always willing to test patches.

Re: [PATCH 1/2] ACPI / scan: Make namespace scanning and trimming mutually exclusive

[Rafael J. Wysocki]

On Saturday, February 02, 2013 08:28:01 PM Steven Newbury wrote:
> On Sat,   2 Feb 2013, 20:18:28 GMT, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > On Saturday, February 02, 2013 11:58:41 AM Steven Newbury wrote:
> > > On Sat, 2013-01-26 at 15:19 -0800, Yinghai Lu wrote:
> > > > On Sat, Jan 26, 2013 at 2:41 PM, Rafael J. Wysocki <rjw@sisk.pl>
> > > > wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > 
> > > > > There is no guarantee that acpi_bus_scan() and acpi_bus_trim() will
> > > > > not be run in parallel for the same scope of the ACPI namespace,
> > > > > which may lead to a great deal of confusion, so introduce a new
> > > > > mutex to prevent that from happening.
> > > > > 
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > > Acked-by: Yinghai Lu <yinghai@kernel.org>
> > > > 
> > > > Steven,
> > > > 
> > > > Can you apply this one to for-pci-res-alloc to check if racing with
> > > > docking hotplug/eject
> > > > still happen?
> > > > or wait one or two days after i rebase that branch.
> > > 
> > > Tried merging with linux-pm/bleeding-edge, same behaviour:
> > > 
> > > [ 3589.013578] ACPI: \_SB_.PCI0.PCIE.GDCK: undocking
> > > [ 3589.585356] vgaarb: device changed decodes:
> > > PCI:0000:00:02.0,olddecodes=none,decodes=io+mem:owns=none [
> > > 3589.585422] ACPI: Delete PCI Interrupt Routing Table for 0000:04 [
> > > 3589.585426] pci 0000:03:08.0: Oops, 'acpi_handle' corrupt [
> > > 3589.585446] pci_bus 0000:04: busn_res: [bus 04] is released
> > > 
> > > 03:08.0 PCI bridge: PLX Technology, Inc. PEX8112 x1 Lane PCI
> > > Express-to-PCI Bridge (rev aa)
> > > 04:00.0 VGA compatible controller: Advanced Micro Devices [AMD] nee ATI
> > > Manhattan [Mobility Radeon HD 5430 Series]
> > > 04:00.1 Audio device: Advanced Micro Devices [AMD] nee ATI Cedar HDMI
> > > Audio [Radeon HD 5400/6300 Series]
> > 
> > That's because of a bug in the dock code I believe.
> > 
> > If you're willing to test patches, I can try to cook up something to
> > debug/fix this.
> Sure, I'm always willing to test patches.

Can you please test this one in addition to the $subject one:

https://patchwork.kernel.org/patch/2068621/

and see if that helps?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/2] ACPI / scan: Make namespace scanning and trimming mutually exclusive

[Steven Newbury]

On Sat, 2013-02-02 at 23:27 +0100, Rafael J. Wysocki wrote:
> On Saturday, February 02, 2013 08:28:01 PM Steven Newbury wrote:
> > > > Tried merging with linux-pm/bleeding-edge, same behaviour:
> > > > 
> > > > [ 3589.013578] ACPI: \_SB_.PCI0.PCIE.GDCK: undocking
> > > > [ 3589.585356] vgaarb: device changed decodes:
> > > > PCI:0000:00:02.0,olddecodes=none,decodes=io+mem:owns=none [
> > > > 3589.585422] ACPI: Delete PCI Interrupt Routing Table for 0000:04 [
> > > > 3589.585426] pci 0000:03:08.0: Oops, 'acpi_handle' corrupt [
> > > > 3589.585446] pci_bus 0000:04: busn_res: [bus 04] is released
> > > > 
> > > > 03:08.0 PCI bridge: PLX Technology, Inc. PEX8112 x1 Lane PCI
> > > > Express-to-PCI Bridge (rev aa)
> > > > 04:00.0 VGA compatible controller: Advanced Micro Devices [AMD] nee ATI
> > > > Manhattan [Mobility Radeon HD 5430 Series]
> > > > 04:00.1 Audio device: Advanced Micro Devices [AMD] nee ATI Cedar HDMI
> > > > Audio [Radeon HD 5400/6300 Series]
> > > 
> > > That's because of a bug in the dock code I believe.
> > > 
> > > If you're willing to test patches, I can try to cook up something to
> > > debug/fix this.
> > Sure, I'm always willing to test patches.
> 
> Can you please test this one in addition to the $subject one:
> 
> https://patchwork.kernel.org/patch/2068621/
> 
> and see if that helps?
No change wrt the "Oops, 'acpi_handle' corrupt".

Just to be clear, this happens during the logical undock phase after
writing to "/sys/devices/platform/dock.0/undock".

Right now I manually tear down the seat associated with the dock gfx
card (waiting for the X server to terminate and requiring SIGSTOPing the
gdm process to stop it respawning!) then attempt radeon module unload,
if it succeeds I write to the undock file, otherwise I fail the undock;
I saw there is intention to put it infrastructure to have drivers
prepare themselves for removal on 'eject' request, that would be really
helpful.

A couple of other things regarding the dock system:

Making an undock/eject request with the dock eject button results in an
ACPI 'undock' event, same ACPI event happens when the
electrical/physical undock occurs.

The IDE docks seem not to be working; maybe I need to turn on some
debugging.  This used to work at some point.

Feb  2 23:51:28 infinity kernel: ACPI: \_SB_.PCI0.IDE1.PRI_.MAST:
docking
Feb  2 23:51:28 infinity kernel: ACPI: \_SB_.PCI0.IDE1.PRI_.MAST: Unable
to dock!