[PATCH v3 05/22] posix-timers:Convert to the 64bit methods for the timer_gettime syscall function
Convert to the 64bit methods with timespec64/itimerspec64 type for the timer_gettime syscall function, and change the timer_gettime syscall implementation according to the CONFIG_64BIT macro.
Signed-off-by: Baolin Wang baolin.wang@linaro.org --- include/linux/posix-timers.h | 2 ++ kernel/time/posix-timers.c | 31 +++++++++++++++++++++++++++---- 2 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h index 907f3fd..e84436b 100644 --- a/include/linux/posix-timers.h +++ b/include/linux/posix-timers.h @@ -113,6 +113,8 @@ struct k_clock { #define TIMER_RETRY 1 void (*timer_get) (struct k_itimer * timr, struct itimerspec * cur_setting); + void (*timer_get64) (struct k_itimer *timr, + struct itimerspec64 *cur_setting); };
extern struct k_clock clock_posix_cpu; diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c index 9df664a..f7cf00b 100644 --- a/kernel/time/posix-timers.c +++ b/kernel/time/posix-timers.c @@ -140,6 +140,7 @@ static int common_timer_del(struct k_itimer *timer); static enum hrtimer_restart posix_timer_fn(struct hrtimer *data);
static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags); +static struct k_clock *clockid_to_kclock(const clockid_t id);
#define lock_timer(tid, flags) \ ({ struct k_itimer *__timr; \ @@ -513,6 +514,16 @@ static struct pid *good_sigevent(sigevent_t * event) return task_pid(rtn); }
+static int default_timer_get64(struct k_itimer *timr, + struct itimerspec64 *cur_setting64) +{ + struct itimerspec cur_setting; + struct k_clock *kc = clockid_to_kclock(timr->it_clock); + + kc->timer_get(timr, &cur_setting); + return 0; +} + void posix_timers_register_clock(const clockid_t clock_id, struct k_clock *new_clock) { @@ -533,6 +544,9 @@ void posix_timers_register_clock(const clockid_t clock_id, return; }
+ if (new_clock->timer_get && !new_clock->timer_get64) + new_clock->timer_get64 = default_timer_get64; + posix_clocks[clock_id] = *new_clock; } EXPORT_SYMBOL_GPL(posix_timers_register_clock); @@ -766,7 +780,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 +792,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; @@ -791,8 +805,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;
On Monday 11 May 2015 19:15:14 Baolin Wang wrote:
+static int default_timer_get64(struct k_itimer *timr,
struct itimerspec64 *cur_setting64)+{
struct itimerspec cur_setting;struct k_clock *kc = clockid_to_kclock(timr->it_clock);kc->timer_get(timr, &cur_setting);return 0;+}
This function is unfortunately incorrect, because you never copy the cur_setting value into cur_setting64.
Arnd
On 12 May 2015 at 00:30, Arnd Bergmann arnd@arndb.de wrote:
On Monday 11 May 2015 19:15:14 Baolin Wang wrote:
+static int default_timer_get64(struct k_itimer *timr,
struct itimerspec64 *cur_setting64)+{
struct itimerspec cur_setting;struct k_clock *kc = clockid_to_kclock(timr->it_clock);kc->timer_get(timr, &cur_setting);return 0;+}
This function is unfortunately incorrect, because you never copy the cur_setting value into cur_setting64.
Arnd
Hi Arnd,
Thanks for your comments. But i think this is just a temporary default function, and will be removed after all the drivers' conversion, so just ensure it won't cause the kernel crash.
On Tuesday 12 May 2015 10:05:45 Baolin Wang wrote:
On 12 May 2015 at 00:30, Arnd Bergmann arnd@arndb.de wrote:
On Monday 11 May 2015 19:15:14 Baolin Wang wrote:
+static int default_timer_get64(struct k_itimer *timr,
struct itimerspec64 *cur_setting64)+{
struct itimerspec cur_setting;struct k_clock *kc = clockid_to_kclock(timr->it_clock);kc->timer_get(timr, &cur_setting);return 0;+}
This function is unfortunately incorrect, because you never copy the cur_setting value into cur_setting64.
Thanks for your comments. But i think this is just a temporary default function, and will be removed after all the drivers' conversion, so just ensure it won't cause the kernel crash.
No, that function has to do the right thing. The purpose of the function is to keep the kernel working when only half the series is applied, this is a fundamental part of how we do kernel development: Each patch in you series needs to bring the kernel closer to what we want to have in the end but not introduce bugs. Your current function stops the timer_gettime() system call from working and makes it return uninitialized kernel data.
Arnd
On 12 May 2015 at 15:33, Arnd Bergmann arnd@arndb.de wrote:
On Tuesday 12 May 2015 10:05:45 Baolin Wang wrote:
On 12 May 2015 at 00:30, Arnd Bergmann arnd@arndb.de wrote:
On Monday 11 May 2015 19:15:14 Baolin Wang wrote:
+static int default_timer_get64(struct k_itimer *timr,
struct itimerspec64 *cur_setting64)+{
struct itimerspec cur_setting;struct k_clock *kc = clockid_to_kclock(timr->it_clock);kc->timer_get(timr, &cur_setting);return 0;+}
This function is unfortunately incorrect, because you never copy the cur_setting value into cur_setting64.
Thanks for your comments. But i think this is just a temporary default function, and will be removed after all the drivers' conversion, so just ensure it won't cause the kernel crash.
No, that function has to do the right thing. The purpose of the function is to keep the kernel working when only half the series is applied, this is a fundamental part of how we do kernel development: Each patch in you series needs to bring the kernel closer to what we want to have in the end but not introduce bugs. Your current function stops the timer_gettime() system call from working and makes it return uninitialized kernel data.
Arnd
Hi Arnd,
Thanks for your explanations, and i'll fix that bug in next patch.
On Tue, 12 May 2015, Baolin Wang wrote:
On 12 May 2015 at 00:30, Arnd Bergmann arnd@arndb.de wrote:
On Monday 11 May 2015 19:15:14 Baolin Wang wrote:
+static int default_timer_get64(struct k_itimer *timr,
struct itimerspec64 *cur_setting64)+{
struct itimerspec cur_setting;struct k_clock *kc = clockid_to_kclock(timr->it_clock);kc->timer_get(timr, &cur_setting);return 0;+}
This function is unfortunately incorrect, because you never copy the cur_setting value into cur_setting64.
Thanks for your comments. But i think this is just a temporary default function, and will be removed after all the drivers' conversion, so just ensure it won't cause the kernel crash.
The function is crap no matter whether its removed later or not. And it breaks bisectability.
Thanks,
tglx
On 12 May 2015 at 22:39, Thomas Gleixner tglx@linutronix.de wrote:
On Tue, 12 May 2015, Baolin Wang wrote:
On 12 May 2015 at 00:30, Arnd Bergmann arnd@arndb.de wrote:
On Monday 11 May 2015 19:15:14 Baolin Wang wrote:
+static int default_timer_get64(struct k_itimer *timr,
struct itimerspec64 *cur_setting64)+{
struct itimerspec cur_setting;struct k_clock *kc = clockid_to_kclock(timr->it_clock);kc->timer_get(timr, &cur_setting);return 0;+}
This function is unfortunately incorrect, because you never copy the cur_setting value into cur_setting64.
Thanks for your comments. But i think this is just a temporary default function, and will be removed after all the drivers' conversion, so just ensure it won't cause the kernel crash.
The function is crap no matter whether its removed later or not. And it breaks bisectability.
Thanks,
tglx
I have fixed this function, thanks.
-
Arnd Bergmann -
Baolin Wang -
Thomas Gleixner