Message-ID: <20070729095040.GZ9840@enneenne.com>
Date:	Sun, 29 Jul 2007 11:50:40 +0200
From:	Rodolfo Giometti <giometti@...eenne.com>
To:	Satyam Sharma <satyam.sharma@...il.com>
Cc:	Chris Friesen <cfriesen@...tel.com>,
	David Woodhouse <dwmw2@...radead.org>,
	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: LinuxPPS & spinlocks

On Sat, Jul 28, 2007 at 05:11:17AM +0530, Satyam Sharma wrote:
> 
> 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 are right. I'll add spinlocks. :)

> > [ 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 see. I'll study the problem but I think this is can be done
later, now I think is better having a working code. :)

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

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.

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

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

This is just the reason why I added those functions. :)

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

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

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

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

Ditto.

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

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

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

Ok, I see, but I think this should be done when everithing is ok. I
think after fixing locking issue I can do it, can't I?

Thanks,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti@...eenne.com
Linux Device Driver                             giometti@...dd.com
Embedded Systems                     		giometti@...ux.it
UNIX programming                     phone:     +39 349 2432127
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists