Message-ID: <20070512055935.GD979@enneenne.com>
Date:	Sat, 12 May 2007 07:59:35 +0200
From:	Rodolfo Giometti <giometti@...eenne.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-kernel@...r.kernel.org, linuxpps@...enneenne.com
Subject: Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux

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! :)

On Thu, May 10, 2007 at 12:27:52AM -0700, akpm@...ux-foundation.org wrote:
>
> Review comments:
>
> - Running a timer once per second will make the super-low-power people upset.

The ktimer modules is just for debugging pourpose and it's not needed
into real working system.

> - This uses netlink?  Is that interface documented anywhere?
>
>   Please check with Dave Miller that this:
>
>       #define NETLINK_PPSAPI          20
>
>   reservation is OK.

Is not ok. To be fixed.

> - This:
>
>       if ((nlpps->tsformat != PPS_TSFMT_TSPEC) != 0 ) {
>
>   is weird.  I changed it to
>
>       if (nlpps->tsformat != PPS_TSFMT_TSPEC) {

Fixed.

> - This:
>
>       timeout += nlpps->timeout.tv_nsec/(1000000000/HZ);
>
>   probably won't work on i386.  We use do_div() for 64/32 divides.  I'll
>   find out when I compile it.
>
>   It's nice to use NSEC_PER_SEC rather than having to count all those
>   zeroes.

Fixed.

> - The code uses interruptible_sleep_on_timeout().  That API is deprecated
>   and is racy.  Please convert to wait_event_interruptible_timeout().
>
>   Ditto interruptible_sleep_on()

Fixed.

> - This:
>
>         memset(pps_source, 0, sizeof(struct pps_s) * PPS_MAX_SOURCES);
>
>   was unneeded.  The C startup code already did that.

Fixed.

> - All these separators:
>
> +/* --- Input function ------------------------------------------------------
+*/
>
>   aren't typical for kernel code.  I left them in, but please consider
>   removing them all.

Fixed.

> - 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?

> - What's this doing in 8250.c?
>
> +     if (up->port.flags & UPF_HARDPPS_CD)
> +             up->ier |= UART_IER_MSI;        /* enable interrupts */
>       
>   Please fully describe the reasons for this change in the changelog, and in
>   a code comment and then get the change reviewed by Russell King
>   <rmk@....linux.org.uk>.

If user specify a serial port as PPS source we enable IRQ on that
port.

> - Please document within the changelog the other changes to the serial code
>   and we'll ask Russell to take a look at those as well.

OK. I'll do it.

> - The Kconfig purports to support CONFIG_PPS=m.  Does that actually work?

Yes. It works...

>   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).

> - 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@...db.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.

> - 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.

Also I'm planning to add a dinamic enable/disable mechanism...

> - struct pps_s has volatiles in it.  Please remove them.  There's lots of
>   discussion on this topic on linux-kernel just today.

Fixed.

> - Why did the
>   
>       port->icount.dcd++;
> 
>   get moved in uart_handle_dcd_change()?

That's why we have to register the PPS interrupt as soon as possible!
Even few CPU instructions dalay may result in a bad time settings.

> - In lots of places you do:
>
> +#ifdef CONFIG_PPS_CLIENT_UART
> +#include <linux/pps.h>
> +#endif
> 
>   Please remove the ifdefs at all these sites and make the header file
>   handle it.

Fixed.

> - It no longer compiles, because netlink_kernel_create now requires a
>   `struct mutex *'.  I stuck a NULL in there, which is permitted.  But I don't>   know if that was a good thing - please check this.
>
>   Please also chase the net guys with a pointy stick for failing to document
>   their damned APIs.

This should vanish when new netlink layer will be used.

> - Generally: code looks OK and is probably useful.  Please keep going ;)

Hope I forgot nothing!

Ciao,

Rodolfo

----

View attachment "ntp-pps-2.6.21-bis.diff" of type "text/x-diff" (50874 bytes)

Powered by blists - more mailing lists