[Kernel-janitors] please comment on this patch

[Kernel-janitors] please comment on this patch

[Kristen Carlson]

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

For removing check_region from drivers/char/esp.c

 
--- linux-2.6.6.kc/drivers/char/esp.orig	2004-06-15 11:40:24.643063272 -0700
+++ linux-2.6.6.kc/drivers/char/esp.c	2004-06-15 11:43:27.223306856 -0700
@@ -2369,9 +2369,21 @@ static _INLINE_ int autoconfig(struct es
 	/*
 	 * Check for ESP card
 	 */
+	if (ports && (ports->port == (info->port - 8))) {
+		release_region(*region_start,
+			       info->port - *region_start);
+	} else
+		*region_start = info->port;
+
+	if (!request_region(*region_start,
+		       info->port - *region_start + 8,
+		       "esp serial"))
+	{
+		restore_flags(flags);
+		return -EIO;
+	}
 
-	if (!check_region(info->port, 8) && 
-	    serial_in(info, UART_ESI_BASE) == 0xf3) {
+	if (serial_in(info, UART_ESI_BASE) == 0xf3) {
 		serial_out(info, UART_ESI_CMD1, 0x00);
 		serial_out(info, UART_ESI_CMD1, 0x01);
 
@@ -2387,19 +2399,6 @@ static _INLINE_ int autoconfig(struct es
 					info->irq = 4;
 			}
 
-			if (ports && (ports->port == (info->port - 8))) {
-				release_region(*region_start,
-					       info->port - *region_start);
-			} else
-				*region_start = info->port;
-
-			if (!request_region(*region_start,
-				       info->port - *region_start + 8,
-				       "esp serial"))
-			{
-				restore_flags(flags);
-				return -EIO;
-			}
 
 			/* put card in enhanced mode */
 			/* this prevents access through */
@@ -2410,8 +2409,11 @@ static _INLINE_ int autoconfig(struct es
 			serial_out(info, UART_ESI_CMD1, ESI_WRITE_UART);
 			serial_out(info, UART_ESI_CMD2, UART_MCR);
 			serial_out(info, UART_ESI_CMD2, 0x00);
-		}
-	}
+		} 
+	} 
+	if (!port_detected)
+		release_region(*region_start,
+				info->port - *region_start + 8);
 
 	restore_flags(flags);
 	return (port_detected);


Thanks!
Kristen


-- 
WWXD (What Would Xena Do?) 


[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] please comment on this patch

[Matthew Wilcox]

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

On Tue, Jun 15, 2004 at 07:57:32PM -0700, Kristen Carlson wrote:
> For removing check_region from drivers/char/esp.c

Better than most first attempts, but some problems.  Mostly inherent in the
original driver ;-)

It's quite a bizarre driver really.  It has a list of io port addresses:

        int esp[] = {0x100,0x140,0x180,0x200,0x240,0x280,0x300,0x380};

and then a do..while loop like this:

        i = 0;
        offset = 0;

        do {
                info->port = esp[i] + offset;
                if (!autoconfig(info, &region_start)) {
                        i++;
                        offset = 0;
                        continue;
                }
...
                if (offset == 56) {
                        i++;
                        offset = 0;
                } else {
                        offset += 8;
                }
        } while (i < NR_PRIMARY);

ie we have two loops that have been turned into one.  Blech.

Now we have autoconfig() itself.  We don't need interrupts disabled while
calling request_region() [indeed, this is actually a bug -- request_region()
calls kmalloc(GFP_KERNEL)].  So autoconfig() should look more like:

static _INLINE_ int autoconfig(struct esp_struct * info, int *region_start)
{
        int port_detected = 0;
        unsigned long flags;

	if (!request_region(*region_start, info->port - *region_start + 8,
			"esp serial"))
		return -EIO;

	save_flags(flags); cli();
...

Personally, I dislike the port_detected flag.  I would do that part as:

	if (serial_in(info, UART_ESI_BASE) == 0xf3) {
		serial_out(info, UART_ESI_CMD1, 0x00);
		serial_out(info, UART_ESI_CMD1, 0x01);
		if ((serial_in(info, UART_ESI_STAT2) & 0x70) != 0x20)
			goto not_present;
...
	}

	restore_flags(flags);
	return 1;

 not_present:
	release_region(*region_start, info->port - *region_start + 8);
	restore_flags(flags);
	return 0;
}

The big thing that concerns me about the original driver is:

                        if (ports && (ports->port == (info->port - 8))) {
                                release_region(*region_start,
                                               info->port - *region_start);
                        } else
                                *region_start = info->port;

This seems to be an attempt to combine all the contiguous port ranges
into one.  Your patch breaks this as we'll always end up with the region
being released when we fail to discover the last consecutive port.
But the original code was so obfuscated, I can't really blame you for
missing this one.

