Re: [PATCH] linux-2.417 devfs 64bit portablility issue

["Carsten Otte" <COTTE@de.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]

Richard Gooch writes:
>Except that devfs_register_???dev() (which are in fact minor
>variations on the register_???dev() calls) *do not* avoid assigned
>majors. That is why I wrote devfs_alloc_major() in the first place.
To me, the disadvantage of this soloution is that if a device driver has
called devfs_register_blkdev with major 0 (automatic allocation) and
another device driver calls devfs_alloc_major, the same major number may
be assigned twice. Same situation is vice versa (major has been allocated,
devfs_register_blkdev with major 0 gives the same major to a different
driver).

>And while I do think that register_???dev() should in fact do just
>what devfs_alloc_major() does, that's not a battle I care to fight. By
>writing devfs_alloc_major(), this functionality is optional, and I can
>avoid a whole pile of stupid flaming.
I reworked my initial patch in order to
- avoid setting up the array on initialisation (since you dislike this)
- have a portable soloution that works on any arch
- avoid assigning staticaly allocated majors with any devfs_* functions
- avoid assigning the same major twice with devfs_* functions

Please do have a look at this patch (sorry, attached again) and let me
know what you think of including this solution.

w/kind regards
Carsten Otte
(See attached file: linux-2.4.17-devfs_fixup.diff)

[-- Attachment #2: linux-2.4.17-devfs_fixup.diff --]
[-- Type: application/octet-stream, Size: 10707 bytes --]

diff -ruN linux-2.3-SDMANY/fs/devfs/base.c linux-2.3-SDMANY-DEVFSFIX/fs/devfs/base.c
--- linux-2.3-SDMANY/fs/devfs/base.c	Wed Feb 13 13:25:50 2002
+++ linux-2.3-SDMANY-DEVFSFIX/fs/devfs/base.c	Thu Feb 21 11:12:26 2002
@@ -2201,12 +2201,21 @@
 			   struct file_operations *fops)
 {
     unsigned int new_major;
-    if (boot_options & OPTION_ONLY) return 0;
-    if (major)
-	new_major = major;
-    else
-	new_major = devfs_alloc_major (DEVFS_SPECIAL_CHR);
-    return register_chrdev (new_major, name, fops);
+    int result;
+    if (boot_options & OPTION_ONLY) 
+	    return 0;
+    new_major = __devfs_register_major (major, DEVFS_SPECIAL_CHR);
+    if (new_major < 0) 
+	    return new_major;
+    result=register_chrdev (new_major, name, fops);
+    if (result < 0) {
+	    __devfs_unregister_major (major, DEVFS_SPECIAL_CHR);
+	    return result;
+    }
+    if (major) 
+	    return 0;
+    else 
+	    return new_major;
 }   /*  End Function devfs_register_chrdev  */
 
 
@@ -2225,12 +2234,21 @@
 			   struct block_device_operations *bdops)
 {
     unsigned int new_major;
-    if (boot_options & OPTION_ONLY) return 0;
-    if (major)
-	new_major = major;
-    else
-	new_major = devfs_alloc_major (DEVFS_SPECIAL_BLK);
-    return register_blkdev (new_major, name, bdops);
+    int result;
+    if (boot_options & OPTION_ONLY) 
+	    return 0;
+    new_major = __devfs_register_major (major, DEVFS_SPECIAL_BLK);
+    if (new_major < 0)
+	    return new_major;
+    result=register_blkdev (new_major, name, bdops);
+    if (result < 0) {
+	    __devfs_unregister_major (major, DEVFS_SPECIAL_BLK);
+	    return result;
+    }
+    if (major) 
+	    return 0;
+    else 
+	    return new_major;
 }   /*  End Function devfs_register_blkdev  */
 
 
@@ -2246,8 +2264,9 @@
 
 int devfs_unregister_chrdev (unsigned int major, const char *name)
 {
-    if (boot_options & OPTION_ONLY) return 0;
-    devfs_dealloc_major (major, DEVFS_SPECIAL_CHR);
+    if (boot_options & OPTION_ONLY) 
+	    return 0;
+    __devfs_unregister_major (major, DEVFS_SPECIAL_CHR);
     return unregister_chrdev (major, name);
 }   /*  End Function devfs_unregister_chrdev  */
 
