[patch-2.4.2-ac8] misc_register() fix (was Re: Linux 2.4.2ac8

[patch-2.4.2-ac8] misc_register() fix (was Re: Linux 2.4.2ac8

[Tigran Aivazian]

On Fri, 2 Mar 2001, Tigran Aivazian wrote:
> On Thu, 1 Mar 2001, Alan Cox wrote:
> > 2.4.2-ac8
> > o	Stop two people claiming the same misc dev id	(Philipp Rumpf)
> 
> is this what has broken misc devi registration on my machine? I have two
> misc devices -- microcode and psaux -- now (ac8) I get none, /proc/misc is
> empty. Also, on boot gpm generates an "oops" from gpm.c(968) saying
> "/dev/mouse: No such device"

Hi Alan,

here is the fix, tested, it works fine. The only unsatisfactory thing is
that we do an extra if() on each iteration making misc_register()
typically a few instructions slower. I will think a few minutes on how to
make the old version work (i.e. I suspect it was just an incorrect walking
of the misc_list in ac8).

Regards,
Tigran

--- linux/drivers/char/misc.c.0	Fri Mar  2 09:35:01 2001
+++ linux/drivers/char/misc.c	Fri Mar  2 10:01:17 2001
@@ -175,14 +175,16 @@
 	
 	if (misc->next || misc->prev)
 		return -EBUSY;
+
 	down(&misc_sem);
-	c = misc_list.next;
 
-	while ((c != &misc_list) && (c->minor != misc->minor))
+	c = misc_list.next;
+	while (c != &misc_list) {
+		if (c->minor == misc->minor) {
+			up(&misc_sem);
+			return -EBUSY;
+		}
 		c = c->next;
-	if (c == &misc_list) {
-		up(&misc_sem);
-		return -EBUSY;
 	}
 
 	if (misc->minor == MISC_DYNAMIC_MINOR) {

Re: [patch-2.4.2-ac8] misc_register() fix (was Re: Linux 2.4.2ac8

[Philipp Rumpf]

On Fri, Mar 02, 2001 at 10:55:39AM +0000, Tigran Aivazian wrote:
> On Fri, 2 Mar 2001, Tigran Aivazian wrote:
> > On Thu, 1 Mar 2001, Alan Cox wrote:
> > > 2.4.2-ac8
> > > o	Stop two people claiming the same misc dev id	(Philipp Rumpf)
> > 
> > is this what has broken misc devi registration on my machine? I have two
> > misc devices -- microcode and psaux -- now (ac8) I get none, /proc/misc is
> > empty. Also, on boot gpm generates an "oops" from gpm.c(968) saying
> > "/dev/mouse: No such device"
> 
> Hi Alan,
> 
> here is the fix, tested, it works fine. The only unsatisfactory thing is
> that we do an extra if() on each iteration making misc_register()
> typically a few instructions slower. I will think a few minutes on how to
> make the old version work (i.e. I suspect it was just an incorrect walking
> of the misc_list in ac8).

See earlier patch.

> --- linux/drivers/char/misc.c.0	Fri Mar  2 09:35:01 2001
> +++ linux/drivers/char/misc.c	Fri Mar  2 10:01:17 2001
> @@ -175,14 +175,16 @@
>  	
>  	if (misc->next || misc->prev)
>  		return -EBUSY;
> +
>  	down(&misc_sem);
> -	c = misc_list.next;
>  
> -	while ((c != &misc_list) && (c->minor != misc->minor))
> +	c = misc_list.next;
> +	while (c != &misc_list) {
> +		if (c->minor == misc->minor) {
> +			up(&misc_sem);
> +			return -EBUSY;
> +		}
>  		c = c->next;
> -	if (c == &misc_list) {
> -		up(&misc_sem);
> -		return -EBUSY;
>  	}
>  
>  	if (misc->minor == MISC_DYNAMIC_MINOR) {

This is fine as well, and possibly more readable.

[OT] style-curiosity

[J . A . Magallon]

On 03.02 Tigran Aivazian wrote:
> +	c = misc_list.next;
> +	while (c != &misc_list) {
> +		if (c->minor == misc->minor) {
> +			up(&misc_sem);
> +			return -EBUSY;
> +		}
>  		c = c->next;
> -	if (c == &misc_list) {
> -		up(&misc_sem);
> -		return -EBUSY;
>  	}

Just a matter of style and curiosity.

Would you kernel programmers consider this 'bad C' (I usually see this
much more clear...)

for (c = misc_list.next; c != &misc_list; c = c->next)
{
	if (c->minor == misc->minor) {
		up(&misc_sem);
		return -EBUSY;
	}	
}

Has any effect on output assembly ?

-- 
J.A. Magallon                                                      $> cd pub
mailto:jamagallon@able.es                                          $> more beer

Linux werewolf 2.4.2-ac7 #1 SMP Fri Mar 2 02:36:23 CET 2001 i686

Re: [OT] style-curiosity

[J . A . Magallon]

On 03.02 Tigran Aivazian wrote:
> On Fri, 2 Mar 2001, J . A . Magallon wrote:
> > for (c = misc_list.next; c != &misc_list; c = c->next)
> > {
> > 	if (c->minor == misc->minor) {
> > 		up(&misc_sem);
> > 		return -EBUSY;
> > 	}	
> > }
> 
> the above is good but the below is better:
> 
> for (c = misc_list.next; c != &misc_list; c = c->next)
>        if (c->minor == misc->minor) {
>                up(&misc_sem);
>                return -EBUSY;
>        }
>

I have suffered so many bugs coming from bad grouping that I always put
the braces even if they are not needed.

-- 
J.A. Magallon                                                      $> cd pub
mailto:jamagallon@able.es                                          $> more beer

Linux werewolf 2.4.2-ac8 #2 SMP Fri Mar 2 12:12:45 CET 2001 i686