[PATCH v3 0/6] ACPI: dock: code hygiene

[PATCH v3 0/6] ACPI: dock: code hygiene

[Alex Chiang]

This is v3 of a modest attempt to clean up the dock driver.

As before, compile tested only, since I don't have any machines
that provide _DCK.

Thanks.
/ac

v2 -> v3:
	- now using an attribute_group, as suggested by Dmitry Torokhov

v1 -> v2:
	- changed how we add platform device data, based on Shaohua Li's
	  review
---

Alex Chiang (6):
      ACPI: dock: convert sysfs attributes to an attribute_group
      ACPI: dock: combine add|alloc_dock_dependent_device
      ACPI: dock: remove global 'dock_device_name'
      ACPI: dock: dock_add - hoist up platform_device_register_simple()
      ACPI: dock: add struct dock_station * directly to platform device data
      ACPI: dock: minor whitespace and style cleanups


 drivers/acpi/dock.c |  265 ++++++++++++++++++---------------------------------
 1 files changed, 92 insertions(+), 173 deletions(-)

[PATCH v3 1/6] ACPI: dock: convert sysfs attributes to an attribute_group

[Alex Chiang]

As suggested by Dmitry Torokhov, convert the individual sysfs
attributes into an attribute group.

This change eliminates quite a bit of copy/paste code in the
error handling paths.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Alex Chiang <achiang@hp.com>
---

 drivers/acpi/dock.c |   80 +++++++++++++++------------------------------------
 1 files changed, 23 insertions(+), 57 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 7338b6a..3f71f0a 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -936,6 +936,19 @@ static ssize_t show_dock_type(struct device *dev,
 }
 static DEVICE_ATTR(type, S_IRUGO, show_dock_type, NULL);
 
+static struct attribute *dock_attributes[] = {
+	&dev_attr_docked.attr,
+	&dev_attr_flags.attr,
+	&dev_attr_undock.attr,
+	&dev_attr_uid.attr,
+	&dev_attr_type.attr,
+	NULL
+};
+
+static struct attribute_group dock_attribute_group = {
+	.attrs = dock_attributes
+};
+
 /**
  * dock_add - add a new dock station
  * @handle: the dock station handle
@@ -969,9 +982,8 @@ static int dock_add(acpi_handle handle)
 			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);
+		ret = PTR_ERR(dock_device);
+		goto out;
 	}
 	platform_device_add_data(dock_device, &dock_station,
 		sizeof(struct dock_station *));
@@ -986,47 +998,9 @@ static int dock_add(acpi_handle handle)
 	if (is_battery(handle))
 		dock_station->flags |= DOCK_IS_BAT;
 
-	ret = device_create_file(&dock_device->dev, &dev_attr_docked);
-	if (ret) {
-		printk(KERN_ERR "Error %d adding sysfs file\n", ret);
-		platform_device_unregister(dock_device);
-		kfree(dock_station);
-		dock_station = NULL;
-		return ret;
-	}
-	ret = device_create_file(&dock_device->dev, &dev_attr_undock);
-	if (ret) {
-		printk(KERN_ERR "Error %d adding sysfs file\n", ret);
-		device_remove_file(&dock_device->dev, &dev_attr_docked);
-		platform_device_unregister(dock_device);
-		kfree(dock_station);
-		dock_station = NULL;
-		return ret;
-	}
-	ret = device_create_file(&dock_device->dev, &dev_attr_uid);
-	if (ret) {
-		printk(KERN_ERR "Error %d adding sysfs file\n", ret);
-		device_remove_file(&dock_device->dev, &dev_attr_docked);
-		device_remove_file(&dock_device->dev, &dev_attr_undock);
-		platform_device_unregister(dock_device);
-		kfree(dock_station);
-		dock_station = NULL;
-		return ret;
-	}
-	ret = device_create_file(&dock_device->dev, &dev_attr_flags);
-	if (ret) {
-		printk(KERN_ERR "Error %d adding sysfs file\n", ret);
-		device_remove_file(&dock_device->dev, &dev_attr_docked);
-		device_remove_file(&dock_device->dev, &dev_attr_undock);
-		device_remove_file(&dock_device->dev, &dev_attr_uid);
-		platform_device_unregister(dock_device);
-		kfree(dock_station);
-		dock_station = NULL;
-		return ret;
-	}
-	ret = device_create_file(&dock_device->dev, &dev_attr_type);
+	ret = sysfs_create_group(&dock_device->dev.kobj, &dock_attribute_group);
 	if (ret)
-		printk(KERN_ERR"Error %d adding sysfs file\n", ret);
+		goto err_unregister;
 
 	/* Find dependent devices */
 	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
