Re: [PATCH v3 2/9] suppress "Device nodeX does not have a release()function" warning

[Greg KH]

On Mon, Oct 22, 2012 at 03:52:24PM -0700, Andrew Morton wrote:
> On Fri, 19 Oct 2012 14:46:35 +0800
> wency@xxxxxxxxxxxxxx wrote:
> 
> > From: Yasuaki Ishimatsu <isimatu.yasuaki@xxxxxxxxxxxxxx>
> > 
> > When calling unregister_node(), the function shows following message at
> > device_release().
> > 
> > "Device 'node2' does not have a release() function, it is broken and must
> > be fixed."
> > 
> > The reason is node's device struct does not have a release() function.
> > 
> > So the patch registers node_device_release() to the device's release()
> > function for suppressing the warning message. Additionally, the patch adds
> > memset() to initialize a node struct into register_node(). Because the node
> > struct is part of node_devices[] array and it cannot be freed by
> > node_device_release(). So if system reuses the node struct, it has a garbage.
> > 
> > ...
> >
> > --- a/drivers/base/node.c
> > +++ b/drivers/base/node.c
> > @@ -252,6 +252,9 @@ static inline void hugetlb_register_node(struct node *node) {}
> >  static inline void hugetlb_unregister_node(struct node *node) {}
> >  #endif
> >  
> > +static void node_device_release(struct device *dev)
> > +{
> > +}
> >  
> >  /*
> >   * register_node - Setup a sysfs device for a node.
> > @@ -263,8 +266,11 @@ int register_node(struct node *node, int num, struct node *parent)
> >  {
> >  	int error;
> >  
> > +	memset(node, 0, sizeof(*node));
> > +
> >  	node->dev.id = num;
> >  	node->dev.bus = &node_subsys;
> > +	node->dev.release = node_device_release;
> >  	error = device_register(&node->dev);
> >  
> >  	if (!error){
> 
> Greg won't like that empty ->release function ;)
> 
> As you say, this device item does not reside in per-device dynamically
> allocated memory - it is part of an externally managed array.
> 
> So a proper fix here would be to convert this storage so that it *is*
> dynamically allocated on a per-device basis.

I thought we had this fixed up already?

> Or perhaps we should recognize that the whole kobject
> get/put/release-on-last-put model is inappropriate for these objects,
> and stop using it entirely.

Yes, you can do the same thing with dynamic struct devices for what you
need to do here, it should be easy to convert the code to use it.

> From Kosaki's comment, it does sound that we plan to take the first
> option: convert to per-device dynamically allocated memory?  If so, I
> suggest that we just leave the warning as-is for now, until we fix
> things proprely.

Sounds good to me.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/