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@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages