[patch 4/11]makeing dock driver supports bay and battery hotplug

[patch 4/11]makeing dock driver supports bay and battery hotplug

[Shaohua Li]

Making dock driver supports bay and battery hotplug. They are all
regarded as dock, and unified handled.

Signed-off-by: Shaohua Li <shaohua.li@intel.com> 
---
 drivers/acpi/dock.c |  223 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 175 insertions(+), 48 deletions(-)

Index: linux/drivers/acpi/dock.c
===================================================================
--- linux.orig/drivers/acpi/dock.c	2008-08-28 09:36:25.000000000 +0800
+++ linux/drivers/acpi/dock.c	2008-08-28 09:38:41.000000000 +0800
@@ -48,7 +48,6 @@ MODULE_PARM_DESC(immediate_undock, "1 (d
 	" before undocking");
 
 static struct atomic_notifier_head dock_notifier_list;
-static struct platform_device *dock_device;
 static char dock_device_name[] = "dock";
 
 static const struct acpi_device_id dock_device_ids[] = {
@@ -65,7 +64,12 @@ struct dock_station {
 	struct mutex hp_lock;
 	struct list_head dependent_devices;
 	struct list_head hotplug_devices;
+
+	struct list_head sibiling;
+	struct platform_device *dock_device;
 };
+static LIST_HEAD(dock_stations);
+static int dock_station_count;
 
 struct dock_dependent_device {
 	struct list_head list;
@@ -77,11 +81,12 @@ struct dock_dependent_device {
 
 #define DOCK_DOCKING	0x00000001
 #define DOCK_UNDOCKING  0x00000002
+#define DOCK_IS_DOCK	0x00000010
+#define DOCK_IS_ATA	0x00000020
+#define DOCK_IS_BAT	0x00000040
 #define DOCK_EVENT	3
 #define UNDOCK_EVENT	2
 
-static struct dock_station *dock_station;
-
 /*****************************************************************************
  *                         Dock Dependent device functions                   *
  *****************************************************************************/
@@ -199,6 +204,60 @@ static int is_dock(acpi_handle handle)
 	return 1;
 }
 
+static int is_ejectable(acpi_handle handle)
+{
+	acpi_status status;
+	acpi_handle tmp;
+
+	status = acpi_get_handle(handle, "_EJ0", &tmp);
+	if (ACPI_FAILURE(status))
+		return 0;
+	return 1;
+}
+
+static int is_ata(acpi_handle handle)
+{
+	acpi_handle tmp;
+
+	if ((ACPI_SUCCESS(acpi_get_handle(handle, "_GTF", &tmp))) ||
+	   (ACPI_SUCCESS(acpi_get_handle(handle, "_GTM", &tmp))) ||
+	   (ACPI_SUCCESS(acpi_get_handle(handle, "_STM", &tmp))) ||
+	   (ACPI_SUCCESS(acpi_get_handle(handle, "_SDD", &tmp))))
+		return 1;
+
+	return 0;
+}
+
+static int is_battery(acpi_handle handle)
+{
+	struct acpi_device_info *info;
+	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
+	int ret = 1;
+
+	if (!ACPI_SUCCESS(acpi_get_object_info(handle, &buffer)))
+		return 0;
+	info = buffer.pointer;
+	if (!(info->valid & ACPI_VALID_HID))
+		ret = 0;
+	else
+		ret = !strcmp("PNP0C0A", info->hardware_id.value);
+
+	kfree(buffer.pointer);
+	return ret;
+}
+
+static int is_ejectable_bay(acpi_handle handle)
+{
+	acpi_handle phandle;
+	if (!is_ejectable(handle))
+		return 0;
+	if (is_battery(handle) || is_ata(handle))
+		return 1;
+	if (!acpi_get_parent(handle, &phandle) && is_ata(phandle))
+		return 1;
+	return 0;
+}
+
 /**
  * is_dock_device - see if a device is on a dock station
  * @handle: acpi handle of the device
@@ -209,11 +268,17 @@ static int is_dock(acpi_handle handle)
  */
 int is_dock_device(acpi_handle handle)
 {
-	if (!dock_station)
+	struct dock_station *dock_station;
+
+	if (!dock_station_count)
 		return 0;
 
-	if (is_dock(handle) || find_dock_dependent_device(dock_station, handle))
+	if (is_dock(handle))
 		return 1;
+	list_for_each_entry(dock_station, &dock_stations, sibiling) {
+		if (find_dock_dependent_device(dock_station, handle))
+			return 1;
+	}
 
 	return 0;
 }
@@ -341,7 +406,7 @@ static void hotplug_dock_devices(struct 
 
 static void dock_event(struct dock_station *ds, u32 event, int num)
 {
-	struct device *dev = &dock_device->dev;
+	struct device *dev = &ds->dock_device->dev;
 	char event_string[13];
 	char *envp[] = { event_string, NULL };
 
@@ -414,7 +479,7 @@ static void handle_dock(struct dock_stat
 	arg.type = ACPI_TYPE_INTEGER;
 	arg.integer.value = dock;
 	status = acpi_evaluate_object(ds->handle, "_DCK", &arg_list, &buffer);
-	if (ACPI_FAILURE(status))
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
 		printk(KERN_ERR PREFIX "%s - failed to execute _DCK\n",
 			 (char *)name_buffer.pointer);
 	kfree(buffer.pointer);
@@ -498,7 +563,7 @@ static int dock_in_progress(struct dock_
  */
 int register_dock_notifier(struct notifier_block *nb)
 {
-	if (!dock_station)
+	if (!dock_station_count)
 		return -ENODEV;
 
 	return atomic_notifier_chain_register(&dock_notifier_list, nb);
@@ -512,7 +577,7 @@ EXPORT_SYMBOL_GPL(register_dock_notifier
  */
 void unregister_dock_notifier(struct notifier_block *nb)
 {
-	if (!dock_station)
+	if (!dock_station_count)
 		return;
 
 	atomic_notifier_chain_unregister(&dock_notifier_list, nb);
@@ -535,20 +600,23 @@ register_hotplug_dock_device(acpi_handle
 			     void *context)
 {
 	struct dock_dependent_device *dd;
+	struct dock_station *dock_station;
 
-	if (!dock_station)
+	if (!dock_station_count)
 		return -ENODEV;
 
 	/*
 	 * make sure this handle is for a device dependent on the dock,
 	 * this would include the dock station itself
 	 */
-	dd = find_dock_dependent_device(dock_station, handle);
-	if (dd) {
-		dd->handler = handler;
-		dd->context = context;
-		dock_add_hotplug_device(dock_station, dd);
-		return 0;
+	list_for_each_entry(dock_station, &dock_stations, sibiling) {
+		dd = find_dock_dependent_device(dock_station, handle);
+		if (dd) {
+			dd->handler = handler;
+			dd->context = context;
+			dock_add_hotplug_device(dock_station, dd);
+			return 0;
+		}
 	}
 
 	return -EINVAL;
@@ -563,13 +631,16 @@ EXPORT_SYMBOL_GPL(register_hotplug_dock_
 void unregister_hotplug_dock_device(acpi_handle handle)
 {
 	struct dock_dependent_device *dd;
+	struct dock_station *dock_station;
 
-	if (!dock_station)
+	if (!dock_station_count)
 		return;
 
-	dd = find_dock_dependent_device(dock_station, handle);
-	if (dd)
-		dock_del_hotplug_device(dock_station, dd);
+	list_for_each_entry(dock_station, &dock_stations, sibiling) {
+		dd = find_dock_dependent_device(dock_station, handle);
+		if (dd)
+			dock_del_hotplug_device(dock_station, dd);
+	}
 }
 
 EXPORT_SYMBOL_GPL(unregister_hotplug_dock_device);
@@ -620,9 +691,28 @@ static void dock_notify(acpi_handle hand
 {
 	struct dock_station *ds = data;
 	struct acpi_device *tmp;
+	int surprise_removal = 0;
+
+	/*
+	 * According to acpi spec 3.0a, if a DEVICE_CHECK notification
+	 * is sent and _DCK is present, it is assumed to mean an undock
+	 * request.
+	 */
+	if ((ds->flags & DOCK_IS_DOCK) && event == ACPI_NOTIFY_DEVICE_CHECK)
+		event = ACPI_NOTIFY_EJECT_REQUEST;
 
+	/*
+	 * dock station: BUS_CHECK - docked or surprise removal
+	 *		 DEVICE_CHECK - undocked
+	 * other device: BUS_CHECK/DEVICE_CHECK - added or surprise removal
+	 *
+	 * To simplify event handling, dock dependent device handler always
+	 * get ACPI_NOTIFY_BUS_CHECK/ACPI_NOTIFY_DEVICE_CHECK for add and
+	 * ACPI_NOTIFY_EJECT_REQUEST for removal
+	 */
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
+	case ACPI_NOTIFY_DEVICE_CHECK:
 		if (!dock_in_progress(ds) && acpi_bus_get_device(ds->handle,
 		   &tmp)) {
 			begin_dock(ds);
@@ -638,20 +728,17 @@ static void dock_notify(acpi_handle hand
 			complete_dock(ds);
 			dock_event(ds, event, DOCK_EVENT);
 			dock_lock(ds, 1);
+			break;
 		}
-		break;
-	case ACPI_NOTIFY_DEVICE_CHECK:
-	/*
-         * According to acpi spec 3.0a, if a DEVICE_CHECK notification
-         * is sent and _DCK is present, it is assumed to mean an
-         * undock request.  This notify routine will only be called
-         * for objects defining _DCK, so we will fall through to eject
-         * request here.  However, we will pass an eject request through
-	 * to the driver who wish to hotplug.
-         */
+		if (dock_present(ds) || dock_in_progress(ds))
+			break;
+		/* This is a surprise removal */
+		surprise_removal = 1;
+		event = ACPI_NOTIFY_EJECT_REQUEST;
+		/* Fall back */
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		begin_undock(ds);
-		if (immediate_undock)
+		if (immediate_undock || surprise_removal)
 			handle_eject_request(ds, event);
 		else
 			dock_event(ds, event, UNDOCK_EVENT);
@@ -707,6 +794,8 @@ fdd_out:
 static ssize_t show_docked(struct device *dev,
 			   struct device_attribute *attr, char *buf)
 {
+	struct dock_station *dock_station = *((struct dock_station **)
+		dev->platform_data);
 	return snprintf(buf, PAGE_SIZE, "%d\n", dock_present(dock_station));
 
 }
@@ -718,6 +807,8 @@ static DEVICE_ATTR(docked, S_IRUGO, show
 static ssize_t show_flags(struct device *dev,
 			  struct device_attribute *attr, char *buf)
 {
+	struct dock_station *dock_station = *((struct dock_station **)
+		dev->platform_data);
 	return snprintf(buf, PAGE_SIZE, "%d\n", dock_station->flags);
 
 }
@@ -730,6 +821,8 @@ static ssize_t write_undock(struct devic
 			   const char *buf, size_t count)
 {
 	int ret;
+	struct dock_station *dock_station = *((struct dock_station **)
+		dev->platform_data);
 
 	if (!count)
 		return -EINVAL;
@@ -747,6 +840,8 @@ static ssize_t show_dock_uid(struct devi
 			     struct device_attribute *attr, char *buf)
 {
 	unsigned long lbuf;
+	struct dock_station *dock_station = *((struct dock_station **)
+		dev->platform_data);
 	acpi_status status = acpi_evaluate_integer(dock_station->handle,
 					"_UID", NULL, &lbuf);
 	if (ACPI_FAILURE(status))
@@ -768,6 +863,8 @@ static int dock_add(acpi_handle handle)
 	int ret;
 	acpi_status status;
 	struct dock_dependent_device *dd;
+	struct dock_station *dock_station;
+	struct platform_device *dock_device;
 
 	/* allocate & initialize the dock_station private data */
 	dock_station = kzalloc(sizeof(*dock_station), GFP_KERNEL);
@@ -777,22 +874,34 @@ static int dock_add(acpi_handle handle)
 	dock_station->last_dock_time = jiffies - HZ;
 	INIT_LIST_HEAD(&dock_station->dependent_devices);
 	INIT_LIST_HEAD(&dock_station->hotplug_devices);
+	INIT_LIST_HEAD(&dock_station->sibiling);
 	spin_lock_init(&dock_station->dd_lock);
 	mutex_init(&dock_station->hp_lock);
 	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
 
 	/* initialize platform device stuff */
-	dock_device =
-		platform_device_register_simple(dock_device_name, 0, NULL, 0);
+	dock_station->dock_device =
+		platform_device_register_simple(dock_device_name,
+			dock_station_count, NULL, 0);
+	dock_device = dock_station->dock_device;
 	if (IS_ERR(dock_device)) {
 		kfree(dock_station);
 		dock_station = NULL;
 		return PTR_ERR(dock_device);
 	}
+	platform_device_add_data(dock_device, &dock_station,
+		sizeof(struct dock_station *));
 
 	/* we want the dock device to send uevents */
 	dock_device->dev.uevent_suppress = 0;
 
+	if (is_dock(handle))
+		dock_station->flags |= DOCK_IS_DOCK;
+	if (is_ata(handle))
+		dock_station->flags |= DOCK_IS_ATA;
+	if (is_battery(handle))
+		dock_station->flags |= DOCK_IS_BAT;
+
 	ret = device_create_file(&dock_device->dev, &dev_attr_docked);
 	if (ret) {
 		printk("Error %d adding sysfs file\n", ret);
@@ -858,8 +967,8 @@ static int dock_add(acpi_handle handle)
 		goto dock_add_err;
 	}
 
-	printk(KERN_INFO PREFIX "%s\n", ACPI_DOCK_DRIVER_DESCRIPTION);
-
+	dock_station_count++;
+	list_add(&dock_station->sibiling, &dock_stations);
 	return 0;
 
 dock_add_err:
@@ -878,12 +987,13 @@ dock_add_err_unregister:
 /**
  * dock_remove - free up resources related to the dock station
  */
-static int dock_remove(void)
+static int dock_remove(struct dock_station *dock_station)
 {
 	struct dock_dependent_device *dd, *tmp;
 	acpi_status status;
+	struct platform_device *dock_device = dock_station->dock_device;
 
-	if (!dock_station)
+	if (!dock_station_count)
 		return 0;
 
 	/* remove dependent devices */
@@ -923,41 +1033,58 @@ static int dock_remove(void)
 static acpi_status
 find_dock(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
-	int *count = context;
 	acpi_status status = AE_OK;
 
 	if (is_dock(handle)) {
 		if (dock_add(handle) >= 0) {
-			(*count)++;
 			status = AE_CTRL_TERMINATE;
 		}
 	}
 	return status;
 }
 
-static int __init dock_init(void)
+static acpi_status
+find_bay(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
-	int num = 0;
-
-	dock_station = NULL;
+	/* If bay is in a dock, it's already handled */
+	if (is_ejectable_bay(handle) && !is_dock_device(handle))
+		dock_add(handle);
+	return AE_OK;
+}
 
+static int __init dock_init(void)
+{
 	if (acpi_disabled)
 		return 0;
 
 	/* look for a dock station */
 	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
-			    ACPI_UINT32_MAX, find_dock, &num, NULL);
+			    ACPI_UINT32_MAX, find_dock, NULL, NULL);
 
-	if (!num)
-		printk(KERN_INFO "No dock devices found.\n");
+	/* look for bay */
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+			ACPI_UINT32_MAX, find_bay, NULL, NULL);
+	if (!dock_station_count) {
+		printk(KERN_INFO PREFIX "No dock devices found.\n");
+		return 0;
+	}
 
+	printk(KERN_INFO PREFIX "%s: %d docks/bays found\n",
+		ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
 	return 0;
 }
 
 static void __exit dock_exit(void)
 {
-	dock_remove();
+	struct dock_station *dock_station;
+
+	list_for_each_entry(dock_station, &dock_stations, sibiling)
+		dock_remove(dock_station);
 }
 
-postcore_initcall(dock_init);
+/*
+ * Must be called before drivers of devices in dock, otherwise we can't know
+ * which devices are in a dock
+ */
+subsys_initcall(dock_init);
 module_exit(dock_exit);

Re: [patch 4/11]makeing dock driver supports bay and battery hotplug

[Thomas Renninger]

Sorry for the late reply...


On Thursday 28 August 2008 04:03:58 Shaohua Li wrote:
> Making dock driver supports bay and battery hotplug. They are all
> regarded as dock, and unified handled.
>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> ---
...
> +
> +static int is_battery(acpi_handle handle)
> +{
> +	struct acpi_device_info *info;
> +	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> +	int ret = 1;
> +
> +	if (!ACPI_SUCCESS(acpi_get_object_info(handle, &buffer)))
> +		return 0;
> +	info = buffer.pointer;
> +	if (!(info->valid & ACPI_VALID_HID))
> +		ret = 0;
> +	else
> +		ret = !strcmp("PNP0C0A", info->hardware_id.value);
> +
> +	kfree(buffer.pointer);
Better get the device and then use:
const struct acpi_device_id battery_device_ids[] = {
        {"PNP0C0A", 0},
        {"", 0},
};
acpi_match_device_ids(struct acpi_device *device, battery_device_ids);
No need to allocate memory. This also matches batteries where the ID might be 
hidden in the CID list.
...

Something general:
I do not like the idea of this approach.
Battery and ATA device do get an eject notification?
Why can't they handle things themselves?
What is the gain to make battery and ata "dock" capable devices?
What if a PCI(e) device or whatever other device is also connected to the 
docking station. Do we want to add all devices attached to a dock station 
statically?

Would it make sense to add (if notifications are sent to these even this 
should not be needed):
  .eject
  .dock
functions. The dock driver can then go down the tree and call all children's 
eject functions.

I wonder how this can be solved in a more generic way.

Also this patchset mixes up sever fixes and dock design changes patches.
It would be great if the fixes can just go into test, not sure about the 
design change...

    Thomas

Re: [patch 4/11]makeing dock driver supports bay and battery hotplug

[Thomas Renninger]

On Thursday 18 September 2008 19:10:02 Thomas Renninger wrote:
> Sorry for the late reply...
>
> On Thursday 28 August 2008 04:03:58 Shaohua Li wrote:
> > Making dock driver supports bay and battery hotplug. They are all
> > regarded as dock, and unified handled.
> >
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > ---
>
> ...
>
> > +
> > +static int is_battery(acpi_handle handle)
> > +{
> > +	struct acpi_device_info *info;
> > +	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > +	int ret = 1;
> > +
> > +	if (!ACPI_SUCCESS(acpi_get_object_info(handle, &buffer)))
> > +		return 0;
> > +	info = buffer.pointer;
> > +	if (!(info->valid & ACPI_VALID_HID))
> > +		ret = 0;
> > +	else
> > +		ret = !strcmp("PNP0C0A", info->hardware_id.value);
> > +
> > +	kfree(buffer.pointer);
>
> Better get the device and then use:
> const struct acpi_device_id battery_device_ids[] = {
>         {"PNP0C0A", 0},
>         {"", 0},
> };
> acpi_match_device_ids(struct acpi_device *device, battery_device_ids);
> No need to allocate memory. This also matches batteries where the ID might
> be hidden in the CID list.
> ...
Above is still valid.
Below are simply questions to better understand what the patches are for.
Thanks to Holger, I get a bit clearer picture now.
While these patches are needed to get things going for now, would it be 
possible for the future to add a directory "dependent_devices" into a dock's 
sysfs entry and fill it with links to devices for which userspace has to care 
for?

> Something general:
> I do not like the idea of this approach.
> Battery and ATA device do get an eject notification?
> Why can't they handle things themselves?
> What is the gain to make battery and ata "dock" capable devices?
> What if a PCI(e) device or whatever other device is also connected to the
> docking station. Do we want to add all devices attached to a dock station
> statically?
>
> Would it make sense to add (if notifications are sent to these even this
> should not be needed):
>   .eject
>   .dock
> functions. The dock driver can then go down the tree and call all
> children's eject functions.
>
> I wonder how this can be solved in a more generic way.
>
> Also this patchset mixes up sever fixes and dock design changes patches.
> It would be great if the fixes can just go into test, not sure about the
> design change...
Some of the patches looked like it would be worth for .27, but it's too late 
now anyway and the problem I hoped it could fix (kacpid utilizes 100% of CPU 
after suspend, due to _STA -> notify loop) is not solved by these according 
to Holger.

  Thomas

Re: [patch 4/11]makeing dock driver supports bay and battery hotplug

[Shaohua Li]

On Thu, 2008-09-18 at 13:16 -0600, Thomas Renninger wrote:
> On Thursday 18 September 2008 19:10:02 Thomas Renninger wrote:
> > Sorry for the late reply...
> >
> > On Thursday 28 August 2008 04:03:58 Shaohua Li wrote:
> > > Making dock driver supports bay and battery hotplug. They are all
> > > regarded as dock, and unified handled.
> > >
> > > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > > ---
> >
> > ...
> >
> > > +
> > > +static int is_battery(acpi_handle handle)
> > > +{
> > > +   struct acpi_device_info *info;
> > > +   struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > > +   int ret = 1;
> > > +
> > > +   if (!ACPI_SUCCESS(acpi_get_object_info(handle, &buffer)))
> > > +           return 0;
> > > +   info = buffer.pointer;
> > > +   if (!(info->valid & ACPI_VALID_HID))
> > > +           ret = 0;
> > > +   else
> > > +           ret = !strcmp("PNP0C0A", info->hardware_id.value);
> > > +
> > > +   kfree(buffer.pointer);
> >
> > Better get the device and then use:
> > const struct acpi_device_id battery_device_ids[] = {
> >         {"PNP0C0A", 0},
> >         {"", 0},
> > };
> > acpi_match_device_ids(struct acpi_device *device, battery_device_ids);
> > No need to allocate memory. This also matches batteries where the ID might
> > be hidden in the CID list.
> > ...
> Above is still valid.
No, this doesn't work. there isn't a acpi_device because battery is
absent when this is called, but I could add CID support.

> Below are simply questions to better understand what the patches are for.
> Thanks to Holger, I get a bit clearer picture now.
> While these patches are needed to get things going for now, would it be
> possible for the future to add a directory "dependent_devices" into a dock's
> sysfs entry and fill it with links to devices for which userspace has to care
> for?
Yes, we need this. Holger mentioned he want to add this after the patch
is merged. This isn't urgent currently.

> > Something general:
> > I do not like the idea of this approach.
> > Battery and ATA device do get an eject notification?
> > Why can't they handle things themselves?
> > What is the gain to make battery and ata "dock" capable devices?
> > What if a PCI(e) device or whatever other device is also connected to the
> > docking station. Do we want to add all devices attached to a dock station
> > statically?
> >
> > Would it make sense to add (if notifications are sent to these even this
> > should not be needed):
> >   .eject
> >   .dock
> > functions. The dock driver can then go down the tree and call all
> > children's eject functions.
> >
> > I wonder how this can be solved in a more generic way.
> >
> > Also this patchset mixes up sever fixes and dock design changes patches.
> > It would be great if the fixes can just go into test, not sure about the
> > design change...
> Some of the patches looked like it would be worth for .27, but it's too late
> now anyway and the problem I hoped it could fix (kacpid utilizes 100% of CPU
> after suspend, due to _STA -> notify loop) is not solved by these according
> to Holger.
Yes, this patch set just fixed some bugs. Is there a bugzilla for this
issue I can look at?

Thanks,
Shaohua

Re: [patch 4/11]makeing dock driver supports bay and battery hotplug

[Thomas Renninger]

On Monday 22 September 2008 03:24:54 Shaohua Li wrote:
> On Thu, 2008-09-18 at 13:16 -0600, Thomas Renninger wrote:
> > On Thursday 18 September 2008 19:10:02 Thomas Renninger wrote:
...
> > Above is still valid.
>
> No, this doesn't work. there isn't a acpi_device because battery is
> absent when this is called, but I could add CID support.
I don't know whether there are devices with battery ID in the CID list.
With or without the workaround (rewriting cid checking) it's not perfect.

This is an issue I tried to solve years ago.
Ignoring not present devices is wrong IMO.
The whole current design destroys hotplug integration and results in more
and more workarounds (acpi_memoryhotplug and container driver are nice 
examples).
One has to be careful that only _STA and no other functions are called, but 
not present devices should still get registered in some way and somone (the 
driver itself or maybe better the ACPI subsytem and introduce .notify driver 
ops) should listen on Notify AML calls.
Please tell me if someone looks at this again or has ideas/concerns. I always 
wanted to and will at some point, but still don't have the time.

> > Some of the patches looked like it would be worth for .27, but it's too
> > late now anyway and the problem I hoped it could fix (kacpid utilizes
> > 100% of CPU after suspend, due to _STA -> notify loop) is not solved by
> > these according to Holger.
>
> Yes, this patch set just fixed some bugs. Is there a bugzilla for this
> issue I can look at?
[Bug 401740] kacpi* eat a lot of cpu after s2disk
https://bugzilla.novell.com/show_bug.cgi?id=401740

Thanks,

     Thomas

Re: [patch 4/11]makeing dock driver supports bay and battery hotplug

[Holger Macht]

On Mon 22. Sep - 11:17:42, Thomas Renninger wrote:
> On Monday 22 September 2008 03:24:54 Shaohua Li wrote:
> > On Thu, 2008-09-18 at 13:16 -0600, Thomas Renninger wrote:
> > > On Thursday 18 September 2008 19:10:02 Thomas Renninger wrote:
> ...
> > > Above is still valid.
> >
> > No, this doesn't work. there isn't a acpi_device because battery is
> > absent when this is called, but I could add CID support.
> I don't know whether there are devices with battery ID in the CID list.
> With or without the workaround (rewriting cid checking) it's not perfect.
> 
> This is an issue I tried to solve years ago.
> Ignoring not present devices is wrong IMO.
> The whole current design destroys hotplug integration and results in more
> and more workarounds (acpi_memoryhotplug and container driver are nice 
> examples).
> One has to be careful that only _STA and no other functions are called, but 
> not present devices should still get registered in some way and somone (the 
> driver itself or maybe better the ACPI subsytem and introduce .notify driver 
> ops) should listen on Notify AML calls.
> Please tell me if someone looks at this again or has ideas/concerns. I always 
> wanted to and will at some point, but still don't have the time.
> 
> > > Some of the patches looked like it would be worth for .27, but it's too
> > > late now anyway and the problem I hoped it could fix (kacpid utilizes
> > > 100% of CPU after suspend, due to _STA -> notify loop) is not solved by
> > > these according to Holger.
> >
> > Yes, this patch set just fixed some bugs. Is there a bugzilla for this
> > issue I can look at?
> [Bug 401740] kacpi* eat a lot of cpu after s2disk
> https://bugzilla.novell.com/show_bug.cgi?id=401740

The patch series fixes this bug on the Dell D620 I have here. It's more an
design problem we are seeing here, of which I could not find the root
cause yet. The single fixup patches do not help. So maybe there is a
little chance that we still get this patchset in...

Regards,
	Holger

[patch 4/11]makeing dock driver supports bay and battery hotplug

[Shaohua Li]

Making dock driver supports bay and battery hotplug. They are all
regarded as dock, and unified handled.

Signed-off-by: Shaohua Li <shaohua.li@intel.com> 
---
 drivers/acpi/dock.c |  214 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 166 insertions(+), 48 deletions(-)

Index: linux/drivers/acpi/dock.c
===================================================================
--- linux.orig/drivers/acpi/dock.c	2008-08-27 10:22:05.000000000 +0800
+++ linux/drivers/acpi/dock.c	2008-08-27 10:22:10.000000000 +0800
@@ -48,7 +48,6 @@ MODULE_PARM_DESC(immediate_undock, "1 (d
 	" before undocking");
 
 static struct atomic_notifier_head dock_notifier_list;
-static struct platform_device *dock_device;
 static char dock_device_name[] = "dock";
 
 static const struct acpi_device_id dock_device_ids[] = {
@@ -65,7 +64,12 @@ struct dock_station {
 	struct mutex hp_lock;
 	struct list_head dependent_devices;
 	struct list_head hotplug_devices;
+
+	struct list_head sibiling;
+	struct platform_device *dock_device;
 };
+static LIST_HEAD(dock_stations);
+static int dock_station_count;
 
 struct dock_dependent_device {
 	struct list_head list;
@@ -77,11 +81,12 @@ struct dock_dependent_device {
 
 #define DOCK_DOCKING	0x00000001
 #define DOCK_UNDOCKING  0x00000002
+#define DOCK_IS_DOCK	0x00000010
+#define DOCK_IS_ATA	0x00000020
+#define DOCK_IS_BAT	0x00000040
 #define DOCK_EVENT	3
 #define UNDOCK_EVENT	2
 
-static struct dock_station *dock_station;
-
 /*****************************************************************************
  *                         Dock Dependent device functions                   *
  *****************************************************************************/
@@ -199,6 +204,60 @@ static int is_dock(acpi_handle handle)
 	return 1;
 }
 
+static int is_ejectable(acpi_handle handle)
+{
+	acpi_status status;
+	acpi_handle tmp;
+
+	status = acpi_get_handle(handle, "_EJ0", &tmp);
+	if (ACPI_FAILURE(status))
+		return 0;
+	return 1;
+}
+
+static int is_ata(acpi_handle handle)
+{
+	acpi_handle tmp;
+
+	if ((ACPI_SUCCESS(acpi_get_handle(handle, "_GTF", &tmp))) ||
+	   (ACPI_SUCCESS(acpi_get_handle(handle, "_GTM", &tmp))) ||
+	   (ACPI_SUCCESS(acpi_get_handle(handle, "_STM", &tmp))) ||
+	   (ACPI_SUCCESS(acpi_get_handle(handle, "_SDD", &tmp))))
+		return 1;
+
+	return 0;
+}
+
+static int is_battery(acpi_handle handle)
+{
+	struct acpi_device_info *info;
+	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
+	int ret = 1;
+
+	if (!ACPI_SUCCESS(acpi_get_object_info(handle, &buffer)))
+		return 0;
+	info = buffer.pointer;
+	if (!(info->valid & ACPI_VALID_HID))
+		ret = 0;
+	else
+		ret = !strcmp("PNP0C0A", info->hardware_id.value);
+
+	kfree(buffer.pointer);
+	return ret;
+}
+
+static int is_ejectable_bay(acpi_handle handle)
+{
+	acpi_handle phandle;
+	if (!is_ejectable(handle))
+		return 0;
+	if (is_battery(handle) || is_ata(handle))
+		return 1;
+	if (!acpi_get_parent(handle, &phandle) && is_ata(phandle))
+		return 1;
+	return 0;
+}
+
 /**
  * is_dock_device - see if a device is on a dock station
  * @handle: acpi handle of the device
@@ -209,11 +268,17 @@ static int is_dock(acpi_handle handle)
  */
 int is_dock_device(acpi_handle handle)
 {
-	if (!dock_station)
+	struct dock_station *dock_station;
+
+	if (!dock_station_count)
 		return 0;
 
-	if (is_dock(handle) || find_dock_dependent_device(dock_station, handle))
+	if (is_dock(handle))
 		return 1;
+	list_for_each_entry(dock_station, &dock_stations, sibiling) {
+		if (find_dock_dependent_device(dock_station, handle))
+			return 1;
+	}
 
 	return 0;
 }
@@ -341,7 +406,7 @@ static void hotplug_dock_devices(struct 
 
 static void dock_event(struct dock_station *ds, u32 event, int num)
 {
-	struct device *dev = &dock_device->dev;
+	struct device *dev = &ds->dock_device->dev;
 	char event_string[13];
 	char *envp[] = { event_string, NULL };
 
@@ -414,7 +479,7 @@ static void handle_dock(struct dock_stat
 	arg.type = ACPI_TYPE_INTEGER;
 	arg.integer.value = dock;
 	status = acpi_evaluate_object(ds->handle, "_DCK", &arg_list, &buffer);
-	if (ACPI_FAILURE(status))
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
 		printk(KERN_ERR PREFIX "%s - failed to execute _DCK\n",
 			 (char *)name_buffer.pointer);
 	kfree(buffer.pointer);
@@ -495,7 +560,7 @@ static int dock_in_progress(struct dock_
  */
 int register_dock_notifier(struct notifier_block *nb)
 {
-	if (!dock_station)
+	if (!dock_station_count)
 		return -ENODEV;
 
 	return atomic_notifier_chain_register(&dock_notifier_list, nb);
@@ -509,7 +574,7 @@ EXPORT_SYMBOL_GPL(register_dock_notifier
  */
 void unregister_dock_notifier(struct notifier_block *nb)
 {
-	if (!dock_station)
+	if (!dock_station_count)
 		return;
 
 	atomic_notifier_chain_unregister(&dock_notifier_list, nb);
@@ -532,20 +597,23 @@ register_hotplug_dock_device(acpi_handle
 			     void *context)
 {
 	struct dock_dependent_device *dd;
+	struct dock_station *dock_station;
 
-	if (!dock_station)
+	if (!dock_station_count)
 		return -ENODEV;
 
 	/*
 	 * make sure this handle is for a device dependent on the dock,
 	 * this would include the dock station itself
 	 */
-	dd = find_dock_dependent_device(dock_station, handle);
-	if (dd) {
-		dd->handler = handler;
-		dd->context = context;
-		dock_add_hotplug_device(dock_station, dd);
-		return 0;
+	list_for_each_entry(dock_station, &dock_stations, sibiling) {
+		dd = find_dock_dependent_device(dock_station, handle);
+		if (dd) {
+			dd->handler = handler;
+			dd->context = context;
+			dock_add_hotplug_device(dock_station, dd);
+			return 0;
+		}
 	}
 
 	return -EINVAL;
@@ -560,13 +628,16 @@ EXPORT_SYMBOL_GPL(register_hotplug_dock_
 void unregister_hotplug_dock_device(acpi_handle handle)
 {
 	struct dock_dependent_device *dd;
+	struct dock_station *dock_station;
 
-	if (!dock_station)
+	if (!dock_station_count)
 		return;
 
-	dd = find_dock_dependent_device(dock_station, handle);
-	if (dd)
-		dock_del_hotplug_device(dock_station, dd);
+	list_for_each_entry(dock_station, &dock_stations, sibiling) {
+		dd = find_dock_dependent_device(dock_station, handle);
+		if (dd)
+			dock_del_hotplug_device(dock_station, dd);
+	}
 }
 
 EXPORT_SYMBOL_GPL(unregister_hotplug_dock_device);
@@ -617,9 +688,28 @@ static void dock_notify(acpi_handle hand
 {
 	struct dock_station *ds = data;
 	struct acpi_device *tmp;
+	int surprise_removal = 0;
 
+	/*
+         * According to acpi spec 3.0a, if a DEVICE_CHECK notification
+	 * is sent and _DCK is present, it is assumed to mean an undock
+	 * request.
+	 */
+	if ((ds->flags & DOCK_IS_DOCK) && event == ACPI_NOTIFY_DEVICE_CHECK)
+		event = ACPI_NOTIFY_EJECT_REQUEST;
+
+	/*
+	 * dock station: BUS_CHECK - docked or surprise removal
+	 *		 DEVICE_CHECK - undocked
+	 * other device: BUS_CHECK/DEVICE_CHECK - added or surprise removal
+	 *
+	 * To simplify event handling, dock dependent device handler always
+	 * get ACPI_NOTIFY_BUS_CHECK/ACPI_NOTIFY_DEVICE_CHECK for add and
+	 * ACPI_NOTIFY_EJECT_REQUEST for removal
+	 */
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
+	case ACPI_NOTIFY_DEVICE_CHECK:
 		if (!dock_in_progress(ds) && acpi_bus_get_device(ds->handle, &tmp)) {
 			begin_dock(ds);
 			dock(ds);
@@ -634,20 +724,17 @@ static void dock_notify(acpi_handle hand
 			complete_dock(ds);
 			dock_event(ds, event, DOCK_EVENT);
 			dock_lock(ds, 1);
+			break;
 		}
-		break;
-	case ACPI_NOTIFY_DEVICE_CHECK:
-	/*
-         * According to acpi spec 3.0a, if a DEVICE_CHECK notification
-         * is sent and _DCK is present, it is assumed to mean an
-         * undock request.  This notify routine will only be called
-         * for objects defining _DCK, so we will fall through to eject
-         * request here.  However, we will pass an eject request through
-	 * to the driver who wish to hotplug.
-         */
+		if (dock_present(ds) || dock_in_progress(ds))
+			break;
+		/* This is a surprise removal */
+		surprise_removal = 1;
+		event = ACPI_NOTIFY_EJECT_REQUEST;
+		/* Fall back */
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		begin_undock(ds);
-		if (immediate_undock)
+		if (immediate_undock || surprise_removal)
 			handle_eject_request(ds, event);
 		else
 			dock_event(ds, event, UNDOCK_EVENT);
@@ -703,6 +790,7 @@ fdd_out:
 static ssize_t show_docked(struct device *dev,
 			   struct device_attribute *attr, char *buf)
 {
+	struct dock_station *dock_station = *((struct dock_station **)dev->platform_data);
 	return snprintf(buf, PAGE_SIZE, "%d\n", dock_present(dock_station));
 
 }
@@ -714,6 +802,7 @@ static DEVICE_ATTR(docked, S_IRUGO, show
 static ssize_t show_flags(struct device *dev,
 			  struct device_attribute *attr, char *buf)
 {
+	struct dock_station *dock_station = *((struct dock_station **)dev->platform_data);
 	return snprintf(buf, PAGE_SIZE, "%d\n", dock_station->flags);
 
 }
@@ -726,6 +815,7 @@ static ssize_t write_undock(struct devic
 			   const char *buf, size_t count)
 {
 	int ret;
+	struct dock_station *dock_station = *((struct dock_station **)dev->platform_data);
 
 	if (!count)
 		return -EINVAL;
@@ -743,6 +833,7 @@ static ssize_t show_dock_uid(struct devi
 			     struct device_attribute *attr, char *buf)
 {
 	unsigned long lbuf;
+	struct dock_station *dock_station = *((struct dock_station **)dev->platform_data);
 	acpi_status status = acpi_evaluate_integer(dock_station->handle,
 					"_UID", NULL, &lbuf);
 	if (ACPI_FAILURE(status))
@@ -764,6 +855,8 @@ static int dock_add(acpi_handle handle)
 	int ret;
 	acpi_status status;
 	struct dock_dependent_device *dd;
+	struct dock_station *dock_station;
+	struct platform_device *dock_device;
 
 	/* allocate & initialize the dock_station private data */
 	dock_station = kzalloc(sizeof(*dock_station), GFP_KERNEL);
@@ -773,22 +866,33 @@ static int dock_add(acpi_handle handle)
 	dock_station->last_dock_time = jiffies - HZ;
 	INIT_LIST_HEAD(&dock_station->dependent_devices);
 	INIT_LIST_HEAD(&dock_station->hotplug_devices);
+	INIT_LIST_HEAD(&dock_station->sibiling);
 	spin_lock_init(&dock_station->dd_lock);
 	mutex_init(&dock_station->hp_lock);
 	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
 
 	/* initialize platform device stuff */
-	dock_device =
-		platform_device_register_simple(dock_device_name, 0, NULL, 0);
+	dock_station->dock_device =
+		platform_device_register_simple(dock_device_name,
+			dock_station_count, NULL, 0);
+	dock_device = dock_station->dock_device;
 	if (IS_ERR(dock_device)) {
 		kfree(dock_station);
 		dock_station = NULL;
 		return PTR_ERR(dock_device);
 	}
+	platform_device_add_data(dock_device, &dock_station, sizeof(struct dock_station *));
 
 	/* we want the dock device to send uevents */
 	dock_device->dev.uevent_suppress = 0;
 
+	if (is_dock(handle))
+		dock_station->flags |= DOCK_IS_DOCK;
+	if (is_ata(handle))
+		dock_station->flags |= DOCK_IS_ATA;
+	if (is_battery(handle))
+		dock_station->flags |= DOCK_IS_BAT;
+
 	ret = device_create_file(&dock_device->dev, &dev_attr_docked);
 	if (ret) {
 		printk("Error %d adding sysfs file\n", ret);
@@ -854,8 +958,8 @@ static int dock_add(acpi_handle handle)
 		goto dock_add_err;
 	}
 
-	printk(KERN_INFO PREFIX "%s\n", ACPI_DOCK_DRIVER_DESCRIPTION);
-
+	dock_station_count++;
+	list_add(&dock_station->sibiling, &dock_stations);
 	return 0;
 
 dock_add_err:
@@ -874,12 +978,13 @@ dock_add_err_unregister:
 /**
  * dock_remove - free up resources related to the dock station
  */
-static int dock_remove(void)
+static int dock_remove(struct dock_station *dock_station)
 {
 	struct dock_dependent_device *dd, *tmp;
 	acpi_status status;
+	struct platform_device *dock_device = dock_station->dock_device;
 
-	if (!dock_station)
+	if (!dock_station_count)
 		return 0;
 
 	/* remove dependent devices */
@@ -919,41 +1024,54 @@ static int dock_remove(void)
 static acpi_status
 find_dock(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
-	int *count = context;
 	acpi_status status = AE_OK;
 
 	if (is_dock(handle)) {
 		if (dock_add(handle) >= 0) {
-			(*count)++;
 			status = AE_CTRL_TERMINATE;
 		}
 	}
 	return status;
 }
 
-static int __init dock_init(void)
+static acpi_status
+find_bay(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
-	int num = 0;
-
-	dock_station = NULL;
+	/* If bay is in a dock, it's already handled */
+	if (is_ejectable_bay(handle) && !is_dock_device(handle))
+		dock_add(handle);
+	return AE_OK;
+}
 
+static int __init dock_init(void)
+{
 	if (acpi_disabled)
 		return 0;
 
 	/* look for a dock station */
 	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
-			    ACPI_UINT32_MAX, find_dock, &num, NULL);
+			    ACPI_UINT32_MAX, find_dock, NULL, NULL);
 
-	if (!num)
-		printk(KERN_INFO "No dock devices found.\n");
+	/* look for bay */
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+			ACPI_UINT32_MAX, find_bay, NULL, NULL);
+	if (!dock_station_count) {
+		printk(KERN_INFO PREFIX "No dock devices found.\n");
+		return 0;
+	}
 
+	printk(KERN_INFO PREFIX "%s: %d docks/bays found\n",
+		ACPI_DOCK_DRIVER_DESCRIPTION, dock_station_count);
 	return 0;
 }
 
 static void __exit dock_exit(void)
 {
-	dock_remove();
+	struct dock_station *dock_station;
+
+	list_for_each_entry(dock_station, &dock_stations, sibiling)
+		dock_remove(dock_station);
 }
 
-postcore_initcall(dock_init);
+subsys_initcall(dock_init);
 module_exit(dock_exit);


--
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 4/11]makeing dock driver supports bay and battery hotplug

[Andi Kleen]

Shaohua Li <shaohua.li@intel.com> writes:

> @@ -714,6 +802,7 @@ static DEVICE_ATTR(docked, S_IRUGO, show
>  static ssize_t show_flags(struct device *dev,
>  			  struct device_attribute *attr, char *buf)
>  {
> +	struct dock_station *dock_station = *((struct dock_station **)dev->platform_data);

80 characters? multiple occurrences.

>  
> -postcore_initcall(dock_init);
> +subsys_initcall(dock_init);

Why so early?  

First that needs a comment. Also I don't understand it
(especially since it's modular too) 

-Andi
-- 
ak@linux.intel.com