I think it's probably better to ignore this previous "feature" of the driver.
No other driver (that I know of) does this, and we can simplify the driver
as follows:

	if (!request_region(*region_start, info->port - *region_start + 8,
			 "esp serial"))
becomes
	if (!request_region(info->port, 8, "esp serial"))

We can lose the region_start variable entirely (every use of it becomes
obsolete across the entire driver).  espserial_exit() needs to be changed
to go along with this new scheme.

Somebody really needs to step up and maintain this driver (preferably
someone with hardware).  It won't work on SMP with 2.6 and it needs to
be converted to use the new serial core (drivers/serial/)

Hope that was useful.

>  
> --- linux-2.6.6.kc/drivers/char/esp.orig	2004-06-15 11:40:24.643063272 -0700
> +++ linux-2.6.6.kc/drivers/char/esp.c	2004-06-15 11:43:27.223306856 -0700
> @@ -2369,9 +2369,21 @@ static _INLINE_ int autoconfig(struct es
>  	/*
>  	 * Check for ESP card
>  	 */
> +	if (ports && (ports->port == (info->port - 8))) {
> +		release_region(*region_start,
> +			       info->port - *region_start);
> +	} else
> +		*region_start = info->port;
> +
> +	if (!request_region(*region_start,
> +		       info->port - *region_start + 8,
> +		       "esp serial"))
> +	{
> +		restore_flags(flags);
> +		return -EIO;
> +	}
>  
> -	if (!check_region(info->port, 8) && 
> -	    serial_in(info, UART_ESI_BASE) == 0xf3) {
> +	if (serial_in(info, UART_ESI_BASE) == 0xf3) {
>  		serial_out(info, UART_ESI_CMD1, 0x00);
>  		serial_out(info, UART_ESI_CMD1, 0x01);
>  
> @@ -2387,19 +2399,6 @@ static _INLINE_ int autoconfig(struct es
>  					info->irq = 4;
>  			}
>  
> -			if (ports && (ports->port == (info->port - 8))) {
> -				release_region(*region_start,
> -					       info->port - *region_start);
> -			} else
> -				*region_start = info->port;
> -
> -			if (!request_region(*region_start,
> -				       info->port - *region_start + 8,
> -				       "esp serial"))
> -			{
> -				restore_flags(flags);
> -				return -EIO;
> -			}
>  
>  			/* put card in enhanced mode */
>  			/* this prevents access through */
> @@ -2410,8 +2409,11 @@ static _INLINE_ int autoconfig(struct es
>  			serial_out(info, UART_ESI_CMD1, ESI_WRITE_UART);
>  			serial_out(info, UART_ESI_CMD2, UART_MCR);
>  			serial_out(info, UART_ESI_CMD2, 0x00);
> -		}
> -	}
> +		} 
> +	} 
> +	if (!port_detected)
> +		release_region(*region_start,
> +				info->port - *region_start + 8);
>  
>  	restore_flags(flags);
>  	return (port_detected);
> 
> 
> Thanks!
> Kristen
> 
> 
> -- 
> WWXD (What Would Xena Do?) 
> 

> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> http://lists.osdl.org/mailman/listinfo/kernel-janitors


-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] please comment on this patch

[Kristen Carlson]

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

> On Tue, Jun 15, 2004 at 07:57:32PM -0700, Kristen Carlson wrote:
> > For removing check_region from drivers/char/esp.c
> 
> Better than most first attempts, but some problems.  Mostly inherent in the
> original driver ;-)


Agreed, the driver is not written in the clearest way it could have been .  
This actually brings me to ask a philosophical clarification for janitor work.
In general, should we stick with our appointed task (i.e. do as little as 
possible to the driver except what the instructions in the TODO list specify), 
or is it acceptable to do more extensive changes?  AND, without hardware to 
test with, how much changes should you really do to a driver?  For example, 
I don't know if I'd feel comfortable re-writing too much of this driver 
without being able to test it.

> 
> Now we have autoconfig() itself.  We don't need interrupts disabled while
> calling request_region() [indeed, this is actually a bug -- request_region()
> calls kmalloc(GFP_KERNEL)].  So autoconfig() should look more like:
> 
> static _INLINE_ int autoconfig(struct esp_struct * info, int *region_start)
> {
>         int port_detected = 0;
>         unsigned long flags;
> 
> 	if (!request_region(*region_start, info->port - *region_start + 8,
> 			"esp serial"))
> 		return -EIO;
> 
> 	save_flags(flags); cli();


Ok, I will fix this also.

> ...
> 
> Personally, I dislike the port_detected flag.  I would do that part as:
> 
> 	if (serial_in(info, UART_ESI_BASE) == 0xf3) {
> 		serial_out(info, UART_ESI_CMD1, 0x00);
> 		serial_out(info, UART_ESI_CMD1, 0x01);
> 		if ((serial_in(info, UART_ESI_STAT2) & 0x70) != 0x20)
> 			goto not_present;
> ...
> 	}
> 
> 	restore_flags(flags);
> 	return 1;
> 
>  not_present:
> 	release_region(*region_start, info->port - *region_start + 8);
> 	restore_flags(flags);
> 	return 0;
> }
> 
> The big thing that concerns me about the original driver is:
> 
>                         if (ports && (ports->port == (info->port - 8))) {
>                                 release_region(*region_start,
>                                                info->port - *region_start);
>                         } else
>                                 *region_start = info->port;
> 
> This seems to be an attempt to combine all the contiguous port ranges
> into one.  Your patch breaks this as we'll always end up with the region
> being released when we fail to discover the last consecutive port.
> But the original code was so obfuscated, I can't really blame you for
> missing this one.


