Re: [PATCH 1/3] revert register_chrdev_region change

Re: [PATCH 1/3] revert register_chrdev_region change

[Andries.Brouwer]

> If you look at Roman's patches they don't hinder your dev_t enlargement

Not very much. A little.
And in some ways they are a step back.

> I'm personally not yet completly happy with his interface either
> because he still uses the major/minor split

Yes, it is more elegant to register one or more ranges.
(But ranges of what? Ranges in dev_t space? Or in kdev_t space?
Here you see one reason to wait a little until dev_t/kdev_t
stuff has settled.)
Also, you'll notice that the current simple hash scheme is insufficient
if we want to have subranges that override larger ranges.
But life is easier if we postpone that discussion a bit.

# It would help a lot if you would explain what the next stages are.

- Polish the kernel until a change of the size of dev_t is possible.
- Agree on a new size for dev_t, major, minor.  Make the change.
- Ask Ulrich to update glibc.

On the last part: Ulrich already said that the changes are trivial
and that he is waiting for kernel people to make up their minds on
what dev_t and major and minor are supposed to be.

On the first part: Earlier today I sent a patch on stat.h for a
number of architectures. There are a few more such steps.

Andries

Re: [PATCH 1/3] revert register_chrdev_region change

[Christoph Hellwig]

On Mon, Mar 24, 2003 at 08:07:05PM +0100, Andries.Brouwer@cwi.nl wrote:
> Yes, it is more elegant to register one or more ranges.
> (But ranges of what? Ranges in dev_t space? Or in kdev_t space?

In dev_t space.

> Here you see one reason to wait a little until dev_t/kdev_t
> stuff has settled.)

I guess basically everyone will disagree with you on making kdev_t
a different representation.  This will end up as messy as the
internal/external dev_t stuff on some of the SVR4 ports.

Re: [PATCH 1/3] revert register_chrdev_region change

[Roman Zippel]

Hi,

On Mon, 24 Mar 2003 Andries.Brouwer@cwi.nl wrote:

