LinuxPPS - definitive version

Hello,

here my last patch for PPS support.

In my opinion it should be ok for inclusion... please, let me know if something should be still changed.

s/Documentaion/Documentation/ in the last line of Documentation/pps/pps.txt

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

You've also reverted to structures which vary between 32-bit and 64-bit userspace, because they use 'long' and 'struct timespec', but you haven't provided the compat_* routines which are then necessary.

Documentation/pps/timepps.h new file +4 -0

+typedef int pps_handle_t;              /* represents a PPS source */
+typedef unsigned long pps_seq_t;       /* sequence number */
+typedef struct ntp_fp ntp_fp_t;                /* NTP-compatible time stamp */
+typedef union pps_timeu pps_timeu_t;   /* generic data type to represent time s

tamps */

Documentation/pps/timepps.h new file +2 -0

+typedef struct pps_info pps_info_t;    
+typedef struct pps_params pps_params_t;

Don't do this for the structures. It's dubious enough for the integer types.

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 some of what it points out is valid.

Quoted 4 lines · On Mon, 23 Jul 2007 14:35:16 +0100 David Woodhouse <[email protected]> wrote:

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 some of what it points out is valid.

The above are David's opinions. Many, probably most kernel developers do not agree with them.

s/Documentaion/Documentation/ in the last line of Documentation/pps/pps.txt

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

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.

You've also reverted to structures which vary between 32-bit and 64-bit userspace, because they use 'long' and 'struct timespec', but you haven't provided the compat_* routines which are then necessary.

(quoted diff) +4 -0

+typedef int pps_handle_t;              /* represents a PPS source */
+typedef unsigned long pps_seq_t;       /* sequence number */
+typedef struct ntp_fp ntp_fp_t;                /* NTP-compatible time stamp */
+typedef union pps_timeu pps_timeu_t;   /* generic data type to represent time s

tamps */

(quoted diff) +2 -0

+typedef struct pps_info pps_info_t;    
+typedef struct pps_params pps_params_t;

Don't do this for the structures. It's dubious enough for the integer types.

Quoted 2 lines · David Woodhouse

s/Documentaion/Documentation/ in the last line of Documentation/pps/pps.txt

Fixed.

Quoted 3 lines

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.

Quoted 4 lines

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

Quoted 1 line

...

This should be not needed due new 'struct pps_ktime'.

Quoted 1 line

...

These typedefs are into timepps.h which is an userland file (located into Documentation/pps/) and are requested by the RFC.

Also 's/unknow /unknown /' (2 instances)

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?

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?

The problem is that we can have several PPS sources into a system and all these sources will arise their IRQ line (quasi)simultaneously and I don't wish a CPU may delay one of these IRQ handler due a spinlock into the pps_event().

That's why I'm trying to avoid any lock into pps_event().

Quoted 1 line · David Woodhouse

Also 's/unknow /unknown /' (2 instances)

?? I didn't find them:

   $ grep 'unknow ' Documentation/pps/pps.txt

Quoted 8 lines

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.

This can be done only with locks, but it's not necessary since even if a pps_unregister_source() runs while pps_event() executes on another CPU the latter will write always on a valid area (even if it could be a dummy one) and the data are not corrupted (note also that the data will be, in any case, discarted since we are executing a pps_unregister_source()).

Elsewhere in the patch.

Read Documentation/memory-barriers.txt

There is a tiny but possibly non-zero chance that one CPU could be in pps_event() and might not yet have 'seen' the change to the .info field. Releasing the pps_mutex provides a write-barrier on the CPU which runs pps_unregister_source(), but there's no corresponding read-barrier on the CPU running pps_event(). You have to be careful about when pps_event() is run. It _MUST_ not touch the old info structure after pps_unregister_source() has completed.

At the moment, I think it's OK because you won't be calling pps_event() at the wrong times. But you do need to make sure that requirement is documented. And I think you can remove the whole dummy_info thing because it's not necessary.

The pps_event() is now protected by a spinlock against pps_register_source() and pps_unregister_source()...

Quoted 2 lines · Rodolfo Giometti

The pps_event() is now protected by a spinlock against pps_register_source() and pps_unregister_source()...

Locks protect data, not code. It may make more sense to identify the specific data being protected by the spinlock.

What do you mean? Did you find an error into my patch? :-o

Functions pps_event() and pps_register_source()/pps_unregister_source() take accesso to shared data, that's why I used spinlocks.

My point is that the lock should be used to protect specific data. Thus, it would be more correct to say, "spinlock foo is taken because pps_register_source() accesses variable bar".

That way, if someone else wants to access "bar", they know that they need to take lock "foo".

Quoted 7 lines · Chris Friesen

My point is that the lock should be used to protect specific data. Thus, it would be more correct to say, "spinlock foo is taken because pps_register_source() accesses variable bar".

That way, if someone else wants to access "bar", they know that they need to take lock "foo".

Ah, ok! I see. :)

Hi Rodolfo,

Quoted 10 lines · Rodolfo Giometti

Quoted 7 lines · Chris Friesen

My point is that the lock should be used to protect specific data. Thus, it would be more correct to say, "spinlock foo is taken because pps_register_source() accesses variable bar".

That way, if someone else wants to access "bar", they know that they need to take lock "foo".

Ah, ok! I see. :)

I only glanced through the code, so could be wrong, but I noticed that the only global / shared data you have in there is a global "pps_source" array of pps_s structs. That's accessed / modified from the various syscalls introduced in the API exported to userspace, as well as the register/unregister/pps_event API exported to in-kernel client subsystems, yes? So it looks like you need to introduce proper locking for it, simply type-qualifying it as "volatile" is not enough.

However, I think you've introduced two locks for it. The syscalls (that run in process context, obviously) seem to use a pps_mutex and pps_event() seems to be using the pps_lock spinlock (because that gets executed from interrupt context) -- and from the looks of it, the register/unregister functions are using /both/ the mutex and spinlock (!)

This isn't quite right, (in fact there's nothing to protect pps_event from racing against a syscall), so you should use *only* the spinlock for synchronization -- the spin_lock_irqsave/restore() variants, in fact.

[ Also, have you considered making pps_source a list and not an array? It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc kind of gymnastics in there, and you _can_ return a pointer to the corresponding pps source struct from the register() function to the in-kernel users, so that way you get to retain the O(1) access to the corresponding source when a client calls into pps_event(), similar to how you're using the array index presently. ]

I also noticed code like (from pps_event):

drivers/pps/kapi.c new file +3 -0

+       /* Try to grab the lock, if not we prefere loose the event... */
+       if (!spin_trylock(&pps_lock))
+               return;

which looks worrisome and unnecessary. That spinlock looks to be of fine enough granularity to me, do you think there'd be any contention on it? I /think/ you can simply make that a spin_lock().

Overall the code looks simple / straightforward enough to me (except for the parport / uart stuff that I have no clue about), and I'll also read up on the relevant RFC for this and would hopefully try and give you a more meaningful review over the weekend.

Take the race between the time_pps_setparams() syscall and a concurrent pps_event() from an interrupt for instance. From sys_time_pps_setparams, the parameters for an existing source are not modified / set atomically, which means a pps_event() called on the same source in between will see invalid parameters ... and bad things will happen.

Quoted 7 lines

[ Also, have you considered making pps_source a list and not an array? It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc kind of gymnastics in there, and you _can_ return a pointer to the corresponding pps source struct from the register() function to the in-kernel users, so that way you get to retain the O(1) access to the corresponding source when a client calls into pps_event(), similar to how you're using the array index presently. ]

I think the above would be sane and safe -- your driver has pretty simple lifetime rules, and "sources" are only created / destroyed from within kernel, as and when clients call pps_register_source() and pps_unregister_source(). So pps_event() can be called on a given source only between the corresponding register() and unregister() -- which means register() can return us a reference/pointer on the source after allocating / adding it to the list (instead of the fixed array index as it presently is), which remains valid for the entire duration of the source, till unregister() is called, after which we can't be calling pps_event() on the same source anyway.

Ok, I've looked through (most of) the RFC and code now, and am only commenting on a design-level for now. Anyway, I didn't like the way you've significantly drifted from the RFC in several ways:

1. The RFC mandates no such userspace interface / syscall as the time_pps_cmd() that you've implemented -- it looks, smells, and feels like an ioctl, in fact that's what it is for practical purposes. I'm confused as to why didn't you just go ahead and implement the special-file-and- file-descriptor based approach as advocated / mandated there.

[ You've implemented the (optional, as per RFC) time_pps_findsource operation in the kernel using the above "pseudo-ioctl", but that wasn't necessary -- as the RFC itself illustrates, it's something that can easily be done (in fact should be done) completely in userspace itself. ]

2. If you fix the above two issues, you'll notice that you don't need to short-circuit the (RFC-mandated) time_pps_create/destroy(handle) syscalls in the userspace header/library anymore, as you presently are.

Here's how I'd go about desiging/implementing this:

* At the time of pps_register_source() -- called by an in-kernel client subsystem that creates a PPS source -- allocate a pps source, generate an identifier for it, instantiate a special file -- the RFC does not mention whether a char or block device, but char device (I noticed an example in the RFC where they've used /dev/ppsXX as a possible path) would be proper for this. Finally add it to the list of sources. This returns a reference/pointer on that source back to the in-kernel client, which then passes *that* to pps_event(), similar to how you're presently using the array index.

[ The way you've passed the path of the parport/uart device itself (/dev/lpXX or [/dev/%s%d, drv->name, port->line]) to register_source() in pps_info.path doesn't quite look right to me. Note that the userspace is expected to open(2) the special file corresponding to the *PPS* source, as instantiated from the above code, and not the /dev/xyz special file of the *physical* port through which a pulse-generating device may be connected to the PC. ]

* Userspace will open(2) the special file, and get an fd. Then calls the time_pps_create(fd, &handle) syscall -- kernel will find the pps source that matches that passed fd from the list of sources, and instantiates a "handle" associated with that source and returns that back to userspace.

The rest would happen as usual / as you've currently implemented.

I /think/ the RFC does envision such an implementation, so it helps us comply with that standard, and would also get rid of a lot of kludgy "findpath" and "findsource" stuff that we otherwise have to do in-kernel and userspace, as we're presently doing in the patch.

[ BTW, it would be nice if you submit this stuff as a patchset that brings in functionality over a series of multiple patches -- the sysfs interface bits can be introduced in a different patch from the syscalls, which can be introduced in a different patch from the kernel-side API, etc ... that helps a code-level review. ]

Quoted 8 lines · Satyam Sharma

Quoted 7 lines

[ Also, have you considered making pps_source a list and not an array? It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc kind of gymnastics in there, and you _can_ return a pointer to the corresponding pps source struct from the register() function to the in-kernel users, so that way you get to retain the O(1) access to the corresponding source when a client calls into pps_event(), similar to how you're using the array index presently. ]

Quoted 9 lines

I think the above would be sane and safe -- your driver has pretty simple lifetime rules, and "sources" are only created / destroyed from within kernel, as and when clients call pps_register_source() and pps_unregister_source(). So pps_event() can be called on a given source only between the corresponding register() and unregister() -- which means register() can return us a reference/pointer on the source after allocating / adding it to the list (instead of the fixed array index as it presently is), which remains valid for the entire duration of the source, till unregister() is called, after which we can't be calling pps_event() on the same source anyway.

Ok. I see. I'll study the problem but I think this is can be done later, now I think is better having a working code. :)

Quoted 4 lines

Ok, I've looked through (most of) the RFC and code now, and am only commenting on a design-level for now. Anyway, I didn't like the way you've significantly drifted from the RFC in several ways:

Quoted 5 lines

1. The RFC mandates no such userspace interface / syscall as the time_pps_cmd() that you've implemented -- it looks, smells, and feels like an ioctl, in fact that's what it is for practical purposes. I'm confused as to why didn't you just go ahead and implement the special-file-and- file-descriptor based approach as advocated / mandated there.

This is because is not always true that a PPS source is connected with a char (or other) device.

People whose designed RFC didn't think about
systems where the PPS signal is connected with a CPU's GPIO and the O.S. doesn't provide any char device to refere with!

In the common desktop PCs the GPS antenna is connected with the serial line and the PPS source is attached to the serial CD, but in the embedded systems this is _not_ true. GPS antennae may still be connected with serial line but the PPS signal is usually connected with a GPIO pin.

In this scenario you cannot use the serial file descriptor to manage the PPS signal since it cannot goes to the serial port.

Quoted 4 lines

[ You've implemented the (optional, as per RFC) time_pps_findsource operation in the kernel using the above "pseudo-ioctl", but that wasn't necessary -- as the RFC itself illustrates, it's something that can easily be done (in fact should be done) completely in userspace itself. ]

I used pseudo-ioctl interface since it allows me to easily extend PPS support with special, and uncommon, commands.

Quoted 9 lines

* At the time of pps_register_source() -- called by an in-kernel client subsystem that creates a PPS source -- allocate a pps source, generate an identifier for it, instantiate a special file -- the RFC does not mention whether a char or block device, but char device (I noticed an example in the RFC where they've used /dev/ppsXX as a possible path) would be proper for this. Finally add it to the list of sources. This returns a reference/pointer on that source back to the in-kernel client, which then passes *that* to pps_event(), similar to how you're presently using the array index.

If your GPS antenna is connected with a CPU's GPIO you have _no_ in-kernel client subsystem that creates a PPS source.

Quoted 7 lines

[ The way you've passed the path of the parport/uart device itself (/dev/lpXX or [/dev/%s%d, drv->name, port->line]) to register_source() in pps_info.path doesn't quite look right to me. Note that the userspace is expected to open(2) the special file corresponding to the *PPS* source, as instantiated from the above code, and not the /dev/xyz special file of the *physical* port through which a pulse-generating device may be connected to the PC. ]

As above, if your GPS antenna is connected with a CPU's GPIO you have _no_ device to open at all, that's why you need at least a function like pps_findsource().

Note that in this case the pps_info.path is void. Please, see the special client drivers/pps/clients/ktimer.c, it emulates the case where you have no /dev/XXX to open(2). If your modifications resolve the problems to manage the ktimer client you are going in the right direction.

Quoted 4 lines

I /think/ the RFC does envision such an implementation, so it helps us comply with that standard, and would also get rid of a lot of kludgy "findpath" and "findsource" stuff that we otherwise have to do in-kernel and userspace, as we're presently doing in the patch.

Unluckily the RFC does _not_ take into account PPS sources connected with CPU's GPIO... in this case, in fact, you have _no_ char/block device to open().

Fair enough, but I think the code could become a trifle simpler/easier after the conversion, so probably greater chances of getting merged :-)

But that's alright -- see, as I said, you're confusing between the "special device" that represents the *PPS source* itself, with the port or device that it uses to *physically* connect to the PC.

In the RFC, when they say that the userspace app must open(2) the PPS source (as they have illustrated in the example too), they mean that it open(2)'s the special device/file associated with the PPS source, and *not* the /dev/lpXXX or /dev/ttySXXX that it might be connected through physically.

So they mean something like /dev/pps0, /dev/pps1 etc instead. Of course, no such special device exists on a Linux box already, but that's fine and obvious! *You* are supposed to create / instantiate that when a pps_register_source() is done from some in-kernel subsystem.

As I said, it's not the char device for the physical interface itself being discussed there. That could be parport, uart, some arbit GPIO pin whatever. But whenever the corresponding kernel subsystem does a register_source(), you could create the /dev/ppsXXX device ...

Hmm, but that's a non-standard, not-mandated-by-RFC syscall. I don't see how you can get this merged, really :-)

Quoted 3 lines · Satyam Sharma

Fair enough, but I think the code could become a trifle simpler/easier after the conversion, so probably greater chances of getting merged :-)

I see. I'll start thinging about it.

Quoted 10 lines

But that's alright -- see, as I said, you're confusing between the "special device" that represents the *PPS source* itself, with the port or device that it uses to *physically* connect to the PC.

In the RFC, when they say that the userspace app must open(2) the PPS source (as they have illustrated in the example too), they mean that it open(2)'s the special device/file associated with the PPS source, and *not* the /dev/lpXXX or /dev/ttySXXX that it might be connected through physically.

Quoted 4 lines

So they mean something like /dev/pps0, /dev/pps1 etc instead. Of course, no such special device exists on a Linux box already, but that's fine and obvious! *You* are supposed to create / instantiate that when a pps_register_source() is done from some in-kernel subsystem.

So your are proposing to create a char device interface then using syscalls one? In this case how do you manage the case where your GPS antenna and PPS source are both connected with the serial port (i.e. /dev/ttyS0)?

Currently the RFC says to you that you should open the serial port:

	fd = open("/dev/ttyS0", ...);

and the passing its filedes to pps_time_create() in order to get the corresponding PPS source handler:

	pps_time_create(fd, &handler);

As you propose you need _two_ open() and not just one...

and even if
you decide to open the /dev/ppsX inside the pps_time_create(), how do you recognise _which_ /dev/ppsX is connected with filedse "fd"?

I quite sure that RFC is broken since it doesn't take in account that a PPS source maybe not connected with any cahr device at all. I tried to explain this problem to RFC's gurus but they never answered to me, so I decided to resolve the problem by myself. ;)

Quoted 4 lines

As I said, it's not the char device for the physical interface itself being discussed there. That could be parport, uart, some arbit GPIO pin whatever. But whenever the corresponding kernel subsystem does a register_source(), you could create the /dev/ppsXXX device ...

Ok, but in this case you still are _not_ RFC compliant (as showed above). You need that users give to you _two_ devices (the serial line and the PPS source), meanwhile, for the RFC, you just need one. So no differences from my solution from this point of view.

Quoted 2 lines

Hmm, but that's a non-standard, not-mandated-by-RFC syscall. I don't see how you can get this merged, really :-)

They are not-mandated-by-RFC since simply RFC _is broken_! :)

I need them (or just one of them) in order to find a PPS source into the system. Just as you need the second device name in your solution with char devices.

Quoted 4 lines · Rodolfo Giometti

On Mon, Jul 30, 2007 at 10:33:35AM +0530, Satyam Sharma wrote:
Currently the RFC says to you that you should open the serial port:

	fd = open("/dev/ttyS0", ...);

No, it does *NOT*. All it says is:

The time_pps_create() is used to convert an already-open UNIX file descriptor, for an appropriate special file, into a PPS handle.

See? What I said is precisely the implementation the RFC envisages (and the only sane way to implement it too).

Quoted 9 lines · Satyam Sharma <satyam <at> infradead.org> writes:

Quoted 4 lines · Rodolfo Giometti

On Mon, Jul 30, 2007 at 10:33:35AM +0530, Satyam Sharma wrote:
Currently the RFC says to you that you should open the serial port:

	fd = open("/dev/ttyS0", ...);

No, it does *NOT*. All it says is:

Quoted 5 lines

The time_pps_create() is used to convert an already-open UNIX file descriptor, for an appropriate special file, into a PPS handle.

See? What I said is precisely the implementation the RFC envisages (and the only sane way to implement it too).

If we were totally rigurous about representing each device as a device node, your solution would be fine. But we don't.

The clocksource model (/sys/devices/system/clocksource) is a better way to go. One sysfs file is used to enumerate the possible sources and another is
used to read or set the current source. No new system calls; no new ioctls.

Ok, I've looked through (most of) the RFC and code now, and am only commenting on a design-level for now. Anyway, I didn't like the way you've significantly drifted from the RFC in several ways:

Take the race between the time_pps_setparams() syscall and a concurrent pps_event() from an interrupt for instance. From sys_time_pps_setparams, the parameters for an existing source are not modified / set atomically, which means a pps_event() called on the same source in between will see invalid parameters ... and bad things will happen.

Quoted 5 lines · Satyam Sharma

Take the race between the time_pps_setparams() syscall and a concurrent pps_event() from an interrupt for instance. From sys_time_pps_setparams, the parameters for an existing source are not modified / set atomically, which means a pps_event() called on the same source in between will see invalid parameters ... and bad things will happen.

I think this should be a good solution... :)