@@ -1036,10 +1010,8 @@ static int dock_add(acpi_handle handle)
 	/* add the dock station as a device dependent on itself */
 	dd = alloc_dock_dependent_device(handle);
 	if (!dd) {
-		kfree(dock_station);
-		dock_station = NULL;
 		ret = -ENOMEM;
-		goto dock_add_err_unregister;
+		goto err_unregister;
 	}
 	add_dock_dependent_device(dock_station, dd);
 
@@ -1047,13 +1019,11 @@ static int dock_add(acpi_handle handle)
 	list_add(&dock_station->sibling, &dock_stations);
 	return 0;
 
-dock_add_err_unregister:
-	device_remove_file(&dock_device->dev, &dev_attr_type);
-	device_remove_file(&dock_device->dev, &dev_attr_docked);
-	device_remove_file(&dock_device->dev, &dev_attr_undock);
-	device_remove_file(&dock_device->dev, &dev_attr_uid);
-	device_remove_file(&dock_device->dev, &dev_attr_flags);
+err_unregister:
+	printk(KERN_ERR "%s encountered error %d\n", __func__, ret);
+	sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
 	platform_device_unregister(dock_device);
+out:
 	kfree(dock_station);
 	dock_station = NULL;
 	return ret;
@@ -1076,11 +1046,7 @@ static int dock_remove(struct dock_station *dock_station)
 	    kfree(dd);
 
 	/* cleanup sysfs */
-	device_remove_file(&dock_device->dev, &dev_attr_type);
-	device_remove_file(&dock_device->dev, &dev_attr_docked);
-	device_remove_file(&dock_device->dev, &dev_attr_undock);
-	device_remove_file(&dock_device->dev, &dev_attr_uid);
-	device_remove_file(&dock_device->dev, &dev_attr_flags);
+	sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
 	platform_device_unregister(dock_device);
 
 	/* free dock station memory */

[PATCH v3 2/6] ACPI: dock: combine add|alloc_dock_dependent_device

[Alex Chiang]

There's no real need to have a separate allocation step when adding
a dock dependent device.

Combining the two functions is both logical and helps with legibility.

