Lesson 5 time managment - #17
dmytrokirtoka wants to merge 1 commit into
Conversation
3c58b0d to
786b319
Compare
786b319 to
d894f5e
Compare
|
Check my work please. |
AleksandrBulyshchenko
left a comment
AleksandrBulyshchenko
left a comment
There was a problem hiding this comment.
I like how the solution looks like. 👍
-
Fibonacci period is quite small, but with handled overflow is ok.
-
Output format isn't very suitable for sysfs.
Such wordy interface is better to place in procfs.
(but this is a nitpick here) -
There's a lot of coding standard issues:
${KERNEL_SOURCE}/scripts/checkpatch.pl --file timeman.c --terse
total: 31 errors, 18 warnings, 159 lines checked
And I'd like you to fix them.
|
|
||
| char* print_abstime(char* buf) | ||
| { | ||
| struct timespec64 ts = current_kernel_time64(); |
| #define DMASK 3 | ||
| static int64_t n1, n2; | ||
| static int64_t n[DEPTH]; | ||
| static volatile int head, is_started; |
There was a problem hiding this comment.
Please read volatile-considered-harmful.rst
Atomics should be used here.
|
|
||
| static ssize_t absolute_show(struct class *class, struct class_attribute *attr, char *buf) { | ||
| print_abstime(buf); | ||
| printk("read time %s\n", buf); |
There was a problem hiding this comment.
the buf already contains '\n'
|
|
||
| /* Show last 3 digit from fibonacci calculated one per second */ | ||
| #define DEPTH 4 | ||
| #define DMASK 3 |
There was a problem hiding this comment.
Use hex representation for binary masks.
| { | ||
| if (is_started) { | ||
| is_started = 0; | ||
| msleep(1000); |
There was a problem hiding this comment.
In worst case you can gate race between timer callback and msleep return.
Looks like you should just call del_timer() instead.
|
|
||
| res = class_create_file( tman_class, &class_attr_interval ); | ||
| res |= class_create_file( tman_class, &class_attr_absolute ); | ||
| res |= class_create_file( tman_class, &class_attr_fibonacci ); |
There was a problem hiding this comment.
There's no cleanup in case of errors.
| #include <linux/delay.h> | ||
|
|
||
| /* Show interval from prev read */ | ||
| static uint64_t last_time; |
There was a problem hiding this comment.
It makes sense to move the last_time variable into print_interval() as local static.
| } | ||
|
|
||
| /* Show Absolute time for prev read */ | ||
| static time64_t prev_read_time; |
There was a problem hiding this comment.
It makes sense to move the prev_read_time variable into print_abstime() as local static.
All managment set as one module wich used sysfs class tman_class and
next sub_class i/o files:
- interval: show how many seconds has been passed from prev read
- absolute: show prev read time YY/MM/DD HH:MM:SS
- fibonacci: calculate next sequense per second
read show last 3 calculations
write resetting sequence
Signed-off-by: dmytro.kirtoka <dimk334@gmail.com>
d894f5e to
84146c3
Compare
|
All done. |
AleksandrBulyshchenko
left a comment
AleksandrBulyshchenko
left a comment
There was a problem hiding this comment.
👍
- But error handling in
tman_init()should be fixed. - And don't forget tu update commit messages.
- Also there're few minor checkpatch issues.
Use
git format-patch master
${KERNEL_SOURCE}/scripts/checkpatch.pl *.patch --terse
|
|
||
| /* Show last 3 digit from fibonacci calculated one per second */ | ||
| #define DEPTH 0x4 | ||
| #define DMASK 0x3 |
There was a problem hiding this comment.
Actually the DEPTH is array size - it's not binary (like the DMASK) and should be written as decimal value.
| static ssize_t fibonacci_show(struct class *class, | ||
| struct class_attribute *attr, char *buf) | ||
| { | ||
| sprintf(buf, "fibonacci: %lld\n", n[atomic_read(&head) & DMASK]); |
There was a problem hiding this comment.
Output doesn't correspond to described in the commit message.
And it doesn't make sense to store the array n[DEPTH] if you output only n2.
| struct class_attribute *attr, const char *buf, size_t count) | ||
| { | ||
| if (atomic_cmpxchg(&is_started, 1, 0)) | ||
| del_timer(&mytimer); |
There was a problem hiding this comment.
AFAIK, you can call del_timer() unconditionally.
| memset(n, 0, DEPTH); | ||
| n1 = 0; | ||
| n2 = 1; | ||
| atomic_set(&is_started, 1); |
There was a problem hiding this comment.
Actually, instead of using is_started,
all manipulations with n1, n2, head and nfrom fib_handler() and fibonacci_store() should be under spinlock.
| if (res) { | ||
| pr_err("init class_attr_fibonacci failed\n"); | ||
| return res; | ||
| } |
There was a problem hiding this comment.
In case of errors, it's not enough to just exit - all already allocated resources should be freed.
2 participants
All managment set as single module wich used sysfs class tman_class and
next sub_class i/o files:
Read Only
Read / Write
read show last 3 calculations
write resetting sequence
Signed-off-by: dmytro.kirtoka dimk334@gmail.com