While looking through the tree and checking out possible signedness issues
pointed out to me by gcc -W I came across this one :
kernel/timer.c:464: warning: comparison between signed and unsigned
The issue there is this little bit of code :
[...]
462 u32 preempt_count = preempt_count();
463 fn(data);
464 if (preempt_count != preempt_count()) {
[...]
gcc is complaining about that since preempt_count in struct thread_info
(which is what preempt_count() returns) is a signed integer. Initially I
thought "that's a bit sloppy, I'll just make the local variable a s32 and
that should be that", but then I looked a little closer at struct
thread_info for the different archs, and found that the type used differs
between archs and it's not even a signed type on all (s390 being the
unsigned exception). So all of a sudden fixing up this little warning was
not so simple after all.
The different types used for struct thread_info.preempt_count are
__s32, int and unsigned int.
Why are different types used for struct thread_info.preempt_count? Would
it not make sense to make it a __s32 or plain int on all archs? I would
think that consistency here would be a good thing.
And why on earth is it an unsigned int on s390? It seems to me that that
makes it impossible to catch bugs where preempt_count is decremented below
zero.
Any reason not to apply a patch like this one?
my choice of 'int' over '__s32' was pretty arbitrary - as far as I know we
don't support any archs where 'int' is less than 32bits, do we? So I
figured that using a plain int type should give us at least 32 bits
everywhere as well as using the given platforms fastest type. If any of
the archs have <32bit int types, then I guess __s32 would be a better
choice.
Signed-of-by: Jesper Juhl <[email protected]>
--- linux-2.6.12-rc2-mm3-orig/include/asm-arm/thread_info.h 2005-03-02 08:38:08.000000000 +0100
+++ linux-2.6.12-rc2-mm3/include/asm-arm/thread_info.h 2005-04-23 23:16:04.000000000 +0200
@@ -45,7 +45,7 @@
*/
struct thread_info {
unsigned long flags; /* low level flags */
- __s32 preempt_count; /* 0 => preemptable, <0 => bug */
+ int preempt_count; /* 0 => preemptable, <0 => bug */
mm_segment_t addr_limit; /* address limit */
struct task_struct *task; /* main task structure */
struct exec_domain *exec_domain; /* execution domain */
--- linux-2.6.12-rc2-mm3-orig/include/asm-arm26/thread_info.h 2005-03-02 08:37:50.000000000 +0100
+++ linux-2.6.12-rc2-mm3/include/asm-arm26/thread_info.h 2005-04-23 23:16:22.000000000 +0200
@@ -44,7 +44,7 @@
*/
struct thread_info {
unsigned long flags; /* low level flags */
- __s32 preempt_count; /* 0 => preemptable, <0 => bug */
+ int preempt_count; /* 0 => preemptable, <0 => bug */
mm_segment_t addr_limit; /* address limit */
struct task_struct *task; /* main task structure */
struct exec_domain *exec_domain; /* execution domain */
--- linux-2.6.12-rc2-mm3-orig/include/asm-cris/thread_info.h 2005-03-02 08:38:32.000000000 +0100
+++ linux-2.6.12-rc2-mm3/include/asm-cris/thread_info.h 2005-04-23 23:16:29.000000000 +0200
@@ -31,7 +31,7 @@
struct exec_domain *exec_domain; /* execution domain */
unsigned long flags; /* low level flags */
__u32 cpu; /* current CPU */
- __s32 preempt_count; /* 0 => preemptable, <0 => BUG */
+ int preempt_count; /* 0 => preemptable, <0 => BUG */
mm_segment_t addr_limit; /* thread address space:
0-0xBFFFFFFF for user-thead
--- linux-2.6.12-rc2-mm3-orig/include/asm-frv/thread_info.h 2005-03-02 08:37:50.000000000 +0100
+++ linux-2.6.12-rc2-mm3/include/asm-frv/thread_info.h 2005-04-23 23:16:37.000000000 +0200
@@ -33,7 +33,7 @@
unsigned long flags; /* low level flags */
unsigned long status; /* thread-synchronous flags */
__u32 cpu; /* current CPU */
- __s32 preempt_count; /* 0 => preemptable, <0 => BUG */
+ int preempt_count; /* 0 => preemptable, <0 => BUG */
mm_segment_t addr_limit; /* thread address space:
0-0xBFFFFFFF for user-thead
--- linux-2.6.12-rc2-mm3-orig/include/asm-i386/thread_info.h 2005-04-05 21:21:48.000000000 +0200
+++ linux-2.6.12-rc2-mm3/include/asm-i386/thread_info.h 2005-04-23 23:16:50.000000000 +0200
@@ -31,7 +31,7 @@
unsigned long flags; /* low level flags */
unsigned long status; /* thread-synchronous flags */
__u32 cpu; /* current CPU */
- __s32 preempt_count; /* 0 => preemptable, <0 => BUG */
+ int preempt_count; /* 0 => preemptable, <0 => BUG */
mm_segment_t addr_limit; /* thread address space:
--- linux-2.6.12-rc2-mm3-orig/include/asm-ia64/thread_info.h 2005-03-02 08:38:33.000000000 +0100
+++ linux-2.6.12-rc2-mm3/include/asm-ia64/thread_info.h 2005-04-23 23:17:04.000000000 +0200
@@ -25,7 +25,7 @@
__u32 flags; /* thread_info flags (see TIF_*) */
__u32 cpu; /* current CPU */
mm_segment_t addr_limit; /* user-level address space limit */
- __s32 preempt_count; /* 0=premptable, <0=BUG; will also serve as bh-counter */
+ int preempt_count; /* 0=premptable, <0=BUG; will also serve as bh-counter */
struct restart_block restart_block;
struct {
int signo;
--- linux-2.6.12-rc2-mm3-orig/include/asm-m32r/thread_info.h 2005-03-02 08:38:26.000000000 +0100
+++ linux-2.6.12-rc2-mm3/include/asm-m32r/thread_info.h 2005-04-23 23:17:21.000000000 +0200
@@ -28,7 +28,7 @@
unsigned long flags; /* low level flags */
unsigned long status; /* thread-synchronous flags */
__u32 cpu; /* current CPU */
- __s32 preempt_count; /* 0 => preemptable, <0 => BUG */
+ int preempt_count; /* 0 => preemptable, <0 => BUG */
mm_segment_t addr_limit; /* thread address space:
0-0xBFFFFFFF for user-thread
--- linux-2.6.12-rc2-mm3-orig/include/asm-m68k/thread_info.h 2005-04-05 21:21:49.000000000 +0200
+++ linux-2.6.12-rc2-mm3/include/asm-m68k/thread_info.h 2005-04-23 23:17:29.000000000 +0200
@@ -8,7 +8,7 @@
struct thread_info {
struct task_struct *task; /* main task structure */
struct exec_domain *exec_domain; /* execution domain */
- __s32 preempt_count; /* 0 => preemptable, <0 => BUG */
+ int preempt_count; /* 0 => preemptable, <0 => BUG */
__u32 cpu; /* should always be 0 on m68k */
struct restart_block restart_block;
--- linux-2.6.12-rc2-mm3-orig/include/asm-mips/thread_info.h 2005-03-02 08:37:30.000000000 +0100
+++ linux-2.6.12-rc2-mm3/include/asm-mips/thread_info.h 2005-04-23 23:17:47.000000000 +0200
@@ -27,7 +27,7 @@
struct exec_domain *exec_domain; /* execution domain */
unsigned long flags; /* low level flags */
__u32 cpu; /* current CPU */
- __s32 preempt_count; /* 0 => preemptable, <0 => BUG */
+ int preempt_count; /* 0 => preemptable, <0 => BUG */
mm_segment_t addr_limit; /* thread address space:
0-0xBFFFFFFF for user-thead
--- linux-2.6.12-rc2-mm3-orig/include/asm-parisc/thread_info.h 2005-04-05 21:21:49.000000000 +0200
+++ linux-2.6.12-rc2-mm3/include/asm-parisc/thread_info.h 2005-04-23 23:17:55.000000000 +0200
@@ -12,7 +12,7 @@
unsigned long flags; /* thread_info flags (see TIF_*) */
mm_segment_t addr_limit; /* user-level address space limit */
__u32 cpu; /* current CPU */
- __s32 preempt_count; /* 0=premptable, <0=BUG; will also serve as bh-counter */
+ int preempt_count; /* 0=premptable, <0=BUG; will also serve as bh-counter */
struct restart_block restart_block;
};
--- linux-2.6.12-rc2-mm3-orig/include/asm-s390/thread_info.h 2005-03-02 08:38:13.000000000 +0100
+++ linux-2.6.12-rc2-mm3/include/asm-s390/thread_info.h 2005-04-23 23:18:12.000000000 +0200
@@ -50,7 +50,7 @@
struct exec_domain *exec_domain; /* execution domain */
unsigned long flags; /* low level flags */
unsigned int cpu; /* current CPU */
- unsigned int preempt_count; /* 0 => preemptable */
+ int preempt_count; /* 0 => preemptable */
struct restart_block restart_block;
};
--- linux-2.6.12-rc2-mm3-orig/include/asm-sh/thread_info.h 2005-03-02 08:38:13.000000000 +0100
+++ linux-2.6.12-rc2-mm3/include/asm-sh/thread_info.h 2005-04-23 23:18:20.000000000 +0200
@@ -20,7 +20,7 @@
struct exec_domain *exec_domain; /* execution domain */
__u32 flags; /* low level flags */
__u32 cpu;
- __s32 preempt_count; /* 0 => preemptable, <0 => BUG */
+ int preempt_count; /* 0 => preemptable, <0 => BUG */
struct restart_block restart_block;
__u8 supervisor_stack[0];
};
--- linux-2.6.12-rc2-mm3-orig/include/asm-sh64/thread_info.h 2005-04-05 21:21:49.000000000 +0200
+++ linux-2.6.12-rc2-mm3/include/asm-sh64/thread_info.h 2005-04-23 23:18:27.000000000 +0200
@@ -22,7 +22,7 @@
struct exec_domain *exec_domain; /* execution domain */
unsigned long flags; /* low level flags */
/* Put the 4 32-bit fields together to make asm offsetting easier. */
- __s32 preempt_count; /* 0 => preemptable, <0 => BUG */
+ int preempt_count; /* 0 => preemptable, <0 => BUG */
__u16 cpu;
mm_segment_t addr_limit;
--- linux-2.6.12-rc2-mm3-orig/include/asm-um/thread_info.h 2005-03-02 08:37:54.000000000 +0100
+++ linux-2.6.12-rc2-mm3/include/asm-um/thread_info.h 2005-04-23 23:18:50.000000000 +0200
@@ -17,7 +17,7 @@
struct exec_domain *exec_domain; /* execution domain */
unsigned long flags; /* low level flags */
__u32 cpu; /* current CPU */
- __s32 preempt_count; /* 0 => preemptable,
+ int preempt_count; /* 0 => preemptable,
<0 => BUG */
mm_segment_t addr_limit; /* thread address space:
0-0xBFFFFFFF for user
and then the litle detail in kernel/timer.c can be fixed nicely like this:
Signed-off-by: Jesper Juhl <[email protected]>
--- linux-2.6.12-rc2-mm3-orig/kernel/timer.c 2005-04-11 21:20:56.000000000 +0200
+++ linux-2.6.12-rc2-mm3/kernel/timer.c 2005-04-23 23:19:21.000000000 +0200
@@ -459,7 +459,7 @@ static inline void __run_timers(tvec_bas
detach_timer(timer, 1);
spin_unlock_irq(&base->t_base.lock);
{
- u32 preempt_count = preempt_count();
+ int preempt_count = preempt_count();
fn(data);
if (preempt_count != preempt_count()) {
printk("huh, entered %p with %08x, exited with %08x?\n", fn, preempt_count, preempt_count());
or is there some good reason I'm missing that make the above a bad idea?
--
Jesper Juhl
PS. Please CC: me on replies.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