drivers/pps/kapi.c +0 -6

index 08de71d..f0c42ec 100644
@@ -29,12 +29,6 @@
 #include <linux/pps.h>
 
 /*
- * Local variables
- */
-
-static spinlock_t pps_lock = SPIN_LOCK_UNLOCKED;
-
-/*
  * Local functions
  */
 

drivers/pps/pps.c +3 -0

index 9176c01..91b7e4d 100644
@@ -35,6 +35,7 @@
 
 struct pps_s pps_source[PPS_MAX_SOURCES];
 DEFINE_MUTEX(pps_mutex);
+spinlock_t pps_lock = SPIN_LOCK_UNLOCKED;
 
 /*
  * Misc functions

@@ -227,6 +228,8 @@ asmlinkage long sys_time_pps_setparams(int source,
                goto sys_time_pps_setparams_exit;
        }
 
+       spin_lock(&pps_lock);
+
        /* Save the new parameters */
        ret = copy_from_user(&pps_source[source].params, params,
                                                sizeof(struct pps_kparams));

Nopes, this isn't quite correct/safe. I suggest you should read:

http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/

Quoted 4 lines · Satyam Sharma

Nopes, this isn't quite correct/safe. I suggest you should read:

http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/

I read it but still I don't see why my solution isn't correct/safe. :)

Can you please propose to me your solution?

Quoted 5 lines · Rodolfo Giometti

In pps_event() is not useful using spin_lock_irqsave/restore() since the only difference between spin_lock_irqsave() and spin_lock() is that the former will turn off interrupts if they are on, otherwise does nothing (if we are already in an interrupt handler).

Yup. But two pps_event()'s on different CPU's could still race.

Quoted 2 lines

Maybe you meant I should using spin_lock_irqsave/restore() in user context, but doing like this I will disable interrupts

Yup, but the goal is to avoid races. Otherwise why bother doing any locking at all?

Quoted 3 lines

Of course, I meant "protecting data". In fact to protect pps_source[] I need spin_lock() to protect user context from interrupt context and mutex to protect user context from itself.

But that's nonsensical! That's not how you implement locking!

First, spin_lock() is *not* enough to protect access from process context from access from interrupt context.

Second, if you *already* have a lock to protect any data, introducing *another* lock to protect the same data is ... utterly crazy!

Quoted 6 lines

I see. But consider pps_register_source(). This function should provide protection of pps_source against both interrupt context (pps_event()) and user context (maybe pps_unregister_source() or one syscalls). Using only mutex is not possible, since we cannot use mutex in interrupt context, and using only spin_locks is not possible since in UP() they became void.

Yup, but that's okay. On UP, spin_lock_irqsave() becomes local_irq_save() which is what you want anyway on UP.

The simplest, most straightforward, and safest, most correct, way would be to just use spin_lock_irqsave/restore() to around all access to the shared/global data, from _any_ context.

Anyway, I'll try and see if I find some time this week to implement what I was mentioning ...

Quoted 3 lines

That's the point. I don't wish using _irqsave/restore() since they may delay interrupt handler execution. As above, I prefere loosing the event then registering it at wrong time.

Ok, think of it this way -- you don't have an option. You just *have* to use them. As I said, please read Rusty Russell's introduction to locking in the kernel.

Quoted 2 lines

Why you wish using one lock per sources? Just one lock for the list/array is not enought? :-o

No, I am *not* wishing / advocating that at all. Just that you appear so _reluctant_ to use spinlocks and are unnecessarily worrying about contention, disabling interrupts, etc etc.

Just use the spin_lock_irqsave/restore() variants, and you'll be fine.

No, it does *NOT*. All it says is:

The time_pps_create() is used to convert an already-open UNIX file descriptor, for an appropriate special file, into a PPS handle.

See? What I said is precisely the implementation the RFC envisages (and the only sane way to implement it too).

And later, where it gives an example, it shows:

    fd = open(PPSfilename, O_RDWR, 0);

What I'm saying is that the "PPSfilename", as is obvious from the name itself, is *not* a port such as lpXXX or ttySXXX, but an "appropriate special file" corresponding to a ... PPS source! Really, the RFC is quite clear and easy to read, I have no idea how to explain that more clearly ...

Quoted 1 line

As you propose you need _two_ open() and not just one...

No, why?

Quoted 3 lines

and even if
you decide to open the /dev/ppsX inside the pps_time_create(), how do you recognise _which_ /dev/ppsX is connected with filedse "fd"?

That's trivial to implement in the kernel code for the time_pps_create() syscall.

Quoted 4 lines

I quite sure that RFC is broken since it doesn't take in account that a PPS source maybe not connected with any cahr device at all. I tried to explain this problem to RFC's gurus but they never answered to me, so I decided to resolve the problem by myself. ;)

Nopes, the RFC is not broken at all. All this physical-connection-port device vs PPS-source-device confusion is just in your mind :-)

Quoted 4 lines

Ok, but in this case you still are _not_ RFC compliant (as showed above). You need that users give to you _two_ devices (the serial line and the PPS source), meanwhile, for the RFC, you just need one. So no differences from my solution from this point of view.

Yeah, so how am I not RFC compliant? Userspace will *only* open(2) the special char device of the *PPS source*, and have *nothing* to do with the device corresponding to the physical device/port it is connected through!

Quoted 3 lines

I need them (or just one of them) in order to find a PPS source into the system. Just as you need the second device name in your solution with char devices.

No, I don't need any "second device". I *only* need the "appropriate special file" as mentioned in the RFC. I don't give a *damn* for what *physical device/port* the source is actually connected through. I suggest you should read the RFC again ...

What does the section on locking between hard irq contexts (or between user process context and hard irq context) say?

As I said, you could just use the spin_lock_irqsave/restore() variants ... If you want, I can try and implement the other bits that I had suggested for the other things as well :-)

Quoted 3 lines · Satyam Sharma

Quoted 2 lines

Maybe you meant I should using spin_lock_irqsave/restore() in user context, but doing like this I will disable interrupts

Quoted 2 lines

Yup, but the goal is to avoid races. Otherwise why bother doing any locking at all?

I meant that if you have to lock between user context and interrupt context you have two choises:

1) using spin_lock_irqsave/restore() in user context and spin_lock/unlock() in the interrupt context (as Rusty says),

2) or using spin_lock/unlock() in user context and spin_trylock/unlock() in the interrupt context in order to avoid dead locks.

Quoted 11 lines

Quoted 3 lines

Of course, I meant "protecting data". In fact to protect pps_source[] I need spin_lock() to protect user context from interrupt context and mutex to protect user context from itself.

But that's nonsensical! That's not how you implement locking!

First, spin_lock() is *not* enough to protect access from process context from access from interrupt context.

Second, if you *already* have a lock to protect any data, introducing *another* lock to protect the same data is ... utterly crazy!

I see what you mean. But my question is about using spin_locks where we can sleep. Let me explain, consider sys_time_pps_getparams():
[...]
The copy_to_user() may sleep and if I change mutex_lock_interruptible() with a spin_lock I may hold it and then going to sleep... using mutex we can use the CPU for other computations.

Quoted 8 lines

Quoted 2 lines

Why you wish using one lock per sources? Just one lock for the list/array is not enought? :-o

No, I am *not* wishing / advocating that at all. Just that you appear so _reluctant_ to use spinlocks and are unnecessarily worrying about contention, disabling interrupts, etc etc.

Just use the spin_lock_irqsave/restore() variants, and you'll be fine.

What I wish is just to avoid disabling IRQs in user context in order to minimize the possibility to delay events recording.

Quoted 3 lines

Quoted 1 line

As you propose you need _two_ open() and not just one...

No, why?

Please, take alook at NTPD code. Common usage is:

   fd = open("/dev/ttyS0", ...);

   pps_time_create(fd, &handler);

since RFC supposes that at filedes "fd" is mapped both GPS data _and_ PPS source.

Furthermore with my pps_findpath() I can do:

   fd = open("/dev/ttyS0", ...);

   handler = pps_findpath("/dev/ttyS0", ...);

which in most cases its more easy to manage for both user and kernel land.

Can do the same with char devices?

Maybe you can easily create
/dev/pps0 but how can you relate it with the /dev/ttyS0 if your GPS antenna and PPS source share the same serial port?

Quoted 7 lines

Quoted 4 lines

I quite sure that RFC is broken since it doesn't take in account that a PPS source maybe not connected with any cahr device at all. I tried to explain this problem to RFC's gurus but they never answered to me, so I decided to resolve the problem by myself. ;)

Nopes, the RFC is not broken at all. All this physical-connection-port device vs PPS-source-device confusion is just in your mind :-)

Ok, if you don't think so try looking at NTPD code (written by RFC's gurus) and try to resolve the problem where both GPS antenna and PPS source are connected with a serial port, and the problem where only the GPS antenna is connected with the serial port but the PPS source is connected with a CPU's GPIO.

If you solve both them all PPS users will thank you a lot forever! :)

No, they require an _already_ opened file descriptor because they supposed you can use serial line file descriptor!

Again, RFC was sane if it said that you can get a PPS hadler by simply doing:

   time_pps_create("/dev/pps0", &handle);

this could be easily implemented with just an open()...

They didn't like this because they __never__ considered the case where the PPS source is not connected at all with any serial/parallel decices.

Quoted 2 lines

If you want, I can try and implement the other bits that I had suggested for the other things as well :-)

Great! Thanks a lot, so we can discuss on them and maybe we can prive Linux of this PPS support! :)

Hi Rodolfo,

Yup, this would avoid races, but then we will lose events. Why is that acceptable, when better alternative (above) exists?

Seriously, your aversion to implement good, safe locking is amazing! :-)

The proper way to do this is to use a kernel buffer to do all kernel-side work (you acquire/release lock during that work) and then copy_to_user() later, at the end. [ And something opposite for the set_xxx syscall, i.e. first off copy_from_user() to a kernel buffer up front, before doing anything else itself, and then do all the work in the kernel on that. ]

BTW your syscall implementations totally lack any kind of security checks whatsoever ...

Amazing. On the one hand, you want to use spin_trylock() in the hard irq handler and spin_lock() (not irq safe) in the process context, because you "don't care about losing some events". And now you want to avoid "disabling irqs in user context to minimize possibility to delay events recording"?

Anyway, any such requirement you're talking about is just bogus, IMHO. You're just disabling interrupts to access a data structure, for Gods' sakes, how many nanoseconds do you imagine would you be "delaying"?

Quoted 8 lines

Please, take alook at NTPD code. Common usage is:

   fd = open("/dev/ttyS0", ...);

   pps_time_create(fd, &handler);

since RFC supposes that at filedes "fd" is mapped both GPS data _and_ PPS source.

Umm, I don't think the RFC supposes/assumes this anywhere.

Of course. BTW time_pps_findpath/findsource do not have to be kernel implemented syscalls in the first place. The best, simplest and most straightforward place to implement them is in userspace -- the RFC mentions this as well.

A solution I can think of is to create a mapping at the time of pps_register_source() between the PPS source (physically) and the special char device. Userspace open(2)'s the special char device corresponding to the PPS source, and then issues the time_pps_create() syscall. Here, we lookup the mapping previously created and return handle to the PPS source based on the special device's fd that's passed to us in time_pps_create().

Ok, I'll take a look, thanks :-)

[ what's the URL for these sources? ]

open(PPSfilename, O_RDWR) doesn't sound like that at all. If the device for the serial port / parport was intended, it wouldn't be called "PPSfilename", would it?

The physical port/device through which a PPS source is connected is immaterial. Anyway, I'll take a look at the NTPD/userspace code you are mentioning as well. [ again, could you point me to URL of source code? ]

Sure, I'll get to implementing what I have in mind here -- will be glad to be of any help, thanks.

Quoted 2 lines · Satyam Sharma

Yup, this would avoid races, but then we will lose events. Why is that acceptable, when better alternative (above) exists?

Because is better lossign events then recording them delayed. In the past we (LinuxPPS users) noticed that just postponing of one instruction the timestamp recording degrade the timesetting of about 50%!

Quoted 5 lines

Amazing. On the one hand, you want to use spin_trylock() in the hard irq handler and spin_lock() (not irq safe) in the process context, because you "don't care about losing some events". And now you want to avoid "disabling irqs in user context to minimize possibility to delay events recording"?

Just for not delaying the IRQ handler. As already said, I prefere loosing events that delaying their timestamps recording.

Quoted 3 lines

Anyway, any such requirement you're talking about is just bogus, IMHO. You're just disabling interrupts to access a data structure, for Gods' sakes, how many nanoseconds do you imagine would you be "delaying"?

As already said we noticed that just delaying of one instruction the timestamp recording the time setting degrades of about 50%.

We have to take care of this point. It's _very_ important that _each_ event had its timestamp recorded with delay as small as possible.

Quoted 5 lines

The proper way to do this is to use a kernel buffer to do all kernel-side work (you acquire/release lock during that work) and then copy_to_user() later, at the end. [ And something opposite for the set_xxx syscall, i.e. first off copy_from_user() to a kernel buffer up front, before doing anything else itself, and then do all the work in the kernel on that. ]

Mmm... I think this is not easy at all for sys_time_pps_fetch(). I suppose you have to complicate its code a lot!

I don't undestand why we must complicate, and made unreadable, a code in order to follow the rule use-only-one-lock-mechanism. If by using mutex and spinlocks the code is more readable, where is the fault?

Quoted 2 lines

BTW your syscall implementations totally lack any kind of security checks whatsoever ...

Ok, we can correct them. :)

Quoted 6 lines

Quoted 6 lines

Please, take alook at NTPD code. Common usage is:

   fd = open("/dev/ttyS0", ...);

   pps_time_create(fd, &handler);

Quoted 4 lines

Quoted 2 lines

since RFC supposes that at filedes "fd" is mapped both GPS data _and_ PPS source.

Umm, I don't think the RFC supposes/assumes this anywhere.

I think so. If not, why they pretend an _already opened_ filedes then just a filename to be used as parameter for the open(2) syscall? :)

That's just absolute bullshit.

I'm sorry to say this, Rodolfo, but _all_ your arguments above are *totally* nonsensical and factually incorrect -- and I have had enough of trying to talk sense to you, it's been ~15 mails in this thread already, and I've been WASTING my time trying to teach / explain to you, but it's just *shocking* that you prefer to stick to a wholly incorrect (which I suspect you've already understood by now anyway) position rather than just accepting that the present patch is wrong and go about correcting it instead.
[snip]

Sorry for wasting your time. :'( Maybe you can provide your solution for PPS support and get it included into kernel tree so we can use it and live happy! :)

I only glanced through the code, so could be wrong, but I noticed that the only global / shared data you have in there is a global "pps_source" array of pps_s structs. That's accessed / modified from the various syscalls introduced in the API exported to userspace, as well as the register/unregister/pps_event API exported to in-kernel client subsystems, yes? So it looks like you need to introduce proper locking for it, simply type-qualifying it as "volatile" is not enough.

However, I think you've introduced two locks for it. The syscalls (that run in process context, obviously) seem to use a pps_mutex and pps_event() seems to be using the pps_lock spinlock (because that gets executed from interrupt context) -- and from the looks of it, the register/unregister functions are using /both/ the mutex and spinlock (!)

This isn't quite right, (in fact there's nothing to protect pps_event from racing against a syscall), so you should use *only* the spinlock for synchronization -- the spin_lock_irqsave/restore() variants, in fact.

[ Also, have you considered making pps_source a list and not an array? It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc kind of gymnastics in there, and you _can_ return a pointer to the corresponding pps source struct from the register() function to the in-kernel users, so that way you get to retain the O(1) access to the corresponding source when a client calls into pps_event(), similar to how you're using the array index presently. ]

I also noticed code like (from pps_event):

drivers/pps/kapi.c new file +3 -0

+	/* Try to grab the lock, if not we prefere loose the event... */
+	if (!spin_trylock(&pps_lock))
+		return;

which looks worrisome and unnecessary. That spinlock looks to be of fine enough granularity to me, do you think there'd be any contention on it? I /think/ you can simply make that a spin_lock().

Overall the code looks simple / straightforward enough to me (except for the parport / uart stuff that I have no clue about), and I'll also read up on the relevant RFC for this and would hopefully try and give you a more meaningful review over the weekend.

Quoted 9 lines · Satyam Sharma

I only glanced through the code, so could be wrong, but I noticed that the only global / shared data you have in there is a global "pps_source" array of pps_s structs. That's accessed / modified from the various syscalls introduced in the API exported to userspace, as well as the register/unregister/pps_event API exported to in-kernel client subsystems, yes? So it looks like you need to introduce proper locking for it, simply type-qualifying it as "volatile" is not enough.

Quoted 5 lines

However, I think you've introduced two locks for it. The syscalls (that run in process context, obviously) seem to use a pps_mutex and pps_event() seems to be using the pps_lock spinlock (because that gets executed from interrupt context) -- and from the looks of it, the register/unregister functions are using /both/ the mutex and spinlock (!)

This is right.

Quoted 3 lines

This isn't quite right, (in fact there's nothing to protect pps_event from racing against a syscall), so you should use *only* the spinlock for synchronization -- the spin_lock_irqsave/restore() variants, in fact.

We can't use the spin_lock_irqsave/restore() variants since PPS sources cannot manage IRQ enable/disable. For instance, the serial source doesn't manage IRQs directly but just uses it to record PPS events. The serial driver manages the IRQ enable/disable, not the PPS source which only uses the IRQ handler to records events.

About using both mutex and spinlock I did it since (I think) I should protect syscalls from each others and from pps_register/unregister(), and pps_event() against pps_register/unregister().

Quoted 14 lines