Whooops. :). 

> 
> I think it's probably better to ignore this previous "feature" of the driver.
> No other driver (that I know of) does this, and we can simplify the driver
> as follows:
> 
> 	if (!request_region(*region_start, info->port - *region_start + 8,
> 			 "esp serial"))
> becomes
> 	if (!request_region(info->port, 8, "esp serial"))
> 
> We can lose the region_start variable entirely (every use of it becomes
> obsolete across the entire driver).  espserial_exit() needs to be changed
> to go along with this new scheme.


Ok, I will take this approach and resubmit another patch to see if it
works better.


> 
> Somebody really needs to step up and maintain this driver (preferably
> someone with hardware).  It won't work on SMP with 2.6 and it needs to
> be converted to use the new serial core (drivers/serial/)
> 
> Hope that was useful.



Yes, thanks so much for spending time on this :).
Kristen



-- 
WWXD (What Would Xena Do?) 


[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] please comment on this patch

[Kristen Carlson]

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

Here's a new patch, please let me know what you think.


--- linux-2.6.6.kc/drivers/char/esp.orig	2004-06-15 11:40:24.000000000 -0700
+++ linux-2.6.6.kc/drivers/char/esp.c	2004-06-16 11:43:59.724569424 -0700
@@ -70,6 +70,7 @@
 
 #define NR_PORTS 64	/* maximum number of ports */
 #define NR_PRIMARY 8	/* maximum number of primary ports */
+#define REGION_SIZE 8   /* size of io region to request */
 
 /* The following variables can be set by giving module options */
 static int irq[NR_PRIMARY];	/* IRQ for each base port */
@@ -2359,19 +2360,21 @@ static _INLINE_ void show_serial_version
  * This routine is called by espserial_init() to initialize a specific serial
  * port.
  */
-static _INLINE_ int autoconfig(struct esp_struct * info, int *region_start)
+static _INLINE_ int autoconfig(struct esp_struct * info)
 {
 	int port_detected = 0;
 	unsigned long flags;
 
+	if (!request_region(info->port, REGION_SIZE, "esp serial"))
+		return -EIO;
+
 	save_flags(flags); cli();
 	
 	/*
 	 * Check for ESP card
 	 */
-
-	if (!check_region(info->port, 8) && 
-	    serial_in(info, UART_ESI_BASE) == 0xf3) {
+	
+	if (serial_in(info, UART_ESI_BASE) == 0xf3) {
 		serial_out(info, UART_ESI_CMD1, 0x00);
 		serial_out(info, UART_ESI_CMD1, 0x01);
 
@@ -2387,19 +2390,6 @@ static _INLINE_ int autoconfig(struct es
 					info->irq = 4;
 			}
 
-			if (ports && (ports->port == (info->port - 8))) {
-				release_region(*region_start,
-					       info->port - *region_start);
-			} else
-				*region_start = info->port;
-
-			if (!request_region(*region_start,
-				       info->port - *region_start + 8,
-				       "esp serial"))
-			{
-				restore_flags(flags);
-				return -EIO;
-			}
 
 			/* put card in enhanced mode */
 			/* this prevents access through */
@@ -2410,8 +2400,10 @@ static _INLINE_ int autoconfig(struct es
 			serial_out(info, UART_ESI_CMD1, ESI_WRITE_UART);
 			serial_out(info, UART_ESI_CMD2, UART_MCR);
 			serial_out(info, UART_ESI_CMD2, 0x00);
-		}
-	}
+		} 
+	} 
+	if (!port_detected)
+		release_region(info->port, REGION_SIZE);
 
 	restore_flags(flags);
 	return (port_detected);
@@ -2445,7 +2437,6 @@ static struct tty_operations esp_ops = {
 int __init espserial_init(void)
 {
 	int i, offset;
-	int region_start;
 	struct esp_struct * info;
 	struct esp_struct *last_primary = 0;
 	int esp[] = {0x100,0x140,0x180,0x200,0x240,0x280,0x300,0x380};
@@ -2531,7 +2522,7 @@ int __init espserial_init(void)
 		info->irq = irq[i];
 		info->line = (i * 8) + (offset / 8);
 
-		if (!autoconfig(info, &region_start)) {
+		if (!autoconfig(info)) {
 			i++;
 			offset = 0;
 			continue;
@@ -2607,7 +2598,6 @@ static void __exit espserial_exit(void) 
 {
 	unsigned long flags;
 	int e1;
-	unsigned int region_start, region_end;
 	struct esp_struct *temp_async;
 	struct esp_pio_buffer *pio_buf;
 
@@ -2622,27 +2612,8 @@ static void __exit espserial_exit(void) 
 
 	while (ports) {
 		if (ports->port) {
-			region_start = region_end = ports->port;
-			temp_async = ports;
-
-			while (temp_async) {
-				if ((region_start - temp_async->port) == 8) {
-					region_start = temp_async->port;
-					temp_async->port = 0;
-					temp_async = ports;
-				} else if ((temp_async->port - region_end)
-					   == 8) {
-					region_end = temp_async->port;
-					temp_async->port = 0;
-					temp_async = ports;
-				} else
-					temp_async = temp_async->next_port;
-			}
-			
-			release_region(region_start,
-				       region_end - region_start + 8);
+			release_region(ports->port, REGION_SIZE);
 		}
-
 		temp_async = ports->next_port;
 		kfree(ports);
 		ports = temp_async;



Thanks!
Kristen


-- 
WWXD (What Would Xena Do?) 


[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] please comment on this patch

[Arnaldo Carvalho de Melo]

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

Kristen Carlson wrote:
>>On Tue, Jun 15, 2004 at 07:57:32PM -0700, Kristen Carlson wrote:
>>
>>>For removing check_region from drivers/char/esp.c
>>
>>Better than most first attempts, but some problems.  Mostly inherent in the
>>original driver ;-)
> 
> 
> 
> Agreed, the driver is not written in the clearest way it could have been .  
> This actually brings me to ask a philosophical clarification for janitor work.

:-) IMHO there are lots of ways people see what being a "kernel janitor" 
  really means... I think the best definition is: to have taste.

I.e. stick to the APIs, follow the "de facto" mostly undocumented "style 
of the ``good chunks'' of the kernel sources", i.e. look at how Al Viro 
writes code, read his reasonings on the linux-kernel mailing list, 
follow it, that is to be a janitor, in my humble opinion.

> In general, should we stick with our appointed task (i.e. do as little as 
> possible to the driver except what the instructions in the TODO list specify), 
> or is it acceptable to do more extensive changes?  AND, without hardware to 
> test with, how much changes should you really do to a driver?  For example, 
> I don't know if I'd feel comfortable re-writing too much of this driver 
> without being able to test it.

1. if the code is good, there is no need to touch it

2. if the code works, but is ugly, do changes that make it more 
understandable and closer to what is considered good taste

3. if the code doesn't works, fix it, taking advantage that you're 
_studying_ it to understand what is to be fixed and see if it follow the 
magical definition of "good taste" and send some preparatory patches
cleaning it. Then fix it.

Sometimes, just suggest ditching it from the kernel sources, I'm sure 
many here remember some cases where there was no hope for salvation :-)

- Arnaldo

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] please comment on this patch

[Matthew Wilcox]

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

On Wed, Jun 16, 2004 at 08:38:25PM -0700, Kristen Carlson wrote:
> Here's a new patch, please let me know what you think.

I don't see any bugs.  More eyeballs?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] please comment on this patch

[Nicolas Kaiser]

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

* Kristen Carlson <kristenc@cs.pdx.edu>:

> Here's a new patch, please let me know what you think.

>  	/*
>  	 * Check for ESP card
>  	 */
> -
> -	if (!check_region(info->port, 8) && 
> -	    serial_in(info, UART_ESI_BASE) == 0xf3) {
> +	
    ^^^^
> +	if (serial_in(info, UART_ESI_BASE) == 0xf3) {


Just nitpicking: Looks like trailing whitespace.

Cheers,
n.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] please comment on this patch

[maximilian attems]

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

On Fri, 18 Jun 2004, Nicolas Kaiser wrote:

> * Kristen Carlson <kristenc@cs.pdx.edu>:
> 
> > Here's a new patch, please let me know what you think.
> 
> >  	/*
> >  	 * Check for ESP card
> >  	 */
> > -
> > -	if (!check_region(info->port, 8) && 
> > -	    serial_in(info, UART_ESI_BASE) == 0xf3) {
> > +	
>     ^^^^
> > +	if (serial_in(info, UART_ESI_BASE) == 0xf3) {
> 
> 
> Just nitpicking: Looks like trailing whitespace.

corrected 2 other occurrences for the kjt patch set.
a++ maks


[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors