struct timespec will overflow in year 2038, so replace it with ktime_t. And replace functions that use struct timespec, timespec_sub with ktime_sub. Also use monotonic time instead of real time, by replacing getnstimeofday with ktime_get, to be more robust against leap seconds and settimeofday() calls.
Signed-off-by: Ksenija Stanojevic ksenija.stanojevic@gmail.com ---
Changes in v2: -use ktime_t instead timespec64 -use ktime_sub instead timespec64_sub -use monotonic instead real-time.
drivers/staging/fbtft/fbtft-core.c | 35 +++++++++++++---------------------- drivers/staging/fbtft/fbtft.h | 2 +- 2 files changed, 14 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index 7f5fa3d..a1645e1 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -365,16 +365,15 @@ static void fbtft_update_display(struct fbtft_par *par, unsigned start_line, unsigned end_line) { size_t offset, len; - struct timespec ts_start, ts_end, ts_fps, ts_duration; - long fps_ms, fps_us, duration_ms, duration_us; - long fps, throughput; + ktime_t ts_start, ts_end, ts_fps; + long long fps, throughput; bool timeit = false; int ret = 0;
if (unlikely(par->debug & (DEBUG_TIME_FIRST_UPDATE | DEBUG_TIME_EACH_UPDATE))) { if ((par->debug & DEBUG_TIME_EACH_UPDATE) || ((par->debug & DEBUG_TIME_FIRST_UPDATE) && !par->first_update_done)) { - getnstimeofday(&ts_start); + ts_start = ktime_get(); timeit = true; } } @@ -411,30 +410,22 @@ static void fbtft_update_display(struct fbtft_par *par, unsigned start_line, __func__);
if (unlikely(timeit)) { - getnstimeofday(&ts_end); - if (par->update_time.tv_nsec == 0 && par->update_time.tv_sec == 0) { - par->update_time.tv_sec = ts_start.tv_sec; - par->update_time.tv_nsec = ts_start.tv_nsec; - } - ts_fps = timespec_sub(ts_start, par->update_time); - par->update_time.tv_sec = ts_start.tv_sec; - par->update_time.tv_nsec = ts_start.tv_nsec; - fps_ms = (ts_fps.tv_sec * 1000) + ((ts_fps.tv_nsec / 1000000) % 1000); - fps_us = (ts_fps.tv_nsec / 1000) % 1000; - fps = fps_ms * 1000 + fps_us; + ts_end = ktime_get(); + if (par->update_time.tv64 == 0) + par->update_time = ts_start; + + ts_fps = ktime_sub(ts_start, par->update_time); + par->update_time = ts_start; + fps = ktime_to_us(ts_fps); fps = fps ? 1000000 / fps : 0;
- ts_duration = timespec_sub(ts_end, ts_start); - duration_ms = (ts_duration.tv_sec * 1000) + ((ts_duration.tv_nsec / 1000000) % 1000); - duration_us = (ts_duration.tv_nsec / 1000) % 1000; - throughput = duration_ms * 1000 + duration_us; + throughput = ktime_us_delta(ts_end, ts_start); throughput = throughput ? (len * 1000) / throughput : 0; throughput = throughput * 1000 / 1024;
dev_info(par->info->device, - "Display update: %ld kB/s (%ld.%.3ld ms), fps=%ld (%ld.%.3ld ms)\n", - throughput, duration_ms, duration_us, - fps, fps_ms, fps_us); + "Display update: %lld kB/s, fps=%lld\n", + throughput, fps); par->first_update_done = true; } } diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h index 7e9a506..8107f55 100644 --- a/drivers/staging/fbtft/fbtft.h +++ b/drivers/staging/fbtft/fbtft.h @@ -251,7 +251,7 @@ struct fbtft_par { } gamma; unsigned long debug; bool first_update_done; - struct timespec update_time; + ktime_t update_time; bool bgr; void *extra; };
On Wednesday 30 September 2015 18:10:49 Ksenija Stanojevic wrote:
struct timespec will overflow in year 2038, so replace it with ktime_t. And replace functions that use struct timespec, timespec_sub with ktime_sub. Also use monotonic time instead of real time, by replacing getnstimeofday with ktime_get, to be more robust against leap seconds and settimeofday() calls. diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index 7f5fa3d..a1645e1 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -365,16 +365,15 @@ static void fbtft_update_display(struct fbtft_par *par, unsigned start_line, unsigned end_line) { size_t offset, len;
- struct timespec ts_start, ts_end, ts_fps, ts_duration;
- long fps_ms, fps_us, duration_ms, duration_us;
- long fps, throughput;
- ktime_t ts_start, ts_end, ts_fps;
- long long fps, throughput;
Here you declare fps and throughput as 'long long', which causes problems later:
@@ -411,30 +410,22 @@ static void fbtft_update_display(struct fbtft_par *par, unsigned start_line, __func__); if (unlikely(timeit)) {
getnstimeofday(&ts_end);if (par->update_time.tv_nsec == 0 && par->update_time.tv_sec == 0) {par->update_time.tv_sec = ts_start.tv_sec;par->update_time.tv_nsec = ts_start.tv_nsec;}ts_fps = timespec_sub(ts_start, par->update_time);par->update_time.tv_sec = ts_start.tv_sec;par->update_time.tv_nsec = ts_start.tv_nsec;fps_ms = (ts_fps.tv_sec * 1000) + ((ts_fps.tv_nsec / 1000000) % 1000);fps_us = (ts_fps.tv_nsec / 1000) % 1000;fps = fps_ms * 1000 + fps_us;
ts_end = ktime_get();if (par->update_time.tv64 == 0)par->update_time = ts_start;
It's better not to access the 'tv64' field of the ktime_t directly, this is supposed to be hidden. Just use ktime_to_ns() to do the same thing.
ts_fps = ktime_sub(ts_start, par->update_time);par->update_time = ts_start;fps = ktime_to_us(ts_fps);
This can be written slightly simpler using the ktime_us_delta() function, like you do below.
fps = fps ? 1000000 / fps : 0;
ts_duration = timespec_sub(ts_end, ts_start);duration_ms = (ts_duration.tv_sec * 1000) + ((ts_duration.tv_nsec / 1000000) % 1000);duration_us = (ts_duration.tv_nsec / 1000) % 1000;throughput = duration_ms * 1000 + duration_us;
throughput = ktime_us_delta(ts_end, ts_start);
As mentioned above, throughput is a 64-bit 'long long', so the last line of the context will result in a 64-bit division, which is not allowed in 32-bit kernels.
This is a bit hard to detect, and the most reliable way to find issues like this is to compile the kernel for both a 32-bit target and a 64-bit target (on x86, just turn CONFIG_64BIT on/off) to see all the compile-time errors and warnings.
Arnd
Hi,
On Wed, Sep 30, 2015 at 11:52 PM, Arnd Bergmann arnd@arndb.de wrote:
On Wednesday 30 September 2015 18:10:49 Ksenija Stanojevic wrote:
struct timespec will overflow in year 2038, so replace it with ktime_t. And replace functions that use struct timespec, timespec_sub with ktime_sub. Also use monotonic time instead of real time, by replacing getnstimeofday with ktime_get, to be more robust against leap seconds and settimeofday() calls. diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index 7f5fa3d..a1645e1 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -365,16 +365,15 @@ static void fbtft_update_display(struct fbtft_par *par, unsigned start_line, unsigned end_line) { size_t offset, len;
struct timespec ts_start, ts_end, ts_fps, ts_duration;long fps_ms, fps_us, duration_ms, duration_us;long fps, throughput;
ktime_t ts_start, ts_end, ts_fps;long long fps, throughput;Here you declare fps and throughput as 'long long', which causes problems later:
@@ -411,30 +410,22 @@ static void fbtft_update_display(struct fbtft_par *par, unsigned start_line, __func__);
if (unlikely(timeit)) {
getnstimeofday(&ts_end);if (par->update_time.tv_nsec == 0 && par->update_time.tv_sec == 0) {par->update_time.tv_sec = ts_start.tv_sec;par->update_time.tv_nsec = ts_start.tv_nsec;}ts_fps = timespec_sub(ts_start, par->update_time);par->update_time.tv_sec = ts_start.tv_sec;par->update_time.tv_nsec = ts_start.tv_nsec;fps_ms = (ts_fps.tv_sec * 1000) + ((ts_fps.tv_nsec / 1000000) % 1000);fps_us = (ts_fps.tv_nsec / 1000) % 1000;fps = fps_ms * 1000 + fps_us;
ts_end = ktime_get();if (par->update_time.tv64 == 0)par->update_time = ts_start;It's better not to access the 'tv64' field of the ktime_t directly, this is supposed to be hidden. Just use ktime_to_ns() to do the same thing.
ts_fps = ktime_sub(ts_start, par->update_time);par->update_time = ts_start;fps = ktime_to_us(ts_fps);This can be written slightly simpler using the ktime_us_delta() function, like you do below.
fps = fps ? 1000000 / fps : 0;
ts_duration = timespec_sub(ts_end, ts_start);duration_ms = (ts_duration.tv_sec * 1000) + ((ts_duration.tv_nsec / 1000000) % 1000);duration_us = (ts_duration.tv_nsec / 1000) % 1000;throughput = duration_ms * 1000 + duration_us;
throughput = ktime_us_delta(ts_end, ts_start); throughput = throughput ? (len * 1000) / throughput : 0; throughput = throughput * 1000 / 1024;As mentioned above, throughput is a 64-bit 'long long', so the last line of the context will result in a 64-bit division, which is not allowed in 32-bit kernels.
This is a bit hard to detect, and the most reliable way to find issues like this is to compile the kernel for both a 32-bit target and a 64-bit target (on x86, just turn CONFIG_64BIT on/off) to see all the compile-time errors and warnings.
In my local repository I modified my .config file so that CONFIG_64BIT is not set, and after that I recompiled all directory, but I don't get any errors/warning at compile-time. Also I separetly compiled this specific file but still no warnings My .config looks something like this:
# # Automatically generated file; DO NOT EDIT. # Linux/x86 4.3.0-rc3 Kernel Configuration # # CONFIG_64BIT is not set CONFIG_X86_32=y CONFIG_X86=y CONFIG_INSTRUCTION_DECODER=y CONFIG_PERF_EVENTS_INTEL_UNCORE=y CONFIG_OUTPUT_FORMAT="elf32-i386" CONFIG_ARCH_DEFCONFIG="arch/x86/configs/i386_defconfig" . . .
Should I change my working kernel or .config file is just enough?
Thanks, Ksenija
Arnd
On Thursday 01 October 2015 19:44:46 Ksenija Stanojević wrote:
In my local repository I modified my .config file so that CONFIG_64BIT is not set, and after that I recompiled all directory, but I don't get any errors/warning at compile-time. Also I separetly compiled this specific file but still no warnings My .config looks something like this:
. . .
Should I change my working kernel or .config file is just enough?
I think the problem is that you only compiled that directory but did not attempt to do a full rebuild of the kernel and modules, which is required to catch link-time errors.
The compiler does not know at this point that the 64-bit division function is undefined in the kernel, you only get a warning at the 'make vmlinux' link stage (for built-in drivers) or the 'make modules' modpost stage afterwards.
Arnd
On Thu, Oct 1, 2015 at 10:05 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 01 October 2015 19:44:46 Ksenija Stanojević wrote:
In my local repository I modified my .config file so that CONFIG_64BIT is not set, and after that I recompiled all directory, but I don't get any errors/warning at compile-time. Also I separetly compiled this specific file but still no warnings My .config looks something like this:
. . .
Should I change my working kernel or .config file is just enough?
I think the problem is that you only compiled that directory but did not attempt to do a full rebuild of the kernel and modules, which is required to catch link-time errors.
The compiler does not know at this point that the 64-bit division function is undefined in the kernel, you only get a warning at the 'make vmlinux' link stage (for built-in drivers) or the 'make modules' modpost stage afterwards.
I rebuilded my repository with: make vmlinux make modules but I still don't get any warnings. Do you have any other suggestion on what I'm doing wrong?
Thanks, Ksenija
On Saturday 03 October 2015 21:15:46 Ksenija Stanojević wrote:
On Thu, Oct 1, 2015 at 10:05 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 01 October 2015 19:44:46 Ksenija Stanojević wrote:
In my local repository I modified my .config file so that CONFIG_64BIT is not set, and after that I recompiled all directory, but I don't get any errors/warning at compile-time. Also I separetly compiled this specific file but still no warnings My .config looks something like this:
. . .
Should I change my working kernel or .config file is just enough?
I think the problem is that you only compiled that directory but did not attempt to do a full rebuild of the kernel and modules, which is required to catch link-time errors.
The compiler does not know at this point that the 64-bit division function is undefined in the kernel, you only get a warning at the 'make vmlinux' link stage (for built-in drivers) or the 'make modules' modpost stage afterwards.
I rebuilded my repository with: make vmlinux make modules but I still don't get any warnings. Do you have any other suggestion on what I'm doing wrong?
I just tried it on my machine and I get (for an ARM build)
$ make -skj30 ERROR: "__aeabi_ldivmod" [drivers/staging/fbtft/fbtft.ko] undefined!
Two possible explanations why you don't get it:
- your .config file got changed back to CONFIG_64BIT being enabled - you don't have CONFIG_FB_TFT enabled in this build.
Arnd
On Sun, Oct 04, 2015 at 09:34:40PM +0200, Arnd Bergmann wrote:
On Saturday 03 October 2015 21:15:46 Ksenija Stanojević wrote:
On Thu, Oct 1, 2015 at 10:05 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 01 October 2015 19:44:46 Ksenija Stanojević wrote:
In my local repository I modified my .config file so that CONFIG_64BIT is not set, and after that I recompiled all directory, but I don't get any errors/warning at compile-time. Also I separetly compiled this specific file but still no warnings My .config looks something like this:
. . .
Should I change my working kernel or .config file is just enough?
I think the problem is that you only compiled that directory but did not attempt to do a full rebuild of the kernel and modules, which is required to catch link-time errors.
The compiler does not know at this point that the 64-bit division function is undefined in the kernel, you only get a warning at the 'make vmlinux' link stage (for built-in drivers) or the 'make modules' modpost stage afterwards.
I rebuilded my repository with: make vmlinux make modules but I still don't get any warnings. Do you have any other suggestion on what I'm doing wrong?
I just tried it on my machine and I get (for an ARM build)
$ make -skj30 ERROR: "__aeabi_ldivmod" [drivers/staging/fbtft/fbtft.ko] undefined!
Two possible explanations why you don't get it:
- your .config file got changed back to CONFIG_64BIT being enabled
- you don't have CONFIG_FB_TFT enabled in this build.
After modifying manually .config file use make oldconfig and then make prepare. I think i missed the beginning of the thread. Are you saying that fbtft build fails on 32 bit arch?
regards sudip
On Monday 05 October 2015 10:47:05 Sudip Mukherjee wrote:
On Sun, Oct 04, 2015 at 09:34:40PM +0200, Arnd Bergmann wrote:
On Saturday 03 October 2015 21:15:46 Ksenija Stanojević wrote: Two possible explanations why you don't get it:
- your .config file got changed back to CONFIG_64BIT being enabled
- you don't have CONFIG_FB_TFT enabled in this build.
After modifying manually .config file use make oldconfig and then make prepare. I think i missed the beginning of the thread. Are you saying that fbtft build fails on 32 bit arch?
Ksenija's patch from last week causes this build regression, but it is not merged yet. This is my original comment:
| > - ts_duration = timespec_sub(ts_end, ts_start); | > - duration_ms = (ts_duration.tv_sec * 1000) + ((ts_duration.tv_nsec / 1000000) % 1000); | > - duration_us = (ts_duration.tv_nsec / 1000) % 1000; | > - throughput = duration_ms * 1000 + duration_us; | > + throughput = ktime_us_delta(ts_end, ts_start); | > throughput = throughput ? (len * 1000) / throughput : 0; | > throughput = throughput * 1000 / 1024; | | As mentioned above, throughput is a 64-bit 'long long', so the last line | of the context will result in a 64-bit division, which is not allowed in | 32-bit kernels.
Arnd
On Mon, Oct 05, 2015 at 10:20:12AM +0200, Arnd Bergmann wrote:
On Monday 05 October 2015 10:47:05 Sudip Mukherjee wrote:
On Sun, Oct 04, 2015 at 09:34:40PM +0200, Arnd Bergmann wrote:
On Saturday 03 October 2015 21:15:46 Ksenija Stanojević wrote: Two possible explanations why you don't get it:
- your .config file got changed back to CONFIG_64BIT being enabled
- you don't have CONFIG_FB_TFT enabled in this build.
After modifying manually .config file use make oldconfig and then make prepare. I think i missed the beginning of the thread. Are you saying that fbtft build fails on 32 bit arch?
Ksenija's patch from last week causes this build regression, but it is not merged yet.
Ohhh .. ok. thanks. I have also checked the thread from my archieve now.
regards sudip
On Sun, Oct 4, 2015 at 9:34 PM, Arnd Bergmann arnd@arndb.de wrote:
On Saturday 03 October 2015 21:15:46 Ksenija Stanojević wrote:
On Thu, Oct 1, 2015 at 10:05 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 01 October 2015 19:44:46 Ksenija Stanojević wrote:
In my local repository I modified my .config file so that CONFIG_64BIT is not set, and after that I recompiled all directory, but I don't get any errors/warning at compile-time. Also I separetly compiled this specific file but still no warnings My .config looks something like this:
. . .
Should I change my working kernel or .config file is just enough?
I think the problem is that you only compiled that directory but did not attempt to do a full rebuild of the kernel and modules, which is required to catch link-time errors.
The compiler does not know at this point that the 64-bit division function is undefined in the kernel, you only get a warning at the 'make vmlinux' link stage (for built-in drivers) or the 'make modules' modpost stage afterwards.
I rebuilded my repository with: make vmlinux make modules but I still don't get any warnings. Do you have any other suggestion on what I'm doing wrong?
I just tried it on my machine and I get (for an ARM build)
$ make -skj30 ERROR: "__aeabi_ldivmod" [drivers/staging/fbtft/fbtft.ko] undefined!
I got this error: ERROR: "__divdi3" [drivers/staging/fbtft/fbtft.ko] undefined!
I suppose that divdi3 is division used for long long by gcc on x86
Ksenija
On Wednesday 07 October 2015 20:46:23 Ksenija Stanojević wrote:
On Sun, Oct 4, 2015 at 9:34 PM, Arnd Bergmann arnd@arndb.de wrote:
On Saturday 03 October 2015 21:15:46 Ksenija Stanojević wrote:
On Thu, Oct 1, 2015 at 10:05 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 01 October 2015 19:44:46 Ksenija Stanojević wrote:
In my local repository I modified my .config file so that CONFIG_64BIT is not set, and after that I recompiled all directory, but I don't get any errors/warning at compile-time. Also I separetly compiled this specific file but still no warnings My .config looks something like this:
. . .
Should I change my working kernel or .config file is just enough?
I think the problem is that you only compiled that directory but did not attempt to do a full rebuild of the kernel and modules, which is required to catch link-time errors.
The compiler does not know at this point that the 64-bit division function is undefined in the kernel, you only get a warning at the 'make vmlinux' link stage (for built-in drivers) or the 'make modules' modpost stage afterwards.
I rebuilded my repository with: make vmlinux make modules but I still don't get any warnings. Do you have any other suggestion on what I'm doing wrong?
I just tried it on my machine and I get (for an ARM build)
$ make -skj30 ERROR: "__aeabi_ldivmod" [drivers/staging/fbtft/fbtft.ko] undefined!
I got this error: ERROR: "__divdi3" [drivers/staging/fbtft/fbtft.ko] undefined!
I suppose that divdi3 is division used for long long by gcc on x86
Correct, this function name is architecture specific. These functions (__divdi3, __aeabi_ldivmod, ...) are intentionally not part of the kernel image so force kernel developers to either use the div_u64() family of functions for division or find a way to avoid it, which is usually preferred but not always possible.
Arnd
-
Arnd Bergmann -
Ksenija Stanojevic -
Ksenija Stanojević
-
Sudip Mukherjee