> > I'm personally not yet completly happy with his interface either
> > because he still uses the major/minor split
> 
> Yes, it is more elegant to register one or more ranges.
> (But ranges of what? Ranges in dev_t space? Or in kdev_t space?

Ok, I'm slightly confused now, what is the difference between the "dev_t 
space" and the "kdev_t space"? The answer I'd like to hear is: none.
A difference might be the encoding, that's why I mentioned the ext2 
example. How will be e.g. 0x0301 encoded on disk with your changes?
So far I understood kdev_t as a marker, which has to be replaced with 
either struct block_device or char_device, so that at some point kdev_t 
goes away completely. (Especially Al did some great work here with the 
block layer.) You removed now part of this work by removing the i_cdev 
pointer from the inode. What will you replace it with?

> Also, you'll notice that the current simple hash scheme is insufficient
> if we want to have subranges that override larger ranges.
> But life is easier if we postpone that discussion a bit.

I'd prefer to have the discussion now, as I still don't know what we need 
ranges or even subranges for. What problem are you trying to solve?

> # It would help a lot if you would explain what the next stages are.
> 
> - Polish the kernel until a change of the size of dev_t is possible.
> - Agree on a new size for dev_t, major, minor.  Make the change.
> - Ask Ulrich to update glibc.

I don't care much about the specific major/minor encoding, I want to know 
how it will be used at the kernel level.

bye, Roman

Re: [PATCH 1/3] revert register_chrdev_region change

[Andries.Brouwer]

    > > I still don't know what we need ranges or even subranges for.
    > > What problem are you trying to solve?
    > 
    > I mentioned the structure of Al's block device code to you.
    > Haven't you read blk_register_region()?

    I did, have you seen add_disk()? ...
    So which problem requires a complex (sub)ranges solution?

Roman, please. There is no need to invent discussions.
Al wrote certain code. It is not my code. I mentioned
that this code has certain properties.

If you want to know why Al wrote the code he wrote, ask him.

Andries

Re: [PATCH 1/3] revert register_chrdev_region change

[Roman Zippel]

Hi,

On Mon, 24 Mar 2003 Andries.Brouwer@cwi.nl wrote:

>     So which problem requires a complex (sub)ranges solution?
> 
> Roman, please. There is no need to invent discussions.
> Al wrote certain code. It is not my code. I mentioned
> that this code has certain properties.
> 
> If you want to know why Al wrote the code he wrote, ask him.

Huh? What am I inventing?
You removed the character device hash. You removed the i_cdev member. You 
added the region support. Which part of the kernel will use this? You must 
have a reason for doing this, so I don't understand your reference to Al.

bye, Roman

Re: [PATCH 1/3] revert register_chrdev_region change

[Andries.Brouwer]

> I still don't know what we need ranges or even subranges for.
> What problem are you trying to solve?

I mentioned the structure of Al's block device code to you.
Haven't you read blk_register_region()?

Andries

Re: [PATCH 1/3] revert register_chrdev_region change

[Roman Zippel]

Hi,

On Mon, 24 Mar 2003 Andries.Brouwer@cwi.nl wrote:

> > I still don't know what we need ranges or even subranges for.
> > What problem are you trying to solve?
> 
> I mentioned the structure of Al's block device code to you.
> Haven't you read blk_register_region()?

I did, have you seen add_disk()? Did you notice that more drivers use 
add_disk() than blk_register_region() and that most of the 
blk_register_region() users are legacy drivers?
Which character device has partitions? Even for block devices it will be 
easier to just define MAX_PART_NR and simply use a constant shift to get 
from a partition to the disk.
Please try to keep the problem simple, all examples I've seen so far only 
can be dealt with quite easily. So which problem requires a complex 
(sub)ranges solution?

bye, Roman

Re: [PATCH 1/3] revert register_chrdev_region change

[Andries.Brouwer]

    From: Christoph Hellwig <hch@infradead.org>

    > Yes, it is more elegant to register one or more ranges.
    > (But ranges of what? Ranges in dev_t space? Or in kdev_t space?

    In dev_t space.

    I guess basically everyone will disagree with you on making kdev_t
    a different representation.  This will end up as messy as the
    internal/external dev_t stuff on some of the SVR4 ports.

You are the first to say so. I'll think about it.

A. On kdev_t vs. dev_t:
kdev_t gives small and fast code, dev_t needs a conditional
That was my main reason. I suppose you say that dev_t is a cookie
and that the kernel should never want to ask about major and minor,
except perhaps at filesystem interfaces. So the 1000+ invocations
that we have now should all go away. A reasonable point of view.

B. On what is registered:
The main question here is what the documented outside reality is.
Is that phrased in terms of dev_t intervals? Or is that phrased
in terms of (major,minor) pairs?
Until convinced otherwise I will hold that users talk about
(major,minor) pairs. They do ls -l and see major,minor pairs.
They want to do mknod and need a major,minor pair.
So I suppose that the documented reality will give a minor
range for a given major, or give a major range.
Such ranges correspond to kdev_t ranges, not to dev_t ranges.

Of course one can avoid the distinction by decreeing that
majors 0-255 cannot have more than 256 minors.

Andries

Re: [PATCH 1/3] revert register_chrdev_region change

[Christoph Hellwig]

On Mon, Mar 24, 2003 at 09:34:14PM +0100, Andries.Brouwer@cwi.nl wrote:
> A. On kdev_t vs. dev_t:
> kdev_t gives small and fast code, dev_t needs a conditional
> That was my main reason. I suppose you say that dev_t is a cookie
> and that the kernel should never want to ask about major and minor,
> except perhaps at filesystem interfaces. So the 1000+ invocations
> that we have now should all go away. A reasonable point of view.

Yupp.  And you'll notice that we're almost there for block devices
already.

> B. On what is registered:
> The main question here is what the documented outside reality is.
> Is that phrased in terms of dev_t intervals? Or is that phrased
> in terms of (major,minor) pairs?
> Until convinced otherwise I will hold that users talk about
> (major,minor) pairs. They do ls -l and see major,minor pairs.
> They want to do mknod and need a major,minor pair.
> So I suppose that the documented reality will give a minor
> range for a given major, or give a major range.

Well, users can do that if it makes their live easier.  The kernel
doesn't need nor should know about this split internally.  There's
a few legacy interfaces left that hardcode this split, but it's
okay - no one expects the major/minor split to have more meaning
than __low/__high anyway - we already have far too many subsystems
that hand out ranges (or in the case of sound individual dev_t s)
to drivers.

> Of course one can avoid the distinction by decreeing that
> majors 0-255 cannot have more than 256 minors.

It might be a wise idea to not use them to avoid subtile driver
breakage unless we do a full audit, yes.

[PATCH 1/3] revert register_chrdev_region change

[Roman Zippel]

Hi,

This patch removes Andries dev patch, which was unfortunately merged.
It doesn't really help to manage a large number of character devices.
Besides of this the unregister_chrdev() function is buggy (it just
removes a random region).
There is no unregister_chrdev_region function.
Unless kdev_t is 64bit, MAX_CHRDEV is still needed to check the input
argument to register_chrdev().
Dynamic majors have to be allocated from the end of the available number 
space (or from a fixed range) and not at a random number (MAX_PROBE_HASH).

bye, Roman

diff -Nur -X /opt/home/roman/nodiff linux-2.5-dev1/drivers/char/tty_io.c linux-2.5-dev2/drivers/char/tty_io.c
--- linux-2.5-dev1/drivers/char/tty_io.c	2003-03-23 18:08:49.000000000 +0100
+++ linux-2.5-dev2/drivers/char/tty_io.c	2003-03-23 18:20:14.000000000 +0100
@@ -2118,8 +2118,7 @@
 	if (driver->flags & TTY_DRIVER_INSTALLED)
 		return 0;
 
-	error = register_chrdev_region(driver->major, driver->minor_start,
-				       driver->num, driver->name, &tty_fops);
+	error = register_chrdev(driver->major, driver->name, &tty_fops);
 	if (error < 0)
 		return error;
 	else if(driver->major == 0)
diff -Nur -X /opt/home/roman/nodiff linux-2.5-dev1/fs/char_dev.c linux-2.5-dev2/fs/char_dev.c
--- linux-2.5-dev1/fs/char_dev.c	2003-03-23 18:08:54.000000000 +0100
+++ linux-2.5-dev2/fs/char_dev.c	2003-03-23 18:28:37.000000000 +0100
@@ -19,183 +19,122 @@
 
 #ifdef CONFIG_KMOD
 #include <linux/kmod.h>
+#include <linux/tty.h>
+
+/* serial module kmod load support */
+struct tty_driver *get_tty_driver(kdev_t device);
+#define is_a_tty_dev(ma)	(ma == TTY_MAJOR || ma == TTYAUX_MAJOR)
+#define need_serial(ma,mi) (get_tty_driver(mk_kdev(ma,mi)) == NULL)
 #endif
 
-#define MAX_PROBE_HASH 255	/* random */
+struct device_struct {
+	const char * name;
+	struct file_operations * fops;
+};
 
 static rwlock_t chrdevs_lock = RW_LOCK_UNLOCKED;
+static struct device_struct chrdevs[MAX_CHRDEV];
 
-static struct char_device_struct {
-	struct char_device_struct *next;
-	unsigned int major;
-	unsigned int baseminor;
-	int minorct;
-	const char *name;
-	struct file_operations *fops;
-} *chrdevs[MAX_PROBE_HASH];
-
-/* index in the above */
-static inline int major_to_index(int major)
-{
-	return major % MAX_PROBE_HASH;
-}
-
-/* get char device names in somewhat random order */
 int get_chrdev_list(char *page)
 {
-	struct char_device_struct *cd;
-	int i, len;
+	int i;
+	int len;
 
 	len = sprintf(page, "Character devices:\n");
-
 	read_lock(&chrdevs_lock);
-	for (i = 0; i < ARRAY_SIZE(chrdevs) ; i++) {
-		for (cd = chrdevs[i]; cd; cd = cd->next)
+	for (i = 0; i < MAX_CHRDEV ; i++) {
+		if (chrdevs[i].fops) {
 			len += sprintf(page+len, "%3d %s\n",
-				       cd->major, cd->name);
-	}
-	read_unlock(&chrdevs_lock);
-
-	return len;
-}
-
-/*
- * Return the function table of a device, if present.
- * Increment the reference count of module in question.
- */
-static struct file_operations *
-lookup_chrfops(unsigned int major, unsigned int minor)
-{
-	struct char_device_struct *cd;
-	struct file_operations *ret = NULL;
-	int i;
-
-	i = major_to_index(major);
-
-	read_lock(&chrdevs_lock);
-	for (cd = chrdevs[i]; cd; cd = cd->next) {
-		if (major == cd->major &&
-		    minor - cd->baseminor < cd->minorct) {
-			ret = fops_get(cd->fops);
-			break;
+				       i, chrdevs[i].name);
 		}
 	}
 	read_unlock(&chrdevs_lock);
-
-	return ret;
+	return len;
 }
 
 /*
- * Return the function table of a device, if present.
- * Load the driver if needed.
- * Increment the reference count of module in question.
+ *	Return the function table of a device.
+ *	Load the driver if needed.
+ *	Increment the reference count of module in question.
  */
 static struct file_operations *
 get_chrfops(unsigned int major, unsigned int minor)
 {
-	struct file_operations *ret = NULL;
+	struct file_operations *ret;
 
-	if (!major)
+	if (!major || major >= MAX_CHRDEV)
 		return NULL;
 
-	ret = lookup_chrfops(major, minor);
-
+	read_lock(&chrdevs_lock);
+	ret = fops_get(chrdevs[major].fops);
+	read_unlock(&chrdevs_lock);
 #ifdef CONFIG_KMOD
+	if (ret && is_a_tty_dev(major)) {
+		lock_kernel();
+		if (need_serial(major,minor)) {
+			/* Force request_module anyway, but what for? */
+			/* The reason is that we may have a driver for
+			   /dev/tty1 already, but need one for /dev/ttyS1. */
+			fops_put(ret);
+			ret = NULL;
+		}
+		unlock_kernel();
+	}
 	if (!ret) {
-		char name[32];
+		char name[20];
 		sprintf(name, "char-major-%d", major);
 		request_module(name);
 
 		read_lock(&chrdevs_lock);
-		ret = lookup_chrfops(major, minor);
+		ret = fops_get(chrdevs[major].fops);
 		read_unlock(&chrdevs_lock);
 	}
 #endif
 	return ret;
 }
 
-/*
- * Register a single major with a specified minor range
- */
-int register_chrdev_region(unsigned int major, unsigned int baseminor,
-			   int minorct, const char *name,
-			   struct file_operations *fops)
+int register_chrdev(unsigned int major, const char *name,
+		    struct file_operations *fops)
 {
-	struct char_device_struct *cd, **cp;
-	int ret = 0;
-	int i;
-
-	/* temporary */
 	if (major == 0) {
-		read_lock(&chrdevs_lock);
-		for (i = ARRAY_SIZE(chrdevs)-1; i > 0; i--)
-			if (chrdevs[i] == NULL)
-				break;
-		read_unlock(&chrdevs_lock);
-
-		if (i == 0)
-			return -EBUSY;
-		ret = major = i;
-	}
-
-	cd = kmalloc(sizeof(struct char_device_struct), GFP_KERNEL);
-	if (cd == NULL)
-		return -ENOMEM;
-
-	cd->major = major;
-	cd->baseminor = baseminor;
-	cd->minorct = minorct;
-	cd->name = name;
-	cd->fops = fops;
-
-	i = major_to_index(major);
-
+		write_lock(&chrdevs_lock);
+		for (major = MAX_CHRDEV-1; major > 0; major--) {
+			if (chrdevs[major].fops == NULL) {
+				chrdevs[major].name = name;
+				chrdevs[major].fops = fops;
+				write_unlock(&chrdevs_lock);
+				return major;
+			}
+		}
+		write_unlock(&chrdevs_lock);
+		return -EBUSY;
+	}
+	if (major >= MAX_CHRDEV)
+		return -EINVAL;
 	write_lock(&chrdevs_lock);
-	for (cp = &chrdevs[i]; *cp; cp = &(*cp)->next)
-		if ((*cp)->major > major ||
-		    ((*cp)->major == major && (*cp)->baseminor >= baseminor))
-			break;
-	if (*cp && (*cp)->major == major &&
-	    (*cp)->baseminor < baseminor + minorct) {
-		ret = -EBUSY;
-	} else {
-		cd->next = *cp;
-		*cp = cd;
+	if (chrdevs[major].fops && chrdevs[major].fops != fops) {
+		write_unlock(&chrdevs_lock);
+		return -EBUSY;
 	}
+	chrdevs[major].name = name;
+	chrdevs[major].fops = fops;
 	write_unlock(&chrdevs_lock);
-
-	return ret;
+	return 0;
 }
 
-int register_chrdev(unsigned int major, const char *name,
-		    struct file_operations *fops)
-{
-	return register_chrdev_region(major, 0, 256, name, fops);
-}
-
-/* todo: make void - error printk here */
 int unregister_chrdev(unsigned int major, const char * name)
 {
-	struct char_device_struct *cd, **cp;
-	int ret = 0;
-	int i;
-
-	i = major_to_index(major);
-
+	if (major >= MAX_CHRDEV)
+		return -EINVAL;
 	write_lock(&chrdevs_lock);
-	for (cp = &chrdevs[i]; *cp; cp = &(*cp)->next)
-		if ((*cp)->major == major)
-			break;
-	if (!*cp || strcmp((*cp)->name, name))
-		ret = -EINVAL;
-	else {
-		cd = *cp;
-		*cp = cd->next;
-		kfree(cd);
+	if (!chrdevs[major].fops || strcmp(chrdevs[major].name, name)) {
+		write_unlock(&chrdevs_lock);
+		return -EINVAL;
 	}
+	chrdevs[major].name = NULL;
+	chrdevs[major].fops = NULL;
 	write_unlock(&chrdevs_lock);
-
-	return ret;
+	return 0;
 }
 
 /*
@@ -229,20 +168,10 @@
 const char *cdevname(kdev_t dev)
 {
 	static char buffer[40];
-	const char *name = "unknown-char";
-	unsigned int major = major(dev);
-	unsigned int minor = minor(dev);
-	int i = major_to_index(major);
-	struct char_device_struct *cd;
-
-	read_lock(&chrdevs_lock);
-	for (cd = chrdevs[i]; cd; cd = cd->next)
-		if (cd->major == major)
-			break;
-	if (cd)
-		name = cd->name;
-	sprintf(buffer, "%s(%d,%d)", name, major, minor);
-	read_unlock(&chrdevs_lock);
+	const char * name = chrdevs[major(dev)].name;
 
+	if (!name)
+		name = "unknown-char";
+	sprintf(buffer, "%s(%d,%d)", name, major(dev), minor(dev));
 	return buffer;
 }
diff -Nur -X /opt/home/roman/nodiff linux-2.5-dev1/fs/inode.c linux-2.5-dev2/fs/inode.c
--- linux-2.5-dev1/fs/inode.c	2003-03-23 18:08:54.000000000 +0100
+++ linux-2.5-dev2/fs/inode.c	2003-03-23 18:20:14.000000000 +0100
@@ -145,7 +145,7 @@
 		mapping->assoc_mapping = NULL;
 		mapping->backing_dev_info = &default_backing_dev_info;
 		if (sb->s_bdev)
-			mapping->backing_dev_info = sb->s_bdev->bd_inode->i_mapping->backing_dev_info;
+			inode->i_data.backing_dev_info = sb->s_bdev->bd_inode->i_mapping->backing_dev_info;
 		memset(&inode->u, 0, sizeof(inode->u));
 		inode->i_mapping = mapping;
 	}
diff -Nur -X /opt/home/roman/nodiff linux-2.5-dev1/include/linux/fs.h linux-2.5-dev2/include/linux/fs.h
--- linux-2.5-dev1/include/linux/fs.h	2003-03-23 18:08:55.000000000 +0100
+++ linux-2.5-dev2/include/linux/fs.h	2003-03-23 18:20:14.000000000 +0100
@@ -1055,10 +1055,7 @@
 extern void blk_run_queues(void);
 
 /* fs/char_dev.c */
-extern int register_chrdev_region(unsigned int, unsigned int, int,
-				  const char *, struct file_operations *);
-extern int register_chrdev(unsigned int, const char *,
-			   struct file_operations *);
+extern int register_chrdev(unsigned int, const char *, struct file_operations *);
 extern int unregister_chrdev(unsigned int, const char *);
 extern int chrdev_open(struct inode *, struct file *);
 
diff -Nur -X /opt/home/roman/nodiff linux-2.5-dev1/include/linux/major.h linux-2.5-dev2/include/linux/major.h
--- linux-2.5-dev1/include/linux/major.h	2003-03-23 22:19:03.000000000 +0100
+++ linux-2.5-dev2/include/linux/major.h	2003-03-23 18:22:44.000000000 +0100
@@ -160,6 +160,13 @@
 #define IBM_FS3270_MAJOR	228
 
 /*
+ * Important: Don't change this to 256.  Major number 255 is and must be
+ * reserved for future expansion into a larger dev_t space.
+ */
+#define MAX_CHRDEV		255
+#define MAX_BLKDEV		255
+
+/*
  * Tests for SCSI devices.
  */
 

Re: [PATCH 1/3] revert register_chrdev_region change

[Andries Brouwer]

On Mon, Mar 24, 2003 at 12:25:57AM +0100, Roman Zippel wrote:

> This patch removes Andries dev patch, which was unfortunately merged.
> It doesn't really help to manage a large number of character devices.

Hi Roman -

It still looks like you do not understand the purpose of these patches.
First of all, it is a series - code is morphed into a more desirable
state; at each point in time there are imperfections, and some of these
disappear the next stage.
The first goal is not at all handling many devices. The first goal is
having a larger dev_t. Handling many devices comes after that.

The patch that you want to revert made the kernel source and binary smaller,
made chardev handling more efficient, and enables stuff impossible so far.
But if you need hundreds of regions on the same major, yes, then this very
simplistic hash scheme requires some further work.
This is not important today.

> unregister_chrdev() function is buggy
> There is no unregister_chrdev_region function.

True. This interface that you write all your letters against is for me
just something uninteresting, something temporary, a stage we pass through.
But if you want, you can so very easily fix these particular flaws:

int unregister_chrdev(unsigned int major, const char *name)
{
        return unregister_chrdev_region(major, 0, 256, name);
}

int unregister_chrdev_region(unsigned int major, unsigned int baseminor,
                             int minorct, const char *name)
...
                if ((*cp)->major == major &&
                    (*cp)->baseminor == baseminor &&
                    (*cp)->minorct == minorct)
                        break;

together with the appropriate invocation in tty_unregister_driver()
(causing a nice cleanup there).

[Now that you complained about this, I made this change in my tree -
may submit it against 2.5.66 or so.]

> Dynamic majors have to be allocated from a fixed range

I entirely agree, and already in ancient l-k posts you can see that that
is what I do myself. But we are in transition, and dev_t has not yet
become larger, so this big free fixed range is not yet available.
We are going there, LV.

Andries

Re: [PATCH 1/3] revert register_chrdev_region change

[Roman Zippel]

Hi,

On Mon, 24 Mar 2003, Andries Brouwer wrote:

> It still looks like you do not understand the purpose of these patches.
> First of all, it is a series - code is morphed into a more desirable
> state; at each point in time there are imperfections, and some of these
> disappear the next stage.

It would help a lot if you would explain what these next stages are.

> The first goal is not at all handling many devices. The first goal is
> having a larger dev_t. Handling many devices comes after that.

My patch already does both. What am I doing wrong?

bye, Roman

Re: [PATCH 1/3] revert register_chrdev_region change

[Christoph Hellwig]

On Mon, Mar 24, 2003 at 03:25:15PM +0100, Andries Brouwer wrote:
> It still looks like you do not understand the purpose of these patches.
> First of all, it is a series - code is morphed into a more desirable
> state; at each point in time there are imperfections, and some of these
> disappear the next stage.
> The first goal is not at all handling many devices. The first goal is
> having a larger dev_t. Handling many devices comes after that.

Well, there's people here who disagree with tour order.  And yes, making
dev_t larger before making the kernel ready for a large number of devices
is the wrong way around.

If you look at Roman's patches they don't even hinder your dev_t enlargement
but they provide a singificant benefit.   Now I'm personally not yet
completly happy with his interface either because he still uses the
major/minor split, but I'm working on fixing this properly.

Re: [PATCH 1/3] revert register_chrdev_region change

[Roman Zippel]

Hi,

On Mon, 24 Mar 2003, Christoph Hellwig wrote:

> If you look at Roman's patches they don't even hinder your dev_t enlargement
> but they provide a singificant benefit.   Now I'm personally not yet
> completly happy with his interface either because he still uses the
> major/minor split, but I'm working on fixing this properly.

BTW I have an updated patch, which stores the major char_device at 
(0,major), so the remaining space is free for whatever you need.
Below is a patch which shows how drivers/usb/core/file.c could look 
after this. There are of course further cleanups possible. :)

bye, Roman

diff -Nurp -X /home/roman/nodiff linux-2.5.65-bk4-cdev3/drivers/usb/core/file.c linux-2.5.65-bk4-cdev4/drivers/usb/core/file.c
--- linux-2.5.65-bk4-cdev3/drivers/usb/core/file.c	2003-03-24 10:26:47.000000000 +0100
+++ linux-2.5.65-bk4-cdev4/drivers/usb/core/file.c	2003-03-24 14:13:37.000000000 +0100
@@ -32,41 +32,9 @@ devfs_handle_t usb_devfs_handle;	/* /dev
 EXPORT_SYMBOL(usb_devfs_handle);
 
 #define MAX_USB_MINORS	256
-static struct file_operations *usb_minors[MAX_USB_MINORS];
-static spinlock_t minor_lock = SPIN_LOCK_UNLOCKED;
-
-static int usb_open(struct inode * inode, struct file * file)
-{
-	int minor = minor(inode->i_rdev);
-	struct file_operations *c;
-	int err = -ENODEV;
-	struct file_operations *old_fops, *new_fops = NULL;
-
-	spin_lock (&minor_lock);
-	c = usb_minors[minor];
-
-	if (!c || !(new_fops = fops_get(c))) {
-		spin_unlock(&minor_lock);
-		return err;
-	}
-	spin_unlock(&minor_lock);
-
-	old_fops = file->f_op;
-	file->f_op = new_fops;
-	/* Curiouser and curiouser... NULL ->open() as "no device" ? */
-	if (file->f_op->open)
-		err = file->f_op->open(inode,file);
-	if (err) {
-		fops_put(file->f_op);
-		file->f_op = fops_get(old_fops);
-	}
-	fops_put(old_fops);
-	return err;
-}
 
 static struct file_operations usb_fops = {
 	.owner =	THIS_MODULE,
-	.open =		usb_open,
 };
 
 int usb_major_init(void)
@@ -107,12 +75,9 @@ void usb_major_cleanup(void)
  * device, and 0 on success, alone with a value that the driver should
  * use in start_minor.
  */
-int usb_register_dev (struct file_operations *fops, int minor, int num_minors, int *start_minor)
+int usb_register_dev (struct file_operations *fops, int minor, int *start_minor)
 {
 	int i;
-	int j;
-	int good_spot;
-	int retval = -EINVAL;
 
 #ifdef CONFIG_USB_DYNAMIC_MINORS
 	/* 
@@ -123,37 +88,29 @@ int usb_register_dev (struct file_operat
 	minor = 0;
 #endif
 
-	dbg ("asking for %d minors, starting at %d", num_minors, minor);
+	dbg ("asking for 1 minors, starting at %d", minor);
 
 	if (fops == NULL)
-		goto exit;
+		return  -EINVAL;
 
 	*start_minor = 0; 
-	spin_lock (&minor_lock);
 	for (i = minor; i < MAX_USB_MINORS; ++i) {
-		if (usb_minors[i])
-			continue;
-
-		good_spot = 1;
-		for (j = 1; j <= num_minors-1; ++j)
-			if (usb_minors[i+j]) {
-				good_spot = 0;
-				break;
+		cdev = cdget(MKDEV(USB_MAJOR, i));
+		if (!cdev->cd_fops) {
+			down(&cdev->cd_sem);
+			if (!cdev->cd_fops) {
+				dbg("found a minor chunk free, starting at %d", i);
+				cdev->cd_fops = fops;
+				up(&cdev->cd_sem);
+				start_minor = i;
+				return 0;
 			}
-		if (good_spot == 0)
-			continue;
-
-		*start_minor = i;
-		dbg("found a minor chunk free, starting at %d", i);
-		for (i = *start_minor; i < (*start_minor + num_minors); ++i)
-			usb_minors[i] = fops;
-
-		retval = 0;
-		goto exit;
+			up(&cdev->cd_sem);
+		}
+		cdput(cdev);
 	}
-exit:
-	spin_unlock (&minor_lock);
-	return retval;
+
+	return -EBUSY;
 }
 EXPORT_SYMBOL(usb_register_dev);
 
@@ -169,16 +126,21 @@ EXPORT_SYMBOL(usb_register_dev);
  * 
  * This should be called by all drivers that use the USB major number.
  */
-void usb_deregister_dev (int num_minors, int start_minor)
+void usb_deregister_dev (int minor)
 {
 	int i;
 
 	dbg ("removing %d minors starting at %d", num_minors, start_minor);
 
-	spin_lock (&minor_lock);
-	for (i = start_minor; i < (start_minor + num_minors); ++i)
-		usb_minors[i] = NULL;
-	spin_unlock (&minor_lock);
+	cdev = cdget(MKDEV(USB_MAJOR, minor));
+	down(&cdev->cd_sem);
+	if (cdev->cd_fops) {
+		cdev->cd_fops = NULL;
+		cdput(cdev);
+	} else
+		printk("usb_deregister_dev: releasing invalid dev %d\n", minor);
+	up(&cdev->cd_sem);
+	cdput(cdev);
 }
 EXPORT_SYMBOL(usb_deregister_dev);