[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20070512070851.GA18752@kroah.com>
Date: Sat, 12 May 2007 00:08:51 -0700
From: Greg KH <greg@...ah.com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: Rodolfo Giometti <giometti@...eenne.com>,
linux-kernel@...r.kernel.org, linuxpps@...enneenne.com
Subject: Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
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@...eenne.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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists