[PATCH 0/4] ACPI / dock: Cleanups and simplifications

[PATCH 0/4] ACPI / dock: Cleanups and simplifications

[Rafael J. Wysocki]

Hi,

The following patches clean up and simplify dock.c slightly.

They conflict with the Gerry's changes currently, but I'll fix them up if
everyone is comfortable with the changes.

[1/4] Remove hp_lock from struct dock_station.
[2/4] Rework find_dock_devices() (so that it doesn't worry about parents).
[3/4] Walk the list of dependent devices in reverse order on removal.
[4/4] Simplify dock_init_hotplug() and dock_release_hotplug()

Patches [1-3/4] have been tested by Alexander Patrakov on his Sony Vaio with
a docking station and he haven't seen any problems related to them.

Thanks,
Rafael


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

[PATCH 1/4] ACPI / dock: Drop the hp_lock mutex from struct dock_station

[Rafael J. Wysocki]

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

The only existing user of the hp_lock mutex in struct dock_station,
hotplug_dock_devices(), is always called under acpi_scan_lock and
cannot race with another instance of itself, so drop the mutex
which is not necessary.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/dock.c |    5 -----
 1 file changed, 5 deletions(-)

Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -64,7 +64,6 @@ struct dock_station {
 	unsigned long last_dock_time;
 	u32 flags;
 	spinlock_t dd_lock;
-	struct mutex hp_lock;
 	struct list_head dependent_devices;
 
 	struct list_head sibling;
@@ -410,8 +409,6 @@ static void hotplug_dock_devices(struct
 {
 	struct dock_dependent_device *dd;
 
-	mutex_lock(&ds->hp_lock);
-
 	/*
 	 * First call driver specific hotplug functions
 	 */
@@ -430,7 +427,6 @@ static void hotplug_dock_devices(struct
 		else
 			dock_create_acpi_device(dd->handle);
 	}
-	mutex_unlock(&ds->hp_lock);
 }
 
 static void dock_event(struct dock_station *ds, u32 event, int num)
@@ -1004,7 +1000,6 @@ static int __init dock_add(acpi_handle h
 	dock_station->dock_device = dd;
 	dock_station->last_dock_time = jiffies - HZ;
 
-	mutex_init(&dock_station->hp_lock);
 	spin_lock_init(&dock_station->dd_lock);
 	INIT_LIST_HEAD(&dock_station->sibling);
 	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);

[PATCH 2/4] ACPI / dock: Rework and simplify find_dock_devices()

[Rafael J. Wysocki]

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

Since acpi_walk_namespace() calls find_dock_devices() during tree
pre-order visit, the latter doesn't need to add devices whose
parents have _EJD pointing to the docking station to the list of
that station's dependent devices, because those parents are going to
be added to that list anyway and the removal of a parent will take
care of the removal of its children in those cases.

For this reason, rework find_dock_devices() to only call
add_dock_dependent_device() for devices whose _EJD point directy to
the docking station represented by its context argument and simplify
it slightly.

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

Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -855,26 +855,13 @@ static struct notifier_block dock_acpi_n
 static acpi_status
 find_dock_devices(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
-	acpi_status status;
-	acpi_handle tmp, parent;
 	struct dock_station *ds = context;
+	acpi_handle ejd = NULL;
 
-	status = acpi_bus_get_ejd(handle, &tmp);
-	if (ACPI_FAILURE(status)) {
-		/* try the parent device as well */
-		status = acpi_get_parent(handle, &parent);
-		if (ACPI_FAILURE(status))
-			goto fdd_out;
-		/* see if parent is dependent on dock */
-		status = acpi_bus_get_ejd(parent, &tmp);
-		if (ACPI_FAILURE(status))
-			goto fdd_out;
-	}
-
-	if (tmp == ds->handle)
+	acpi_bus_get_ejd(handle, &ejd);
+	if (ejd == ds->handle)
 		add_dock_dependent_device(ds, handle);
 
-fdd_out:
 	return AE_OK;
 }
 

[PATCH 3/4] ACPI / dock: Walk list in reverse order during removal of devices

[Rafael J. Wysocki]

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

If there are indirect dependencies between devices in a dock
station's dependent devices list, they may be broken if the devices
are removed in the same order in which they have been added.

For this reason, make the code in handle_eject_request() walk the
list of dependent devices in reverse order.

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

Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -396,9 +396,29 @@ static void dock_remove_acpi_device(acpi
 }
 
 /**
- * hotplug_dock_devices - insert or remove devices on the dock station
+ * hot_remove_dock_devices - Remove dock station devices.
+ * @ds: Dock station.
+ */
+static void hot_remove_dock_devices(struct dock_station *ds)
+{
+	struct dock_dependent_device *dd;
+
+	/*
+	 * Walk the list in reverse order so that devices that have been added
+	 * last are removed first (in case there are some indirect dependencies
+	 * between them).
+	 */
+	list_for_each_entry_reverse(dd, &ds->dependent_devices, list)
+		dock_hotplug_event(dd, ACPI_NOTIFY_EJECT_REQUEST, false);
+
+	list_for_each_entry_reverse(dd, &ds->dependent_devices, list)
+		dock_remove_acpi_device(dd->handle);
+}
+
+/**
+ * hotplug_dock_devices - Insert devices on a dock station.
  * @ds: the dock station
- * @event: either bus check or eject request
+ * @event: either bus check or device check request
  *
  * Some devices on the dock station need to have drivers called
  * to perform hotplug operations after a dock event has occurred.
@@ -409,24 +429,17 @@ static void hotplug_dock_devices(struct
 {
 	struct dock_dependent_device *dd;
 
-	/*
-	 * First call driver specific hotplug functions
-	 */
+	/* Call driver specific hotplug functions. */
 	list_for_each_entry(dd, &ds->dependent_devices, list)
 		dock_hotplug_event(dd, event, false);
 
 	/*
-	 * Now make sure that an acpi_device is created for each
-	 * dependent device, or removed if this is an eject request.
-	 * This will cause acpi_drivers to be stopped/started if they
-	 * exist
+	 * Now make sure that an acpi_device is created for each dependent
+	 * device.  That will cause scan handlers to be attached to device
+	 * objects or acpi_drivers to be stopped/started if they are present.
 	 */
-	list_for_each_entry(dd, &ds->dependent_devices, list) {
-		if (event == ACPI_NOTIFY_EJECT_REQUEST)
-			dock_remove_acpi_device(dd->handle);
-		else
-			dock_create_acpi_device(dd->handle);
-	}
+	list_for_each_entry(dd, &ds->dependent_devices, list)
+		dock_create_acpi_device(dd->handle);
 }
 
 static void dock_event(struct dock_station *ds, u32 event, int num)
@@ -699,7 +712,7 @@ static int handle_eject_request(struct d
 	 */
 	dock_event(ds, event, UNDOCK_EVENT);
 
-	hotplug_dock_devices(ds, ACPI_NOTIFY_EJECT_REQUEST);
+	hot_remove_dock_devices(ds);
 	undock(ds);
 	dock_lock(ds, 0);
 	eject_dock(ds);

[PATCH 4/4] ACPI / dock: Simplify dock_init_hotplug() and dock_release_hotplug()

[Rafael J. Wysocki]

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

Make dock_init_hotplug() and dock_release_hotplug() slightly simpler
and move some checks in those functions to the code paths where they
are needed.

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

Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -134,19 +134,16 @@ static int dock_init_hotplug(struct dock
 	int ret = 0;
 
 	mutex_lock(&hotplug_lock);
-
-	if (dd->hp_context) {
+	if (WARN_ON(dd->hp_context)) {
 		ret = -EEXIST;
 	} else {
 		dd->hp_refcount = 1;
 		dd->hp_ops = ops;
 		dd->hp_context = context;
 		dd->hp_release = release;
+		if (init)
+			init(context);
 	}
-
-	if (!WARN_ON(ret) && init)
-		init(context);
-
 	mutex_unlock(&hotplug_lock);
 	return ret;
 }
@@ -161,22 +158,17 @@ static int dock_init_hotplug(struct dock
  */
 static void dock_release_hotplug(struct dock_dependent_device *dd)
 {
-	void (*release)(void *) = NULL;
-	void *context = NULL;
-
 	mutex_lock(&hotplug_lock);
-
 	if (dd->hp_context && !--dd->hp_refcount) {
+		void (*release)(void *) = dd->hp_release;
+		void *context = dd->hp_context;
+
 		dd->hp_ops = NULL;
-		context = dd->hp_context;
 		dd->hp_context = NULL;
-		release = dd->hp_release;
 		dd->hp_release = NULL;
+		if (release)
+			release(context);
 	}
-
-	if (release && context)
-		release(context);
-
 	mutex_unlock(&hotplug_lock);
 }
 

Re: [PATCH 2/4] ACPI / dock: Rework and simplify find_dock_devices()

[Yinghai Lu]

On Fri, Jun 28, 2013 at 12:46 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Since acpi_walk_namespace() calls find_dock_devices() during tree
> pre-order visit, the latter doesn't need to add devices whose
> parents have _EJD pointing to the docking station to the list of
> that station's dependent devices, because those parents are going to
> be added to that list anyway and the removal of a parent will take
> care of the removal of its children in those cases.
>
> For this reason, rework find_dock_devices() to only call
> add_dock_dependent_device() for devices whose _EJD point directy to
> the docking station represented by its context argument and simplify
> it slightly.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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

> ---
>  drivers/acpi/dock.c |   19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
>
> Index: linux-pm/drivers/acpi/dock.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/dock.c
> +++ linux-pm/drivers/acpi/dock.c
> @@ -855,26 +855,13 @@ static struct notifier_block dock_acpi_n
>  static acpi_status
>  find_dock_devices(acpi_handle handle, u32 lvl, void *context, void **rv)
>  {
> -       acpi_status status;
> -       acpi_handle tmp, parent;
>         struct dock_station *ds = context;
> +       acpi_handle ejd = NULL;
>
> -       status = acpi_bus_get_ejd(handle, &tmp);
> -       if (ACPI_FAILURE(status)) {
> -               /* try the parent device as well */
> -               status = acpi_get_parent(handle, &parent);
> -               if (ACPI_FAILURE(status))
> -                       goto fdd_out;
> -               /* see if parent is dependent on dock */
> -               status = acpi_bus_get_ejd(parent, &tmp);
> -               if (ACPI_FAILURE(status))
> -                       goto fdd_out;
> -       }
> -
> -       if (tmp == ds->handle)
> +       acpi_bus_get_ejd(handle, &ejd);
> +       if (ejd == ds->handle)
>                 add_dock_dependent_device(ds, handle);
>
> -fdd_out:
>         return AE_OK;
>  }
>
>

Re: [PATCH 3/4] ACPI / dock: Walk list in reverse order during removal of devices

[Yinghai Lu]

On Fri, Jun 28, 2013 at 12:47 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> If there are indirect dependencies between devices in a dock
> station's dependent devices list, they may be broken if the devices
> are removed in the same order in which they have been added.
>
> For this reason, make the code in handle_eject_request() walk the
> list of dependent devices in reverse order.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/dock.c |   45 +++++++++++++++++++++++++++++----------------
>  1 file changed, 29 insertions(+), 16 deletions(-)
>
> Index: linux-pm/drivers/acpi/dock.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/dock.c
> +++ linux-pm/drivers/acpi/dock.c
> @@ -396,9 +396,29 @@ static void dock_remove_acpi_device(acpi
>  }
>
>  /**
> - * hotplug_dock_devices - insert or remove devices on the dock station
> + * hot_remove_dock_devices - Remove dock station devices.
> + * @ds: Dock station.
> + */
> +static void hot_remove_dock_devices(struct dock_station *ds)
> +{
> +       struct dock_dependent_device *dd;
> +
> +       /*
> +        * Walk the list in reverse order so that devices that have been added
> +        * last are removed first (in case there are some indirect dependencies
> +        * between them).
> +        */
> +       list_for_each_entry_reverse(dd, &ds->dependent_devices, list)
> +               dock_hotplug_event(dd, ACPI_NOTIFY_EJECT_REQUEST, false);
> +
> +       list_for_each_entry_reverse(dd, &ds->dependent_devices, list)
> +               dock_remove_acpi_device(dd->handle);
> +}
> +
> +/**
> + * hotplug_dock_devices - Insert devices on a dock station.
>   * @ds: the dock station
> - * @event: either bus check or eject request
> + * @event: either bus check or device check request

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

>   *
>   * Some devices on the dock station need to have drivers called
>   * to perform hotplug operations after a dock event has occurred.
> @@ -409,24 +429,17 @@ static void hotplug_dock_devices(struct
>  {
>         struct dock_dependent_device *dd;
>
> -       /*
> -        * First call driver specific hotplug functions
> -        */
> +       /* Call driver specific hotplug functions. */
>         list_for_each_entry(dd, &ds->dependent_devices, list)
>                 dock_hotplug_event(dd, event, false);
>
>         /*
> -        * Now make sure that an acpi_device is created for each
> -        * dependent device, or removed if this is an eject request.
> -        * This will cause acpi_drivers to be stopped/started if they
> -        * exist
> +        * Now make sure that an acpi_device is created for each dependent
> +        * device.  That will cause scan handlers to be attached to device
> +        * objects or acpi_drivers to be stopped/started if they are present.
>          */
> -       list_for_each_entry(dd, &ds->dependent_devices, list) {
> -               if (event == ACPI_NOTIFY_EJECT_REQUEST)
> -                       dock_remove_acpi_device(dd->handle);
> -               else
> -                       dock_create_acpi_device(dd->handle);
> -       }
> +       list_for_each_entry(dd, &ds->dependent_devices, list)
> +               dock_create_acpi_device(dd->handle);
>  }
>
>  static void dock_event(struct dock_station *ds, u32 event, int num)
> @@ -699,7 +712,7 @@ static int handle_eject_request(struct d
>          */
>         dock_event(ds, event, UNDOCK_EVENT);
>
> -       hotplug_dock_devices(ds, ACPI_NOTIFY_EJECT_REQUEST);
> +       hot_remove_dock_devices(ds);
>         undock(ds);
>         dock_lock(ds, 0);
>         eject_dock(ds);
>

Re: [PATCH 4/4] ACPI / dock: Simplify dock_init_hotplug() and dock_release_hotplug()

[Yinghai Lu]

On Fri, Jun 28, 2013 at 12:48 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Make dock_init_hotplug() and dock_release_hotplug() slightly simpler
> and move some checks in those functions to the code paths where they
> are needed.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/acpi/dock.c |   24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
>
> Index: linux-pm/drivers/acpi/dock.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/dock.c
> +++ linux-pm/drivers/acpi/dock.c
> @@ -134,19 +134,16 @@ static int dock_init_hotplug(struct dock
>         int ret = 0;
>
>         mutex_lock(&hotplug_lock);
> -
> -       if (dd->hp_context) {
> +       if (WARN_ON(dd->hp_context)) {
>                 ret = -EEXIST;
>         } else {
>                 dd->hp_refcount = 1;
>                 dd->hp_ops = ops;
>                 dd->hp_context = context;
>                 dd->hp_release = release;
> +               if (init)
> +                       init(context);
>         }
> -
> -       if (!WARN_ON(ret) && init)
> -               init(context);
> -
>         mutex_unlock(&hotplug_lock);
>         return ret;
>  }
> @@ -161,22 +158,17 @@ static int dock_init_hotplug(struct dock
>   */
>  static void dock_release_hotplug(struct dock_dependent_device *dd)
>  {
> -       void (*release)(void *) = NULL;
> -       void *context = NULL;
> -
>         mutex_lock(&hotplug_lock);
> -
>         if (dd->hp_context && !--dd->hp_refcount) {
> +               void (*release)(void *) = dd->hp_release;
> +               void *context = dd->hp_context;
> +
>                 dd->hp_ops = NULL;
> -               context = dd->hp_context;
>                 dd->hp_context = NULL;
> -               release = dd->hp_release;
>                 dd->hp_release = NULL;
> +               if (release)
> +                       release(context);
>         }
> -
> -       if (release && context)
> -               release(context);
> -
>         mutex_unlock(&hotplug_lock);
>  }
>
>

[PATCH 0/3] ACPI / dock: Notification rework and memory leak fix

[Rafael J. Wysocki]

Hi,

More fixes/cleanups for the ACPI dock driver (and related), on top of the
series I sent earlier today.  [Well, that code is like the Rabbit hole in
"Alice's Adventures in Wonderland".]

[1/3] Rework the dock driver's handling of notifications (the changelog tells
      the whole story).

[2/3] After [1/3] there are no more users of the ACPI bus notifier call chain,
      so drop it.

[3/3] dock_add() leaks memory on errors (although only theoretically, because
      it is very unlikely to fail), so fix that.

Thanks,
Rafael


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

[PATCH 1/3] ACPI / dock: Rework the handling of notifications

[Rafael J. Wysocki]

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

The ACPI dock driver uses register_acpi_bus_notifier() which
installs a notifier triggered globally for all system notifications.
That first of all is inefficient, because the dock driver is only
interested in notifications associated with the devices it handles,
but it has to handle all system notifies for all devices.  Moreover,
it does that even if no docking stations are present in the system
(CONFIG_ACPI_DOCK set is sufficient for that to happen).  Besides,
that is inconvenient, because it requires the driver to do extra work
for each notification to find the target dock station object.

For these reasons, rework the dock driver to install a notify
handler individually for each dock station in the system using
acpi_install_notify_handler().  This allows the dock station
object to be passed directly to the notify handler and makes it
possible to simplify the dock driver quite a bit.  It also
reduces the overhead related to the handling of all system
notifies when CONFIG_ACPI_DOCK is set.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/dock.c |   69 ++++++++++++++++++----------------------------------
 1 file changed, 25 insertions(+), 44 deletions(-)

Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -718,18 +718,17 @@ static int handle_eject_request(struct d
 
 /**
  * dock_notify - act upon an acpi dock notification
- * @handle: the dock station handle
+ * @ds: dock station
  * @event: the acpi event
- * @data: our driver data struct
  *
  * If we are notified to dock, then check to see if the dock is
  * present and then dock.  Notify all drivers of the dock event,
  * and then hotplug and devices that may need hotplugging.
  */
-static void dock_notify(acpi_handle handle, u32 event, void *data)
+static void dock_notify(struct dock_station *ds, u32 event)
 {
-	struct dock_station *ds = data;
-	struct acpi_device *tmp;
+	acpi_handle handle = ds->handle;
+	struct acpi_device *ad;
 	int surprise_removal = 0;
 
 	/*
@@ -752,8 +751,7 @@ static void dock_notify(acpi_handle hand
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
 	case ACPI_NOTIFY_DEVICE_CHECK:
-		if (!dock_in_progress(ds) && acpi_bus_get_device(ds->handle,
-		   &tmp)) {
+		if (!dock_in_progress(ds) && acpi_bus_get_device(handle, &ad)) {
 			begin_dock(ds);
 			dock(ds);
 			if (!dock_present(ds)) {
@@ -790,9 +788,8 @@ static void dock_notify(acpi_handle hand
 }
 
 struct dock_data {
-	acpi_handle handle;
-	unsigned long event;
 	struct dock_station *ds;
+	u32 event;
 };
 
 static void acpi_dock_deferred_cb(void *context)
@@ -800,52 +797,31 @@ static void acpi_dock_deferred_cb(void *
 	struct dock_data *data = context;
 
 	acpi_scan_lock_acquire();
-	dock_notify(data->handle, data->event, data->ds);
+	dock_notify(data->ds, data->event);
 	acpi_scan_lock_release();
 	kfree(data);
 }
 
-static int acpi_dock_notifier_call(struct notifier_block *this,
-	unsigned long event, void *data)
+static void dock_notify_handler(acpi_handle handle, u32 event, void *data)
 {
-	struct dock_station *dock_station;
-	acpi_handle handle = data;
+	struct dock_data *dd;
 
 	if (event != ACPI_NOTIFY_BUS_CHECK && event != ACPI_NOTIFY_DEVICE_CHECK
 	   && event != ACPI_NOTIFY_EJECT_REQUEST)
-		return 0;
-
-	acpi_scan_lock_acquire();
+		return;
 
-	list_for_each_entry(dock_station, &dock_stations, sibling) {
-		if (dock_station->handle == handle) {
-			struct dock_data *dd;
-			acpi_status status;
-
-			dd = kmalloc(sizeof(*dd), GFP_KERNEL);
-			if (!dd)
-				break;
-
-			dd->handle = handle;
-			dd->event = event;
-			dd->ds = dock_station;
-			status = acpi_os_hotplug_execute(acpi_dock_deferred_cb,
-							 dd);
-			if (ACPI_FAILURE(status))
-				kfree(dd);
-
-			break;
-		}
+	dd = kmalloc(sizeof(*dd), GFP_KERNEL);
+	if (dd) {
+		acpi_status status;
+
+		dd->ds = data;
+		dd->event = event;
+		status = acpi_os_hotplug_execute(acpi_dock_deferred_cb, dd);
+		if (ACPI_FAILURE(status))
+			kfree(dd);
 	}
-
-	acpi_scan_lock_release();
-	return 0;
 }
 
-static struct notifier_block dock_acpi_notifier = {
-	.notifier_call = acpi_dock_notifier_call,
-};
-
 /**
  * find_dock_devices - find devices on the dock station
  * @handle: the handle of the device we are examining
@@ -979,6 +955,7 @@ static int __init dock_add(acpi_handle h
 	int ret, id;
 	struct dock_station ds, *dock_station;
 	struct platform_device *dd;
+	acpi_status status;
 
 	id = dock_station_count;
 	memset(&ds, 0, sizeof(ds));
@@ -1021,6 +998,11 @@ static int __init dock_add(acpi_handle h
 	if (ret)
 		goto err_rmgroup;
 
+	status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+					     dock_notify_handler, dock_station);
+	if (ACPI_FAILURE(status))
+		goto err_rmgroup;
+
 	dock_station_count++;
 	list_add(&dock_station->sibling, &dock_stations);
 	return 0;
@@ -1065,7 +1047,6 @@ int __init acpi_dock_init(void)
 		return 0;
 	}
 
-	register_acpi_bus_notifier(&dock_acpi_notifier);
 	pr_info(PREFIX "%s: %d docks/bays found\n",
 		ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
 	return 0;

[PATCH 2/3] ACPI: Drop ACPI bus notifier call chain

[Rafael J. Wysocki]

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

There are no users of the ACPI bus notifier call chain,
acpi_bus_notify_list, any more, so drop it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c      |   16 ----------------
 include/acpi/acpi_bus.h |    2 --
 2 files changed, 18 deletions(-)

Index: linux-pm/drivers/acpi/bus.c
===================================================================
--- linux-pm.orig/drivers/acpi/bus.c
+++ linux-pm/drivers/acpi/bus.c
@@ -499,19 +499,6 @@ static void acpi_bus_check_scope(acpi_ha
 	 */
 }
 
-static BLOCKING_NOTIFIER_HEAD(acpi_bus_notify_list);
-int register_acpi_bus_notifier(struct notifier_block *nb)
-{
-	return blocking_notifier_chain_register(&acpi_bus_notify_list, nb);
-}
-EXPORT_SYMBOL_GPL(register_acpi_bus_notifier);
-
-void unregister_acpi_bus_notifier(struct notifier_block *nb)
-{
-	blocking_notifier_chain_unregister(&acpi_bus_notify_list, nb);
-}
-EXPORT_SYMBOL_GPL(unregister_acpi_bus_notifier);
-
 /**
  * acpi_bus_notify
  * ---------------
@@ -525,9 +512,6 @@ static void acpi_bus_notify(acpi_handle
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Notification %#02x to handle %p\n",
 			  type, handle));
 
-	blocking_notifier_call_chain(&acpi_bus_notify_list,
-		type, (void *)handle);
-
 	switch (type) {
 
 	case ACPI_NOTIFY_BUS_CHECK:
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -356,8 +356,6 @@ extern int acpi_notifier_call_chain(stru
 extern int register_acpi_notifier(struct notifier_block *);
 extern int unregister_acpi_notifier(struct notifier_block *);
 
-extern int register_acpi_bus_notifier(struct notifier_block *nb);
-extern void unregister_acpi_bus_notifier(struct notifier_block *nb);
 /*
  * External Functions
  */

[PATCH 3/3] ACPI / dock: Do not leak memory on falilures to add a dock station

[Rafael J. Wysocki]

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

The function creating and registering dock station objects,
dock_add(), leaks memory if there's an error after it's walked
the ACPI namespace calling find_dock_devices(), because it doesn't
free the list of dependent devices it's just created in those cases.

Fix that issue by adding the missing code to free the list of
dependent devices on errors.

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

Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -119,6 +119,16 @@ add_dock_dependent_device(struct dock_st
 	return 0;
 }
 
+static void remove_dock_dependent_devices(struct dock_station *ds)
+{
+	struct dock_dependent_device *dd, *aux;
+
+	list_for_each_entry_safe(dd, aux, &ds->dependent_devices, list) {
+		list_del(&dd->list);
+		kfree(dd);
+	}
+}
+
 /**
  * dock_init_hotplug - Initialize a hotplug device on a docking station.
  * @dd: Dock-dependent device.
@@ -1008,6 +1018,7 @@ static int __init dock_add(acpi_handle h
 	return 0;
 
 err_rmgroup:
+	remove_dock_dependent_devices(dock_station);
 	sysfs_remove_group(&dd->dev.kobj, &dock_attribute_group);
 err_unregister:
 	platform_device_unregister(dd);

Re: [PATCH 1/3] ACPI / dock: Rework the handling of notifications

[Yinghai Lu]

On Fri, Jun 28, 2013 at 3:53 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The ACPI dock driver uses register_acpi_bus_notifier() which
> installs a notifier triggered globally for all system notifications.
> That first of all is inefficient, because the dock driver is only
> interested in notifications associated with the devices it handles,
> but it has to handle all system notifies for all devices.  Moreover,
> it does that even if no docking stations are present in the system
> (CONFIG_ACPI_DOCK set is sufficient for that to happen).  Besides,
> that is inconvenient, because it requires the driver to do extra work
> for each notification to find the target dock station object.
>
> For these reasons, rework the dock driver to install a notify
> handler individually for each dock station in the system using
> acpi_install_notify_handler().  This allows the dock station
> object to be passed directly to the notify handler and makes it
> possible to simplify the dock driver quite a bit.  It also
> reduces the overhead related to the handling of all system
> notifies when CONFIG_ACPI_DOCK is set.

original change to use register_acpi_bus_notifier, have two assumption
1.  two dock_station will have same handle.
2. acpi subsystem: non root acpi device only can have one system
notifier installed.

Maybe you can rework acpi_install_notify_handler to fix 2.

Thanks

Yinghai

Re: [PATCH 1/3] ACPI / dock: Rework the handling of notifications

[Rafael J. Wysocki]

On Friday, June 28, 2013 04:34:21 PM Yinghai Lu wrote:
> On Fri, Jun 28, 2013 at 3:53 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The ACPI dock driver uses register_acpi_bus_notifier() which
> > installs a notifier triggered globally for all system notifications.
> > That first of all is inefficient, because the dock driver is only
> > interested in notifications associated with the devices it handles,
> > but it has to handle all system notifies for all devices.  Moreover,
> > it does that even if no docking stations are present in the system
> > (CONFIG_ACPI_DOCK set is sufficient for that to happen).  Besides,
> > that is inconvenient, because it requires the driver to do extra work
> > for each notification to find the target dock station object.
> >
> > For these reasons, rework the dock driver to install a notify
> > handler individually for each dock station in the system using
> > acpi_install_notify_handler().  This allows the dock station
> > object to be passed directly to the notify handler and makes it
> > possible to simplify the dock driver quite a bit.  It also
> > reduces the overhead related to the handling of all system
> > notifies when CONFIG_ACPI_DOCK is set.
> 
> original change to use register_acpi_bus_notifier, have two assumption
> 1.  two dock_station will have same handle.

Well, that would mean that dock_add() might be called twice for the same handle
and I don't see how that's possible.

Moreover, even if that were possible, the loop in acpi_dock_notifier_call()
would break after finding the *first* matching handle anyway, so
acpi_dock_deferred_cb() wouldn't be called for the second dock station with
the same handle, if there were two.

> 2. acpi subsystem: non root acpi device only can have one system
> notifier installed.

No, that limitation is long gone.  We removed it when we were working on ACPI
wakeup support for runtime PM.

Thanks,
Rafael


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

Re: [PATCH 1/3] ACPI / dock: Rework the handling of notifications

[Yinghai Lu]

On Sat, Jun 29, 2013 at 4:16 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, June 28, 2013 04:34:21 PM Yinghai Lu wrote:
>> On Fri, Jun 28, 2013 at 3:53 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >
>> > The ACPI dock driver uses register_acpi_bus_notifier() which
>> > installs a notifier triggered globally for all system notifications.
>> > That first of all is inefficient, because the dock driver is only
>> > interested in notifications associated with the devices it handles,
>> > but it has to handle all system notifies for all devices.  Moreover,
>> > it does that even if no docking stations are present in the system
>> > (CONFIG_ACPI_DOCK set is sufficient for that to happen).  Besides,
>> > that is inconvenient, because it requires the driver to do extra work
>> > for each notification to find the target dock station object.
>> >
>> > For these reasons, rework the dock driver to install a notify
>> > handler individually for each dock station in the system using
>> > acpi_install_notify_handler().  This allows the dock station
>> > object to be passed directly to the notify handler and makes it
>> > possible to simplify the dock driver quite a bit.  It also
>> > reduces the overhead related to the handling of all system
>> > notifies when CONFIG_ACPI_DOCK is set.
>>
>> original change to use register_acpi_bus_notifier, have two assumption
>> 1.  two dock_station will have same handle.
>
> Well, that would mean that dock_add() might be called twice for the same handle
> and I don't see how that's possible.
>
> Moreover, even if that were possible, the loop in acpi_dock_notifier_call()
> would break after finding the *first* matching handle anyway, so
> acpi_dock_deferred_cb() wouldn't be called for the second dock station with
> the same handle, if there were two.

related commit:
commit 6bd00a61ab63d4ceb635ae0316353c11c900b8d8
Author: Shaohua Li <shaohua.li@intel.com>
Date:   Thu Aug 28 10:04:29 2008 +0800

    ACPI: introduce notifier change to avoid duplicates

    The battery driver already registers notification handler.
    To avoid registering notification handler again,
    introduce a notifier chain in global system notifier handler
    and use it in dock driver.

so it is not two dock station have same handle. it is battery acpi driver.

but notitifer installing is changed

commit d94066910943837558d2a461c6766da981260bf0
Author: Bjorn Helgaas <bjorn.helgaas@hp.com>
Date:   Thu Apr 30 09:35:47 2009 -0600

    ACPI: battery: use .notify method instead of installing handler directly

    This patch adds a .notify() method.  The presence of .notify() causes
    Linux/ACPI to manage event handlers and notify handlers on our behalf,
    so we don't have to install and remove them ourselves.

    This driver apparently relies on seeing ALL notify events, not just
    device-specific ones (because it used ACPI_ALL_NOTIFY).  We use the
    ACPI_DRIVER_ALL_NOTIFY_EVENTS driver flag to request all events.

    Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
    CC: Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com>
    Signed-off-by: Len Brown <len.brown@intel.com>


>
>> 2. acpi subsystem: non root acpi device only can have one system
>> notifier installed.
>
> No, that limitation is long gone.  We removed it when we were working on ACPI
> wakeup support for runtime PM.

looks like i misread that...

 * NOTES:       The Root namespace object may have only one handler for each
 *              type of notify (System/Device). Device/Thermal/Processor objects
 *              may have one device notify handler, and multiple system notify
 *              handlers.

device could have multiple system notify handlers.

So

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

Re: [PATCH 1/3] ACPI / dock: Rework the handling of notifications

[Rafael J. Wysocki]

On Saturday, June 29, 2013 09:12:59 AM Yinghai Lu wrote:
> On Sat, Jun 29, 2013 at 4:16 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Friday, June 28, 2013 04:34:21 PM Yinghai Lu wrote:
> >> On Fri, Jun 28, 2013 at 3:53 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> >
> >> > The ACPI dock driver uses register_acpi_bus_notifier() which
> >> > installs a notifier triggered globally for all system notifications.
> >> > That first of all is inefficient, because the dock driver is only
> >> > interested in notifications associated with the devices it handles,
> >> > but it has to handle all system notifies for all devices.  Moreover,
> >> > it does that even if no docking stations are present in the system
> >> > (CONFIG_ACPI_DOCK set is sufficient for that to happen).  Besides,
> >> > that is inconvenient, because it requires the driver to do extra work
> >> > for each notification to find the target dock station object.
> >> >
> >> > For these reasons, rework the dock driver to install a notify
> >> > handler individually for each dock station in the system using
> >> > acpi_install_notify_handler().  This allows the dock station
> >> > object to be passed directly to the notify handler and makes it
> >> > possible to simplify the dock driver quite a bit.  It also
> >> > reduces the overhead related to the handling of all system
> >> > notifies when CONFIG_ACPI_DOCK is set.
> >>
> >> original change to use register_acpi_bus_notifier, have two assumption
> >> 1.  two dock_station will have same handle.
> >
> > Well, that would mean that dock_add() might be called twice for the same handle
> > and I don't see how that's possible.
> >
> > Moreover, even if that were possible, the loop in acpi_dock_notifier_call()
> > would break after finding the *first* matching handle anyway, so
> > acpi_dock_deferred_cb() wouldn't be called for the second dock station with
> > the same handle, if there were two.
> 
> related commit:
> commit 6bd00a61ab63d4ceb635ae0316353c11c900b8d8
> Author: Shaohua Li <shaohua.li@intel.com>
> Date:   Thu Aug 28 10:04:29 2008 +0800
> 
>     ACPI: introduce notifier change to avoid duplicates
> 
>     The battery driver already registers notification handler.
>     To avoid registering notification handler again,
>     introduce a notifier chain in global system notifier handler
>     and use it in dock driver.
> 
> so it is not two dock station have same handle. it is battery acpi driver.
> 
> but notitifer installing is changed
> 
> commit d94066910943837558d2a461c6766da981260bf0
> Author: Bjorn Helgaas <bjorn.helgaas@hp.com>
> Date:   Thu Apr 30 09:35:47 2009 -0600
> 
>     ACPI: battery: use .notify method instead of installing handler directly
> 
>     This patch adds a .notify() method.  The presence of .notify() causes
>     Linux/ACPI to manage event handlers and notify handlers on our behalf,
>     so we don't have to install and remove them ourselves.
> 
>     This driver apparently relies on seeing ALL notify events, not just
>     device-specific ones (because it used ACPI_ALL_NOTIFY).  We use the
>     ACPI_DRIVER_ALL_NOTIFY_EVENTS driver flag to request all events.
> 
>     Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
>     CC: Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com>
>     Signed-off-by: Len Brown <len.brown@intel.com>
> 
> 
> >
> >> 2. acpi subsystem: non root acpi device only can have one system
> >> notifier installed.
> >
> > No, that limitation is long gone.  We removed it when we were working on ACPI
> > wakeup support for runtime PM.
> 
> looks like i misread that...
> 
>  * NOTES:       The Root namespace object may have only one handler for each
>  *              type of notify (System/Device). Device/Thermal/Processor objects
>  *              may have one device notify handler, and multiple system notify
>  *              handlers.
> 
> device could have multiple system notify handlers.
> 
> So
> 
> Acked-by: Yinghai Lu <yinghai@kernel.org>

Thanks (and for the other ACKs too)!

Re: [PATCH 1/3] ACPI / dock: Rework the handling of notifications

[Bjorn Helgaas]

On Fri, Jun 28, 2013 at 4:53 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The ACPI dock driver uses register_acpi_bus_notifier() which
> installs a notifier triggered globally for all system notifications.
> That first of all is inefficient, because the dock driver is only
> interested in notifications associated with the devices it handles,
> but it has to handle all system notifies for all devices.  Moreover,
> it does that even if no docking stations are present in the system
> (CONFIG_ACPI_DOCK set is sufficient for that to happen).  Besides,
> that is inconvenient, because it requires the driver to do extra work
> for each notification to find the target dock station object.
>
> For these reasons, rework the dock driver to install a notify
> handler individually for each dock station in the system using
> acpi_install_notify_handler().  This allows the dock station
> object to be passed directly to the notify handler and makes it
> possible to simplify the dock driver quite a bit.  It also
> reduces the overhead related to the handling of all system
> notifies when CONFIG_ACPI_DOCK is set.

I fully support what you're doing, even though I haven't read it in
enough detail to actually review it.  I'm pretty sure that whatever
you do, you won't make things worse :)

It sounds like you are keeping the approach of "look for certain AML
features to identify a dock, and install notify handlers when you find
one."  I think acpiphp uses a similar approach, and I'm not sure it's
a good one.  The spec is not explicit about how the AML should be
organized, and AML writers are very creative.  I think acpiphp suffers
because it only works with certain arrangements of _ADR, _EJ0, _RMV,
etc., and I think that has led to a brittle design and possibly more
separation between dock and acpiphp than is necessary.

I think it would be better if we *always* had a more generic notify
handler installed and figured out where to route the notification
based on its type and what services are configured in.  The ultimate
handler might do different things based on what methods are present,
of course.

I don't know if this rambling makes sense for docks (or even for
acpiphp), so if it doesn't, feel free to just ignore it :)

Bjorn

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/dock.c |   69 ++++++++++++++++++----------------------------------
>  1 file changed, 25 insertions(+), 44 deletions(-)
>
> Index: linux-pm/drivers/acpi/dock.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/dock.c
> +++ linux-pm/drivers/acpi/dock.c
> @@ -718,18 +718,17 @@ static int handle_eject_request(struct d
>
>  /**
>   * dock_notify - act upon an acpi dock notification
> - * @handle: the dock station handle
> + * @ds: dock station
>   * @event: the acpi event
> - * @data: our driver data struct
>   *
>   * If we are notified to dock, then check to see if the dock is
>   * present and then dock.  Notify all drivers of the dock event,
>   * and then hotplug and devices that may need hotplugging.
>   */
> -static void dock_notify(acpi_handle handle, u32 event, void *data)
> +static void dock_notify(struct dock_station *ds, u32 event)
>  {
> -       struct dock_station *ds = data;
> -       struct acpi_device *tmp;
> +       acpi_handle handle = ds->handle;
> +       struct acpi_device *ad;
>         int surprise_removal = 0;
>
>         /*
> @@ -752,8 +751,7 @@ static void dock_notify(acpi_handle hand
>         switch (event) {
>         case ACPI_NOTIFY_BUS_CHECK:
>         case ACPI_NOTIFY_DEVICE_CHECK:
> -               if (!dock_in_progress(ds) && acpi_bus_get_device(ds->handle,
> -                  &tmp)) {
> +               if (!dock_in_progress(ds) && acpi_bus_get_device(handle, &ad)) {
>                         begin_dock(ds);
>                         dock(ds);
>                         if (!dock_present(ds)) {
> @@ -790,9 +788,8 @@ static void dock_notify(acpi_handle hand
>  }
>
>  struct dock_data {
> -       acpi_handle handle;
> -       unsigned long event;
>         struct dock_station *ds;
> +       u32 event;
>  };
>
>  static void acpi_dock_deferred_cb(void *context)
> @@ -800,52 +797,31 @@ static void acpi_dock_deferred_cb(void *
>         struct dock_data *data = context;
>
>         acpi_scan_lock_acquire();
> -       dock_notify(data->handle, data->event, data->ds);
> +       dock_notify(data->ds, data->event);
>         acpi_scan_lock_release();
>         kfree(data);
>  }
>
> -static int acpi_dock_notifier_call(struct notifier_block *this,
> -       unsigned long event, void *data)
> +static void dock_notify_handler(acpi_handle handle, u32 event, void *data)
>  {
> -       struct dock_station *dock_station;
> -       acpi_handle handle = data;
> +       struct dock_data *dd;
>
>         if (event != ACPI_NOTIFY_BUS_CHECK && event != ACPI_NOTIFY_DEVICE_CHECK
>            && event != ACPI_NOTIFY_EJECT_REQUEST)
> -               return 0;
> -
> -       acpi_scan_lock_acquire();
> +               return;
>
> -       list_for_each_entry(dock_station, &dock_stations, sibling) {
> -               if (dock_station->handle == handle) {
> -                       struct dock_data *dd;
> -                       acpi_status status;
> -
> -                       dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> -                       if (!dd)
> -                               break;
> -
> -                       dd->handle = handle;
> -                       dd->event = event;
> -                       dd->ds = dock_station;
> -                       status = acpi_os_hotplug_execute(acpi_dock_deferred_cb,
> -                                                        dd);
> -                       if (ACPI_FAILURE(status))
> -                               kfree(dd);
> -
> -                       break;
> -               }
> +       dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> +       if (dd) {
> +               acpi_status status;
> +
> +               dd->ds = data;
> +               dd->event = event;
> +               status = acpi_os_hotplug_execute(acpi_dock_deferred_cb, dd);
> +               if (ACPI_FAILURE(status))
> +                       kfree(dd);
>         }
> -
> -       acpi_scan_lock_release();
> -       return 0;
>  }
>
> -static struct notifier_block dock_acpi_notifier = {
> -       .notifier_call = acpi_dock_notifier_call,
> -};
> -
>  /**
>   * find_dock_devices - find devices on the dock station
>   * @handle: the handle of the device we are examining
> @@ -979,6 +955,7 @@ static int __init dock_add(acpi_handle h
>         int ret, id;
>         struct dock_station ds, *dock_station;
>         struct platform_device *dd;
> +       acpi_status status;
>
>         id = dock_station_count;
>         memset(&ds, 0, sizeof(ds));
> @@ -1021,6 +998,11 @@ static int __init dock_add(acpi_handle h
>         if (ret)
>                 goto err_rmgroup;
>
> +       status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> +                                            dock_notify_handler, dock_station);
> +       if (ACPI_FAILURE(status))
> +               goto err_rmgroup;
> +
>         dock_station_count++;
>         list_add(&dock_station->sibling, &dock_stations);
>         return 0;
> @@ -1065,7 +1047,6 @@ int __init acpi_dock_init(void)
>                 return 0;
>         }
>
> -       register_acpi_bus_notifier(&dock_acpi_notifier);
>         pr_info(PREFIX "%s: %d docks/bays found\n",
>                 ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
>         return 0;
>

Re: [PATCH 1/3] ACPI / dock: Rework the handling of notifications

[Rafael J. Wysocki]

On Monday, July 01, 2013 02:21:45 PM Bjorn Helgaas wrote:
> On Fri, Jun 28, 2013 at 4:53 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The ACPI dock driver uses register_acpi_bus_notifier() which
> > installs a notifier triggered globally for all system notifications.
> > That first of all is inefficient, because the dock driver is only
> > interested in notifications associated with the devices it handles,
> > but it has to handle all system notifies for all devices.  Moreover,
> > it does that even if no docking stations are present in the system
> > (CONFIG_ACPI_DOCK set is sufficient for that to happen).  Besides,
> > that is inconvenient, because it requires the driver to do extra work
> > for each notification to find the target dock station object.
> >
> > For these reasons, rework the dock driver to install a notify
> > handler individually for each dock station in the system using
> > acpi_install_notify_handler().  This allows the dock station
> > object to be passed directly to the notify handler and makes it
> > possible to simplify the dock driver quite a bit.  It also
> > reduces the overhead related to the handling of all system
> > notifies when CONFIG_ACPI_DOCK is set.
> 
> I fully support what you're doing, even though I haven't read it in
> enough detail to actually review it.  I'm pretty sure that whatever
> you do, you won't make things worse :)

Well, fair enough. :-)

> It sounds like you are keeping the approach of "look for certain AML
> features to identify a dock, and install notify handlers when you find
> one."  I think acpiphp uses a similar approach, and I'm not sure it's
> a good one.  The spec is not explicit about how the AML should be
> organized, and AML writers are very creative.  I think acpiphp suffers
> because it only works with certain arrangements of _ADR, _EJ0, _RMV,
> etc., and I think that has led to a brittle design and possibly more
> separation between dock and acpiphp than is necessary.

I generally agree that this is not a robust approach, but for now I'm avoiding
making changes that may just go too far.

> I think it would be better if we *always* had a more generic notify
> handler installed and figured out where to route the notification
> based on its type and what services are configured in.  The ultimate
> handler might do different things based on what methods are present,
> of course.
> 
> I don't know if this rambling makes sense for docks (or even for
> acpiphp), so if it doesn't, feel free to just ignore it :)

Well, ideally, it would be great if we could handle all of the bus check,
device check and eject notifications through something like
acpi_hotplug_notify_cb() and dispatch more specific handling from
there depending on what's represented by the target handle.  That's why I
introduced hotplug profiles.

That said it's a long way to that point from where we are now. :-)

Thanks,
Rafael


> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/dock.c |   69 ++++++++++++++++++----------------------------------
> >  1 file changed, 25 insertions(+), 44 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/dock.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/dock.c
> > +++ linux-pm/drivers/acpi/dock.c
> > @@ -718,18 +718,17 @@ static int handle_eject_request(struct d
> >
> >  /**
> >   * dock_notify - act upon an acpi dock notification
> > - * @handle: the dock station handle
> > + * @ds: dock station
> >   * @event: the acpi event
> > - * @data: our driver data struct
> >   *
> >   * If we are notified to dock, then check to see if the dock is
> >   * present and then dock.  Notify all drivers of the dock event,
> >   * and then hotplug and devices that may need hotplugging.
> >   */
> > -static void dock_notify(acpi_handle handle, u32 event, void *data)
> > +static void dock_notify(struct dock_station *ds, u32 event)
> >  {
> > -       struct dock_station *ds = data;
> > -       struct acpi_device *tmp;
> > +       acpi_handle handle = ds->handle;
> > +       struct acpi_device *ad;
> >         int surprise_removal = 0;
> >
> >         /*
> > @@ -752,8 +751,7 @@ static void dock_notify(acpi_handle hand
> >         switch (event) {
> >         case ACPI_NOTIFY_BUS_CHECK:
> >         case ACPI_NOTIFY_DEVICE_CHECK:
> > -               if (!dock_in_progress(ds) && acpi_bus_get_device(ds->handle,
> > -                  &tmp)) {
> > +               if (!dock_in_progress(ds) && acpi_bus_get_device(handle, &ad)) {
> >                         begin_dock(ds);
> >                         dock(ds);
> >                         if (!dock_present(ds)) {
> > @@ -790,9 +788,8 @@ static void dock_notify(acpi_handle hand
> >  }
> >
> >  struct dock_data {
> > -       acpi_handle handle;
> > -       unsigned long event;
> >         struct dock_station *ds;
> > +       u32 event;
> >  };
> >
> >  static void acpi_dock_deferred_cb(void *context)
> > @@ -800,52 +797,31 @@ static void acpi_dock_deferred_cb(void *
> >         struct dock_data *data = context;
> >
> >         acpi_scan_lock_acquire();
> > -       dock_notify(data->handle, data->event, data->ds);
> > +       dock_notify(data->ds, data->event);
> >         acpi_scan_lock_release();
> >         kfree(data);
> >  }
> >
> > -static int acpi_dock_notifier_call(struct notifier_block *this,
> > -       unsigned long event, void *data)
> > +static void dock_notify_handler(acpi_handle handle, u32 event, void *data)
> >  {
> > -       struct dock_station *dock_station;
> > -       acpi_handle handle = data;
> > +       struct dock_data *dd;
> >
> >         if (event != ACPI_NOTIFY_BUS_CHECK && event != ACPI_NOTIFY_DEVICE_CHECK
> >            && event != ACPI_NOTIFY_EJECT_REQUEST)
> > -               return 0;
> > -
> > -       acpi_scan_lock_acquire();
> > +               return;
> >
> > -       list_for_each_entry(dock_station, &dock_stations, sibling) {
> > -               if (dock_station->handle == handle) {
> > -                       struct dock_data *dd;
> > -                       acpi_status status;
> > -
> > -                       dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> > -                       if (!dd)
> > -                               break;
> > -
> > -                       dd->handle = handle;
> > -                       dd->event = event;
> > -                       dd->ds = dock_station;
> > -                       status = acpi_os_hotplug_execute(acpi_dock_deferred_cb,
> > -                                                        dd);
> > -                       if (ACPI_FAILURE(status))
> > -                               kfree(dd);
> > -
> > -                       break;
> > -               }
> > +       dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> > +       if (dd) {
> > +               acpi_status status;
> > +
> > +               dd->ds = data;
> > +               dd->event = event;
> > +               status = acpi_os_hotplug_execute(acpi_dock_deferred_cb, dd);
> > +               if (ACPI_FAILURE(status))
> > +                       kfree(dd);
> >         }
> > -
> > -       acpi_scan_lock_release();
> > -       return 0;
> >  }
> >
> > -static struct notifier_block dock_acpi_notifier = {
> > -       .notifier_call = acpi_dock_notifier_call,
> > -};
> > -
> >  /**
> >   * find_dock_devices - find devices on the dock station
> >   * @handle: the handle of the device we are examining
> > @@ -979,6 +955,7 @@ static int __init dock_add(acpi_handle h
> >         int ret, id;
> >         struct dock_station ds, *dock_station;
> >         struct platform_device *dd;
> > +       acpi_status status;
> >
> >         id = dock_station_count;
> >         memset(&ds, 0, sizeof(ds));
> > @@ -1021,6 +998,11 @@ static int __init dock_add(acpi_handle h
> >         if (ret)
> >                 goto err_rmgroup;
> >
> > +       status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> > +                                            dock_notify_handler, dock_station);
> > +       if (ACPI_FAILURE(status))
> > +               goto err_rmgroup;
> > +
> >         dock_station_count++;
> >         list_add(&dock_station->sibling, &dock_stations);
> >         return 0;
> > @@ -1065,7 +1047,6 @@ int __init acpi_dock_init(void)
> >                 return 0;
> >         }
> >
> > -       register_acpi_bus_notifier(&dock_acpi_notifier);
> >         pr_info(PREFIX "%s: %d docks/bays found\n",
> >                 ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
> >         return 0;
> >
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 0/4] ACPI / dock: One fix and more cleanups

[Rafael J. Wysocki]

Hi,

As I said before, the ACPI dock driver seems to be in an endless need for
cleanups and fixes.  This time I found one oddity (patch [1/4]) a couple of
nitpicks ([2/4] and [4/4]) and a notifier chain that can be dropped (unless
I'm overlooking a reason why those things have to be done atomically - [3/4]).

[1/4] Actually define acpi_dock_init() as void (-stable material).
[2/4] Do not check if ACPI dock is a module (it can't be).
[3/4] Drop ACPI dock notifier chain (and modify acpiphp to use something
      different for fixups).
[4/4] Drop unnecessary local variable from dock_add().

All on top of the bleeding-edge branch of the linux-pm.git tree.

Thanks,
Rafael


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

[PATCH 1/4] ACPI / dock: Actually define acpi_dock_init() as void

[Rafael J. Wysocki]

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

Commit 94add0f (ACPI / dock: Initialize ACPI dock subsystem upfront)
changed the header of acpi_dock_init() in internal.h so that it is
supposed to be a void function now, but it forgot to update its
actual definition in dock.c according to which it still is supposed
to return int.

Although that didn't cause any visible breakage or even a compiler
warning to be thrown, which is odd enough, fix it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: 3.10+ <stable@vger.kernel.org>
---
 drivers/acpi/dock.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -931,10 +931,10 @@ find_dock_and_bay(acpi_handle handle, u3
 	return AE_OK;
 }
 
-int __init acpi_dock_init(void)
+void __init acpi_dock_init(void)
 {
 	if (acpi_disabled)
-		return 0;
+		return;
 
 	/* look for dock stations and bays */
 	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
@@ -942,11 +942,10 @@ int __init acpi_dock_init(void)
 
 	if (!dock_station_count) {
 		pr_info(PREFIX "No dock devices found.\n");
-		return 0;
+		return;
 	}
 
 	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
 	pr_info(PREFIX "%s: %d docks/bays found\n",
 		ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
-	return 0;
 }

[PATCH 2/4] ACPI / dock: Do not check CONFIG_ACPI_DOCK_MODULE

[Rafael J. Wysocki]

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

Since commit 94add0f (ACPI / dock: Initialize ACPI dock subsystem
upfront) the ACPI dock driver cannot be a module, so
CONFIG_ACPI_DOCK_MODULE is never set.  For this reason, simplify
the preprocessor conditional in include/acpi/acpi_drivers.h
referring to that symbol unnecessarily.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 include/acpi/acpi_drivers.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-pm/include/acpi/acpi_drivers.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_drivers.h
+++ linux-pm/include/acpi/acpi_drivers.h
@@ -117,7 +117,7 @@ struct acpi_dock_ops {
 	acpi_notify_handler uevent;
 };
 
-#if defined(CONFIG_ACPI_DOCK) || defined(CONFIG_ACPI_DOCK_MODULE)
+#ifdef CONFIG_ACPI_DOCK
 extern int is_dock_device(acpi_handle handle);
 extern int register_dock_notifier(struct notifier_block *nb);
 extern void unregister_dock_notifier(struct notifier_block *nb);
@@ -150,6 +150,6 @@ static inline int register_hotplug_dock_
 static inline void unregister_hotplug_dock_device(acpi_handle handle)
 {
 }
-#endif
+#endif /* CONFIG_ACPI_DOCK */
 
 #endif /*__ACPI_DRIVERS_H__*/

[PATCH 3/4] ACPI / dock / PCI: Drop ACPI dock notifier chain

[Rafael J. Wysocki]

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

The only user of the ACPI dock notifier chain is the ACPI-based PCI
hotplug (acpiphp) driver that uses it to carry out post-dock fixups
needed by some systems with broken _DCK.  However, it is not
necessary to use a separate notifier chain for that, as it can be
simply replaced with a new callback in struct acpi_dock_ops.

For this reason, add a new .fixup() callback to struct acpi_dock_ops
and make hotplug_dock_devices() execute it for all dock devices with
hotplug operations registered.  Accordingly, make acpiphp point that
callback to the function carrying out the post-dock fixups and
do not register a separate dock notifier for each device
registering dock operations.  Finally, drop the ACPI dock notifier
chain that has no more users.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/dock.c                |   66 ++++++++++++++-----------------------
 drivers/pci/hotplug/acpiphp.h      |    1 
 drivers/pci/hotplug/acpiphp_glue.c |   18 ++--------
 include/acpi/acpi_drivers.h        |   10 -----
 4 files changed, 30 insertions(+), 65 deletions(-)

Index: linux-pm/include/acpi/acpi_drivers.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_drivers.h
+++ linux-pm/include/acpi/acpi_drivers.h
@@ -113,14 +113,13 @@ void pci_acpi_crs_quirks(void);
                                   Dock Station
   -------------------------------------------------------------------------- */
 struct acpi_dock_ops {
+	acpi_notify_handler fixup;
 	acpi_notify_handler handler;
 	acpi_notify_handler uevent;
 };
 
 #ifdef CONFIG_ACPI_DOCK
 extern int is_dock_device(acpi_handle handle);
-extern int register_dock_notifier(struct notifier_block *nb);
-extern void unregister_dock_notifier(struct notifier_block *nb);
 extern int register_hotplug_dock_device(acpi_handle handle,
 					const struct acpi_dock_ops *ops,
 					void *context,
@@ -132,13 +131,6 @@ static inline int is_dock_device(acpi_ha
 {
 	return 0;
 }
-static inline int register_dock_notifier(struct notifier_block *nb)
-{
-	return -ENODEV;
-}
-static inline void unregister_dock_notifier(struct notifier_block *nb)
-{
-}
 static inline int register_hotplug_dock_device(acpi_handle handle,
 					       const struct acpi_dock_ops *ops,
 					       void *context,
Index: linux-pm/drivers/pci/hotplug/acpiphp.h
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/acpiphp.h
+++ linux-pm/drivers/pci/hotplug/acpiphp.h
@@ -122,7 +122,6 @@ struct acpiphp_func {
 	struct acpiphp_slot *slot;	/* parent */
 
 	struct list_head sibling;
-	struct notifier_block nb;
 	acpi_handle	handle;
 
 	u8		function;	/* pci function# */
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
@@ -119,15 +119,14 @@ static void free_bridge(struct kref *kre
  * TBD - figure out a way to only call fixups for
  * systems that require them.
  */
-static int post_dock_fixups(struct notifier_block *nb, unsigned long val,
-	void *v)
+static void post_dock_fixups(acpi_handle not_used, u32 event, void *data)
 {
-	struct acpiphp_func *func = container_of(nb, struct acpiphp_func, nb);
+	struct acpiphp_func *func = data;
 	struct pci_bus *bus = func->slot->bridge->pci_bus;
 	u32 buses;
 
 	if (!bus->self)
-		return  NOTIFY_OK;
+		return;
 
 	/* fixup bad _DCK function that rewrites
 	 * secondary bridge on slot
@@ -143,11 +142,11 @@ static int post_dock_fixups(struct notif
 			| ((unsigned int)(bus->busn_res.end) << 16);
 		pci_write_config_dword(bus->self, PCI_PRIMARY_BUS, buses);
 	}
-	return NOTIFY_OK;
 }
 
 
 static const struct acpi_dock_ops acpiphp_dock_ops = {
+	.fixup = post_dock_fixups,
 	.handler = hotplug_event_func,
 };
 
@@ -315,14 +314,6 @@ register_slot(acpi_handle handle, u32 lv
 			&acpiphp_dock_ops, newfunc,
 			acpiphp_dock_init, acpiphp_dock_release))
 			dbg("failed to register dock device\n");
-
-		/* we need to be notified when dock events happen
-		 * outside of the hotplug operation, since we may
-		 * need to do fixups before we can hotplug.
-		 */
-		newfunc->nb.notifier_call = post_dock_fixups;
-		if (register_dock_notifier(&newfunc->nb))
-			dbg("failed to register a dock notifier");
 	}
 
 	/* install notify handler */
@@ -472,7 +463,6 @@ static void cleanup_bridge(struct acpiph
 		list_for_each_entry(func, &slot->funcs, sibling) {
 			if (is_dock_device(func->handle)) {
 				unregister_hotplug_dock_device(func->handle);
-				unregister_dock_notifier(&func->nb);
 			}
 			if (!(func->flags & FUNC_HAS_DCK)) {
 				status = acpi_remove_notify_handler(func->handle,
Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -51,8 +51,6 @@ MODULE_PARM_DESC(immediate_undock, "1 (d
 	" the driver to wait for userspace to write the undock sysfs file "
 	" before undocking");
 
-static struct atomic_notifier_head dock_notifier_list;
-
 static const struct acpi_device_id dock_device_ids[] = {
 	{"LNXDOCK", 0},
 	{"", 0},
@@ -89,6 +87,12 @@ struct dock_dependent_device {
 #define DOCK_EVENT	3
 #define UNDOCK_EVENT	2
 
+enum dock_callback_type {
+	DOCK_CALL_HANDLER,
+	DOCK_CALL_FIXUP,
+	DOCK_CALL_UEVENT,
+};
+
 /*****************************************************************************
  *                         Dock Dependent device functions                   *
  *****************************************************************************/
@@ -179,7 +183,7 @@ static void dock_release_hotplug(struct
 }
 
 static void dock_hotplug_event(struct dock_dependent_device *dd, u32 event,
-			       bool uevent)
+			       enum dock_callback_type cb_type)
 {
 	acpi_notify_handler cb = NULL;
 	bool run = false;
@@ -189,8 +193,18 @@ static void dock_hotplug_event(struct do
 	if (dd->hp_context) {
 		run = true;
 		dd->hp_refcount++;
-		if (dd->hp_ops)
-			cb = uevent ? dd->hp_ops->uevent : dd->hp_ops->handler;
+		if (dd->hp_ops) {
+			switch (cb_type) {
+			case DOCK_CALL_FIXUP:
+				cb = dd->hp_ops->fixup;
+				break;
+			case DOCK_CALL_UEVENT:
+				cb = dd->hp_ops->uevent;
+				break;
+			default:
+				cb = dd->hp_ops->handler;
+			}
+		}
 	}
 
 	mutex_unlock(&hotplug_lock);
@@ -372,9 +386,13 @@ static void hotplug_dock_devices(struct
 {
 	struct dock_dependent_device *dd;
 
+	/* Call driver specific post-dock fixups. */
+	list_for_each_entry(dd, &ds->dependent_devices, list)
+		dock_hotplug_event(dd, event, DOCK_CALL_FIXUP);
+
 	/* Call driver specific hotplug functions. */
 	list_for_each_entry(dd, &ds->dependent_devices, list)
-		dock_hotplug_event(dd, event, false);
+		dock_hotplug_event(dd, event, DOCK_CALL_HANDLER);
 
 	/*
 	 * Now make sure that an acpi_device is created for each dependent
@@ -405,7 +423,7 @@ static void dock_event(struct dock_stati
 		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
 
 	list_for_each_entry(dd, &ds->dependent_devices, list)
-		dock_hotplug_event(dd, event, true);
+		dock_hotplug_event(dd, event, DOCK_CALL_UEVENT);
 
 	if (num != DOCK_EVENT)
 		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
@@ -488,37 +506,6 @@ static int dock_in_progress(struct dock_
 }
 
 /**
- * register_dock_notifier - add yourself to the dock notifier list
- * @nb: the callers notifier block
- *
- * If a driver wishes to be notified about dock events, they can
- * use this function to put a notifier block on the dock notifier list.
- * this notifier call chain will be called after a dock event, but
- * before hotplugging any new devices.
- */
-int register_dock_notifier(struct notifier_block *nb)
-{
-	if (!dock_station_count)
-		return -ENODEV;
-
-	return atomic_notifier_chain_register(&dock_notifier_list, nb);
-}
-EXPORT_SYMBOL_GPL(register_dock_notifier);
-
-/**
- * unregister_dock_notifier - remove yourself from the dock notifier list
- * @nb: the callers notifier block
- */
-void unregister_dock_notifier(struct notifier_block *nb)
-{
-	if (!dock_station_count)
-		return;
-
-	atomic_notifier_chain_unregister(&dock_notifier_list, nb);
-}
-EXPORT_SYMBOL_GPL(unregister_dock_notifier);
-
-/**
  * register_hotplug_dock_device - register a hotplug function
  * @handle: the handle of the device
  * @ops: handlers to call after docking
@@ -658,8 +645,6 @@ static void dock_notify(struct dock_stat
 				complete_dock(ds);
 				break;
 			}
-			atomic_notifier_call_chain(&dock_notifier_list,
-						   event, NULL);
 			hotplug_dock_devices(ds, event);
 			complete_dock(ds);
 			dock_event(ds, event, DOCK_EVENT);
@@ -945,7 +930,6 @@ void __init acpi_dock_init(void)
 		return;
 	}
 
-	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
 	pr_info(PREFIX "%s: %d docks/bays found\n",
 		ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
 }

[PATCH 4/4] ACPI / dock: Drop unnecessary local variable from dock_add()

[Rafael J. Wysocki]

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

The local variable id in dock_add() is not necessary, so drop it.

While we're at it, use an initializer to clear the local variable ds
and drop the memset() used for this purpose.

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

Index: linux-pm/drivers/acpi/dock.c
===================================================================
--- linux-pm.orig/drivers/acpi/dock.c
+++ linux-pm/drivers/acpi/dock.c
@@ -836,14 +836,13 @@ static struct attribute_group dock_attri
  */
 static int __init dock_add(acpi_handle handle)
 {
-	int ret, id;
-	struct dock_station ds, *dock_station;
+	struct dock_station *dock_station, ds = { 0 };
 	struct platform_device *dd;
 	acpi_status status;
+	int ret;
 
-	id = dock_station_count;
-	memset(&ds, 0, sizeof(ds));
-	dd = platform_device_register_data(NULL, "dock", id, &ds, sizeof(ds));
+	dd = platform_device_register_data(NULL, "dock", dock_station_count,
+					   &ds, sizeof(ds));
 	if (IS_ERR(dd))
 		return PTR_ERR(dd);