On Sat, Mar 09, 2019 at 08:21:35AM +0100, Greg KH wrote: > On Fri, Mar 08, 2019 at 05:46:11PM -0500, Kimberly Brown wrote: > > static struct kobj_type vmbus_chan_ktype = { > > .sysfs_ops = &vmbus_chan_sysfs_ops, > > .release = vmbus_chan_release, > > - .default_attrs = vmbus_chan_attrs, > > As discussed on IRC, a kobj_type needs to get an attribute group one of > these days, not just a attribute list :) > > So thanks for persisting with this, sorry for the earlier review > comments, you were totally right about this, nice work. > > Minor review comments below: > > > }; > > > > /* > > @@ -1571,11 +1624,34 @@ int vmbus_add_channel_kobj(struct hv_device *dev, > > struct vmbus_channel *channel) > > if (ret) > > return ret; > > > > + ret = sysfs_create_group(kobj, &vmbus_chan_group); > > + channel->sysfs_group_ready = !ret; > > Why do you need this flag? >
I added this flag to prevent sysfs_remove_group() from being called if sysfs_create_group() or the earlier call to kobject_init_and_add() failed. However, it looks like that would just lead to WARN() being called, which seems appropriate. I'll remove this flag. > > + > > + if (ret) { > > + /* > > + * If an error is returned to the calling functions, those > > + * functions will call kobject_put() on the channel kobject, > > + * which will cleanup the empty channel directory created by > > + * kobject_init_and_add(). > > Why is this comment needed? > Another reviewer asked why the empty directory created by kobject_init_and_add() isn't removed here when sysfs_create_group() fails. We decided that a comment would help clear up any future confusion. > > + */ > > + pr_err("Unable to set up channel sysfs files\n"); > > dev_err() to show who had the problem with the files? > Yes, good point. I'll change this. > > + return ret; > > + } > > + > > kobject_uevent(kobj, KOBJ_ADD); > > > > return 0; > > } > > > > +/* > > + * vmbus_remove_channel_attr_group - remove the channel's attribute group > > + */ > > +void vmbus_remove_channel_attr_group(struct vmbus_channel *channel) > > +{ > > + if (channel->sysfs_group_ready) > > + sysfs_remove_group(&channel->kobj, &vmbus_chan_group); > > You should be able to just always remove these, no need for a flag to > say you have created them or not, right? > > thanks, > > greg k-h