bit ops on unsigned long?
Subsystems (10)
Hi Linus,
nfs is broken in 2.5 ATM because it does set_bit on an "int".
Can be *please* just bite the bullet and change the prototype on these ops so we stop seeing the same mistakes over and over?
This and "copy_from_user doesn't return -EFAULT" are the two classic trivial kernel bugs.
Thanks,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
Quoted 4 lines · Rusty Russell
nfs is broken in 2.5 ATM because it does
set_biton an "int".
Can be *please* just bite the bullet and change the prototype on these ops so we stop seeing the same mistakes over and over?
The problem with the prototype is that it's not always correct.
It's fine to pass non-"unsigned long *" pointers to set_bit, if you know the pointers are otherwise aligned. Things like buffer bitmaps etc.
How does the patch look? If it has a lot of unnecessary casts, I don't want it, but if the casts are only in things like ext2_setbit(), then that might be ok..
Linus
In message <[email protected]> you wri te:
Quoted 12 lines · Rusty Russell
Quoted 4 lines
nfs is broken in 2.5 ATM because it does
set_biton an "int".
Can be *please* just bite the bullet and change the prototype on these ops so we stop seeing the same mistakes over and over?The problem with the prototype is that it's not always correct.
It's fine to pass non-"unsigned long *" pointers to
set_bit, if you know the pointers are otherwise aligned. Things like buffer bitmaps etc.
Sure: just like casting.
Quoted 3 lines
How does the patch look? If it has a lot of unnecessary casts, I don't want it, but if the casts are only in things like
ext2_setbit(), then that might be ok..
Actually, looks good: seems most kernel coders are expecting this. Mainly changing from set_bit(x, &array) => set_bit(x, array).
Found 5 bugs doing this, too. Meaning they have crept in since the last such audit about 6 months ago 8(
Richard: 3 bugs in devfs. Particularly note that the memset was bogus. I can't convince myself that your memcpy & memset stuff is right anyway, given that you can ONLY treat them as unsigned longs (ie. bit 31 will be in byte 0 or byte 3, depending on endianness).
This fixes all the warnings on my ppc and x86/SMP boxes, Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
arch/i386/kernel/apic.c +1 -1
@@ -634,7 +634,7 @@printk ("Could not enable APIC!\n" );return -1 ; } -set_bit (X86_FEATURE_APIC , &boot_cpu_data.x86_capability); +set_bit (X86_FEATURE_APIC , boot_cpu_data.x86_capability); mp_lapic_addr =APIC_DEFAULT_PHYS_BASE ; boot_cpu_physical_apicid =0 ;if (nmi_watchdog !=NMI_NONE )
arch/i386/kernel/bluesmoke.c +2 -2
@@ -123,7 +123,7 @@* Check for MCE support */ -if ( !test_bit (X86_FEATURE_MCE , &c->x86_capability) ) +if ( !test_bit (X86_FEATURE_MCE , c->x86_capability) )return ;/*
@@ -153,7 +153,7 @@* Check for PPro style MCA */ -if ( !test_bit (X86_FEATURE_MCA , &c->x86_capability) ) +if ( !test_bit (X86_FEATURE_MCA , c->x86_capability) )return ;/* Ok machine check is available */
arch/i386/kernel/mtrr.c +6 -6
@@ -387,7 +387,7 @@return ;/* Save value of CR4 and clear Page Global Enable (bit 7) */ -if (test_bit (X86_FEATURE_PGE , &boot_cpu_data.x86_capability) ) { +if (test_bit (X86_FEATURE_PGE , boot_cpu_data.x86_capability) ) { ctxt->cr4val =read_cr4 ();write_cr4 (ctxt->cr4val & (unsigned char ) ~(1 <<7 )); }
@@ -448,7 +448,7 @@write_cr0 (read_cr0 () &0xbfffffff );/* Restore value of CR4 */ -if (test_bit (X86_FEATURE_PGE , &boot_cpu_data.x86_capability) ) +if (test_bit (X86_FEATURE_PGE , boot_cpu_data.x86_capability) )write_cr4 (ctxt->cr4val);/* Re-enable interrupts locally (if enabled previously) */
@@ -2123,7 +2123,7 @@static int __init mtrr_setup (void ) { -if (test_bit (X86_FEATURE_MTRR , &boot_cpu_data.x86_capability) ) { +if (test_bit (X86_FEATURE_MTRR , boot_cpu_data.x86_capability) ) {/* Intel (P6) standard MTRRs */ mtrr_if =MTRR_IF_INTEL ; get_mtrr = intel_get_mtrr;
@@ -2167,14 +2167,14 @@break ; } - }else if (test_bit (X86_FEATURE_K6_MTRR , &boot_cpu_data.x86_capability) ) { + }else if (test_bit (X86_FEATURE_K6_MTRR , boot_cpu_data.x86_capability) ) {/* Pre-Athlon (K6) AMD CPU MTRRs */ mtrr_if =MTRR_IF_AMD_K6 ; get_mtrr = amd_get_mtrr; set_mtrr_up = amd_set_mtrr_up; size_or_mask =0xfff00000 ;/* 32 bits */ size_and_mask =0 ; - }else if (test_bit (X86_FEATURE_CYRIX_ARR , &boot_cpu_data.x86_capability) ) { + }else if (test_bit (X86_FEATURE_CYRIX_ARR , boot_cpu_data.x86_capability) ) {/* Cyrix ARRs */ mtrr_if =MTRR_IF_CYRIX_ARR ; get_mtrr = cyrix_get_arr;
@@ -2183,7 +2183,7 @@cyrix_arr_init (); size_or_mask =0xfff00000 ;/* 32 bits */ size_and_mask =0 ; - }else if (test_bit (X86_FEATURE_CENTAUR_MCR , &boot_cpu_data.x86_capability) ) { + }else if (test_bit (X86_FEATURE_CENTAUR_MCR , boot_cpu_data.x86_capability) ) {/* Centaur MCRs */ mtrr_if =MTRR_IF_CENTAUR_MCR ; get_mtrr = centaur_get_mcr;
arch/i386/kernel/setup.c +35 -35
235 lines not on this page · raw message
arch/ppc/platforms/pmac_pic.c +1 -1
@@ -492,7 +492,7 @@* and disables all interrupts except for the nominated one. * sleep_restore_intrs() restores the states of all interrupt enables. */ -unsigned int sleep_save_mask[2 ]; +unsigned long sleep_save_mask[2 ]; void __pmacpmac_sleep_save_intrs (int viaint)
drivers/char/mem.c +4 -4
@@ -173,10 +173,10 @@* caching for the high addresses through the KEN pin, but * we maintain the tradition of paranoia in this code. */ -return !(test_bit (X86_FEATURE_MTRR , &boot_cpu_data.x86_capability) || -test_bit (X86_FEATURE_K6_MTRR , &boot_cpu_data.x86_capability) || -test_bit (X86_FEATURE_CYRIX_ARR , &boot_cpu_data.x86_capability) || -test_bit (X86_FEATURE_CENTAUR_MCR , &boot_cpu_data.x86_capability) ) +return !(test_bit (X86_FEATURE_MTRR , boot_cpu_data.x86_capability) || +test_bit (X86_FEATURE_K6_MTRR , boot_cpu_data.x86_capability) || +test_bit (X86_FEATURE_CYRIX_ARR , boot_cpu_data.x86_capability) || +test_bit (X86_FEATURE_CENTAUR_MCR , boot_cpu_data.x86_capability) ) && addr >=__pa (high_memory);#else return addr >=__pa (high_memory);
drivers/char/n_tty.c +20 -20
@@ -538,7 +538,7 @@* handle specially, do shortcut processing to speed things * up. */ -if (!test_bit (c, &tty->process_char_map) || tty->lnext) { +if (!test_bit (c, tty->process_char_map) || tty->lnext) {finish_erasing (tty); tty->lnext =0 ;if (L_ECHO (tty)) {
@@ -659,7 +659,7 @@ handle_newline:spin_lock_irqsave (&tty->read_lock, flags); -set_bit (tty->read_head, &tty->read_flags); +set_bit (tty->read_head, tty->read_flags);put_tty_queue_nolock (c, tty); tty->canon_head = tty->read_head; tty->canon_data++;
@@ -811,38 +811,38 @@memset (tty->process_char_map,0 ,256 /8 );if (I_IGNCR (tty) ||I_ICRNL (tty)) -set_bit ('\r' , &tty->process_char_map); +set_bit ('\r' , tty->process_char_map);if (I_INLCR (tty)) -set_bit ('\n' , &tty->process_char_map); +set_bit ('\n' , tty->process_char_map);if (L_ICANON (tty)) { -set_bit (ERASE_CHAR (tty), &tty->process_char_map); -set_bit (KILL_CHAR (tty), &tty->process_char_map); -set_bit (EOF_CHAR (tty), &tty->process_char_map); -set_bit ('\n' , &tty->process_char_map); -set_bit (EOL_CHAR (tty), &tty->process_char_map); +set_bit (ERASE_CHAR (tty), tty->process_char_map); +set_bit (KILL_CHAR (tty), tty->process_char_map); +set_bit (EOF_CHAR (tty), tty->process_char_map); +set_bit ('\n' , tty->process_char_map); +set_bit (EOL_CHAR (tty), tty->process_char_map);if (L_IEXTEN (tty)) {set_bit (WERASE_CHAR (tty), - &tty->process_char_map); + tty->process_char_map);set_bit (LNEXT_CHAR (tty), - &tty->process_char_map); + tty->process_char_map);set_bit (EOL2_CHAR (tty), - &tty->process_char_map); + tty->process_char_map);if (L_ECHO (tty))set_bit (REPRINT_CHAR (tty), - &tty->process_char_map); + tty->process_char_map); } }if (I_IXON (tty)) { -set_bit (START_CHAR (tty), &tty->process_char_map); -set_bit (STOP_CHAR (tty), &tty->process_char_map); +set_bit (START_CHAR (tty), tty->process_char_map); +set_bit (STOP_CHAR (tty), tty->process_char_map); }if (L_ISIG (tty)) { -set_bit (INTR_CHAR (tty), &tty->process_char_map); -set_bit (QUIT_CHAR (tty), &tty->process_char_map); -set_bit (SUSP_CHAR (tty), &tty->process_char_map); +set_bit (INTR_CHAR (tty), tty->process_char_map); +set_bit (QUIT_CHAR (tty), tty->process_char_map); +set_bit (SUSP_CHAR (tty), tty->process_char_map); } -clear_bit (__DISABLED_CHAR, &tty->process_char_map); +clear_bit (__DISABLED_CHAR, tty->process_char_map);sti (); tty->raw =0 ; tty->real_raw =0 ;
@@ -1058,7 +1058,7 @@int eol; eol =test_and_clear_bit (tty->read_tail, - &tty->read_flags); + tty->read_flags); c = tty->read_buf[tty->read_tail];spin_lock_irqsave (&tty->read_lock, flags); tty->read_tail = ((tty->read_tail+1) &
drivers/char/random.c +1 -1
@@ -736,7 +736,7 @@int entropy =0 ;#if defined (__i386__) -if (test_bit (X86_FEATURE_TSC , &boot_cpu_data.x86_capability) ) { +if (test_bit (X86_FEATURE_TSC , boot_cpu_data.x86_capability) ) {__u32 high;rdtsc (time, high); num ^= high;
drivers/char/tty_ioctl.c +1 -1
@@ -188,7 +188,7 @@ nr = (head - tail) & (N_TTY_BUF_SIZE -1 );/* Skip EOF-chars.. */ while (head != tail) { -if (test_bit (tail, &tty->read_flags) && +if (test_bit (tail, tty->read_flags) && tty->read_buf[tail] == __DISABLED_CHAR) nr--; tail = (tail+1 ) & (N_TTY_BUF_SIZE -1 );
drivers/macintosh/adb.c +2 -2
@@ -77,7 +77,7 @@static int adb_got_sleep =0 ;static int adb_inited =0 ;static pid_t adb_probe_task_pid; -static int adb_probe_task_flag; +static unsigned long adb_probe_task_flag;static wait_queue_head_t adb_probe_task_wq;static int sleepy_trackpad;int __adb_probe_sync;
@@ -439,7 +439,7 @@ }static struct adb_request adb_sreq; -static int adb_sreq_lock;// Use semaphore ! */ +static unsigned long adb_sreq_lock;// Use semaphore ! */ int adb_request(struct adb_request *req, void (*done)(struct adb_request *),
drivers/usb/hub.c +2 -2
@@ -1120,7 +1120,7 @@ dev->devpath, sizeof(dev->descriptor), ret); -clear_bit (dev->devnum, &dev->bus->devmap.devicemap); +clear_bit (dev->devnum, dev->bus->devmap.devicemap); dev->devnum =-1 ;return -EIO ; }
@@ -1129,7 +1129,7 @@if (ret <0 ) {err ("unable to get configuration (error=%d)" , ret);usb_destroy_configuration (dev); -clear_bit (dev->devnum, &dev->bus->devmap.devicemap); +clear_bit (dev->devnum, dev->bus->devmap.devicemap); dev->devnum =-1 ;return 1 ; }
drivers/usb/usb.c +6 -6
@@ -1749,7 +1749,7 @@/* Free the device number and remove the /proc/bus/usb entry */ if (dev->devnum >0 ) { -clear_bit (dev->devnum, &dev->bus->devmap.devicemap); +clear_bit (dev->devnum, dev->bus->devmap.devicemap);usbfs_remove_device (dev);put_device (&dev->dev); }
@@ -2449,7 +2449,7 @@if (err <0 ) {err ("USB device not accepting new address=%d (error=%d)" , dev->devnum, err); -clear_bit (dev->devnum, &dev->bus->devmap.devicemap); +clear_bit (dev->devnum, dev->bus->devmap.devicemap); dev->devnum =-1 ;return 1 ; }
@@ -2462,7 +2462,7 @@err ("USB device not responding, giving up (error=%d)" , err); else err("USB device descriptor short read (expected %i, got %i)", 8, err); -clear_bit (dev->devnum, &dev->bus->devmap.devicemap); +clear_bit (dev->devnum, dev->bus->devmap.devicemap); dev->devnum =-1 ;return 1 ; }
@@ -2477,7 +2477,7 @@err ("USB device descriptor short read (expected %Zi, got %i)" ,sizeof (dev->descriptor), err); -clear_bit (dev->devnum, &dev->bus->devmap.devicemap); +clear_bit (dev->devnum, dev->bus->devmap.devicemap); dev->devnum =-1 ;return 1 ; }
@@ -2486,7 +2486,7 @@if (err <0 ) {err ("unable to get device %d configuration (error=%d)" , dev->devnum, err); -clear_bit (dev->devnum, &dev->bus->devmap.devicemap); +clear_bit (dev->devnum, dev->bus->devmap.devicemap); dev->devnum =-1 ;return 1 ; }
@@ -2496,7 +2496,7 @@if (err) {err ("failed to set device %d default configuration (error=%d)" , dev->devnum, err); -clear_bit (dev->devnum, &dev->bus->devmap.devicemap); +clear_bit (dev->devnum, dev->bus->devmap.devicemap); dev->devnum =-1 ;return 1 ; }
fs/devfs/util.c +4 -4
@@ -125,7 +125,7 @@struct major_list {spinlock_t lock; -__u32 bits[8 ]; +unsigned long bits[256 /BITS_PER_LONG ]; };/* Block majors already assigned:
@@ -212,7 +212,7 @@struct minor_list {int major; -__u32 bits[8 ]; +unsigned long bits[256 /BITS_PER_LONG ];struct minor_list *next; };
@@ -355,7 +355,7 @@ {int number;unsigned int length; -__u32 *bits; +unsigned long *bits;/* Get around stupid lack of semaphore initialiser */ spin_lock (&space->init_lock);
@@ -382,7 +382,7 @@ } space->num_free = (length - space->length) <<3 ; space->bits = bits; -memset (bits + space->length,0 , length - space->length); +memset ((void *)bits + space->length,0 , length - space->length); space->length = length; } number =find_first_zero_bit (space->bits, space->length <<3 );
fs/open.c +1 -1
@@ -704,7 +704,7 @@write_lock (&files->file_lock); repeat: - fd =find_next_zero_bit (files->open_fds, + fd =find_next_zero_bit (files->open_fds->fds_bits, files->max_fdset, files->next_fd);
include/asm-i386/bitops.h +35 -30
186 lines not on this page · raw message
include/asm-i386/bugs.h +1 -1
@@ -194,7 +194,7 @@*/ #if defined(CONFIG_X86_LOCAL_APIC ) && defined(CONFIG_X86_GOOD_APIC )if (boot_cpu_data.x86_vendor ==X86_VENDOR_INTEL - &&test_bit (X86_FEATURE_APIC , &boot_cpu_data.x86_capability) + &&test_bit (X86_FEATURE_APIC , boot_cpu_data.x86_capability) && boot_cpu_data.x86 ==5 && boot_cpu_data.x86_model ==2 && (boot_cpu_data.x86_mask <6 || boot_cpu_data.x86_mask ==11 ))
include/asm-i386/pgtable.h +4 -4
@@ -288,10 +288,10 @@static inline pte_t pte_mkyoung (pte_t pte) { (pte).pte_low |= _PAGE_ACCESSED;return pte; }static inline pte_t pte_mkwrite (pte_t pte) { (pte).pte_low |= _PAGE_RW;return pte; } -static inline int ptep_test_and_clear_dirty (pte_t *ptep) {return test_and_clear_bit (_PAGE_BIT_DIRTY, ptep); } -static inline int ptep_test_and_clear_young (pte_t *ptep) {return test_and_clear_bit (_PAGE_BIT_ACCESSED, ptep); } -static inline void ptep_set_wrprotect (pte_t *ptep) {clear_bit (_PAGE_BIT_RW, ptep); } -static inline void ptep_mkdirty (pte_t *ptep) {set_bit (_PAGE_BIT_DIRTY, ptep); } +static inline int ptep_test_and_clear_dirty (pte_t *ptep) {return test_and_clear_bit (_PAGE_BIT_DIRTY, &ptep->pte_low); } +static inline int ptep_test_and_clear_young (pte_t *ptep) {return test_and_clear_bit (_PAGE_BIT_ACCESSED, &ptep->pte_low); } +static inline void ptep_set_wrprotect (pte_t *ptep) {clear_bit (_PAGE_BIT_RW, &ptep->pte_low); } +static inline void ptep_mkdirty (pte_t *ptep) {set_bit (_PAGE_BIT_DIRTY, &ptep->pte_low); }/* * Conversion functions: convert a page and protection to a page entry,
include/asm-i386/processor.h +1 -1
@@ -40,7 +40,7 @@char hard_math;char rfu;int cpuid_level;/* Maximum supported CPUID level, -1=no CPUID */ -__u32 x86_capability[NCAPINTS ]; +unsigned long x86_capability[NCAPINTS ];char x86_vendor_id[16 ];char x86_model_id[64 ];int x86_cache_size;/* in KB - valid for CPUS which support this
include/asm-i386/thread_info.h +1 -1
@@ -23,7 +23,7 @@struct thread_info {struct task_struct *task;/* main task structure */ struct exec_domain *exec_domain;/* execution domain */ -__u32 flags;/* low level flags */ +unsigned long flags;/* low level flags */ __u32 cpu;/* current CPU */ __s32 preempt_count;/* 0 => preemptable, <0 => BUG */
include/asm-ppc/bitops.h +19 -19
@@ -30,7 +30,7 @@* These used to be if'd out here because using : "cc" as a constraint * resulted in errors from egcs. Things appear to be OK with gcc-2.95. */ -static __inline__void set_bit (int nr,volatile void * addr) +static __inline__void set_bit (int nr,volatile unsigned long * addr) {unsigned long old;unsigned long mask =1 << (nr &0x1f );
@@ -50,7 +50,7 @@/* * non-atomic version */ -static __inline__void __set_bit (int nr,volatile void *addr) +static __inline__void __set_bit (int nr,volatile unsigned long *addr) {unsigned long mask =1 << (nr &0x1f );unsigned long *p = ((unsigned long *)addr) + (nr >>5 );
@@ -64,7 +64,7 @@#define smp_mb__before_clear_bit () smp_mb()#define smp_mb__after_clear_bit () smp_mb() -static __inline__void clear_bit (int nr,volatile void *addr) +static __inline__void clear_bit (int nr,volatile unsigned long *addr) {unsigned long old;unsigned long mask =1 << (nr &0x1f );
@@ -84,7 +84,7 @@/* * non-atomic version */ -static __inline__void __clear_bit (int nr,volatile void *addr) +static __inline__void __clear_bit (int nr,volatile unsigned long *addr) {unsigned long mask =1 << (nr &0x1f );unsigned long *p = ((unsigned long *)addr) + (nr >>5 );
@@ -92,7 +92,7 @@ *p &= ~mask; } -static __inline__void change_bit (int nr,volatile void *addr) +static __inline__void change_bit (int nr,volatile unsigned long *addr) {unsigned long old;unsigned long mask =1 << (nr &0x1f );
@@ -112,7 +112,7 @@/* * non-atomic version */ -static __inline__void __change_bit (int nr,volatile void *addr) +static __inline__void __change_bit (int nr,volatile unsigned long *addr) {unsigned long mask =1 << (nr &0x1f );unsigned long *p = ((unsigned long *)addr) + (nr >>5 );
@@ -123,7 +123,7 @@/* * test_and_*_bit do imply a memory barrier (?) */ -static __inline__int test_and_set_bit (int nr,volatile void *addr) +static __inline__int test_and_set_bit (int nr,volatile unsigned long *addr) {unsigned int old, t;unsigned int mask =1 << (nr &0x1f );
@@ -146,7 +146,7 @@/* * non-atomic version */ -static __inline__int __test_and_set_bit (int nr,volatile void *addr) +static __inline__int __test_and_set_bit (int nr,volatile unsigned long *addr) {unsigned long mask =1 << (nr &0x1f );unsigned long *p = ((unsigned long *)addr) + (nr >>5 );
@@ -156,7 +156,7 @@return (old & mask) !=0 ; } -static __inline__int test_and_clear_bit (int nr,volatile void *addr) +static __inline__int test_and_clear_bit (int nr,volatile unsigned long *addr) {unsigned int old, t;unsigned int mask =1 << (nr &0x1f );
@@ -179,7 +179,7 @@/* * non-atomic version */ -static __inline__int __test_and_clear_bit (int nr,volatile void *addr) +static __inline__int __test_and_clear_bit (int nr,volatile unsigned long *addr) {unsigned long mask =1 << (nr &0x1f );unsigned long *p = ((unsigned long *)addr) + (nr >>5 );
@@ -189,7 +189,7 @@return (old & mask) !=0 ; } -static __inline__int test_and_change_bit (int nr,volatile void *addr) +static __inline__int test_and_change_bit (int nr,volatile unsigned long *addr) {unsigned int old, t;unsigned int mask =1 << (nr &0x1f );
@@ -212,7 +212,7 @@/* * non-atomic version */ -static __inline__int __test_and_change_bit (int nr,volatile void *addr) +static __inline__int __test_and_change_bit (int nr,volatile unsigned long *addr) {unsigned long mask =1 << (nr &0x1f );unsigned long *p = ((unsigned long *)addr) + (nr >>5 );
@@ -222,7 +222,7 @@return (old & mask) !=0 ; } -static __inline__ int test_bit(int nr, __const__ volatile void *addr) +static __inline__ int test_bit(int nr, __const__ volatile unsigned long *addr) { __const__ unsigned int *p = (__const__ unsigned int *) addr;
@@ -230,7 +230,7 @@ }/* Return the bit position of the most significant 1 bit in a word */ -static __inline__int __ilog2 (unsigned int x) +static __inline__int __ilog2 (unsigned long x) {int lz;
@@ -238,7 +238,7 @@return 31 - lz; } -static __inline__int ffz (unsigned int x) +static __inline__int ffz (unsigned long x) {if ((x = ~x) ==0 )return 32 ;
@@ -296,7 +296,7 @@* @offset: The bitnumber to start searching at * @size: The maximum size to search */ -static __inline__unsigned long find_next_bit (void *addr, +static __inline__unsigned long find_next_bit (unsigned long *addr,unsigned long size,unsigned long offset) {unsigned int *p = ((unsigned int *) addr) + (offset >>5 );
@@ -353,7 +353,7 @@#define find_first_zero_bit (addr, size) \ find_next_zero_bit((addr), (size), 0) -static __inline__unsigned long find_next_zero_bit (void * addr, +static __inline__unsigned long find_next_zero_bit (unsigned long * addr,unsigned long size,unsigned long offset) {unsigned int * p = ((unsigned int *) addr) + (offset >>5 );
@@ -394,8 +394,8 @@#ifdef __KERNEL__ -#define ext2_set_bit (nr, addr) __test_and_set_bit((nr) ^ 0x18, addr) -#define ext2_clear_bit (nr, addr) __test_and_clear_bit((nr) ^ 0x18, addr) +#define ext2_set_bit (nr, addr) __test_and_set_bit((nr) ^ 0x18, (unsigned long *)(addr)) +#define ext2_clear_bit (nr, addr) __test_and_clear_bit((nr) ^ 0x18, (unsigned long *)(addr)) static __inline__ int ext2_test_bit(int nr, __const__ void * addr) {
include/linux/devfs_fs_kernel.h +1 -1
@@ -54,7 +54,7 @@unsigned char sem_initialised;unsigned int num_free;/* Num free in bits */ unsigned int length;/* Array length in bytes */ -__u32 *bits; +unsigned long *bits;struct semaphore semaphore; };
include/linux/hfs_sysdep.h +3 -3
@@ -200,16 +200,16 @@#endif static inline int hfs_clear_bit (int bitnr,hfs_u32 *lword) { -return test_and_clear_bit (BITNR (bitnr), lword); +return test_and_clear_bit (BITNR (bitnr), (unsigned long *)lword); }static inline int hfs_set_bit (int bitnr,hfs_u32 *lword) { -return test_and_set_bit (BITNR (bitnr), lword); +return test_and_set_bit (BITNR (bitnr), (unsigned long *)lword); }static inline int hfs_test_bit (int bitnr,const hfs_u32 *lword) {/* the kernel should declare the second arg of test_bit as const */ -return test_bit (BITNR (bitnr), (void *)lword); +return test_bit (BITNR (bitnr), (unsigned long *)lword); }#undef BITNR
include/linux/sunrpc/svcsock.h +1 -1
@@ -22,7 +22,7 @@struct svc_serv * sk_server;/* service for this socket */ unsigned char sk_inuse;/* use count */ -unsigned int sk_flags; +unsigned long sk_flags;#define SK_BUSY 0/* enqueued/receiving */ #define SK_CONN 1/* conn pending */ #define SK_CLOSE 2/* dead or dying */
kernel/sched.c +2 -0
@@ -1451,8 +1451,10 @@set_tsk_need_resched (idle);__restore_flags (flags); +#ifdef CONFIG_PREEMPT /* Set the preempt count _outside_ the spinlocks! */ idle->thread_info->preempt_count = (idle->lock_depth >=0 ); +#endif }extern void init_timervecs (void );
mm/swapfile.c +2 -2
@@ -959,7 +959,7 @@ p->lowest_bit =0 ; p->highest_bit =0 ;for (i =1 ; i <8 *PAGE_SIZE ; i++) { -if (test_bit (i,(char *) swap_header)) { +if (test_bit (i,(unsigned long *) swap_header)) {if (!p->lowest_bit) p->lowest_bit = i; p->highest_bit = i;
@@ -974,7 +974,7 @@ goto bad_swap; }for (i =1 ; i < maxpages ; i++) { -if (test_bit (i,(char *) swap_header)) +if (test_bit (i,(unsigned long *) swap_header)) p->swap_map[i] =0 ;else p->swap_map[i] =SWAP_MAP_BAD ;
From: Rusty Russell <[email protected]>
Date: Sat, 16 Mar 2002 14:08:08 +1100
+#ifdef CONFIG_PREEMPT
/* Set the preempt count _outside_ the spinlocks! */
idle->thread_info->preempt_count = (idle->lock_depth >= 0);+#endif
This part of your patch has to go. Every port must provide the preempt_count member of thread_info regardless of the CONFIG_PREEMPT setting.
Quoted 4 lines · Rusty Russell
Richard: 3 bugs in devfs. Particularly note that the memset was bogus. I can't convince myself that your memcpy & memset stuff is right anyway, given that you can ONLY treat them as unsigned longs (ie. bit 31 will be in byte 0 or byte 3, depending on endianness).
Yes, the memset is bogus because I didn't cast the pointer to a char * or void *. The memcpy should be fine, though. And so should everything else, because the bitfield array is allocated in 16 byte multiples. So there should be no issues with big vs. little endian, since memset/memcpy operations are done in blocks of sufficient alignment.
So, really, the only problem is the stupid lack of a cast. Unless you noticed some other problem?
Regards, Richard.... Permanent: [email protected] Current: [email protected]
In message <[email protected]> you write:
Quoted 8 lines
Quoted 4 lines · Rusty Russell
Richard: 3 bugs in devfs. Particularly note that the memset was bogus. I can't convince myself that your memcpy & memset stuff is right anyway, given that you can ONLY treat them as unsigned longs (ie. bit 31 will be in byte 0 or byte 3, depending on endianness).
Yes, the memset is bogus because I didn't cast the pointer to a char * or void *.
Yes.
Quoted 3 lines
The memcpy should be fine, though. And so should everything else, because the bitfield array is allocated in 16 byte multiples.
No:
struct major_list
{
spinlock_t lock;- __u32 bits[8];
+ unsigned long bits[256 / BITS_PER_LONG];
};
/* Block majors already assigned:
@@ -212,7 +212,7 @@
struct minor_list
{
int major;
- __u32 bits[8];
+ unsigned long bits[256 / BITS_PER_LONG];
struct minor_list *next;};
These changed are required because otherwise you try to do set_bit on something not aligned as a long on all archs.
(Turning to the gallery) I assert: if you're going to do bitops on it, make it a "unsigned long".
Quoted 3 lines
So there should be no issues with big vs. little endian, since memset/memcpy operations are done in blocks of sufficient alignment.
I think you're right, as long as length is always a multiple of sizeof(long). This is not obvious from this hunk of code alone, which is why I queried it...
if (space->num_free < 1)
{
if (space->length < 16) length = 16;
else length = space->length << 1;
if ( ( bits = vmalloc (length) ) == NULL )
{
up (&space->semaphore);
return -ENOMEM;
}
if (space->bits != NULL)
{
memcpy (bits, space->bits, space->length);
vfree (space->bits);
}
space->num_free = (length - space->length) << 3;
space->bits = bits;
memset (bits + space->length, 0, length - space->length);
space->length = length;
}Thanks!
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
Quoted 17 lines · Rusty Russell
In message <[email protected]> you write:
Quoted 8 lines
Quoted 4 lines · Rusty Russell
Richard: 3 bugs in devfs. Particularly note that the memset was bogus. I can't convince myself that your memcpy & memset stuff is right anyway, given that you can ONLY treat them as unsigned longs (ie. bit 31 will be in byte 0 or byte 3, depending on endianness).
Yes, the memset is bogus because I didn't cast the pointer to a char * or void *.
Yes.
Quoted 3 lines
The memcpy should be fine, though. And so should everything else, because the bitfield array is allocated in 16 byte multiples.
No:
[...]
Quoted 2 lines
These changed are required because otherwise you try to do
set_biton something not aligned as a long on all archs.
But of course. I'm not denying that. Naturally the type should be changed. I thought that was obvious so I didn't bother agreeing. But in fact, it already *is* aligned on a long boundary. Better, in fact. It's aligned on a 16 byte boundary. Even though the type was __u32.
Quoted 2 lines
(Turning to the gallery) I assert: if you're going to do bitops on it, make it a "unsigned long".
Agreed.
Quoted 4 lines
Quoted 3 lines
So there should be no issues with big vs. little endian, since memset/memcpy operations are done in blocks of sufficient alignment.
Quoted 8 lines
I think you're right, as long as length is always a multiple of sizeof(long). This is not obvious from this hunk of code alone, which is why I queried it...
if (space->num_free < 1) { if (space->length < 16) length = 16; else length = space->length << 1;
Assuming sizeof (long) <= 16 bytes, then length will always be a multiple of sizeof (long). So, even for a 128 bit CPU, this code is fine. It might break down on a 256 bit CPU...
Anyway, it looks like we're in agreement.
Regards, Richard.... Permanent: [email protected] Current: [email protected]
In message <[email protected]> you write:
Quoted 8 lines
Quoted 2 lines
These changed are required because otherwise you try to do
set_biton something not aligned as a long on all archs.But of course. I'm not denying that. Naturally the type should be changed. I thought that was obvious so I didn't bother agreeing. But in fact, it already *is* aligned on a long boundary. Better, in fact. It's aligned on a 16 byte boundary. Even though the type was
__u32.
I'm confused:
@@ -212,7 +212,7 @@
struct minor_list
{
int major;
- __u32 bits[8];
+ unsigned long bits[256 / BITS_PER_LONG];
struct minor_list *next;};
How, exactly, did "bits" end up on a 16-bute boundary before this patch?
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
Quoted 21 lines · Rusty Russell
In message <[email protected]> you write:
Quoted 8 lines
Quoted 2 lines
These changed are required because otherwise you try to do
set_biton something not aligned as a long on all archs.But of course. I'm not denying that. Naturally the type should be changed. I thought that was obvious so I didn't bother agreeing. But in fact, it already *is* aligned on a long boundary. Better, in fact. It's aligned on a 16 byte boundary. Even though the type was
__u32.I'm confused:
fs/devfs/util.c +1
-1@@ -212,7 +212,7 @@struct minor_list {int major; -__u32 bits[8 ]; +unsigned long bits[256 /BITS_PER_LONG ];struct minor_list *next; };
Quoted 2 lines
How, exactly, did "bits" end up on a 16-bute boundary before this patch?
Oh, I wasn't talking about this part of the code at all. That's actually broken in other ways as well (see the other thread where I replied to Carsten Otte).
I'm just talking about the devfs_alloc_unique_number() implementation, which *also* was using an array of __u32. That's where you cast doubt upon the memset/memcpy, a few emails ago. Apart from the missing cast to void *, I assert that devfs_alloc_unique_number() is 100% correct. The type of the bitfield should be changed, of course, for cleanliness reasons, but it isn't actually a bug.
Clear now? Or should I just telephone you for a few minutes rather than go back and forth in a dozen more emails? :-)
Regards, Richard.... Permanent: [email protected] Current: [email protected]
Quoted 12 lines · David S. Miller
From: Rusty Russell <[email protected]>
Date: Sat, 16 Mar 2002 14:08:08 +1100+#ifdef
CONFIG_PREEMPT
/* Set the preempt count _outside_ the spinlocks! */idle->thread_info->preempt_count= (idle->lock_depth>= 0);
+#endifThis part of your patch has to go. Every port must provide the
preempt_countmember ofthread_inforegardless of theCONFIG_PREEMPTsetting.
Even if the port doesn't support CONFIG_PREEMPT at all?
Jeff
From: Jeff Garzik <[email protected]>
Date: Mon, 18 Mar 2002 01:03:30 -0500
Even if the port doesn't support CONFIG_PREEMPT at all?
Linus and myself plan on using it for something else (something akin to a softirq+otherstuff count).
In message <[email protected]> you write:
Quoted 5 lines
Quoted 3 lines · David S. Miller
From: Rusty Russell <[email protected]>
Date: Sat, 16 Mar 2002 14:08:08 +1100
Quoted 8 lines
Quoted 8 lines
+#ifdef
CONFIG_PREEMPT
/* Set the preempt count _outside_ the spinlocks! */idle->thread_info->preempt_count= (idle->lock_depth>= 0);
+#endifThis part of your patch has to go. Every port must provide the
preempt_countmember ofthread_inforegardless of theCONFIG_PREEMPTsetting.
Sorry, slipped in so I could compile on PPC. Discard that part of the patch please.
Thanks,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
Quoted 4 lines
nfs is broken in 2.5 ATM because it does
set_biton an "int".
Can be *please* just bite the bullet and change the prototype on these ops so we stop seeing the same mistakes over and over?
I present to you Exhibit A:
include/linux/sunrpc/svcsock.h +1 -1
@@ -22,7 +22,7 @@struct svc_serv * sk_server;/* service for this socket */ unsigned char sk_inuse;/* use count */ -unsigned int sk_flags; +unsigned long sk_flags;#define SK_BUSY 0/* enqueued/receiving */ #define SK_CONN 1/* conn pending */ #define SK_CLOSE 2/* dead or dying */