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

[Andrew Morton <akpm@linux-foundation.org>]

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?

> >   We have a bunch of code in random other drivers which is dependent upon
> >   CONFIG_PPS_CLIENT_foo.  The problem is that if a kernel was compiled with
> >   CONFIG_PPS_CLIENT_foo=n and then the pps driver is later built for that
> >   kernel, it won't actually work because lp, serial etc weren't correctly
> >   configured when _they_ were built.
> > 
> >   This sort of cross-module coupling is considered to be a bad thing, but
> >   I'm not really sure it's all that important.
> >
> > - Please split the patch up into a series of patches: one for pps core and
> >   one for each of the clients (servers?): one for lp, one for serial, etc.
> > 
> >   Try to arrange for that series of patches to build and run at each stage
> >   of application.
> > 
> >   Please don't lose my changes when you do so ;)
> > 
> >   Please review the changes I made and a) stick to the same style and b) fix
> >   up any sites which I missed.
> > 
> > - Please remove all the typedefs:
> > 
> > +typedef struct ntp_fp {
> > +typedef union pps_timeu {
> > +typedef struct pps_info {
> > +typedef struct pps_params {
> > 
> >   and just use `struct ntp_fp' everywhere.
> 
> Those typedefs are defined in PPS specifications (please, see RFC 2783).

We don't use typedefs in-kernel.  Please convert the code to use `struct
ntp_fp' everywhere.

For RFC compatibility to userspace you can do

#ifndef __KERNEL__
typedef struct ntp_fp ntp_fp_t;
...
#endif

> > - The above four structures are communicated with userspace, yes?
> > 
> >   I believe that they will not work correctly when 32-bit userspace is
> >   communicating with a 64-bit kernel.  Alignments change and sizeof(long)
> >   changes.
> > 
> >   You don't want to have to write compat code.  I suggest that you redo
> >   those structures in terms of __u32, __u64, etc.  You probably need to use
> >   attribute((packed)) too, not sure.
> > 
> >   Then let's get that part carefully reviewed (Arnd Bergmann <arnd@arndb.de>
> >   is my go-to guru on this) and please test it carefully.
> > 
> >   Yeah, you just haven't got a chance that something as huge and as complex
> >   as struct pps_netlink_msg will survive the 32->64 transition.
> 
> The same as above. These structure are fixed by RFC 2783.

Your answer has no relationship to my question.

The problem here is that under a 64-bit kernel we require that applications
which use this structure definition work correctly when they are compiled
to generate 32-bit code and when they are compiled to generate 64-bit code.

Furthermore we should aim to to have to code work correctly across
different version of the compiler, and when different compiler options are
used, and when altogether different compilers are used.

It is not clear to me that your definition is sufficiently defensive
against _any_ of these things.

> > - Please ensure that `make headers_check' passes OK (you'll hear from me if
> >   it doesn't ;))
> 
> Done.
> 
> > - Can we get rid of the private dbg, err and info macros?  Surely there are
> >   generic ones somewhere.
> 
> They are very useful to LinuxPPS users who can enable/disable them by
> configuration menu.

You misunderstand.  I'm not saying "remove the callsites".  I'm saying
"remove the definitions".

Because we already have things like pr_debug() and pr_info(), so new code
should use those rather than reinventing them.

Plus, we already have at least 52 different implementations of "dbg" in the
tree and your 53rd one didn't compile because it clashed with someone
else's.  This is the compiler sending us a message: "use the exiting
infrastructure".   If that infrastructure is insufficient then let's
improve it.