Pulse per Second (PPS) support for Linux.
Signed-off-by: Rodolfo Giometti <[email protected]>
---
Here my last release of PPS support for Linux.
The difference against my last patch is about all userland specific
code (timepps.h) which has been removed, I hope now you can consider
adding it into kernel source tree!
Please, let me know if I still should fix something else.
---
On Wed, 2007-05-02 at 21:33 +0200, Rodolfo Giometti wrote:
> Pulse per Second (PPS) support for Linux.
>
> Signed-off-by: Rodolfo Giometti <[email protected]>
>
> ---
>
> Here my last release of PPS support for Linux.
>
> The difference against my last patch is about all userland specific
> code (timepps.h) which has been removed, I hope now you can consider
> adding it into kernel source tree!
>
> Please, let me know if I still should fix something else.
Please inline your patch, rather then attaching them. It makes it very
difficult to discuss when it is attached.
> diff --git a/drivers/pps/clients/ktimer.c b/drivers/pps/clients/ktimer.c
> new file mode 100644
> index 0000000..7514389
> --- /dev/null
> +++ b/drivers/pps/clients/ktimer.c
> @@ -0,0 +1,106 @@
> +/*
> + * ktimer.c -- kernel timer test client
> + *
Could you use a better name, like pps_ktimer_test.c, so it is less generic?
Same goes for your kabi.c and sysfs.c files.
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 98ec861..543c7cb 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -2004,6 +2004,8 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
> up->ier |= UART_IER_MSI;
> if (up->capabilities & UART_CAP_UUE)
> up->ier |= UART_IER_UUE | UART_IER_RTOIE;
> + if (up->port.flags & UPF_HARDPPS_CD)
> + up->ier |= UART_IER_MSI; /* enable interrupts */
>
This isn't covered by a #ifdef, so is this always safe? Should it be in
a separate patch?
Unfortunately I don't have any hardware to play with this, but I'd
suggest you send this to Andrew Morton for inclusion into his tree for
testing.
thanks
-john
On Wed, May 02, 2007 at 02:06:53PM -0700, john stultz wrote:
>
> Please inline your patch, rather then attaching them. It makes it very
> difficult to discuss when it is attached.
Ok.
> > +++ b/drivers/pps/clients/ktimer.c
> > @@ -0,0 +1,106 @@
> > +/*
> > + * ktimer.c -- kernel timer test client
> > + *
>
> Could you use a better name, like pps_ktimer_test.c, so it is less generic?
Ok. But consider that this is just a testing program.
> Same goes for your kabi.c and sysfs.c files.
Why? These files don't generate .ko files.
> > @@ -2004,6 +2004,8 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
> > up->ier |= UART_IER_MSI;
> > if (up->capabilities & UART_CAP_UUE)
> > up->ier |= UART_IER_UUE | UART_IER_RTOIE;
> > + if (up->port.flags & UPF_HARDPPS_CD)
> > + up->ier |= UART_IER_MSI; /* enable interrupts */
> >
>
> This isn't covered by a #ifdef, so is this always safe? Should it be in
> a separate patch?
No, this part is regarding the serial driver itself. Maybe it can be
placed in a separate patch but it is still about the PPS support for
Linux...
> Unfortunately I don't have any hardware to play with this, but I'd
> suggest you send this to Andrew Morton for inclusion into his tree for
> testing.
I'll do it.
Thanks,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Wed, 2 May 2007 21:33:15 +0200 Rodolfo Giometti <[email protected]> wrote:
> Pulse per Second (PPS) support for Linux.
Have a patch:
From: Andrew Morton <[email protected]>
Review comments:
- Running a timer once per second will make the super-low-power people upset.
- This uses netlink? Is that interface documented anywhere?
Please check with Dave Miller that this:
#define NETLINK_PPSAPI 20
reservation is OK.
- This:
if ((nlpps->tsformat != PPS_TSFMT_TSPEC) != 0 ) {
is weird. I changed it to
if (nlpps->tsformat != PPS_TSFMT_TSPEC) {
- 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.
- 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()
- This:
memset(pps_source, 0, sizeof(struct pps_s) * PPS_MAX_SOURCES);
was unneeded. The C startup code already did that.
- All these separators:
+/* --- Input function ------------------------------------------------------ */
aren't typical for kernel code. I left them in, but please consider
removing them all.
- 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.
- 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
<[email protected]>.
- 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.
- The Kconfig purports to support CONFIG_PPS=m. Does that actually work?
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.
- 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 <[email protected]>
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.
- Please ensure that `make headers_check' passes OK (you'll hear from me if
it doesn't ;))
- Can we get rid of the private dbg, err and info macros? Surely there are
generic ones somewhere.
- struct pps_s has volatiles in it. Please remove them. There's lots of
discussion on this topic on linux-kernel just today.
- Why did the
port->icount.dcd++;
get moved in uart_handle_dcd_change()?
- In lots of places you do:
+#ifdef CONFIG_PPS_CLIENT_UART
+#include <linux/pps.h>
+#endif
Please remove the ifdefs at all these sites and make the header file
handle it.
- 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.
- Generally: code looks OK and is probably useful. Please keep going ;)
Code changes:
- fix docs a bit
- uninline lp_pps_echo(): we take its address.
- remove unneeded and undesirable casts of void*'s
- coding-style fixes
- various changes to the use of the timer API.
- Remove lots of inlinings. The compiler gets this right.
- DEFINE_SPINLOCK is required: SPIN_LOCK_UNLOCKED defeats lockdep.
Cc: Rodolfo Giometti <[email protected]>
Cc: john stultz <[email protected]>
Cc: Roman Zippel <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
Documentation/pps.txt | 4 -
drivers/char/lp.c | 7 +--
drivers/pps/clients/Kconfig | 8 +--
drivers/pps/clients/ktimer.c | 19 +++-----
drivers/pps/kapi.c | 40 ++++++++---------
drivers/pps/pps.c | 75 +++++++++++----------------------
drivers/pps/sysfs.c | 14 +++---
drivers/serial/8250.c | 1
include/linux/pps.h | 9 ++-
9 files changed, 77 insertions(+), 100 deletions(-)
diff -puN Documentation/pps.txt~linuxpps-pulse-per-second-support-for-linux-fix Documentation/pps.txt
--- a/Documentation/pps.txt~linuxpps-pulse-per-second-support-for-linux-fix
+++ a/Documentation/pps.txt
@@ -49,7 +49,7 @@ problem:
This implies that the source has a /dev/... entry. This assumption is
ok for the serial and parallel port, where you can do something
-usefull besides(!) the gathering of timestamps as it is the central
+useful besides(!) the gathering of timestamps as it is the central
task for a PPS-API. But this assumption does not work for a single
purpose GPIO line. In this case even basic file-related functionality
(like read() and write()) makes no sense at all and should not be a
@@ -167,7 +167,7 @@ inside you find several files:
Inside each "assert" and "clear" file you can find the timestamp and a
sequence number:
- $ cat cat /sys/class/pps/00/assert
+ $ cat /sys/class/pps/00/assert
1170026870.983207967#8
Where before the "#" is the timestamp in seconds and after it is the
diff -puN drivers/char/lp.c~linuxpps-pulse-per-second-support-for-linux-fix drivers/char/lp.c
--- a/drivers/char/lp.c~linuxpps-pulse-per-second-support-for-linux-fix
+++ a/drivers/char/lp.c
@@ -750,9 +750,9 @@ static struct console lpcons = {
#ifdef CONFIG_PPS_CLIENT_LP
-static inline void lp_pps_echo(int source, int event, void *data)
+static void lp_pps_echo(int source, int event, void *data)
{
- struct parport *port = (struct parport *) data;
+ struct parport *port = data;
unsigned char status = parport_read_status(port);
/* echo event via SEL bit */
@@ -860,8 +860,7 @@ static int lp_register(int nr, struct pa
else
info("PPS source #%d \"%s\" added to the system",
port->pps_source, port->pps_info.path);
- }
- else {
+ } else {
port->pps_source = -1;
err("PPS support disabled due port \"%s\" is in polling mode",
port->pps_info.path);
diff -puN drivers/pps/clients/Kconfig~linuxpps-pulse-per-second-support-for-linux-fix drivers/pps/clients/Kconfig
--- a/drivers/pps/clients/Kconfig~linuxpps-pulse-per-second-support-for-linux-fix
+++ a/drivers/pps/clients/Kconfig
@@ -16,21 +16,21 @@ config PPS_CLIENT_KTIMER
will be called ktimer.o.
comment "UART serial support (forced off)"
- depends on ! ( SERIAL_CORE != n && ! ( PPS = m && SERIAL_CORE = y ) )
+ depends on ! (SERIAL_CORE != n && !(PPS = m && SERIAL_CORE = y))
config PPS_CLIENT_UART
bool "UART serial support"
- depends on SERIAL_CORE != n && ! ( PPS = m && SERIAL_CORE = y )
+ depends on SERIAL_CORE != n && !(PPS = m && SERIAL_CORE = y)
help
If you say yes here you get support for a PPS source connected
with the CD (Carrier Detect) pin of your serial port.
comment "Parallel printer support (forced off)"
- depends on ! ( PRINTER != n && ! ( PPS = m && PRINTER = y ) )
+ depends on !( PRINTER != n && !(PPS = m && PRINTER = y))
config PPS_CLIENT_LP
bool "Parallel printer support"
- depends on PRINTER != n && ! ( PPS = m && PRINTER = y )
+ depends on PRINTER != n && !(PPS = m && PRINTER = y)
help
If you say yes here you get support for a PPS source connected
with the interrupt pin of your parallel port.
diff -puN drivers/pps/clients/ktimer.c~linuxpps-pulse-per-second-support-for-linux-fix drivers/pps/clients/ktimer.c
--- a/drivers/pps/clients/ktimer.c~linuxpps-pulse-per-second-support-for-linux-fix
+++ a/drivers/pps/clients/ktimer.c
@@ -35,14 +35,13 @@ static struct timer_list ktimer;
/* --- The kernel timer ----------------------------------------------------- */
-static void pps_ktimer_event(unsigned long ptr) {
+static void pps_ktimer_event(unsigned long ptr)
+{
info("PPS event at %lu", jiffies);
pps_event(source, PPS_CAPTUREASSERT, NULL);
- /* Rescheduling */
- ktimer.expires = jiffies+HZ; /* 1 second */
- add_timer(&ktimer);
+ mod_timer(&ktimer, jiffies + HZ);
}
/* --- The echo function ---------------------------------------------------- */
@@ -50,9 +49,9 @@ static void pps_ktimer_event(unsigned lo
static void pps_ktimer_echo(int source, int event, void *data)
{
info("echo %s %s for source %d",
- event&PPS_CAPTUREASSERT ? "assert" : "",
- event&PPS_CAPTURECLEAR ? "clear" : "",
- source);
+ event & PPS_CAPTUREASSERT ? "assert" : "",
+ event & PPS_CAPTURECLEAR ? "clear" : "",
+ source);
}
/* --- The PPS info struct -------------------------------------------------- */
@@ -88,10 +87,8 @@ static int __init pps_ktimer_init(void)
}
source = ret;
- init_timer(&ktimer);
- ktimer.function = pps_ktimer_event;
- ktimer.expires = jiffies+HZ; /* 1 second */
- add_timer(&ktimer);
+ setup_timer(&ktimer, pps_ktimer_event, 0);
+ mod_timer(&ktimer, jiffies + HZ);
info("ktimer PPS source registered at %d", source);
diff -puN drivers/pps/kapi.c~linuxpps-pulse-per-second-support-for-linux-fix drivers/pps/kapi.c
--- a/drivers/pps/kapi.c~linuxpps-pulse-per-second-support-for-linux-fix
+++ a/drivers/pps/kapi.c
@@ -24,7 +24,6 @@
#include <linux/module.h>
#include <linux/init.h>
#include <linux/time.h>
-
#include <linux/pps.h>
/* --- Local functions ----------------------------------------------------- */
@@ -44,7 +43,8 @@ static void pps_add_offset(struct timesp
/* --- Exported functions -------------------------------------------------- */
-static inline int __pps_register_source(struct pps_source_info_s *info, int default_params, int try_id)
+static int __pps_register_source(struct pps_source_info_s *info,
+ int default_params, int try_id)
{
int i;
@@ -54,8 +54,7 @@ static inline int __pps_register_source(
return -EBUSY;
}
i = try_id;
- }
- else {
+ } else {
for (i = 0; i < PPS_MAX_SOURCES; i++)
if (!pps_is_allocated(i))
break;
@@ -66,15 +65,16 @@ static inline int __pps_register_source(
}
/* Sanity checks */
- if ((info->mode&default_params) != default_params) {
+ if ((info->mode & default_params) != default_params) {
err("unsupported default parameters");
return -EINVAL;
}
- if ((info->mode&(PPS_ECHOASSERT|PPS_ECHOCLEAR)) != 0 && info->echo == NULL) {
+ if ((info->mode & (PPS_ECHOASSERT|PPS_ECHOCLEAR)) != 0 &&
+ info->echo == NULL) {
err("echo function is not defined");
return -EINVAL;
}
- if ((info->mode&(PPS_TSFMT_TSPEC|PPS_TSFMT_NTPFP)) == 0) {
+ if ((info->mode & (PPS_TSFMT_TSPEC|PPS_TSFMT_NTPFP)) == 0) {
err("unspecified time format");
return -EINVAL;
}
@@ -89,7 +89,8 @@ static inline int __pps_register_source(
return i;
}
-int pps_register_source(struct pps_source_info_s *info, int default_params, int try_id)
+int pps_register_source(struct pps_source_info_s *info, int default_params,
+ int try_id)
{
unsigned long flags;
int i, ret;
@@ -108,8 +109,9 @@ int pps_register_source(struct pps_sourc
return i;
}
+EXPORT_SYMBOL(pps_register_source);
-static inline void __pps_unregister_source(struct pps_source_info_s *info)
+static void __pps_unregister_source(struct pps_source_info_s *info)
{
int i;
@@ -136,6 +138,7 @@ void pps_unregister_source(struct pps_so
__pps_unregister_source(info);
spin_unlock_irqrestore(&pps_lock, flags);
}
+EXPORT_SYMBOL(pps_unregister_source);
void pps_event(int source, int event, void *data)
{
@@ -153,21 +156,22 @@ void pps_event(int source, int event, vo
err("unknow source for event!");
return;
}
- if ((event&(PPS_CAPTUREASSERT|PPS_CAPTURECLEAR)) == 0 ) {
+ if ((event & (PPS_CAPTUREASSERT|PPS_CAPTURECLEAR)) == 0 ) {
err("unknow event (%x) for source %d", event, source);
return;
}
/* Must call the echo function? */
- if ((pps_source[source].params.mode&(PPS_ECHOASSERT|PPS_ECHOCLEAR)) != 0)
+ if ((pps_source[source].params.mode & (PPS_ECHOASSERT|PPS_ECHOCLEAR)) != 0)
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) {
+ 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.tspec);
+ pps_add_offset(&ts,
+ &pps_source[source].params.assert_off_tu.tspec);
/* Save the time stamp */
pps_source[source].assert_tu.tspec = ts;
@@ -175,10 +179,11 @@ void pps_event(int source, int event, vo
dbg("capture assert seq #%lu for source %d",
pps_source[source].assert_sequence, source);
}
- if (event&PPS_CAPTURECLEAR) {
+ 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.tspec);
+ pps_add_offset(&ts,
+ &pps_source[source].params.clear_off_tu.tspec);
/* Save the time stamp */
pps_source[source].clear_tu.tspec = ts;
@@ -189,9 +194,4 @@ void pps_event(int source, int event, vo
wake_up_interruptible(&pps_source[source].queue);
}
-
-/* --- Exported functions -------------------------------------------------- */
-
-EXPORT_SYMBOL(pps_register_source);
-EXPORT_SYMBOL(pps_unregister_source);
EXPORT_SYMBOL(pps_event);
diff -puN drivers/pps/pps.c~linuxpps-pulse-per-second-support-for-linux-fix drivers/pps/pps.c
--- a/drivers/pps/pps.c~linuxpps-pulse-per-second-support-for-linux-fix
+++ a/drivers/pps/pps.c
@@ -31,7 +31,7 @@
/* --- Global variables ---------------------------------------------------- */
struct pps_s pps_source[PPS_MAX_SOURCES];
-spinlock_t pps_lock = SPIN_LOCK_UNLOCKED;
+DEFINE_SPINLOCK(pps_lock);
/* --- Local variables ----------------------------------------------------- */
@@ -44,7 +44,7 @@ static inline int pps_check_source(int s
return (source < 0 || !pps_is_allocated(source)) ? -EINVAL : 0;
}
-static inline int pps_find_source(int source)
+static int pps_find_source(int source)
{
int i;
@@ -65,7 +65,7 @@ static inline int pps_find_source(int so
return i;
}
-static inline int pps_find_path(char *path)
+static int pps_find_path(char *path)
{
int i;
@@ -90,11 +90,9 @@ static void pps_nl_data_ready(struct soc
struct sk_buff *skb;
struct nlmsghdr *nlh;
struct pps_netlink_msg *nlpps;
-
int cmd, source, id;
wait_queue_head_t *queue;
unsigned long timeout;
-
int ret;
while ((skb = skb_dequeue(&sk->sk_receive_queue)) != NULL) {
@@ -108,7 +106,7 @@ static void pps_nl_data_ready(struct soc
source = nlpps->source;
switch (cmd) {
- case PPS_CREATE : {
+ case PPS_CREATE:
dbg("PPS_CREATE: source %d", source);
/* Check if the requested source is allocated */
@@ -120,20 +118,16 @@ static void pps_nl_data_ready(struct soc
nlpps->source = ret;
nlpps->ret = 0;
-
break;
- }
- case PPS_DESTROY : {
+ case PPS_DESTROY:
dbg("PPS_DESTROY: source %d", source);
/* Nothing to do here! Just answer ok... */
nlpps->ret = 0;
-
break;
- }
- case PPS_SETPARMS : {
+ case PPS_SETPARMS:
dbg("PPS_SETPARMS: source %d", source);
/* Check the capabilities */
@@ -148,17 +142,17 @@ static void pps_nl_data_ready(struct soc
nlpps->ret = ret;
break;
}
- if ((nlpps->params.mode&~pps_source[source].info->mode) != 0) {
+ if ((nlpps->params.mode & ~pps_source[source].info->mode) != 0) {
dbg("unsupported capabilities");
nlpps->ret = -EINVAL;
break;
}
- if ((nlpps->params.mode&(PPS_CAPTUREASSERT|PPS_CAPTURECLEAR)) == 0) {
+ if ((nlpps->params.mode & (PPS_CAPTUREASSERT|PPS_CAPTURECLEAR)) == 0) {
dbg("capture mode unspecified");
nlpps->ret = -EINVAL;
break;
}
- if ((nlpps->params.mode&(PPS_TSFMT_TSPEC|PPS_TSFMT_NTPFP)) == 0) {
+ if ((nlpps->params.mode & (PPS_TSFMT_TSPEC|PPS_TSFMT_NTPFP)) == 0) {
/* section 3.3 of RFC 2783 interpreted */
dbg("time format unspecified");
nlpps->params.mode |= PPS_TSFMT_TSPEC;
@@ -173,11 +167,9 @@ static void pps_nl_data_ready(struct soc
pps_source[source].params.api_version = PPS_API_VERS;
nlpps->ret = 0;
-
break;
- }
- case PPS_GETPARMS : {
+ case PPS_GETPARMS:
dbg("PPS_GETPARMS: source %d", source);
/* Sanity checks */
@@ -189,11 +181,9 @@ static void pps_nl_data_ready(struct soc
nlpps->params = pps_source[source].params;
nlpps->ret = 0;
-
break;
- }
- case PPS_GETCAP : {
+ case PPS_GETCAP:
dbg("PPS_GETCAP: source %d", source);
/* Sanity checks */
@@ -205,11 +195,9 @@ static void pps_nl_data_ready(struct soc
nlpps->mode = pps_source[source].info->mode;
nlpps->ret = 0;
-
break;
- }
- case PPS_FETCH : {
+ case PPS_FETCH:
dbg("PPS_FETCH: source %d", source);
queue = &pps_source[source].queue;
@@ -219,7 +207,7 @@ static void pps_nl_data_ready(struct soc
nlpps->ret = ret;
break;
}
- if ((nlpps->tsformat != PPS_TSFMT_TSPEC) != 0 ) {
+ if (nlpps->tsformat != PPS_TSFMT_TSPEC) {
dbg("unsupported time format");
nlpps->ret = -EINVAL;
break;
@@ -227,8 +215,9 @@ static void pps_nl_data_ready(struct soc
/* Manage the timeout */
if (nlpps->timeout.tv_sec != -1) {
- timeout = nlpps->timeout.tv_sec*HZ;
- timeout += nlpps->timeout.tv_nsec/(1000000000/HZ);
+ timeout = nlpps->timeout.tv_sec * HZ;
+ timeout += nlpps->timeout.tv_nsec/
+ (NSEC_PER_SEC/HZ);
if (timeout != 0) {
timeout = interruptible_sleep_on_timeout(queue, timeout);
@@ -238,8 +227,7 @@ static void pps_nl_data_ready(struct soc
break;
}
}
- }
- else
+ } else
interruptible_sleep_on(queue);
/* Return the fetched timestamp */
@@ -250,19 +238,15 @@ static void pps_nl_data_ready(struct soc
nlpps->info.current_mode = pps_source[source].current_mode;
nlpps->ret = 0;
-
break;
- }
- case PPS_KC_BIND : {
+ case PPS_KC_BIND:
dbg("PPS_KC_BIND: source %d", source);
/* Feature currently not supported */
nlpps->ret = -EOPNOTSUPP;
-
break;
- }
- case PPS_FIND_SRC : {
+ case PPS_FIND_SRC:
dbg("PPS_FIND_SRC: source %d", source);
source = pps_find_source(source);
if (source < 0) {
@@ -278,11 +262,9 @@ static void pps_nl_data_ready(struct soc
strncpy(nlpps->path, pps_source[source].info->path,
PPS_MAX_NAME_LEN);
nlpps->ret = 0;
-
break;
- }
- case PPS_FIND_PATH : {
+ case PPS_FIND_PATH:
dbg("PPS_FIND_PATH: source %s", nlpps->path);
source = pps_find_path(nlpps->path);
if (source < 0) {
@@ -298,17 +280,14 @@ static void pps_nl_data_ready(struct soc
strncpy(nlpps->path, pps_source[source].info->path,
PPS_MAX_NAME_LEN);
nlpps->ret = 0;
-
break;
- }
- default : {
+ default:
/* Unknow command */
dbg("unknow command %d", nlpps->cmd);
nlpps->ret = -EINVAL;
}
- }
/* Send an answer to the userland */
id = NETLINK_CB(skb).pid;
@@ -336,16 +315,13 @@ static int __init pps_init(void)
int ret;
nl_sk = netlink_kernel_create(NETLINK_PPSAPI, 0,
- pps_nl_data_ready, THIS_MODULE);
+ pps_nl_data_ready, NULL, THIS_MODULE);
if (nl_sk == NULL) {
err("unable to create netlink kernel socket");
return -EBUSY;
}
dbg("netlink protocol %d created", NETLINK_PPSAPI);
- /* Init the main struct */
- memset(pps_source, 0, sizeof(struct pps_s)*PPS_MAX_SOURCES);
-
/* Register to sysfs */
ret = pps_sysfs_register();
if (ret < 0) {
@@ -354,11 +330,12 @@ static int __init pps_init(void)
}
info("LinuxPPS API ver. %d registered", PPS_API_VERS);
- info("Software ver. %s - Copyright 2005-2007 Rodolfo Giometti <[email protected]>", PPS_VERSION);
+ info("Software ver. %s - Copyright 2005-2007 Rodolfo Giometti "
+ "<[email protected]>", PPS_VERSION);
- return 0;
+ return 0;
-pps_sysfs_register_error :
+pps_sysfs_register_error:
sock_release(nl_sk->sk_socket);
diff -puN drivers/pps/sysfs.c~linuxpps-pulse-per-second-support-for-linux-fix drivers/pps/sysfs.c
--- a/drivers/pps/sysfs.c~linuxpps-pulse-per-second-support-for-linux-fix
+++ a/drivers/pps/sysfs.c
@@ -112,7 +112,8 @@ static struct class pps_class = {
/* ----- Public functions --------------------------------------------- */
-void pps_sysfs_remove_source_entry(struct pps_source_info_s *info) {
+void pps_sysfs_remove_source_entry(struct pps_source_info_s *info)
+{
int i;
/* Sanity checks */
@@ -136,7 +137,8 @@ void pps_sysfs_remove_source_entry(struc
class_device_unregister(&info->class_dev);
}
-int pps_sysfs_create_source_entry(struct pps_source_info_s *info, int id) {
+int pps_sysfs_create_source_entry(struct pps_source_info_s *info, int id)
+{
char buf[32];
int i, ret;
@@ -161,14 +163,14 @@ int pps_sysfs_create_source_entry(struct
/* Create info files */
/* Create file "assert" and "clear" according to source capability */
- if (info->mode&PPS_CAPTUREASSERT) {
+ if (info->mode & PPS_CAPTUREASSERT) {
ret = class_device_create_file(&info->class_dev,
&pps_class_device_attributes[0]);
i = 0;
if (unlikely(ret))
goto error_class_device_create_file;
}
- if (info->mode&PPS_CAPTURECLEAR) {
+ if (info->mode & PPS_CAPTURECLEAR) {
ret = class_device_create_file(&info->class_dev,
&pps_class_device_attributes[1]);
i = 1;
@@ -185,7 +187,7 @@ int pps_sysfs_create_source_entry(struct
return 0;
-error_class_device_create_file :
+error_class_device_create_file:
while (--i >= 0)
class_device_remove_file(&info->class_dev,
&pps_class_device_attributes[i]);
@@ -193,7 +195,7 @@ error_class_device_create_file :
class_device_unregister(&info->class_dev);
/* Here the release() method was already called */
-error_class_device_register :
+error_class_device_register:
return ret;
}
diff -puN drivers/serial/8250.c~linuxpps-pulse-per-second-support-for-linux-fix drivers/serial/8250.c
--- a/drivers/serial/8250.c~linuxpps-pulse-per-second-support-for-linux-fix
+++ a/drivers/serial/8250.c
@@ -2820,7 +2820,6 @@ void serial8250_unregister_port(int line
struct uart_8250_port *uart = &serial8250_ports[line];
mutex_lock(&serial_mutex);
-
uart_remove_one_port(&serial8250_reg, &uart->port);
if (serial8250_isa_devs) {
uart->port.flags &= ~UPF_BOOT_AUTOCONF;
diff -puN include/linux/pps.h~linuxpps-pulse-per-second-support-for-linux-fix include/linux/pps.h
--- a/include/linux/pps.h~linuxpps-pulse-per-second-support-for-linux-fix
+++ a/include/linux/pps.h
@@ -185,7 +185,8 @@ extern spinlock_t pps_lock;
/* --- Global functions ---------------------------------------------------- */
-static inline int pps_is_allocated(int source) {
+static inline int pps_is_allocated(int source)
+{
return pps_source[source].info != NULL;
}
@@ -193,11 +194,13 @@ static inline int pps_is_allocated(int s
/* --- Exported functions -------------------------------------------------- */
-extern int pps_register_source(struct pps_source_info_s *info, int default_params, int try_id);
+extern int pps_register_source(struct pps_source_info_s *info,
+ int default_params, int try_id);
extern void pps_unregister_source(struct pps_source_info_s *info);
extern void pps_event(int source, int event, void *data);
-extern int pps_sysfs_create_source_entry(struct pps_source_info_s *info, int id);
+extern int pps_sysfs_create_source_entry(struct pps_source_info_s *info,
+ int id);
extern void pps_sysfs_remove_source_entry(struct pps_source_info_s *info);
extern int pps_sysfs_register(void);
extern void pps_sysfs_unregister(void);
_
On Thu, 10 May 2007 00:27:40 -0700 Andrew Morton <[email protected]> wrote:
> - Can we get rid of the private dbg, err and info macros? Surely there are
> generic ones somewhere.
i386 allmodconfig:
In file included from drivers/usb/misc/uss720.c:48:
include/linux/usb.h:1505:1: warning: "dbg" redefined
In file included from include/linux/parport.h:108,
from drivers/usb/misc/uss720.c:46:
include/linux/pps.h:139:1: warning: this is the location of the previous definition
In file included from drivers/usb/misc/uss720.c:48:
include/linux/usb.h:1511:1: warning: "err" redefined
In file included from include/linux/parport.h:108,
from drivers/usb/misc/uss720.c:46:
include/linux/pps.h:145:1: warning: this is the location of the previous definition
In file included from drivers/usb/misc/uss720.c:48:
include/linux/usb.h:1513:1: warning: "info" redefined
In file included from include/linux/parport.h:108,
from drivers/usb/misc/uss720.c:46:
include/linux/pps.h:147:1: warning: this is the location of the previous definition
On Thu, May 10, 2007 at 12:27:40AM -0700, Andrew Morton wrote:
>
> Please check with Dave Miller that this:
>
> #define NETLINK_PPSAPI 20
>
> reservation is OK.
Hello, as you can see here Andrew Morton asked to me to check with you
about NETLINK_PPSAPI reservation.
Please, let me know if it's ok.
Thanks,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
From: Rodolfo Giometti <[email protected]>
Date: Thu, 10 May 2007 12:58:37 +0200
> On Thu, May 10, 2007 at 12:27:40AM -0700, Andrew Morton wrote:
> >
> > Please check with Dave Miller that this:
> >
> > #define NETLINK_PPSAPI 20
> >
> > reservation is OK.
>
> Hello, as you can see here Andrew Morton asked to me to check with you
> about NETLINK_PPSAPI reservation.
>
> Please, let me know if it's ok.
It's not OK, please use the generic netlink interface and as
such you will not need to allocate any numbers at all.
Documentation/networking/generic_netlink.txt gives a link
to some infomration on this topic.
On Thu, May 10, 2007 at 04:01:52AM -0700, David Miller wrote:
>
> It's not OK, please use the generic netlink interface and as
> such you will not need to allocate any numbers at all.
>
> Documentation/networking/generic_netlink.txt gives a link
> to some infomration on this topic.
If I well understand doing like this means that I have to modify also
userland API, is that true?
I know that you are forcing in using this new interface for new kernel
projects, but if I have to change my code I need also change NTPD
related code and this is frustrating. I have to interact with both
kernel developers and NTPD ones... :)
It could be acceptable let me use id "20" (or just another number) to
allow Andrew Morton and other LinuxPPS users to test this new support?
Please, consider that this is not a new project, it was developed
since 2005 when this new interface was not available.
Thanks for your attention,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
From: Rodolfo Giometti <[email protected]>
Date: Thu, 10 May 2007 13:45:03 +0200
> On Thu, May 10, 2007 at 04:01:52AM -0700, David Miller wrote:
> >
> > It's not OK, please use the generic netlink interface and as
> > such you will not need to allocate any numbers at all.
> >
> > Documentation/networking/generic_netlink.txt gives a link
> > to some infomration on this topic.
>
> If I well understand doing like this means that I have to modify also
> userland API, is that true?
Yes.
> I know that you are forcing in using this new interface for new kernel
> projects, but if I have to change my code I need also change NTPD
> related code and this is frustrating. I have to interact with both
> kernel developers and NTPD ones... :)
>
> It could be acceptable let me use id "20" (or just another number) to
> allow Andrew Morton and other LinuxPPS users to test this new support?
> Please, consider that this is not a new project, it was developed
> since 2005 when this new interface was not available.
Being a 2005 project means only that you've been out of tree for
nearly 2 years.
Sorry, we are not allocating a netlink IDs for folks, and we're doing
it exactly because generic netlink avoids all the fixed numbering API
issues.
BTW, please remove the linuxpps list from the CC: for future postings
in this thread, it bounces every one of my emails back because it only
allows postings from subscribers.
Thanks a lot.
On Thu, May 10, 2007 at 04:01:52AM -0700, David Miller wrote:
>
> It's not OK, please use the generic netlink interface and as
> such you will not need to allocate any numbers at all.
>
> Documentation/networking/generic_netlink.txt gives a link
> to some infomration on this topic.
Where can I find some infos about userland programming _without_ using
the libnl library?
There are something similar to the magic command:
ret = socket(PF_NETLINK, SOCK_RAW, NETLINK_PPSAPI);
in this new netlink API?
Thanks for your help,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
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, [email protected] 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
> <[email protected]>.
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 <[email protected]>
> 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 <linux/pps.h>
> +#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
----
On Fri, 11 May 2007 23:55:37 +0200
Rodolfo Giometti <[email protected]> wrote:
> 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! :)
Well I suppose I could toss it in there for a bit of review-and-test. But
I'll need to drop it again because we do need to split this patch into the series
of patches, please.
You should do this earlier rather than later because it improves reviewability.
> > - 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?
No, it needs to be a proper release function, like all the other ones
around the place.
This comes up again and again and again and I recently asked Greg to direct
me to (or to write) suitable documentation, and I think he did, but I lost
it. Greg, can you remind us please?
> > 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).
We don't use typedefs in-kernel. Please convert the code to use `struct
ntp_fp' everywhere.
For RFC compatibility to userspace you can do
#ifndef __KERNEL__
typedef struct ntp_fp ntp_fp_t;
...
#endif
> > - 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 <[email protected]>
> > 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.
Your answer has no relationship to my question.
The problem here is that under a 64-bit kernel we require that applications
which use this structure definition work correctly when they are compiled
to generate 32-bit code and when they are compiled to generate 64-bit code.
Furthermore we should aim to to have to code work correctly across
different version of the compiler, and when different compiler options are
used, and when altogether different compilers are used.
It is not clear to me that your definition is sufficiently defensive
against _any_ of these things.
> > - 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.
You misunderstand. I'm not saying "remove the callsites". I'm saying
"remove the definitions".
Because we already have things like pr_debug() and pr_info(), so new code
should use those rather than reinventing them.
Plus, we already have at least 52 different implementations of "dbg" in the
tree and your 53rd one didn't compile because it clashed with someone
else's. This is the compiler sending us a message: "use the exiting
infrastructure". If that infrastructure is insufficient then let's
improve it.
On Fri, May 11, 2007 at 11:17:11PM -0700, Andrew Morton wrote:
> On Fri, 11 May 2007 23:55:37 +0200
> Rodolfo Giometti <[email protected]> wrote:
>
> > 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! :)
>
> Well I suppose I could toss it in there for a bit of review-and-test. But
> I'll need to drop it again because we do need to split this patch into the series
> of patches, please.
>
> You should do this earlier rather than later because it improves reviewability.
>
> > > - 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?
>
> No, it needs to be a proper release function, like all the other ones
> around the place.
>
> This comes up again and again and again and I recently asked Greg to direct
> me to (or to write) suitable documentation, and I think he did, but I lost
> it. Greg, can you remind us please?
I need to put it in some permanent place, but basically the problem is
that if you need to shut the kernel up by using an empty function for a
release, then you are not understanding why the kernel was trying to
warn you in the first place :(
You need to free your memory for the class_device in the release
function, you can not have a static structure, or rely on some other
reference count to handle the cleanup properly.
Also note that 'struct class_device' is going away and you should use
'struct device' instead. That too needs to be documented better, and is
on my list of things to do...
thanks,
greg k-h