advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Randy.Dunlap]

On Tue, 5 Aug 2003 20:50:07 -0400 Gene Heskett <gene.heskett@verizon.net> wrote:

| On Tuesday 05 August 2003 10:57, Randy.Dunlap wrote:
| >On Tue, 5 Aug 2003 10:41:08 -0400 Gene Heskett 
| <gene.heskett@verizon.net> wrote:
| >| Greetings;
| >|
| >| In the 2.4 includes, find this in ioport.h
| >| ----
| >| /* Compatibility cruft */
| >| #define check_region(start,n)   __check_region(&ioport_resource,
| >| (start), (n))
| >| [snip]
| >| extern int __check_region(struct resource *, unsigned long,
| >| unsigned long);
| >| ----
| >| But in the 2.6 version, find this:
| >| ----
| >| /* Compatibility cruft */
| >| [snip]
| >| extern int __check_region(struct resource *, unsigned long,
| >| unsigned long);
| >| [snip]
| >| static inline int __deprecated check_region(unsigned long s,
| >| unsigned long n)
| >| {
| >|         return __check_region(&ioport_resource, s, n);
| >| }
| >| ----
| >| First, the define itself is missing in the 2.6 version.
| >|
| >| Many drivers seem to use this call, and in that which I'm trying
| >| to build, the nforce and advansys modules use it.  And while the
| >| modules seem to build, they do not run properly.
| >|
| >| I cannot run 2.6.x for extended tests because of the advansys
| >| breakage this causes.  I also haven't even tried to run X because
| >| of the nforce error reported when its built, the same error as
| >| attacks the advansys code.
| >|
| >| Can I ask why this change was made, and is there a suitable
| >| replacement call available that these drivers could use instead of
| >| check_region(), as shown here in a snip from advansys.c?
| >| ----
| >| if (check_region(iop, ASC_IOADR_GAP) != 0) {
| >| ...
| >| if (check_region(iop_base, ASC_IOADR_GAP) != 0) {
| >| ...
| >|
| >| Hopeing for some hints here.
| >
| >check_region() was racy.  Use request_region() instead.
| >
| >   if (!request_region(iop, ASC_IOADR_GAP, "advansys")) {
| >   ...
| >
| >   if (!request_region(iop_base, ASC_IOADR, "advansys")) {
| >   ...
| >
| >Of course, if successful, this assigns the region to the driver,
| >while check_region() didn't do this, so release_region() should be
| >used as needed to return the resources.
| 
| Randy, look the attached patch over & see if its suitable.  Its 
| against the driver in the 2.6.0-test2 archive, and was done from 
| within the drivers/scsi directory.  Its working fine here, booting 
| and accessing my tape drive just fine with the advansys driver 
| incorporated into the kernel, or as a module.  I didn't see any 
| memory leakage, but my test method isn't definitive.
| 
| If its suitable, pass it on to Linus.  It will add one more card to 
| the NOT broken by 2.6 list.

First, I've moved this to the linux-scsi mailing list for more/better
comments.

Second, (minor nit) part of your patch[1] contained a line that only
added a trailing space.

But most importantly, if I'm reading it correctly, the driver now does
request_region() 2 times for an IO region (note: for CONFIG_ISA only;
is your Advansys adapter PCI or ISA?).

I'm not terribly interested in spending much time on this driver,
so the simplest thing to do IMO is to emulate check_region() with
pairs of request_region() and release_region() calls.  Yes, this
is still racy, just like check_region() was.  Is that a problem
in your machine environment?

Below is a patch that uses request_region/release_region pairs
instead of check_region().  Does it work for you?

[1] http://lkml.org/lkml/2003/8/5/304

--
~Randy				For Linux-2.6, see:
http://www.kernel.org/pub/linux/kernel/people/davej/misc/post-halloween-2.5.txt


patch_name:	advansys_regions_260.patch
patch_version:	2003-08-06.10:27:17
author:		Randy.Dunlap <rddunlap@osdl.org>
description:	convert check_region() to request/release
product:	Linux
product_versions: linux-260-test2
diffstat:	=
 drivers/scsi/advansys.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)


diff -Naurp ./drivers/scsi/advansys.c~advans ./drivers/scsi/advansys.c
--- ./drivers/scsi/advansys.c~advans	2003-07-27 09:59:39.000000000 -0700
+++ ./drivers/scsi/advansys.c	2003-08-06 10:24:45.000000000 -0700
@@ -4619,17 +4619,19 @@ advansys_detect(Scsi_Host_Template *tpnt
                         ASC_DBG1(1,
                                 "advansys_detect: probing I/O port 0x%x...\n",
                             iop);
-                        if (check_region(iop, ASC_IOADR_GAP) != 0) {
+                        if (!request_region(iop, ASC_IOADR_GAP, "advansys")) {
                             printk(
 "AdvanSys SCSI: specified I/O Port 0x%X is busy\n", iop);
                             /* Don't try this I/O port twice. */
                             asc_ioport[ioport] = 0;
+			    release_region(iop, ASC_IOADR_GAP);
                             goto ioport_try_again;
                         } else if (AscFindSignature(iop) == ASC_FALSE) {
                             printk(
 "AdvanSys SCSI: specified I/O Port 0x%X has no adapter\n", iop);
                             /* Don't try this I/O port twice. */
                             asc_ioport[ioport] = 0;
+			    release_region(iop, ASC_IOADR_GAP);
                             goto ioport_try_again;
                         } else {
                             /*
@@ -4647,6 +4649,7 @@ advansys_detect(Scsi_Host_Template *tpnt
                                   * 'ioport' past this board.
                                   */
                                  ioport++;
+				 release_region(iop, ASC_IOADR_GAP);
                                  goto ioport_try_again;
                             }
                         }
@@ -10001,16 +10004,18 @@ AscSearchIOPortAddr11(
     }
     for (; i < ASC_IOADR_TABLE_MAX_IX; i++) {
         iop_base = _asc_def_iop_base[i];
-        if (check_region(iop_base, ASC_IOADR_GAP) != 0) {
+        if (!request_region(iop_base, ASC_IOADR_GAP, "advansys")) {
             ASC_DBG1(1,
-               "AscSearchIOPortAddr11: check_region() failed I/O port 0x%x\n",
+               "AscSearchIOPortAddr11: request_region() failed I/O port 0x%x\n",
                      iop_base);
             continue;
         }
         ASC_DBG1(1, "AscSearchIOPortAddr11: probing I/O port 0x%x\n", iop_base);
         if (AscFindSignature(iop_base)) {
+	    release_region(iop_base, ASC_IOADR_GAP);
             return (iop_base);
         }
+	release_region(iop_base, ASC_IOADR_GAP);
     }
     return (0);
 }

Re: advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Matthew Wilcox]

On Wed, Aug 06, 2003 at 10:40:41AM -0700, Randy.Dunlap wrote:
> | On Tuesday 05 August 2003 10:57, Randy.Dunlap wrote:
> | >On Tue, 5 Aug 2003 10:41:08 -0400 Gene Heskett 
> | <gene.heskett@verizon.net> wrote:
> | >| Greetings;
> | >|
> | >| In the 2.4 includes, find this in ioport.h
> | >| ----
> | >| /* Compatibility cruft */
> | >| #define check_region(start,n)   __check_region(&ioport_resource,
> | >| (start), (n))
> | >| [snip]
> | >| extern int __check_region(struct resource *, unsigned long,
> | >| unsigned long);
> | >| ----
> | >| But in the 2.6 version, find this:
> | >| ----
> | >| /* Compatibility cruft */
> | >| [snip]
> | >| extern int __check_region(struct resource *, unsigned long,
> | >| unsigned long);
> | >| [snip]
> | >| static inline int __deprecated check_region(unsigned long s,
> | >| unsigned long n)
> | >| {
> | >|         return __check_region(&ioport_resource, s, n);
> | >| }
> | >| ----
> | >| First, the define itself is missing in the 2.6 version.

What define?  check_region() is still there, just as an inline function,
not a macro.

> | >| Many drivers seem to use this call, and in that which I'm trying
> | >| to build, the nforce and advansys modules use it.  And while the
> | >| modules seem to build, they do not run properly.

Explain?  Has someone broken check_region?

> I'm not terribly interested in spending much time on this driver,
> so the simplest thing to do IMO is to emulate check_region() with
> pairs of request_region() and release_region() calls.  Yes, this
> is still racy, just like check_region() was.  Is that a problem
> in your machine environment?

I think this is the wrong thig to do.  Leaving the check_region()
calls there indicate this driver still needs to be fixed properly.
Replacing them with unsafe uses of request & release_region leaves the
driver racy and removes the indication.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk


-------------------------------------------------------
This SF.Net email sponsored by: Free pre-built ASP.NET sites including
Data Reports, E-commerce, Portals, and Forums are available now.
Download today and enter to win an XBOX or Visual Studio .NET.
http://aspnet.click-url.com/go/psa00100003ave/direct;at.aspnet_072303_01/01

Re: advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Randy.Dunlap]

On Wed, 6 Aug 2003 18:59:54 +0100 Matthew Wilcox <willy@debian.org> wrote:

| On Wed, Aug 06, 2003 at 10:40:41AM -0700, Randy.Dunlap wrote:
| > | On Tuesday 05 August 2003 10:57, Randy.Dunlap wrote:
| > | >On Tue, 5 Aug 2003 10:41:08 -0400 Gene Heskett 
| > | <gene.heskett@verizon.net> wrote:
| > | >| Greetings;
| > | >|
| > | >| In the 2.4 includes, find this in ioport.h
| > | >| ----
| > | >| /* Compatibility cruft */
| > | >| #define check_region(start,n)   __check_region(&ioport_resource,
| > | >| (start), (n))
| > | >| [snip]
| > | >| extern int __check_region(struct resource *, unsigned long,
| > | >| unsigned long);
| > | >| ----
| > | >| But in the 2.6 version, find this:
| > | >| ----
| > | >| /* Compatibility cruft */
| > | >| [snip]
| > | >| extern int __check_region(struct resource *, unsigned long,
| > | >| unsigned long);
| > | >| [snip]
| > | >| static inline int __deprecated check_region(unsigned long s,
| > | >| unsigned long n)
| > | >| {
| > | >|         return __check_region(&ioport_resource, s, n);
| > | >| }
| > | >| ----
| > | >| First, the define itself is missing in the 2.6 version.
| 
| What define?  check_region() is still there, just as an inline function,
| not a macro.

and it's "__deprecated", so there is some desire not to use it.

| > | >| Many drivers seem to use this call, and in that which I'm trying
| > | >| to build, the nforce and advansys modules use it.  And while the
| > | >| modules seem to build, they do not run properly.
| 
| Explain?  Has someone broken check_region?

Just deprecated it, so it gives compile warnings when it's used.

| > I'm not terribly interested in spending much time on this driver,
| > so the simplest thing to do IMO is to emulate check_region() with
| > pairs of request_region() and release_region() calls.  Yes, this
| > is still racy, just like check_region() was.  Is that a problem
| > in your machine environment?
| 
| I think this is the wrong thig to do.  Leaving the check_region()
| calls there indicate this driver still needs to be fixed properly.
| Replacing them with unsafe uses of request & release_region leaves the
| driver racy and removes the indication.

Yes, I see your point, and it will work that way as well as it ever did.

Think it will ever be fixed?  Maybe after more 2.6.x users show up...?

--
~Randy				For Linux-2.6, see:
http://www.kernel.org/pub/linux/kernel/people/davej/misc/post-halloween-2.5.txt


-------------------------------------------------------
This SF.Net email sponsored by: Free pre-built ASP.NET sites including
Data Reports, E-commerce, Portals, and Forums are available now.
Download today and enter to win an XBOX or Visual Studio .NET.
http://aspnet.click-url.com/go/psa00100003ave/direct;at.aspnet_072303_01/01

Re: advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Gene Heskett]

On Wednesday 06 August 2003 13:59, Matthew Wilcox wrote:
>On Wed, Aug 06, 2003 at 10:40:41AM -0700, Randy.Dunlap wrote:
>> | On Tuesday 05 August 2003 10:57, Randy.Dunlap wrote:
>> | >On Tue, 5 Aug 2003 10:41:08 -0400 Gene Heskett
>> |
>> | <gene.heskett@verizon.net> wrote:
>> | >| Greetings;
>> | >|
>> | >| In the 2.4 includes, find this in ioport.h
>> | >| ----
>> | >| /* Compatibility cruft */
>> | >| #define check_region(start,n)  
>> | >| __check_region(&ioport_resource, (start), (n))
>> | >| [snip]
>> | >| extern int __check_region(struct resource *, unsigned long,
>> | >| unsigned long);
>> | >| ----
>> | >| But in the 2.6 version, find this:
>> | >| ----
>> | >| /* Compatibility cruft */
>> | >| [snip]
>> | >| extern int __check_region(struct resource *, unsigned long,
>> | >| unsigned long);
>> | >| [snip]
>> | >| static inline int __deprecated check_region(unsigned long s,
>> | >| unsigned long n)
>> | >| {
>> | >|         return __check_region(&ioport_resource, s, n);
>> | >| }
>> | >| ----
>> | >| First, the define itself is missing in the 2.6 version.
>
>What define?  check_region() is still there, just as an inline
> function, not a macro.
>
I found that later.

>> | >| Many drivers seem to use this call, and in that which I'm
>> | >| trying to build, the nforce and advansys modules use it.  And
>> | >| while the modules seem to build, they do not run properly.
>
>Explain?  Has someone broken check_region?

Its now marked __deprecated.  The module builds while indicating the 
errors, but gets into a bus reset loop and hangs in the second 
iteration of it.

>
>> I'm not terribly interested in spending much time on this driver,
>> so the simplest thing to do IMO is to emulate check_region() with
>> pairs of request_region() and release_region() calls.  Yes, this
>> is still racy, just like check_region() was.  Is that a problem
>> in your machine environment?
>
>I think this is the wrong thig to do.  Leaving the check_region()
>calls there indicate this driver still needs to be fixed properly.
>Replacing them with unsafe uses of request & release_region leaves
> the driver racy and removes the indication.

What do you suggest doing?

-- 
Cheers, Gene
AMD K6-III@500mhz 320M
Athlon1600XP@1400mhz  512M
99.27% setiathome rank, not too shabby for a WV hillbilly
Yahoo.com attornies please note, additions to this message
by Gene Heskett are:
Copyright 2003 by Maurice Eugene Heskett, all rights reserved.



-------------------------------------------------------
This SF.Net email sponsored by: Free pre-built ASP.NET sites including
Data Reports, E-commerce, Portals, and Forums are available now.
Download today and enter to win an XBOX or Visual Studio .NET.
http://aspnet.click-url.com/go/psa00100003ave/direct;at.aspnet_072303_01/01

Re: advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Randy.Dunlap]

On Wed, 6 Aug 2003 14:35:43 -0400 Gene Heskett <gene.heskett@verizon.net> wrote:

...
| >> | >| ----
| >> | >| First, the define itself is missing in the 2.6 version.
| >
| >What define?  check_region() is still there, just as an inline
| > function, not a macro.
| >
| I found that later.
| 
| >> | >| Many drivers seem to use this call, and in that which I'm
| >> | >| trying to build, the nforce and advansys modules use it.  And
| >> | >| while the modules seem to build, they do not run properly.
| >
| >Explain?  Has someone broken check_region?
| 
| Its now marked __deprecated.  The module builds while indicating the 
| errors, but gets into a bus reset loop and hangs in the second 
| iteration of it.

So the kernel driver, without mods, is broken?
I didn't realize this earlier.

| >> I'm not terribly interested in spending much time on this driver,
| >> so the simplest thing to do IMO is to emulate check_region() with
| >> pairs of request_region() and release_region() calls.  Yes, this
| >> is still racy, just like check_region() was.  Is that a problem
| >> in your machine environment?
| >
| >I think this is the wrong thig to do.  Leaving the check_region()
| >calls there indicate this driver still needs to be fixed properly.
| >Replacing them with unsafe uses of request & release_region leaves
| > the driver racy and removes the indication.

--
~Randy				For Linux-2.6, see:
http://www.kernel.org/pub/linux/kernel/people/davej/misc/post-halloween-2.5.txt

Re: advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Gene Heskett]

On Wednesday 06 August 2003 14:50, Randy.Dunlap wrote:
>On Wed, 6 Aug 2003 14:35:43 -0400 Gene Heskett
> <gene.heskett@verizon.net> wrote:
>
>...
>
>| >> | >| ----
>| >> | >| First, the define itself is missing in the 2.6 version.
>| >
>| >What define?  check_region() is still there, just as an inline
>| > function, not a macro.
>|
>| I found that later.
>|
>| >> | >| Many drivers seem to use this call, and in that which I'm
>| >> | >| trying to build, the nforce and advansys modules use it. 
>| >> | >| And while the modules seem to build, they do not run
>| >> | >| properly.
>| >
>| >Explain?  Has someone broken check_region?
>|
>| Its now marked __deprecated.  The module builds while indicating
>| the errors, but gets into a bus reset loop and hangs in the second
>| iteration of it.
>
>So the kernel driver, without mods, is broken?
>I didn't realize this earlier.

Yes.  I've said as much 2-3 times although it could have been 
ambiguous once the translation from WV to english was made. :-)

My patch I sent worked 'nominally' in that ANAICT, all functions were 
restored,  and the memory useage wandered around mainly because 
setiathome is also running, started in rc.local here.  I did not seem 
to successively lose 50k for everytime I accessed something thru the 
patched driver, but I've now added the release_region calls as you 
suggested.  I'll test it later today.

>| >> I'm not terribly interested in spending much time on this
>| >> driver, so the simplest thing to do IMO is to emulate
>| >> check_region() with pairs of request_region() and
>| >> release_region() calls.  Yes, this is still racy, just like
>| >> check_region() was.  Is that a problem in your machine
>| >> environment?
>| >
>| >I think this is the wrong thig to do.  Leaving the check_region()
>| >calls there indicate this driver still needs to be fixed
>| > properly. Replacing them with unsafe uses of request &
>| > release_region leaves the driver racy and removes the
>| > indication.
>
>--
>~Randy				For Linux-2.6, see:
>http://www.kernel.org/pub/linux/kernel/people/davej/misc/post-hallow
>een-2.5.txt

-- 
Cheers, Gene
AMD K6-III@500mhz 320M
Athlon1600XP@1400mhz  512M
99.27% setiathome rank, not too shabby for a WV hillbilly
Yahoo.com attornies please note, additions to this message
by Gene Heskett are:
Copyright 2003 by Maurice Eugene Heskett, all rights reserved.



-------------------------------------------------------
This SF.Net email sponsored by: Free pre-built ASP.NET sites including
Data Reports, E-commerce, Portals, and Forums are available now.
Download today and enter to win an XBOX or Visual Studio .NET.
http://aspnet.click-url.com/go/psa00100003ave/direct;at.aspnet_072303_01/01

Re: advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Gene Heskett]

On Wednesday 06 August 2003 13:59, Matthew Wilcox wrote:

This is a resend.

>On Wed, Aug 06, 2003 at 10:40:41AM -0700, Randy.Dunlap wrote:
>> | On Tuesday 05 August 2003 10:57, Randy.Dunlap wrote:
>> | >On Tue, 5 Aug 2003 10:41:08 -0400 Gene Heskett
>> |
>> | <gene.heskett@verizon.net> wrote:
>> | >| Greetings;
>> | >|
>> | >| In the 2.4 includes, find this in ioport.h
>> | >| ----
>> | >| /* Compatibility cruft */
>> | >| #define check_region(start,n)  
>> | >| __check_region(&ioport_resource, (start), (n))
>> | >| [snip]
>> | >| extern int __check_region(struct resource *, unsigned long,
>> | >| unsigned long);
>> | >| ----
>> | >| But in the 2.6 version, find this:
>> | >| ----
>> | >| /* Compatibility cruft */
>> | >| [snip]
>> | >| extern int __check_region(struct resource *, unsigned long,
>> | >| unsigned long);
>> | >| [snip]
>> | >| static inline int __deprecated check_region(unsigned long s,
>> | >| unsigned long n)
>> | >| {
>> | >|         return __check_region(&ioport_resource, s, n);
>> | >| }
>> | >| ----
>> | >| First, the define itself is missing in the 2.6 version.
>
>What define?  check_region() is still there, just as an inline
> function, not a macro.
>
I found that later.

>> | >| Many drivers seem to use this call, and in that which I'm
>> | >| trying to build, the nforce and advansys modules use it.  And
>> | >| while the modules seem to build, they do not run properly.
>
>Explain?  Has someone broken check_region?

Its now marked __deprecated.  The module builds while indicating the 
errors, but gets into a bus reset loop and hangs in the second 
iteration of it.

>
>> I'm not terribly interested in spending much time on this driver,
>> so the simplest thing to do IMO is to emulate check_region() with
>> pairs of request_region() and release_region() calls.  Yes, this
>> is still racy, just like check_region() was.  Is that a problem
>> in your machine environment?
>
>I think this is the wrong thig to do.  Leaving the check_region()
>calls there indicate this driver still needs to be fixed properly.
>Replacing them with unsafe uses of request & release_region leaves
> the driver racy and removes the indication.

What do you suggest doing?

-- 
Cheers, Gene
AMD K6-III@500mhz 320M
Athlon1600XP@1400mhz  512M
99.27% setiathome rank, not too shabby for a WV hillbilly
Yahoo.com attornies please note, additions to this message
by Gene Heskett are:
Copyright 2003 by Maurice Eugene Heskett, all rights reserved.




-------------------------------------------------------
This SF.Net email sponsored by: Free pre-built ASP.NET sites including
Data Reports, E-commerce, Portals, and Forums are available now.
Download today and enter to win an XBOX or Visual Studio .NET.
http://aspnet.click-url.com/go/psa00100003ave/direct;at.aspnet_072303_01/01

Re: advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Gene Heskett]

On Wednesday 06 August 2003 13:40, Randy.Dunlap wrote:
>On Tue, 5 Aug 2003 20:50:07 -0400 Gene Heskett 
<gene.heskett@verizon.net> wrote:
>| On Tuesday 05 August 2003 10:57, Randy.Dunlap wrote:
>| >On Tue, 5 Aug 2003 10:41:08 -0400 Gene Heskett
>|
>| <gene.heskett@verizon.net> wrote:
>| >| Greetings;
>| >|
>| >| In the 2.4 includes, find this in ioport.h
>| >| ----
>| >| /* Compatibility cruft */
>| >| #define check_region(start,n)  
>| >| __check_region(&ioport_resource, (start), (n))
>| >| [snip]
>| >| extern int __check_region(struct resource *, unsigned long,
>| >| unsigned long);
>| >| ----
>| >| But in the 2.6 version, find this:
>| >| ----
>| >| /* Compatibility cruft */
>| >| [snip]
>| >| extern int __check_region(struct resource *, unsigned long,
>| >| unsigned long);
>| >| [snip]
>| >| static inline int __deprecated check_region(unsigned long s,
>| >| unsigned long n)
>| >| {
>| >|         return __check_region(&ioport_resource, s, n);
>| >| }
>| >| ----
>| >| First, the define itself is missing in the 2.6 version.
>| >|
>| >| Many drivers seem to use this call, and in that which I'm
>| >| trying to build, the nforce and advansys modules use it.  And
>| >| while the modules seem to build, they do not run properly.
>| >|
>| >| I cannot run 2.6.x for extended tests because of the advansys
>| >| breakage this causes.  I also haven't even tried to run X
>| >| because of the nforce error reported when its built, the same
>| >| error as attacks the advansys code.
>| >|
>| >| Can I ask why this change was made, and is there a suitable
>| >| replacement call available that these drivers could use instead
>| >| of check_region(), as shown here in a snip from advansys.c?
>| >| ----
>| >| if (check_region(iop, ASC_IOADR_GAP) != 0) {
>| >| ...
>| >| if (check_region(iop_base, ASC_IOADR_GAP) != 0) {
>| >| ...
>| >|
>| >| Hopeing for some hints here.
>| >
>| >check_region() was racy.  Use request_region() instead.
>| >
>| >   if (!request_region(iop, ASC_IOADR_GAP, "advansys")) {
>| >   ...
>| >
>| >   if (!request_region(iop_base, ASC_IOADR, "advansys")) {
>| >   ...
>| >
>| >Of course, if successful, this assigns the region to the driver,
>| >while check_region() didn't do this, so release_region() should
>| > be used as needed to return the resources.
>|
>| Randy, look the attached patch over & see if its suitable.  Its
>| against the driver in the 2.6.0-test2 archive, and was done from
>| within the drivers/scsi directory.  Its working fine here, booting
>| and accessing my tape drive just fine with the advansys driver
>| incorporated into the kernel, or as a module.  I didn't see any
>| memory leakage, but my test method isn't definitive.
>|
>| If its suitable, pass it on to Linus.  It will add one more card
>| to the NOT broken by 2.6 list.
>
>First, I've moved this to the linux-scsi mailing list for
> more/better comments.
>
>Second, (minor nit) part of your patch[1] contained a line that only
>added a trailing space.

Probably my editors word wrap at work. :(

>But most importantly, if I'm reading it correctly, the driver now
> does request_region() 2 times for an IO region (note: for
> CONFIG_ISA only; is your Advansys adapter PCI or ISA?).

Its pci.  And isa adaptor would be bus speed limited.

>I'm not terribly interested in spending much time on this driver,
>so the simplest thing to do IMO is to emulate check_region() with
>pairs of request_region() and release_region() calls.  Yes, this
>is still racy, just like check_region() was.  Is that a problem
>in your machine environment?

It hasn't been when using check_region.

>Below is a patch that uses request_region/release_region pairs
>instead of check_region().

>From my perusal of that code, its all in the init category, and 
requests a total of 11 buffers of UNK size (not without rechecking 
the constant defines anyway), which would appear to be assigned for 
the duration of the uptime.  That would appear to me that its 
reserving working room it should never release as long as the drive 
is powered up.  Or did I read it wrong?

>[1] http://lkml.org/lkml/2003/8/5/304
>
>Does it work for you?
>--
>~Randy				For Linux-2.6, see:
>http://www.kernel.org/pub/linux/kernel/people/davej/misc/post-hallow
>een-2.5.txt
>
>
>patch_name:	advansys_regions_260.patch
>patch_version:	2003-08-06.10:27:17
>author:		Randy.Dunlap <rddunlap@osdl.org>
>description:	convert check_region() to request/release
>product:	Linux
>product_versions: linux-260-test2
>diffstat:	=
> drivers/scsi/advansys.c |   11 ++++++++---
> 1 files changed, 8 insertions(+), 3 deletions(-)
>
>
>diff -Naurp ./drivers/scsi/advansys.c~advans
> ./drivers/scsi/advansys.c ---
> ./drivers/scsi/advansys.c~advans	2003-07-27 09:59:39.000000000
> -0700 +++ ./drivers/scsi/advansys.c	2003-08-06 10:24:45.000000000
> -0700 @@ -4619,17 +4619,19 @@ advansys_detect(Scsi_Host_Template
> *tpnt ASC_DBG1(1,
>                                 "advansys_detect: probing I/O port
> 0x%x...\n", iop);
>-                        if (check_region(iop, ASC_IOADR_GAP) != 0)
> { +                        if (!request_region(iop, ASC_IOADR_GAP,
> "advansys")) { printk(
> "AdvanSys SCSI: specified I/O Port 0x%X is busy\n", iop);
>                             /* Don't try this I/O port twice. */
>                             asc_ioport[ioport] = 0;
>+			    release_region(iop, ASC_IOADR_GAP);
>                             goto ioport_try_again;
>                         } else if (AscFindSignature(iop) ==
> ASC_FALSE) { printk(
> "AdvanSys SCSI: specified I/O Port 0x%X has no adapter\n", iop);
>                             /* Don't try this I/O port twice. */
>                             asc_ioport[ioport] = 0;
>+			    release_region(iop, ASC_IOADR_GAP);
>                             goto ioport_try_again;
>                         } else {
>                             /*
>@@ -4647,6 +4649,7 @@ advansys_detect(Scsi_Host_Template *tpnt
>                                   * 'ioport' past this board.
>                                   */
>                                  ioport++;
>+				 release_region(iop, ASC_IOADR_GAP);
>                                  goto ioport_try_again;
>                             }
>                         }
>@@ -10001,16 +10004,18 @@ AscSearchIOPortAddr11(
>     }
>     for (; i < ASC_IOADR_TABLE_MAX_IX; i++) {
>         iop_base = _asc_def_iop_base[i];
>-        if (check_region(iop_base, ASC_IOADR_GAP) != 0) {
>+        if (!request_region(iop_base, ASC_IOADR_GAP, "advansys")) {
>             ASC_DBG1(1,
>-               "AscSearchIOPortAddr11: check_region() failed I/O
> port 0x%x\n", +               "AscSearchIOPortAddr11:
> request_region() failed I/O port 0x%x\n", iop_base);
>             continue;
>         }
>         ASC_DBG1(1, "AscSearchIOPortAddr11: probing I/O port
> 0x%x\n", iop_base); if (AscFindSignature(iop_base)) {
>+	    release_region(iop_base, ASC_IOADR_GAP);
>             return (iop_base);
>         }
>+	release_region(iop_base, ASC_IOADR_GAP);
>     }
>     return (0);
> }

Thanks, I've patched my copy, and will try it later today, submitting 
a new diff if it works.

-- 
Cheers, Gene
AMD K6-III@500mhz 320M
Athlon1600XP@1400mhz  512M
99.27% setiathome rank, not too shabby for a WV hillbilly
Yahoo.com attornies please note, additions to this message
by Gene Heskett are:
Copyright 2003 by Maurice Eugene Heskett, all rights reserved.

Re: advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Christoph Hellwig]

On Wed, Aug 06, 2003 at 10:40:41AM -0700, Randy.Dunlap wrote:
> I'm not terribly interested in spending much time on this driver,
> so the simplest thing to do IMO is to emulate check_region() with
> pairs of request_region() and release_region() calls.  Yes, this
> is still racy, just like check_region() was.  Is that a problem
> in your machine environment?
> 
> Below is a patch that uses request_region/release_region pairs
> instead of check_region().  Does it work for you?

What the heck?  Just continue using check_region if you don't want
to spend the time fixing it.  At least we get a warning in that
case.

Re: advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Gene Heskett]

On Thursday 07 August 2003 03:36, Christoph Hellwig wrote:
>On Wed, Aug 06, 2003 at 10:40:41AM -0700, Randy.Dunlap wrote:
>> I'm not terribly interested in spending much time on this driver,
>> so the simplest thing to do IMO is to emulate check_region() with
>> pairs of request_region() and release_region() calls.  Yes, this
>> is still racy, just like check_region() was.  Is that a problem
>> in your machine environment?
>>
>> Below is a patch that uses request_region/release_region pairs
>> instead of check_region().  Does it work for you?
>
>What the heck?  Just continue using check_region if you don't want
>to spend the time fixing it.  At least we get a warning in that
>case.

But I'm getting tired of repeating myself here Christoph, the driver 
as built in the 2.6.0-test2-mm2, whether built as a module, or in the 
kernel, IS BROKEN!  It gets into a bus reset loop during the init, 
doing 2 of them, AND LOCKS THE MACHINE UP TIGHT REQUIRING A HARDWARE 
RESET TO RECOVER, which of course is by rebooting back to a 2.4 
kernel.

The diff between this driver in kernel-2.4.22-pre10 and the one in 
test2-mm2 is several kilobytes, so *somebody* has been working on it, 
but they not leaving much of a trail, or even apparently adding their 
names to the roll call.

Frankly, from the lack of support by Advansys turned Connectcom turned 
now by way of a bankruptcy into Initio, and the well meaning but 
don't always work suggestions from this group, I'm about to import 
the 2.4.22-pre10 version, header and all, and see what pukes because 
the 2.2-2.4 version has worked flawlessly for quite some time, at 
least 5 or 6 years and I've worn out several tape drives with it.

I even posted a patch here that MADE IT WORK, based on Randy Dunlaps 
first suggestion, but its not been propagated to Linus that I know 
of.  I guess it wasn't a proper patch, but when I tried the next 
suggestions to "make it better", I was then 2 steps short of square 
one in that it was broken again, and this time it couldn't even find 
my tape drive and its robot in between bus resets.

You could say I'm frustrated.  Like everyone else on this list, I'm 
tired of the machine, when not doing anything else except munching on 
a seti packet, has a half a gig of ram xosview says isn't all used 
and it still takes 20 to 30 seconds to launch mozilla when running a 
2.4 kernel.  This is after all, an athlon running at 1400 mhz and 
seti is running at a nice of 20.  2.4 has turned this machine into a 
one legged sick dog, one that needs to be put out of its misery.

IE on a 1.2 ghz winderz box is up and running in 1 second flat.  I saw 
it yesterday at the neighbors.

Does anyone know what became of Bob Frey?  He was at turbolinux.com.?? 
at one time, but google finds no references < 2 years old.

-- 
Cheers, Gene
AMD K6-III@500mhz 320M
Athlon1600XP@1400mhz  512M
99.27% setiathome rank, not too shabby for a WV hillbilly
Yahoo.com attornies please note, additions to this message
by Gene Heskett are:
Copyright 2003 by Maurice Eugene Heskett, all rights reserved.

Re: advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Christoph Hellwig]

On Thu, Aug 07, 2003 at 05:33:10AM -0400, Gene Heskett wrote:
> But I'm getting tired of repeating myself here Christoph, the driver 
> as built in the 2.6.0-test2-mm2, whether built as a module, or in the 
> kernel, IS BROKEN!  It gets into a bus reset loop during the init, 
> doing 2 of them, AND LOCKS THE MACHINE UP TIGHT REQUIRING A HARDWARE 
> RESET TO RECOVER, which of course is by rebooting back to a 2.4 
> kernel.

So how do you think replacing check_region by it's opencoded version
is going to help you?

Re: advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Gene Heskett]

On Thursday 07 August 2003 05:47, Christoph Hellwig wrote:
>On Thu, Aug 07, 2003 at 05:33:10AM -0400, Gene Heskett wrote:
>> But I'm getting tired of repeating myself here Christoph, the
>> driver as built in the 2.6.0-test2-mm2, whether built as a module,
>> or in the kernel, IS BROKEN!  It gets into a bus reset loop during
>> the init, doing 2 of them, AND LOCKS THE MACHINE UP TIGHT
>> REQUIRING A HARDWARE RESET TO RECOVER, which of course is by
>> rebooting back to a 2.4 kernel.
>
>So how do you think replacing check_region by it's opencoded version
>is going to help you?

I have no idea if request_region is the proper replacement call 
Christoph.  But in tests here, it did restore what appears to be 
normal operation.

Randy Dunlop says check_region is "racy".  And I'm no kernel coder 
unless I have complete hardware specs in front of me, which I do not.
Which is why I'm "pestering" this list for help and hopefully some 
nuggets of wisdom.
 
-- 
Cheers, Gene
AMD K6-III@500mhz 320M
Athlon1600XP@1400mhz  512M
99.27% setiathome rank, not too shabby for a WV hillbilly
Yahoo.com attornies please note, additions to this message
by Gene Heskett are:
Copyright 2003 by Maurice Eugene Heskett, all rights reserved.

Re: advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Randy.Dunlap]

On Thu, 7 Aug 2003 14:34:21 -0400 Gene Heskett <gene.heskett@verizon.net> wrote:

| On Thursday 07 August 2003 05:47, Christoph Hellwig wrote:
| >On Thu, Aug 07, 2003 at 05:33:10AM -0400, Gene Heskett wrote:
| >> But I'm getting tired of repeating myself here Christoph, the
| >> driver as built in the 2.6.0-test2-mm2, whether built as a module,
| >> or in the kernel, IS BROKEN!  It gets into a bus reset loop during
| >> the init, doing 2 of them, AND LOCKS THE MACHINE UP TIGHT
| >> REQUIRING A HARDWARE RESET TO RECOVER, which of course is by
| >> rebooting back to a 2.4 kernel.
| >
| >So how do you think replacing check_region by it's opencoded version
| >is going to help you?
| 
| I have no idea if request_region is the proper replacement call 
| Christoph.  But in tests here, it did restore what appears to be 
| normal operation.
| 
| Randy Dunlop says check_region is "racy".  And I'm no kernel coder 
| unless I have complete hardware specs in front of me, which I do not.
| Which is why I'm "pestering" this list for help and hopefully some 
| nuggets of wisdom.

The real problem to be solved is why you are having problems
with the stock 2.6.0-test driver.  It uses check_region(), which
is deprecated, but that shouldn't stop it from working.
Do you have any logs of the "bus reset loop" during init?
You shouldn't have to change check_region() in the driver to
make it work.

--
~Randy				For Linux-2.6, see:
http://www.kernel.org/pub/linux/kernel/people/davej/misc/post-halloween-2.5.txt

Re: advansys regions [Re: 2.4 vs 2.6 versions of include/linux/ioport.h]

[Gene Heskett]

On Thursday 07 August 2003 14:42, Randy.Dunlap wrote:
>On Thu, 7 Aug 2003 14:34:21 -0400 Gene Heskett 
<gene.heskett@verizon.net> wrote:
>| On Thursday 07 August 2003 05:47, Christoph Hellwig wrote:
>| >On Thu, Aug 07, 2003 at 05:33:10AM -0400, Gene Heskett wrote:
>| >> But I'm getting tired of repeating myself here Christoph, the
>| >> driver as built in the 2.6.0-test2-mm2, whether built as a
>| >> module, or in the kernel, IS BROKEN!  It gets into a bus reset
>| >> loop during the init, doing 2 of them, AND LOCKS THE MACHINE UP
>| >> TIGHT REQUIRING A HARDWARE RESET TO RECOVER, which of course is
>| >> by rebooting back to a 2.4 kernel.
>| >
>| >So how do you think replacing check_region by it's opencoded
>| > version is going to help you?
>|
>| I have no idea if request_region is the proper replacement call
>| Christoph.  But in tests here, it did restore what appears to be
>| normal operation.
>|
>| Randy Dunlop says check_region is "racy".  And I'm no kernel coder
>| unless I have complete hardware specs in front of me, which I do
>| not. Which is why I'm "pestering" this list for help and hopefully
>| some nuggets of wisdom.
>
>The real problem to be solved is why you are having problems
>with the stock 2.6.0-test driver.  It uses check_region(), which
>is deprecated, but that shouldn't stop it from working.
>Do you have any logs of the "bus reset loop" during init?
>You shouldn't have to change check_region() in the driver to
>make it work.
>
>--
>~Randy				For Linux-2.6, see:
>http://www.kernel.org/pub/linux/kernel/people/davej/misc/post-hallow
>een-2.5.txt

I do have some logs,  and I note that anytime I try to boot a 2.6, I 
get this:
----
Jul 31 10:42:41 coyote kernel: No module found in object
Jul 31 10:42:47 coyote wall[1242]: wall: user root broadcasted 1 lines 
(50 chars)
Jul 31 10:42:48 coyote kernel: warning: process `update' used the 
obsolete bdflush system call
Jul 31 10:42:48 coyote kernel: Fix your initscripts?
Jul 31 10:42:53 coyote kernel: warning: process `update' used the 
obsolete bdflush system call
Jul 31 10:42:53 coyote kernel: Fix your initscripts?

And here is one from after I built the stocker as a module:
----
Jul 30 19:15:35 coyote kernel: scsi0 : AdvanSys SCSI 3.3GJ: PCI Ultra: 
IO 0xD000-0xD00F, IRQ 0xB
Jul 30 19:15:41 coyote kernel: advansys: advansys_reset: board 0: SCSI 
bus reset started...
Jul 30 19:15:41 coyote kernel: advansys: advansys_reset: board 0: SCSI 
bus reset successful.
Jul 30 19:15:52 coyote kernel: scsi: Device offlined - not ready after 
error recovery: host 0 channel 0 id 0 lun 0
Jul 30 19:15:52 coyote kernel:   Vendor: ARCHIVE   Model: Python 
28849-XXX  Rev: 4.CM
Jul 30 19:15:52 coyote kernel:   Type:   Sequential-Access                  
ANSI SCSI revision: 02
Jul 30 19:15:52 coyote kernel: Attached scsi tape st0 at scsi0, 
channel 0, id 1, lun 0
Jul 30 19:15:52 coyote kernel: st0: try direct i/o: yes, max page 
reachable by HBA 131056
Jul 30 19:15:52 coyote kernel: Attached scsi generic sg0 at scsi0, 
channel 0, id 1, lun 0,  type 1
Jul 30 19:15:52 coyote kernel:   Vendor: ARCHIVE   Model: Python 
28849-XXX  Rev: 4.CM
Jul 30 19:15:52 coyote kernel:   Type:   Medium Changer                     
ANSI SCSI revision: 02
Jul 30 19:15:52 coyote kernel: Attached scsi generic sg1 at scsi0, 
channel 0, id 1, lun 1,  type 8
Jul 30 19:16:00 coyote kernel: advansys: advansys_reset: board 0: SCSI 
bus reset started...
Jul 30 19:16:00 coyote kernel: advansys: advansys_reset: board 0: SCSI 
bus reset successful.
Jul 30 19:16:20 coyote kernel: scsi: Device offlined - not ready after 
error recovery: host 0 channel 0 id 6 lun 0
### there is no device at this---^^^^ address.  Now back to your 
### regularly scheduled program
Jul 30 19:17:28 coyote su(pam_unix)[3556]: session opened for user 
amanda by root(uid=0)
Jul 30 19:18:38 coyote login(pam_unix)[1235]: session opened for user 
root by LOGIN(uid=0)
Jul 30 19:18:38 coyote  -- root[1235]: ROOT LOGIN ON tty2
Jul 30 19:19:16 coyote kernel: advansys: asc_isr_callback: scp 
0xc045ac80 has bad host pointer, host 0x0
Jul 30 19:19:16 coyote kernel: st0: Block limits 1 - 16777215 bytes.
Jul 30 19:20:00 coyote su(pam_unix)[3556]: session closed for user 
amanda
Jul 30 19:30:31 coyote shutdown: shutting down for system reboot
=====

And here is the log from when I had put in request_region and built it 
as a module:
----
Aug  5 16:36:38 coyote kernel: scsi0 : AdvanSys SCSI 3.3GJ: PCI Ultra: 
IO 0xD000-0xD00F, IRQ 0xB
Aug  5 16:36:38 coyote kernel:   Vendor: ARCHIVE   Model: Python 
28849-XXX  Rev: 4.CM
Aug  5 16:36:38 coyote kernel:   Type:   Sequential-Access                  
ANSI SCSI revision: 02
Aug  5 16:36:38 coyote kernel: Attached scsi tape st0 at scsi0, 
channel 0, id 1, lun 0
Aug  5 16:36:38 coyote kernel: st0: try direct i/o: yes, max page 
reachable by HBA 131056
Aug  5 16:36:38 coyote kernel: Attached scsi generic sg0 at scsi0, 
channel 0, id 1, lun 0,  type 1
Aug  5 16:36:38 coyote kernel:   Vendor: ARCHIVE   Model: Python 
28849-XXX  Rev: 4.CM
Aug  5 16:36:38 coyote kernel:   Type:   Medium Changer                     
ANSI SCSI revision: 02
Aug  5 16:36:38 coyote kernel: Attached scsi generic sg1 at scsi0, 
channel 0, id 1, lun 1,  type 8
====
And here is where I had built that same module into the kernel, after 
I had updated the version strings and some comments in it:
----
Aug  5 17:30:16 coyote kernel: scsi0 : AdvanSys SCSI 3.4MGH: PCI 
Ultra: IO 0xD000-0xD00F, IRQ 0xB
Aug  5 17:30:16 coyote kernel:   Vendor: ARCHIVE   Model: Python 
28849-XXX  Rev: 4.CM
Aug  5 17:30:16 coyote kernel:   Type:   Sequential-Access                  
ANSI SCSI revision: 02
Aug  5 17:30:16 coyote kernel:   Vendor: ARCHIVE   Model: Python 
28849-XXX  Rev: 4.CM
Aug  5 17:30:16 coyote kernel:   Type:   Medium Changer                     
ANSI SCSI revision: 02
Aug  5 17:30:16 coyote kernel: st: Version 20030622, fixed bufsize 
32768, s/g segs 256
Aug  5 17:30:16 coyote kernel: Attached scsi tape st0 at scsi0, 
channel 0, id 1, lun 0
Aug  5 17:30:16 coyote kernel: st0: try direct i/o: yes, max page 
reachable by HBA 131056
Aug  5 17:30:16 coyote kernel: Attached scsi generic sg0 at scsi0, 
channel 0, id 1, lun 0,  type 1
Aug  5 17:30:16 coyote kernel: Attached scsi generic sg1 at scsi0, 
channel 0, id 1, lun 1,  type 8

After this particular boot I repeatedly exersized the amanda and mt 
related stuff trying to break it, but I could not.

That is the total of the 2.6.0-test2-mm2 logging as most of the time 
it hung the machine before it could generate a syslog entry and flush 
it to the media.  I presume there is a var in the headers I could set 
that would make it do a much more verbose init?

-- 
Cheers, Gene
AMD K6-III@500mhz 320M
Athlon1600XP@1400mhz  512M
99.27% setiathome rank, not too shabby for a WV hillbilly
Yahoo.com attornies please note, additions to this message
by Gene Heskett are:
Copyright 2003 by Maurice Eugene Heskett, all rights reserved.