On 28 April 2015 at 17:42, Arnd Bergmann arnd@linaro.org wrote:
On Tuesday 28 April 2015 16:05:47 Baolin Wang wrote:
This patch converts to the 64bit methods with timespec64/itimerspec64
type,
and changes the syscall implementation according to the CONFIG_64BIT
macro.
Signed-off-by: Baolin Wang baolin.wang@linaro.org
kernel/time/posix-timers.c | 103
+++++++++++++++++++++++++++++++++-----------
1 file changed, 78 insertions(+), 25 deletions(-)
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c index 8564b88..7109688 100644 --- a/kernel/time/posix-timers.c +++ b/kernel/time/posix-timers.c @@ -522,13 +522,13 @@ void posix_timers_register_clock(const clockid_t
clock_id,
return; }
if (!new_clock->clock_get) {printk(KERN_WARNING "POSIX clock id %d lacksclock_get()\n",
if (!new_clock->clock_get && !new_clock->clock_get64) {printk(KERN_WARNING "POSIX clock id %d lacks clock_get()and clock_get64()\n",
clock_id); return; }Here you are missing a step that Thomas suggested in the previous review:
add a default clock_get64() implementation that calls clock_get()
But the default_timer_get64((struct k_clock *kc, struct k_itimer *timr, struct itimerspec64 *cur_setting64) function he suggested can't get the "kc" parameter, cause the "timer_get" point prototype is "void (*timer_get64) (struct k_itimer *timr, struct itimerspec64 *cur_setting);".
static int default_timer_get64(struct k_clock *kc, struct k_itimer *timr, struct itimerspec64 *cur_setting64) { struct itimerspec cur_setting;
kc->timer_get(timer, &cur_setting); return 0; }
@@ -766,7 +767,7 @@ common_timer_get(struct k_itimer *timr, struct itimerspec *cur_setting)
cur_setting->it_value = ktime_to_timespec(remaining);}
-static int __timer_gettime(timer_t timer_id, struct itimerspec
*cur_setting)
+static int __timer_gettime(timer_t timer_id, struct itimerspec64
*cur_setting)
{ struct k_itimer *timr; struct k_clock *kc; @@ -778,10 +779,10 @@ static int __timer_gettime(timer_t timer_id,
struct itimerspec *cur_setting)
return -EINVAL; kc = clockid_to_kclock(timr->it_clock);
if (WARN_ON_ONCE(!kc || !kc->timer_get))
if (WARN_ON_ONCE(!kc || !kc->timer_get64)) ret = -EINVAL; else
kc->timer_get(timr, cur_setting);
kc->timer_get64(timr, cur_setting); unlock_timer(timr, flags); return ret;Without that change, you are now breaking all implementations of timer_get() because the kernel tries to call timer_get64, which is a NULL pointer. The same thing applies to all the system calls of course.
@@ -791,8 +792,17 @@ static int __timer_gettime(timer_t timer_id, struct
itimerspec *cur_setting)
SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, struct itimerspec __user *, setting) {
struct itimerspec cur_setting;+#ifdef CONFIG_64BIT
struct itimerspec64 cur_setting; int ret = __timer_gettime(timer_id, &cur_setting);+#else
struct itimerspec64 cur_setting64;struct itimerspec cur_setting;int ret = __timer_gettime(timer_id, &cur_setting64);if (!ret)cur_setting = itimerspec64_to_itimerspec(&cur_setting64);+#endif
if (!ret && copy_to_user(setting, &cur_setting, sizeof(cur_setting)))
return -EFAULT;At this point, you are overlapping a bit with the series that I'm doing, but I guess you can leave it like this for now.
What I want to do here is to introduce a get_itimerspec64()/put_itimerspec64() function that copies an itimerspec structure from user space and puts it into a local variable, with different implementations for 32-bit and 64-bit, in order to avoid the #ifdef. I'm hoping to have a first cut at my patch series soon, and we can coordinate this part then.
Arnd
OK.