[PATCH 1/2] chrdev: implement __[un]register_chrdev()

[PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Tejun Heo]

[un]register_chrdev() assume minor range 0-255.  This patch adds __
prefixed versions which take @minorbase and @count explicitly.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
Hello,

These two patches make sound_core grabbing OSS device number optional.
If there's no objection, I think it would be easiest to push this
through Takashi's tree.

Thanks.

 fs/char_dev.c      |   63 +++++++++++++++++++++++++++++++++++++++++++----------
 include/linux/fs.h |   11 ++++++---
 2 files changed, 60 insertions(+), 14 deletions(-)

Index: work/fs/char_dev.c
===================================================================
--- work.orig/fs/char_dev.c
+++ work/fs/char_dev.c
@@ -237,8 +237,10 @@ int alloc_chrdev_region(dev_t *dev, unsi
 }

 /**
- * register_chrdev() - Register a major number for character devices.
+ * __register_chrdev() - create and register a cdev occupying a range of minors
  * @major: major device number or 0 for dynamic allocation
+ * @baseminor: first of the requested range of minor numbers
+ * @count: the number of minor numbers required
  * @name: name of this range of devices
  * @fops: file operations associated with this devices
  *
@@ -254,19 +256,17 @@ int alloc_chrdev_region(dev_t *dev, unsi
  * /dev. It only helps to keep track of the different owners of devices. If
  * your module name has only one type of devices it's ok to use e.g. the name
  * of the module here.
- *
- * This function registers a range of 256 minor numbers. The first minor number
- * is 0.
  */
-int register_chrdev(unsigned int major, const char *name,
-		    const struct file_operations *fops)
+int __register_chrdev(unsigned int major, unsigned int baseminor,
+		      unsigned int count, const char *name,
+		      const struct file_operations *fops)
 {
 	struct char_device_struct *cd;
 	struct cdev *cdev;
 	char *s;
 	int err = -ENOMEM;

-	cd = __register_chrdev_region(major, 0, 256, name);
+	cd = __register_chrdev_region(major, baseminor, count, name);
 	if (IS_ERR(cd))
 		return PTR_ERR(cd);
 	
@@ -280,7 +280,7 @@ int register_chrdev(unsigned int major,
 	for (s = strchr(kobject_name(&cdev->kobj),'/'); s; s = strchr(s, '/'))
 		*s = '!';
 		
-	err = cdev_add(cdev, MKDEV(cd->major, 0), 256);
+	err = cdev_add(cdev, MKDEV(cd->major, baseminor), count);
 	if (err)
 		goto out;

@@ -290,11 +290,26 @@ int register_chrdev(unsigned int major,
 out:
 	kobject_put(&cdev->kobj);
 out2:
-	kfree(__unregister_chrdev_region(cd->major, 0, 256));
+	kfree(__unregister_chrdev_region(cd->major, baseminor, count));
 	return err;
 }

 /**
+ * register_chrdev() - create and register a cdev occupying a full major
+ * @major: major device number or 0 for dynamic allocation
+ * @name: name of this range of devices
+ * @fops: file operations associated with this devices
+ *
+ * Wrapper around __register_chrdev() which always allocates full
+ * minor range from 0 to 256.
+ */
+int register_chrdev(unsigned int major, const char *name,
+		    const struct file_operations *fops)
+{
+	return __register_chrdev(major, 0, 256, name, fops);
+}
+
+/**
  * unregister_chrdev_region() - return a range of device numbers
  * @from: the first in the range of numbers to unregister
  * @count: the number of device numbers to unregister
@@ -316,15 +331,41 @@ void unregister_chrdev_region(dev_t from
 	}
 }

-void unregister_chrdev(unsigned int major, const char *name)
+/**
+ * __unregister_chrdev - unregister and destroy a cdev
+ * @major: major device number
+ * @baseminor: first of the range of minor numbers
+ * @count: the number of minor numbers this cdev is occupying
+ * @name: name of this range of devices
+ *
+ * Unregister and destroy the cdev occupying the region described by
+ * @major, @baseminor and @count.  This function undoes what
+ * __register_chrdev() did.
+ */
+void __unregister_chrdev(unsigned int major, unsigned int baseminor,
+			 unsigned int count, const char *name)
 {
 	struct char_device_struct *cd;
-	cd = __unregister_chrdev_region(major, 0, 256);
+
+	cd = __unregister_chrdev_region(major, baseminor, count);
 	if (cd && cd->cdev)
 		cdev_del(cd->cdev);
 	kfree(cd);
 }

+/**
+ * unregister_chrdev - unregister and destroy a cdev
+ * @major: major device number
+ * @name: name of this range of devices
+ *
+ * Wrapper around __unregister_chrdev() which assumes full minor range
+ * from 0 to 256.
+ */
+void unregister_chrdev(unsigned int major, const char *name)
+{
+	__unregister_chrdev(major, 0, 256, name);
+}
+
 static DEFINE_SPINLOCK(cdev_lock);

 static struct kobject *cdev_get(struct cdev *p)
Index: work/include/linux/fs.h
===================================================================
--- work.orig/include/linux/fs.h
+++ work/include/linux/fs.h
@@ -1998,9 +1998,14 @@ extern void bd_release_from_disk(struct
 #define CHRDEV_MAJOR_HASH_SIZE	255
 extern int alloc_chrdev_region(dev_t *, unsigned, unsigned, const char *);
 extern int register_chrdev_region(dev_t, unsigned, const char *);
-extern int register_chrdev(unsigned int, const char *,
-			   const struct file_operations *);
-extern void unregister_chrdev(unsigned int, const char *);
+extern int __register_chrdev(unsigned int major, unsigned int baseminor,
+			     unsigned int count, const char *name,
+			     const struct file_operations *fops);
+extern int register_chrdev(unsigned int major, const char *name,
+			   const struct file_operations *fops);
+extern void __unregister_chrdev(unsigned int major, unsigned int baseminor,
+				unsigned int count, const char *name);
+extern void unregister_chrdev(unsigned int major, const char *name);
 extern void unregister_chrdev_region(dev_t, unsigned);
 extern void chrdev_show(struct seq_file *,off_t);

[PATCH 2/2] sound: make OSS device number claiming optional

[Tejun Heo]

If any OSS support is enabled, regardless of built-in or module,
sound_core claims full OSS major number (that is, the old 0-255
region) to trap open attempts and request sound module automatically.
This feature is redundant as chrdev already has such mechanism and no
longer used by modern distros.  This preemptive claiming prevents
alternative OSS implementation.

This patch makes the preclaiming optional and adds a config option
SOUND_OSS_CORE_PRECLAIM and kernel parameter soundcore.preclaim_oss to
control whether preclaim is enabled or not.  This allows distros and
developers to try new things without rebuilding kernel.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Colin Guthrie <cguthrie@mandriva.org>
---
Hello, Takashi.

This patch is to allow using ossp without needing to recompile the
kernel.  The affected code is mostly deprecated.  chrdev already has
exactly about the same mechanism although it would use different
alises and AFAIK udev based modern distros just don't use it at all
(SUSE doesn't at least).  I thought about removing the code altogether
but this part of code is hopefully in slow death so the least amount
change should be the best.

Thanks.

 sound/Kconfig      |   20 +++++++++++++++
 sound/sound_core.c |   68 ++++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 67 insertions(+), 21 deletions(-)

Index: work/sound/Kconfig
===================================================================
--- work.orig/sound/Kconfig
+++ work/sound/Kconfig
@@ -32,6 +32,26 @@ config SOUND_OSS_CORE
 	bool
 	default n

+config SOUND_OSS_CORE_PRECLAIM
+	bool "Preclaim OSS device numbers"
+	depends on SOUND_OSS_CORE
+	default y
+	help
+	  With this option enabled, the kernel will claim all OSS device
+	  numbers if any OSS support (native or emulation) is enabled
+	  whether the respective module is loaded or not and, if necessary,
+	  try to load the appropriate module when one of the device numbers
+	  is opened.  With this option disabled, kernel will only claim
+	  actually in-use device numbers and module loading is left to
+	  userland.
+
+	  Disabling it allows alternative out-of-kernel OSS implementation.
+
+	  The behavior can be overridden using kernel parameter
+	  soundcore.preclaim_oss.
+
+	  If unusre, say Y.
+
 source "sound/oss/dmasound/Kconfig"

 if !M68K
Index: work/sound/sound_core.c
===================================================================
--- work.orig/sound/sound_core.c
+++ work/sound/sound_core.c
@@ -127,6 +127,23 @@ extern int msnd_classic_init(void);
 extern int msnd_pinnacle_init(void);
 #endif

+#ifdef CONFIG_SOUND_OSS_CORE_PRECLAIM
+static int preclaim_oss = 1;
+#else
+static int preclaim_oss = 0;
+#endif
+
+module_param(preclaim_oss, int, 0444);
+
+static int soundcore_open(struct inode *, struct file *);
+
+static const struct file_operations soundcore_fops =
+{
+	/* We must have an owner or the module locking fails */
+	.owner	= THIS_MODULE,
+	.open	= soundcore_open,
+};
+
 /*
  *	Low level list operator. Scan the ordered list, find a hole and
  *	join into it. Called with the lock asserted
@@ -215,7 +232,8 @@ static DEFINE_SPINLOCK(sound_loader_lock
 static int sound_insert_unit(struct sound_unit **list, const struct file_operations *fops, int index, int low, int top, const char *name, umode_t mode, struct device *dev)
 {
 	struct sound_unit *s = kmalloc(sizeof(*s), GFP_KERNEL);
-	int r;
+	struct device *gdev;
+	int r, minor;

 	if (!s)
 		return -ENOMEM;
@@ -225,17 +243,34 @@ static int sound_insert_unit(struct soun
 	spin_unlock(&sound_loader_lock);
 	
 	if (r < 0)
-		goto fail;
+		goto fail_free;
 	else if (r < SOUND_STEP)
 		sprintf(s->name, "sound/%s", name);
 	else
 		sprintf(s->name, "sound/%s%d", name, r / SOUND_STEP);
+	minor = r;

-	device_create(sound_class, dev, MKDEV(SOUND_MAJOR, s->unit_minor),
-		      NULL, s->name+6);
-	return r;
+	if (!preclaim_oss) {
+		r = __register_chrdev(SOUND_MAJOR, s->unit_minor, 1, s->name,
+				      &soundcore_fops);
+		if (r < 0)
+			goto fail_remove;
+	}

- fail:
+	gdev = device_create(sound_class, dev,
+			     MKDEV(SOUND_MAJOR, s->unit_minor), NULL,
+			     s->name + 6);
+	if (IS_ERR(gdev)) {
+		r = PTR_ERR(gdev);
+		goto fail_destroy;
+	}
+	return minor;
+
+fail_destroy:
+	device_destroy(sound_class, MKDEV(SOUND_MAJOR, s->unit_minor));
+fail_remove:
+	__sound_remove_unit(list, s->unit_minor);
+fail_free:
 	kfree(s);
 	return r;
 }
@@ -254,6 +289,9 @@ static void sound_remove_unit(struct sou
 	p = __sound_remove_unit(list, unit);
 	spin_unlock(&sound_loader_lock);
 	if (p) {
+		if (!preclaim_oss)
+			__unregister_chrdev(SOUND_MAJOR, p->unit_minor, 1,
+					    p->name);
 		device_destroy(sound_class, MKDEV(SOUND_MAJOR, p->unit_minor));
 		kfree(p);
 	}
@@ -491,19 +529,6 @@ void unregister_sound_dsp(int unit)

 EXPORT_SYMBOL(unregister_sound_dsp);

-/*
- *	Now our file operations
- */
-
-static int soundcore_open(struct inode *, struct file *);
-
-static const struct file_operations soundcore_fops=
-{
-	/* We must have an owner or the module locking fails */
-	.owner	= THIS_MODULE,
-	.open	= soundcore_open,
-};
-
 static struct sound_unit *__look_for_unit(int chain, int unit)
 {
 	struct sound_unit *s;
@@ -539,7 +564,7 @@ static int soundcore_open(struct inode *
 	s = __look_for_unit(chain, unit);
 	if (s)
 		new_fops = fops_get(s->unit_fops);
-	if (!new_fops) {
+	if (preclaim_oss && !new_fops) {
 		spin_unlock(&sound_loader_lock);
 		/*
 		 *  Please, don't change this order or code.
@@ -593,7 +618,8 @@ static void cleanup_oss_soundcore(void)

 static int __init init_oss_soundcore(void)
 {
-	if (register_chrdev(SOUND_MAJOR, "sound", &soundcore_fops)==-1) {
+	if (preclaim_oss &&
+	    register_chrdev(SOUND_MAJOR, "sound", &soundcore_fops) == -1) {
 		printk(KERN_ERR "soundcore: sound device already in use.\n");
 		return -EBUSY;
 	}

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Takashi Iwai]

At Wed, 05 Aug 2009 15:35:42 +0900,
Tejun Heo wrote:
> 
> [un]register_chrdev() assume minor range 0-255.  This patch adds __
> prefixed versions which take @minorbase and @count explicitly.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> ---
> Hello,
> 
> These two patches make sound_core grabbing OSS device number optional.
> If there's no objection, I think it would be easiest to push this
> through Takashi's tree.

I'm fine to take this.

But, I think EXPORT_SYMBOL(__register_chrdev) and
EXPORT_SYMBOL(__unregister_chrdev) are missing in this patch.


thanks,

Takashi

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Tejun Heo]

Takashi Iwai wrote:
> At Wed, 05 Aug 2009 15:35:42 +0900,
> Tejun Heo wrote:
>> [un]register_chrdev() assume minor range 0-255.  This patch adds __
>> prefixed versions which take @minorbase and @count explicitly.
>>
>> Signed-off-by: Tejun Heo <tj@kernel.org>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Cc: Greg Kroah-Hartman <gregkh@suse.de>
>> ---
>> Hello,
>>
>> These two patches make sound_core grabbing OSS device number optional.
>> If there's no objection, I think it would be easiest to push this
>> through Takashi's tree.
> 
> I'm fine to take this.
> 
> But, I think EXPORT_SYMBOL(__register_chrdev) and
> EXPORT_SYMBOL(__unregister_chrdev) are missing in this patch.

The only current users would be sound_core.c which is always compiled
built-in.  Adrian would be mighty unhappy about adding EXPORT_SYMBOL()
there.

Thanks.

-- 
tejun

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Takashi Iwai]

At Wed, 05 Aug 2009 16:11:21 +0900,
Tejun Heo wrote:
> 
> Takashi Iwai wrote:
> > At Wed, 05 Aug 2009 15:35:42 +0900,
> > Tejun Heo wrote:
> >> [un]register_chrdev() assume minor range 0-255.  This patch adds __
> >> prefixed versions which take @minorbase and @count explicitly.
> >>
> >> Signed-off-by: Tejun Heo <tj@kernel.org>
> >> Cc: Al Viro <viro@zeniv.linux.org.uk>
> >> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> >> ---
> >> Hello,
> >>
> >> These two patches make sound_core grabbing OSS device number optional.
> >> If there's no objection, I think it would be easiest to push this
> >> through Takashi's tree.
> > 
> > I'm fine to take this.
> > 
> > But, I think EXPORT_SYMBOL(__register_chrdev) and
> > EXPORT_SYMBOL(__unregister_chrdev) are missing in this patch.
> 
> The only current users would be sound_core.c which is always compiled
> built-in. 

CONFIG_SOUND is tristate, so it can be a module.
Actually SUSE kernels have it as a module :)

> Adrian would be mighty unhappy about adding EXPORT_SYMBOL()
> there.

How about to replace the old *register_chrdev() with static inline,
instead?


thanks,

Takashi

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Tejun Heo]

Hello,

Takashi Iwai wrote:
> CONFIG_SOUND is tristate, so it can be a module.
> Actually SUSE kernels have it as a module :)

Ah... crap.  I got confused.  I somehow thought it was always included
in kernel.

>> Adrian would be mighty unhappy about adding EXPORT_SYMBOL()
>> there.
> 
> How about to replace the old *register_chrdev() with static inline,
> instead?

Will do so.

Thanks.

-- 
tejun

[PATCH 1/2 UPDATED] chrdev: implement __[un]register_chrdev()

[Tejun Heo]

[un]register_chrdev() assume minor range 0-255.  This patch adds __
prefixed versions which take @minorbase and @count explicitly.  The
original ones become inline wrappers around __ ones.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 fs/char_dev.c      |   39 ++++++++++++++++++++++++++-------------
 include/linux/fs.h |   19 ++++++++++++++++---
 2 files changed, 42 insertions(+), 16 deletions(-)

Index: work/fs/char_dev.c
===================================================================
--- work.orig/fs/char_dev.c
+++ work/fs/char_dev.c
@@ -237,8 +237,10 @@ int alloc_chrdev_region(dev_t *dev, unsi
 }

 /**
- * register_chrdev() - Register a major number for character devices.
+ * __register_chrdev() - create and register a cdev occupying a range of minors
  * @major: major device number or 0 for dynamic allocation
+ * @baseminor: first of the requested range of minor numbers
+ * @count: the number of minor numbers required
  * @name: name of this range of devices
  * @fops: file operations associated with this devices
  *
@@ -254,19 +256,17 @@ int alloc_chrdev_region(dev_t *dev, unsi
  * /dev. It only helps to keep track of the different owners of devices. If
  * your module name has only one type of devices it's ok to use e.g. the name
  * of the module here.
- *
- * This function registers a range of 256 minor numbers. The first minor number
- * is 0.
  */
-int register_chrdev(unsigned int major, const char *name,
-		    const struct file_operations *fops)
+int __register_chrdev(unsigned int major, unsigned int baseminor,
+		      unsigned int count, const char *name,
+		      const struct file_operations *fops)
 {
 	struct char_device_struct *cd;
 	struct cdev *cdev;
 	char *s;
 	int err = -ENOMEM;

-	cd = __register_chrdev_region(major, 0, 256, name);
+	cd = __register_chrdev_region(major, baseminor, count, name);
 	if (IS_ERR(cd))
 		return PTR_ERR(cd);
 	
@@ -280,7 +280,7 @@ int register_chrdev(unsigned int major,
 	for (s = strchr(kobject_name(&cdev->kobj),'/'); s; s = strchr(s, '/'))
 		*s = '!';
 		
-	err = cdev_add(cdev, MKDEV(cd->major, 0), 256);
+	err = cdev_add(cdev, MKDEV(cd->major, baseminor), count);
 	if (err)
 		goto out;

@@ -290,7 +290,7 @@ int register_chrdev(unsigned int major,
 out:
 	kobject_put(&cdev->kobj);
 out2:
-	kfree(__unregister_chrdev_region(cd->major, 0, 256));
+	kfree(__unregister_chrdev_region(cd->major, baseminor, count));
 	return err;
 }

@@ -316,10 +316,23 @@ void unregister_chrdev_region(dev_t from
 	}
 }

-void unregister_chrdev(unsigned int major, const char *name)
+/**
+ * __unregister_chrdev - unregister and destroy a cdev
+ * @major: major device number
+ * @baseminor: first of the range of minor numbers
+ * @count: the number of minor numbers this cdev is occupying
+ * @name: name of this range of devices
+ *
+ * Unregister and destroy the cdev occupying the region described by
+ * @major, @baseminor and @count.  This function undoes what
+ * __register_chrdev() did.
+ */
+void __unregister_chrdev(unsigned int major, unsigned int baseminor,
+			 unsigned int count, const char *name)
 {
 	struct char_device_struct *cd;
-	cd = __unregister_chrdev_region(major, 0, 256);
+
+	cd = __unregister_chrdev_region(major, baseminor, count);
 	if (cd && cd->cdev)
 		cdev_del(cd->cdev);
 	kfree(cd);
@@ -568,6 +581,6 @@ EXPORT_SYMBOL(cdev_alloc);
 EXPORT_SYMBOL(cdev_del);
 EXPORT_SYMBOL(cdev_add);
 EXPORT_SYMBOL(cdev_index);
-EXPORT_SYMBOL(register_chrdev);
-EXPORT_SYMBOL(unregister_chrdev);
+EXPORT_SYMBOL(__register_chrdev);
+EXPORT_SYMBOL(__unregister_chrdev);
 EXPORT_SYMBOL(directly_mappable_cdev_bdi);
Index: work/include/linux/fs.h
===================================================================
--- work.orig/include/linux/fs.h
+++ work/include/linux/fs.h
@@ -1998,12 +1998,25 @@ extern void bd_release_from_disk(struct
 #define CHRDEV_MAJOR_HASH_SIZE	255
 extern int alloc_chrdev_region(dev_t *, unsigned, unsigned, const char *);
 extern int register_chrdev_region(dev_t, unsigned, const char *);
-extern int register_chrdev(unsigned int, const char *,
-			   const struct file_operations *);
-extern void unregister_chrdev(unsigned int, const char *);
+extern int __register_chrdev(unsigned int major, unsigned int baseminor,
+			     unsigned int count, const char *name,
+			     const struct file_operations *fops);
+extern void __unregister_chrdev(unsigned int major, unsigned int baseminor,
+				unsigned int count, const char *name);
 extern void unregister_chrdev_region(dev_t, unsigned);
 extern void chrdev_show(struct seq_file *,off_t);

+static inline int register_chrdev(unsigned int major, const char *name,
+				  const struct file_operations *fops)
+{
+	return __register_chrdev(major, 0, 256, name, fops);
+}
+
+static inline void unregister_chrdev(unsigned int major, const char *name)
+{
+	__unregister_chrdev(major, 0, 256, name);
+}
+
 /* fs/block_dev.c */
 #define BDEVNAME_SIZE	32	/* Largest string for a blockdev identifier */
 #define BDEVT_SIZE	10	/* Largest string for MAJ:MIN for blkdev */

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Alan Cox]

On Wed, 05 Aug 2009 15:40:42 +0900
Tejun Heo <teheo@suse.de> wrote:

> If any OSS support is enabled, regardless of built-in or module,
> sound_core claims full OSS major number (that is, the old 0-255
> region) to trap open attempts and request sound module automatically.
> This feature is redundant as chrdev already has such mechanism and no
> longer used by modern distros.  This preemptive claiming prevents
> alternative OSS implementation.
> 
> This patch makes the preclaiming optional and adds a config option
> SOUND_OSS_CORE_PRECLAIM and kernel parameter soundcore.preclaim_oss to
> control whether preclaim is enabled or not.  This allows distros and
> developers to try new things without rebuilding kernel.

This looks like a random private devel hack sand I don't see why it's
appropriate for mainstream, especially as these "new things" don't exist
mainstream either.

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Colin Guthrie]

'Twas brillig, and Alan Cox at 05/08/09 10:15 did gyre and gimble:
> On Wed, 05 Aug 2009 15:40:42 +0900
> Tejun Heo <teheo@suse.de> wrote:
> 
>> If any OSS support is enabled, regardless of built-in or module,
>> sound_core claims full OSS major number (that is, the old 0-255
>> region) to trap open attempts and request sound module automatically.
>> This feature is redundant as chrdev already has such mechanism and no
>> longer used by modern distros.  This preemptive claiming prevents
>> alternative OSS implementation.
>>
>> This patch makes the preclaiming optional and adds a config option
>> SOUND_OSS_CORE_PRECLAIM and kernel parameter soundcore.preclaim_oss to
>> control whether preclaim is enabled or not.  This allows distros and
>> developers to try new things without rebuilding kernel.
> 
> This looks like a random private devel hack sand I don't see why it's
> appropriate for mainstream, especially as these "new things" don't exist
> mainstream either.

While I'm not familiar enough with the kernel itself to comment on *how* 
the patch works, the principle is quite important here.

As far as I know most distros enable snd-*-oss module loading via a 
modprobe trick (e.g. in user space) so the change here shouldn't affect 
this approach.

However, I certainly want to experiment with osspd and while I hope it's 
going to be a good solution generally, it'll only make sense if the user 
chooses to use pulseaudio.

If the user decided they want to use pure alsa, then they'll have to 
drop back to using snd-*-oss for that instead.

This is all something I'd like to enable without making the user change 
kernels based on a preference.

So if this patch is not accepted, I'd still like to see the kernel not 
claim this device number, and leave it up to userspace to load the modules.

Sorry if this is not all factually accurate, but I think i've got the 
gist right! :)

-- 

Colin Guthrie
cguthrie(at)mandriva.org
http://colin.guthr.ie/

Day Job:
   Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
   Mandriva Linux Contributor [http://www.mandriva.com/]
   PulseAudio Hacker [http://www.pulseaudio.org/]
   Trac Hacker [http://trac.edgewall.org/]

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Tejun Heo]

Hello, Alan.

Alan Cox wrote:
> This looks like a random private devel hack sand

Why so?

> I don't see why it's appropriate for mainstream, especially as these
> "new things" don't exist mainstream either.

The new things is CUSE and it's already in mainstream.

Thanks.

-- 
tejun

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Alan Cox]

> While I'm not familiar enough with the kernel itself to comment on *how* 
> the patch works, the principle is quite important here.
> 
> As far as I know most distros enable snd-*-oss module loading via a 
> modprobe trick (e.g. in user space) so the change here shouldn't affect 
> this approach.
> 
> However, I certainly want to experiment with osspd and while I hope it's 
> going to be a good solution generally, it'll only make sense if the user 
> chooses to use pulseaudio.
> 
> If the user decided they want to use pure alsa, then they'll have to 
> drop back to using snd-*-oss for that instead.
> 
> This is all something I'd like to enable without making the user change 
> kernels based on a preference.

As you said "I'd like to experiment"

We don't put everyones random experiments in the kernel or we'd have a
kernel that had a million unfinished project hacks.

Suppose this doesn't work out - how do we get rid of the new hack,
someone might by then have decided to depend upon it. More crud, more
APIs we can't get rid of

More importantly there is an API *in* soundcore for registering sound
devices in the OSS category. You can use that.

Not only was that interface written precisely to allow the kind of
sharing in question but it will also let you mix OSS, ALSA *and* your own
experimental device work at the same time because it hands out minor
numbers properly.

Given we have an existing proper interface for this I think this hack
belongs in a private dev tree only. In fact I suspect it belongs in the
bitbucket and experimental code should be using the proper provided and
exported interfaces as it will need to if it is ever going to be accepted
upstream.

NAK

Alan

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Alan Cox]

On Wed, 05 Aug 2009 18:32:32 +0900
Tejun Heo <teheo@suse.de> wrote:

> Hello, Alan.
> 
> Alan Cox wrote:
> > This looks like a random private devel hack sand
> 
> Why so?
> 
> > I don't see why it's appropriate for mainstream, especially as these
> > "new things" don't exist mainstream either.
> 
> The new things is CUSE and it's already in mainstream.

Then fix it to use the sound interfaces properly. We have a multiplexor
for sound devices so they can be shared between OSS/ALSA/whatever. Use it.
Ditto if you want CUSE to handle miscdevices use the miscdevice interface
properly.

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Takashi Iwai]

At Wed, 5 Aug 2009 10:59:16 +0100,
Alan Cox wrote:
> 
> > While I'm not familiar enough with the kernel itself to comment on *how* 
> > the patch works, the principle is quite important here.
> > 
> > As far as I know most distros enable snd-*-oss module loading via a 
> > modprobe trick (e.g. in user space) so the change here shouldn't affect 
> > this approach.
> > 
> > However, I certainly want to experiment with osspd and while I hope it's 
> > going to be a good solution generally, it'll only make sense if the user 
> > chooses to use pulseaudio.
> > 
> > If the user decided they want to use pure alsa, then they'll have to 
> > drop back to using snd-*-oss for that instead.
> > 
> > This is all something I'd like to enable without making the user change 
> > kernels based on a preference.
> 
> As you said "I'd like to experiment"
> 
> We don't put everyones random experiments in the kernel or we'd have a
> kernel that had a million unfinished project hacks.
> 
> Suppose this doesn't work out - how do we get rid of the new hack,
> someone might by then have decided to depend upon it. More crud, more
> APIs we can't get rid of
> 
> More importantly there is an API *in* soundcore for registering sound
> devices in the OSS category. You can use that.
> 
> Not only was that interface written precisely to allow the kind of
> sharing in question but it will also let you mix OSS, ALSA *and* your own
> experimental device work at the same time because it hands out minor
> numbers properly.
> 
> Given we have an existing proper interface for this I think this hack
> belongs in a private dev tree only. In fact I suspect it belongs in the
> bitbucket and experimental code should be using the proper provided and
> exported interfaces as it will need to if it is ever going to be accepted
> upstream.
> 
> NAK
> 
> Alan

As far as I understand, the current problem is that ALSA core becomes
dependent when you build with CONFIG_SND_*_OSS config, even if you
load no snd-*-oss modules and use only ALSA-native ones.  It's because
the OSS device registration is done through the ALSA snd module.
Thus, loading ALSA modules always results in loading soundcore
module, too.

So... instead of hacking around soundcore stuff, splitting off the OSS
(soundcore) dependency from ALSA snd module could be another solution?
Then soundcore doesn't have to be loaded unless you load snd-*-oss or
any real OSS modules.

One drawback is that this splits to yet another module.  Or, maybe it
can be put into somewhere else...


Takashi

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Alan Cox]

> As far as I understand, the current problem is that ALSA core becomes
> dependent when you build with CONFIG_SND_*_OSS config, even if you
> load no snd-*-oss modules and use only ALSA-native ones.  It's because
> the OSS device registration is done through the ALSA snd module.
> Thus, loading ALSA modules always results in loading soundcore
> module, too.

That isn't a bug, but a feature.

> So... instead of hacking around soundcore stuff, splitting off the OSS
> (soundcore) dependency from ALSA snd module could be another solution?
> Then soundcore doesn't have to be loaded unless you load snd-*-oss or
> any real OSS modules.

This makes it worse.

> One drawback is that this splits to yet another module.  Or, maybe it
> can be put into somewhere else...

Currently I can load bits of OSS driver, ALSA drivers and something else
at the same time and soundcore will properly share the major number
between devices and do allocations (Just as miscdevice does with its
major).

If people start hacking around the interface rather than using it
properly then the end user is left with

- obscure magical configuration Kconfig items
- a weird boot parameter/module option
- no ability to mix and match

Exactly why is that a forward step compared with teaching CUSE to behave
like a civilised kernel driver with regard to sound ?

If CUSE/Sound is fixed to use the sound APIs then lo and behold

- no weird options
- no weird boot parameters
- mix and match just works

no user intervention, no complexity to the end user, and more flexible
all around.

(Its also better for debug as you can load two different versions of the
CUSE/Sound stack at once ...)

Hence I still think this patch is an ugly pointless hack that is nothing
but private debug and quick hack stuff. Teach CUSE to behave and
everything works nicely. CUSE needs this sort of additional plug in
support anyway in order to do anything with it involving the other
multiplexed device interfaces (eg tty, video4linux, framebuffer, usb, ...)

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Takashi Iwai]

At Wed, 5 Aug 2009 11:26:49 +0100,
Alan Cox wrote:
> 
> > As far as I understand, the current problem is that ALSA core becomes
> > dependent when you build with CONFIG_SND_*_OSS config, even if you
> > load no snd-*-oss modules and use only ALSA-native ones.  It's because
> > the OSS device registration is done through the ALSA snd module.
> > Thus, loading ALSA modules always results in loading soundcore
> > module, too.
> 
> That isn't a bug, but a feature.

Not really :)  The soundcore is only for OSS device files.  As long as
you use only ALSA-native APIs, you don't need it.  But, currently it's
loaded and initialized just because of the module dependency.  This is
no bug but also not an intended feature.

> > So... instead of hacking around soundcore stuff, splitting off the OSS
> > (soundcore) dependency from ALSA snd module could be another solution?
> > Then soundcore doesn't have to be loaded unless you load snd-*-oss or
> > any real OSS modules.
> 
> This makes it worse.

Well, the only regression would be the case where you create static
/dev/dsp (or else) devices and let auto-loading through sound-slot-*
or sound-service-*-* aliases.  Of course, this still works if you
load soundcore in some way.

What I suggested in the above is to cut off an unneeded dependency
between soundcore and ALSA-native stuff instead of hacking soundcore.
It won't change anything else, so everything else can coexist as
before.


Just my $0.02, though...

Takashi

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Alan Cox]

> Not really :)  The soundcore is only for OSS device files.  As long as
> you use only ALSA-native APIs, you don't need it.  But, currently it's
> loaded and initialized just because of the module dependency.  This is
> no bug but also not an intended feature.

Sure for the ALSA device node it makes sense because ALSA is the mux for
it. Fortunately soundcore is tiny anyway.

> > This makes it worse.
> 
> Well, the only regression would be the case where you create static
> /dev/dsp (or else) devices and let auto-loading through sound-slot-*
> or sound-service-*-* aliases.  Of course, this still works if you
> load soundcore in some way.

Unless some ugly cuse hack got there first.

> What I suggested in the above is to cut off an unneeded dependency
> between soundcore and ALSA-native stuff instead of hacking soundcore.
> It won't change anything else, so everything else can coexist as
> before.

Agreed - but that is really a separate issue to having something break
the soundcore by being rude.

Alan

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Tejun Heo]

Hello, Alan.

Alan Cox wrote:
> On Wed, 05 Aug 2009 18:32:32 +0900
> Tejun Heo <teheo@suse.de> wrote:
> 
>> Hello, Alan.
>>
>> Alan Cox wrote:
>>> This looks like a random private devel hack sand
>> Why so?
>>
>>> I don't see why it's appropriate for mainstream, especially as these
>>> "new things" don't exist mainstream either.
>> The new things is CUSE and it's already in mainstream.
> 
> Then fix it to use the sound interfaces properly. We have a multiplexor
> for sound devices so they can be shared between OSS/ALSA/whatever. Use it.
> Ditto if you want CUSE to handle miscdevices use the miscdevice interface
> properly.

The only problem here is the now obsolete hackish way OSS module
auto-loading is handled.  CUSE is being a regular chardev citizen.
The OSS multiplexing mechanism itself isn't a problem and the posted
patch doesn't distort it in any way but by preclaiming all OSS device
numbers, OSS requires its own different rule for no good reason.  The
only actual difference here would be what module aliases are used.

Given the situation, I sure can add hacks to CUSE so that it doesn't
do regular chardev thing but does OSS specific stuff if OSS major is
detected.  To me, this would be much painful exercise in spreading the
hack.

The proper solution would be extending chardev such that OSS sound
core can use its own custom module aliases for backward compatibility.
But given that OSS is about the only user left for such facility and
modern distros don't depend on the feature or can easily work around
it from userland, that would be an overkill.

Another option is to rip the custom module requesting code altogether.
It just doesn't matter at all anymore.  modprobe is smart enough to
work around those issues with a couple lines of configuration changes.
We can add those alises to feature-removal-schedule.txt and then kill
it in a year or so.  If OSS is still alive and well, I would have
chosen this path.

However, in-kernel OSS is largely dead or at least is dying a slow
death.  Native OSS drivers are no longer in development and the only
left user is in-kernel emulation from ALSA, which I'm sure have its
places but is getting more and more inadequate for general desktop
usage due to lack of software multiplexing.

I don't see much reason to go through feature removal process for the
now obsolete OSS-specific module requesting feature.  Allowing distros
to make the switch as they see fit and letting the code wither away is
the least painful way.

So, in summary, the hack here is how OSS does custom module requesting
and the proper solution would be removing the mis-feature according to
our feature removal protocol or reimplementing it in a non-hackish
way, both of which are unnecessarily painful and overkill at this
point.

Thanks.

-- 
tejun

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Tejun Heo]

Alan Cox wrote:
>>> This makes it worse.
>> Well, the only regression would be the case where you create static
>> /dev/dsp (or else) devices and let auto-loading through sound-slot-*
>> or sound-service-*-* aliases.  Of course, this still works if you
>> load soundcore in some way.
> 
> Unless some ugly cuse hack got there first.
> 
>> What I suggested in the above is to cut off an unneeded dependency
>> between soundcore and ALSA-native stuff instead of hacking soundcore.
>> It won't change anything else, so everything else can coexist as
>> before.
> 
> Agreed - but that is really a separate issue to having something break
> the soundcore by being rude.

Sigh, does the wording really have to be 'ugly' and 'rude'?
sound_core.c itself is a strange hack at this point.  :-(

-- 
tejun

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Tejun Heo]

Takashi Iwai wrote:
> Well, the only regression would be the case where you create static
> /dev/dsp (or else) devices and let auto-loading through sound-slot-*
> or sound-service-*-* aliases.  Of course, this still works if you
> load soundcore in some way.

If we're gonna do that, we might as well just rip off the pointless
'claim everything so we can use our own module alises' code from
sound_core.c and be done with it.  That really is the only problem.

Thanks.

-- 
tejun

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Alan Cox]

> The only problem here is the now obsolete hackish way OSS module
> auto-loading is handled.  CUSE is being a regular chardev citizen.

"obsolete hackish". It's the same mechanism used by just about every
other driver that multiplexes. Doesn't seem obsolete to me. You are
expected to register subdrivers of most specialist interfaces through
that interface. You use misc_register, video_register_device etc.

Providing you do so it all works nicely.

The OSS device major is owned by OSS (soundcore). If you want to use it
your driver should use the interfaces provided by soundcore. Ditto any
other similar interface.

If you want to completely pre-empt soundcore then use a different
(probably dynamic) major number and tweak your udev rules to create
different /dev files.

None of this requires ugly kernel hacks and weird user config options and
module params.

> Given the situation, I sure can add hacks to CUSE so that it doesn't
> do regular chardev thing but does OSS specific stuff if OSS major is
> detected.  To me, this would be much painful exercise in spreading the
> hack.

And it can then be a good citizen and share the resource. CUSE needs to
do the same for video/misc/framebuffer/tty/usb/etc so this is a general
pattern it needs to handle. In short cuse needs to grow this ability
anyway if it wants to be support these sorts of device.

> However, in-kernel OSS is largely dead or at least is dying a slow
> death.  Native OSS drivers are no longer in development and the only
> left user is in-kernel emulation from ALSA, which I'm sure have its
> places but is getting more and more inadequate for general desktop
> usage due to lack of software multiplexing.

The desktop uses pulseaudio for the most part (or jack in some setups).
OSS uses are pretty rare now that flash has gone away. Unfortunately
many of those existing users aren't terribly fixable
 
> I don't see much reason to go through feature removal process for the
> now obsolete OSS-specific module requesting feature.  Allowing distros
> to make the switch as they see fit and letting the code wither away is
> the least painful way.

You appear to be self contradictory here. Either

- OSS is irrelevant in which case we don't need the CUSE stuff either
- OSS is relevant in which case we shouldn't break it

A few old old games still use OSS, are proprietary apps and have never
been re-released. Some of them also do irritiating to emulate things like
use mmap. Flash at least appears to have finally gotten the message.

Alan

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Alan Cox]

> If we're gonna do that, we might as well just rip off the pointless
> 'claim everything so we can use our own module alises' code from
> sound_core.c and be done with it.  That really is the only problem.

The interface expected is the sound_register_* interface and the sharing
is likewise expected so you could for example mix OSS. CUSE and ALSA oss
emulation drivers on the same system at the same time.

Rewriting sound_register* in terms of minor number allocations using the
fact modern kernels have better allocators for devices would also be
preferable to the weird module hacks initially proposed. I guess a
starting point would be to tweak soundcore to generate both message sets
itself for a year and then pull the old stuff out.

Alan

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Tejun Heo]

Hello, Alan.

Alan Cox wrote:
>> The only problem here is the now obsolete hackish way OSS module
>> auto-loading is handled.  CUSE is being a regular chardev citizen.
> 
> "obsolete hackish". It's the same mechanism used by just about every
> other driver that multiplexes. Doesn't seem obsolete to me. You are
> expected to register subdrivers of most specialist interfaces through
> that interface. You use misc_register, video_register_device etc.

Subsystem registration isn't hackish or obsolete but claiming all
minor devices to use custom module alises is at best obsolete.  There
just is no reason to do that.

> Providing you do so it all works nicely.
> 
> The OSS device major is owned by OSS (soundcore). If you want to use it
> your driver should use the interfaces provided by soundcore. Ditto any
> other similar interface.

None of this would be a problem if OSS doesn't claim minors it doesn't
have actual device for and as I said multiple times there is no
benefit in doing that, not anymore and the only reason to keep that
around is to maintain backward compatibility for a mostly unused and
easy to work around feature.

>> Given the situation, I sure can add hacks to CUSE so that it doesn't
>> do regular chardev thing but does OSS specific stuff if OSS major is
>> detected.  To me, this would be much painful exercise in spreading the
>> hack.
> 
> And it can then be a good citizen and share the resource. CUSE needs to
> do the same for video/misc/framebuffer/tty/usb/etc so this is a general
> pattern it needs to handle. In short cuse needs to grow this ability
> anyway if it wants to be support these sorts of device.

The sharing and mixing and matching can happen very gracefully at
chrdev level.  Does it make sense to drag it down to each subsystem
level just to use custom module alises?  Not at all to me.  The same
goes for video or whatever.  There is no reason to claim unused device
numbers anymore and as long as that holds everyone is good citizen at
the chrdev region.  That's _much_ cleaner.

>> I don't see much reason to go through feature removal process for the
>> now obsolete OSS-specific module requesting feature.  Allowing distros
>> to make the switch as they see fit and letting the code wither away is
>> the least painful way.
> 
> You appear to be self contradictory here. Either
> 
> - OSS is irrelevant in which case we don't need the CUSE stuff either
> - OSS is relevant in which case we shouldn't break it

What's broken?  Module aliases?  The whole point of the patch is to
let people keep the module alises while allowing distros which wish to
try or switch to different mechanism to do so.

> A few old old games still use OSS, are proprietary apps and have never
> been re-released. Some of them also do irritiating to emulate things like
> use mmap. Flash at least appears to have finally gotten the message.

I like backward compatibility and think it's relevant but its
relevance is much lower than current stuff and ever diminishing.  Plus
the current in-kernel emulation has annoying limitations which can be
alleviated by userland implementation.  So,

 * OSS compatibility as seen from userland apps : relevant

 * doing everything right by sound_core.c which basically is only used
   by in-kernel emulation : not so much

Thanks.

-- 
tejun

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Tejun Heo]

Hello,

Alan Cox wrote:
>> If we're gonna do that, we might as well just rip off the pointless
>> 'claim everything so we can use our own module alises' code from
>> sound_core.c and be done with it.  That really is the only problem.
> 
> The interface expected is the sound_register_* interface and the sharing
> is likewise expected so you could for example mix OSS. CUSE and ALSA oss
> emulation drivers on the same system at the same time.

If OSS behaves like a good chrdev and claims what it can really do,
all that can be done so much easier at the chrdev level.

> Rewriting sound_register* in terms of minor number allocations using the
> fact modern kernels have better allocators for devices would also be
> preferable to the weird module hacks initially proposed. I guess a
> starting point would be to tweak soundcore to generate both message sets
> itself for a year and then pull the old stuff out.

I don't disagree with you in that the above would be the perfect
technical solution but I just don't think the balance sheet is right.

Thanks.

-- 
tejun

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Alan Cox]

> > And it can then be a good citizen and share the resource. CUSE needs to
> > do the same for video/misc/framebuffer/tty/usb/etc so this is a general
> > pattern it needs to handle. In short cuse needs to grow this ability
> > anyway if it wants to be support these sorts of device.
> 
> The sharing and mixing and matching can happen very gracefully at
> chrdev level.  Does it make sense to drag it down to each subsystem
> level just to use custom module alises?  Not at all to me.  The same
> goes for video or whatever.  There is no reason to claim unused device
> numbers anymore and as long as that holds everyone is good citizen at
> the chrdev region.  That's _much_ cleaner.

I would suggest you read the code for the others. OSS is probably the
easy one to kill but the others create objects for platform wide sysfs
files and the like (as indeed OSS once did for a proc file) so in the
general case CUSE needs to work through them.

>  * OSS compatibility as seen from userland apps : relevant
> 
>  * doing everything right by sound_core.c which basically is only used
>    by in-kernel emulation : not so much

So instead of weird config magic why not send both messages. Initially
soundcore can generate both requests for sound-slot-foo and also fake up
its own char requests. It's easy to do, it doesn't put any code into the
kernel we'll be stuck with for years with weird config options. It
doesn't require the user choose a magic kernel option - it just works.

It's not much code either - just another request call in soundcore_open -
one line extra to write, a year to wait (4 releases ?) and then the old
stuff can go away, ALSA can then avoid soundcore and soundcore can vanish.

Much cleaner, and I believe one line of code ?

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Tejun Heo]

Hello, Alan.

Alan Cox wrote:
> I would suggest you read the code for the others. OSS is probably the
> easy one to kill but the others create objects for platform wide sysfs
> files and the like (as indeed OSS once did for a proc file) so in the
> general case CUSE needs to work through them.

I don't think it's wise for CUSE to try to emulate all those.  CUSE
provides emulation for a character device file and that's it.  It's a
building block not a complete solution.  In many cases information
provided via sysfs or proc doesn't matter to much for individual apps
and there's no reason to try to emulate them.  For other cases, those
nodes would need to be emulated using other building blocks - tmpfs or
fuse overlay mount maybe.

>>  * OSS compatibility as seen from userland apps : relevant
>>
>>  * doing everything right by sound_core.c which basically is only used
>>    by in-kernel emulation : not so much
> 
> So instead of weird config magic why not send both messages.

It's weird but simple and the alternative behavior of only acquiring
existent devices shouldn't hurt anyone who's using anything remotely
modern.  The only thing that changes is the module alias kernel
requests after all.  Oh well...

> Initially soundcore can generate both requests for sound-slot-foo
> and also fake up its own char requests. It's easy to do, it doesn't
> put any code into the kernel we'll be stuck with for years with
> weird config options. It doesn't require the user choose a magic
> kernel option - it just works.
> 
> It's not much code either - just another request call in soundcore_open -
> one line extra to write, a year to wait (4 releases ?) and then the old
> stuff can go away, ALSA can then avoid soundcore and soundcore can vanish.
> 
> Much cleaner, and I believe one line of code ?

Hmm... the problem is that the old stuff claims the whole chrdev
region.  I was thinking about adding support for alternative module
alises at the chrdev layer if this is that essential and changing
sound_core to claim only the devices which actually exist.  That way
we can continue to use the same module aliases and allo OSS emulation
and ossp to occupy the same kernel.

I don't see how I would be able to achieve the latter with one liner.
Can you please elaborate a little bit?

Thanks.

-- 
tejun

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Greg KH]

On Wed, Aug 05, 2009 at 03:35:42PM +0900, Tejun Heo wrote:
> [un]register_chrdev() assume minor range 0-255.  This patch adds __
> prefixed versions which take @minorbase and @count explicitly.

What's the difference between this and the existing
register_chrdev_region() function?  Why doesn't that work for you?

What am I missing here?

thanks,

greg k-h

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Tejun Heo]

Greg KH wrote:
> On Wed, Aug 05, 2009 at 03:35:42PM +0900, Tejun Heo wrote:
>> [un]register_chrdev() assume minor range 0-255.  This patch adds __
>> prefixed versions which take @minorbase and @count explicitly.
> 
> What's the difference between this and the existing
> register_chrdev_region() function?  Why doesn't that work for you?
> 
> What am I missing here?

The function names in char_dev.c are quite confusing.
register_chrdev_region() only registers chrdev region.
register_chrdev() registers chrdev region and creates a cdev for the
region.  :-)

-- 
tejun

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Alan Cox]

> I don't see how I would be able to achieve the latter with one liner.
> Can you please elaborate a little bit?

It depends how much of a hurry you are in, but for the mainstream we can
do this

1.	Make soundcore also issue the request_module() as if the char
minor was unclaimed. Mark the old one as obsolete

2.	Wait a year while people adjust their scripts to trigger on the
char event instead (and fix the ordering bits for ALSA if the soundcore
docs are still valid on that)

3.	Apply a patch which makes ALSA use char dev allocation directly
for its OSS devices and dumps out soundcore, remove soundcore and switch
any other code using it.

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Alan Cox]

On Wed, 5 Aug 2009 17:33:46 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > I don't see how I would be able to achieve the latter with one liner.
> > Can you please elaborate a little bit?
> 
> It depends how much of a hurry you are in, but for the mainstream we can
> do this


and a PS to this

If we have a clear plan on obsoleting sound_, a proper documented entry
for it going obsolete and a clear roadmap which ends up with soundcore
being recycled[1] then adding a temporary option to soundcore to make it
shut up is much less of a problem because the option also clearly goes
obsolete with soundcore so no mess is left when the process is over -
which would let you get your CUSE stuff up as a hack in the meantime.

Ala
[1] 0 bits in the green bag, 1 bits in the black bag please

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Greg KH]

On Thu, Aug 06, 2009 at 01:30:30AM +0900, Tejun Heo wrote:
> Greg KH wrote:
> > On Wed, Aug 05, 2009 at 03:35:42PM +0900, Tejun Heo wrote:
> >> [un]register_chrdev() assume minor range 0-255.  This patch adds __
> >> prefixed versions which take @minorbase and @count explicitly.
> > 
> > What's the difference between this and the existing
> > register_chrdev_region() function?  Why doesn't that work for you?
> > 
> > What am I missing here?
> 
> The function names in char_dev.c are quite confusing.

That's an understatement :)


> register_chrdev_region() only registers chrdev region.
> register_chrdev() registers chrdev region and creates a cdev for the
> region.  :-)

Yes, but you can use register_chrdev_region() and then call cdev_alloc()
for the specific cdevs you need, right?  It's not the prettiest, but it
should work.

thanks,

greg k-h

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Tejun Heo]

Alan Cox wrote:
>> I don't see how I would be able to achieve the latter with one liner.
>> Can you please elaborate a little bit?
> 
> It depends how much of a hurry you are in, but for the mainstream we can
> do this
> 
> 1.	Make soundcore also issue the request_module() as if the char
> minor was unclaimed. Mark the old one as obsolete
> 
> 2.	Wait a year while people adjust their scripts to trigger on the
> char event instead (and fix the ordering bits for ALSA if the soundcore
> docs are still valid on that)
> 
> 3.	Apply a patch which makes ALSA use char dev allocation directly
> for its OSS devices and dumps out soundcore, remove soundcore and switch
> any other code using it.

It's quite difficult to think that those now mostly unused module
alises are worth full year of waiting and careful coordination.  I
think we're chasing a non-issue here.  If it really matters, I'll try
to teach chrdev about alternative module alises.

If you think the above is a good solution, how about the following?

1. Merge the weird switch thing and the extra standard chrdev module
   alias patch

2. Add to feature-removal that snd-slot/service-* are going away in a
   year along with the weird switches.  This allows people who wish to
   try or switch in the meantime to do so.

3. After a year, drop module loading related code from sound_core
   along with the weird config option and kernel parameter.

In the end, the only choice we have to make is whether to keep
snd-slot/service-* aliases.  If we're gonna (I don't see why tho), the
cleanest way would be to teach chrdev about aliases.  If not, the best
way is to add a switch so that it can be phased out gradually.

Thanks.

-- 
tejun

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Alan Cox]

> 1. Merge the weird switch thing and the extra standard chrdev module
>    alias patch

Might as well fake the normal aliases in soundcore if loaded rather than
pollute chardev with it but otherwise I think we agree.


> 2. Add to feature-removal that snd-slot/service-* are going away in a
>    year along with the weird switches.  This allows people who wish to
>    try or switch in the meantime to do so.

Yep

> 3. After a year, drop module loading related code from sound_core
>    along with the weird config option and kernel parameter.

Do we need soundcore at all at that point ? It seems phase 3 is "move any
needed logic to ALSA oss emulation and kill it off"

> In the end, the only choice we have to make is whether to keep
> snd-slot/service-* aliases.  If we're gonna (I don't see why tho), the

I think we need to for a year or so - and its trivial to do so.

> cleanest way would be to teach chrdev about aliases.  If not, the best
> way is to add a switch so that it can be phased out gradually.

In the mean time if you are grabbing just some minors sound_core.c:chains
also needs keeping in sync with direct character range grabs because your
CUSE based device might grab some minors before sound_core, and then it
appears tears will result ?

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Tejun Heo]

Greg KH wrote:
> On Thu, Aug 06, 2009 at 01:30:30AM +0900, Tejun Heo wrote:
>> Greg KH wrote:
>>> On Wed, Aug 05, 2009 at 03:35:42PM +0900, Tejun Heo wrote:
>>>> [un]register_chrdev() assume minor range 0-255.  This patch adds __
>>>> prefixed versions which take @minorbase and @count explicitly.
>>> What's the difference between this and the existing
>>> register_chrdev_region() function?  Why doesn't that work for you?
>>>
>>> What am I missing here?
>> The function names in char_dev.c are quite confusing.
> 
> That's an understatement :)

