Re: [PATCH] LinuxPPS - definitive version

[Rodolfo Giometti]

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.

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

Ok, I'll do it.

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

About this debate, please, take a look at the pps_event() function:

void pps_event(int source, int event, void *data)
{
        struct timespec __ts;
        struct pps_ktime ts;

        /* First of all we get the time stamp... */
        getnstimeofday(&__ts);

        /* ... and translate it to PPS time data struct */
        ts.sec = __ts.tv_sec;
        ts.nsec = __ts.tv_nsec;

        if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0 ) {
                pps_err("unknow event (%x) for source %d", event, source);
                return;
        }

        /* We wish not using locks at all into this function... a possible
         * solution is to check the "info" field against the pointer to
         * "dummy_info".
         * If "info" points to "dummy_info" we can return doing nothing since,
         * even if a new PPS source is registered by another CPU we can
         * safely not register current event.
         * If "info" points to a valid PPS source's info data we can continue
         * without problem since, even if current PPS source is deregistered
         * by another CPU, we still continue writing data into a valid area
         * (dummy_info).
         */
        if (pps_source[source].info == &dummy_info)
                return;

        /* Must call the echo function? */
        if ((pps_source[source].params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)))
                pps_source[source].info->echo(source, event, data);

        /* Check the event */
        pps_source[source].current_mode = pps_source[source].params.mode;
        if (event & PPS_CAPTUREASSERT) {
                /* We have to add an offset? */
                if (pps_source[source].params.mode & PPS_OFFSETASSERT)
                        pps_add_offset(&ts,
                                &pps_source[source].params.assert_off_tu);

                /* Save the time stamp */
                pps_source[source].assert_tu = ts;
                pps_source[source].assert_sequence++;
                pps_dbg("capture assert seq #%u for source %d",
                        pps_source[source].assert_sequence, source);
        }
        if (event & PPS_CAPTURECLEAR) {
                /* We have to add an offset? */
                if (pps_source[source].params.mode & PPS_OFFSETCLEAR)
                        pps_add_offset(&ts,
                                &pps_source[source].params.clear_off_tu);

                /* Save the time stamp */
                pps_source[source].clear_tu = ts;
                pps_source[source].clear_sequence++;
                pps_dbg("capture clear seq #%u for source %d",
                        pps_source[source].clear_sequence, source);
        }

        pps_source[source].go = ~0;
        wake_up_interruptible(&pps_source[source].queue);
}

The problems should arise at:

	if (pps_source[source].info == &dummy_info)
		return;

but as explained into the comment there should be no problems at
all...

About "where" to put the "volatile" attribute I don't understand what
you mean... such attribute is needed (IMHO) for "assert_sequence"&C,
where should I put it? :-o
 
> 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.

As already suggested I used fixed size variables. See the new struct
"struct pps_ktime".

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

Such code is for userland since RFC2783 requires such types... I moved
all userland code into Documentation/pps/timepps.h which can be used
by userland programs whose require RFC compatibility.

I'll post a new patch ASAP.

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti@xxxxxxxxxxxx
Linux Device Driver                             giometti@xxxxxxxxx
Embedded Systems                     		giometti@xxxxxxxx
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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/