@@ -2264,8 +2283,9 @@
 
 int devfs_unregister_blkdev (unsigned int major, const char *name)
 {
-    if (boot_options & OPTION_ONLY) return 0;
-    devfs_dealloc_major (major, DEVFS_SPECIAL_BLK);
+    if (boot_options & OPTION_ONLY) 
+	    return 0;
+    __devfs_unregister_major (major, DEVFS_SPECIAL_BLK);
     return unregister_blkdev (major, name);
 }   /*  End Function devfs_unregister_blkdev  */
 
diff -ruN linux-2.3-SDMANY/fs/devfs/util.c linux-2.3-SDMANY-DEVFSFIX/fs/devfs/util.c
--- linux-2.3-SDMANY/fs/devfs/util.c	Fri Oct 26 20:00:52 2001
+++ linux-2.3-SDMANY-DEVFSFIX/fs/devfs/util.c	Wed Feb 20 19:12:18 2002
@@ -185,43 +185,62 @@
 
 struct major_list
 {
-    spinlock_t lock;
-    __u32 bits[8];
+	spinlock_t lock;
+	unsigned char state[256];
 };
 
+
+
 /*  Block majors already assigned:
     0-3, 7-9, 11-63, 65-99, 101-113, 120-127, 199, 201, 240-255
     Total free: 122
 */
+
 static struct major_list block_major_list =
-{SPIN_LOCK_UNLOCKED,
-    {0xfffffb8f,  /*  Majors 0   to 31   */
-     0xffffffff,  /*  Majors 32  to 63   */
-     0xfffffffe,  /*  Majors 64  to 95   */
-     0xff03ffef,  /*  Majors 96  to 127  */
-     0x00000000,  /*  Majors 128 to 159  */
-     0x00000000,  /*  Majors 160 to 191  */
-     0x00000280,  /*  Majors 192 to 223  */
-     0xffff0000}  /*  Majors 224 to 255  */
+{SPIN_LOCK_UNLOCKED, 
+ { [  0 ...   3] = DEVFS_MAJOR_STATIC,
+   [  4 ...   6] = DEVFS_MAJOR_FREE,
+   [  7 ...   9] = DEVFS_MAJOR_STATIC,
+   [ 10 ...  10] = DEVFS_MAJOR_FREE,
+   [ 11 ...  63] = DEVFS_MAJOR_STATIC,
+   [ 64 ...  64] = DEVFS_MAJOR_FREE,
+   [ 65 ...  99] = DEVFS_MAJOR_STATIC,
+   [100 ... 100] = DEVFS_MAJOR_FREE,
+   [101 ... 113] = DEVFS_MAJOR_STATIC,
+   [114 ... 119] = DEVFS_MAJOR_FREE,
+   [120 ... 127] = DEVFS_MAJOR_STATIC,
+   [128 ... 198] = DEVFS_MAJOR_FREE,
+   [199 ... 199] = DEVFS_MAJOR_STATIC,
+   [200 ... 200] = DEVFS_MAJOR_FREE,
+   [201 ... 201] = DEVFS_MAJOR_STATIC,
+   [202 ... 239] = DEVFS_MAJOR_FREE,
+   [240 ... 255] = DEVFS_MAJOR_STATIC
+ }
 };
 
 /*  Char majors already assigned:
     0-7, 9-151, 154-158, 160-211, 216-221, 224-230, 240-255
     Total free: 19
 */
+
 static struct major_list char_major_list =
 {SPIN_LOCK_UNLOCKED,
-    {0xfffffeff,  /*  Majors 0   to 31   */
-     0xffffffff,  /*  Majors 32  to 63   */
-     0xffffffff,  /*  Majors 64  to 95   */
-     0xffffffff,  /*  Majors 96  to 127  */
-     0x7cffffff,  /*  Majors 128 to 159  */
-     0xffffffff,  /*  Majors 160 to 191  */
-     0x3f0fffff,  /*  Majors 192 to 223  */
-     0xffff007f}  /*  Majors 224 to 255  */
+ { [  0 ...   7] = DEVFS_MAJOR_STATIC,
+   [  8 ...   8] = DEVFS_MAJOR_FREE,
+   [  9 ... 151] = DEVFS_MAJOR_STATIC,
+   [152 ... 153] = DEVFS_MAJOR_FREE,
+   [154 ... 158] = DEVFS_MAJOR_STATIC,
+   [159 ... 159] = DEVFS_MAJOR_FREE,
+   [160 ... 211] = DEVFS_MAJOR_STATIC,
+   [212 ... 215] = DEVFS_MAJOR_FREE,
+   [216 ... 221] = DEVFS_MAJOR_STATIC,
+   [222 ... 223] = DEVFS_MAJOR_FREE,
+   [224 ... 230] = DEVFS_MAJOR_STATIC,
+   [231 ... 239] = DEVFS_MAJOR_FREE,
+   [240 ... 255] = DEVFS_MAJOR_STATIC
+ }
 };
 
-
 /**
  *	devfs_alloc_major - Allocate a major number.
  *	@type: The type of the major (DEVFS_SPECIAL_CHR or DEVFS_SPECIAL_BLK)
@@ -237,14 +256,63 @@
 
     list = (type == DEVFS_SPECIAL_CHR) ? &char_major_list : &block_major_list;
     spin_lock (&list->lock);
-    major = find_first_zero_bit (list->bits, 256);
-    if (major < 256) __set_bit (major, list->bits);
-    else major = -1;
+    for (major=0; major<256; major++)
+	    if (list->state[major] == DEVFS_MAJOR_FREE)
+		    break;
+    if (major < 256) 
+	    list->state[major] = DEVFS_MAJOR_ALLOCED;
+    else 
+	    major = -1;
     spin_unlock (&list->lock);
     return major;
 }   /*  End Function devfs_alloc_major  */
 EXPORT_SYMBOL(devfs_alloc_major);
 
+/**
+ *	__devfs_register_major - Internaly called by devfs_register_*dev
+ *	@type: The type of the major (DEVFS_SPECIAL_CHR or DEVFS_SPECIAL_BLK)
+ *       @major: The major number or 0 for automatic selection
+
+ *	Returns the allocated major, else -1 if none are available.
+ *	This routine is thread safe and does not block.
+ */
+
+int __devfs_register_major (int major, char type)
+{
+    struct major_list *list;
+
+    list = (type == DEVFS_SPECIAL_CHR) ? &char_major_list : &block_major_list;
+    spin_lock (&list->lock);
+    switch (major) {
+    case 0:
+	    for (major=0; (major < 256) && (list->state[major] != DEVFS_MAJOR_FREE); major++);
+	    if (major < 256) 
+		    list->state[major] = DEVFS_MAJOR_REGISTERED;
+	    else 
+		    major = -1;
+	    break;
+    default:
+	    switch (list->state[major]) {
+	    case DEVFS_MAJOR_REGISTERED:
+		    major = -1;
+		    break;
+	    case DEVFS_MAJOR_FREE:
+		    list->state[major] = DEVFS_MAJOR_REGISTERED;
+		    break;
+	    case DEVFS_MAJOR_STATIC:
+	    case DEVFS_MAJOR_ALLOCED:
+		    /* fine with this one */
+		    break;
+	    default:
+		    /* inconsistency in list->state array */
+		    spin_unlock(&list->lock);
+		    BUG();
+	    }
+    }
+    spin_unlock (&list->lock);
+    return major;
+}   /*  End Function __devfs_register_major  */
+
 
 /**
  *	devfs_dealloc_major - Deallocate a major number.
@@ -255,20 +323,55 @@
 
 void devfs_dealloc_major (char type, int major)
 {
-    int was_set;
     struct major_list *list;
 
-    if (major < 0) return;
+    if (major < 0) 
+	    return;
     list = (type == DEVFS_SPECIAL_CHR) ? &char_major_list : &block_major_list;
     spin_lock (&list->lock);
-    was_set = __test_and_clear_bit (major, list->bits);
+    if (list->state[major] != DEVFS_MAJOR_ALLOCED)
+	printk (KERN_ERR __FUNCTION__ "(): major %d was not alloced. State was: %d\n",
+		major, list->state[major]);
+    list->state[major] = DEVFS_MAJOR_FREE;
     spin_unlock (&list->lock);
-    if (!was_set)
-	printk (KERN_ERR __FUNCTION__ "(): major %d was already free\n",
-		major);
 }   /*  End Function devfs_dealloc_major  */
 EXPORT_SYMBOL(devfs_dealloc_major);
 
+/**
+ *	__devfs_unregister_major - Internaly called from devfs_unregister_*dev
+ *	@type: The type of the major (DEVFS_SPECIAL_CHR or DEVFS_SPECIAL_BLK)
+ *	@major: The major number.
+ *	This routine is thread safe and does not block.
+ */
+
+void __devfs_unregister_major (char type, int major)
+{
+    struct major_list *list;
+
+    if (major < 0) 
+	    return;
+    list = (type == DEVFS_SPECIAL_CHR) ? &char_major_list : &block_major_list;
+    spin_lock (&list->lock);
+    switch (list->state[major]) {
+    case DEVFS_MAJOR_FREE:
+	    printk (KERN_ERR __FUNCTION__ "(): major %d was free\n",
+		    major);
+	    break;
+    case DEVFS_MAJOR_ALLOCED:
+    case DEVFS_MAJOR_STATIC:
+	    /* major is still in use -> do not free */
+	    break;
+    case DEVFS_MAJOR_REGISTERED:
+	    list->state[major] = DEVFS_MAJOR_FREE;
+	    break;
+    default:
+	    /* inconsistency in list->state array */
+	    spin_unlock (&list->lock);
+	    BUG();
+    }
+    spin_unlock (&list->lock);
+}   /*  End Function devfs_unregister_major  */
+
 
 struct minor_list
 {
diff -ruN linux-2.3-SDMANY/include/linux/devfs_fs_kernel.h linux-2.3-SDMANY-DEVFSFIX/include/linux/devfs_fs_kernel.h
--- linux-2.3-SDMANY/include/linux/devfs_fs_kernel.h	Thu Dec 27 19:53:07 2001
+++ linux-2.3-SDMANY-DEVFSFIX/include/linux/devfs_fs_kernel.h	Wed Feb 20 18:41:39 2002
@@ -20,7 +20,6 @@
        (devfs_get_maj_min(devfs_get_handle_from_inode((inode)),NULL,&m)==0) \
       ) ? m : MINOR((inode)->r_dev); })
 