Yeah, it sure is.  It should have used a different verb at the very
least.

>> register_chrdev_region() only registers chrdev region.
>> register_chrdev() registers chrdev region and creates a cdev for the
>> region.  :-)
> 
> Yes, but you can use register_chrdev_region() and then call cdev_alloc()
> for the specific cdevs you need, right?  It's not the prettiest, but it
> should work.

Yeap, I can.  That's basically what register_chrdev() is.  If it had a
better name, adding a new variant would have been much nicer.  Having
the helper makes sense tho.  Names too confusing?

Thanks.

-- 
tejun

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Greg KH]

On Thu, Aug 06, 2009 at 02:01:56AM +0900, Tejun Heo wrote:
> Greg KH wrote:
> > On Thu, Aug 06, 2009 at 01:30:30AM +0900, Tejun Heo wrote:
> >> Greg KH wrote:
> >>> On Wed, Aug 05, 2009 at 03:35:42PM +0900, Tejun Heo wrote:
> >>>> [un]register_chrdev() assume minor range 0-255.  This patch adds __
> >>>> prefixed versions which take @minorbase and @count explicitly.
> >>> What's the difference between this and the existing
> >>> register_chrdev_region() function?  Why doesn't that work for you?
> >>>
> >>> What am I missing here?
> >> The function names in char_dev.c are quite confusing.
> > 
> > That's an understatement :)
> 
> Yeah, it sure is.  It should have used a different verb at the very
> least.
> 
> >> register_chrdev_region() only registers chrdev region.
> >> register_chrdev() registers chrdev region and creates a cdev for the
> >> region.  :-)
> > 
> > Yes, but you can use register_chrdev_region() and then call cdev_alloc()
> > for the specific cdevs you need, right?  It's not the prettiest, but it
> > should work.
> 
> Yeap, I can.  That's basically what register_chrdev() is.  If it had a
> better name, adding a new variant would have been much nicer.  Having
> the helper makes sense tho.  Names too confusing?

Yes the names are confusing, the whole api needs a rework to make it
easier to understand :(

thanks,

greg k-h

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Tejun Heo]

Hello,

Greg KH wrote:
> Yes the names are confusing, the whole api needs a rework to make it
> easier to understand :(

For now, I'll open code it then but it would be really nice to clean
the names up.

Thanks.

-- 
tejun

Re: [PATCH 2/2] sound: make OSS device number claiming optional

[Tejun Heo]

Hello, Alan.

Alan Cox wrote:
>> 1. Merge the weird switch thing and the extra standard chrdev module
>>    alias patch
> 
> Might as well fake the normal aliases in soundcore if loaded rather than
> pollute chardev with it but otherwise I think we agree.

Yeap, that was what I meant by the second part of the sentence.

>> 2. Add to feature-removal that snd-slot/service-* are going away in a
>>    year along with the weird switches.  This allows people who wish to
>>    try or switch in the meantime to do so.
> 
> Yep
> 
>> 3. After a year, drop module loading related code from sound_core
>>    along with the weird config option and kernel parameter.
> 
> Do we need soundcore at all at that point ? It seems phase 3 is "move any
> needed logic to ALSA oss emulation and kill it off"

Yeah, sure.  There's nothing preventing ALSA from directly registering
chrdevs.

>> In the end, the only choice we have to make is whether to keep
>> snd-slot/service-* aliases.  If we're gonna (I don't see why tho), the
> 
> I think we need to for a year or so - and its trivial to do so.
> 
>> cleanest way would be to teach chrdev about aliases.  If not, the best
>> way is to add a switch so that it can be phased out gradually.
> 
> In the mean time if you are grabbing just some minors sound_core.c:chains
> also needs keeping in sync with direct character range grabs because your
> CUSE based device might grab some minors before sound_core, and then it
> appears tears will result ?

Hmmm... okay, will take a deeper look at that.

Thanks.

-- 
tejun

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Tejun Heo]

Tejun Heo wrote:
> Hello,
> 
> Greg KH wrote:
>> Yes the names are confusing, the whole api needs a rework to make it
>> easier to understand :(
> 
> For now, I'll open code it then but it would be really nice to clean
> the names up.

Crap, there's a difference.  For [un]register_chrdev() the chrdev
layer keeps track of cdev while using cdev_alloc/add/del() directly
requires the caller to keep track of the pointer.  Also, it's a bit of
pain to open code the whole thing.

For now, it looks like adding the __ prefixed versions seems to be the
proper solution.  It's strange to provide the helper interface only
for full minor acquisitions anyway.  Let's do naming cleanup later.
How does that sound?

Thanks.

-- 
tejun

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Greg KH]

On Thu, Aug 06, 2009 at 05:13:31PM +0900, Tejun Heo wrote:
> Tejun Heo wrote:
> > Hello,
> > 
> > Greg KH wrote:
> >> Yes the names are confusing, the whole api needs a rework to make it
> >> easier to understand :(
> > 
> > For now, I'll open code it then but it would be really nice to clean
> > the names up.
> 
> Crap, there's a difference.  For [un]register_chrdev() the chrdev
> layer keeps track of cdev while using cdev_alloc/add/del() directly
> requires the caller to keep track of the pointer.  Also, it's a bit of
> pain to open code the whole thing.
> 
> For now, it looks like adding the __ prefixed versions seems to be the
> proper solution.  It's strange to provide the helper interface only
> for full minor acquisitions anyway.  Let's do naming cleanup later.
> How does that sound?

Well, what's the odds that I can get you to do cleanup now?  :)

Sure, I'll take this, only if Takashi wants the other stuff that depends
on it.  Let me know.

thanks,

greg k-h

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Tejun Heo]

Hello, Greg.

Greg KH wrote:
>> For now, it looks like adding the __ prefixed versions seems to be the
>> proper solution.  It's strange to provide the helper interface only
>> for full minor acquisitions anyway.  Let's do naming cleanup later.
>> How does that sound?
> 
> Well, what's the odds that I can get you to do cleanup now?  :)

I'd be happy to but whether cleaning it up would be benefitial or not
is a bit unclear.  We have 214 users of [un]register_chrdev_region()
and 118 users of [un]register_chrdev().  So, renaming the latter to,
say, create/destroy_chrdev() and make it take @baseminor and
@nr_minors should do it but I'm not quite sure whether it would worth
touching all those files.  Maybe it's better left as a mess like the
page allocation functions?

> Sure, I'll take this, only if Takashi wants the other stuff that
> depends on it.  Let me know.

Thanks.

-- 
tejun

Re: [PATCH 1/2] chrdev: implement __[un]register_chrdev()

[Greg KH]

On Fri, Aug 07, 2009 at 11:34:52AM +0900, Tejun Heo wrote:
> Hello, Greg.
> 
> Greg KH wrote:
> >> For now, it looks like adding the __ prefixed versions seems to be the
> >> proper solution.  It's strange to provide the helper interface only
> >> for full minor acquisitions anyway.  Let's do naming cleanup later.
> >> How does that sound?
> > 
> > Well, what's the odds that I can get you to do cleanup now?  :)
> 
> I'd be happy to but whether cleaning it up would be benefitial or not
> is a bit unclear.  We have 214 users of [un]register_chrdev_region()
> and 118 users of [un]register_chrdev().  So, renaming the latter to,
> say, create/destroy_chrdev() and make it take @baseminor and
> @nr_minors should do it but I'm not quite sure whether it would worth
> touching all those files.  Maybe it's better left as a mess like the
> page allocation functions?

Yeah, I guess it's easier to leave it alone for now.  If I ever get
bored, I'll work on cleaning them up :)

thanks,

greg k-h