Hello,
here my new version of LinuxPPS with a new syscalls API.
Please take a look at this new patch and report any
suggestions/modifications so I can prepare a proper patch for kernel
inclusion.
Thanks in advance,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Tue, 2007-06-26 at 12:06 +0200, Rodolfo Giometti wrote:
> Hello,
>
> here my new version of LinuxPPS with a new syscalls API.
>
> Please take a look at this new patch and report any
> suggestions/modifications so I can prepare a proper patch for kernel
> inclusion.
Your syscalls blindly dereference userspace pointers instead of using
copy_{to,from} user.
Why did you split all your syscalls into two functions?
s/__FUNCTION__/__func__/
s/antennas/antennae/
You seem to have added debugging messages mentioning 'serial8250' into
serial_core.h
You added <linux/pps.h> with #ifdef __KERNEL__ in it, but didn't export
it to userspace. Why?
Your structures for userspace communication look OK -- I don't think you
need special 32/64 compatibility for them. You do need it for the
'struct timespec' in sys_time_pps_fetch() though.
Must we have the ioctl-like interface to sys_time_pps_cmd()? If the
second argument is always 'struct pps_source_data_s *', why does the
syscall pretend it's 'void *'?
--
dwmw2
On Tue, Jun 26, 2007 at 11:57:07AM +0100, David Woodhouse wrote:
>
> Your syscalls blindly dereference userspace pointers instead of using
> copy_{to,from} user.
I use access_ok() to test userspace addresses. It should be ok,
shouldn't it?
> Why did you split all your syscalls into two functions?
>
> s/__FUNCTION__/__func__/
Just for an easy management of mutex locking.
> s/antennas/antennae/
Done. However I found other files in the kernel code with the same
error... ;)
> You seem to have added debugging messages mentioning 'serial8250' into
> serial_core.h
Yes! Fixed.
> You added <linux/pps.h> with #ifdef __KERNEL__ in it, but didn't export
> it to userspace. Why?
This file is called by timepps.h who exports the userland data.
> Your structures for userspace communication look OK -- I don't think you
> need special 32/64 compatibility for them. You do need it for the
> 'struct timespec' in sys_time_pps_fetch() though.
Mmm... can you please explain a bit what do you mean? Maybe just a
link...
> Must we have the ioctl-like interface to sys_time_pps_cmd()? If the
It seems to me stronger then other solutions...
> second argument is always 'struct pps_source_data_s *', why does the
> syscall pretend it's 'void *'?
Just to keep sys_time_pps_cmd() generic for future new commands.
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 Tue, 2007-06-26 at 19:06 +0200, Rodolfo Giometti wrote:
> On Tue, Jun 26, 2007 at 11:57:07AM +0100, David Woodhouse wrote:
> >
> > Your syscalls blindly dereference userspace pointers instead of using
> > copy_{to,from} user.
>
> I use access_ok() to test userspace addresses. It should be ok,
> shouldn't it?
No; it's racy. You must use copy_from_user() and copy_to_user().
> > Why did you split all your syscalls into two functions?
> >
> > s/__FUNCTION__/__func__/
>
> Just for an easy management of mutex locking.
That sounds like you're scared of using goto. Don't be :)
> > s/antennas/antennae/
>
> Done. However I found other files in the kernel code with the same
> error... ;)
This is often true of anything which gets pointed out during review. :)
> > You seem to have added debugging messages mentioning 'serial8250' into
> > serial_core.h
>
> Yes! Fixed.
>
> > You added <linux/pps.h> with #ifdef __KERNEL__ in it, but didn't export
> > it to userspace. Why?
>
> This file is called by timepps.h who exports the userland data.
I don't see this timepps.h of which you speak. If it's a _userspace_
file, it cannot include <linux/pps.h> unless you actually add
<linux/pps.h> to the list of files which are exported.
Run 'make headers_install' and observe that there is no file
usr/include/linux/pps.h -- so there would be no /usr/include/linux/pps.h
generated from your kernel tree. You need to add 'unifdef-y += pps.h' to
include/linux/Kbuild for that to happen.
> > Your structures for userspace communication look OK -- I don't think you
> > need special 32/64 compatibility for them. You do need it for the
> > 'struct timespec' in sys_time_pps_fetch() though.
>
> Mmm... can you please explain a bit what do you mean? Maybe just a
> link...
64-bit kernels can run 32-bit userspace programs. But some structures
come out _differently_ between 32-bit and 64-bit compilation, so the
system call needs a special 'compat' handler instead of just running the
normal 64-bit system call.
The 'struct timespec' is one structure which is sometimes different for
32-bit vs. 64-bit, so any system call taking a 'struct timespec' must
have a separate compat_sys_xxxx() to handle that. See something like
compat_sys_clock_settime() in kernel/compat.c for an example (but don't
use set_fs() like it does; just see how it handles the compat_timespec).
> > Must we have the ioctl-like interface to sys_time_pps_cmd()? If the
>
> It seems to me stronger then other solutions...
>
> > second argument is always 'struct pps_source_data_s *', why does the
> > syscall pretend it's 'void *'?
>
> Just to keep sys_time_pps_cmd() generic for future new commands.
Hm. I'll let other people complain at you about that :)
--
dwmw2
On Tue, Jun 26, 2007 at 06:38:40PM +0100, David Woodhouse wrote:
> That sounds like you're scared of using goto. Don't be :)
But it's not wrong... should I change it or not?
> I don't see this timepps.h of which you speak. If it's a _userspace_
> file, it cannot include <linux/pps.h> unless you actually add
> <linux/pps.h> to the list of files which are exported.
>
> Run 'make headers_install' and observe that there is no file
> usr/include/linux/pps.h -- so there would be no /usr/include/linux/pps.h
> generated from your kernel tree. You need to add 'unifdef-y += pps.h' to
> include/linux/Kbuild for that to happen.
Is this right?
diff --git a/include/linux/Kbuild b/include/linux/Kbuild
index 94cc04a..87f1e2c 100644
--- a/include/linux/Kbuild
+++ b/include/linux/Kbuild
@@ -290,6 +290,7 @@ unifdef-y += pmu.h
unifdef-y += poll.h
unifdef-y += ppp_defs.h
unifdef-y += ppp-comp.h
+unifdef-y += pps.h
unifdef-y += ptrace.h
unifdef-y += qnx4_fs.h
unifdef-y += quota.h
> 64-bit kernels can run 32-bit userspace programs. But some structures
> come out _differently_ between 32-bit and 64-bit compilation, so the
> system call needs a special 'compat' handler instead of just running the
> normal 64-bit system call.
>
> The 'struct timespec' is one structure which is sometimes different for
> 32-bit vs. 64-bit, so any system call taking a 'struct timespec' must
> have a separate compat_sys_xxxx() to handle that. See something like
> compat_sys_clock_settime() in kernel/compat.c for an example (but don't
> use set_fs() like it does; just see how it handles the compat_timespec).
Ok, I'll take a look at it.
Thanks a lot,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Tue, 2007-06-26 at 20:13 +0200, Rodolfo Giometti wrote:
> On Tue, Jun 26, 2007 at 06:38:40PM +0100, David Woodhouse wrote:
>
> > That sounds like you're scared of using goto. Don't be :)
>
> But it's not wrong... should I change it or not?
I would suggest changing it. It makes it more obvious to the casual that
there are no other callers of the 'internal' functions. It's a matter of
taste though; if you feel _very_ strongly about it then feel free to
ignore me.
> Is this right?
> +unifdef-y += pps.h
Yes, that looks right. As a matter of course you should also run 'make
headers_check' before submitting your new patch, to make sure everything
is sane. That'll tell you if your exported pps.h is trying to include
other header files which aren't also exported.
--
dwmw2
David Woodhouse wrote:
> On Tue, 2007-06-26 at 19:06 +0200, Rodolfo Giometti wrote:
>> On Tue, Jun 26, 2007 at 11:57:07AM +0100, David Woodhouse wrote:
>>> Your syscalls blindly dereference userspace pointers instead of using
>>> copy_{to,from} user.
>> I use access_ok() to test userspace addresses. It should be ok,
>> shouldn't it?
>
> No; it's racy. You must use copy_from_user() and copy_to_user().
Not only is it racy, but it doesn't even do all of the checks that
copy_to/from_user does. access_ok only validates that the region given
is potentially valid, not that it actually is. Using access_ok only
allows you to use __copy_to/from_user instead, which skips the same
checks that access_ok does - not worth it unless you do repeated copies
to/from the same region of memory.
--
Robert Hancock Saskatoon, SK, Canada
To email, remove "nospam" from [email protected]
Home Page: http://www.roberthancock.com/
On Tue, Jun 26, 2007 at 06:38:40PM +0100, David Woodhouse wrote:
>
> 64-bit kernels can run 32-bit userspace programs. But some structures
> come out _differently_ between 32-bit and 64-bit compilation, so the
> system call needs a special 'compat' handler instead of just running the
> normal 64-bit system call.
>
> The 'struct timespec' is one structure which is sometimes different for
> 32-bit vs. 64-bit, so any system call taking a 'struct timespec' must
> have a separate compat_sys_xxxx() to handle that. See something like
> compat_sys_clock_settime() in kernel/compat.c for an example (but don't
> use set_fs() like it does; just see how it handles the compat_timespec).
Did you mean something like this?
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index befe292..3e401e5 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -26,6 +26,7 @@
#include <linux/init.h>
#include <linux/linkage.h>
#include <linux/sched.h>
+#include <linux/compat.h>
#include <linux/pps.h>
#include <asm/uaccess.h>
@@ -284,9 +285,15 @@ sys_time_pps_getcap_exit:
return ret;
}
+#ifdef CONFIG_COMPAT
asmlinkage long sys_time_pps_fetch(int source, const int tsformat,
- struct pps_info __user *info,
- const struct timespec __user *timeout)
+ struct pps_info __user *info,
+ const struct compat_timespec __user *timeout)
+#else
+asmlinkage long sys_time_pps_fetch(int source, const int tsformat,
+ struct pps_info __user *info,
+ const struct timespec __user *timeout)
+#endif
{
unsigned long ticks;
struct pps_info pi;
@@ -318,7 +325,11 @@ asmlinkage long sys_time_pps_fetch(int source, const int ts
/* Manage the timeout */
if (timeout) {
+#ifdef CONFIG_COMPAT
+ ret = get_compat_timespec(&to, timeout);
+#else
ret = copy_from_user(&to, timeout, sizeof(struct timespec));
+#endif
if (ret)
goto sys_time_pps_fetch_exit;
if (to.tv_sec != -1) {
Ciao,
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, 2007-06-27 at 12:14 +0200, Rodolfo Giometti wrote:
> On Tue, Jun 26, 2007 at 06:38:40PM +0100, David Woodhouse wrote:
> >
> > 64-bit kernels can run 32-bit userspace programs. But some structures
> > come out _differently_ between 32-bit and 64-bit compilation, so the
> > system call needs a special 'compat' handler instead of just running the
> > normal 64-bit system call.
> >
> > The 'struct timespec' is one structure which is sometimes different for
> > 32-bit vs. 64-bit, so any system call taking a 'struct timespec' must
> > have a separate compat_sys_xxxx() to handle that. See something like
> > compat_sys_clock_settime() in kernel/compat.c for an example (but don't
> > use set_fs() like it does; just see how it handles the compat_timespec).
>
> Did you mean something like this?
How will 64-bit system calls work if you do it like that? You need to
provide _both_ sys_time_pps_fetch() and compat_sys_time_pps_fetch().
--
dwmw2
On Wed, Jun 27, 2007 at 11:18:30AM +0100, David Woodhouse wrote:
> On Wed, 2007-06-27 at 12:14 +0200, Rodolfo Giometti wrote:
> > On Tue, Jun 26, 2007 at 06:38:40PM +0100, David Woodhouse wrote:
> > >
> > > 64-bit kernels can run 32-bit userspace programs. But some structures
> > > come out _differently_ between 32-bit and 64-bit compilation, so the
> > > system call needs a special 'compat' handler instead of just running the
> > > normal 64-bit system call.
> > >
> > > The 'struct timespec' is one structure which is sometimes different for
> > > 32-bit vs. 64-bit, so any system call taking a 'struct timespec' must
> > > have a separate compat_sys_xxxx() to handle that. See something like
> > > compat_sys_clock_settime() in kernel/compat.c for an example (but don't
> > > use set_fs() like it does; just see how it handles the compat_timespec).
> >
> > Did you mean something like this?
>
> How will 64-bit system calls work if you do it like that? You need to
> provide _both_ sys_time_pps_fetch() and compat_sys_time_pps_fetch().
Sorry, I'm new to this 32/64 bits issues...
Now is it correct?
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index befe292..b9df17b 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -26,6 +26,7 @@
#include <linux/init.h>
#include <linux/linkage.h>
#include <linux/sched.h>
+#include <linux/compat.h>
#include <linux/pps.h>
#include <asm/uaccess.h>
@@ -362,6 +363,22 @@ sys_time_pps_fetch_exit:
return ret;
}
+#ifdef CONFIG_COMPAT
+asmlinkage long compat_sys_time_pps_fetch(int source, const int tsformat,
+ struct pps_info __user *info,
+ const struct compat_timespec __user *timeout)
+{
+ int ret;
+ struct timespec to;
+
+ ret = get_compat_timespec(&to, timeout);
+ if (ret)
+ return -EFAULT;
+
+ return sys_time_pps_fetch(source, tsformat, info, &to);
+}
+#endif
+
/*
* Module staff
*/
Since I have no way to test this code maybe is better add no function
at all and simply using a warning message if someone try compiling
this code with CONFIG_COMPAT enabled...
Ciao,
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, 2007-06-27 at 14:58 +0200, Rodolfo Giometti wrote:
> On Wed, Jun 27, 2007 at 11:18:30AM +0100, David Woodhouse wrote:
> > On Wed, 2007-06-27 at 12:14 +0200, Rodolfo Giometti wrote:
> > > On Tue, Jun 26, 2007 at 06:38:40PM +0100, David Woodhouse wrote:
> > > >
> > > > 64-bit kernels can run 32-bit userspace programs. But some structures
> > > > come out _differently_ between 32-bit and 64-bit compilation, so the
> > > > system call needs a special 'compat' handler instead of just running the
> > > > normal 64-bit system call.
> > > >
> > > > The 'struct timespec' is one structure which is sometimes different for
> > > > 32-bit vs. 64-bit, so any system call taking a 'struct timespec' must
> > > > have a separate compat_sys_xxxx() to handle that. See something like
> > > > compat_sys_clock_settime() in kernel/compat.c for an example (but don't
> > > > use set_fs() like it does; just see how it handles the compat_timespec).
> > >
> > > Did you mean something like this?
> >
> > How will 64-bit system calls work if you do it like that? You need to
> > provide _both_ sys_time_pps_fetch() and compat_sys_time_pps_fetch().
>
> Sorry, I'm new to this 32/64 bits issues...
>
> Now is it correct?
No, because you're passing a _kernel_ pointer to sys_time_pps_fetch()
where it expects a userspace pointer. Use compat_alloc_user_space() to
find somewhere to put it in user space, instead. Or change your internal
__sys_time_pps_fetch() function to take a number of ticks instead of a
pointer to a timespec, then call that directly with appropriate
arguments, from both the normal and compat syscall routines.
>
> Since I have no way to test this code maybe is better add no function
> at all and simply using a warning message if someone try compiling
> this code with CONFIG_COMPAT enabled...
No. The fact that you cannot test it is no excuse for submitting
something which is _known_ to be broken. Once you have it to the point
where a casual observer can't point out errors, then people can test it
for you.
--
dwmw2
On Wed, Jun 27, 2007 at 05:11:00PM +0100, David Woodhouse wrote:
> No, because you're passing a _kernel_ pointer to sys_time_pps_fetch()
> where it expects a userspace pointer. Use compat_alloc_user_space() to
> find somewhere to put it in user space, instead. Or change your internal
> __sys_time_pps_fetch() function to take a number of ticks instead of a
> pointer to a timespec, then call that directly with appropriate
> arguments, from both the normal and compat syscall routines.
Ok. Please see the attached patch.
Thanks a lot,
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, 2007-06-27 at 19:45 +0200, Rodolfo Giometti wrote:
> Ok. Please see the attached patch.
Looks better. All I can find to complain about is the fact that you
return whatever copy_from_user() returns. Don't -- that's the number of
bytes left to copy. It should be if (copy_from_user(..)) return -EFAULT;
--
dwmw2
On Wed, Jun 27, 2007 at 06:49:48PM +0100, David Woodhouse wrote:
>
> Looks better. All I can find to complain about is the fact that you
> return whatever copy_from_user() returns. Don't -- that's the number of
> bytes left to copy. It should be if (copy_from_user(..)) return -EFAULT;
Ok, I'll fix it.
Just last question: I still don't well understand where I should
declare the new compat_sys_time_pps_fetch() syscall... it's
automagically defined by the system when CONFIG_COMPAT is enabled? :-o
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
On Thu, 2007-06-28 at 00:46 +0200, Rodolfo Giometti wrote:
> Just last question: I still don't well understand where I should
> declare the new compat_sys_time_pps_fetch() syscall... it's
> automagically defined by the system when CONFIG_COMPAT is enabled? :-o
It isn't used on i386. On a 64-bit architecture, you need to put
compat_sys_time_pps_fetch() into the syscall table for 32-bit
processes.
On PowerPC you do this by using COMPAT_SYS_SPU(time_pps_fetch) instead
of SYSCALL_SPU(..) in include/asm-powerpc/systbl.h. On x86_64 you'd put
it into the ia32_sys_call_table in arch/x86_64/ia32/ia32entry.S
--
dwmw2
On Thu, Jun 28, 2007 at 09:08:53AM +0100, David Woodhouse wrote:
> On Thu, 2007-06-28 at 00:46 +0200, Rodolfo Giometti wrote:
> > Just last question: I still don't well understand where I should
> > declare the new compat_sys_time_pps_fetch() syscall... it's
> > automagically defined by the system when CONFIG_COMPAT is enabled? :-o
>
> It isn't used on i386. On a 64-bit architecture, you need to put
> compat_sys_time_pps_fetch() into the syscall table for 32-bit
> processes.
>
> On PowerPC you do this by using COMPAT_SYS_SPU(time_pps_fetch) instead
> of SYSCALL_SPU(..) in include/asm-powerpc/systbl.h. On x86_64 you'd put
> it into the ia32_sys_call_table in arch/x86_64/ia32/ia32entry.S
I see.
Do you think I should add these functions into my patch, even if I
cannot test it, or it's enought providing just the
compat_sys_time_pps_fetch() function?
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 Thu, 2007-06-28 at 10:15 +0200, Rodolfo Giometti wrote:
> Do you think I should add these functions into my patch, even if I
> cannot test it, or it's enought providing just the
> compat_sys_time_pps_fetch() function?
Probably best to put them in. That way, you make it easier for people to
test. Ideally, you should also provide a simple program that other
people can use to test it, preferably without needing hardware (or at
least just a nullmodem cable and another test program at the other end
of it).
--
dwmw2
On Thu, Jun 28, 2007 at 09:31:14AM +0100, David Woodhouse wrote:
> On Thu, 2007-06-28 at 10:15 +0200, Rodolfo Giometti wrote:
> > Do you think I should add these functions into my patch, even if I
> > cannot test it, or it's enought providing just the
> > compat_sys_time_pps_fetch() function?
>
> Probably best to put them in. That way, you make it easier for people to
Mmm... so I should provide new syscalls for _all_
architectures... gulp! :)
It could be acceptable, just for the first release, to provide the
support for x86 only?
In this manner we can have a first release of LinuxPPS in the main
line just for x86 and then me, or other people, may add support for
the several supported architectures.
> test. Ideally, you should also provide a simple program that other
> people can use to test it, preferably without needing hardware (or at
> least just a nullmodem cable and another test program at the other end
> of it).
I already have a basic testing program... I can add it into
Documentation/pps directory, can't I?
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Thu, 2007-06-28 at 10:40 +0200, Rodolfo Giometti wrote:
> On Thu, Jun 28, 2007 at 09:31:14AM +0100, David Woodhouse wrote:
> > On Thu, 2007-06-28 at 10:15 +0200, Rodolfo Giometti wrote:
> > > Do you think I should add these functions into my patch, even if I
> > > cannot test it, or it's enought providing just the
> > > compat_sys_time_pps_fetch() function?
> >
> > Probably best to put them in. That way, you make it easier for people to
>
> Mmm... so I should provide new syscalls for _all_
> architectures... gulp! :)
It's nice if you can do so, but I wouldn't suggest that you _have_ to.
I have to admit that I rarely bother actually wiring new system calls up
on anything but PowerPC to start with.
The important thing is that you've _considered_ the other architectures,
and the 32/64 compatibility implications. As long as the API of your new
system call is sensible and takes that kind of thing into account, it
should be fine.
Had you considered changing the API so that you don't need the
compatibility wrapper at all? Could you take an integer number of µS or
ms instead of a struct timespec?
--
dwmw2
On Thu, Jun 28, 2007 at 12:44:20PM +0100, David Woodhouse wrote:
>
> It's nice if you can do so, but I wouldn't suggest that you _have_ to.
> I have to admit that I rarely bother actually wiring new system calls up
> on anything but PowerPC to start with.
>
> The important thing is that you've _considered_ the other architectures,
> and the 32/64 compatibility implications. As long as the API of your new
> system call is sensible and takes that kind of thing into account, it
> should be fine.
Ok. :)
> Had you considered changing the API so that you don't need the
> compatibility wrapper at all? Could you take an integer number of ?S or
> ms instead of a struct timespec?
Not before now, but I followed the API specified into RFC 2783 who
specifies struct timespec...
Thanks for your suggestions! I'll send a new patch ASAP!
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 LinuxPPS patch.
What to do now for kernel inclusion? Should I provide several patches?
If so how should I divide them?
Thanks a lot,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Thu, 2007-06-28 at 18:14 +0200, Rodolfo Giometti wrote:
> here my new LinuxPPS patch.
>
> What to do now for kernel inclusion? Should I provide several patches?
> If so how should I divide them?
It doesn't apply to the current git tree, which has already had some new
system calls added.
--
dwmw2
On Fri, Jun 29, 2007 at 12:38:02PM +0100, David Woodhouse wrote:
>
> It doesn't apply to the current git tree, which has already had some new
> system calls added.
Ok, here the patch against latest git commit.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Fri, 2007-06-29 at 17:08 +0200, Rodolfo Giometti wrote:
> On Fri, Jun 29, 2007 at 12:38:02PM +0100, David Woodhouse wrote:
> >
> > It doesn't apply to the current git tree, which has already had some new
> > system calls added.
>
> Ok, here the patch against latest git commit.
CC fs/fcntl.o
In file included from include/linux/syscalls.h:69,
from fs/fcntl.c:8:
include/linux/pps.h:51: error: field ‘tspec’ has incomplete type
make[1]: *** [fs/fcntl.o] Error 1
--
dwmw2
On Fri, Jun 29, 2007 at 04:25:16PM +0100, David Woodhouse wrote:
> On Fri, 2007-06-29 at 17:08 +0200, Rodolfo Giometti wrote:
> > On Fri, Jun 29, 2007 at 12:38:02PM +0100, David Woodhouse wrote:
> > >
> > > It doesn't apply to the current git tree, which has already had some new
> > > system calls added.
> >
> > Ok, here the patch against latest git commit.
>
> CC fs/fcntl.o
> In file included from include/linux/syscalls.h:69,
> from fs/fcntl.c:8:
> include/linux/pps.h:51: error: field ???tspec??? has incomplete type
> make[1]: *** [fs/fcntl.o] Error 1
Gulp! =:-o
On my system I get:
giometti@zaigor:~/Projects/linuxpps/kernel$ touch fs/fcntl.c
giometti@zaigor:~/Projects/linuxpps/kernel$ make
CHK include/linux/version.h
CHK include/linux/utsrelease.h
CALL scripts/checksyscalls.sh
CHK include/linux/compile.h
CC fs/fcntl.o
LD fs/built-in.o
GEN .version
CHK include/linux/compile.h
UPD include/linux/compile.h
CC init/version.o
LD init/built-in.o
LD .tmp_vmlinux1
KSYM .tmp_kallsyms1.S
AS .tmp_kallsyms1.o
LD .tmp_vmlinux2
KSYM .tmp_kallsyms2.S
AS .tmp_kallsyms2.o
LD vmlinux
SYSMAP System.map
SYSMAP .tmp_System.map
MODPOST vmlinux
WARNING: arch/i386/kernel/built-in.o(.exit.text+0x18): Section mismatch: reference to .init.text: (after 'cache_remove_dev')
WARNING: kernel/built-in.o(.text+0x13826): Section mismatch: reference to .init.text: (between 'kthreadd' and 'init_waitqueue_head')
AS arch/i386/boot/setup.o
LD arch/i386/boot/setup
OBJCOPY arch/i386/boot/compressed/vmlinux.bin
GZIP arch/i386/boot/compressed/vmlinux.bin.gz
LD arch/i386/boot/compressed/piggy.o
LD arch/i386/boot/compressed/vmlinux
OBJCOPY arch/i386/boot/vmlinux.bin
BUILD arch/i386/boot/bzImage
Root device is (9, 0)
Boot sector 512 bytes.
Setup is 7057 bytes.
System is 1348 kB
Kernel: arch/i386/boot/bzImage is ready (#135)
Building modules, stage 2.
MODPOST 69 modules
How is that possible??? I just git pull the linux code... maybe you
have a bit older version?
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Fri, 2007-06-29 at 17:38 +0200, Rodolfo Giometti wrote:
> How is that possible??? I just git pull the linux code... maybe you
> have a bit older version?
<asm-i386/signal.h> includes <linux/time.h>, for some reason.
<asm-powerpc/signal.h> doesn't.
You shouldn't rely on <linux/time.h> being pulled in like that -- you
either need a forward declaration of struct timespec, or to include
<linux/time.h> for yourself from <linux/pps.h>
--
dwmw2
On Fri, 2007-06-29 at 17:08 +0200, Rodolfo Giometti wrote:
>
> +asmlinkage long sys_time_pps_fetch(int source, const int tsformat,
> + struct pps_info __user *info,
> + const struct timespec __user *timeout)
> +{
> + int ret;
> + struct timespec to;
> +
> + if (timeout) {
> + ret = copy_from_user(&to, timeout, sizeof(struct timespec));
> + if (ret)
> + return ret;
You missed one. This should be -EFAULT too. And there's not a huge
amount of point in keeping the access_ok() checks elsewhere, since
copy_to_user() does that for itself.
Oh, and I think you do need compat magic for 'struct pps_info' and
'struct pps_params' too -- there's a struct timespec hidden deep in
there, as well as 'unsigned long longpad[3]'.
Can you explain the 'union pps_timeu'? It seems very odd. How do we know
which member of the union should be used?
--
dwmw2
On Fri, Jun 29, 2007 at 04:41:33PM +0100, David Woodhouse wrote:
>
> <asm-i386/signal.h> includes <linux/time.h>, for some reason.
> <asm-powerpc/signal.h> doesn't.
>
> You shouldn't rely on <linux/time.h> being pulled in like that -- you
> either need a forward declaration of struct timespec, or to include
> <linux/time.h> for yourself from <linux/pps.h>
Can you please check if this patch resolve your problem?
diff --git a/include/linux/pps.h b/include/linux/pps.h
index 6b53864..fe8c645 100644
--- a/include/linux/pps.h
+++ b/include/linux/pps.h
@@ -33,6 +33,7 @@
#ifndef __KERNEL__
#include <asm/types.h>
+#include <linux/time.h>
#include <sys/time.h>
#endif
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 Fri, 2007-06-29 at 18:23 +0200, Rodolfo Giometti wrote:
> On Fri, Jun 29, 2007 at 04:41:33PM +0100, David Woodhouse wrote:
> >
> > <asm-i386/signal.h> includes <linux/time.h>, for some reason.
> > <asm-powerpc/signal.h> doesn't.
> >
> > You shouldn't rely on <linux/time.h> being pulled in like that -- you
> > either need a forward declaration of struct timespec, or to include
> > <linux/time.h> for yourself from <linux/pps.h>
>
> Can you please check if this patch resolve your problem?
>
> diff --git a/include/linux/pps.h b/include/linux/pps.h
> index 6b53864..fe8c645 100644
> --- a/include/linux/pps.h
> +++ b/include/linux/pps.h
> @@ -33,6 +33,7 @@
>
> #ifndef __KERNEL__
> #include <asm/types.h>
> +#include <linux/time.h>
> #include <sys/time.h>
> #endif
You'll need to put it in an #else case, not in #ifndef __KERNEL__.
--
dwmw2
On Fri, Jun 29, 2007 at 04:55:47PM +0100, David Woodhouse wrote:
> You missed one. This should be -EFAULT too. And there's not a huge
> amount of point in keeping the access_ok() checks elsewhere, since
> copy_to_user() does that for itself.
Ok, fixed.
> Oh, and I think you do need compat magic for 'struct pps_info' and
> 'struct pps_params' too -- there's a struct timespec hidden deep in
> there, as well as 'unsigned long longpad[3]'.
Gulp! Can you please give me some advices in order to solve also this
problem? Should I use some "ifdef CONFIG_COMPAT" into those
structures? :-o
> Can you explain the 'union pps_timeu'? It seems very odd. How do we know
> which member of the union should be used?
This union is defined by the RFC 2783... we can know which member of
the union should be used by using define PPS_TSFMT_TSPEC for variable
tsformat into function time_pps_fetch().
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Fri, Jun 29, 2007 at 05:23:28PM +0100, David Woodhouse wrote:
>
> You'll need to put it in an #else case, not in #ifndef __KERNEL__.
Sorry. :)
diff --git a/include/linux/pps.h b/include/linux/pps.h
index 6b53864..9e3af51 100644
--- a/include/linux/pps.h
+++ b/include/linux/pps.h
@@ -34,6 +34,8 @@
#ifndef __KERNEL__
#include <asm/types.h>
#include <sys/time.h>
+#else
+#include <linux/time.h>
#endif
#define PPS_API_VERS_2 2 /* LinuxPPS proposal, dated 2006-05 */
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Fri, 2007-06-29 at 18:36 +0200, Rodolfo Giometti wrote:
> On Fri, Jun 29, 2007 at 05:23:28PM +0100, David Woodhouse wrote:
> >
> > You'll need to put it in an #else case, not in #ifndef __KERNEL__.
>
> Sorry. :)
That matches what I built with earlier.
--
dwmw2
On Fri, 2007-06-29 at 18:34 +0200, Rodolfo Giometti wrote:
> Gulp! Can you please give me some advices in order to solve also this
> problem? Should I use some "ifdef CONFIG_COMPAT" into those
> structures? :-o
Remember you have to support _both_ 32-bit and 64-bit system calls. You
need to define struct compat_pps_info and struct compat_pps_params, and
you'll have to provide a compat wrapper for sys_time_pps_getparams() and
sys_time_pps_setparams(). You'll also need to extend your
compat_sys_time_pps_fetch() wrapper to handle the struct pps_info too.
--
dwmw2
On Thu, Jun 28, 2007 at 06:14:50PM +0200, Rodolfo Giometti wrote:
> Hello,
>
> here my new LinuxPPS patch.
>
> What to do now for kernel inclusion? Should I provide several patches?
> If so how should I divide them?
>
> Thanks a lot,
Sorry for coming in that late, but using syscalls for something as
periphal sounds like a very bad idea to me, and the syscalls aren't
defined nicely either (e.g. you have an ioctl lookalike). I'd say
back to the drawingboard.
And yes, even ioctls are nicer than badly designed syscalls :)
Also code seems to be odd at least in a few places, e.g. all the access_oks
and double checks of the ioctl-lookalike commands should go.
On Sat, Jun 30, 2007 at 09:38:27AM +0100, Christoph Hellwig wrote:
>
> Sorry for coming in that late, but using syscalls for something as
> periphal sounds like a very bad idea to me, and the syscalls aren't
> defined nicely either (e.g. you have an ioctl lookalike). I'd say
> back to the drawingboard.
PPS API is not only a periphal class. RFC 2783 defines new ?functions?
that allow accesso to internal system data collected by some
periferals but these devices are never managed as a tipical char or
block device.
I think implementing parts or full RFC 2783 PPS's functions as
syscalls are not so wrong... IMHO, at least! :)
> And yes, even ioctls are nicer than badly designed syscalls :)
I see, but consider that some PPS devices cannot be always connected
with a filedescriptor since, for example, some PPS devices are
connected by serial lines, other by parallel ports and other
(expecially on embedded systems) are connected by CPU's GPIOs.
An userland programs shouldn't know whenever these devices are
connected, they should only know how to collect PPS data from the
system.
> Also code seems to be odd at least in a few places, e.g. all the access_oks
> and double checks of the ioctl-lookalike commands should go.
I already removed such functions in my latest patches. :)
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 Fri, Jun 29, 2007 at 05:40:52PM +0100, David Woodhouse wrote:
>
> Remember you have to support _both_ 32-bit and 64-bit system calls. You
> need to define struct compat_pps_info and struct compat_pps_params, and
> you'll have to provide a compat wrapper for sys_time_pps_getparams() and
> sys_time_pps_setparams(). You'll also need to extend your
> compat_sys_time_pps_fetch() wrapper to handle the struct pps_info too.
At this point I'm seriously considfering your previous suggestion:
Had you considered changing the API so that you don't need the
compatibility wrapper at all? Could you take an integer number of
?S or ms instead of a struct timespec?
Maybe I can define a special struct for exchanging time data as:
struct pps_timedata_s {
long sec;
long nsec;
}
and managing time data conversions at userland...
What do you think about that? :)
Thanks,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127