On Mon, Oct 19, 2009 at 11:21:38AM -0600, Alex Chiang wrote: > Hi Dmitry, > > * Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>: > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