Signed-off-by: Alex Chiang <achiang@hp.com>
---

 drivers/acpi/dock.c |   55 ++++++++++++++++++---------------------------------
 1 files changed, 19 insertions(+), 36 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 3f71f0a..7a95113 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -93,40 +93,30 @@ struct dock_dependent_device {
  *                         Dock Dependent device functions                   *
  *****************************************************************************/
 /**
- *  alloc_dock_dependent_device - allocate and init a dependent device
- *  @handle: the acpi_handle of the dependent device
+ * add_dock_dependent_device - associate a device with the dock station
+ * @handle: handle of the dependent device
+ * @ds: The dock station
  *
- *  Allocate memory for a dependent device structure for a device referenced
- *  by the acpi handle
+ * Add the dependent device to the dock's dependent device list.
  */
-static struct dock_dependent_device *
-alloc_dock_dependent_device(acpi_handle handle)
+static int
+add_dock_dependent_device(acpi_handle handle, struct dock_station *ds)
 {
 	struct dock_dependent_device *dd;
 
 	dd = kzalloc(sizeof(*dd), GFP_KERNEL);
-	if (dd) {
-		dd->handle = handle;
-		INIT_LIST_HEAD(&dd->list);
-		INIT_LIST_HEAD(&dd->hotplug_list);
-	}
-	return dd;
-}
+	if (!dd)
+		return -ENOMEM;
+
+	dd->handle = handle;
+	INIT_LIST_HEAD(&dd->list);
+	INIT_LIST_HEAD(&dd->hotplug_list);
 
-/**
- * add_dock_dependent_device - associate a device with the dock station
- * @ds: The dock station
- * @dd: The dependent device
- *
- * Add the dependent device to the dock's dependent device list.
- */
-static void
-add_dock_dependent_device(struct dock_station *ds,
-			  struct dock_dependent_device *dd)
-{
 	spin_lock(&ds->dd_lock);
 	list_add_tail(&dd->list, &ds->dependent_devices);
 	spin_unlock(&ds->dd_lock);
+
+	return 0;
 }
 
 /**
@@ -826,7 +816,6 @@ find_dock_devices(acpi_handle handle, u32 lvl, void *context, void **rv)
 	acpi_status status;
 	acpi_handle tmp, parent;
 	struct dock_station *ds = context;
-	struct dock_dependent_device *dd;
 
 	status = acpi_bus_get_ejd(handle, &tmp);
 	if (ACPI_FAILURE(status)) {
@@ -840,11 +829,9 @@ find_dock_devices(acpi_handle handle, u32 lvl, void *context, void **rv)
 			goto fdd_out;
 	}
 
-	if (tmp == ds->handle) {
-		dd = alloc_dock_dependent_device(handle);
-		if (dd)
-			add_dock_dependent_device(ds, dd);
-	}
+	if (tmp == ds->handle)
+		add_dock_dependent_device(ds, handle);
+
 fdd_out:
 	return AE_OK;
 }
@@ -959,7 +946,6 @@ static struct attribute_group dock_attribute_group = {
 static int dock_add(acpi_handle handle)
 {
 	int ret;
-	struct dock_dependent_device *dd;
 	struct dock_station *dock_station;
 	struct platform_device *dock_device;
 
@@ -1008,12 +994,9 @@ static int dock_add(acpi_handle handle)
 			    NULL);
 
 	/* add the dock station as a device dependent on itself */
-	dd = alloc_dock_dependent_device(handle);
-	if (!dd) {
-		ret = -ENOMEM;
+	ret = add_dock_dependent_device(dock_station, handle);
+	if (ret)
 		goto err_unregister;
-	}
-	add_dock_dependent_device(dock_station, dd);
 
 	dock_station_count++;
 	list_add(&dock_station->sibling, &dock_stations);

[PATCH v3 3/6] ACPI: dock: remove global 'dock_device_name'

[Alex Chiang]

We only use it in one spot, so it probably gets optimized out, but there's
still no need to use a global variable for this.

Signed-off-by: Alex Chiang <achiang@hp.com>
---

 drivers/acpi/dock.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 7a95113..766eccb 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -50,7 +50,6 @@ MODULE_PARM_DESC(immediate_undock, "1 (default) will cause the driver to "
 	" before undocking");
 
 static struct atomic_notifier_head dock_notifier_list;
-static char dock_device_name[] = "dock";
 
 static const struct acpi_device_id dock_device_ids[] = {
 	{"LNXDOCK", 0},
@@ -964,7 +963,7 @@ static int dock_add(acpi_handle handle)
 
 	/* initialize platform device stuff */
 	dock_station->dock_device =
-		platform_device_register_simple(dock_device_name,
+		platform_device_register_simple("dock",
 			dock_station_count, NULL, 0);
 	dock_device = dock_station->dock_device;
 	if (IS_ERR(dock_device)) {

[PATCH v3 4/6] ACPI: dock: dock_add - hoist up platform_device_register_simple()

[Alex Chiang]

Move the call to platform_device_register_simple so that we do it
before allocating and initializing our struct dock_station.

Signed-off-by: Alex Chiang <achiang@hp.com>
---

 drivers/acpi/dock.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 766eccb..939babe 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -948,11 +948,18 @@ static int dock_add(acpi_handle handle)
 	struct dock_station *dock_station;
 	struct platform_device *dock_device;
 
+	dock_device =
+		platform_device_register_simple("dock",
+			dock_station_count, NULL, 0);
+	if (IS_ERR(dock_device))
+		return PTR_ERR(dock_device);
+
 	/* allocate & initialize the dock_station private data */
 	dock_station = kzalloc(sizeof(*dock_station), GFP_KERNEL);
 	if (!dock_station)
 		return -ENOMEM;
 	dock_station->handle = handle;
+	dock_station->dock_device = dock_device;
 	dock_station->last_dock_time = jiffies - HZ;
 	INIT_LIST_HEAD(&dock_station->dependent_devices);
 	INIT_LIST_HEAD(&dock_station->hotplug_devices);
@@ -961,15 +968,6 @@ static int dock_add(acpi_handle handle)
 	mutex_init(&dock_station->hp_lock);
 	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
 
-	/* initialize platform device stuff */
-	dock_station->dock_device =
-		platform_device_register_simple("dock",
-			dock_station_count, NULL, 0);
-	dock_device = dock_station->dock_device;
-	if (IS_ERR(dock_device)) {
-		ret = PTR_ERR(dock_device);
-		goto out;
-	}
 	platform_device_add_data(dock_device, &dock_station,
 		sizeof(struct dock_station *));
 

[PATCH v3 5/6] ACPI: dock: add struct dock_station * directly to platform device data

[Alex Chiang]

Instead of adding a (struct dock_station **) to our dock device's
platform data, we can add the (struct dock_station *) directly.

This change saves us some ugly casting and improves readability.

The cost of making this change is an extra 290 bytes of stack usage,
but this is an infrequently called code-path and unlikely to cause
the kernel to blow up.

Signed-off-by: Alex Chiang <achiang@hp.com>
---

 drivers/acpi/dock.c |   40 +++++++++++++---------------------------
 1 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 939babe..e0441fb 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -843,8 +843,7 @@ static ssize_t show_docked(struct device *dev,
 {
 	struct acpi_device *tmp;
 
-	struct dock_station *dock_station = *((struct dock_station **)
-		dev->platform_data);
+	struct dock_station *dock_station = dev->platform_data;
 
 	if (ACPI_SUCCESS(acpi_bus_get_device(dock_station->handle, &tmp)))
 		return snprintf(buf, PAGE_SIZE, "1\n");
@@ -858,8 +857,7 @@ static DEVICE_ATTR(docked, S_IRUGO, show_docked, NULL);
 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);
+	struct dock_station *dock_station = dev->platform_data;
 	return snprintf(buf, PAGE_SIZE, "%d\n", dock_station->flags);
 
 }
@@ -872,8 +870,7 @@ static ssize_t write_undock(struct device *dev, struct device_attribute *attr,
 			   const char *buf, size_t count)
 {
 	int ret;
-	struct dock_station *dock_station = *((struct dock_station **)
-		dev->platform_data);
+	struct dock_station *dock_station = dev->platform_data;
 
 	if (!count)
 		return -EINVAL;
@@ -891,8 +888,7 @@ static ssize_t show_dock_uid(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
 	unsigned long long lbuf;
-	struct dock_station *dock_station = *((struct dock_station **)
-		dev->platform_data);
+	struct dock_station *dock_station = dev->platform_data;
 	acpi_status status = acpi_evaluate_integer(dock_station->handle,
 					"_UID", NULL, &lbuf);
 	if (ACPI_FAILURE(status))
@@ -905,8 +901,7 @@ static DEVICE_ATTR(uid, S_IRUGO, show_dock_uid, NULL);
 static ssize_t show_dock_type(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct dock_station *dock_station = *((struct dock_station **)
-		dev->platform_data);
+	struct dock_station *dock_station = dev->platform_data;
 	char *type;
 
 	if (dock_station->flags & DOCK_IS_DOCK)
@@ -944,20 +939,18 @@ static struct attribute_group dock_attribute_group = {
  */
 static int dock_add(acpi_handle handle)
 {
-	int ret;
-	struct dock_station *dock_station;
+	int ret, id;
+	struct dock_station ds, *dock_station;
 	struct platform_device *dock_device;
 
+	id = dock_station_count;
 	dock_device =
-		platform_device_register_simple("dock",
-			dock_station_count, NULL, 0);
+		platform_device_register_data(NULL, "dock",
+			id, &ds, sizeof(ds));
 	if (IS_ERR(dock_device))
 		return PTR_ERR(dock_device);
 
-	/* allocate & initialize the dock_station private data */
-	dock_station = kzalloc(sizeof(*dock_station), GFP_KERNEL);
-	if (!dock_station)
-		return -ENOMEM;
+	dock_station = dock_device->dev.platform_data;
 	dock_station->handle = handle;
 	dock_station->dock_device = dock_device;
 	dock_station->last_dock_time = jiffies - HZ;
@@ -968,9 +961,6 @@ static int dock_add(acpi_handle handle)
 	mutex_init(&dock_station->hp_lock);
 	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
 
-	platform_device_add_data(dock_device, &dock_station,
-		sizeof(struct dock_station *));
-
 	/* we want the dock device to send uevents */
 	dev_set_uevent_suppress(&dock_device->dev, 0);
 
@@ -1003,9 +993,6 @@ err_unregister:
 	printk(KERN_ERR "%s encountered error %d\n", __func__, ret);
 	sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
 	platform_device_unregister(dock_device);
-out:
-	kfree(dock_station);
-	dock_station = NULL;
 	return ret;
 }
 
@@ -1025,13 +1012,12 @@ static int dock_remove(struct dock_station *dock_station)
 				 list)
 	    kfree(dd);
 
+	list_del(&dock_station->sibling);
+
 	/* cleanup sysfs */
 	sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
 	platform_device_unregister(dock_device);
 
-	/* free dock station memory */
-	kfree(dock_station);
-	dock_station = NULL;
 	return 0;
 }
 

[PATCH v3 6/6] ACPI: dock: minor whitespace and style cleanups

[Alex Chiang]

Removed some stray whitespaces
Added whitespace when needed for legibility
Removed unneeded curly braces
Removed useless void casts
Removed unnecessary local variable initialization
Renamed variables to help out with 80-column fixes

Signed-off-by: Alex Chiang <achiang@hp.com>
---

 drivers/acpi/dock.c |  103 ++++++++++++++++++++++-----------------------------
 1 files changed, 45 insertions(+), 58 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index e0441fb..8f085e5 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -238,6 +238,7 @@ static int is_battery(acpi_handle handle)
 static int is_ejectable_bay(acpi_handle handle)
 {
 	acpi_handle phandle;
+
 	if (!is_ejectable(handle))
 		return 0;
 	if (is_battery(handle) || is_ata(handle))
@@ -264,14 +265,13 @@ int is_dock_device(acpi_handle handle)
 
 	if (is_dock(handle))
 		return 1;
-	list_for_each_entry(dock_station, &dock_stations, sibling) {
+
+	list_for_each_entry(dock_station, &dock_stations, sibling)
 		if (find_dock_dependent_device(dock_station, handle))
 			return 1;
-	}
 
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(is_dock_device);
 
 /**
@@ -294,8 +294,6 @@ static int dock_present(struct dock_station *ds)
 	return 0;
 }
 
-
-
 /**
  * dock_create_acpi_device - add new devices to acpi
  * @handle - handle of the device to add
@@ -309,7 +307,7 @@ static int dock_present(struct dock_station *ds)
  */
 static struct acpi_device * dock_create_acpi_device(acpi_handle handle)
 {
-	struct acpi_device *device = NULL;
+	struct acpi_device *device;
 	struct acpi_device *parent_device;
 	acpi_handle parent;
 	int ret;
@@ -326,8 +324,7 @@ static struct acpi_device * dock_create_acpi_device(acpi_handle handle)
 		ret = acpi_bus_add(&device, parent_device, handle,
 			ACPI_BUS_TYPE_DEVICE);
 		if (ret) {
-			pr_debug("error adding bus, %x\n",
-				-ret);
+			pr_debug("error adding bus, %x\n", -ret);
 			return NULL;
 		}
 	}
@@ -353,7 +350,6 @@ static void dock_remove_acpi_device(acpi_handle handle)
 	}
 }
 
-
 /**
  * hotplug_dock_devices - insert or remove devices on the dock station
  * @ds: the dock station
@@ -373,10 +369,9 @@ static void hotplug_dock_devices(struct dock_station *ds, u32 event)
 	/*
 	 * First call driver specific hotplug functions
 	 */
-	list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list) {
+	list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list)
 		if (dd->ops && dd->ops->handler)
 			dd->ops->handler(dd->handle, event, dd->context);
-	}
 
 	/*
 	 * Now make sure that an acpi_device is created for each
@@ -415,6 +410,7 @@ static void dock_event(struct dock_station *ds, u32 event, int num)
 	list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list)
 		if (dd->ops && dd->ops->uevent)
 			dd->ops->uevent(dd->handle, event, dd->context);
+
 	if (num != DOCK_EVENT)
 		kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
 }
@@ -445,8 +441,8 @@ static void eject_dock(struct dock_station *ds)
 	arg.type = ACPI_TYPE_INTEGER;
 	arg.integer.value = 1;
 
-	if (ACPI_FAILURE(acpi_evaluate_object(ds->handle, "_EJ0",
-					      &arg_list, NULL)))
+	status = acpi_evaluate_object(ds->handle, "_EJ0", &arg_list, NULL);
+	if (ACPI_FAILURE(status))
 		pr_debug("Failed to evaluate _EJ0!\n");
 }
 
@@ -566,7 +562,6 @@ int register_dock_notifier(struct notifier_block *nb)
 
 	return atomic_notifier_chain_register(&dock_notifier_list, nb);
 }
-
 EXPORT_SYMBOL_GPL(register_dock_notifier);
 
 /**
@@ -580,7 +575,6 @@ void unregister_dock_notifier(struct notifier_block *nb)
 
 	atomic_notifier_chain_unregister(&dock_notifier_list, nb);
 }
-
 EXPORT_SYMBOL_GPL(unregister_dock_notifier);
 
 /**
@@ -625,7 +619,6 @@ register_hotplug_dock_device(acpi_handle handle, struct acpi_dock_ops *ops,
 
 	return ret;
 }
-
 EXPORT_SYMBOL_GPL(register_hotplug_dock_device);
 
 /**
@@ -646,7 +639,6 @@ void unregister_hotplug_dock_device(acpi_handle handle)
 			dock_del_hotplug_device(dock_station, dd);
 	}
 }
-
 EXPORT_SYMBOL_GPL(unregister_hotplug_dock_device);
 
 /**
@@ -761,7 +753,7 @@ struct dock_data {
 
 static void acpi_dock_deferred_cb(void *context)
 {
-	struct dock_data *data = (struct dock_data *)context;
+	struct dock_data *data = context;
 
 	dock_notify(data->handle, data->event, data->ds);
 	kfree(data);
@@ -771,23 +763,22 @@ static int acpi_dock_notifier_call(struct notifier_block *this,
 	unsigned long event, void *data)
 {
 	struct dock_station *dock_station;
-	acpi_handle handle = (acpi_handle)data;
+	acpi_handle handle = data;
 
 	if (event != ACPI_NOTIFY_BUS_CHECK && event != ACPI_NOTIFY_DEVICE_CHECK
 	   && event != ACPI_NOTIFY_EJECT_REQUEST)
 		return 0;
 	list_for_each_entry(dock_station, &dock_stations, sibling) {
 		if (dock_station->handle == handle) {
-			struct dock_data *dock_data;
+			struct dock_data *dd;
 
-			dock_data = kmalloc(sizeof(*dock_data), GFP_KERNEL);
-			if (!dock_data)
+			dd = kmalloc(sizeof(*dd), GFP_KERNEL);
+			if (!dd)
 				return 0;
-			dock_data->handle = handle;
-			dock_data->event = event;
-			dock_data->ds = dock_station;
-			acpi_os_hotplug_execute(acpi_dock_deferred_cb,
-				dock_data);
+			dd->handle = handle;
+			dd->event = event;
+			dd->ds = dock_station;
+			acpi_os_hotplug_execute(acpi_dock_deferred_cb, dd);
 			return 0 ;
 		}
 	}
@@ -941,28 +932,28 @@ static int dock_add(acpi_handle handle)
 {
 	int ret, id;
 	struct dock_station ds, *dock_station;
-	struct platform_device *dock_device;
+	struct platform_device *dd;
 
 	id = dock_station_count;
-	dock_device =
-		platform_device_register_data(NULL, "dock",
-			id, &ds, sizeof(ds));
-	if (IS_ERR(dock_device))
-		return PTR_ERR(dock_device);
+	dd = platform_device_register_data(NULL, "dock", id, &ds, sizeof(ds));
+	if (IS_ERR(dd))
+		return PTR_ERR(dd);
+
+	dock_station = dd->dev.platform_data;
 
-	dock_station = dock_device->dev.platform_data;
 	dock_station->handle = handle;
-	dock_station->dock_device = dock_device;
+	dock_station->dock_device = dd;
 	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->sibling);
-	spin_lock_init(&dock_station->dd_lock);
+
 	mutex_init(&dock_station->hp_lock);
+	spin_lock_init(&dock_station->dd_lock);
+	INIT_LIST_HEAD(&dock_station->sibling);
+	INIT_LIST_HEAD(&dock_station->hotplug_devices);
 	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
+	INIT_LIST_HEAD(&dock_station->dependent_devices);
 
 	/* we want the dock device to send uevents */
-	dev_set_uevent_suppress(&dock_device->dev, 0);
+	dev_set_uevent_suppress(&dd->dev, 0);
 
 	if (is_dock(handle))
 		dock_station->flags |= DOCK_IS_DOCK;
@@ -971,14 +962,13 @@ static int dock_add(acpi_handle handle)
 	if (is_battery(handle))
 		dock_station->flags |= DOCK_IS_BAT;
 
-	ret = sysfs_create_group(&dock_device->dev.kobj, &dock_attribute_group);
+	ret = sysfs_create_group(&dd->dev.kobj, &dock_attribute_group);
 	if (ret)
 		goto err_unregister;
 
 	/* Find dependent devices */
-	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
-			    ACPI_UINT32_MAX, find_dock_devices, dock_station,
-			    NULL);
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX,
+			    find_dock_devices, dock_station, NULL);
 
 	/* add the dock station as a device dependent on itself */
 	ret = add_dock_dependent_device(dock_station, handle);
@@ -991,28 +981,27 @@ static int dock_add(acpi_handle handle)
 
 err_unregister:
 	printk(KERN_ERR "%s encountered error %d\n", __func__, ret);
-	sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
-	platform_device_unregister(dock_device);
+	sysfs_remove_group(&dd->dev.kobj, &dock_attribute_group);
+	platform_device_unregister(dd);
 	return ret;
 }
 
 /**
  * dock_remove - free up resources related to the dock station
  */
-static int dock_remove(struct dock_station *dock_station)
+static int dock_remove(struct dock_station *ds)
 {
 	struct dock_dependent_device *dd, *tmp;
-	struct platform_device *dock_device = dock_station->dock_device;
+	struct platform_device *dock_device = ds->dock_device;
 
 	if (!dock_station_count)
 		return 0;
 
 	/* remove dependent devices */
-	list_for_each_entry_safe(dd, tmp, &dock_station->dependent_devices,
-				 list)
-	    kfree(dd);
+	list_for_each_entry_safe(dd, tmp, &ds->dependent_devices, list)
+		kfree(dd);
 
-	list_del(&dock_station->sibling);
+	list_del(&ds->sibling);
 
 	/* cleanup sysfs */
 	sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
@@ -1035,11 +1024,10 @@ find_dock(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
 	acpi_status status = AE_OK;
 
-	if (is_dock(handle)) {
-		if (dock_add(handle) >= 0) {
+	if (is_dock(handle))
+		if (dock_add(handle) >= 0)
 			status = AE_CTRL_TERMINATE;
-		}
-	}
+
 	return status;
 }
 
@@ -1077,8 +1065,7 @@ static int __init dock_init(void)
 
 static void __exit dock_exit(void)
 {
-	struct dock_station *dock_station;
-	struct dock_station *tmp;
+	struct dock_station *tmp, *dock_station;
 
 	unregister_acpi_bus_notifier(&dock_acpi_notifier);
 	list_for_each_entry_safe(dock_station, tmp, &dock_stations, sibling)

Re: [PATCH v3 1/6] ACPI: dock: convert sysfs attributes to an attribute_group

[Dmitry Torokhov]

Hi Alex,

On Fri, Oct 16, 2009 at 03:14:59PM -0600, Alex Chiang wrote:
> As suggested by Dmitry Torokhov, convert the individual sysfs
> attributes into an attribute group.
> 
> This change eliminates quite a bit of copy/paste code in the
> error handling paths.
> 

Looks much better, one more suggestion though:

> +err_unregister:
> +	printk(KERN_ERR "%s encountered error %d\n", __func__, ret);

If you want to print error this it should probably go down, right before
"return ret".

> +	sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);

It begs another label right here. There are cases when yo0u already
registered the platform device but haven't added the sysfs group, right?

>  	platform_device_unregister(dock_device);
> +out:
>  	kfree(dock_station);
>  	dock_station = NULL;
>  	return ret;
> @@ -1076,11 +1046,7 @@ static int dock_remove(struct dock_station *dock_station)
>  	    kfree(dd);
>  
>  	/* cleanup sysfs */
> -	device_remove_file(&dock_device->dev, &dev_attr_type);
> -	device_remove_file(&dock_device->dev, &dev_attr_docked);
> -	device_remove_file(&dock_device->dev, &dev_attr_undock);
> -	device_remove_file(&dock_device->dev, &dev_attr_uid);
> -	device_remove_file(&dock_device->dev, &dev_attr_flags);
> +	sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
>  	platform_device_unregister(dock_device);
>  
>  	/* free dock station memory */
> 

-- 
Dmitry

Re: [PATCH v3 1/6] ACPI: dock: convert sysfs attributes to an attribute_group

[Alex Chiang]

Hi Dmitry,

* Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> On Fri, Oct 16, 2009 at 03:14:59PM -0600, Alex Chiang wrote:
> > As suggested by Dmitry Torokhov, convert the individual sysfs
> > attributes into an attribute group.
> > 
> > This change eliminates quite a bit of copy/paste code in the
> > error handling paths.
> > 
> 
> Looks much better, one more suggestion though:
> 
> > +err_unregister:
> > +	printk(KERN_ERR "%s encountered error %d\n", __func__, ret);
> 
> If you want to print error this it should probably go down, right before
> "return ret".

This is true for this patch, 1/6... but by the end of the series,
the problem has resolved itself.

I agree that it's sloppy to have this bit of inconsistency in the
middle of the patch series, but I'm reluctant to spin the entire
series again, for sake of a printk.

> > +	sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
> 
> It begs another label right here. There are cases when yo0u already
> registered the platform device but haven't added the sysfs group, right?

This isn't quite true. In this patch, 1/6, our sequence goes:

	platform_device_register_simple()
	platform_device_add_data()
	/* twiddle some state in the platform device, no error paths though */
	sysfs_create_group()

Arguably, the platform_device_add_data() call could fail with
-ENOMEM, but the code today doesn't deal with that error
condition, and I didn't touch the platform_device_add_data()
line.

So really, there are no other exit paths between registering the
platform device and adding the sysfs group.

By the end of the patch series, I combine the _register_simple()
call with the _add_data() call and the final sequence looks like
this:

	if (platform_device_register_data() == error)
		return error;

	/* twiddle local state in platform device */

	if (sysfs_create_group())
		goto err_unregister;

	/* other stuff */

	err_unregister:
		printk(KERN_ERR "%s encountered error %d\n", __func__, ret);
		sysfs_remove_group(&dd->dev.kobj, &dock_attribute_group);
		platform_device_unregister(dd);
		return ret;

Checking other callsites of sysfs_remove_group(), it seems to be
valid to call that API even if the creation step failed.

Basically, I don't see the necessity of adding another label.

Below is the final end state of dock_add(). Hopefully the code is
a lot clearer than before. If there are still semantic issues,
please let me know and I'll happily respin.

Thanks.

/ac

static int dock_add(acpi_handle handle)
{
	int ret, id;
	struct dock_station ds, *dock_station;
	struct platform_device *dd;

	id = dock_station_count;
	dd = platform_device_register_data(NULL, "dock", id, &ds, sizeof(ds));
	if (IS_ERR(dd))
		return PTR_ERR(dd);

	dock_station = dd->dev.platform_data;

	dock_station->handle = handle;
	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);
	INIT_LIST_HEAD(&dock_station->hotplug_devices);
	ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
	INIT_LIST_HEAD(&dock_station->dependent_devices);

	/* we want the dock device to send uevents */
	dev_set_uevent_suppress(&dd->dev, 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 = sysfs_create_group(&dd->dev.kobj, &dock_attribute_group);
	if (ret)
		goto err_unregister;

	/* Find dependent devices */
	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX,
			    find_dock_devices, dock_station, NULL);

	/* add the dock station as a device dependent on itself */
	ret = add_dock_dependent_device(dock_station, handle);
	if (ret)
		goto err_unregister;

	dock_station_count++;
	list_add(&dock_station->sibling, &dock_stations);
	return 0;

err_unregister:
	printk(KERN_ERR "%s encountered error %d\n", __func__, ret);
	sysfs_remove_group(&dd->dev.kobj, &dock_attribute_group);
	platform_device_unregister(dd);
	return ret;
}

Re: [PATCH v3 1/6] ACPI: dock: convert sysfs attributes to an attribute_group

[Dmitry Torokhov]

On Mon, Oct 19, 2009 at 11:21:38AM -0600, Alex Chiang wrote:
> Hi Dmitry,
> 
> * Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> > On Fri, Oct 16, 2009 at 03:14:59PM -0600, Alex Chiang wrote:
> > > As suggested by Dmitry Torokhov, convert the individual sysfs
> > > attributes into an attribute group.
> > > 
> > > This change eliminates quite a bit of copy/paste code in the
> > > error handling paths.
> > > 
> > 
> > Looks much better, one more suggestion though:
> > 
> > > +err_unregister:
> > > +	printk(KERN_ERR "%s encountered error %d\n", __func__, ret);
> > 
> > If you want to print error this it should probably go down, right before
> > "return ret".
> 
> This is true for this patch, 1/6... but by the end of the series,
> the problem has resolved itself.
> 
> I agree that it's sloppy to have this bit of inconsistency in the
> middle of the patch series, but I'm reluctant to spin the entire
> series again, for sake of a printk.
> 
> > > +	sysfs_remove_group(&dock_device->dev.kobj, &dock_attribute_group);
> > 
> > It begs another label right here. There are cases when yo0u already
> > registered the platform device but haven't added the sysfs group, right?
> 
> This isn't quite true. In this patch, 1/6, our sequence goes:
> 
> 	platform_device_register_simple()
> 	platform_device_add_data()
> 	/* twiddle some state in the platform device, no error paths though */
> 	sysfs_create_group()
> 
> Arguably, the platform_device_add_data() call could fail with
> -ENOMEM, but the code today doesn't deal with that error
> condition, and I didn't touch the platform_device_add_data()
> line.
> 
> So really, there are no other exit paths between registering the
> platform device and adding the sysfs group.
>

