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@linux-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 > . 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 > 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 > +#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 ----