Patchworkβ [v3,1/6] ACPI: dock: convert sysfs attributes to an attribute_group

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(-)


--
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

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 */
>

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;
}
--
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

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 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

--
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