If sysfs_create_group() fails you will go to err_unregister: which will
try to remove the non-existing group. While the current sysfs code is
relsilient against such errors it may not be so in the future. It is
better to have a separate label and bypass sysfs_remove_group() if
sysfs_create_group() returned error.

> By the end of the patch series, I combine the _register_simple()
> call with the _add_data() call and the final sequence looks like
> this:
> 
> 	if (platform_device_register_data() == error)
> 		return error;
> 
> 	/* twiddle local state in platform device */
> 
> 	if (sysfs_create_group())
> 		goto err_unregister;
> 
> 	/* other stuff */
> 
> 	err_unregister:
> 		printk(KERN_ERR "%s encountered error %d\n", __func__, ret);
> 		sysfs_remove_group(&dd->dev.kobj, &dock_attribute_group);
> 		platform_device_unregister(dd);
> 		return ret;
> 

-- 
Dmitry

Re: [PATCH v3 1/6] ACPI: dock: convert sysfs attributes to an attribute_group

[Alex Chiang]

* Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> 
> If sysfs_create_group() fails you will go to err_unregister:
> which will try to remove the non-existing group. While the
> current sysfs code is relsilient against such errors it may not
> be so in the future. It is better to have a separate label and
> bypass sysfs_remove_group() if sysfs_create_group() returned
> error.

Yeah, you're right. I was trying to be lazy (everyone else is
doing it!), but I guess you won't let me. ;)

I'll respin.

Thanks for the review.

/ac