Messages in this thread | | | Subject | Re: [PATCH] LinuxPPS - definitive version | From | David Woodhouse <> | Date | Tue, 24 Jul 2007 14:49:02 +0100 |
| |
On Tue, 2007-07-24 at 10:00 +0200, Rodolfo Giometti wrote: > On Mon, Jul 23, 2007 at 02:35:16PM +0100, David Woodhouse wrote: > > > > s/Documentaion/Documentation/ in the last line of Documentation/pps/pps.txt > > Fixed.
Also 's/unknow /unknown /' (2 instances)
> > Please feed it to scripts/checkpatch.pl -- you can ignore all the > > warnings about lines greater than 80 characters, and the complete crap > > about "declaring multiple variables together should be avoided", but > > Done. Most fixed.
Looks better.
> > some of what it points out is valid. Including the one about 'volatile' > > -- your explanation lacked credibility. If you really need 'volatile' > > then put it at the places you actually need it; not the declaration of > > the structure. > > Can you please explain better where should I put the 'volatile' > attribute? :-o
Am I right in thinking that the only place it matters is within pps_event()? In that case, at the very least you should probably remove the 'volatile' from the definition of the structure, and _cast_ to volatile where you want it treated that way.
But I don't see why you can't protect it with a spinlock. As long as you acquire that spinlock _after_ your call to getnstimeofday() what's the problem?
> > ... > This should be not needed due new 'struct pps_ktime'. > > ... > These typedefs are into timepps.h which is an userland file (located > into Documentation/pps/) and are requested by the RFC.
Sorry, yes. I shouldn't have been looking at that as if it was kernel code.
I think you still haven't quite got the 32-bit vs. 64-bit compatibility right. Remember that on i386, the alignment of a uint64_t is only 4 bytes, while on most other architectures it's 8 bytes. On i386, there will be no padding between the two consecutive 'struct pps_ktime' members of struct pps_kinfo and struct pps_kparams. But on most platforms there will be padding to ensure correct alignment.
The simple fix is probably to make the 'nsec' member a 64-bit integer too. Then it'll be the same for i386 and x86_64 and you won't need a compatibility syscall routine.
In order for your handling of 'pps_source[source].info' to be safe with respect to pps_unregister_source(), you have to guarantee that pps_event() has finished -- and can't be in progress on another CPU -- by the time your client's call to pps_unregister_source() completes. At first glance I think your existing clients have that right (you have del_timer_sync() before pps_unregister_source() in ktimer.c, for example). But you should make sure it's clearly documented for new clients.
Shouldn't your PPS_CLIENT_LP and PPS_CLIENT_UART options depend on PARPORT and SERIAL_CORE respectively?
-- dwmw2
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |