Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux

[Greg KH <greg@kroah.com>]

On Fri, May 11, 2007 at 11:17:11PM -0700, Andrew Morton wrote:
> On Fri, 11 May 2007 23:55:37 +0200
> Rodolfo Giometti <giometti@enneenne.com> wrote:
> 
> > Hello,
> > 
> > here my new patch with a lot of fixes.
> > 
> > The only issue not still fixed is the one related with:
> > 
> > 	#define NETLINK_PPSAPI          20
> > 
> > I need time to resolve it.
> > 
> > Follows my comments and then the patch, hope now I can came back into
> > -mm tree again! :)
> 
> Well I suppose I could toss it in there for a bit of review-and-test.  But
> I'll need to drop it again because we do need to split this patch into the series
> of patches, please.
> 
> You should do this earlier rather than later because it improves reviewability.
> 
> > > - This:
> > > 
> > > 	static void pps_class_release(struct class_device *cdev)
> > > 	{
> > > 		/* Nop??? */
> > > 	}
> > > 
> > >   is a bug and it earns you a nastygram from Greg.  These objects must be
> > >   dynamically allocated - this is not optional.
> > 
> > It could be acceptable defining this function as void?
> 
> No, it needs to be a proper release function, like all the other ones
> around the place.
> 
> This comes up again and again and again and I recently asked Greg to direct
> me to (or to write) suitable documentation, and I think he did, but I lost
> it.  Greg, can you remind us please?

I need to put it in some permanent place, but basically the problem is
that if you need to shut the kernel up by using an empty function for a
release, then you are not understanding why the kernel was trying to
warn you in the first place :(

You need to free your memory for the class_device in the release
function, you can not have a static structure, or rely on some other
reference count to handle the cleanup properly.

Also note that 'struct class_device' is going away and you should use
'struct device' instead.  That too needs to be documented better, and is
on my list of things to do...

thanks,

greg k-h