-
 #define DEVFS_FL_NONE           0x000 /* This helps to make code more readable
 				       */
 #define DEVFS_FL_AUTO_OWNER     0x001 /* When a closed inode is opened the
@@ -46,6 +45,10 @@
 
 typedef struct devfs_entry * devfs_handle_t;
 
+#define DEVFS_MAJOR_FREE       0
+#define DEVFS_MAJOR_STATIC     1
+#define DEVFS_MAJOR_ALLOCED    2
+#define DEVFS_MAJOR_REGISTERED 3
 
 #ifdef CONFIG_BLK_DEV_INITRD
 #  define ROOT_DEVICE_NAME ((real_root_dev ==ROOT_DEV) ? root_device_name:NULL)
@@ -115,8 +118,11 @@
 				   unsigned int flags, unsigned int major,
 				   unsigned int minor_start,
 				   umode_t mode, void *ops, void *info);
+extern void devfs_init_major_lists (void);
 extern int devfs_alloc_major (char type);
 extern void devfs_dealloc_major (char type, int major);
+extern int __devfs_register_major (int major, char type);
+extern void __devfs_unregister_major (char type, int major);
 extern kdev_t devfs_alloc_devnum (char type);
 extern void devfs_dealloc_devnum (char type, kdev_t devnum);
 extern int devfs_alloc_unique_number (struct unique_numspace *space);