[ Also, have you considered making pps_source a list and not an array? It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc kind of gymnastics in there, and you _can_ return a pointer to the corresponding pps source struct from the register() function to the in-kernel users, so that way you get to retain the O(1) access to the corresponding source when a client calls into pps_event(), similar to how you're using the array index presently. ]

I also noticed code like (from pps_event):

(quoted diff) +3 -0

+	/* Try to grab the lock, if not we prefere loose the event... */
+	if (!spin_trylock(&pps_lock))
+		return;

Quoted 3 lines

which looks worrisome and unnecessary. That spinlock looks to be of fine enough granularity to me, do you think there'd be any contention on it? I /think/ you can simply make that a spin_lock().

This is due the fact I cannot manage IRQ enable/disable.

Hmm? I still don't see why you can't introduce spin_lock_irqsave/restore() in pps_event() around the access to pps_source.

Quoted 3 lines

About using both mutex and spinlock I did it since (I think) I should protect syscalls from each others and from pps_register/unregister(), and pps_event() against pps_register/unregister().

Nopes, it's not about protecting code from each other, you're needlessly complicating things. Locking is pretty simple, really -- any shared data, that can be concurrently accessed by multiple threads (or from interrupts) must be protected with a lock. Note that *data* is protected by a lock, and not "code" that handles it (well, this is the kind of behaviour most cases need, at least, including yours).

So here we're introducing the lock to protect *pps_source*, and not keep *threads* of execution from stepping over each other. So, simply, just ensure you grab the lock whenever you want to start accessing the shared data, and release it when you're done.

The _irqsave/restore() variants are required because (say) one of the syscalls executing in process context grabs the spinlock. Then, before it has released it, it gets interrupted and pps_event() begins executing. Now pps_event() also wants to grab the lock, but the syscall already has it, so will continue spinning and deadlock!

I think you're unnecessarily worrying about contention here -- you can have multiple locks (one for the list, and separate ones for your sources) if you're really worrying about contention -- or probably rwlocks. But really, rwlocks would end up being *slower* than spinlocks, unless the contention is really heavy and it helps to keep multiple readers in the critical section. But frankly, with at max a few (I'd expect generally one) PPS sources ever to be connected / registered with teh system, and just one-pulse-per-second, I don't see why any contention is ever gonna happen.

Quoted 3 lines · Satyam Sharma

Hmm? I still don't see why you can't introduce spin_lock_irqsave/restore() in pps_event() around the access to pps_source.

In pps_event() is not useful using spin_lock_irqsave/restore() since the only difference between spin_lock_irqsave() and spin_lock() is that the former will turn off interrupts if they are on, otherwise does nothing (if we are already in an interrupt handler).

Maybe you meant I should using spin_lock_irqsave/restore() in user context, but doing like this I will disable interrupts

and I don't
wish doing it since, in this manner, the interrupt handler will be delayed and the (probably) PPS event recording will be wrong. I prefere loosing the event that registering it at delayed time.

Quoted 10 lines

Quoted 3 lines

About using both mutex and spinlock I did it since (I think) I should protect syscalls from each others and from pps_register/unregister(), and pps_event() against pps_register/unregister().

Nopes, it's not about protecting code from each other, you're needlessly complicating things. Locking is pretty simple, really -- any shared data, that can be concurrently accessed by multiple threads (or from interrupts) must be protected with a lock. Note that *data* is protected by a lock, and not "code" that handles it (well, this is the kind of behaviour most cases need, at least, including yours).

Of course, I meant "protecting data". In fact to protect pps_source[] I need spin_lock() to protect user context from interrupt context and mutex to protect user context from itself.

Quoted 4 lines

So here we're introducing the lock to protect *pps_source*, and not keep *threads* of execution from stepping over each other. So, simply, just ensure you grab the lock whenever you want to start accessing the shared data, and release it when you're done.

I see. But consider pps_register_source(). This function should provide protection of pps_source against both interrupt context (pps_event()) and user context (maybe pps_unregister_source() or one syscalls). Using only mutex is not possible, since we cannot use mutex in interrupt context, and using only spin_locks is not possible since in UP() they became void.

Can you please show me how I could write pps_register_source() in order to be correct from your point of view?

Quoted 5 lines

The _irqsave/restore() variants are required because (say) one of the syscalls executing in process context grabs the spinlock. Then, before it has released it, it gets interrupted and pps_event() begins executing. Now pps_event() also wants to grab the lock, but the syscall already has it, so will continue spinning and deadlock!

That's the point. I don't wish using _irqsave/restore() since they may delay interrupt handler execution. As above, I prefere loosing the event then registering it at wrong time.

Quoted 9 lines

I think you're unnecessarily worrying about contention here -- you can have multiple locks (one for the list, and separate ones for your sources) if you're really worrying about contention -- or probably rwlocks. But really, rwlocks would end up being *slower* than spinlocks, unless the contention is really heavy and it helps to keep multiple readers in the critical section. But frankly, with at max a few (I'd expect generally one) PPS sources ever to be connected / registered with teh system, and just one-pulse-per-second, I don't see why any contention is ever gonna happen.

Why you wish using one lock per sources? Just one lock for the list/array is not enought? :-o

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.

drivers/built-in.o: In function `sys_time_pps_fetch':
(.text+0x5f05e): undefined reference to `__udivdi3'

Quoted 2 lines · Rodolfo Giometti

drivers/built-in.o: In function `sys_time_pps_fetch':
(.text+0x5f05e): undefined reference to `__udivdi3'

Hm, not sure. Maybe put it back to uint32_t and then add another uint32_t of explicit padding, or maybe just cast it to uint32_t when you divide:

	ticks += (uint32_t)to.nsec / (NSEC_PER_SEC/HZ);