On Fri, May 11, 2007 at 11:17:11PM -0700, Andrew Morton wrote:
> On Fri, 11 May 2007 23:55:37 +0200
> Rodolfo Giometti <[EMAIL PROTECTED]> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
