[PATCH] linux-2.417 devfs 64bit portablility issue

[PATCH] linux-2.417 devfs 64bit portablility issue

[Carsten Otte]

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

Hi Richard, Hi List-Readers!

In linux-2.4.17/fs/devfs/util.c, I found the following code:
struct major_list
{
    spinlock_t lock;
    __u32 bits[8];
};

/*  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  */
};

Afterwards, the block_major_list.bits is processed using
find_first_zero_bit & set_bit out of asm/bitops.h.
Since bitops are only defined for the datatype long, this does
only work on 32-bit architectures (on 64 bit data gets
incorrectly alligned -not on 8byte boundary & the ordering of
the data is incorrect).
I attached a patch that should fix it for all architectures.
(See attached file: linux-2.4.17-devfs_fixup.diff)

with kind regards
Carsten Otte

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

diff -ruN linux-2.4.17/fs/devfs/base.c linux-2.4.17-fixup/fs/devfs/base.c
--- linux-2.4.17/fs/devfs/base.c	Thu Dec 27 19:53:05 2001
+++ linux-2.4.17-fixup/fs/devfs/base.c	Wed Feb  6 17:49:00 2002
@@ -2318,6 +2318,7 @@
 	if (*str != ',') return 0;  /*  No more options  */
 	++str;
     }
+    devfs_init_major_lists();
     return 1;
 }   /*  End Function devfs_setup  */
 
diff -ruN linux-2.4.17/fs/devfs/util.c linux-2.4.17-fixup/fs/devfs/util.c
--- linux-2.4.17/fs/devfs/util.c	Fri Oct 26 20:00:52 2001
+++ linux-2.4.17-fixup/fs/devfs/util.c	Wed Feb  6 18:02:29 2002
@@ -185,42 +185,88 @@
 
 struct major_list
 {
-    spinlock_t lock;
-    __u32 bits[8];
+	spinlock_t lock;
+	long bits[32/sizeof(long)];
 };
 
+typedef struct _major_preallocated_entry
+{
+	int first_major;
+	int last_major;
+} major_preallocated_entry_t;
+
 /*  Block majors already assigned:
     0-3, 7-9, 11-63, 65-99, 101-113, 120-127, 199, 201, 240-255
     Total free: 122
 */
+
+static major_preallocated_entry_t block_major_preassigned_list[]=
+{
+	{0,3},
+	{7,9},
+	{11,63},
+	{65,99},
+        {101,113},
+        {120,127},
+        {199,199},
+        {201,201},
+        {240,255},
+	{-1,-1} /* end of list marker */
+};
+
+
 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, 
 };
 
 /*  Char majors already assigned:
     0-7, 9-151, 154-158, 160-211, 216-221, 224-230, 240-255
     Total free: 19
 */
+
+static major_preallocated_entry_t char_major_preassigned_list[]=
+{
+	{0,7},
+	{9,151},
+        {154,158},
+        {160,211},
+        {224,230},
+        {240,255},
+	{-1,-1} /* end of list marker */
+};
+
 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  */
 };
 
+/**
+ *       devfs_init_major_lists - Initialize major used bitmaps
+ *
+ */
+void devfs_init_major_lists (void) 
+{
+	int i;
+	major_preallocated_entry_t* entry;
+	// clear all bits
+	for (i=0; i<(32/sizeof(long)) ; i++) {
+		block_major_list.bits[i] = 0;
+		char_major_list.bits[i] = 0;
+	}
+	// set bits in block_major_list and char_major_list of preassigned majors
+	for (entry=&char_major_preassigned_list[0]; 
+	     (entry->first_major!=-1) && (entry->last_major!=-1);
+	     entry++) {
+		for (i=entry->first_major; i<=entry->last_major; i++) 
+			__set_bit (i, char_major_list.bits);
+	}
+	// set bits in block_major_list and char_major_list of preassigned majors
+	for (entry=&block_major_preassigned_list[0]; 
+	     (entry->first_major!=-1) && (entry->last_major!=-1);
+	     entry++) {
+		for (i=entry->first_major; i<=entry->last_major; i++) 
+			__set_bit (i, block_major_list.bits);
+	}
+}
 
 /**
  *	devfs_alloc_major - Allocate a major number.
diff -ruN linux-2.4.17/include/linux/devfs_fs_kernel.h linux-2.4.17-fixup/include/linux/devfs_fs_kernel.h
--- linux-2.4.17/include/linux/devfs_fs_kernel.h	Thu Dec 27 19:53:07 2001
+++ linux-2.4.17-fixup/include/linux/devfs_fs_kernel.h	Thu Feb  7 15:58:00 2002
@@ -115,6 +115,7 @@
 				   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 kdev_t devfs_alloc_devnum (char type);

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

[Russell King]

On Mon, Feb 11, 2002 at 02:00:06PM +0100, Carsten Otte wrote:
> Afterwards, the block_major_list.bits is processed using
> find_first_zero_bit & set_bit out of asm/bitops.h.

Isn't it about time we made the bitops take an unsigned long pointer
argument, rather than a void pointer to catch errors like this?

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

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

[Anton Blanchard]

 
> Isn't it about time we made the bitops take an unsigned long pointer
> argument, rather than a void pointer to catch errors like this?

Davem tried this a while ago (the changes made it into the vger tree)
but I think the number of casts required led to it being removed.

But I do agree, I have found long standing bugs before by changing them
to take an unsigned long.

Anton

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

[David S. Miller]

   From: Russell King <rmk@arm.linux.org.uk>
   Date: Mon, 11 Feb 2002 16:12:59 +0000
   
   Isn't it about time we made the bitops take an unsigned long pointer
   argument, rather than a void pointer to catch errors like this?

No, I tried to do that once, but the casts become stupid
and ugly.

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

[Richard Gooch]

Carsten Otte writes:
> In linux-2.4.17/fs/devfs/util.c, I found the following code:
> struct major_list
> {
>     spinlock_t lock;
>     __u32 bits[8];
> };
> 
> static struct major_list block_major_list =
> {SPIN_LOCK_UNLOCKED,
>     {0xfffffb8f,  /*  Majors 0   to 31   */
[...]
>      0xffff0000}  /*  Majors 224 to 255  */
> };
> 
> Afterwards, the block_major_list.bits is processed using
> find_first_zero_bit & set_bit out of asm/bitops.h.
> Since bitops are only defined for the datatype long, this does
> only work on 32-bit architectures (on 64 bit data gets
> incorrectly alligned -not on 8byte boundary & the ordering of
> the data is incorrect).
> I attached a patch that should fix it for all architectures.

Sorry, but I find your approach grotesque. Apart from basic warts such
as not declaring code+data as __init, the approach of populating the
bitfield from yet another list doesn't appeal to me. I'd much rather
see an approach which preserved the initialisation using bitmasks.

> (See attached file: linux-2.4.17-devfs_fixup.diff)

BTW: please don't send attachments. Send patches inline instead.

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

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

[Carsten Otte]

David S. Miller <davem@redhat.com> wrote:
>No, I tried to do that once, but the casts become stupid
>and ugly.

I agree, and with explicit casts the compiler still would'nt even print a
warning
when the bitops are incorrectly used.

w/kind regards
Carsten Otte

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

[Carsten Otte]

Hi Richard!

>BTW: please don't send attachments. Send patches inline instead.
Sorry for sending the patch as attachment, but Notes messes
up whitespace so the patch would'nt apply if I include it directly.

>Sorry, but I find your approach grotesque. Apart from basic warts such
>as not declaring code+data as __init, the approach of populating the
>bitfield from yet another list doesn't appeal to me. I'd much rather
>see an approach which preserved the initialisation using bitmasks.
I do not think this patch is very nice either & it does not work at
all (the initialisation of the array is only called in error case).
I find the overall thing for registering/deregistering devices &
allocating majors very inconsistent.
devfs_alloc_major and devfs_register_*dev do hold the information
about which majors are allocated in two different places without
knowing about each other (bdops field and this private bitfield).
A good solution would be if *dev_register would never return a
major being statically allocated when called with major 0. If this is the
case, I do not see what alloc_major and dealloc_major are useful for.

mit freundlichem Gruß / with kind regards
Carsten Otte

IBM Deutschland Entwicklung GmbH
Linux for eServer development - device driver team
Phone: +49/07031/16-4076
IBM internal phone: *120-4076
--
We are Linux.
Resistance indicates that you're missing the point!

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

[Richard Gooch]

Carsten Otte writes:
> 
> Hi Richard!
> 
> >BTW: please don't send attachments. Send patches inline instead.
> Sorry for sending the patch as attachment, but Notes messes
> up whitespace so the patch would'nt apply if I include it directly.
> 
> >Sorry, but I find your approach grotesque. Apart from basic warts such
> >as not declaring code+data as __init, the approach of populating the
> >bitfield from yet another list doesn't appeal to me. I'd much rather
> >see an approach which preserved the initialisation using bitmasks.
>
> I do not think this patch is very nice either & it does not work at
> all (the initialisation of the array is only called in error case).

Now you've confused me. Your patch seems to unconditionally initialise
the bitfields at startup. I didn't see logic for restricting that
initialisation to an error path.

> I find the overall thing for registering/deregistering devices &
> allocating majors very inconsistent.
> devfs_alloc_major and devfs_register_*dev do hold the information
> about which majors are allocated in two different places without
> knowing about each other (bdops field and this private bitfield).
> A good solution would be if *dev_register would never return a major
> being statically allocated when called with major 0. If this is the
> case, I do not see what alloc_major and dealloc_major are useful
> for.

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.

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.

Hey, hey! It looks like vger is sending emails again!

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

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

[Carsten Otte]

[-- 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);

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

[Carsten Otte]

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


Hi Richard,

the previous version of my patch did contain a bug,
caused by incorrect parameter order when calling
__devfs_unregister_major. The symptom was, that
major numbers registered with devfs_register_*dev
but not allocated with devfs_alloc_major were not
freed by calling devfs_unregister_*dev. This is
now fixed. Sorry, the patch is attached (due to Notes
messing up with whitespace).
(See attached file: linux-2.4.17-devfs_fixup.diff)

Richard, I would appreciate it if you could finally
look into this.

mit freundlichem Gruß / with kind regards
Carsten Otte

IBM Deutschland Entwicklung GmbH
Linux for eServer development - device driver team
Phone: +49/07031/16-4076
IBM internal phone: *120-4076
--
We are Linux.
Resistance indicates that you're missing the point!

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

diff -ruN linux-2.4.17-sdmany/fs/devfs/base.c linux-2.4.17-sdmany-devfs_fixup/fs/devfs/base.c
--- linux-2.4.17-sdmany/fs/devfs/base.c	Wed Feb 13 13:25:50 2002
+++ linux-2.4.17-sdmany-devfs_fixup/fs/devfs/base.c	Tue Mar 12 20:15:53 2002
@@ -2186,7 +2186,7 @@
 }   /*  End Function devfs_get_name  */
 
 
-/**y
+/**
  *	devfs_register_chrdev - Optionally register a conventional character driver.
  *	@major: The major number for the driver.
  *	@name: The name of the driver (as seen in /proc/devices).
@@ -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.4.17-sdmany/fs/devfs/util.c linux-2.4.17-sdmany-devfs_fixup/fs/devfs/util.c
--- linux-2.4.17-sdmany/fs/devfs/util.c	Fri Oct 26 20:00:52 2001
+++ linux-2.4.17-sdmany-devfs_fixup/fs/devfs/util.c	Tue Mar 12 20:48:52 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 (int major, char type)
+{
+    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.4.17-sdmany/include/linux/devfs_fs_kernel.h linux-2.4.17-sdmany-devfs_fixup/include/linux/devfs_fs_kernel.h
--- linux-2.4.17-sdmany/include/linux/devfs_fs_kernel.h	Thu Dec 27 19:53:07 2001
+++ linux-2.4.17-sdmany-devfs_fixup/include/linux/devfs_fs_kernel.h	Tue Mar 12 20:48:49 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 (int major, char type);
 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);

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

[Richard Gooch]

Carsten Otte writes:
> the previous version of my patch did contain a bug,
> caused by incorrect parameter order when calling
> __devfs_unregister_major. The symptom was, that
> major numbers registered with devfs_register_*dev
> but not allocated with devfs_alloc_major were not
> freed by calling devfs_unregister_*dev. This is
> now fixed. Sorry, the patch is attached (due to Notes
> messing up with whitespace).
> (See attached file: linux-2.4.17-devfs_fixup.diff)

In future, please send patches in plain text, rather than
MIME-encoded.

> Richard, I would appreciate it if you could finally
> look into this.

I don't like your patch because:
- it replaces the bitfield with a character array. This in turn causes
  more data bloat (8 times more), and also prevents the use of the ffz
  functions

- you've mixed in a behavioural change to devfs_register_???dev() to
  call devfs_alloc_major() when the provided major is 0. While this
  might be good idea in the long run (but maybe not), it should be
  separated from the actual fix.

Unfortunately I've been busy chasing other (possible, not sure yet
what the source of the problems are) bugs, so I haven't had time to
code up a solution yet.

Is there any reason why switching from __u32 to unsigned long won't
work? Of course, the initialising values would need to be 64 bits wide
on a 64 bit machine, but that could probably be taken care of with
some clever macros (ab)use:

#if 64 bit
#define INITIALISER64(a,b) (a)<<32|(b)
#else
#define INITIALISER64(a,b) (a),(b)
#endif

static struct major_list block_major_list =
{SPIN_LOCK_UNLOCKED,
    {INITIALISER64(0xfffffb8f,0xffffffff),  /*  Majors 0   to 63    */
     INITIALISER64(0xfffffffe,0xff03ffef),  /*  Majors 64  to 127   */

and so on. Untested, and I only spent a few seconds thinking about it,
but perhaps this will solve it.

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca